{"thread":{"id":"45881","subject":"[PATCH 1/3] diff: export diffstat interface","startedAt":"2017-05-05T18:44:13Z","lastAt":"2017-05-08T12:23:53Z","messageCount":21,"participants":["Daniel Ferreira","Ævar Arnfjörð Bjarmason","Johannes Schindelin","Jonathan Nieder","Daniel Ferreira (theiostream)"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"318875","messageId":"1494009820-2090-2-git-send-email-bnmvco@gmail.com","threadId":"45881","inReplyTo":"1494009820-2090-1-git-send-email-bnmvco@gmail.com","subject":"[PATCH 1/3] diff: export diffstat interface","fromName":"Daniel Ferreira","fromEmail":"bnmvco@gmail.com","sentAt":"2017-05-05T18:43:38Z","receivedAt":"2017-05-05T18:44:13Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"Make the diffstat interface (namely, the diffstat_t struct and\ndiff_flush_stat) no longer be internal to diff.c and allow it to be used\nby other parts of git.\n\nThis is helpful for code that may want to easily extract information\nfrom files using the diff machinery, while flushing it differently from\nhow the show_* functions used by diff_flush() do it.\n\nSigned-off-by: Daniel Ferreira <bnmvco@gmail.com>\n---\n diff.c | 17 +----------------\n diff.h | 19 ++++++++++++++++++-\n 2 files changed, 19 insertions(+), 17 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 74283d9..7c073c2 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1460,21 +1460,6 @@ static char *pprint_rename(const char *a, const char *b)\n \treturn strbuf_detach(&name, NULL);\n }\n \n-struct diffstat_t {\n-\tint nr;\n-\tint alloc;\n-\tstruct diffstat_file {\n-\t\tchar *from_name;\n-\t\tchar *name;\n-\t\tchar *print_name;\n-\t\tunsigned is_unmerged:1;\n-\t\tunsigned is_binary:1;\n-\t\tunsigned is_renamed:1;\n-\t\tunsigned is_interesting:1;\n-\t\tuintmax_t added, deleted;\n-\t} **files;\n-};\n-\n static struct diffstat_file *diffstat_add(struct diffstat_t *diffstat,\n \t\t\t\t\t  const char *name_a,\n \t\t\t\t\t  const char *name_b)\n@@ -4310,7 +4295,7 @@ static void diff_flush_patch(struct diff_filepair *p, struct diff_options *o)\n \trun_diff(p, o);\n }\n \n-static void diff_flush_stat(struct diff_filepair *p, struct diff_options *o,\n+void diff_flush_stat(struct diff_filepair *p, struct diff_options *o,\n \t\t\t    struct diffstat_t *diffstat)\n {\n \tif (diff_unmodified_pair(p))\ndiff --git a/diff.h b/diff.h\nindex 5be1ee7..4cdc72d 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -188,6 +188,21 @@ struct diff_options {\n \tint diff_path_counter;\n };\n \n+struct diffstat_t {\n+\tint nr;\n+\tint alloc;\n+\tstruct diffstat_file {\n+\t\tchar *from_name;\n+\t\tchar *name;\n+\t\tchar *print_name;\n+\t\tunsigned is_unmerged:1;\n+\t\tunsigned is_binary:1;\n+\t\tunsigned is_renamed:1;\n+\t\tunsigned is_interesting:1;\n+\t\tuintmax_t added, deleted;\n+\t} **files;\n+};\n+\n enum color_diff {\n \tDIFF_RESET = 0,\n \tDIFF_CONTEXT = 1,\n@@ -206,7 +221,6 @@ const char *diff_get_color(int diff_use_color, enum color_diff ix);\n \n const char *diff_line_prefix(struct diff_options *);\n \n-\n extern const char mime_boundary_leader[];\n \n extern struct combine_diff_path *diff_tree_paths(\n@@ -262,6 +276,9 @@ extern void diff_change(struct diff_options *,\n \n extern struct diff_filepair *diff_unmerge(struct diff_options *, const char *path);\n \n+void diff_flush_stat(struct diff_filepair *p, struct diff_options *o,\n+\t\t\t    struct diffstat_t *diffstat);\n+\n #define DIFF_SETUP_REVERSE      \t1\n #define DIFF_SETUP_USE_CACHE\t\t2\n #define DIFF_SETUP_USE_SIZE_CACHE\t4\n-- \n2.7.4 (Apple Git-66)\n\n"},{"id":"318876","messageId":"1494009820-2090-3-git-send-email-bnmvco@gmail.com","threadId":"45881","inReplyTo":"1494009820-2090-1-git-send-email-bnmvco@gmail.com","subject":"[PATCH 2/3] add--interactive: add builtin helper for interactive add","fromName":"Daniel Ferreira","fromEmail":"bnmvco@gmail.com","sentAt":"2017-05-05T18:43:39Z","receivedAt":"2017-05-05T18:44:15Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"Create a builtin helper for git-add--interactive, which right now is\nonly able to reproduce git-add--interactive.perl's status_cmd()\nfunction, providing a summarized diff numstat to the user.\n\nThis is the first step in an effort to convert git-add--interactive.perl\nto a C builtin, in search for better portability, expressibility and\nperformance (specially on non-POSIX systems like Windows).\n\nAdditionally, an eventual complete port of git-add--interactive would\nremove the last \"big\" Git script to have Perl as a dependency, allowing\nmost Git users to have a NOPERL build running without big losses.\n\nSigned-off-by: Daniel Ferreira <bnmvco@gmail.com>\n---\n .gitignore                        |   1 +\n Makefile                          |   1 +\n builtin.h                         |   1 +\n builtin/add-interactive--helper.c | 258 ++++++++++++++++++++++++++++++++++++++\n git.c                             |   1 +\n 5 files changed, 262 insertions(+)\n create mode 100644 builtin/add-interactive--helper.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 833ef3b..0d6cfe4 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -11,6 +11,7 @@\n /git\n /git-add\n /git-add--interactive\n+/git-add-interactive--helper\n /git-am\n /git-annotate\n /git-apply\ndiff --git a/Makefile b/Makefile\nindex e35542e..842fce2 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -873,6 +873,7 @@ LIB_OBJS += xdiff-interface.o\n LIB_OBJS += zlib.o\n \n BUILTIN_OBJS += builtin/add.o\n+BUILTIN_OBJS += builtin/add-interactive--helper.o\n BUILTIN_OBJS += builtin/am.o\n BUILTIN_OBJS += builtin/annotate.o\n BUILTIN_OBJS += builtin/apply.o\ndiff --git a/builtin.h b/builtin.h\nindex 9e4a898..3d6a0ab 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -30,6 +30,7 @@ extern int textconv_object(const char *path, unsigned mode, const struct object_\n extern int is_builtin(const char *s);\n \n extern int cmd_add(int argc, const char **argv, const char *prefix);\n+extern int cmd_add_interactive__helper(int argc, const char **argv, const char *prefix);\n extern int cmd_am(int argc, const char **argv, const char *prefix);\n extern int cmd_annotate(int argc, const char **argv, const char *prefix);\n extern int cmd_apply(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/add-interactive--helper.c b/builtin/add-interactive--helper.c\nnew file mode 100644\nindex 0000000..97ca1b3\n--- /dev/null\n+++ b/builtin/add-interactive--helper.c\n@@ -0,0 +1,258 @@\n+#include \"builtin.h\"\n+#include \"color.h\"\n+#include \"diff.h\"\n+#include \"diffcore.h\"\n+#include \"revision.h\"\n+\n+#define ADD_INTERACTIVE_HEADER_INDENT \"      \"\n+\n+enum add_interactive_collection_mode {\n+\tCOLLECTION_MODE_NONE,\n+\tCOLLECTION_MODE_WORKTREE,\n+\tCOLLECTION_MODE_INDEX\n+};\n+\n+struct add_interactive_file_status {\n+\tint selected;\n+\n+\tchar path[PATH_MAX];\n+\n+\tint lines_added_index;\n+\tint lines_deleted_index;\n+\tint lines_added_worktree;\n+\tint lines_deleted_worktree;\n+};\n+\n+struct add_interactive_status {\n+\tenum add_interactive_collection_mode current_mode;\n+\n+\tconst char *reference;\n+\tstruct pathspec pathspec;\n+\n+\tint file_count;\n+\tstruct add_interactive_file_status *files;\n+};\n+\n+static int add_interactive_use_color = -1;\n+enum color_add_interactive {\n+\tADD_INTERACTIVE_PROMPT,\n+\tADD_INTERACTIVE_HEADER,\n+\tADD_INTERACTIVE_HELP,\n+\tADD_INTERACTIVE_ERROR\n+};\n+\n+static char add_interactive_colors[][COLOR_MAXLEN] = {\n+\tGIT_COLOR_BOLD_BLUE, /* Prompt */\n+\tGIT_COLOR_BOLD,      /* Header */\n+\tGIT_COLOR_BOLD_RED,  /* Help */\n+\tGIT_COLOR_BOLD_RED   /* Error */\n+};\n+\n+static const char *add_interactive_get_color(enum color_add_interactive ix)\n+{\n+\tif (want_color(add_interactive_use_color))\n+\t\treturn add_interactive_colors[ix];\n+\treturn \"\";\n+}\n+\n+static int parse_add_interactive_color_slot(const char *slot)\n+{\n+\tif (!strcasecmp(slot, \"prompt\"))\n+\t\treturn ADD_INTERACTIVE_PROMPT;\n+\tif (!strcasecmp(slot, \"header\"))\n+\t\treturn ADD_INTERACTIVE_HEADER;\n+\tif (!strcasecmp(slot, \"help\"))\n+\t\treturn ADD_INTERACTIVE_HELP;\n+\tif (!strcasecmp(slot, \"error\"))\n+\t\treturn ADD_INTERACTIVE_ERROR;\n+\n+\treturn -1;\n+}\n+\n+static int git_add_interactive_config(const char *var,\n+\t\tconst char *value, void *cbdata)\n+{\n+\tconst char *name;\n+\n+\tif (!strcmp(var, \"color.interactive\")) {\n+\t\tadd_interactive_use_color = git_config_colorbool(var, value);\n+\t\treturn 0;\n+\t}\n+\n+\tif (skip_prefix(var, \"color.interactive\", &name)) {\n+\t\tint slot = parse_add_interactive_color_slot(name);\n+\t\tif (slot < 0)\n+\t\t\treturn 0;\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n+\t\treturn color_parse(value, add_interactive_colors[slot]);\n+\t}\n+\n+\treturn git_default_config(var, value, cbdata);\n+}\n+\n+static void add_interactive_status_collect_changed_cb(struct diff_queue_struct *q,\n+\t\t\t\t\t struct diff_options *options,\n+\t\t\t\t\t void *data)\n+{\n+\tstruct add_interactive_status *s = data;\n+\tstruct diffstat_t stat;\n+\tint i, j;\n+\n+\tif (!q->nr)\n+\t\treturn;\n+\n+\tmemset(&stat, 0, sizeof(struct diffstat_t));\n+\tfor (i = 0; i < q->nr; i++) {\n+\t\tstruct diff_filepair *p;\n+\t\tp = q->queue[i];\n+\t\tdiff_flush_stat(p, options, &stat);\n+\t}\n+\n+\tfor (i = 0; i < stat.nr; i++) {\n+\t\tint file_index = s->file_count;\n+\t\tfor (j = 0; j < s->file_count; j++) {\n+\t\t\tif (!strcmp(s->files[j].path, stat.files[i]->name)) {\n+\t\t\t\tfile_index = j;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (file_index == s->file_count) {\n+\t\t\ts->file_count++;\n+\t\t\ts->files = realloc(s->files,\n+\t\t\t\t\t(q->nr + s->file_count) * sizeof(*s->files));\n+\t\t\tmemset(&s->files[file_index], 0,\n+\t\t\t\t\tsizeof(struct add_interactive_file_status));\n+\t\t}\n+\n+\t\tmemcpy(s->files[file_index].path, stat.files[i]->name,\n+\t\t\t\tstrlen(stat.files[i]->name) + 1);\n+\t\tif (s->current_mode == COLLECTION_MODE_WORKTREE) {\n+\t\t\ts->files[file_index].lines_added_worktree = stat.files[i]->added;\n+\t\t\ts->files[file_index].lines_deleted_worktree = stat.files[i]->deleted;\n+\t\t} else if (s->current_mode == COLLECTION_MODE_INDEX) {\n+\t\t\ts->files[file_index].lines_added_index = stat.files[i]->added;\n+\t\t\ts->files[file_index].lines_deleted_index = stat.files[i]->deleted;\n+\t\t}\n+\t}\n+}\n+\n+static void add_interactive_status_collect_changes_worktree(struct add_interactive_status *s)\n+{\n+\tstruct rev_info rev;\n+\n+\ts->current_mode = COLLECTION_MODE_WORKTREE;\n+\n+\tinit_revisions(&rev, NULL);\n+\tsetup_revisions(0, NULL, &rev, NULL);\n+\n+\trev.max_count = 0;\n+\n+\trev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n+\trev.diffopt.format_callback = add_interactive_status_collect_changed_cb;\n+\trev.diffopt.format_callback_data = s;\n+\n+\trun_diff_files(&rev, 0);\n+}\n+\n+static void add_interactive_status_collect_changes_index(struct add_interactive_status *s)\n+{\n+\tstruct rev_info rev;\n+\tstruct setup_revision_opt opt;\n+\n+\ts->current_mode = COLLECTION_MODE_INDEX;\n+\n+\tinit_revisions(&rev, NULL);\n+\tmemset(&opt, 0, sizeof(opt));\n+\topt.def = s->reference;\n+\tsetup_revisions(0, NULL, &rev, &opt);\n+\n+\trev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n+\trev.diffopt.format_callback = add_interactive_status_collect_changed_cb;\n+\trev.diffopt.format_callback_data = s;\n+\n+\trun_diff_index(&rev, 1);\n+}\n+\n+static void list_modified_into_status(struct add_interactive_status *s)\n+{\n+\tadd_interactive_status_collect_changes_worktree(s);\n+\tadd_interactive_status_collect_changes_index(s);\n+}\n+\n+static void print_modified(void)\n+{\n+\tint i;\n+\tstruct add_interactive_status s;\n+\tconst char *modified_fmt = _(\"%12s %12s %s\");\n+\tconst char *header_color = add_interactive_get_color(ADD_INTERACTIVE_HEADER);\n+\tunsigned char sha1[20];\n+\n+\tif (read_cache() < 0)\n+\t\treturn;\n+\n+\ts.current_mode = COLLECTION_MODE_NONE;\n+\ts.reference = !get_sha1(\"HEAD\", sha1) ? \"HEAD\": EMPTY_TREE_SHA1_HEX;\n+\ts.file_count = 0;\n+\ts.files = NULL;\n+\tlist_modified_into_status(&s);\n+\n+\tprintf(ADD_INTERACTIVE_HEADER_INDENT);\n+\tcolor_fprintf(stdout, header_color, modified_fmt, _(\"staged\"),\n+\t\t\t_(\"unstaged\"), _(\"path\"));\n+\tprintf(\"\\n\");\n+\n+\tfor (i = 0; i < s.file_count; i++) {\n+\t\tstruct add_interactive_file_status f = s.files[i];\n+\t\tchar selection = f.selected ? '*' : ' ';\n+\n+\t\tchar worktree_changes[50];\n+\t\tchar index_changes[50];\n+\n+\t\tif (f.lines_added_worktree != 0 || f.lines_deleted_worktree != 0)\n+\t\t\tsnprintf(worktree_changes, 50, \"+%d/-%d\", f.lines_added_worktree,\n+\t\t\t\t\tf.lines_deleted_worktree);\n+\t\telse\n+\t\t\tsnprintf(worktree_changes, 50, \"%s\", _(\"nothing\"));\n+\n+\t\tif (f.lines_added_index != 0 || f.lines_deleted_index != 0)\n+\t\t\tsnprintf(index_changes, 50, \"+%d/-%d\", f.lines_added_index,\n+\t\t\t\t\tf.lines_deleted_index);\n+\t\telse\n+\t\t\tsnprintf(index_changes, 50, \"%s\", _(\"unchanged\"));\n+\n+\t\tprintf(\"%c%2d: \", selection, i + 1);\n+\t\tprintf(modified_fmt, index_changes, worktree_changes, f.path);\n+\t\tprintf(\"\\n\");\n+\t}\n+}\n+\n+static void status_cmd(void)\n+{\n+\tprint_modified();\n+}\n+\n+static const char add_interactive_helper_usage[] =\n+\"git add-interactive--helper <command>\";\n+\n+int cmd_add_interactive__helper(int argc, const char **argv, const char *prefix)\n+{\n+\tint i, found_opt = 0;\n+\n+\tgit_config(git_add_interactive_config, NULL);\n+\n+\tfor (i = 1; i < argc; i++) {\n+\t\tconst char *arg = argv[i];\n+\n+\t\tif (!strcmp(arg, \"--status\")) {\n+\t\t\tstatus_cmd();\n+\t\t\tfound_opt = 1;\n+\t\t}\n+\t}\n+\n+\tif (!found_opt)\n+\t\tusage(add_interactive_helper_usage);\n+\n+\treturn 0;\n+}\ndiff --git a/git.c b/git.c\nindex 8ff44f0..796971e 100644\n--- a/git.c\n+++ b/git.c\n@@ -391,6 +391,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \n static struct cmd_struct commands[] = {\n \t{ \"add\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n+\t{ \"add-interactive--helper\", cmd_add_interactive__helper, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"am\", cmd_am, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"annotate\", cmd_annotate, RUN_SETUP },\n \t{ \"apply\", cmd_apply, RUN_SETUP_GENTLY },\n-- \n2.7.4 (Apple Git-66)\n\n"},{"id":"318877","messageId":"1494009820-2090-1-git-send-email-bnmvco@gmail.com","threadId":"45881","inReplyTo":null,"subject":"[PATCH 0/3] Port git-add--interactive.perl:status_cmd to C","fromName":"Daniel Ferreira","fromEmail":"bnmvco@gmail.com","sentAt":"2017-05-05T18:43:37Z","receivedAt":"2017-05-05T18:44:17Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"Hm, it looks like my GSoC project won't be in the Git organization,\nalthough I'm still interested in going for this so I guess I'll send\npatches to implement my proposal anyway (although certainly in a slower\npace than I would if on the program).\n\nThis series introduces git-add-interactive--helper (or should it be\ncalled git-add--interactive--helper?) as a builtin capable of doing\nwhat the Perl script's status_cmd() would do.\n\nI wish this patch series could have been smaller, although I don't\nthink it would make sense to bring add-interactive \"subunits\" smaller\nthan a command to the helper, and status_cmd (aside from probably\nadd_untracked_cmd) was the simplest one to do after all -- and still\nrequired ~250 lines on the new builtin.\n\nAnother regret I had was not being able to retire any code from the\nPerl script yet (and will likely not be able to do so until all\ncommands have been ported), but that is not such a big thing after\nall.\n\nAs for the new builtin, I think the color handling code is pretty\nstraightforward. In fact, it was practically copied from places like\nclean.c or diff.c (which makes me wonder if some of that code could\nbe generalized to avoid duplication). The same goes for the pretty\nsimple option parsing code.\n\nBigger issues seem to arise when dealing with getting the numstat.\nWhile (as Junio anticipated on an RFC) some tasks like getting the\nactual diff and splitting it may require making the \"diff\nmachinery\" write to a temporary file that we will read and do things\nwith, I think it would be weird to do that for parsing a simple\nnumstat from it. My first instinct was to create something like\nshow_numstat_interactive() or something on diff.c (analogous to the\nother show_* functions). Doing that, however, would stumble upon\nanother issue: we would not be able to print both a file's diff\nagainst the index (obtained from run_diff_index) and against the\nworktree (obtained from run_diff_files) in that function. The solution\nI came up with was to export the diffstat interface from diff.c into\nthe world and allow our new builtin to use that and build our status_cmd\noutput. Unless this breaks some rule of Git's API design, the result\nseems pretty reasonable to me.\n\nTravis CI build: https://travis-ci.org/theiostream/git/builds/229232202\n\nLooking forward for feedback,\nDaniel.\n\nDaniel Ferreira (3):\n  diff: export diffstat interface\n  add--interactive: add builtin helper for interactive add\n  add--interactive: use add-interactive--helper for status_cmd\n\n .gitignore                        |   1 +\n Makefile                          |   1 +\n builtin.h                         |   1 +\n builtin/add-interactive--helper.c | 258 ++++++++++++++++++++++++++++++++++++++\n diff.c                            |  17 +--\n diff.h                            |  19 ++-\n git-add--interactive.perl         |   4 +-\n git.c                             |   1 +\n 8 files changed, 282 insertions(+), 20 deletions(-)\n create mode 100644 builtin/add-interactive--helper.c\n\n--\n2.7.4 (Apple Git-66)\n\n"},{"id":"318878","messageId":"1494009820-2090-4-git-send-email-bnmvco@gmail.com","threadId":"45881","inReplyTo":"1494009820-2090-1-git-send-email-bnmvco@gmail.com","subject":"[PATCH 3/3] add--interactive: use add-interactive--helper for status_cmd","fromName":"Daniel Ferreira","fromEmail":"bnmvco@gmail.com","sentAt":"2017-05-05T18:43:40Z","receivedAt":"2017-05-05T18:44:18Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"Call the newly introduced git-add-interactive--helper builtin on\nstatus_cmd() instead of relying on git-add--interactive's Perl\nfunctions to build print the numstat.\n\nSigned-off-by: Daniel Ferreira <bnmvco@gmail.com>\n---\n git-add--interactive.perl | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 709a5f6..8617462 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -632,9 +632,7 @@ EOF\n }\n \n sub status_cmd {\n-\tlist_and_choose({ LIST_ONLY => 1, HEADER => $status_head },\n-\t\t\tlist_modified());\n-\tprint \"\\n\";\n+\tsystem(qw(git add-interactive--helper --status));\n }\n \n sub say_n_paths {\n-- \n2.7.4 (Apple Git-66)\n\n"},{"id":"318880","messageId":"CACBZZX6oTEToPPFF0LpAXBCru9_B+VcaBL=KQUjkUhNdyanF1g@mail.gmail.com","threadId":"45881","inReplyTo":"1494009820-2090-1-git-send-email-bnmvco@gmail.com","subject":"Re: [PATCH 0/3] Port git-add--interactive.perl:status_cmd to C","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-05T19:31:18Z","receivedAt":"2017-05-05T19:31:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, May 5, 2017 at 8:43 PM, Daniel Ferreira <bnmvco@gmail.com> wrote:\n> This series introduces git-add-interactive--helper (or should it be\n> called git-add--interactive--helper?) as a builtin capable of doing\n> what the Perl script's status_cmd() would do.\n\nThe existing script is git-add--interactive.perl, so\ngit-add--interactive--helper.c would be consistent with that, since\nthere's no git-add-interactive command.\n"},{"id":"318896","messageId":"20170505201621.25560-1-avarab@gmail.com","threadId":"45881","inReplyTo":"1494009820-2090-3-git-send-email-bnmvco@gmail.com","subject":"Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-05T20:16:21Z","receivedAt":"2017-05-05T20:16:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, May 5, 2017 at 8:43 PM, Daniel Ferreira <bnmvco@gmail.com> wrote:\n> Create a builtin helper for git-add--interactive, which right now is\n> only able to reproduce git-add--interactive.perl's status_cmd()\n> function, providing a summarized diff numstat to the user.\n\nI'm not a user of add -i and didn't review the main logic in detail,\nbut I ran this, and aside from two issues this LGTM:\n\n * You missed a trailing \\n in the output, so your formatting is\n   different from the current behavior.\n\n * You should be using the getopt API instead of rolling your own.\n\nFixes for both below, thanks a lot for hacking on this. As you pointed\nout this doesn't help with removing any of the perl code for now, but\nafter replacing a few more command modes we can start chopping away at\nthe perl code.\n\ndiff --git a/builtin/add-interactive--helper.c b/builtin/add-interactive--helper.c\nindex 97ca1b38dc..ea0f790bd3 100644\n--- a/builtin/add-interactive--helper.c\n+++ b/builtin/add-interactive--helper.c\n@@ -226,6 +226,7 @@ static void print_modified(void)\n \t\tprintf(modified_fmt, index_changes, worktree_changes, f.path);\n \t\tprintf(\"\\n\");\n \t}\n+\tprintf(\"\\n\");\n }\n \n static void status_cmd(void)\n@@ -233,26 +234,31 @@ static void status_cmd(void)\n \tprint_modified();\n }\n \n-static const char add_interactive_helper_usage[] =\n-\"git add-interactive--helper <command>\";\n+static const char * const builtin_add_interactive_helper_usage[] = {\n+\tN_(\"git add-interactive--helper <command>\"),\n+\tNULL\n+};\n \n int cmd_add_interactive__helper(int argc, const char **argv, const char *prefix)\n {\n-\tint i, found_opt = 0;\n-\n-\tgit_config(git_add_interactive_config, NULL);\n+\tint opt_status = 0;\n \n-\tfor (i = 1; i < argc; i++) {\n-\t\tconst char *arg = argv[i];\n+\tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"status\", &opt_status,\n+\t\t\t N_(\"print status information with diffstat\")),\n+\t\tOPT_END()\n+\t};\n \n-\t\tif (!strcmp(arg, \"--status\")) {\n-\t\t\tstatus_cmd();\n-\t\t\tfound_opt = 1;\n-\t\t}\n-\t}\n-\n-\tif (!found_opt)\n-\t\tusage(add_interactive_helper_usage);\n+\tgit_config(git_add_interactive_config, NULL);\n+\targc = parse_options(argc, argv, NULL, options,\n+\t\t\t     builtin_add_interactive_helper_usage,\n+\t\t\t     PARSE_OPT_KEEP_ARGV0);\n+\n+\tif (opt_status)\n+\t\tstatus_cmd();\n+\telse\n+\t\tusage_with_options(builtin_add_interactive_helper_usage,\n+\t\t\t\t   options);\n \n \treturn 0;\n }\n-- \n2.11.0\n\n"},{"id":"318899","messageId":"alpine.DEB.2.21.1.1705052320270.146734@virtualbox","threadId":"45881","inReplyTo":"20170505201621.25560-1-avarab@gmail.com","subject":"Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-05T21:21:40Z","receivedAt":"2017-05-05T21:22:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 5 May 2017, Ævar Arnfjörð Bjarmason wrote:\n\n>  int cmd_add_interactive__helper(int argc, const char **argv, const char *prefix)\n>  {\n> -\tint i, found_opt = 0;\n> -\n> -\tgit_config(git_add_interactive_config, NULL);\n> +\tint opt_status = 0;\n>  \n> -\tfor (i = 1; i < argc; i++) {\n> -\t\tconst char *arg = argv[i];\n> +\tstruct option options[] = {\n> +\t\tOPT_BOOL(0, \"status\", &opt_status,\n> +\t\t\t N_(\"print status information with diffstat\")),\n\nPlease use OPT_CMDMODE() instead; it was invented exactly for this kind of\nscenario.\n\n> -- \n> 2.11.0\n\nReally? ;-)\n\nCiao,\nDscho"},{"id":"318900","messageId":"alpine.DEB.2.21.1.1705052326520.146734@virtualbox","threadId":"45881","inReplyTo":"1494009820-2090-2-git-send-email-bnmvco@gmail.com","subject":"Re: [PATCH 1/3] diff: export diffstat interface","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-05T21:28:31Z","receivedAt":"2017-05-05T21:28:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Daniel,\n\nOn Fri, 5 May 2017, Daniel Ferreira wrote:\n\n> Make the diffstat interface (namely, the diffstat_t struct and\n> diff_flush_stat) no longer be internal to diff.c and allow it to be used\n> by other parts of git.\n> \n> This is helpful for code that may want to easily extract information\n> from files using the diff machinery, while flushing it differently from\n> how the show_* functions used by diff_flush() do it.\n> \n> Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>\n\nMaybe add one more paragraph with the motivation, i.e. the conversion of\nadd -i into a builtin?\n\nThe patch is straight-forward, and I do not even see any names that may\nneed to be adjusted for a more visible API than a file-local function.\n\n> -- \n> 2.7.4 (Apple Git-66)\n\nI guess I take my comment about Ævar's Git version back ;-)\n\nCiao,\nJohannes"},{"id":"318901","messageId":"CACBZZX7LgTLHprSoc5ng47GO8vv+HLJXcu7a+bTkR5kB=NfUjg@mail.gmail.com","threadId":"45881","inReplyTo":"alpine.DEB.2.21.1.1705052320270.146734@virtualbox","subject":"Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-05T22:09:38Z","receivedAt":"2017-05-05T22:10:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, May 5, 2017 at 11:21 PM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> Hi,\n>\n> On Fri, 5 May 2017, Ævar Arnfjörð Bjarmason wrote:\n>\n>>  int cmd_add_interactive__helper(int argc, const char **argv, const char *prefix)\n>>  {\n>> -     int i, found_opt = 0;\n>> -\n>> -     git_config(git_add_interactive_config, NULL);\n>> +     int opt_status = 0;\n>>\n>> -     for (i = 1; i < argc; i++) {\n>> -             const char *arg = argv[i];\n>> +     struct option options[] = {\n>> +             OPT_BOOL(0, \"status\", &opt_status,\n>> +                      N_(\"print status information with diffstat\")),\n>\n> Please use OPT_CMDMODE() instead; it was invented exactly for this kind of\n> scenario.\n\nIndeed. Forgot about that. That's much better.\n\n>> --\n>> 2.11.0\n>\n> Really? ;-)\n\nCC-ing the Debian stretch release team with your complaint [not really].\n\nI don't bother to 'make install' the git I'm hacking on, so E-Mail\ngets sent with whatever's in Debian testing.\n"},{"id":"318902","messageId":"alpine.DEB.2.21.1.1705052328380.146734@virtualbox","threadId":"45881","inReplyTo":"1494009820-2090-3-git-send-email-bnmvco@gmail.com","subject":"Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-05T22:30:12Z","receivedAt":"2017-05-05T22:31:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Daniel,\n\n\nOn Fri, 5 May 2017, Daniel Ferreira wrote:\n\n> Create a builtin helper for git-add--interactive, which right now is\n> only able to reproduce git-add--interactive.perl's status_cmd()\n> function, providing a summarized diff numstat to the user.\n> \n> This is the first step in an effort to convert git-add--interactive.perl\n> to a C builtin, in search for better portability, expressibility and\n> performance (specially on non-POSIX systems like Windows).\n> \n> Additionally, an eventual complete port of git-add--interactive would\n> remove the last \"big\" Git script to have Perl as a dependency, allowing\n> most Git users to have a NOPERL build running without big losses.\n> \n> Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>\n\nGood. I would think that git-add--helper is a better name. It is an\ninternal command only, anyway, and hopefully it will go away soon (i.e.\nhopefully all of add -i will be builtin soon and can then be even moved\ninto a separate file outside builtin/, say, add-interactive.c).\n\n> diff --git a/.gitignore b/.gitignore\n> index 833ef3b..0d6cfe4 100644\n> --- a/.gitignore\n> +++ b/.gitignore\n> @@ -11,6 +11,7 @@\n>  /git\n>  /git-add\n>  /git-add--interactive\n> +/git-add-interactive--helper\n\nPersonally, I would prefer a separate commit adding the helper, before it\nlearns about any functionality. Somehow I have an easier time reviewing\npatches if they tease such things apart.\n\n> diff --git a/builtin/add-interactive--helper.c b/builtin/add-interactive--helper.c\n> new file mode 100644\n> index 0000000..97ca1b3\n> --- /dev/null\n> +++ b/builtin/add-interactive--helper.c\n> @@ -0,0 +1,258 @@\n> +#include \"builtin.h\"\n> +#include \"color.h\"\n> +#include \"diff.h\"\n> +#include \"diffcore.h\"\n> +#include \"revision.h\"\n> +\n> +#define ADD_INTERACTIVE_HEADER_INDENT \"      \"\n\nThis definition is local to this helper, and we already know that this is\nall about add -i. So maybe not repeat it? HEADER_INDENT would be catchier\nand just as expressive.\n\n> +enum add_interactive_collection_mode {\n> +\tCOLLECTION_MODE_NONE,\n> +\tCOLLECTION_MODE_WORKTREE,\n> +\tCOLLECTION_MODE_INDEX\n> +};\n\nLikewise, maybe we do not want to have this COLLECTION_MODE_ prefix\nlittering the source code... NONE, WORKTREE and INDEX should be short and\nsweet.\n\n> +struct add_interactive_file_status {\n\nJust struct file_status, maybe?\n\nFrom a cursory glance, it also would appear that the only need for this\nstruct to be named is the memset() call when initializing a new item,\nright? If that is the case, it would be better to use sizeof(*s->files)\nand keep it unnamed, as part of the enclosing struct.\n\n> +\tint selected;\n> +\n> +\tchar path[PATH_MAX];\n\nThat is a bit wasteful. `char *name` is what diffstat_t uses...\n\n> +\tint lines_added_index;\n> +\tint lines_deleted_index;\n> +\tint lines_added_worktree;\n> +\tint lines_deleted_worktree;\n\nMaybe for better readability:\n\n\tstruct {\n\t\tuintmax_t added, deleted;\n\t} index, worktree;\n\n> +};\n>\n> +struct add_interactive_status {\n> +\tenum add_interactive_collection_mode current_mode;\n\nI am scared of future patches where you cannot wrap the code to the\nrequired <= 80 columns/row because the data type or variable name is too\nlong... ;-)\n\n> +\n> +\tconst char *reference;\n> +\tstruct pathspec pathspec;\n> +\n> +\tint file_count;\n> +\tstruct add_interactive_file_status *files;\n\nThe convention for growable arrays in Git is to have nr, alloc and the\narray, and use ALLOC_GROW() to avoid reallocating for every single new\nitem.\n\n> +};\n> +\n> +static int add_interactive_use_color = -1;\n> +enum color_add_interactive {\n> +\tADD_INTERACTIVE_PROMPT,\n> +\tADD_INTERACTIVE_HEADER,\n> +\tADD_INTERACTIVE_HELP,\n> +\tADD_INTERACTIVE_ERROR\n> +};\n\nAgain, if we were talking about a declaration in a .h file, that prefix\nwould make sense, but here we are safely in file-local territory where I\nthink this is just unnecessary churn.\n\n> +static char add_interactive_colors[][COLOR_MAXLEN] = {\n> +\tGIT_COLOR_BOLD_BLUE, /* Prompt */\n> +\tGIT_COLOR_BOLD,      /* Header */\n> +\tGIT_COLOR_BOLD_RED,  /* Help */\n> +\tGIT_COLOR_BOLD_RED   /* Error */\n> +};\n> +\n> +static const char *add_interactive_get_color(enum color_add_interactive ix)\n> +{\n> +\tif (want_color(add_interactive_use_color))\n> +\t\treturn add_interactive_colors[ix];\n> +\treturn \"\";\n> +}\n> +\n> +static int parse_add_interactive_color_slot(const char *slot)\n> +{\n> +\tif (!strcasecmp(slot, \"prompt\"))\n> +\t\treturn ADD_INTERACTIVE_PROMPT;\n> +\tif (!strcasecmp(slot, \"header\"))\n> +\t\treturn ADD_INTERACTIVE_HEADER;\n> +\tif (!strcasecmp(slot, \"help\"))\n> +\t\treturn ADD_INTERACTIVE_HELP;\n> +\tif (!strcasecmp(slot, \"error\"))\n> +\t\treturn ADD_INTERACTIVE_ERROR;\n> +\n> +\treturn -1;\n> +}\n> +\n> +static int git_add_interactive_config(const char *var,\n\nNot git_add_interactive__helper_config()? ;-)\n\n> +\t\tconst char *value, void *cbdata)\n> +{\n> +\tconst char *name;\n> +\n> +\tif (!strcmp(var, \"color.interactive\")) {\n> +\t\tadd_interactive_use_color = git_config_colorbool(var, value);\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tif (skip_prefix(var, \"color.interactive\", &name)) {\n> +\t\tint slot = parse_add_interactive_color_slot(name);\n> +\t\tif (slot < 0)\n> +\t\t\treturn 0;\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(var);\n> +\t\treturn color_parse(value, add_interactive_colors[slot]);\n> +\t}\n> +\n> +\treturn git_default_config(var, value, cbdata);\n> +}\n> +\n> +static void add_interactive_status_collect_changed_cb(struct diff_queue_struct *q,\n\nI did have that fear, and now it comes true... ;-)\n\nSeriously again, this is a file-local function. We know it is about\nadd --interactive, and we can be pretty succinct about its role, i.e.\nsomething like collect_changes_callback() would be plenty sufficient.\n\n> +\t\t\t\t\t struct diff_options *options,\n> +\t\t\t\t\t void *data)\n> +{\n> +\tstruct add_interactive_status *s = data;\n> +\tstruct diffstat_t stat;\n> +\tint i, j;\n> +\n> +\tif (!q->nr)\n> +\t\treturn;\n> +\n> +\tmemset(&stat, 0, sizeof(struct diffstat_t));\n\nI guess it makes sense to play it safe here (by zeroing the entire struct\nrather than assigning the individual initial values).\n\nBut would\n\n\tstruct diffstat_t stat = { 0 };\n\nnot have done the same?\n\n> +\tfor (i = 0; i < q->nr; i++) {\n> +\t\tstruct diff_filepair *p;\n> +\t\tp = q->queue[i];\n> +\t\tdiff_flush_stat(p, options, &stat);\n> +\t}\n> +\n> +\tfor (i = 0; i < stat.nr; i++) {\n> +\t\tint file_index = s->file_count;\n> +\t\tfor (j = 0; j < s->file_count; j++) {\n> +\t\t\tif (!strcmp(s->files[j].path, stat.files[i]->name)) {\n> +\t\t\t\tfile_index = j;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n\nSo basically, this is looking up in a list whether we saw the file in\nquestion already, and the reason we have to do that is that we run the\nentire shebang twice, once with the worktree and once with the index.\n\nI wonder whether it would not make sense to switch away s->files from a\nlist to a hashmap.\n\n> +\t\tif (file_index == s->file_count) {\n> +\t\t\ts->file_count++;\n> +\t\t\ts->files = realloc(s->files,\n> +\t\t\t\t\t(q->nr + s->file_count) * sizeof(*s->files));\n\nWe already have ALLOC_GROW() to do this, safer. And easier to verify ;-)\n\n> +\t\t\tmemset(&s->files[file_index], 0,\n> +\t\t\t\t\tsizeof(struct add_interactive_file_status));\n> +\t\t}\n> +\n> +\t\tmemcpy(s->files[file_index].path, stat.files[i]->name,\n> +\t\t\t\tstrlen(stat.files[i]->name) + 1);\n\nApart from using PATH_MAX bytes for most likely only short names: should\nthis not be inside the if (file_index == s->file_count) clause? I mean, if\nwe do not hit that conditional code, it is because we already compared the\nfile name and figured out that s->files[file_index].path is identical to\nstat.files[i]->name...\n\n\n> +\t\tif (s->current_mode == COLLECTION_MODE_WORKTREE) {\n> +\t\t\ts->files[file_index].lines_added_worktree = stat.files[i]->added;\n> +\t\t\ts->files[file_index].lines_deleted_worktree = stat.files[i]->deleted;\n> +\t\t} else if (s->current_mode == COLLECTION_MODE_INDEX) {\n> +\t\t\ts->files[file_index].lines_added_index = stat.files[i]->added;\n> +\t\t\ts->files[file_index].lines_deleted_index = stat.files[i]->deleted;\n> +\t\t}\n> +\t}\n> +}\n> +\n> +static void add_interactive_status_collect_changes_worktree(struct add_interactive_status *s)\n\nI would use the name status_worktree() here instead.\n\n> +{\n> +\tstruct rev_info rev;\n> +\n> +\ts->current_mode = COLLECTION_MODE_WORKTREE;\n\nNow that I read this and remember that only WORKTREE and INDEX are handled\nin the callback function: is there actually a use for the NONE enum value?\nI.e. is current_mode read out in any other context than the callback\nfunction? If there is no other read, then the NONE enum value is just\nconfusing.\n\nBTW in the first pass, we pretty much know that we only get unique names,\nso the entire lookup is unnecessary and will just increase the time\ncomplexity from O(n) to O(n^2). So let's avoid that.\n\nBy moving to a hashmap, you can even get the second phase down to an\nexpected O(n).\n\n> +\n> +\tinit_revisions(&rev, NULL);\n> +\tsetup_revisions(0, NULL, &rev, NULL);\n> +\n> +\trev.max_count = 0;\n\nWhere does this come from? We are not logging commits... oh wait, I see\nthat diff-lib.c reuses that field for its diff_unmerged_stage value. Urgh.\nNot your fault, of course.\n\n> +\trev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n> +\trev.diffopt.format_callback = add_interactive_status_collect_changed_cb;\n> +\trev.diffopt.format_callback_data = s;\n> +\n> +\trun_diff_files(&rev, 0);\n> +}\n> +\n> +static void add_interactive_status_collect_changes_index(struct add_interactive_status *s)\n> +{\n> +\tstruct rev_info rev;\n> +\tstruct setup_revision_opt opt;\n> +\n> +\ts->current_mode = COLLECTION_MODE_INDEX;\n> +\n> +\tinit_revisions(&rev, NULL);\n> +\tmemset(&opt, 0, sizeof(opt));\n\n\tstruct setup_revision_opt opt = { NULL };\n\n> +\topt.def = s->reference;\n> +\tsetup_revisions(0, NULL, &rev, &opt);\n\nOh, I see, you want to use the opt argument to pass HEAD or the emtpy tree\nSHA-1 if we're on an unborn branch.\n\nSince you're already calling get_sha1() on \"HEAD\", you may just as well\nkeep the SHA-1 (and use the EMPTY_TREE_SHA1 if there is no HEAD), and then\npass that in via the argc, argv parameters to setup_revisions() (imitating\ncmd_diff_index() a bit closer).\n\n> +\n> +\trev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n> +\trev.diffopt.format_callback = add_interactive_status_collect_changed_cb;\n> +\trev.diffopt.format_callback_data = s;\n> +\n> +\trun_diff_index(&rev, 1);\n> +}\n> +\n> +static void list_modified_into_status(struct add_interactive_status *s)\n> +{\n> +\tadd_interactive_status_collect_changes_worktree(s);\n> +\tadd_interactive_status_collect_changes_index(s);\n> +}\n\nWhy not collapse all three functions into one? It is not like they are\ntotally unrelated nor super-long.\n\n> +static void print_modified(void)\n> +{\n> +\tint i;\n> +\tstruct add_interactive_status s;\n> +\tconst char *modified_fmt = _(\"%12s %12s %s\");\n\nWe cannot really translate that...\n\n> +\tconst char *header_color = add_interactive_get_color(ADD_INTERACTIVE_HEADER);\n> +\tunsigned char sha1[20];\n> +\n> +\tif (read_cache() < 0)\n> +\t\treturn;\n> +\n> +\ts.current_mode = COLLECTION_MODE_NONE;\n> +\ts.reference = !get_sha1(\"HEAD\", sha1) ? \"HEAD\": EMPTY_TREE_SHA1_HEX;\n> +\ts.file_count = 0;\n> +\ts.files = NULL;\n> +\tlist_modified_into_status(&s);\n> +\n> +\tprintf(ADD_INTERACTIVE_HEADER_INDENT);\n> +\tcolor_fprintf(stdout, header_color, modified_fmt, _(\"staged\"),\n> +\t\t\t_(\"unstaged\"), _(\"path\"));\n\nI think these _() need to become N_().\n\n> +\tprintf(\"\\n\");\n\nWouldn't it be better to avoid defining modified_fmt (we do not really\nmodify it, do we?) and at least make the '\\n' part of the format string?\n\n> +\tfor (i = 0; i < s.file_count; i++) {\n> +\t\tstruct add_interactive_file_status f = s.files[i];\n> +\t\tchar selection = f.selected ? '*' : ' ';\n\nI would prefer the variable to be called \"marker\".\n\n> +\n> +\t\tchar worktree_changes[50];\n> +\t\tchar index_changes[50];\n\nThose \"50\" look a bit arbitrary... maybe use strbufs instead (so that we\ndo not have to hope that all translations of \"nothing\" or \"unchanged\",\neven Welsh ones, will fit inside those buffers)?\n\n> +\t\tif (f.lines_added_worktree != 0 || f.lines_deleted_worktree != 0)\n\nIn Git's source code, the convention is to just drop the `!= 0` in\ncomparisons, unless there is a really good reason not to.\n\n> +\t\t\tsnprintf(worktree_changes, 50, \"+%d/-%d\", f.lines_added_worktree,\n> +\t\t\t\t\tf.lines_deleted_worktree);\n> +\t\telse\n> +\t\t\tsnprintf(worktree_changes, 50, \"%s\", _(\"nothing\"));\n> +\n> +\t\tif (f.lines_added_index != 0 || f.lines_deleted_index != 0)\n> +\t\t\tsnprintf(index_changes, 50, \"+%d/-%d\", f.lines_added_index,\n> +\t\t\t\t\tf.lines_deleted_index);\n> +\t\telse\n> +\t\t\tsnprintf(index_changes, 50, \"%s\", _(\"unchanged\"));\n> +\n> +\t\tprintf(\"%c%2d: \", selection, i + 1);\n> +\t\tprintf(modified_fmt, index_changes, worktree_changes, f.path);\n> +\t\tprintf(\"\\n\");\n\nIt would be nicer to make this a single printf() call. The only idea I\nhave to allow for that is to turn modified_fmt into a `#define\nMODIFIED_FMT \"%12s %12s %s\"`, though.\n\n> +\t}\n> +}\n> +\n> +static void status_cmd(void)\n> +{\n> +\tprint_modified();\n> +}\n\nAs long as this function really only calls another function with no\nparameters, let's just drop it. We can call print_modified() instead of\nstatus_cmd() just as easily.\n\nÆvar already commented on the parseopt stuff.\n\nOtherwise this looks pretty good to me!\n\nThanks,\nJohannes"},{"id":"318903","messageId":"alpine.DEB.2.21.1.1705060031450.146734@virtualbox","threadId":"45881","inReplyTo":"1494009820-2090-4-git-send-email-bnmvco@gmail.com","subject":"Re: [PATCH 3/3] add--interactive: use add-interactive--helper for status_cmd","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-05T22:32:00Z","receivedAt":"2017-05-05T22:32:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Daniel,\n\n\nOn Fri, 5 May 2017, Daniel Ferreira wrote:\n\n> Call the newly introduced git-add-interactive--helper builtin on\n> status_cmd() instead of relying on git-add--interactive's Perl\n> functions to build print the numstat.\n> \n> Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>\n> ---\n>  git-add--interactive.perl | 4 +---\n>  1 file changed, 1 insertion(+), 3 deletions(-)\n> \n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index 709a5f6..8617462 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -632,9 +632,7 @@ EOF\n>  }\n>  \n>  sub status_cmd {\n> -\tlist_and_choose({ LIST_ONLY => 1, HEADER => $status_head },\n> -\t\t\tlist_modified());\n> -\tprint \"\\n\";\n> +\tsystem(qw(git add-interactive--helper --status));\n>  }\n>  \n>  sub say_n_paths {\n\nObviously good!\n\nThanks,\nJohannes\n"},{"id":"318904","messageId":"alpine.DEB.2.21.1.1705060032340.146734@virtualbox","threadId":"45881","inReplyTo":"CACBZZX6oTEToPPFF0LpAXBCru9_B+VcaBL=KQUjkUhNdyanF1g@mail.gmail.com","subject":"Re: [PATCH 0/3] Port git-add--interactive.perl:status_cmd to C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-05T22:33:29Z","receivedAt":"2017-05-05T22:33:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 5 May 2017, Ævar Arnfjörð Bjarmason wrote:\n\n> On Fri, May 5, 2017 at 8:43 PM, Daniel Ferreira <bnmvco@gmail.com> wrote:\n> > This series introduces git-add-interactive--helper (or should it be\n> > called git-add--interactive--helper?) as a builtin capable of doing\n> > what the Perl script's status_cmd() would do.\n> \n> The existing script is git-add--interactive.perl, so\n> git-add--interactive--helper.c would be consistent with that, since\n> there's no git-add-interactive command.\n\nActually, to be consistent with the existing git-rebase--helper example\n(which is also only helping the interactive version), it should be\ngit-add--helper.\n\nCiao,\nJohannes"},{"id":"318905","messageId":"20170505223539.GI28740@aiede.svl.corp.google.com","threadId":"45881","inReplyTo":"CACBZZX6oTEToPPFF0LpAXBCru9_B+VcaBL=KQUjkUhNdyanF1g@mail.gmail.com","subject":"Re: [PATCH 0/3] Port git-add--interactive.perl:status_cmd to C","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-05-05T22:35:39Z","receivedAt":"2017-05-05T22:35:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> On Fri, May 5, 2017 at 8:43 PM, Daniel Ferreira <bnmvco@gmail.com> wrote:\n\n>> This series introduces git-add-interactive--helper (or should it be\n>> called git-add--interactive--helper?) as a builtin capable of doing\n>> what the Perl script's status_cmd() would do.\n>\n> The existing script is git-add--interactive.perl, so\n> git-add--interactive--helper.c would be consistent with that, since\n> there's no git-add-interactive command.\n\nOr git-add--interactive-helper.c, or git-add--helper.c.\n\nThe important thing is that there's a double-dash somewhere.\n\nThanks,\nJonathan\n"},{"id":"318906","messageId":"alpine.DEB.2.21.1.1705052316250.146734@virtualbox","threadId":"45881","inReplyTo":"1494009820-2090-1-git-send-email-bnmvco@gmail.com","subject":"Re: [PATCH 0/3] Port git-add--interactive.perl:status_cmd to C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-05T22:38:54Z","receivedAt":"2017-05-05T22:39:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Daniel,\n\nOn Fri, 5 May 2017, Daniel Ferreira wrote:\n\n> Hm, it looks like my GSoC project won't be in the Git organization,\n\nYeah, rumors have it that you were quite a popular student and several\norganizations fought bloody wars over getting you... ;-)\n\n> although I'm still interested in going for this so I guess I'll send\n> patches to implement my proposal anyway (although certainly in a slower\n> pace than I would if on the program).\n\nNice!\n\n> This series introduces git-add-interactive--helper (or should it be\n> called git-add--interactive--helper?) as a builtin capable of doing\n> what the Perl script's status_cmd() would do.\n\nSince it is a temporary thing, and not user-visible, I would opt for\ngit-add--helper. I may have mentioned that a couple of times now... ;-)\n\n> I wish this patch series could have been smaller, although I don't\n> think it would make sense to bring add-interactive \"subunits\" smaller\n> than a command to the helper, and status_cmd (aside from probably\n> add_untracked_cmd) was the simplest one to do after all -- and still\n> required ~250 lines on the new builtin.\n\nI think it's fine. There has been a patch series consisting of ~80\nindividual patches before, as long as you do not go close to that one, I\nthink you are fine.\n\n> Another regret I had was not being able to retire any code from the Perl\n> script yet (and will likely not be able to do so until all commands have\n> been ported), but that is not such a big thing after all.\n\nAll in due time.\n\nBut maybe you want to keep the naming a little more consistent with the\nPerl script, e.g. instead of calling the function `print_modified()` call\nit already `list()` (and rename it later to `list_and_choose()` once you\nhave taught it to ask for a choice)?\n\n> As for the new builtin, I think the color handling code is pretty\n> straightforward. In fact, it was practically copied from places like\n> clean.c or diff.c (which makes me wonder if some of that code could\n> be generalized to avoid duplication). The same goes for the pretty\n> simple option parsing code.\n\nTrue. It does look awfully similar to other code handling colors.\n\nMaybe put it on the backlog. Or the GSoC get-your-feet-wet projects for\nnext year.\n\n> Bigger issues seem to arise when dealing with getting the numstat.\n> While (as Junio anticipated on an RFC) some tasks like getting the\n> actual diff and splitting it may require making the \"diff\n> machinery\" write to a temporary file that we will read and do things\n> with, I think it would be weird to do that for parsing a simple\n> numstat from it.\n\nI would actually think that the callback Junio talked about would be more\nappropriate than to write temporary files. Even to split the diff. We can\ndo that much more efficiently in-memory.\n\n> My first instinct was to create something like\n> show_numstat_interactive() or something on diff.c (analogous to the\n> other show_* functions). Doing that, however, would stumble upon another\n> issue: we would not be able to print both a file's diff against the\n> index (obtained from run_diff_index) and against the worktree (obtained\n> from run_diff_files) in that function. The solution I came up with was\n> to export the diffstat interface from diff.c into the world and allow\n> our new builtin to use that and build our status_cmd output. Unless this\n> breaks some rule of Git's API design, the result seems pretty reasonable\n> to me.\n\nIt looks pretty reasonable to me, too.\n\nThank you for this pleasant read!\nJohannes\n"},{"id":"318908","messageId":"CACBZZX6_GbqNpTXTB8UQJzvD2nx_UTN8fwHV8Aczg+b9XoO_tg@mail.gmail.com","threadId":"45881","inReplyTo":"alpine.DEB.2.21.1.1705052328380.146734@virtualbox","subject":"Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-05T22:49:50Z","receivedAt":"2017-05-05T22:50:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, May 6, 2017 at 12:30 AM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> Hi Daniel,\n>\n>\n> On Fri, 5 May 2017, Daniel Ferreira wrote:\n>> +static void print_modified(void)\n>> +{\n>> +     int i;\n>> +     struct add_interactive_status s;\n>> +     const char *modified_fmt = _(\"%12s %12s %s\");\n>\n> We cannot really translate that...\n\nHe copied this from the *.perl code:\n\n    # TRANSLATORS: you can adjust this to align \"git add -i\" status menu\n    my $status_fmt = __('%12s %12s %s');\n\nAnd one translation at least makes use of that (and probably others should).\n\nBut porting the /* TRANSLATORS: ... */ comment over is missing, and\nshould be added.\n"},{"id":"318909","messageId":"CAEA2_RKzUdSPP4bBvGiFVfNnAY3wwp+0LYriC4q5XfCP-1-F4w@mail.gmail.com","threadId":"45881","inReplyTo":"alpine.DEB.2.21.1.1705052328380.146734@virtualbox","subject":"Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add","fromName":"Daniel Ferreira (theiostream)","fromEmail":"bnmvco@gmail.com","sentAt":"2017-05-05T23:13:16Z","receivedAt":"2017-05-05T23:13:54Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"On Fri, May 5, 2017 at 7:30 PM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n>> +static int git_add_interactive_config(const char *var,\n>\n> Not git_add_interactive__helper_config()? ;-)\n\nI don't get if you mean this ironically (because of the verbosity) or\nif you do think this would be a good name ;P\n\n>> +     for (i = 0; i < q->nr; i++) {\n>> +             struct diff_filepair *p;\n>> +             p = q->queue[i];\n>> +             diff_flush_stat(p, options, &stat);\n>> +     }\n>> +\n>> +     for (i = 0; i < stat.nr; i++) {\n>> +             int file_index = s->file_count;\n>> +             for (j = 0; j < s->file_count; j++) {\n>> +                     if (!strcmp(s->files[j].path, stat.files[i]->name)) {\n>> +                             file_index = j;\n>> +                             break;\n>> +                     }\n>> +             }\n>\n> So basically, this is looking up in a list whether we saw the file in\n> question already, and the reason we have to do that is that we run the\n> entire shebang twice, once with the worktree and once with the index.\n>\n> I wonder whether it would not make sense to switch away s->files from a\n> list to a hashmap.\n> [...]\n> BTW in the first pass, we pretty much know that we only get unique names,\n> so the entire lookup is unnecessary and will just increase the time\n> complexity from O(n) to O(n^2). So let's avoid that.\n>\n> By moving to a hashmap, you can even get the second phase down to an\n> expected O(n).\n\nHow would you go about implementing that hashmap (i.e. what should be\nthe hash)? Does Git have any interface for it, or is there any example\nI can look after in the codebase?\n\n> Apart from using PATH_MAX bytes for most likely only short names: [...]\n\nIf not PATH_MAX, what should I go for? Make it a strbuf? I tend to\nbelieve keeping that on the stack would be simpler and more optimal.\n\n> Now that I read this and remember that only WORKTREE and INDEX are handled\n> in the callback function: is there actually a use for the NONE enum value?\n> I.e. is current_mode read out in any other context than the callback\n> function? If there is no other read, then the NONE enum value is just\n> confusing.\n\nI just preferred to have a declared non-handled value than leave\nsomething undefined behind. I felt it might avoid headaches in the\nfuture with petty segfaults.\n\n> Why not collapse all three functions into one? It is not like they are\n> totally unrelated nor super-long.\n\nTo me it is a matter of personal preference to keep them separate. If\nthere is, however, any technical or project-style-related reason to\nget them together, I'll certainly do it.\n\n>> +static void print_modified(void)\n>> +{\n>> +     int i;\n>> +     struct add_interactive_status s;\n>> +     const char *modified_fmt = _(\"%12s %12s %s\");\n>\n> We cannot really translate that...\n\nApparently, we can. Ævar covered that in his reply.\n\n>> +     printf(ADD_INTERACTIVE_HEADER_INDENT);\n>> +     color_fprintf(stdout, header_color, modified_fmt, _(\"staged\"),\n>> +                     _(\"unstaged\"), _(\"path\"));\n>\n> I think these _() need to become N_().\n\nI cannot find any call to N_() outside of Perl code. What should that\neven do differently?\n\n>> +static void status_cmd(void)\n>> +{\n>> +     print_modified();\n>> +}\n>\n> As long as this function really only calls another function with no\n> parameters, let's just drop it. We can call print_modified() instead of\n> status_cmd() just as easily.\n\nI thought calling status_cmd() would make that more clear, but I agree\n-- the options already make it clear enough,\n\nI agree with all points I did not directly address. And thank you for\nthe review :)\n\n-- Daniel.\n"},{"id":"318910","messageId":"CACBZZX6-ZmxkPo14N_D5f9rm=WtR_MkJzgZv_xYMqtcq5xfBHA@mail.gmail.com","threadId":"45881","inReplyTo":"CAEA2_RKzUdSPP4bBvGiFVfNnAY3wwp+0LYriC4q5XfCP-1-F4w@mail.gmail.com","subject":"Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-05T23:28:24Z","receivedAt":"2017-05-05T23:28:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, May 6, 2017 at 1:13 AM, Daniel Ferreira (theiostream)\n<bnmvco@gmail.com> wrote:\n> On Fri, May 5, 2017 at 7:30 PM, Johannes Schindelin\n> <Johannes.Schindelin@gmx.de> wrote:\n>>> +static int git_add_interactive_config(const char *var,\n>>\n>> Not git_add_interactive__helper_config()? ;-)\n>\n> I don't get if you mean this ironically (because of the verbosity) or\n> if you do think this would be a good name ;P\n>\n>>> +     for (i = 0; i < q->nr; i++) {\n>>> +             struct diff_filepair *p;\n>>> +             p = q->queue[i];\n>>> +             diff_flush_stat(p, options, &stat);\n>>> +     }\n>>> +\n>>> +     for (i = 0; i < stat.nr; i++) {\n>>> +             int file_index = s->file_count;\n>>> +             for (j = 0; j < s->file_count; j++) {\n>>> +                     if (!strcmp(s->files[j].path, stat.files[i]->name)) {\n>>> +                             file_index = j;\n>>> +                             break;\n>>> +                     }\n>>> +             }\n>>\n>> So basically, this is looking up in a list whether we saw the file in\n>> question already, and the reason we have to do that is that we run the\n>> entire shebang twice, once with the worktree and once with the index.\n>>\n>> I wonder whether it would not make sense to switch away s->files from a\n>> list to a hashmap.\n>> [...]\n>> BTW in the first pass, we pretty much know that we only get unique names,\n>> so the entire lookup is unnecessary and will just increase the time\n>> complexity from O(n) to O(n^2). So let's avoid that.\n>>\n>> By moving to a hashmap, you can even get the second phase down to an\n>> expected O(n).\n>\n> How would you go about implementing that hashmap (i.e. what should be\n> the hash)? Does Git have any interface for it, or is there any example\n> I can look after in the codebase?\n\nGit has a hashmap API already. 7d4558c462 is a good example of using it.\n\n>>> +     printf(ADD_INTERACTIVE_HEADER_INDENT);\n>>> +     color_fprintf(stdout, header_color, modified_fmt, _(\"staged\"),\n>>> +                     _(\"unstaged\"), _(\"path\"));\n>>\n>> I think these _() need to become N_().\n>\n> I cannot find any call to N_() outside of Perl code. What should that\n> even do differently?\n\nN_() is to mark a string for later translation, not to return it. See\nmy \"how to use getopt\" patch for an example of that, but this doesn't\nlook like such a case, since _() is returning the string to\ncolor_fprintf, surely...\n\n    $ git grep -P '\\bN_\\(' -- '*.c'\n"},{"id":"318911","messageId":"CAEA2_RJFPZ6-ZMQCjX5pXoA8RL6qscLO4QWP3gi46uWXYyD44Q@mail.gmail.com","threadId":"45881","inReplyTo":"alpine.DEB.2.21.1.1705052316250.146734@virtualbox","subject":"Re: [PATCH 0/3] Port git-add--interactive.perl:status_cmd to C","fromName":"Daniel Ferreira (theiostream)","fromEmail":"bnmvco@gmail.com","sentAt":"2017-05-05T23:37:39Z","receivedAt":"2017-05-05T23:38:19Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"On Fri, May 5, 2017 at 7:38 PM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> But maybe you want to keep the naming a little more consistent with the\n> Perl script, e.g. instead of calling the function `print_modified()` call\n> it already `list()` (and rename it later to `list_and_choose()` once you\n> have taught it to ask for a choice)?\n\nActually, I named it print_modified() because I anticipated there\nwould be no list_and_choose() equivalent in C. I don't think the\nbuiltin should have the interactive menu + modified list + untracked\nlist being handled by one function. Rather, I think a saner way to go\non with it would be to create functions like print_untracked();\nchoose_from_input(); print_menu() etc.\n\nThis is still pure speculation from what I've written until now and\nfrom the roadmap I have in my head (I do not know how writing code\nfrom now on will actually be), but I have a hunch that\nlist_and_choose() is already convoluted enough in Perl; in C it might\nbecome a monster.\n\n> Thank you for this pleasant read!\n\nThank *you* for the quick and thorough review :)\n\n-- Daniel.\n"},{"id":"319080","messageId":"alpine.DEB.2.21.1.1705081334440.146734@virtualbox","threadId":"45881","inReplyTo":"CACBZZX6_GbqNpTXTB8UQJzvD2nx_UTN8fwHV8Aczg+b9XoO_tg@mail.gmail.com","subject":"Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-08T11:35:20Z","receivedAt":"2017-05-08T11:35:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 6 May 2017, Ævar Arnfjörð Bjarmason wrote:\n\n> On Sat, May 6, 2017 at 12:30 AM, Johannes Schindelin\n> <Johannes.Schindelin@gmx.de> wrote:\n> > Hi Daniel,\n> >\n> >\n> > On Fri, 5 May 2017, Daniel Ferreira wrote:\n> >> +static void print_modified(void)\n> >> +{\n> >> +     int i;\n> >> +     struct add_interactive_status s;\n> >> +     const char *modified_fmt = _(\"%12s %12s %s\");\n> >\n> > We cannot really translate that...\n> \n> He copied this from the *.perl code:\n> \n>     # TRANSLATORS: you can adjust this to align \"git add -i\" status menu\n>     my $status_fmt = __('%12s %12s %s');\n> \n> And one translation at least makes use of that (and probably others should).\n> \n> But porting the /* TRANSLATORS: ... */ comment over is missing, and\n> should be added.\n\nAh, that explains it. It still is not really translateable, but with the\ncomment, even *I* understand why it is marked for translation.\n\nThanks,\nDscho"},{"id":"319081","messageId":"alpine.DEB.2.21.1.1705081335400.146734@virtualbox","threadId":"45881","inReplyTo":"CAEA2_RKzUdSPP4bBvGiFVfNnAY3wwp+0LYriC4q5XfCP-1-F4w@mail.gmail.com","subject":"Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-08T12:15:21Z","receivedAt":"2017-05-08T12:15:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Daniel,\n\nOn Fri, 5 May 2017, Daniel Ferreira (theiostream) wrote:\n\n> On Fri, May 5, 2017 at 7:30 PM, Johannes Schindelin\n> <Johannes.Schindelin@gmx.de> wrote:\n> >> +static int git_add_interactive_config(const char *var,\n> >\n> > Not git_add_interactive__helper_config()? ;-)\n> \n> I don't get if you mean this ironically (because of the verbosity) or\n> if you do think this would be a good name ;P\n\nHehe. I meant it tongue-in-cheek.\n\nSo let me try again in my endeavor to provide *constructive* criticism,\ni.e. not only pointing out what I think is suboptimal, but *also* how to\nimprove it in my opinion. How about add_i_config() or git_add_config()?\n\n> >> +     for (i = 0; i < q->nr; i++) {\n> >> +             struct diff_filepair *p;\n> >> +             p = q->queue[i];\n> >> +             diff_flush_stat(p, options, &stat);\n> >> +     }\n> >> +\n> >> +     for (i = 0; i < stat.nr; i++) {\n> >> +             int file_index = s->file_count;\n> >> +             for (j = 0; j < s->file_count; j++) {\n> >> +                     if (!strcmp(s->files[j].path, stat.files[i]->name)) {\n> >> +                             file_index = j;\n> >> +                             break;\n> >> +                     }\n> >> +             }\n> >\n> > So basically, this is looking up in a list whether we saw the file in\n> > question already, and the reason we have to do that is that we run the\n> > entire shebang twice, once with the worktree and once with the index.\n> >\n> > I wonder whether it would not make sense to switch away s->files from a\n> > list to a hashmap.\n> > [...]\n> > BTW in the first pass, we pretty much know that we only get unique names,\n> > so the entire lookup is unnecessary and will just increase the time\n> > complexity from O(n) to O(n^2). So let's avoid that.\n> >\n> > By moving to a hashmap, you can even get the second phase down to an\n> > expected O(n).\n> \n> How would you go about implementing that hashmap (i.e. what should be\n> the hash)? Does Git have any interface for it, or is there any example\n> I can look after in the codebase?\n\nThe example Ævar pointed to seems to be pretty good (Michael Haggerty's\ncommits are in general excellent examples to follow):\n\n\thttps://github.com/git-for-windows/git/commit/7d4558c462f0\n\nIn this case, we can even fold the added/deleted part into the struct:\n\n\t#include \"hashmap.h\"\n\n\t...\n\n\tstruct file_stats {\n\t\tstruct hashmap_entry entry;\n\t\tstruct {\n\t\t\tuintmax_t added, deleted;\n\t\t} index, worktree;\n\t\tchar name[FLEX_ARRAY];\n\t}\n\n\t...\n\t\tfor (i = 0; i < stat.nr; i++) {\n\t\t\tstruct file_stats *stats;\n\t\t\tconst char *name = stat.files[i]->name;\n\t\t\tunsigned int hash = strhash(name);\n\n\t\t\tstats = s->phase == INDEX ? NULL :\n\t\t\t\thashmap_get_from_hash(&map, hash, name);\n\t\t\tif (!stats) {\n\t\t\t\tFLEX_ALLOC_STR(stats, name, name);\n\t\t\t\thashmap_entry_init(stats, hash);\n\t\t\t\tstats->index.added = stats->index.deleted = 0;\n\t\t\t\tstats->worktree.added =\n\t\t\t\t\tstats->worktree.deleted = 0;\n\t\t\t\thashmap_add(&map, stats);\n\t\t\t}\n\n\t\t\tif (s->phase == INDEX) {\n\t\t\t\tstats->index.added = stat.files[i]->added;\n\t\t\t\tstats->index.deleted = stat.files[i]->deleted;\n\t\t\t} else {\n\t\t\t\tstats->worktree.added = stat.files[i]->added;\n\t\t\t\tstats->worktree.deleted = stat.files[i]->deleted;\n\t\t\t}\n\t\t}\n\nBut maybe it should simultaneously put those added entries into a growing\narray, as they arrive sorted and we will want to output the entries\nsorted, too.\n\nOh, this reminds me: you are reading the list in two phases, and each time\nthe entries arrive sorted, but the second time we still may append new\nentries.\n\nMaybe we need to sort the entries afterwards?\n\n> > Apart from using PATH_MAX bytes for most likely only short names: [...]\n> \n> If not PATH_MAX, what should I go for? Make it a strbuf? I tend to\n> believe keeping that on the stack would be simpler and more optimal.\n\nOn the stack, no question. But I was talking about struct\nadd_interactive_file_status, which is allocated via malloc(), not\nallocated on the stack/heap.\n\n> > Now that I read this and remember that only WORKTREE and INDEX are\n> > handled in the callback function: is there actually a use for the NONE\n> > enum value?  I.e. is current_mode read out in any other context than\n> > the callback function? If there is no other read, then the NONE enum\n> > value is just confusing.\n> \n> I just preferred to have a declared non-handled value than leave\n> something undefined behind. I felt it might avoid headaches in the\n> future with petty segfaults.\n\nI found it a little harder to read, as I was puzzled about the NONE\nvalue...\n\n> > Why not collapse all three functions into one? It is not like they are\n> > totally unrelated nor super-long.\n> \n> To me it is a matter of personal preference to keep them separate. If\n> there is, however, any technical or project-style-related reason to get\n> them together, I'll certainly do it.\n\nI was looking at it from a readability point of view. These functions\nbecame very small, and the operation (which in my mind was one logical\ntwo-phase operation) became too scattered to follow easily. This may be a\nlimitation of my brain, but I would have had an easier time if those three\nfunctions were one function.\n\nCiao,\nJohannes"},{"id":"319082","messageId":"alpine.DEB.2.21.1.1705081415370.146734@virtualbox","threadId":"45881","inReplyTo":"CAEA2_RJFPZ6-ZMQCjX5pXoA8RL6qscLO4QWP3gi46uWXYyD44Q@mail.gmail.com","subject":"Re: [PATCH 0/3] Port git-add--interactive.perl:status_cmd to C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-08T12:23:37Z","receivedAt":"2017-05-08T12:23:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Daniel,\n\nOn Fri, 5 May 2017, Daniel Ferreira (theiostream) wrote:\n\n> On Fri, May 5, 2017 at 7:38 PM, Johannes Schindelin\n> <Johannes.Schindelin@gmx.de> wrote:\n>\n> > But maybe you want to keep the naming a little more consistent with\n> > the Perl script, e.g. instead of calling the function\n> > `print_modified()` call it already `list()` (and rename it later to\n> > `list_and_choose()` once you have taught it to ask for a choice)?\n> \n> Actually, I named it print_modified() because I anticipated there\n> would be no list_and_choose() equivalent in C. I don't think the\n> builtin should have the interactive menu + modified list + untracked\n> list being handled by one function. Rather, I think a saner way to go\n> on with it would be to create functions like print_untracked();\n> choose_from_input(); print_menu() etc.\n\nOkay. But maybe then the `selected` field should not (at least not yet) be\na part of this patch.\n\n> This is still pure speculation from what I've written until now and from\n> the roadmap I have in my head (I do not know how writing code from now\n> on will actually be), but I have a hunch that list_and_choose() is\n> already convoluted enough in Perl; in C it might become a monster.\n\nTrue.\n\nTo be honest, I would love for this patch to become part of a\n\"work-in-progress\" patch series that lays out the plan a little bit more\nconcretely (I typically implement functions with a single `die(\"TODO\")` in\nthe function body in such patch series). This patch series would of course\nnot be expected to pass the test suite yet, but it would allow other\ndevelopers (e.g. myself) to jump in and complete individual patches...\n\nThere would be real advantages in such a patch series:\n\n- it could be worked on in parallel (coordination required, of course);\n  may be a ton of fun, and\n\n- the overall design could be iterated according with the needs of later\n  patches in the series.\n\n> > Thank you for this pleasant read!\n> \n> Thank *you* for the quick and thorough review :)\n\nHeh... I would not call it \"quick\"... it took a long time. But not as much\ntime as you spent crafting it!\n\nThank you,\nJohannes\n"}]}