[PATCH 0/3] Port git-add--interactive.perl:status_cmd to C

[PATCH 0/3] Port git-add--interactive.perl:status_cmd to C

[Daniel Ferreira]

Hm, it looks like my GSoC project won't be in the Git organization,
although I'm still interested in going for this so I guess I'll send
patches to implement my proposal anyway (although certainly in a slower
pace than I would if on the program).

This series introduces git-add-interactive--helper (or should it be
called git-add--interactive--helper?) as a builtin capable of doing
what the Perl script's status_cmd() would do.

I wish this patch series could have been smaller, although I don't
think it would make sense to bring add-interactive "subunits" smaller
than a command to the helper, and status_cmd (aside from probably
add_untracked_cmd) was the simplest one to do after all -- and still
required ~250 lines on the new builtin.

Another regret I had was not being able to retire any code from the
Perl script yet (and will likely not be able to do so until all
commands have been ported), but that is not such a big thing after
all.

As for the new builtin, I think the color handling code is pretty
straightforward. In fact, it was practically copied from places like
clean.c or diff.c (which makes me wonder if some of that code could
be generalized to avoid duplication). The same goes for the pretty
simple option parsing code.

Bigger issues seem to arise when dealing with getting the numstat.
While (as Junio anticipated on an RFC) some tasks like getting the
actual diff and splitting it may require making the "diff
machinery" write to a temporary file that we will read and do things
with, I think it would be weird to do that for parsing a simple
numstat from it. My first instinct was to create something like
show_numstat_interactive() or something on diff.c (analogous to the
other show_* functions). Doing that, however, would stumble upon
another issue: we would not be able to print both a file's diff
against the index (obtained from run_diff_index) and against the
worktree (obtained from run_diff_files) in that function. The solution
I came up with was to export the diffstat interface from diff.c into
the world and allow our new builtin to use that and build our status_cmd
output. Unless this breaks some rule of Git's API design, the result
seems pretty reasonable to me.

Travis CI build: https://travis-ci.org/theiostream/git/builds/229232202

Looking forward for feedback,
Daniel.

Daniel Ferreira (3):
  diff: export diffstat interface
  add--interactive: add builtin helper for interactive add
  add--interactive: use add-interactive--helper for status_cmd

 .gitignore                        |   1 +
 Makefile                          |   1 +
 builtin.h                         |   1 +
 builtin/add-interactive--helper.c | 258 ++++++++++++++++++++++++++++++++++++++
 diff.c                            |  17 +--
 diff.h                            |  19 ++-
 git-add--interactive.perl         |   4 +-
 git.c                             |   1 +
 8 files changed, 282 insertions(+), 20 deletions(-)
 create mode 100644 builtin/add-interactive--helper.c

--
2.7.4 (Apple Git-66)

[PATCH 1/3] diff: export diffstat interface

[Daniel Ferreira]

Make the diffstat interface (namely, the diffstat_t struct and
diff_flush_stat) no longer be internal to diff.c and allow it to be used
by other parts of git.

This is helpful for code that may want to easily extract information
from files using the diff machinery, while flushing it differently from
how the show_* functions used by diff_flush() do it.

Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>
---
 diff.c | 17 +----------------
 diff.h | 19 ++++++++++++++++++-
 2 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/diff.c b/diff.c
index 74283d9..7c073c2 100644
--- a/diff.c
+++ b/diff.c
@@ -1460,21 +1460,6 @@ static char *pprint_rename(const char *a, const char *b)
 	return strbuf_detach(&name, NULL);
 }
 
-struct diffstat_t {
-	int nr;
-	int alloc;
-	struct diffstat_file {
-		char *from_name;
-		char *name;
-		char *print_name;
-		unsigned is_unmerged:1;
-		unsigned is_binary:1;
-		unsigned is_renamed:1;
-		unsigned is_interesting:1;
-		uintmax_t added, deleted;
-	} **files;
-};
-
 static struct diffstat_file *diffstat_add(struct diffstat_t *diffstat,
 					  const char *name_a,
 					  const char *name_b)
@@ -4310,7 +4295,7 @@ static void diff_flush_patch(struct diff_filepair *p, struct diff_options *o)
 	run_diff(p, o);
 }
 
-static void diff_flush_stat(struct diff_filepair *p, struct diff_options *o,
+void diff_flush_stat(struct diff_filepair *p, struct diff_options *o,
 			    struct diffstat_t *diffstat)
 {
 	if (diff_unmodified_pair(p))
diff --git a/diff.h b/diff.h
index 5be1ee7..4cdc72d 100644
--- a/diff.h
+++ b/diff.h
@@ -188,6 +188,21 @@ struct diff_options {
 	int diff_path_counter;
 };
 
+struct diffstat_t {
+	int nr;
+	int alloc;
+	struct diffstat_file {
+		char *from_name;
+		char *name;
+		char *print_name;
+		unsigned is_unmerged:1;
+		unsigned is_binary:1;
+		unsigned is_renamed:1;
+		unsigned is_interesting:1;
+		uintmax_t added, deleted;
+	} **files;
+};
+
 enum color_diff {
 	DIFF_RESET = 0,
 	DIFF_CONTEXT = 1,
@@ -206,7 +221,6 @@ const char *diff_get_color(int diff_use_color, enum color_diff ix);
 
 const char *diff_line_prefix(struct diff_options *);
 
-
 extern const char mime_boundary_leader[];
 
 extern struct combine_diff_path *diff_tree_paths(
@@ -262,6 +276,9 @@ extern void diff_change(struct diff_options *,
 
 extern struct diff_filepair *diff_unmerge(struct diff_options *, const char *path);
 
+void diff_flush_stat(struct diff_filepair *p, struct diff_options *o,
+			    struct diffstat_t *diffstat);
+
 #define DIFF_SETUP_REVERSE      	1
 #define DIFF_SETUP_USE_CACHE		2
 #define DIFF_SETUP_USE_SIZE_CACHE	4
-- 
2.7.4 (Apple Git-66)

Re: [PATCH 1/3] diff: export diffstat interface

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 853 bytes --]

Hi Daniel,

On Fri, 5 May 2017, Daniel Ferreira wrote:

> Make the diffstat interface (namely, the diffstat_t struct and
> diff_flush_stat) no longer be internal to diff.c and allow it to be used
> by other parts of git.
> 
> This is helpful for code that may want to easily extract information
> from files using the diff machinery, while flushing it differently from
> how the show_* functions used by diff_flush() do it.
> 
> Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>

Maybe add one more paragraph with the motivation, i.e. the conversion of
add -i into a builtin?

The patch is straight-forward, and I do not even see any names that may
need to be adjusted for a more visible API than a file-local function.

> -- 
> 2.7.4 (Apple Git-66)

I guess I take my comment about Ævar's Git version back ;-)

Ciao,
Johannes

[PATCH 2/3] add--interactive: add builtin helper for interactive add

[Daniel Ferreira]

Create a builtin helper for git-add--interactive, which right now is
only able to reproduce git-add--interactive.perl's status_cmd()
function, providing a summarized diff numstat to the user.

This is the first step in an effort to convert git-add--interactive.perl
to a C builtin, in search for better portability, expressibility and
performance (specially on non-POSIX systems like Windows).

Additionally, an eventual complete port of git-add--interactive would
remove the last "big" Git script to have Perl as a dependency, allowing
most Git users to have a NOPERL build running without big losses.

Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>
---
 .gitignore                        |   1 +
 Makefile                          |   1 +
 builtin.h                         |   1 +
 builtin/add-interactive--helper.c | 258 ++++++++++++++++++++++++++++++++++++++
 git.c                             |   1 +
 5 files changed, 262 insertions(+)
 create mode 100644 builtin/add-interactive--helper.c

diff --git a/.gitignore b/.gitignore
index 833ef3b..0d6cfe4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -11,6 +11,7 @@
 /git
 /git-add
 /git-add--interactive
+/git-add-interactive--helper
 /git-am
 /git-annotate
 /git-apply
diff --git a/Makefile b/Makefile
index e35542e..842fce2 100644
--- a/Makefile
+++ b/Makefile
@@ -873,6 +873,7 @@ LIB_OBJS += xdiff-interface.o
 LIB_OBJS += zlib.o
 
 BUILTIN_OBJS += builtin/add.o
+BUILTIN_OBJS += builtin/add-interactive--helper.o
 BUILTIN_OBJS += builtin/am.o
 BUILTIN_OBJS += builtin/annotate.o
 BUILTIN_OBJS += builtin/apply.o
diff --git a/builtin.h b/builtin.h
index 9e4a898..3d6a0ab 100644
--- a/builtin.h
+++ b/builtin.h
@@ -30,6 +30,7 @@ extern int textconv_object(const char *path, unsigned mode, const struct object_
 extern int is_builtin(const char *s);
 
 extern int cmd_add(int argc, const char **argv, const char *prefix);
+extern int cmd_add_interactive__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_am(int argc, const char **argv, const char *prefix);
 extern int cmd_annotate(int argc, const char **argv, const char *prefix);
 extern int cmd_apply(int argc, const char **argv, const char *prefix);
diff --git a/builtin/add-interactive--helper.c b/builtin/add-interactive--helper.c
new file mode 100644
index 0000000..97ca1b3
--- /dev/null
+++ b/builtin/add-interactive--helper.c
@@ -0,0 +1,258 @@
+#include "builtin.h"
+#include "color.h"
+#include "diff.h"
+#include "diffcore.h"
+#include "revision.h"
+
+#define ADD_INTERACTIVE_HEADER_INDENT "      "
+
+enum add_interactive_collection_mode {
+	COLLECTION_MODE_NONE,
+	COLLECTION_MODE_WORKTREE,
+	COLLECTION_MODE_INDEX
+};
+
+struct add_interactive_file_status {
+	int selected;
+
+	char path[PATH_MAX];
+
+	int lines_added_index;
+	int lines_deleted_index;
+	int lines_added_worktree;
+	int lines_deleted_worktree;
+};
+
+struct add_interactive_status {
+	enum add_interactive_collection_mode current_mode;
+
+	const char *reference;
+	struct pathspec pathspec;
+
+	int file_count;
+	struct add_interactive_file_status *files;
+};
+
+static int add_interactive_use_color = -1;
+enum color_add_interactive {
+	ADD_INTERACTIVE_PROMPT,
+	ADD_INTERACTIVE_HEADER,
+	ADD_INTERACTIVE_HELP,
+	ADD_INTERACTIVE_ERROR
+};
+
+static char add_interactive_colors[][COLOR_MAXLEN] = {
+	GIT_COLOR_BOLD_BLUE, /* Prompt */
+	GIT_COLOR_BOLD,      /* Header */
+	GIT_COLOR_BOLD_RED,  /* Help */
+	GIT_COLOR_BOLD_RED   /* Error */
+};
+
+static const char *add_interactive_get_color(enum color_add_interactive ix)
+{
+	if (want_color(add_interactive_use_color))
+		return add_interactive_colors[ix];
+	return "";
+}
+
+static int parse_add_interactive_color_slot(const char *slot)
+{
+	if (!strcasecmp(slot, "prompt"))
+		return ADD_INTERACTIVE_PROMPT;
+	if (!strcasecmp(slot, "header"))
+		return ADD_INTERACTIVE_HEADER;
+	if (!strcasecmp(slot, "help"))
+		return ADD_INTERACTIVE_HELP;
+	if (!strcasecmp(slot, "error"))
+		return ADD_INTERACTIVE_ERROR;
+
+	return -1;
+}
+
+static int git_add_interactive_config(const char *var,
+		const char *value, void *cbdata)
+{
+	const char *name;
+
+	if (!strcmp(var, "color.interactive")) {
+		add_interactive_use_color = git_config_colorbool(var, value);
+		return 0;
+	}
+
+	if (skip_prefix(var, "color.interactive", &name)) {
+		int slot = parse_add_interactive_color_slot(name);
+		if (slot < 0)
+			return 0;
+		if (!value)
+			return config_error_nonbool(var);
+		return color_parse(value, add_interactive_colors[slot]);
+	}
+
+	return git_default_config(var, value, cbdata);
+}
+
+static void add_interactive_status_collect_changed_cb(struct diff_queue_struct *q,
+					 struct diff_options *options,
+					 void *data)
+{
+	struct add_interactive_status *s = data;
+	struct diffstat_t stat;
+	int i, j;
+
+	if (!q->nr)
+		return;
+
+	memset(&stat, 0, sizeof(struct diffstat_t));
+	for (i = 0; i < q->nr; i++) {
+		struct diff_filepair *p;
+		p = q->queue[i];
+		diff_flush_stat(p, options, &stat);
+	}
+
+	for (i = 0; i < stat.nr; i++) {
+		int file_index = s->file_count;
+		for (j = 0; j < s->file_count; j++) {
+			if (!strcmp(s->files[j].path, stat.files[i]->name)) {
+				file_index = j;
+				break;
+			}
+		}
+
+		if (file_index == s->file_count) {
+			s->file_count++;
+			s->files = realloc(s->files,
+					(q->nr + s->file_count) * sizeof(*s->files));
+			memset(&s->files[file_index], 0,
+					sizeof(struct add_interactive_file_status));
+		}
+
+		memcpy(s->files[file_index].path, stat.files[i]->name,
+				strlen(stat.files[i]->name) + 1);
+		if (s->current_mode == COLLECTION_MODE_WORKTREE) {
+			s->files[file_index].lines_added_worktree = stat.files[i]->added;
+			s->files[file_index].lines_deleted_worktree = stat.files[i]->deleted;
+		} else if (s->current_mode == COLLECTION_MODE_INDEX) {
+			s->files[file_index].lines_added_index = stat.files[i]->added;
+			s->files[file_index].lines_deleted_index = stat.files[i]->deleted;
+		}
+	}
+}
+
+static void add_interactive_status_collect_changes_worktree(struct add_interactive_status *s)
+{
+	struct rev_info rev;
+
+	s->current_mode = COLLECTION_MODE_WORKTREE;
+
+	init_revisions(&rev, NULL);
+	setup_revisions(0, NULL, &rev, NULL);
+
+	rev.max_count = 0;
+
+	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
+	rev.diffopt.format_callback = add_interactive_status_collect_changed_cb;
+	rev.diffopt.format_callback_data = s;
+
+	run_diff_files(&rev, 0);
+}
+
+static void add_interactive_status_collect_changes_index(struct add_interactive_status *s)
+{
+	struct rev_info rev;
+	struct setup_revision_opt opt;
+
+	s->current_mode = COLLECTION_MODE_INDEX;
+
+	init_revisions(&rev, NULL);
+	memset(&opt, 0, sizeof(opt));
+	opt.def = s->reference;
+	setup_revisions(0, NULL, &rev, &opt);
+
+	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
+	rev.diffopt.format_callback = add_interactive_status_collect_changed_cb;
+	rev.diffopt.format_callback_data = s;
+
+	run_diff_index(&rev, 1);
+}
+
+static void list_modified_into_status(struct add_interactive_status *s)
+{
+	add_interactive_status_collect_changes_worktree(s);
+	add_interactive_status_collect_changes_index(s);
+}
+
+static void print_modified(void)
+{
+	int i;
+	struct add_interactive_status s;
+	const char *modified_fmt = _("%12s %12s %s");
+	const char *header_color = add_interactive_get_color(ADD_INTERACTIVE_HEADER);
+	unsigned char sha1[20];
+
+	if (read_cache() < 0)
+		return;
+
+	s.current_mode = COLLECTION_MODE_NONE;
+	s.reference = !get_sha1("HEAD", sha1) ? "HEAD": EMPTY_TREE_SHA1_HEX;
+	s.file_count = 0;
+	s.files = NULL;
+	list_modified_into_status(&s);
+
+	printf(ADD_INTERACTIVE_HEADER_INDENT);
+	color_fprintf(stdout, header_color, modified_fmt, _("staged"),
+			_("unstaged"), _("path"));
+	printf("\n");
+
+	for (i = 0; i < s.file_count; i++) {
+		struct add_interactive_file_status f = s.files[i];
+		char selection = f.selected ? '*' : ' ';
+
+		char worktree_changes[50];
+		char index_changes[50];
+
+		if (f.lines_added_worktree != 0 || f.lines_deleted_worktree != 0)
+			snprintf(worktree_changes, 50, "+%d/-%d", f.lines_added_worktree,
+					f.lines_deleted_worktree);
+		else
+			snprintf(worktree_changes, 50, "%s", _("nothing"));
+
+		if (f.lines_added_index != 0 || f.lines_deleted_index != 0)
+			snprintf(index_changes, 50, "+%d/-%d", f.lines_added_index,
+					f.lines_deleted_index);
+		else
+			snprintf(index_changes, 50, "%s", _("unchanged"));
+
+		printf("%c%2d: ", selection, i + 1);
+		printf(modified_fmt, index_changes, worktree_changes, f.path);
+		printf("\n");
+	}
+}
+
+static void status_cmd(void)
+{
+	print_modified();
+}
+
+static const char add_interactive_helper_usage[] =
+"git add-interactive--helper <command>";
+
+int cmd_add_interactive__helper(int argc, const char **argv, const char *prefix)
+{
+	int i, found_opt = 0;
+
+	git_config(git_add_interactive_config, NULL);
+
+	for (i = 1; i < argc; i++) {
+		const char *arg = argv[i];
+
+		if (!strcmp(arg, "--status")) {
+			status_cmd();
+			found_opt = 1;
+		}
+	}
+
+	if (!found_opt)
+		usage(add_interactive_helper_usage);
+
+	return 0;
+}
diff --git a/git.c b/git.c
index 8ff44f0..796971e 100644
--- a/git.c
+++ b/git.c
@@ -391,6 +391,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 
 static struct cmd_struct commands[] = {
 	{ "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
+	{ "add-interactive--helper", cmd_add_interactive__helper, RUN_SETUP | NEED_WORK_TREE },
 	{ "am", cmd_am, RUN_SETUP | NEED_WORK_TREE },
 	{ "annotate", cmd_annotate, RUN_SETUP },
 	{ "apply", cmd_apply, RUN_SETUP_GENTLY },
-- 
2.7.4 (Apple Git-66)

Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add

[Ævar Arnfjörð Bjarmason]

On Fri, May 5, 2017 at 8:43 PM, Daniel Ferreira <bnmvco@gmail.com> wrote:
> Create a builtin helper for git-add--interactive, which right now is
> only able to reproduce git-add--interactive.perl's status_cmd()
> function, providing a summarized diff numstat to the user.

I'm not a user of add -i and didn't review the main logic in detail,
but I ran this, and aside from two issues this LGTM:

 * You missed a trailing \n in the output, so your formatting is
   different from the current behavior.

 * You should be using the getopt API instead of rolling your own.

Fixes for both below, thanks a lot for hacking on this. As you pointed
out this doesn't help with removing any of the perl code for now, but
after replacing a few more command modes we can start chopping away at
the perl code.

diff --git a/builtin/add-interactive--helper.c b/builtin/add-interactive--helper.c
index 97ca1b38dc..ea0f790bd3 100644
--- a/builtin/add-interactive--helper.c
+++ b/builtin/add-interactive--helper.c
@@ -226,6 +226,7 @@ static void print_modified(void)
 		printf(modified_fmt, index_changes, worktree_changes, f.path);
 		printf("\n");
 	}
+	printf("\n");
 }
 
 static void status_cmd(void)
@@ -233,26 +234,31 @@ static void status_cmd(void)
 	print_modified();
 }
 
-static const char add_interactive_helper_usage[] =
-"git add-interactive--helper <command>";
+static const char * const builtin_add_interactive_helper_usage[] = {
+	N_("git add-interactive--helper <command>"),
+	NULL
+};
 
 int cmd_add_interactive__helper(int argc, const char **argv, const char *prefix)
 {
-	int i, found_opt = 0;
-
-	git_config(git_add_interactive_config, NULL);
+	int opt_status = 0;
 
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
+	struct option options[] = {
+		OPT_BOOL(0, "status", &opt_status,
+			 N_("print status information with diffstat")),
+		OPT_END()
+	};
 
-		if (!strcmp(arg, "--status")) {
-			status_cmd();
-			found_opt = 1;
-		}
-	}
-
-	if (!found_opt)
-		usage(add_interactive_helper_usage);
+	git_config(git_add_interactive_config, NULL);
+	argc = parse_options(argc, argv, NULL, options,
+			     builtin_add_interactive_helper_usage,
+			     PARSE_OPT_KEEP_ARGV0);
+
+	if (opt_status)
+		status_cmd();
+	else
+		usage_with_options(builtin_add_interactive_helper_usage,
+				   options);
 
 	return 0;
 }
-- 
2.11.0

Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 601 bytes --]

Hi,

On Fri, 5 May 2017, Ævar Arnfjörð Bjarmason wrote:

>  int cmd_add_interactive__helper(int argc, const char **argv, const char *prefix)
>  {
> -	int i, found_opt = 0;
> -
> -	git_config(git_add_interactive_config, NULL);
> +	int opt_status = 0;
>  
> -	for (i = 1; i < argc; i++) {
> -		const char *arg = argv[i];
> +	struct option options[] = {
> +		OPT_BOOL(0, "status", &opt_status,
> +			 N_("print status information with diffstat")),

Please use OPT_CMDMODE() instead; it was invented exactly for this kind of
scenario.

> -- 
> 2.11.0

Really? ;-)

Ciao,
Dscho

Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add

[Ævar Arnfjörð Bjarmason]

On Fri, May 5, 2017 at 11:21 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Fri, 5 May 2017, Ævar Arnfjörð Bjarmason wrote:
>
>>  int cmd_add_interactive__helper(int argc, const char **argv, const char *prefix)
>>  {
>> -     int i, found_opt = 0;
>> -
>> -     git_config(git_add_interactive_config, NULL);
>> +     int opt_status = 0;
>>
>> -     for (i = 1; i < argc; i++) {
>> -             const char *arg = argv[i];
>> +     struct option options[] = {
>> +             OPT_BOOL(0, "status", &opt_status,
>> +                      N_("print status information with diffstat")),
>
> Please use OPT_CMDMODE() instead; it was invented exactly for this kind of
> scenario.

Indeed. Forgot about that. That's much better.

>> --
>> 2.11.0
>
> Really? ;-)

CC-ing the Debian stretch release team with your complaint [not really].

I don't bother to 'make install' the git I'm hacking on, so E-Mail
gets sent with whatever's in Debian testing.

Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 13538 bytes --]

Hi Daniel,


On Fri, 5 May 2017, Daniel Ferreira wrote:

> Create a builtin helper for git-add--interactive, which right now is
> only able to reproduce git-add--interactive.perl's status_cmd()
> function, providing a summarized diff numstat to the user.
> 
> This is the first step in an effort to convert git-add--interactive.perl
> to a C builtin, in search for better portability, expressibility and
> performance (specially on non-POSIX systems like Windows).
> 
> Additionally, an eventual complete port of git-add--interactive would
> remove the last "big" Git script to have Perl as a dependency, allowing
> most Git users to have a NOPERL build running without big losses.
> 
> Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>

Good. I would think that git-add--helper is a better name. It is an
internal command only, anyway, and hopefully it will go away soon (i.e.
hopefully all of add -i will be builtin soon and can then be even moved
into a separate file outside builtin/, say, add-interactive.c).

> diff --git a/.gitignore b/.gitignore
> index 833ef3b..0d6cfe4 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -11,6 +11,7 @@
>  /git
>  /git-add
>  /git-add--interactive
> +/git-add-interactive--helper

Personally, I would prefer a separate commit adding the helper, before it
learns about any functionality. Somehow I have an easier time reviewing
patches if they tease such things apart.

> diff --git a/builtin/add-interactive--helper.c b/builtin/add-interactive--helper.c
> new file mode 100644
> index 0000000..97ca1b3
> --- /dev/null
> +++ b/builtin/add-interactive--helper.c
> @@ -0,0 +1,258 @@
> +#include "builtin.h"
> +#include "color.h"
> +#include "diff.h"
> +#include "diffcore.h"
> +#include "revision.h"
> +
> +#define ADD_INTERACTIVE_HEADER_INDENT "      "

This definition is local to this helper, and we already know that this is
all about add -i. So maybe not repeat it? HEADER_INDENT would be catchier
and just as expressive.

> +enum add_interactive_collection_mode {
> +	COLLECTION_MODE_NONE,
> +	COLLECTION_MODE_WORKTREE,
> +	COLLECTION_MODE_INDEX
> +};

Likewise, maybe we do not want to have this COLLECTION_MODE_ prefix
littering the source code... NONE, WORKTREE and INDEX should be short and
sweet.

> +struct add_interactive_file_status {

Just struct file_status, maybe?

From a cursory glance, it also would appear that the only need for this
struct to be named is the memset() call when initializing a new item,
right? If that is the case, it would be better to use sizeof(*s->files)
and keep it unnamed, as part of the enclosing struct.

> +	int selected;
> +
> +	char path[PATH_MAX];

That is a bit wasteful. `char *name` is what diffstat_t uses...

> +	int lines_added_index;
> +	int lines_deleted_index;
> +	int lines_added_worktree;
> +	int lines_deleted_worktree;

Maybe for better readability:

	struct {
		uintmax_t added, deleted;
	} index, worktree;

> +};
>
> +struct add_interactive_status {
> +	enum add_interactive_collection_mode current_mode;

I am scared of future patches where you cannot wrap the code to the
required <= 80 columns/row because the data type or variable name is too
long... ;-)

> +
> +	const char *reference;
> +	struct pathspec pathspec;
> +
> +	int file_count;
> +	struct add_interactive_file_status *files;

The convention for growable arrays in Git is to have nr, alloc and the
array, and use ALLOC_GROW() to avoid reallocating for every single new
item.

> +};
> +
> +static int add_interactive_use_color = -1;
> +enum color_add_interactive {
> +	ADD_INTERACTIVE_PROMPT,
> +	ADD_INTERACTIVE_HEADER,
> +	ADD_INTERACTIVE_HELP,
> +	ADD_INTERACTIVE_ERROR
> +};

Again, if we were talking about a declaration in a .h file, that prefix
would make sense, but here we are safely in file-local territory where I
think this is just unnecessary churn.

> +static char add_interactive_colors[][COLOR_MAXLEN] = {
> +	GIT_COLOR_BOLD_BLUE, /* Prompt */
> +	GIT_COLOR_BOLD,      /* Header */
> +	GIT_COLOR_BOLD_RED,  /* Help */
> +	GIT_COLOR_BOLD_RED   /* Error */
> +};
> +
> +static const char *add_interactive_get_color(enum color_add_interactive ix)
> +{
> +	if (want_color(add_interactive_use_color))
> +		return add_interactive_colors[ix];
> +	return "";
> +}
> +
> +static int parse_add_interactive_color_slot(const char *slot)
> +{
> +	if (!strcasecmp(slot, "prompt"))
> +		return ADD_INTERACTIVE_PROMPT;
> +	if (!strcasecmp(slot, "header"))
> +		return ADD_INTERACTIVE_HEADER;
> +	if (!strcasecmp(slot, "help"))
> +		return ADD_INTERACTIVE_HELP;
> +	if (!strcasecmp(slot, "error"))
> +		return ADD_INTERACTIVE_ERROR;
> +
> +	return -1;
> +}
> +
> +static int git_add_interactive_config(const char *var,

Not git_add_interactive__helper_config()? ;-)

> +		const char *value, void *cbdata)
> +{
> +	const char *name;
> +
> +	if (!strcmp(var, "color.interactive")) {
> +		add_interactive_use_color = git_config_colorbool(var, value);
> +		return 0;
> +	}
> +
> +	if (skip_prefix(var, "color.interactive", &name)) {
> +		int slot = parse_add_interactive_color_slot(name);
> +		if (slot < 0)
> +			return 0;
> +		if (!value)
> +			return config_error_nonbool(var);
> +		return color_parse(value, add_interactive_colors[slot]);
> +	}
> +
> +	return git_default_config(var, value, cbdata);
> +}
> +
> +static void add_interactive_status_collect_changed_cb(struct diff_queue_struct *q,

I did have that fear, and now it comes true... ;-)

Seriously again, this is a file-local function. We know it is about
add --interactive, and we can be pretty succinct about its role, i.e.
something like collect_changes_callback() would be plenty sufficient.

> +					 struct diff_options *options,
> +					 void *data)
> +{
> +	struct add_interactive_status *s = data;
> +	struct diffstat_t stat;
> +	int i, j;
> +
> +	if (!q->nr)
> +		return;
> +
> +	memset(&stat, 0, sizeof(struct diffstat_t));

I guess it makes sense to play it safe here (by zeroing the entire struct
rather than assigning the individual initial values).

But would

	struct diffstat_t stat = { 0 };

not have done the same?

> +	for (i = 0; i < q->nr; i++) {
> +		struct diff_filepair *p;
> +		p = q->queue[i];
> +		diff_flush_stat(p, options, &stat);
> +	}
> +
> +	for (i = 0; i < stat.nr; i++) {
> +		int file_index = s->file_count;
> +		for (j = 0; j < s->file_count; j++) {
> +			if (!strcmp(s->files[j].path, stat.files[i]->name)) {
> +				file_index = j;
> +				break;
> +			}
> +		}

So basically, this is looking up in a list whether we saw the file in
question already, and the reason we have to do that is that we run the
entire shebang twice, once with the worktree and once with the index.

I wonder whether it would not make sense to switch away s->files from a
list to a hashmap.

> +		if (file_index == s->file_count) {
> +			s->file_count++;
> +			s->files = realloc(s->files,
> +					(q->nr + s->file_count) * sizeof(*s->files));

We already have ALLOC_GROW() to do this, safer. And easier to verify ;-)

> +			memset(&s->files[file_index], 0,
> +					sizeof(struct add_interactive_file_status));
> +		}
> +
> +		memcpy(s->files[file_index].path, stat.files[i]->name,
> +				strlen(stat.files[i]->name) + 1);

Apart from using PATH_MAX bytes for most likely only short names: should
this not be inside the if (file_index == s->file_count) clause? I mean, if
we do not hit that conditional code, it is because we already compared the
file name and figured out that s->files[file_index].path is identical to
stat.files[i]->name...


> +		if (s->current_mode == COLLECTION_MODE_WORKTREE) {
> +			s->files[file_index].lines_added_worktree = stat.files[i]->added;
> +			s->files[file_index].lines_deleted_worktree = stat.files[i]->deleted;
> +		} else if (s->current_mode == COLLECTION_MODE_INDEX) {
> +			s->files[file_index].lines_added_index = stat.files[i]->added;
> +			s->files[file_index].lines_deleted_index = stat.files[i]->deleted;
> +		}
> +	}
> +}
> +
> +static void add_interactive_status_collect_changes_worktree(struct add_interactive_status *s)

I would use the name status_worktree() here instead.

> +{
> +	struct rev_info rev;
> +
> +	s->current_mode = COLLECTION_MODE_WORKTREE;

Now that I read this and remember that only WORKTREE and INDEX are handled
in the callback function: is there actually a use for the NONE enum value?
I.e. is current_mode read out in any other context than the callback
function? If there is no other read, then the NONE enum value is just
confusing.

BTW in the first pass, we pretty much know that we only get unique names,
so the entire lookup is unnecessary and will just increase the time
complexity from O(n) to O(n^2). So let's avoid that.

By moving to a hashmap, you can even get the second phase down to an
expected O(n).

> +
> +	init_revisions(&rev, NULL);
> +	setup_revisions(0, NULL, &rev, NULL);
> +
> +	rev.max_count = 0;

Where does this come from? We are not logging commits... oh wait, I see
that diff-lib.c reuses that field for its diff_unmerged_stage value. Urgh.
Not your fault, of course.

> +	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
> +	rev.diffopt.format_callback = add_interactive_status_collect_changed_cb;
> +	rev.diffopt.format_callback_data = s;
> +
> +	run_diff_files(&rev, 0);
> +}
> +
> +static void add_interactive_status_collect_changes_index(struct add_interactive_status *s)
> +{
> +	struct rev_info rev;
> +	struct setup_revision_opt opt;
> +
> +	s->current_mode = COLLECTION_MODE_INDEX;
> +
> +	init_revisions(&rev, NULL);
> +	memset(&opt, 0, sizeof(opt));

	struct setup_revision_opt opt = { NULL };

> +	opt.def = s->reference;
> +	setup_revisions(0, NULL, &rev, &opt);

Oh, I see, you want to use the opt argument to pass HEAD or the emtpy tree
SHA-1 if we're on an unborn branch.

Since you're already calling get_sha1() on "HEAD", you may just as well
keep the SHA-1 (and use the EMPTY_TREE_SHA1 if there is no HEAD), and then
pass that in via the argc, argv parameters to setup_revisions() (imitating
cmd_diff_index() a bit closer).

> +
> +	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
> +	rev.diffopt.format_callback = add_interactive_status_collect_changed_cb;
> +	rev.diffopt.format_callback_data = s;
> +
> +	run_diff_index(&rev, 1);
> +}
> +
> +static void list_modified_into_status(struct add_interactive_status *s)
> +{
> +	add_interactive_status_collect_changes_worktree(s);
> +	add_interactive_status_collect_changes_index(s);
> +}

Why not collapse all three functions into one? It is not like they are
totally unrelated nor super-long.

> +static void print_modified(void)
> +{
> +	int i;
> +	struct add_interactive_status s;
> +	const char *modified_fmt = _("%12s %12s %s");

We cannot really translate that...

> +	const char *header_color = add_interactive_get_color(ADD_INTERACTIVE_HEADER);
> +	unsigned char sha1[20];
> +
> +	if (read_cache() < 0)
> +		return;
> +
> +	s.current_mode = COLLECTION_MODE_NONE;
> +	s.reference = !get_sha1("HEAD", sha1) ? "HEAD": EMPTY_TREE_SHA1_HEX;
> +	s.file_count = 0;
> +	s.files = NULL;
> +	list_modified_into_status(&s);
> +
> +	printf(ADD_INTERACTIVE_HEADER_INDENT);
> +	color_fprintf(stdout, header_color, modified_fmt, _("staged"),
> +			_("unstaged"), _("path"));

I think these _() need to become N_().

> +	printf("\n");

Wouldn't it be better to avoid defining modified_fmt (we do not really
modify it, do we?) and at least make the '\n' part of the format string?

> +	for (i = 0; i < s.file_count; i++) {
> +		struct add_interactive_file_status f = s.files[i];
> +		char selection = f.selected ? '*' : ' ';

I would prefer the variable to be called "marker".

> +
> +		char worktree_changes[50];
> +		char index_changes[50];

Those "50" look a bit arbitrary... maybe use strbufs instead (so that we
do not have to hope that all translations of "nothing" or "unchanged",
even Welsh ones, will fit inside those buffers)?

> +		if (f.lines_added_worktree != 0 || f.lines_deleted_worktree != 0)

In Git's source code, the convention is to just drop the `!= 0` in
comparisons, unless there is a really good reason not to.

> +			snprintf(worktree_changes, 50, "+%d/-%d", f.lines_added_worktree,
> +					f.lines_deleted_worktree);
> +		else
> +			snprintf(worktree_changes, 50, "%s", _("nothing"));
> +
> +		if (f.lines_added_index != 0 || f.lines_deleted_index != 0)
> +			snprintf(index_changes, 50, "+%d/-%d", f.lines_added_index,
> +					f.lines_deleted_index);
> +		else
> +			snprintf(index_changes, 50, "%s", _("unchanged"));
> +
> +		printf("%c%2d: ", selection, i + 1);
> +		printf(modified_fmt, index_changes, worktree_changes, f.path);
> +		printf("\n");

It would be nicer to make this a single printf() call. The only idea I
have to allow for that is to turn modified_fmt into a `#define
MODIFIED_FMT "%12s %12s %s"`, though.

> +	}
> +}
> +
> +static void status_cmd(void)
> +{
> +	print_modified();
> +}

As long as this function really only calls another function with no
parameters, let's just drop it. We can call print_modified() instead of
status_cmd() just as easily.

Ævar already commented on the parseopt stuff.

Otherwise this looks pretty good to me!

Thanks,
Johannes

Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add

[Ævar Arnfjörð Bjarmason]

On Sat, May 6, 2017 at 12:30 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Daniel,
>
>
> On Fri, 5 May 2017, Daniel Ferreira wrote:
>> +static void print_modified(void)
>> +{
>> +     int i;
>> +     struct add_interactive_status s;
>> +     const char *modified_fmt = _("%12s %12s %s");
>
> We cannot really translate that...

He copied this from the *.perl code:

    # TRANSLATORS: you can adjust this to align "git add -i" status menu
    my $status_fmt = __('%12s %12s %s');

And one translation at least makes use of that (and probably others should).

But porting the /* TRANSLATORS: ... */ comment over is missing, and
should be added.

Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 952 bytes --]

Hi,

On Sat, 6 May 2017, Ævar Arnfjörð Bjarmason wrote:

> On Sat, May 6, 2017 at 12:30 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi Daniel,
> >
> >
> > On Fri, 5 May 2017, Daniel Ferreira wrote:
> >> +static void print_modified(void)
> >> +{
> >> +     int i;
> >> +     struct add_interactive_status s;
> >> +     const char *modified_fmt = _("%12s %12s %s");
> >
> > We cannot really translate that...
> 
> He copied this from the *.perl code:
> 
>     # TRANSLATORS: you can adjust this to align "git add -i" status menu
>     my $status_fmt = __('%12s %12s %s');
> 
> And one translation at least makes use of that (and probably others should).
> 
> But porting the /* TRANSLATORS: ... */ comment over is missing, and
> should be added.

Ah, that explains it. It still is not really translateable, but with the
comment, even *I* understand why it is marked for translation.

Thanks,
Dscho

Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add

[Daniel Ferreira (theiostream)]

On Fri, May 5, 2017 at 7:30 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>> +static int git_add_interactive_config(const char *var,
>
> Not git_add_interactive__helper_config()? ;-)

I don't get if you mean this ironically (because of the verbosity) or
if you do think this would be a good name ;P

>> +     for (i = 0; i < q->nr; i++) {
>> +             struct diff_filepair *p;
>> +             p = q->queue[i];
>> +             diff_flush_stat(p, options, &stat);
>> +     }
>> +
>> +     for (i = 0; i < stat.nr; i++) {
>> +             int file_index = s->file_count;
>> +             for (j = 0; j < s->file_count; j++) {
>> +                     if (!strcmp(s->files[j].path, stat.files[i]->name)) {
>> +                             file_index = j;
>> +                             break;
>> +                     }
>> +             }
>
> So basically, this is looking up in a list whether we saw the file in
> question already, and the reason we have to do that is that we run the
> entire shebang twice, once with the worktree and once with the index.
>
> I wonder whether it would not make sense to switch away s->files from a
> list to a hashmap.
> [...]
> BTW in the first pass, we pretty much know that we only get unique names,
> so the entire lookup is unnecessary and will just increase the time
> complexity from O(n) to O(n^2). So let's avoid that.
>
> By moving to a hashmap, you can even get the second phase down to an
> expected O(n).

How would you go about implementing that hashmap (i.e. what should be
the hash)? Does Git have any interface for it, or is there any example
I can look after in the codebase?

> Apart from using PATH_MAX bytes for most likely only short names: [...]

If not PATH_MAX, what should I go for? Make it a strbuf? I tend to
believe keeping that on the stack would be simpler and more optimal.

> Now that I read this and remember that only WORKTREE and INDEX are handled
> in the callback function: is there actually a use for the NONE enum value?
> I.e. is current_mode read out in any other context than the callback
> function? If there is no other read, then the NONE enum value is just
> confusing.

I just preferred to have a declared non-handled value than leave
something undefined behind. I felt it might avoid headaches in the
future with petty segfaults.

> Why not collapse all three functions into one? It is not like they are
> totally unrelated nor super-long.

To me it is a matter of personal preference to keep them separate. If
there is, however, any technical or project-style-related reason to
get them together, I'll certainly do it.

>> +static void print_modified(void)
>> +{
>> +     int i;
>> +     struct add_interactive_status s;
>> +     const char *modified_fmt = _("%12s %12s %s");
>
> We cannot really translate that...

Apparently, we can. Ævar covered that in his reply.

>> +     printf(ADD_INTERACTIVE_HEADER_INDENT);
>> +     color_fprintf(stdout, header_color, modified_fmt, _("staged"),
>> +                     _("unstaged"), _("path"));
>
> I think these _() need to become N_().

I cannot find any call to N_() outside of Perl code. What should that
even do differently?

>> +static void status_cmd(void)
>> +{
>> +     print_modified();
>> +}
>
> As long as this function really only calls another function with no
> parameters, let's just drop it. We can call print_modified() instead of
> status_cmd() just as easily.

I thought calling status_cmd() would make that more clear, but I agree
-- the options already make it clear enough,

I agree with all points I did not directly address. And thank you for
the review :)

-- Daniel.

Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add

[Ævar Arnfjörð Bjarmason]

On Sat, May 6, 2017 at 1:13 AM, Daniel Ferreira (theiostream)
<bnmvco@gmail.com> wrote:
> On Fri, May 5, 2017 at 7:30 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>>> +static int git_add_interactive_config(const char *var,
>>
>> Not git_add_interactive__helper_config()? ;-)
>
> I don't get if you mean this ironically (because of the verbosity) or
> if you do think this would be a good name ;P
>
>>> +     for (i = 0; i < q->nr; i++) {
>>> +             struct diff_filepair *p;
>>> +             p = q->queue[i];
>>> +             diff_flush_stat(p, options, &stat);
>>> +     }
>>> +
>>> +     for (i = 0; i < stat.nr; i++) {
>>> +             int file_index = s->file_count;
>>> +             for (j = 0; j < s->file_count; j++) {
>>> +                     if (!strcmp(s->files[j].path, stat.files[i]->name)) {
>>> +                             file_index = j;
>>> +                             break;
>>> +                     }
>>> +             }
>>
>> So basically, this is looking up in a list whether we saw the file in
>> question already, and the reason we have to do that is that we run the
>> entire shebang twice, once with the worktree and once with the index.
>>
>> I wonder whether it would not make sense to switch away s->files from a
>> list to a hashmap.
>> [...]
>> BTW in the first pass, we pretty much know that we only get unique names,
>> so the entire lookup is unnecessary and will just increase the time
>> complexity from O(n) to O(n^2). So let's avoid that.
>>
>> By moving to a hashmap, you can even get the second phase down to an
>> expected O(n).
>
> How would you go about implementing that hashmap (i.e. what should be
> the hash)? Does Git have any interface for it, or is there any example
> I can look after in the codebase?

Git has a hashmap API already. 7d4558c462 is a good example of using it.

>>> +     printf(ADD_INTERACTIVE_HEADER_INDENT);
>>> +     color_fprintf(stdout, header_color, modified_fmt, _("staged"),
>>> +                     _("unstaged"), _("path"));
>>
>> I think these _() need to become N_().
>
> I cannot find any call to N_() outside of Perl code. What should that
> even do differently?

N_() is to mark a string for later translation, not to return it. See
my "how to use getopt" patch for an example of that, but this doesn't
look like such a case, since _() is returning the string to
color_fprintf, surely...

    $ git grep -P '\bN_\(' -- '*.c'

Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 5292 bytes --]

Hi Daniel,

On Fri, 5 May 2017, Daniel Ferreira (theiostream) wrote:

> On Fri, May 5, 2017 at 7:30 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >> +static int git_add_interactive_config(const char *var,
> >
> > Not git_add_interactive__helper_config()? ;-)
> 
> I don't get if you mean this ironically (because of the verbosity) or
> if you do think this would be a good name ;P

Hehe. I meant it tongue-in-cheek.

So let me try again in my endeavor to provide *constructive* criticism,
i.e. not only pointing out what I think is suboptimal, but *also* how to
improve it in my opinion. How about add_i_config() or git_add_config()?

> >> +     for (i = 0; i < q->nr; i++) {
> >> +             struct diff_filepair *p;
> >> +             p = q->queue[i];
> >> +             diff_flush_stat(p, options, &stat);
> >> +     }
> >> +
> >> +     for (i = 0; i < stat.nr; i++) {
> >> +             int file_index = s->file_count;
> >> +             for (j = 0; j < s->file_count; j++) {
> >> +                     if (!strcmp(s->files[j].path, stat.files[i]->name)) {
> >> +                             file_index = j;
> >> +                             break;
> >> +                     }
> >> +             }
> >
> > So basically, this is looking up in a list whether we saw the file in
> > question already, and the reason we have to do that is that we run the
> > entire shebang twice, once with the worktree and once with the index.
> >
> > I wonder whether it would not make sense to switch away s->files from a
> > list to a hashmap.
> > [...]
> > BTW in the first pass, we pretty much know that we only get unique names,
> > so the entire lookup is unnecessary and will just increase the time
> > complexity from O(n) to O(n^2). So let's avoid that.
> >
> > By moving to a hashmap, you can even get the second phase down to an
> > expected O(n).
> 
> How would you go about implementing that hashmap (i.e. what should be
> the hash)? Does Git have any interface for it, or is there any example
> I can look after in the codebase?

The example Ævar pointed to seems to be pretty good (Michael Haggerty's
commits are in general excellent examples to follow):

	https://github.com/git-for-windows/git/commit/7d4558c462f0

In this case, we can even fold the added/deleted part into the struct:

	#include "hashmap.h"

	...

	struct file_stats {
		struct hashmap_entry entry;
		struct {
			uintmax_t added, deleted;
		} index, worktree;
		char name[FLEX_ARRAY];
	}

	...
		for (i = 0; i < stat.nr; i++) {
			struct file_stats *stats;
			const char *name = stat.files[i]->name;
			unsigned int hash = strhash(name);

			stats = s->phase == INDEX ? NULL :
				hashmap_get_from_hash(&map, hash, name);
			if (!stats) {
				FLEX_ALLOC_STR(stats, name, name);
				hashmap_entry_init(stats, hash);
				stats->index.added = stats->index.deleted = 0;
				stats->worktree.added =
					stats->worktree.deleted = 0;
				hashmap_add(&map, stats);
			}

			if (s->phase == INDEX) {
				stats->index.added = stat.files[i]->added;
				stats->index.deleted = stat.files[i]->deleted;
			} else {
				stats->worktree.added = stat.files[i]->added;
				stats->worktree.deleted = stat.files[i]->deleted;
			}
		}

But maybe it should simultaneously put those added entries into a growing
array, as they arrive sorted and we will want to output the entries
sorted, too.

Oh, this reminds me: you are reading the list in two phases, and each time
the entries arrive sorted, but the second time we still may append new
entries.

Maybe we need to sort the entries afterwards?

> > Apart from using PATH_MAX bytes for most likely only short names: [...]
> 
> If not PATH_MAX, what should I go for? Make it a strbuf? I tend to
> believe keeping that on the stack would be simpler and more optimal.

On the stack, no question. But I was talking about struct
add_interactive_file_status, which is allocated via malloc(), not
allocated on the stack/heap.

> > Now that I read this and remember that only WORKTREE and INDEX are
> > handled in the callback function: is there actually a use for the NONE
> > enum value?  I.e. is current_mode read out in any other context than
> > the callback function? If there is no other read, then the NONE enum
> > value is just confusing.
> 
> I just preferred to have a declared non-handled value than leave
> something undefined behind. I felt it might avoid headaches in the
> future with petty segfaults.

I found it a little harder to read, as I was puzzled about the NONE
value...

> > Why not collapse all three functions into one? It is not like they are
> > totally unrelated nor super-long.
> 
> To me it is a matter of personal preference to keep them separate. If
> there is, however, any technical or project-style-related reason to get
> them together, I'll certainly do it.

I was looking at it from a readability point of view. These functions
became very small, and the operation (which in my mind was one logical
two-phase operation) became too scattered to follow easily. This may be a
limitation of my brain, but I would have had an easier time if those three
functions were one function.

Ciao,
Johannes

[PATCH 3/3] add--interactive: use add-interactive--helper for status_cmd

[Daniel Ferreira]

Call the newly introduced git-add-interactive--helper builtin on
status_cmd() instead of relying on git-add--interactive's Perl
functions to build print the numstat.

Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>
---
 git-add--interactive.perl | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 709a5f6..8617462 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -632,9 +632,7 @@ EOF
 }
 
 sub status_cmd {
-	list_and_choose({ LIST_ONLY => 1, HEADER => $status_head },
-			list_modified());
-	print "\n";
+	system(qw(git add-interactive--helper --status));
 }
 
 sub say_n_paths {
-- 
2.7.4 (Apple Git-66)

Re: [PATCH 3/3] add--interactive: use add-interactive--helper for status_cmd

[Johannes Schindelin]

Hi Daniel,


On Fri, 5 May 2017, Daniel Ferreira wrote:

> Call the newly introduced git-add-interactive--helper builtin on
> status_cmd() instead of relying on git-add--interactive's Perl
> functions to build print the numstat.
> 
> Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>
> ---
>  git-add--interactive.perl | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 709a5f6..8617462 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -632,9 +632,7 @@ EOF
>  }
>  
>  sub status_cmd {
> -	list_and_choose({ LIST_ONLY => 1, HEADER => $status_head },
> -			list_modified());
> -	print "\n";
> +	system(qw(git add-interactive--helper --status));
>  }
>  
>  sub say_n_paths {

Obviously good!

Thanks,
Johannes

Re: [PATCH 0/3] Port git-add--interactive.perl:status_cmd to C

[Ævar Arnfjörð Bjarmason]

On Fri, May 5, 2017 at 8:43 PM, Daniel Ferreira <bnmvco@gmail.com> wrote:
> This series introduces git-add-interactive--helper (or should it be
> called git-add--interactive--helper?) as a builtin capable of doing
> what the Perl script's status_cmd() would do.

The existing script is git-add--interactive.perl, so
git-add--interactive--helper.c would be consistent with that, since
there's no git-add-interactive command.

Re: [PATCH 0/3] Port git-add--interactive.perl:status_cmd to C

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 690 bytes --]

Hi,

On Fri, 5 May 2017, Ævar Arnfjörð Bjarmason wrote:

> On Fri, May 5, 2017 at 8:43 PM, Daniel Ferreira <bnmvco@gmail.com> wrote:
> > This series introduces git-add-interactive--helper (or should it be
> > called git-add--interactive--helper?) as a builtin capable of doing
> > what the Perl script's status_cmd() would do.
> 
> The existing script is git-add--interactive.perl, so
> git-add--interactive--helper.c would be consistent with that, since
> there's no git-add-interactive command.

Actually, to be consistent with the existing git-rebase--helper example
(which is also only helping the interactive version), it should be
git-add--helper.

Ciao,
Johannes

Re: [PATCH 0/3] Port git-add--interactive.perl:status_cmd to C

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:
> On Fri, May 5, 2017 at 8:43 PM, Daniel Ferreira <bnmvco@gmail.com> wrote:

>> This series introduces git-add-interactive--helper (or should it be
>> called git-add--interactive--helper?) as a builtin capable of doing
>> what the Perl script's status_cmd() would do.
>
> The existing script is git-add--interactive.perl, so
> git-add--interactive--helper.c would be consistent with that, since
> there's no git-add-interactive command.

Or git-add--interactive-helper.c, or git-add--helper.c.

The important thing is that there's a double-dash somewhere.

Thanks,
Jonathan

Re: [PATCH 0/3] Port git-add--interactive.perl:status_cmd to C

[Johannes Schindelin]

Hi Daniel,

On Fri, 5 May 2017, Daniel Ferreira wrote:

> Hm, it looks like my GSoC project won't be in the Git organization,

Yeah, rumors have it that you were quite a popular student and several
organizations fought bloody wars over getting you... ;-)

> although I'm still interested in going for this so I guess I'll send
> patches to implement my proposal anyway (although certainly in a slower
> pace than I would if on the program).

Nice!

> This series introduces git-add-interactive--helper (or should it be
> called git-add--interactive--helper?) as a builtin capable of doing
> what the Perl script's status_cmd() would do.

Since it is a temporary thing, and not user-visible, I would opt for
git-add--helper. I may have mentioned that a couple of times now... ;-)

> I wish this patch series could have been smaller, although I don't
> think it would make sense to bring add-interactive "subunits" smaller
> than a command to the helper, and status_cmd (aside from probably
> add_untracked_cmd) was the simplest one to do after all -- and still
> required ~250 lines on the new builtin.

I think it's fine. There has been a patch series consisting of ~80
individual patches before, as long as you do not go close to that one, I
think you are fine.

> Another regret I had was not being able to retire any code from the Perl
> script yet (and will likely not be able to do so until all commands have
> been ported), but that is not such a big thing after all.

All in due time.

But maybe you want to keep the naming a little more consistent with the
Perl script, e.g. instead of calling the function `print_modified()` call
it already `list()` (and rename it later to `list_and_choose()` once you
have taught it to ask for a choice)?

> As for the new builtin, I think the color handling code is pretty
> straightforward. In fact, it was practically copied from places like
> clean.c or diff.c (which makes me wonder if some of that code could
> be generalized to avoid duplication). The same goes for the pretty
> simple option parsing code.

True. It does look awfully similar to other code handling colors.

Maybe put it on the backlog. Or the GSoC get-your-feet-wet projects for
next year.

> Bigger issues seem to arise when dealing with getting the numstat.
> While (as Junio anticipated on an RFC) some tasks like getting the
> actual diff and splitting it may require making the "diff
> machinery" write to a temporary file that we will read and do things
> with, I think it would be weird to do that for parsing a simple
> numstat from it.

I would actually think that the callback Junio talked about would be more
appropriate than to write temporary files. Even to split the diff. We can
do that much more efficiently in-memory.

> My first instinct was to create something like
> show_numstat_interactive() or something on diff.c (analogous to the
> other show_* functions). Doing that, however, would stumble upon another
> issue: we would not be able to print both a file's diff against the
> index (obtained from run_diff_index) and against the worktree (obtained
> from run_diff_files) in that function. The solution I came up with was
> to export the diffstat interface from diff.c into the world and allow
> our new builtin to use that and build our status_cmd output. Unless this
> breaks some rule of Git's API design, the result seems pretty reasonable
> to me.

It looks pretty reasonable to me, too.

Thank you for this pleasant read!
Johannes

Re: [PATCH 0/3] Port git-add--interactive.perl:status_cmd to C

[Daniel Ferreira (theiostream)]

On Fri, May 5, 2017 at 7:38 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> But maybe you want to keep the naming a little more consistent with the
> Perl script, e.g. instead of calling the function `print_modified()` call
> it already `list()` (and rename it later to `list_and_choose()` once you
> have taught it to ask for a choice)?

Actually, I named it print_modified() because I anticipated there
would be no list_and_choose() equivalent in C. I don't think the
builtin should have the interactive menu + modified list + untracked
list being handled by one function. Rather, I think a saner way to go
on with it would be to create functions like print_untracked();
choose_from_input(); print_menu() etc.

This is still pure speculation from what I've written until now and
from the roadmap I have in my head (I do not know how writing code
from now on will actually be), but I have a hunch that
list_and_choose() is already convoluted enough in Perl; in C it might
become a monster.

> Thank you for this pleasant read!

Thank *you* for the quick and thorough review :)

-- Daniel.

Re: [PATCH 0/3] Port git-add--interactive.perl:status_cmd to C

[Johannes Schindelin]

Hi Daniel,

On Fri, 5 May 2017, Daniel Ferreira (theiostream) wrote:

> On Fri, May 5, 2017 at 7:38 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > But maybe you want to keep the naming a little more consistent with
> > the Perl script, e.g. instead of calling the function
> > `print_modified()` call it already `list()` (and rename it later to
> > `list_and_choose()` once you have taught it to ask for a choice)?
> 
> Actually, I named it print_modified() because I anticipated there
> would be no list_and_choose() equivalent in C. I don't think the
> builtin should have the interactive menu + modified list + untracked
> list being handled by one function. Rather, I think a saner way to go
> on with it would be to create functions like print_untracked();
> choose_from_input(); print_menu() etc.

Okay. But maybe then the `selected` field should not (at least not yet) be
a part of this patch.

> This is still pure speculation from what I've written until now and from
> the roadmap I have in my head (I do not know how writing code from now
> on will actually be), but I have a hunch that list_and_choose() is
> already convoluted enough in Perl; in C it might become a monster.

True.

To be honest, I would love for this patch to become part of a
"work-in-progress" patch series that lays out the plan a little bit more
concretely (I typically implement functions with a single `die("TODO")` in
the function body in such patch series). This patch series would of course
not be expected to pass the test suite yet, but it would allow other
developers (e.g. myself) to jump in and complete individual patches...

There would be real advantages in such a patch series:

- it could be worked on in parallel (coordination required, of course);
  may be a ton of fun, and

- the overall design could be iterated according with the needs of later
  patches in the series.

> > Thank you for this pleasant read!
> 
> Thank *you* for the quick and thorough review :)

Heh... I would not call it "quick"... it took a long time. But not as much
time as you spent crafting it!

Thank you,
Johannes