{"thread":{"id":"54617","subject":"[PATCH 0/9] Fix color handling in git add -i","startedAt":"2020-11-10T23:42:24Z","lastAt":"2020-11-25T22:00:10Z","messageCount":66,"participants":["Johannes Schindelin via GitGitGadget","Jeff King","Johannes Schindelin","Phillip Wood","Philippe Blain","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"409568","messageId":"pull.785.git.1605051739.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":null,"subject":"[PATCH 0/9] Fix color handling in git add -i","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-10T23:42:10Z","receivedAt":"2020-11-10T23:42:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series started out as a tiny fix for a bug reported in \nhttps://lore.kernel.org/git/313B8999-1E99-4695-A20D-E48840C30879@gmail.com/.\nAnd then I only wanted to add a regression test to make sure that this does\nnot regress. And then I just wanted to ensure that it passes both with the\nPerl version of git add -i as well as with the built-in version.\n\nAnd in no time I was looking at a real patch series.\n\nJohannes Schindelin (9):\n add -i (built-in): do show an error message for incorrect inputs\n add -i (built-in): send error messages to stderr\n add -i: use `reset_color` consistently\n add -i (built-in): prevent the `reset` \"color\" from being configured\n add -i (built-in): use correct names to load color.diff.* config\n add -p (built-in): do not color the progress indicator separately\n add -i (built-in): use the same indentation as the Perl version\n add -i (Perl version): include indentation in the colored header\n add -i: verify in the tests that colors can be overridden\n\n add-interactive.c | 34 ++++++++++++++----------\n add-patch.c | 14 +++++-----\n git-add--interactive.perl | 6 ++---\n t/t3701-add-interactive.sh | 53 ++++++++++++++++++++++++++++++++++++++\n 4 files changed, 82 insertions(+), 25 deletions(-)\n\n\nbase-commit: e4d83eee9239207622e2b1cc43967da5051c189c\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-785%2Fdscho%2Ffix-add-i-colors-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-785/dscho/fix-add-i-colors-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/785\n-- \ngitgitgadget\n"},{"id":"409569","messageId":"6152122c04aacf8c3e3f1aae7b73995da6b9e354.1605051739.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.git.1605051739.gitgitgadget@gmail.com","subject":"[PATCH 1/9] add -i (built-in): do show an error message for incorrect inputs","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-10T23:42:11Z","receivedAt":"2020-11-10T23:42:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThere is a neat feature in `git add -i` where it allows users to select\nitems via unique prefixes.\n\nIn the built-in version of `git add -i`, we specifically sort the items\n(unless they are already sorted) and then perform a binary search to\nfigure out whether the input constitutes a unique prefix. Unfortunately,\nby mistake this code misidentifies matches even if the input string is\nnot actually a prefix of any item.\n\nFor example, in the initial menu, where there is a `status` and an\n`update` command, the input `tadaa` was mistaken as a prefix of\n`update`.\n\nLet's fix this by looking a bit closer whether the input is actually a\nprefix of the item at the found insert index.\n\nSigned-off-by: Johannes Schindelin \n---\n add-interactive.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 555c4abf32..8ca503d803 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -194,7 +194,8 @@ static ssize_t find_unique(const char *string, struct prefix_item_list *list)\n \telse if (index + 1 < list->sorted.nr &&\n \t\t starts_with(list->sorted.items[index + 1].string, string))\n \t\treturn -1;\n-\telse if (index < list->sorted.nr)\n+\telse if (index < list->sorted.nr &&\n+\t\t starts_with(list->sorted.items[index].string, string))\n \t\titem = list->sorted.items[index].util;\n \telse\n \t\treturn -1;\n-- \ngitgitgadget\n\n"},{"id":"409570","messageId":"068813912b9e5133f9d9bbacb239f7954788bf53.1605051739.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.git.1605051739.gitgitgadget@gmail.com","subject":"[PATCH 2/9] add -i (built-in): send error messages to stderr","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-10T23:42:12Z","receivedAt":"2020-11-10T23:42:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe Perl version of that command already does that since a301973641f\n(add -p: print errors in separate color, 2009-02-05). The built-in\nversion's development started by reimplementing the initial version from\n5cde71d64af (git-add --interactive, 2006-12-10) for simplicity, though,\nwhich still printed error messages to stdout.\n\nLet's fix that by imitating the Perl version's behavior in the built-in\nversion of that command.\n\nSigned-off-by: Johannes Schindelin \n---\n add-interactive.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 8ca503d803..0f24992ca4 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -365,7 +365,7 @@ static ssize_t list_and_choose(struct add_i_state *s,\n \n \t\t\tif (from < 0 || from >= items->items.nr ||\n \t\t\t (singleton && from + 1 != to)) {\n-\t\t\t\tcolor_fprintf_ln(stdout, s->error_color,\n+\t\t\t\tcolor_fprintf_ln(stderr, s->error_color,\n \t\t\t\t\t\t _(\"Huh (%s)?\"), p);\n \t\t\t\tbreak;\n \t\t\t} else if (singleton) {\n-- \ngitgitgadget\n\n"},{"id":"409571","messageId":"8df87e639562a93072b86b38da179059be05c231.1605051739.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.git.1605051739.gitgitgadget@gmail.com","subject":"[PATCH 7/9] add -i (built-in): use the same indentation as the Perl version","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-10T23:42:17Z","receivedAt":"2020-11-10T23:42:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen copying the spaces used to indent non-flat lists in `git add -i`,\none space was appended by mistake. This makes the output of the built-in\nversion of `git add -i` inconsistent with the Perl version. Let's adjust\nthe built-in version to produce the same output as the Perl version.\n\nSigned-off-by: Johannes Schindelin \n---\n add-interactive.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 9126684348..c298a8b80f 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -1137,7 +1137,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps)\n \tprint_file_item_data.color = data.color;\n \tprint_file_item_data.reset = data.reset;\n \n-\tstrbuf_addstr(&header, \" \");\n+\tstrbuf_addstr(&header, \" \");\n \tstrbuf_addf(&header, print_file_item_data.modified_fmt,\n \t\t _(\"staged\"), _(\"unstaged\"), _(\"path\"));\n \topts.list_opts.header = header.buf;\n-- \ngitgitgadget\n\n"},{"id":"409572","messageId":"42113e20dd7f2a80bbe09c01f1aedfcdfdbf6ae2.1605051739.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.git.1605051739.gitgitgadget@gmail.com","subject":"[PATCH 8/9] add -i (Perl version): include indentation in the colored header","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-10T23:42:18Z","receivedAt":"2020-11-10T23:42:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe header is formatted by padding each column heading with spaces up to\nthe length of 12 characters. These padding spaces are naturally included\nwhen coloring the entire header.\n\nHowever, the preceding five spaces indentation for non-flat lists were\n_not_ included in the Perl version, but _were_ included in the built-in\nversion. Let's adjust the former to align with the latter's behavior.\n\nSigned-off-by: Johannes Schindelin \n---\n git-add--interactive.perl | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex e713fe3d02..adbac2bc6d 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -483,10 +483,8 @@ sub list_and_choose {\n \t\tmy $last_lf = 0;\n \n \t\tif ($opts->{HEADER}) {\n-\t\t\tif (!$opts->{LIST_FLAT}) {\n-\t\t\t\tprint \" \";\n-\t\t\t}\n-\t\t\tprint colored $header_color, \"$opts->{HEADER}\\n\";\n+\t\t\tmy $indent = $opts->{LIST_FLAT} ? \"\" : \" \";\n+\t\t\tprint colored $header_color, \"$indent$opts->{HEADER}\\n\";\n \t\t}\n \t\tfor ($i = 0; $i < @stuff; $i++) {\n \t\t\tmy $chosen = $chosen[$i] ? '*' : ' ';\n-- \ngitgitgadget\n\n"},{"id":"409573","messageId":"38355ec98f04783367d74e38cda3ce5d6632c7ac.1605051739.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.git.1605051739.gitgitgadget@gmail.com","subject":"[PATCH 9/9] add -i: verify in the tests that colors can be overridden","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-10T23:42:19Z","receivedAt":"2020-11-10T23:42:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nNow that the Perl version produces the same output as the built-in\nversion (mostly fixing bugs in the latter), let's add a regression test\nto verify that it stays this way.\n\nNote that we only `grep` for the colored error message instead of\nverifying that the entire `stderr` consists of just this one line: when\nrunning the test script via `dash`, using the `-x` option to trace the\ncommands, the sub-shell in `force_color` causes those commands to be\ntraced into `err.raw` because we set, but do not export\n`BASH_XTRACEFD`).\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3701-add-interactive.sh | 53 ++++++++++++++++++++++++++++++++++++++\n 1 file changed, 53 insertions(+)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex ca04fac417..28549a41a2 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -589,6 +589,59 @@ test_expect_success 'diffs can be colorized' '\n \tgrep \"$(printf \"\\\\033\")\" output\n '\n \n+test_expect_success 'colors can be overridden' '\n+\ttest_config color.interactive.header red &&\n+\ttest_config color.interactive.help green &&\n+\ttest_config color.interactive.prompt yellow &&\n+\ttest_config color.interactive.error blue &&\n+\ttest_config color.diff.frag magenta &&\n+\ttest_config color.diff.context cyan &&\n+\ttest_config color.diff.old bold &&\n+\ttest_config color.diff.new \"bold red\" &&\n+\n+\tgit reset --hard &&\n+\ttest_when_finished \"git rm -f color-test\" &&\n+\ttest_write_lines context old more-context >color-test &&\n+\tgit add color-test &&\n+\ttest_write_lines context new more-context >color-test &&\n+\n+\techo trigger an error message >input &&\n+\tforce_color git add -i 2>err.raw err &&\n+\tgrep \"Huh (trigger)?\" err &&\n+\n+\ttest_write_lines patch color-test \"\" y quit >input &&\n+\tforce_color git add -i >actual.raw actual.decoded &&\n+\tsed \"/index [0-9a-f]*\\\\.\\\\.[0-9a-f]* 100644/d\" actual &&\n+\tcat >expect <<-\\EOF &&\n+\t staged unstaged path\n+\t 1: +3/-0 +1/-1 color-test\n+\n+\t*** Commands ***\n+\t 1: status\t 2: update\t 3: revert\t 4: add untracked\n+\t 5: patch\t 6: diff\t 7: quit\t 8: help\n+\tWhat now> staged unstaged path\n+\t 1: +3/-0 +1/-1 color-test\n+\tPatch update>> staged unstaged path\n+\t* 1: +3/-0 +1/-1 color-test\n+\tPatch update>> diff --git a/color-test b/color-test\n+\t--- a/color-test\n+\t+++ b/color-test\n+\t@@ -1,3 +1,3 @@\n+\t context\n+\t-old\n+\t+new\n+\t more-context\n+\t(1/1) Stage this hunk [y,n,q,a,d,e,?]? \n+\t*** Commands ***\n+\t 1: status\t 2: update\t 3: revert\t 4: add untracked\n+\t 5: patch\t 6: diff\t 7: quit\t 8: help\n+\tWhat now> Bye.\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '\n \tgit reset --hard &&\n \n-- \ngitgitgadget\n"},{"id":"409574","messageId":"85b0d27d76e31033d691b69b271db6ad033f9e29.1605051739.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.git.1605051739.gitgitgadget@gmail.com","subject":"[PATCH 5/9] add -i (built-in): use correct names to load color.diff.* config","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-10T23:42:15Z","receivedAt":"2020-11-10T23:42:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe builtin version of add-interactive mistakenly loads diff colors from\ncolor.interactive.* instead of color.diff.*. It also accidentally spells\n`frag` as `fraginfo`.\n\nLet's fix that.\n\nNote also that we don't respect the historical `diff.color.*`. The perl\nversion never did, and those have been deprecated since 2007.\n\nReported-by: Philippe Blain \nCo-authored-by: Jeff King \nSigned-off-by: Johannes Schindelin \n---\n add-interactive.c | 23 +++++++++++++----------\n 1 file changed, 13 insertions(+), 10 deletions(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex f3a1d7456e..9126684348 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -12,10 +12,10 @@\n #include \"prompt.h\"\n \n static void init_color(struct repository *r, struct add_i_state *s,\n-\t\t const char *slot_name, char *dst,\n+\t\t const char *section_and_slot, char *dst,\n \t\t const char *default_color)\n {\n-\tchar *key = xstrfmt(\"color.interactive.%s\", slot_name);\n+\tchar *key = xstrfmt(\"color.%s\", section_and_slot);\n \tconst char *value;\n \n \tif (!s->use_color)\n@@ -40,17 +40,20 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)\n \t\t\tgit_config_colorbool(\"color.interactive\", value);\n \ts->use_color = want_color(s->use_color);\n \n-\tinit_color(r, s, \"header\", s->header_color, GIT_COLOR_BOLD);\n-\tinit_color(r, s, \"help\", s->help_color, GIT_COLOR_BOLD_RED);\n-\tinit_color(r, s, \"prompt\", s->prompt_color, GIT_COLOR_BOLD_BLUE);\n-\tinit_color(r, s, \"error\", s->error_color, GIT_COLOR_BOLD_RED);\n-\tinit_color(r, s, \"fraginfo\", s->fraginfo_color,\n+\tinit_color(r, s, \"interactive.header\", s->header_color, GIT_COLOR_BOLD);\n+\tinit_color(r, s, \"interactive.help\", s->help_color, GIT_COLOR_BOLD_RED);\n+\tinit_color(r, s, \"interactive.prompt\", s->prompt_color,\n+\t\t GIT_COLOR_BOLD_BLUE);\n+\tinit_color(r, s, \"interactive.error\", s->error_color,\n+\t\t GIT_COLOR_BOLD_RED);\n+\n+\tinit_color(r, s, \"diff.frag\", s->fraginfo_color,\n \t\t diff_get_color(s->use_color, DIFF_FRAGINFO));\n-\tinit_color(r, s, \"context\", s->context_color,\n+\tinit_color(r, s, \"diff.context\", s->context_color,\n \t\tdiff_get_color(s->use_color, DIFF_CONTEXT));\n-\tinit_color(r, s, \"old\", s->file_old_color,\n+\tinit_color(r, s, \"diff.old\", s->file_old_color,\n \t\tdiff_get_color(s->use_color, DIFF_FILE_OLD));\n-\tinit_color(r, s, \"new\", s->file_new_color,\n+\tinit_color(r, s, \"diff.new\", s->file_new_color,\n \t\tdiff_get_color(s->use_color, DIFF_FILE_NEW));\n \n \tstrlcpy(s->reset_color,\n-- \ngitgitgadget\n\n"},{"id":"409575","messageId":"9a1ba71977e9dda4c2656dc2847106dd8b79a433.1605051739.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.git.1605051739.gitgitgadget@gmail.com","subject":"[PATCH 3/9] add -i: use `reset_color` consistently","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-10T23:42:13Z","receivedAt":"2020-11-10T23:42:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWe already maintain a list of colors in the `add_i_state`, therefore we\nshould use them.\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex be4cf6e9e5..33a24f58fe 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -667,7 +667,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\tif (len)\n \t\t\tstrbuf_add(out, p, len);\n \t\telse if (colored)\n-\t\t\tstrbuf_addf(out, \"%s\\n\", GIT_COLOR_RESET);\n+\t\t\tstrbuf_addf(out, \"%s\\n\", s->s.reset_color);\n \t\telse\n \t\t\tstrbuf_addch(out, '\\n');\n \t}\n@@ -1060,7 +1060,7 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)\n \t\t\t s->s.file_new_color :\n \t\t\t s->s.context_color);\n \t\tstrbuf_add(&s->colored, plain + current, eol - current);\n-\t\tstrbuf_addstr(&s->colored, GIT_COLOR_RESET);\n+\t\tstrbuf_addstr(&s->colored, s->s.reset_color);\n \t\tif (next > eol)\n \t\t\tstrbuf_add(&s->colored, plain + eol, next - eol);\n \t\tcurrent = next;\n-- \ngitgitgadget\n\n"},{"id":"409576","messageId":"059344bfaf40da90dbe951a58f826b1ed21b5e12.1605051739.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.git.1605051739.gitgitgadget@gmail.com","subject":"[PATCH 6/9] add -p (built-in): do not color the progress indicator separately","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-10T23:42:16Z","receivedAt":"2020-11-10T23:42:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe Perl version of this command colors the progress indicator and the\nprompt message in one go, let's do the same in the built-in version.\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 33a24f58fe..6f4c187238 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1456,15 +1456,15 @@ static int patch_update_file(struct add_p_state *s,\n \t\telse\n \t\t\tprompt_mode_type = PROMPT_HUNK;\n \n-\t\tcolor_fprintf(stdout, s->s.prompt_color,\n-\t\t\t \"(%\"PRIuMAX\"/%\"PRIuMAX\") \",\n+\t\tprintf(\"%s(%\"PRIuMAX\"/%\"PRIuMAX\") \", s->s.prompt_color,\n \t\t\t (uintmax_t)hunk_index + 1,\n \t\t\t (uintmax_t)(file_diff->hunk_nr\n \t\t\t\t\t\t? file_diff->hunk_nr\n \t\t\t\t\t\t: 1));\n-\t\tcolor_fprintf(stdout, s->s.prompt_color,\n-\t\t\t _(s->mode->prompt_mode[prompt_mode_type]),\n-\t\t\t s->buf.buf);\n+\t\tprintf(_(s->mode->prompt_mode[prompt_mode_type]),\n+\t\t s->buf.buf);\n+\t\tif (*s->s.reset_color)\n+\t\t\tfputs(s->s.reset_color, stdout);\n \t\tfflush(stdout);\n \t\tif (read_single_character(s) == EOF)\n \t\t\tbreak;\n-- \ngitgitgadget\n\n"},{"id":"409577","messageId":"b48c878fad2966063a44a31034184af4386f8511.1605051739.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.git.1605051739.gitgitgadget@gmail.com","subject":"[PATCH 4/9] add -i (built-in): prevent the `reset` \"color\" from being configured","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-10T23:42:14Z","receivedAt":"2020-11-10T23:42:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe Perl version of that command sneakily uses `git config --get-color`\nto figure out the ANSI sequence to reset the color, but passes the empty\nstring and therefore cannot actually match any config entry.\n\nThis was missed when re-implementing the command as a built-in command.\nLet's fix this, preventing the `reset` sequence from being overridden\nvia the config.\n\nSigned-off-by: Johannes Schindelin \n---\n add-interactive.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 0f24992ca4..f3a1d7456e 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -44,7 +44,6 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)\n \tinit_color(r, s, \"help\", s->help_color, GIT_COLOR_BOLD_RED);\n \tinit_color(r, s, \"prompt\", s->prompt_color, GIT_COLOR_BOLD_BLUE);\n \tinit_color(r, s, \"error\", s->error_color, GIT_COLOR_BOLD_RED);\n-\tinit_color(r, s, \"reset\", s->reset_color, GIT_COLOR_RESET);\n \tinit_color(r, s, \"fraginfo\", s->fraginfo_color,\n \t\t diff_get_color(s->use_color, DIFF_FRAGINFO));\n \tinit_color(r, s, \"context\", s->context_color,\n@@ -54,6 +53,9 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)\n \tinit_color(r, s, \"new\", s->file_new_color,\n \t\tdiff_get_color(s->use_color, DIFF_FILE_NEW));\n \n+\tstrlcpy(s->reset_color,\n+\t\ts->use_color ? GIT_COLOR_RESET : \"\", COLOR_MAXLEN);\n+\n \tFREE_AND_NULL(s->interactive_diff_filter);\n \tgit_config_get_string(\"interactive.difffilter\",\n \t\t\t &s->interactive_diff_filter);\n-- \ngitgitgadget\n\n"},{"id":"409580","messageId":"20201111023549.GB806755@coredump.intra.peff.net","threadId":"54617","inReplyTo":"38355ec98f04783367d74e38cda3ce5d6632c7ac.1605051739.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 9/9] add -i: verify in the tests that colors can be overridden","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-11T02:35:49Z","receivedAt":"2020-11-11T02:35:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 10, 2020 at 11:42:19PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n> \n> Now that the Perl version produces the same output as the built-in\n> version (mostly fixing bugs in the latter), let's add a regression test\n> to verify that it stays this way.\n> \n> Note that we only `grep` for the colored error message instead of\n> verifying that the entire `stderr` consists of just this one line: when\n> running the test script via `dash`, using the `-x` option to trace the\n> commands, the sub-shell in `force_color` causes those commands to be\n> traced into `err.raw` because we set, but do not export\n> `BASH_XTRACEFD`).\n\nYour reasoning here confuses me. If we are using dash, then surely\nBASH_XTRACEFD does not matter either way? Perhaps the subshell is\nimportant if we are running bash, though.\n\nEither way, being forgiving to the use of \"-x\" is a nice convenience and\nI approve.\n\n> +test_expect_success 'colors can be overridden' '\n> +\ttest_config color.interactive.header red &&\n> +\ttest_config color.interactive.help green &&\n> +\ttest_config color.interactive.prompt yellow &&\n> +\ttest_config color.interactive.error blue &&\n> +\ttest_config color.diff.frag magenta &&\n> +\ttest_config color.diff.context cyan &&\n> +\ttest_config color.diff.old bold &&\n> +\ttest_config color.diff.new \"bold red\" &&\n\nSince you are being so thorough, and since it is already in the output,\nmaybe color color.diff.meta, too? Like:\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 28549a41a2..cca866c8f4 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -594,6 +594,7 @@ test_expect_success 'colors can be overridden' '\n \ttest_config color.interactive.help green &&\n \ttest_config color.interactive.prompt yellow &&\n \ttest_config color.interactive.error blue &&\n+\ttest_config color.diff.meta italic &&\n \ttest_config color.diff.frag magenta &&\n \ttest_config color.diff.context cyan &&\n \ttest_config color.diff.old bold &&\n@@ -625,9 +626,9 @@ test_expect_success 'colors can be overridden' '\n \t 1: +3/-0 +1/-1 color-test\n \tPatch update>> staged unstaged path\n \t* 1: +3/-0 +1/-1 color-test\n-\tPatch update>> diff --git a/color-test b/color-test\n-\t--- a/color-test\n-\t+++ b/color-test\n+\tPatch update>> diff --git a/color-test b/color-test\n+\t--- a/color-test\n+\t+++ b/color-test\n \t@@ -1,3 +1,3 @@\n \t context\n \t-old\n\n-Peff\n"},{"id":"409581","messageId":"20201111023636.GC806755@coredump.intra.peff.net","threadId":"54617","inReplyTo":"pull.785.git.1605051739.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/9] Fix color handling in git add -i","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-11T02:36:36Z","receivedAt":"2020-11-11T02:36:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 10, 2020 at 11:42:10PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> This patch series started out as a tiny fix for a bug reported in \n> https://lore.kernel.org/git/313B8999-1E99-4695-A20D-E48840C30879@gmail.com/.\n> And then I only wanted to add a regression test to make sure that this does\n> not regress. And then I just wanted to ensure that it passes both with the\n> Perl version of git add -i as well as with the built-in version.\n> \n> And in no time I was looking at a real patch series.\n\nOK, I see now why you wanted to roll my work in. There was quite a bit\nmore here than I expected. :)\n\n> Johannes Schindelin (9):\n> add -i (built-in): do show an error message for incorrect inputs\n> add -i (built-in): send error messages to stderr\n> add -i: use `reset_color` consistently\n> add -i (built-in): prevent the `reset` \"color\" from being configured\n> add -i (built-in): use correct names to load color.diff.* config\n> add -p (built-in): do not color the progress indicator separately\n> add -i (built-in): use the same indentation as the Perl version\n> add -i (Perl version): include indentation in the colored header\n> add -i: verify in the tests that colors can be overridden\n\nThey all look good to me. I left some minor comments on the final patch.\n\n-Peff\n"},{"id":"409608","messageId":"pull.785.v2.git.1605097704.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.git.1605051739.gitgitgadget@gmail.com","subject":"[PATCH v2 00/11] Fix color handling in git add -i","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-11T12:28:13Z","receivedAt":"2020-11-11T12:28:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series started out as a tiny fix for a bug reported by Philippe\nBlain in \nhttps://lore.kernel.org/git/313B8999-1E99-4695-A20D-E48840C30879@gmail.com/.\nAnd then I only wanted to add a regression test to make sure that this does\nnot regress. And then I just wanted to ensure that it passes both with the\nPerl version of git add -i as well as with the built-in version.\n\nAnd in no time I was looking at a real patch series.\n\nChanges since v1:\n\n * The regression test now actually exercises the re-coloring (that is the\n primary purpose of git add -p looking at the color.diff.* variables).\n * The way the built-in git add -p renders hunk headers of split hunks was\n aligned with how the Perl version does things.\n * We now consistently prefer color.diff.context over color.diff.plain, no\n matter whether using the Perl or the built-in version of git add -p.\n * The commit message for the regression test no longer confuses readers by\n mentioning dash.\n * The regression test was structured a bit better, first testing error\n message coloring, then the menu in git add -i and then the diff coloring\n in git add -p.\n\nJohannes Schindelin (11):\n add -i (built-in): do show an error message for incorrect inputs\n add -i (built-in): send error messages to stderr\n add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk\n headers\n add -i: use `reset_color` consistently\n add -i (built-in): prevent the `reset` \"color\" from being configured\n add -i (built-in): use correct names to load color.diff.* config\n add -p (built-in): do not color the progress indicator separately\n add -i (built-in): use the same indentation as the Perl version\n add -i (Perl version): include indentation in the colored header\n add -p: prefer color.diff.context over color.diff.plain\n add -i: verify in the tests that colors can be overridden\n\n add-interactive.c | 38 ++++++++++-------\n add-patch.c | 25 +++++++-----\n git-add--interactive.perl | 12 +++---\n t/t3701-add-interactive.sh | 84 ++++++++++++++++++++++++++++++++++++++\n 4 files changed, 127 insertions(+), 32 deletions(-)\n\n\nbase-commit: e4d83eee9239207622e2b1cc43967da5051c189c\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-785%2Fdscho%2Ffix-add-i-colors-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-785/dscho/fix-add-i-colors-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/785\n\nRange-diff vs v1:\n\n 1: 6152122c04 = 1: 6152122c04 add -i (built-in): do show an error message for incorrect inputs\n 2: 068813912b = 2: 068813912b add -i (built-in): send error messages to stderr\n -: ---------- > 3: 98deb538d9 add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headers\n 3: 9a1ba71977 = 4: c857c44932 add -i: use `reset_color` consistently\n 4: b48c878fad = 5: 337b45cad8 add -i (built-in): prevent the `reset` \"color\" from being configured\n 5: 85b0d27d76 = 6: dcd2ffc458 add -i (built-in): use correct names to load color.diff.* config\n 6: 059344bfaf = 7: 73b6d60a80 add -p (built-in): do not color the progress indicator separately\n 7: 8df87e6395 = 8: 91ded2fbbe add -i (built-in): use the same indentation as the Perl version\n 8: 42113e20dd = 9: 304614751e add -i (Perl version): include indentation in the colored header\n -: ---------- > 10: 48d8e0badf add -p: prefer color.diff.context over color.diff.plain\n 9: 38355ec98f ! 11: c94abff72f add -i: verify in the tests that colors can be overridden\n @@ Commit message\n \n Note that we only `grep` for the colored error message instead of\n verifying that the entire `stderr` consists of just this one line: when\n - running the test script via `dash`, using the `-x` option to trace the\n + running the test script using the `-x` option to trace the\n commands, the sub-shell in `force_color` causes those commands to be\n - traced into `err.raw` because we set, but do not export\n - `BASH_XTRACEFD`).\n + traced into `err.raw` (unless running in Bash where we set the\n + `BASH_XTRACEFD` variable to avoid that).\n \n + Also note that the color reset in the `+new`\n + line might look funny and unnecessary, as the corresponding `old` line\n + does not reset the color after the diff marker only to turn the color\n + back on right away.\n +\n + However, this is a (necessary) side effect of the white-space check: in\n + `emit_line_ws_markup()`, we first emit the diff marker via\n + `emit_line_0()` and then the rest of the line via `ws_check_emit()`. To\n + leave them somewhat decoupled, the color has to be reset after the diff\n + marker to allow for the rest of the line to start with another color (or\n + inverted, in case of white-space issues).\n +\n + Finally, we have to simulate hunk editing: the `git add -p` command\n + cannot rely on the internal diff machinery for coloring after letting\n + the user edit a hunk; It has to \"re-color\" the edited hunk. This is the\n + primary reason why that command is interested in the exact values of the\n + `color.diff.*` settings in the first place. To test this re-coloring, we\n + therefore have to pretend to edit a hunk and then show that hunk in the\n + regression test.\n +\n + Co-authored-by: Jeff King \n Signed-off-by: Johannes Schindelin \n \n ## t/t3701-add-interactive.sh ##\n @@ t/t3701-add-interactive.sh: test_expect_success 'diffs can be colorized' '\n '\n \n +test_expect_success 'colors can be overridden' '\n -+\ttest_config color.interactive.header red &&\n -+\ttest_config color.interactive.help green &&\n -+\ttest_config color.interactive.prompt yellow &&\n -+\ttest_config color.interactive.error blue &&\n -+\ttest_config color.diff.frag magenta &&\n -+\ttest_config color.diff.context cyan &&\n -+\ttest_config color.diff.old bold &&\n -+\ttest_config color.diff.new \"bold red\" &&\n -+\n +\tgit reset --hard &&\n +\ttest_when_finished \"git rm -f color-test\" &&\n +\ttest_write_lines context old more-context >color-test &&\n +\tgit add color-test &&\n -+\ttest_write_lines context new more-context >color-test &&\n ++\ttest_write_lines context new more-context another-one >color-test &&\n +\n +\techo trigger an error message >input &&\n -+\tforce_color git add -i 2>err.raw err.raw err &&\n +\tgrep \"Huh (trigger)?\" err &&\n +\n -+\ttest_write_lines patch color-test \"\" y quit >input &&\n -+\tforce_color git add -i >actual.raw actual.decoded &&\n -+\tsed \"/index [0-9a-f]*\\\\.\\\\.[0-9a-f]* 100644/d\" actual &&\n ++\ttest_write_lines help quit >input &&\n ++\tforce_color git \\\n ++\t\t-c color.interactive.header=red \\\n ++\t\t-c color.interactive.help=green \\\n ++\t\t-c color.interactive.prompt=yellow \\\n ++\t\tadd -i >actual.raw actual &&\n +\tcat >expect <<-\\EOF &&\n +\t staged unstaged path\n -+\t 1: +3/-0 +1/-1 color-test\n ++\t 1: +3/-0 +2/-1 color-test\n +\n +\t*** Commands ***\n +\t 1: status\t 2: update\t 3: revert\t 4: add untracked\n +\t 5: patch\t 6: diff\t 7: quit\t 8: help\n -+\tWhat now> staged unstaged path\n -+\t 1: +3/-0 +1/-1 color-test\n -+\tPatch update>> staged unstaged path\n -+\t* 1: +3/-0 +1/-1 color-test\n -+\tPatch update>> diff --git a/color-test b/color-test\n -+\t--- a/color-test\n -+\t+++ b/color-test\n -+\t@@ -1,3 +1,3 @@\n -+\t context\n -+\t-old\n -+\t+new\n -+\t more-context\n -+\t(1/1) Stage this hunk [y,n,q,a,d,e,?]? \n ++\tWhat now> status - show paths with changes\n ++\tupdate - add working tree state to the staged set of changes\n ++\trevert - revert staged set of changes back to the HEAD version\n ++\tpatch - pick hunks and update selectively\n ++\tdiff - view diff between HEAD and index\n ++\tadd untracked - add contents of untracked files to the staged set of changes\n +\t*** Commands ***\n +\t 1: status\t 2: update\t 3: revert\t 4: add untracked\n +\t 5: patch\t 6: diff\t 7: quit\t 8: help\n +\tWhat now> Bye.\n +\tEOF\n ++\ttest_cmp expect actual &&\n ++\n ++\t: exercise recolor_hunk by editing and then look at the hunk again &&\n ++\ttest_write_lines s e K q >input &&\n ++\tforce_color git \\\n ++\t\t-c color.interactive.prompt=yellow \\\n ++\t\t-c color.diff.meta=italic \\\n ++\t\t-c color.diff.frag=magenta \\\n ++\t\t-c color.diff.context=cyan \\\n ++\t\t-c color.diff.old=bold \\\n ++\t\t-c color.diff.new=blue \\\n ++\t\t-c core.editor=touch \\\n ++\t\tadd -p >actual.raw actual.decoded &&\n ++\tsed \"s/index [0-9a-f]*\\\\.\\\\.[0-9a-f]* 100644//\" actual &&\n ++\tcat >expect <<-\\EOF &&\n ++\tdiff --git a/color-test b/color-test\n ++\t\n ++\t--- a/color-test\n ++\t+++ b/color-test\n ++\t@@ -1,3 +1,4 @@\n ++\t context\n ++\t-old\n ++\t+new\n ++\t more-context\n ++\t+another-one\n ++\t(1/1) Stage this hunk [y,n,q,a,d,s,e,?]? Split into 2 hunks.\n ++\t@@ -1,3 +1,3 @@\n ++\t context\n ++\t-old\n ++\t+new\n ++\t more-context\n ++\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? @@ -3 +3,2 @@\n ++\t more-context\n ++\t+another-one\n ++\t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? @@ -1,3 +1,3 @@\n ++\t context\n ++\t-old\n ++\t+new\n ++\t more-context\n ++\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? \n ++\tEOF\n +\ttest_cmp expect actual\n +'\n +\n\n-- \ngitgitgadget\n"},{"id":"409607","messageId":"6152122c04aacf8c3e3f1aae7b73995da6b9e354.1605097704.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v2.git.1605097704.gitgitgadget@gmail.com","subject":"[PATCH v2 01/11] add -i (built-in): do show an error message for incorrect inputs","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-11T12:28:14Z","receivedAt":"2020-11-11T12:28:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThere is a neat feature in `git add -i` where it allows users to select\nitems via unique prefixes.\n\nIn the built-in version of `git add -i`, we specifically sort the items\n(unless they are already sorted) and then perform a binary search to\nfigure out whether the input constitutes a unique prefix. Unfortunately,\nby mistake this code misidentifies matches even if the input string is\nnot actually a prefix of any item.\n\nFor example, in the initial menu, where there is a `status` and an\n`update` command, the input `tadaa` was mistaken as a prefix of\n`update`.\n\nLet's fix this by looking a bit closer whether the input is actually a\nprefix of the item at the found insert index.\n\nSigned-off-by: Johannes Schindelin \n---\n add-interactive.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 555c4abf32..8ca503d803 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -194,7 +194,8 @@ static ssize_t find_unique(const char *string, struct prefix_item_list *list)\n \telse if (index + 1 < list->sorted.nr &&\n \t\t starts_with(list->sorted.items[index + 1].string, string))\n \t\treturn -1;\n-\telse if (index < list->sorted.nr)\n+\telse if (index < list->sorted.nr &&\n+\t\t starts_with(list->sorted.items[index].string, string))\n \t\titem = list->sorted.items[index].util;\n \telse\n \t\treturn -1;\n-- \ngitgitgadget\n\n"},{"id":"409609","messageId":"73b6d60a801766605cde9403d7a9e8451ef99e11.1605097704.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v2.git.1605097704.gitgitgadget@gmail.com","subject":"[PATCH v2 07/11] add -p (built-in): do not color the progress indicator separately","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-11T12:28:20Z","receivedAt":"2020-11-11T12:28:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe Perl version of this command colors the progress indicator and the\nprompt message in one go, let's do the same in the built-in version.\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex bf89c43145..2fad92ca37 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1461,15 +1461,15 @@ static int patch_update_file(struct add_p_state *s,\n \t\telse\n \t\t\tprompt_mode_type = PROMPT_HUNK;\n \n-\t\tcolor_fprintf(stdout, s->s.prompt_color,\n-\t\t\t \"(%\"PRIuMAX\"/%\"PRIuMAX\") \",\n+\t\tprintf(\"%s(%\"PRIuMAX\"/%\"PRIuMAX\") \", s->s.prompt_color,\n \t\t\t (uintmax_t)hunk_index + 1,\n \t\t\t (uintmax_t)(file_diff->hunk_nr\n \t\t\t\t\t\t? file_diff->hunk_nr\n \t\t\t\t\t\t: 1));\n-\t\tcolor_fprintf(stdout, s->s.prompt_color,\n-\t\t\t _(s->mode->prompt_mode[prompt_mode_type]),\n-\t\t\t s->buf.buf);\n+\t\tprintf(_(s->mode->prompt_mode[prompt_mode_type]),\n+\t\t s->buf.buf);\n+\t\tif (*s->s.reset_color)\n+\t\t\tfputs(s->s.reset_color, stdout);\n \t\tfflush(stdout);\n \t\tif (read_single_character(s) == EOF)\n \t\t\tbreak;\n-- \ngitgitgadget\n\n"},{"id":"409610","messageId":"91ded2fbbe7197d8ad46005ff3eadeb2a5a69e18.1605097704.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v2.git.1605097704.gitgitgadget@gmail.com","subject":"[PATCH v2 08/11] add -i (built-in): use the same indentation as the Perl version","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-11T12:28:21Z","receivedAt":"2020-11-11T12:28:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen copying the spaces used to indent non-flat lists in `git add -i`,\none space was appended by mistake. This makes the output of the built-in\nversion of `git add -i` inconsistent with the Perl version. Let's adjust\nthe built-in version to produce the same output as the Perl version.\n\nSigned-off-by: Johannes Schindelin \n---\n add-interactive.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 9126684348..c298a8b80f 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -1137,7 +1137,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps)\n \tprint_file_item_data.color = data.color;\n \tprint_file_item_data.reset = data.reset;\n \n-\tstrbuf_addstr(&header, \" \");\n+\tstrbuf_addstr(&header, \" \");\n \tstrbuf_addf(&header, print_file_item_data.modified_fmt,\n \t\t _(\"staged\"), _(\"unstaged\"), _(\"path\"));\n \topts.list_opts.header = header.buf;\n-- \ngitgitgadget\n\n"},{"id":"409611","messageId":"48d8e0badfec5f0e576868f7a406ed7ede6c7200.1605097704.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v2.git.1605097704.gitgitgadget@gmail.com","subject":"[PATCH v2 10/11] add -p: prefer color.diff.context over color.diff.plain","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-11T12:28:23Z","receivedAt":"2020-11-11T12:28:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nGit's diff machinery allows users to override the colors to use in\ndiffs, even the plain-colored context lines. As of 8dbf3eb6850 (diff.h:\nrename DIFF_PLAIN color slot to DIFF_CONTEXT, 2015-05-27), the preferred\nname of the config setting is `color.diff.context`, although Git still\nallows `color.diff.plain`.\n\nIn the context of `git add -p`, this logic is a bit hard to replicate:\n`git_diff_basic_config()` reads all config values sequentially and if it\nsees _any_ `color.diff.context` or `color.diff.plain`, it accepts the\nnew color. The Perl version of `git add -p` needs to go through `git\nconfig --get-color`, though, which allows only one key to be specified.\nThe same goes for the built-in version of `git add -p`, which has to go\nthrough `repo_config_get_value()`.\n\nThe best we can do here is to look for `.context` and if none is found,\nfall back to looking for `.plain`, and if still not found, fall back to\nthe hard-coded default (which in this case is simply the empty string,\nas context lines are typically rendered without colored).\n\nThis still needs to inconsistencies when both config names are used: the\ninitial diff will be colored by the diff machinery. Once edited by a\nuser, a hunk has to be re-colored by `git add -p`, though, which would\nthen use the other setting to color the context lines.\n\nIn practice, this is not _all_ that bad. The `git config` manual says\nthis in the `color.diff.`:\n\n\t`context` (context text - `plain` is a historical synonym)\n\nWe should therefore assume that users use either one or the other, but\nnot both names. Besides, it is relatively uncommon to look at a hunk\nafter editing it because it is immediately staged by default.\n\nSigned-off-by: Johannes Schindelin \n---\n add-interactive.c | 6 ++++--\n git-add--interactive.perl | 6 +++---\n 2 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex c298a8b80f..54dfdc56f5 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -49,8 +49,10 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)\n \n \tinit_color(r, s, \"diff.frag\", s->fraginfo_color,\n \t\t diff_get_color(s->use_color, DIFF_FRAGINFO));\n-\tinit_color(r, s, \"diff.context\", s->context_color,\n-\t\tdiff_get_color(s->use_color, DIFF_CONTEXT));\n+\tinit_color(r, s, \"diff.context\", s->context_color, \"fall back\");\n+\tif (!strcmp(s->context_color, \"fall back\"))\n+\t\tinit_color(r, s, \"diff.plain\", s->context_color,\n+\t\t\t diff_get_color(s->use_color, DIFF_CONTEXT));\n \tinit_color(r, s, \"diff.old\", s->file_old_color,\n \t\tdiff_get_color(s->use_color, DIFF_FILE_OLD));\n \tinit_color(r, s, \"diff.new\", s->file_new_color,\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex adbac2bc6d..bc3a1e8eff 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -30,9 +30,9 @@\n \t$diff_use_color ? (\n \t\t$repo->get_color('color.diff.frag', 'cyan'),\n \t) : ();\n-my ($diff_plain_color) =\n+my ($diff_context_color) =\n \t$diff_use_color ? (\n-\t\t$repo->get_color('color.diff.plain', ''),\n+\t\t$repo->get_color($repo->config('color.diff.context') ? 'color.diff.context' : 'color.diff.plain', ''),\n \t) : ();\n my ($diff_old_color) =\n \t$diff_use_color ? (\n@@ -1046,7 +1046,7 @@ sub color_diff {\n \t\tcolored((/^@/ ? $fraginfo_color :\n \t\t\t /^\\+/ ? $diff_new_color :\n \t\t\t /^-/ ? $diff_old_color :\n-\t\t\t $diff_plain_color),\n+\t\t\t $diff_context_color),\n \t\t\t$_);\n \t} @_;\n }\n-- \ngitgitgadget\n\n"},{"id":"409612","messageId":"304614751ea1b1db80fbbbdc909b3d198c20313e.1605097704.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v2.git.1605097704.gitgitgadget@gmail.com","subject":"[PATCH v2 09/11] add -i (Perl version): include indentation in the colored header","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-11T12:28:22Z","receivedAt":"2020-11-11T12:28:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe header is formatted by padding each column heading with spaces up to\nthe length of 12 characters. These padding spaces are naturally included\nwhen coloring the entire header.\n\nHowever, the preceding five spaces indentation for non-flat lists were\n_not_ included in the Perl version, but _were_ included in the built-in\nversion. Let's adjust the former to align with the latter's behavior.\n\nSigned-off-by: Johannes Schindelin \n---\n git-add--interactive.perl | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex e713fe3d02..adbac2bc6d 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -483,10 +483,8 @@ sub list_and_choose {\n \t\tmy $last_lf = 0;\n \n \t\tif ($opts->{HEADER}) {\n-\t\t\tif (!$opts->{LIST_FLAT}) {\n-\t\t\t\tprint \" \";\n-\t\t\t}\n-\t\t\tprint colored $header_color, \"$opts->{HEADER}\\n\";\n+\t\t\tmy $indent = $opts->{LIST_FLAT} ? \"\" : \" \";\n+\t\t\tprint colored $header_color, \"$indent$opts->{HEADER}\\n\";\n \t\t}\n \t\tfor ($i = 0; $i < @stuff; $i++) {\n \t\t\tmy $chosen = $chosen[$i] ? '*' : ' ';\n-- \ngitgitgadget\n\n"},{"id":"409613","messageId":"dcd2ffc4586960386eb8a517ff9478a8975d428c.1605097704.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v2.git.1605097704.gitgitgadget@gmail.com","subject":"[PATCH v2 06/11] add -i (built-in): use correct names to load color.diff.* config","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-11T12:28:19Z","receivedAt":"2020-11-11T12:28:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe builtin version of add-interactive mistakenly loads diff colors from\ncolor.interactive.* instead of color.diff.*. It also accidentally spells\n`frag` as `fraginfo`.\n\nLet's fix that.\n\nNote also that we don't respect the historical `diff.color.*`. The perl\nversion never did, and those have been deprecated since 2007.\n\nReported-by: Philippe Blain \nCo-authored-by: Jeff King \nSigned-off-by: Johannes Schindelin \n---\n add-interactive.c | 23 +++++++++++++----------\n 1 file changed, 13 insertions(+), 10 deletions(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex f3a1d7456e..9126684348 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -12,10 +12,10 @@\n #include \"prompt.h\"\n \n static void init_color(struct repository *r, struct add_i_state *s,\n-\t\t const char *slot_name, char *dst,\n+\t\t const char *section_and_slot, char *dst,\n \t\t const char *default_color)\n {\n-\tchar *key = xstrfmt(\"color.interactive.%s\", slot_name);\n+\tchar *key = xstrfmt(\"color.%s\", section_and_slot);\n \tconst char *value;\n \n \tif (!s->use_color)\n@@ -40,17 +40,20 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)\n \t\t\tgit_config_colorbool(\"color.interactive\", value);\n \ts->use_color = want_color(s->use_color);\n \n-\tinit_color(r, s, \"header\", s->header_color, GIT_COLOR_BOLD);\n-\tinit_color(r, s, \"help\", s->help_color, GIT_COLOR_BOLD_RED);\n-\tinit_color(r, s, \"prompt\", s->prompt_color, GIT_COLOR_BOLD_BLUE);\n-\tinit_color(r, s, \"error\", s->error_color, GIT_COLOR_BOLD_RED);\n-\tinit_color(r, s, \"fraginfo\", s->fraginfo_color,\n+\tinit_color(r, s, \"interactive.header\", s->header_color, GIT_COLOR_BOLD);\n+\tinit_color(r, s, \"interactive.help\", s->help_color, GIT_COLOR_BOLD_RED);\n+\tinit_color(r, s, \"interactive.prompt\", s->prompt_color,\n+\t\t GIT_COLOR_BOLD_BLUE);\n+\tinit_color(r, s, \"interactive.error\", s->error_color,\n+\t\t GIT_COLOR_BOLD_RED);\n+\n+\tinit_color(r, s, \"diff.frag\", s->fraginfo_color,\n \t\t diff_get_color(s->use_color, DIFF_FRAGINFO));\n-\tinit_color(r, s, \"context\", s->context_color,\n+\tinit_color(r, s, \"diff.context\", s->context_color,\n \t\tdiff_get_color(s->use_color, DIFF_CONTEXT));\n-\tinit_color(r, s, \"old\", s->file_old_color,\n+\tinit_color(r, s, \"diff.old\", s->file_old_color,\n \t\tdiff_get_color(s->use_color, DIFF_FILE_OLD));\n-\tinit_color(r, s, \"new\", s->file_new_color,\n+\tinit_color(r, s, \"diff.new\", s->file_new_color,\n \t\tdiff_get_color(s->use_color, DIFF_FILE_NEW));\n \n \tstrlcpy(s->reset_color,\n-- \ngitgitgadget\n\n"},{"id":"409614","messageId":"c94abff72fbbd4722c001d5ece1d03a7b48b8d51.1605097704.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v2.git.1605097704.gitgitgadget@gmail.com","subject":"[PATCH v2 11/11] add -i: verify in the tests that colors can be overridden","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-11T12:28:24Z","receivedAt":"2020-11-11T12:28:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nNow that the Perl version produces the same output as the built-in\nversion (mostly fixing bugs in the latter), let's add a regression test\nto verify that it stays this way.\n\nNote that we only `grep` for the colored error message instead of\nverifying that the entire `stderr` consists of just this one line: when\nrunning the test script using the `-x` option to trace the\ncommands, the sub-shell in `force_color` causes those commands to be\ntraced into `err.raw` (unless running in Bash where we set the\n`BASH_XTRACEFD` variable to avoid that).\n\nAlso note that the color reset in the `+new`\nline might look funny and unnecessary, as the corresponding `old` line\ndoes not reset the color after the diff marker only to turn the color\nback on right away.\n\nHowever, this is a (necessary) side effect of the white-space check: in\n`emit_line_ws_markup()`, we first emit the diff marker via\n`emit_line_0()` and then the rest of the line via `ws_check_emit()`. To\nleave them somewhat decoupled, the color has to be reset after the diff\nmarker to allow for the rest of the line to start with another color (or\ninverted, in case of white-space issues).\n\nFinally, we have to simulate hunk editing: the `git add -p` command\ncannot rely on the internal diff machinery for coloring after letting\nthe user edit a hunk; It has to \"re-color\" the edited hunk. This is the\nprimary reason why that command is interested in the exact values of the\n`color.diff.*` settings in the first place. To test this re-coloring, we\ntherefore have to pretend to edit a hunk and then show that hunk in the\nregression test.\n\nCo-authored-by: Jeff King \nSigned-off-by: Johannes Schindelin \n---\n t/t3701-add-interactive.sh | 84 ++++++++++++++++++++++++++++++++++++++\n 1 file changed, 84 insertions(+)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex ca04fac417..cc3f434a97 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -589,6 +589,90 @@ test_expect_success 'diffs can be colorized' '\n \tgrep \"$(printf \"\\\\033\")\" output\n '\n \n+test_expect_success 'colors can be overridden' '\n+\tgit reset --hard &&\n+\ttest_when_finished \"git rm -f color-test\" &&\n+\ttest_write_lines context old more-context >color-test &&\n+\tgit add color-test &&\n+\ttest_write_lines context new more-context another-one >color-test &&\n+\n+\techo trigger an error message >input &&\n+\tforce_color git \\\n+\t\t-c color.interactive.error=blue \\\n+\t\tadd -i 2>err.raw err &&\n+\tgrep \"Huh (trigger)?\" err &&\n+\n+\ttest_write_lines help quit >input &&\n+\tforce_color git \\\n+\t\t-c color.interactive.header=red \\\n+\t\t-c color.interactive.help=green \\\n+\t\t-c color.interactive.prompt=yellow \\\n+\t\tadd -i >actual.raw actual &&\n+\tcat >expect <<-\\EOF &&\n+\t staged unstaged path\n+\t 1: +3/-0 +2/-1 color-test\n+\n+\t*** Commands ***\n+\t 1: status\t 2: update\t 3: revert\t 4: add untracked\n+\t 5: patch\t 6: diff\t 7: quit\t 8: help\n+\tWhat now> status - show paths with changes\n+\tupdate - add working tree state to the staged set of changes\n+\trevert - revert staged set of changes back to the HEAD version\n+\tpatch - pick hunks and update selectively\n+\tdiff - view diff between HEAD and index\n+\tadd untracked - add contents of untracked files to the staged set of changes\n+\t*** Commands ***\n+\t 1: status\t 2: update\t 3: revert\t 4: add untracked\n+\t 5: patch\t 6: diff\t 7: quit\t 8: help\n+\tWhat now> Bye.\n+\tEOF\n+\ttest_cmp expect actual &&\n+\n+\t: exercise recolor_hunk by editing and then look at the hunk again &&\n+\ttest_write_lines s e K q >input &&\n+\tforce_color git \\\n+\t\t-c color.interactive.prompt=yellow \\\n+\t\t-c color.diff.meta=italic \\\n+\t\t-c color.diff.frag=magenta \\\n+\t\t-c color.diff.context=cyan \\\n+\t\t-c color.diff.old=bold \\\n+\t\t-c color.diff.new=blue \\\n+\t\t-c core.editor=touch \\\n+\t\tadd -p >actual.raw actual.decoded &&\n+\tsed \"s/index [0-9a-f]*\\\\.\\\\.[0-9a-f]* 100644//\" actual &&\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/color-test b/color-test\n+\t\n+\t--- a/color-test\n+\t+++ b/color-test\n+\t@@ -1,3 +1,4 @@\n+\t context\n+\t-old\n+\t+new\n+\t more-context\n+\t+another-one\n+\t(1/1) Stage this hunk [y,n,q,a,d,s,e,?]? Split into 2 hunks.\n+\t@@ -1,3 +1,3 @@\n+\t context\n+\t-old\n+\t+new\n+\t more-context\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? @@ -3 +3,2 @@\n+\t more-context\n+\t+another-one\n+\t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? @@ -1,3 +1,3 @@\n+\t context\n+\t-old\n+\t+new\n+\t more-context\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? \n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '\n \tgit reset --hard &&\n \n-- \ngitgitgadget\n"},{"id":"409615","messageId":"337b45cad8b876e1811099f398bf0a05ade21acf.1605097704.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v2.git.1605097704.gitgitgadget@gmail.com","subject":"[PATCH v2 05/11] add -i (built-in): prevent the `reset` \"color\" from being configured","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-11T12:28:18Z","receivedAt":"2020-11-11T12:28:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe Perl version of that command sneakily uses `git config --get-color`\nto figure out the ANSI sequence to reset the color, but passes the empty\nstring and therefore cannot actually match any config entry.\n\nThis was missed when re-implementing the command as a built-in command.\nLet's fix this, preventing the `reset` sequence from being overridden\nvia the config.\n\nSigned-off-by: Johannes Schindelin \n---\n add-interactive.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 0f24992ca4..f3a1d7456e 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -44,7 +44,6 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)\n \tinit_color(r, s, \"help\", s->help_color, GIT_COLOR_BOLD_RED);\n \tinit_color(r, s, \"prompt\", s->prompt_color, GIT_COLOR_BOLD_BLUE);\n \tinit_color(r, s, \"error\", s->error_color, GIT_COLOR_BOLD_RED);\n-\tinit_color(r, s, \"reset\", s->reset_color, GIT_COLOR_RESET);\n \tinit_color(r, s, \"fraginfo\", s->fraginfo_color,\n \t\t diff_get_color(s->use_color, DIFF_FRAGINFO));\n \tinit_color(r, s, \"context\", s->context_color,\n@@ -54,6 +53,9 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)\n \tinit_color(r, s, \"new\", s->file_new_color,\n \t\tdiff_get_color(s->use_color, DIFF_FILE_NEW));\n \n+\tstrlcpy(s->reset_color,\n+\t\ts->use_color ? GIT_COLOR_RESET : \"\", COLOR_MAXLEN);\n+\n \tFREE_AND_NULL(s->interactive_diff_filter);\n \tgit_config_get_string(\"interactive.difffilter\",\n \t\t\t &s->interactive_diff_filter);\n-- \ngitgitgadget\n\n"},{"id":"409616","messageId":"068813912b9e5133f9d9bbacb239f7954788bf53.1605097704.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v2.git.1605097704.gitgitgadget@gmail.com","subject":"[PATCH v2 02/11] add -i (built-in): send error messages to stderr","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-11T12:28:15Z","receivedAt":"2020-11-11T12:28:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe Perl version of that command already does that since a301973641f\n(add -p: print errors in separate color, 2009-02-05). The built-in\nversion's development started by reimplementing the initial version from\n5cde71d64af (git-add --interactive, 2006-12-10) for simplicity, though,\nwhich still printed error messages to stdout.\n\nLet's fix that by imitating the Perl version's behavior in the built-in\nversion of that command.\n\nSigned-off-by: Johannes Schindelin \n---\n add-interactive.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 8ca503d803..0f24992ca4 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -365,7 +365,7 @@ static ssize_t list_and_choose(struct add_i_state *s,\n \n \t\t\tif (from < 0 || from >= items->items.nr ||\n \t\t\t (singleton && from + 1 != to)) {\n-\t\t\t\tcolor_fprintf_ln(stdout, s->error_color,\n+\t\t\t\tcolor_fprintf_ln(stderr, s->error_color,\n \t\t\t\t\t\t _(\"Huh (%s)?\"), p);\n \t\t\t\tbreak;\n \t\t\t} else if (singleton) {\n-- \ngitgitgadget\n\n"},{"id":"409617","messageId":"98deb538d91ba0ab485fa8d97cc6062bcc680938.1605097704.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v2.git.1605097704.gitgitgadget@gmail.com","subject":"[PATCH v2 03/11] add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headers","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-11T12:28:16Z","receivedAt":"2020-11-11T12:28:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn libxdiff, imitating GNU diff, the hunk headers only show the line\ncount if it is different from 1. When splitting hunks, the Perl version\nof `git add -p` already imitates this. Let's do the same in the built-in\nversion of said command.\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex be4cf6e9e5..b6d53229bb 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -661,9 +661,14 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\telse\n \t\t\tnew_offset += delta;\n \n-\t\tstrbuf_addf(out, \"@@ -%lu,%lu +%lu,%lu @@\",\n-\t\t\t old_offset, header->old_count,\n-\t\t\t new_offset, header->new_count);\n+\t\tstrbuf_addf(out, \"@@ -%lu\", old_offset);\n+\t\tif (header->old_count != 1)\n+\t\t\tstrbuf_addf(out, \",%lu\", header->old_count);\n+\t\tstrbuf_addf(out, \" +%lu\", new_offset);\n+\t\tif (header->new_count != 1)\n+\t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n+\t\tstrbuf_addstr(out, \" @@\");\n+\n \t\tif (len)\n \t\t\tstrbuf_add(out, p, len);\n \t\telse if (colored)\n-- \ngitgitgadget\n\n"},{"id":"409618","messageId":"c857c4493213bd7cc4d057487db2d1a74fa0bbd6.1605097704.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v2.git.1605097704.gitgitgadget@gmail.com","subject":"[PATCH v2 04/11] add -i: use `reset_color` consistently","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-11T12:28:17Z","receivedAt":"2020-11-11T12:28:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWe already maintain a list of colors in the `add_i_state`, therefore we\nshould use them.\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex b6d53229bb..bf89c43145 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -672,7 +672,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\tif (len)\n \t\t\tstrbuf_add(out, p, len);\n \t\telse if (colored)\n-\t\t\tstrbuf_addf(out, \"%s\\n\", GIT_COLOR_RESET);\n+\t\t\tstrbuf_addf(out, \"%s\\n\", s->s.reset_color);\n \t\telse\n \t\t\tstrbuf_addch(out, '\\n');\n \t}\n@@ -1065,7 +1065,7 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)\n \t\t\t s->s.file_new_color :\n \t\t\t s->s.context_color);\n \t\tstrbuf_add(&s->colored, plain + current, eol - current);\n-\t\tstrbuf_addstr(&s->colored, GIT_COLOR_RESET);\n+\t\tstrbuf_addstr(&s->colored, s->s.reset_color);\n \t\tif (next > eol)\n \t\t\tstrbuf_add(&s->colored, plain + eol, next - eol);\n \t\tcurrent = next;\n-- \ngitgitgadget\n\n"},{"id":"409635","messageId":"nycvar.QRO.7.76.6.2011111635140.18437@tvgsbejvaqbjf.bet","threadId":"54617","inReplyTo":"20201111023549.GB806755@coredump.intra.peff.net","subject":"Re: [PATCH 9/9] add -i: verify in the tests that colors can be overridden","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-11T15:53:13Z","receivedAt":"2020-11-11T15:53:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 10 Nov 2020, Jeff King wrote:\n\n> On Tue, Nov 10, 2020 at 11:42:19PM +0000, Johannes Schindelin via GitGitGadget wrote:\n>\n> > From: Johannes Schindelin \n> >\n> > Now that the Perl version produces the same output as the built-in\n> > version (mostly fixing bugs in the latter), let's add a regression test\n> > to verify that it stays this way.\n> >\n> > Note that we only `grep` for the colored error message instead of\n> > verifying that the entire `stderr` consists of just this one line: when\n> > running the test script via `dash`, using the `-x` option to trace the\n> > commands, the sub-shell in `force_color` causes those commands to be\n> > traced into `err.raw` because we set, but do not export\n> > `BASH_XTRACEFD`).\n>\n> Your reasoning here confuses me.\n\nSorry!\n\n> If we are using dash, then surely BASH_XTRACEFD does not matter either\n> way?\n\nIt kinda does, though. Dash _does_ respect the `BASH_XTRACEFD` variable,\nfunnily enough, but does not hand the settings through to sub-shells,\nwhereas Bash does.\n\n> Perhaps the subshell is important if we are running bash, though.\n\nThe most important thing, which I of course _failed_ to describe most\nprominently, is that _in general_ `-x` will mess up the `err` file, i.e.\nwhen running with anything but Bash. Therefoer we need the `grep` instead\nof a `test_cmp`, and that's what I tried to convey in v2.\n\n> Either way, being forgiving to the use of \"-x\" is a nice convenience and\n> I approve.\n>\n> > +test_expect_success 'colors can be overridden' '\n> > +\ttest_config color.interactive.header red &&\n> > +\ttest_config color.interactive.help green &&\n> > +\ttest_config color.interactive.prompt yellow &&\n> > +\ttest_config color.interactive.error blue &&\n> > +\ttest_config color.diff.frag magenta &&\n> > +\ttest_config color.diff.context cyan &&\n> > +\ttest_config color.diff.old bold &&\n> > +\ttest_config color.diff.new \"bold red\" &&\n>\n> Since you are being so thorough, and since it is already in the output,\n> maybe color color.diff.meta, too? Like:\n>\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index 28549a41a2..cca866c8f4 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -594,6 +594,7 @@ test_expect_success 'colors can be overridden' '\n> \ttest_config color.interactive.help green &&\n> \ttest_config color.interactive.prompt yellow &&\n> \ttest_config color.interactive.error blue &&\n> +\ttest_config color.diff.meta italic &&\n> \ttest_config color.diff.frag magenta &&\n> \ttest_config color.diff.context cyan &&\n> \ttest_config color.diff.old bold &&\n> @@ -625,9 +626,9 @@ test_expect_success 'colors can be overridden' '\n> \t 1: +3/-0 +1/-1 color-test\n> \tPatch update>> staged unstaged path\n> \t* 1: +3/-0 +1/-1 color-test\n> -\tPatch update>> diff --git a/color-test b/color-test\n> -\t--- a/color-test\n> -\t+++ b/color-test\n> +\tPatch update>> diff --git a/color-test b/color-test\n> +\t--- a/color-test\n> +\t+++ b/color-test\n> \t@@ -1,3 +1,3 @@\n> \t context\n> \t-old\n\nOh my. I really had tried to avoid going _this_ deep. The `.meta` setting\nis not even read by the interactive add command:\n\n\t$ git grep -w meta git-add--interactive.perl add-interactive.c \\\n\t\tadd-patch.c\n\ncomes up empty.\n\nThe reason is that we actually let the diff machinery do all the coloring.\nSo why, then, you might ask, do we bother to read those values in the\nfirst place?\n\nWell, you know, in `git add -p`, you can split hunks. That's still fine,\nwe basically just have to color the hunk header that we insert, and can\nreuse the original coloring of the remaining lines, because they are\nunchanged.\n\nBut. But you can also `edit` hunks.\n\nAnd after that `edit` step, those hunks have to be re-colored. And _that_\nis why `git-add--interactive` bothers to read those `color.diff.*` values\nto begin with.\n\nNow, how do you see this re-coloring in action?\n\nI am almost glad that you asked. By simulating that `edit` in the\nregression test. But that is not enough because the hunk is staged\nimmediately after editing. But you _can_ go back to that hunk, even if it\nalready has been changed, and make up your mind to _not_ stage it, but\nthat only works if `git add -p` hasn't quit already, which it would do\nwith a single hunk, so we have to have _two_ hunks.\n\nThis not only complicates the regression test, but _of course_ (because\n_why would I already be done with this patch series???_) it shows further\ndifferences between the Perl version and the built-in version. It's almost\nas if `git add -i` said \"Johannes, you think those 500 hours were all you\nwould spend on converting me to a built-in? Think again!\".\n\nOne of the issues I found (and fixed in v2) was that the built-in version\ntried to be practical and _not_ special-case a count of one in the hunk\nheader (`@@ -3 +3 @@` is equivalent to `@@ -3,1 +3,1 @@`). Well, now it\ndoes, just like libxdiff and the Perl version.\n\nThe other issue was that the Perl version still used `color.diff.plain`\ninstead of `color.diff.context`. I hope I found a good-enough solution to\nthe problem that we simply cannot call `git config --get-color` or\n`repo_get_config()` with _two_ keys to look for (and the last one wins).\n\nFor those reasons, v2 brings more changes than I had hoped for. In the\nend, it is a better patch series, obviously. So even if I was reluctant to\nwork on all this: thank you for prodding me.\n\nCiao,\nDscho\n"},{"id":"409659","messageId":"20201111180713.GC9902@coredump.intra.peff.net","threadId":"54617","inReplyTo":"nycvar.QRO.7.76.6.2011111635140.18437@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 9/9] add -i: verify in the tests that colors can be overridden","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-11T18:07:13Z","receivedAt":"2020-11-11T18:07:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 11, 2020 at 04:53:13PM +0100, Johannes Schindelin wrote:\n\n> > If we are using dash, then surely BASH_XTRACEFD does not matter either\n> > way?\n> \n> It kinda does, though. Dash _does_ respect the `BASH_XTRACEFD` variable,\n> funnily enough, but does not hand the settings through to sub-shells,\n> whereas Bash does.\n\nReally? That's news to me, and doesn't seem to trigger:\n\n [bash uses it]\n $ bash -xc 'BASH_XTRACEFD=3; echo foo' 3>trace\n + BASH_XTRACEFD=3\n foo\n $ cat trace\n + echo foo\n\n [dash does not]\n $ dash -xc 'BASH_XTRACEFD=3; echo foo' 3>trace\n + BASH_XTRACEFD=3\n + echo foo\n foo\n $ cat trace\n\n> > Perhaps the subshell is important if we are running bash, though.\n> \n> The most important thing, which I of course _failed_ to describe most\n> prominently, is that _in general_ `-x` will mess up the `err` file, i.e.\n> when running with anything but Bash. Therefoer we need the `grep` instead\n> of a `test_cmp`, and that's what I tried to convey in v2.\n\nYeah. I understood that part (because it has bit me so many times ;) ),\nbut I agree it's good to make it clear.\n\n> Oh my. I really had tried to avoid going _this_ deep. The `.meta` setting\n> is not even read by the interactive add command:\n> \n> \t$ git grep -w meta git-add--interactive.perl add-interactive.c \\\n> \t\tadd-patch.c\n> \n> comes up empty.\n> [how and why add--interactive.perl reads color config]\n\nHmm. Right, I knew about that weirdness. But I assumed that the builtin\nadd-interactive was doing the diffs in-core. Otherwise, why would we\nhave seen the failure to load diff.color.frag in the first place?\nPhilippe's simple example just did \"git add -p\". So now I'm doubly\nconfused.\n\nThe answer seems to be that render_hunk() always _replaces_ the colors\nwe got from running the external diff. Whereas the perl version only\napplied coloring when reading back in the results of an edit operation\n(and likewise used the frag color when generating a split hunk header).\n\nI'm not sure that what the builtin version is doing is wrong, but it\nseems like it's putting a lot of extra effort into parsing colors off of\nthe colorized version. Whereas the perl version just assumes the lines\nmatch up. I do wonder if there are corner cases we might hit around\nfilters here, though. The lines we get from a filter might bear no\nresemblance at all to diff lines. The only thing we require in the perl\nversion is that they have correspond 1-to-1 with the unfiltered diff\nlines in meaning.\n\n> For those reasons, v2 brings more changes than I had hoped for. In the\n> end, it is a better patch series, obviously. So even if I was reluctant to\n> work on all this: thank you for prodding me.\n\nHeh. Sorry and thanks, I guess? :) I'll try to read over v2 carefully.\n\n-Peff\n"},{"id":"409662","messageId":"20201111184527.GD9902@coredump.intra.peff.net","threadId":"54617","inReplyTo":"pull.785.v2.git.1605097704.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 00/11] Fix color handling in git add -i","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-11T18:45:27Z","receivedAt":"2020-11-11T18:45:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 11, 2020 at 12:28:13PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> Changes since v1:\n> \n> * The regression test now actually exercises the re-coloring (that is the\n> primary purpose of git add -p looking at the color.diff.* variables).\n> * The way the built-in git add -p renders hunk headers of split hunks was\n> aligned with how the Perl version does things.\n> * We now consistently prefer color.diff.context over color.diff.plain, no\n> matter whether using the Perl or the built-in version of git add -p.\n> * The commit message for the regression test no longer confuses readers by\n> mentioning dash.\n> * The regression test was structured a bit better, first testing error\n> message coloring, then the menu in git add -i and then the diff coloring\n> in git add -p.\n\nThe changes were less scary than I was led to believe after reading your\nearlier response. :)\n\nEverything here looks sensible. As I said elsewhere, I do worry there\nmay be a lingering issue with how parse_diff() looks at the filtered\ndiffs. Let me see if I can get diff-so-fancy working...\n\nAha, yes. I think using diff-so-fancy here isn't entirely robust,\nbecause it has some cases where it fails at the 1-to-1 line\ncorrespondence, but they're aware of the issue. But it does work in\nsimples cases now (there's some coloring which makes the output more\nmeaningful, but I obviously won't paste it here):\n\n $ git -c interactive.difffilter='diff-so-fancy' add -p\n ──────────────────────────────────────────────────────────────────────\n modified: file\n ──────────────────────────────────────────────────────────────────────\n @ file:1 @\n old\n new\n (1/1) Stage this hunk [y,n,q,a,d,e,?]? \n\nBut with the builtin:\n\n $ git -c interactive.difffilter='diff-so-fancy' \\\n -c add.interactive.usebuiltin=true \\\n\tadd -p\n error: could not parse colored hunk header '?[1m?[38;5;1m?[31mold?[m'\n\nI don't use it myself, and as I said, I think they have some fixes to\nmake for it to be robust as a diff-filter. But I suspect this may be a\nproblem once the builtin version becomes the default.\n\nI don't think it should be dealt with in this patch series, though.\nWhile it may touch on some of the coloring code, it's otherwise\northogonal to the fixes here. And while the fix is likely to be to make\nrender_hunk() stop re-coloring in the non-edit cases, your more-robust\ntest here will still be checking what you expect (because it really is\ntriggering the edit case, not relying on the extra coloring to see the\neffect of in-process color config).\n\n-Peff\n"},{"id":"409759","messageId":"nycvar.QRO.7.76.6.2011121448550.18437@tvgsbejvaqbjf.bet","threadId":"54617","inReplyTo":"20201111180713.GC9902@coredump.intra.peff.net","subject":"Re: [PATCH 9/9] add -i: verify in the tests that colors can be overridden","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-12T14:04:01Z","receivedAt":"2020-11-12T14:05:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 11 Nov 2020, Jeff King wrote:\n\n> On Wed, Nov 11, 2020 at 04:53:13PM +0100, Johannes Schindelin wrote:\n>\n> > > If we are using dash, then surely BASH_XTRACEFD does not matter either\n> > > way?\n> >\n> > It kinda does, though. Dash _does_ respect the `BASH_XTRACEFD` variable,\n> > funnily enough, but does not hand the settings through to sub-shells,\n> > whereas Bash does.\n>\n> Really? That's news to me, and doesn't seem to trigger:\n>\n> [bash uses it]\n> $ bash -xc 'BASH_XTRACEFD=3; echo foo' 3>trace\n> + BASH_XTRACEFD=3\n> foo\n> $ cat trace\n> + echo foo\n>\n> [dash does not]\n> $ dash -xc 'BASH_XTRACEFD=3; echo foo' 3>trace\n> + BASH_XTRACEFD=3\n> + echo foo\n> foo\n> $ cat trace\n\nGaaah! I totally forgot that `BASH_XTRACEFD` is only heeded by BusyBox'\nash (and only when built with `ENABLE_ASH_BASH_COMPAT`), not by `dash`.\n\nSorry for the noise.\n\n> > Oh my. I really had tried to avoid going _this_ deep. The `.meta` setting\n> > is not even read by the interactive add command:\n> >\n> > \t$ git grep -w meta git-add--interactive.perl add-interactive.c \\\n> > \t\tadd-patch.c\n> >\n> > comes up empty.\n> > [how and why add--interactive.perl reads color config]\n>\n> Hmm. Right, I knew about that weirdness. But I assumed that the builtin\n> add-interactive was doing the diffs in-core. Otherwise, why would we\n> have seen the failure to load diff.color.frag in the first place?\n\nOh, that's easy to explain: as you can verify reading\nhttps://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898\nthe Perl version of `git add -p` insists on (re-)constructing the hunk\nheaders manually, and obviously it needs to color them manually, too. And\nhttps://github.com/git/git/blob/e31aba42fb12/add-patch.c#L649-L672 shows\nthat the built-in version of `git add -p` slavishly follows that practice.\n\n> Philippe's simple example just did \"git add -p\". So now I'm doubly\n> confused.\n\nRight. I should have been more precise in what parts are used of the diff\nthat is colored via the internal diff machinery. The hunk headers are not\nused. The hunks themselves are, unless edited. The file header is, too:\nhttps://github.com/git/git/blob/e31aba42fb12/add-patch.c#L683-L714\n\n> The answer seems to be that render_hunk() always _replaces_ the colors\n> we got from running the external diff. Whereas the perl version only\n> applied coloring when reading back in the results of an edit operation\n> (and likewise used the frag color when generating a split hunk header).\n\nNo, the Perl version also insists on applying `fraginfo_color`, see\nhttps://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898\n\n> I'm not sure that what the builtin version is doing is wrong, but it\n> seems like it's putting a lot of extra effort into parsing colors off of\n> the colorized version. Whereas the perl version just assumes the lines\n> match up. I do wonder if there are corner cases we might hit around\n> filters here, though. The lines we get from a filter might bear no\n> resemblance at all to diff lines. The only thing we require in the perl\n> version is that they have correspond 1-to-1 with the unfiltered diff\n> lines in meaning.\n\nThey do have to correspond 1-to-1 because the assumption is that the\nindividual lines will then correspond one-to-one, too. This does not need\nto be true, of course, but then the filter is probably less useful than\nthe user wants it to be.\n\n> > For those reasons, v2 brings more changes than I had hoped for. In the\n> > end, it is a better patch series, obviously. So even if I was reluctant to\n> > work on all this: thank you for prodding me.\n>\n> Heh. Sorry and thanks, I guess? :) I'll try to read over v2 carefully.\n\nNo need to be sorry on your side. _I_ am sorry that I did not add that\ntest case during my re-implementation efforts in the first place.\n\nThanks,\nDScho\n"},{"id":"409763","messageId":"nycvar.QRO.7.76.6.2011121504110.18437@tvgsbejvaqbjf.bet","threadId":"54617","inReplyTo":"20201111184527.GD9902@coredump.intra.peff.net","subject":"Re: [PATCH v2 00/11] Fix color handling in git add -i","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-12T14:20:38Z","receivedAt":"2020-11-12T14:21:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 11 Nov 2020, Jeff King wrote:\n\n> On Wed, Nov 11, 2020 at 12:28:13PM +0000, Johannes Schindelin via GitGitGadget wrote:\n>\n> > Changes since v1:\n> >\n> > * The regression test now actually exercises the re-coloring (that is the\n> > primary purpose of git add -p looking at the color.diff.* variables).\n> > * The way the built-in git add -p renders hunk headers of split hunks was\n> > aligned with how the Perl version does things.\n> > * We now consistently prefer color.diff.context over color.diff.plain, no\n> > matter whether using the Perl or the built-in version of git add -p.\n> > * The commit message for the regression test no longer confuses readers by\n> > mentioning dash.\n> > * The regression test was structured a bit better, first testing error\n> > message coloring, then the menu in git add -i and then the diff coloring\n> > in git add -p.\n>\n> The changes were less scary than I was led to believe after reading your\n> earlier response. :)\n>\n> Everything here looks sensible. As I said elsewhere, I do worry there\n> may be a lingering issue with how parse_diff() looks at the filtered\n> diffs. Let me see if I can get diff-so-fancy working...\n>\n> Aha, yes. I think using diff-so-fancy here isn't entirely robust,\n> because it has some cases where it fails at the 1-to-1 line\n> correspondence, but they're aware of the issue. But it does work in\n> simples cases now (there's some coloring which makes the output more\n> meaningful, but I obviously won't paste it here):\n>\n> $ git -c interactive.difffilter='diff-so-fancy' add -p\n> ──────────────────────────────────────────────────────────────────────\n> modified: file\n> ──────────────────────────────────────────────────────────────────────\n> @ file:1 @\n> old\n> new\n> (1/1) Stage this hunk [y,n,q,a,d,e,?]?\n\nIt might _seem_ that it works. But try to split a hunk. I did this with\nthe test case (interrupting it just before running `add -p`):\n\n-- snip --\n$ git -C ./t/trash\\ directory.t3701-add-interactive/ -c\ninteractive.difffilter='diff-so-fancy' -c add.interactive.usebuiltin=false add -p\n────────────────────────────────────────────────────────────────────────────────\nmodified: color-test\n────────────────────────────────────────────────────────────────────────────────\n@ color-test:1 @\ncontext\nold\nnew\nmore-context\nanother-one\n(1/1) Stage this hunk [y,n,q,a,d,s,e,?]? s\nSplit into 2 hunks.\n@@ -1,3 +1,3 @@\nold\nnew\nmore-context\nanother-one\n(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]?\n-- snap --\n\nObviously, I marked it up so you can see what parts were colored.\n\nSee how it first _looks_ as if it works? But then you split the hunk, but\ninstead of showing only the old/new part, it shows the old/new/another-one\npart!\n\nIn other words, it does not work at all, and the fact that it does not\neven warn you about it is misleading, and that's pretty much all I will\nsay about it.\n\n> But with the builtin:\n>\n> $ git -c interactive.difffilter='diff-so-fancy' \\\n> -c add.interactive.usebuiltin=true \\\n> \tadd -p\n> error: could not parse colored hunk header '?[1m?[38;5;1m?[31mold?[m'\n\nGranted, this is not quite helpful. I _think_ what is happening is that\nthe number of lines is different, and `add -p` goes like: noooope! But it\ncould probably be better at describing what the issue is. Or it could\ncater to `diff-so-fancy`, if that is what people use these days, and\nspecial-case it by falling back to detecting the hunk boundaries in a\nmanner that is compatible with `diff-so-fancy`.\n\nOr we might be able to come up with a strategy that is not so limited and\nthat works for more than just `diff-so-fancy`.\n\n> I don't use it myself, and as I said, I think they have some fixes to\n> make for it to be robust as a diff-filter. But I suspect this may be a\n> problem once the builtin version becomes the default.\n>\n> I don't think it should be dealt with in this patch series, though.\n\nOh, _that_ I wholeheartedly agree with.\n\n> While it may touch on some of the coloring code, it's otherwise\n> orthogonal to the fixes here. And while the fix is likely to be to make\n> render_hunk() stop re-coloring in the non-edit cases, your more-robust\n> test here will still be checking what you expect (because it really is\n> triggering the edit case, not relying on the extra coloring to see the\n> effect of in-process color config).\n\nI don't actually think that we _can_ stop re-coloring the hunk header in\nthe general case because we need to keep this working even for split\nhunks. It would be a very bad idea to make it work for non-split cases and\nthen something like `die()` only when the hunk is split. Better re-color\nall of them, then, to not even risk that situation.\n\nCiao,\nDscho\n"},{"id":"409781","messageId":"20201112182925.GA701197@coredump.intra.peff.net","threadId":"54617","inReplyTo":"nycvar.QRO.7.76.6.2011121448550.18437@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 9/9] add -i: verify in the tests that colors can be overridden","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-12T18:29:25Z","receivedAt":"2020-11-12T18:29:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 12, 2020 at 03:04:01PM +0100, Johannes Schindelin wrote:\n\n> Gaaah! I totally forgot that `BASH_XTRACEFD` is only heeded by BusyBox'\n> ash (and only when built with `ENABLE_ASH_BASH_COMPAT`), not by `dash`.\n> \n> Sorry for the noise.\n\nAh, that makes more sense. And I learned something new about busybox. :)\n\n> > Hmm. Right, I knew about that weirdness. But I assumed that the builtin\n> > add-interactive was doing the diffs in-core. Otherwise, why would we\n> > have seen the failure to load diff.color.frag in the first place?\n> \n> Oh, that's easy to explain: as you can verify reading\n> https://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898\n> the Perl version of `git add -p` insists on (re-)constructing the hunk\n> headers manually, and obviously it needs to color them manually, too. And\n> https://github.com/git/git/blob/e31aba42fb12/add-patch.c#L649-L672 shows\n> that the built-in version of `git add -p` slavishly follows that practice.\n\nBut that is only when we split hunks (your link to the perl script is in\nsplit_hunks()). I agree we must color manually there when creating our\nown hunk header. But outside of that and patch-editing, the perl script\ndoes not otherwise recolor or rewrite (nor even really parse beyond\nline-splitting) what it gets from the colorized version.\n\nWhereas add-patch parses the colors off of the version and then\nre-colors every hunk header. Which seems doubly weird to me. Even if we\nwere going to re-color every hunk (e.g., because we don't want to store\nthe original hunk line, but instead a parsed version of it), why bother\nparsing the color version at all, and not just the machine-readable\nversion?\n\n> > The answer seems to be that render_hunk() always _replaces_ the colors\n> > we got from running the external diff. Whereas the perl version only\n> > applied coloring when reading back in the results of an edit operation\n> > (and likewise used the frag color when generating a split hunk header).\n> \n> No, the Perl version also insists on applying `fraginfo_color`, see\n> https://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898\n\nOnly when we split. Try this to give different colors between the\ninteractive script and diff:\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex e713fe3d02..862a21ff1f 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -28,8 +28,9 @@\n my $diff_use_color = $repo->get_colorbool('color.diff');\n my ($fraginfo_color) =\n \t$diff_use_color ? (\n-\t\t$repo->get_color('color.diff.frag', 'cyan'),\n+\t\t$repo->get_color('color.diff.nonsense', 'yellow'),\n \t) : ();\n+# noop to create split hunk\n my ($diff_plain_color) =\n \t$diff_use_color ? (\n \t\t$repo->get_color('color.diff.plain', ''),\n\nRunning \"git add -p\" does not result in yellow hunk headers. But issuing\na split command does.\n\nThe distinction is mostly academic, because diff-tree and the\ninteractive patch code should be using the same colors, so the result\nshould look the same. It could matter if the diff-filter chooses\ndifferent colors, though then the split headers will not match the\noriginals in style. We _could_ run the newly created hunk header\nindividually through the diff-filter, though I'm not sure how various\nfilters would handle that.\n\nThat's true of the perl version as well as the builtin one, but I think\nthe builtin one's insistence on parsing the colored output is taking us\nin the wrong direction to eventually fix that.\n\n> > I'm not sure that what the builtin version is doing is wrong, but it\n> > seems like it's putting a lot of extra effort into parsing colors off of\n> > the colorized version. Whereas the perl version just assumes the lines\n> > match up. I do wonder if there are corner cases we might hit around\n> > filters here, though. The lines we get from a filter might bear no\n> > resemblance at all to diff lines. The only thing we require in the perl\n> > version is that they have correspond 1-to-1 with the unfiltered diff\n> > lines in meaning.\n> \n> They do have to correspond 1-to-1 because the assumption is that the\n> individual lines will then correspond one-to-one, too. This does not need\n> to be true, of course, but then the filter is probably less useful than\n> the user wants it to be.\n\nRight, I'm not disputing the 1-to-1 thing (I was after all the one who\nimplemented interactive.diffilter, and added the \"complain if the counts\ndon't line up\" check). But in the perl script they only need to\ncorrespond _semantically_, not syntactically.\n\n-Peff\n"},{"id":"409784","messageId":"20201112184026.GB701197@coredump.intra.peff.net","threadId":"54617","inReplyTo":"nycvar.QRO.7.76.6.2011121504110.18437@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 00/11] Fix color handling in git add -i","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-12T18:40:26Z","receivedAt":"2020-11-12T18:40:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 12, 2020 at 03:20:38PM +0100, Johannes Schindelin wrote:\n\n> > $ git -c interactive.difffilter='diff-so-fancy' add -p\n> > ──────────────────────────────────────────────────────────────────────\n> > modified: file\n> > ──────────────────────────────────────────────────────────────────────\n> > @ file:1 @\n> > old\n> > new\n> > (1/1) Stage this hunk [y,n,q,a,d,e,?]?\n> \n> It might _seem_ that it works. But try to split a hunk. I did this with\n> the test case (interrupting it just before running `add -p`):\n\nYes, there are definitely problems with how diff-so-fancy represents\nthings (because they don't maintain a 1-to-1 correspondence). You should\ngenerally get a complaint like:\n\n $ git -c interactive.difffilter='diff-so-fancy' add -p\n fatal: mismatched output from interactive.diffFilter\n hint: Your filter must maintain a one-to-one correspondence\n hint: between its input and output lines.\n\nThe diff-so-fancy folks have asked me about this, and I've made clear\nthe problem to them, and that the solution is to have a mode which\nmaintains the line correspondence. AFAIK there's no fix yet. I don't\nknow how many people are actually using it in practice in its current\nbuggy state.\n\nThere's a big thread here:\n\n https://github.com/so-fancy/diff-so-fancy/issues/35\n\nI don't really recommend reading the whole thing. Beyond what I just\nsaid, the interesting bits are:\n\n - people recommend using the \"delta\" tool; it has a \"--color-only\"\n option which does just diff-highlight-style coloring, but doesn't\n break line correspondence\n\n - somebody suggested instead of using interactive.difffilter, to add\n an option to show each hunk in an individual pager command. So\n add-interactive would never see the \"fancy\" version at all, but it\n would be generated on the fly when the user sees it (and that filter\n would have to be able to handle seeing individual hunks without the\n diff header).\n\nAll of which is to say that the current state is a bit of a mess, and I\ndon't consider it completely necessary to fix it before the builtin\nversion becomes the default. But:\n\n - I think we should expect some possible complaints / bug reports\n from fringe users of the already-somewhat-broken state\n\n - IMHO the parsing of the filtered version done by the builtin is\n going in the wrong direction (see my other mail for an elaboration)\n\n> > While it may touch on some of the coloring code, it's otherwise\n> > orthogonal to the fixes here. And while the fix is likely to be to make\n> > render_hunk() stop re-coloring in the non-edit cases, your more-robust\n> > test here will still be checking what you expect (because it really is\n> > triggering the edit case, not relying on the extra coloring to see the\n> > effect of in-process color config).\n> \n> I don't actually think that we _can_ stop re-coloring the hunk header in\n> the general case because we need to keep this working even for split\n> hunks. It would be a very bad idea to make it work for non-split cases and\n> then something like `die()` only when the hunk is split. Better re-color\n> all of them, then, to not even risk that situation.\n\nYeah, obviously calling die() in the split case is bad. And the fact\nthat newly-created split hunk headers are not filtered the same way as\nthe original hunk headers isn't ideal. But it's a pretty easy fix in the\nperl version, and not in the builtin version.\n\n-Peff\n"},{"id":"409867","messageId":"6405f7c9-b084-ed48-e33e-009f388933bf@gmail.com","threadId":"54617","inReplyTo":"c857c4493213bd7cc4d057487db2d1a74fa0bbd6.1605097704.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 04/11] add -i: use `reset_color` consistently","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-11-13T12:03:59Z","receivedAt":"2020-11-13T12:04:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> We already maintain a list of colors in the `add_i_state`, therefore we\n> should use them.\n\nPlaying devil's advocate for a moment - why do we have a reset entry in \nthat list? The next patch makes sure it cannot be customized which is a \ngood thing so why not drop the `reset` member from `add_i_state` \nentirely? The perl version needed to get the reset string from `git \nconfig` and store it somewhere but in the C version we have a perfectly \ngood constant we can use instead.\n\nBest Wishes\n\nPhillip\n\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> add-patch.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/add-patch.c b/add-patch.c\n> index b6d53229bb..bf89c43145 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -672,7 +672,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n> \t\tif (len)\n> \t\t\tstrbuf_add(out, p, len);\n> \t\telse if (colored)\n> -\t\t\tstrbuf_addf(out, \"%s\\n\", GIT_COLOR_RESET);\n> +\t\t\tstrbuf_addf(out, \"%s\\n\", s->s.reset_color);\n> \t\telse\n> \t\t\tstrbuf_addch(out, '\\n');\n> \t}\n> @@ -1065,7 +1065,7 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)\n> \t\t\t s->s.file_new_color :\n> \t\t\t s->s.context_color);\n> \t\tstrbuf_add(&s->colored, plain + current, eol - current);\n> -\t\tstrbuf_addstr(&s->colored, GIT_COLOR_RESET);\n> +\t\tstrbuf_addstr(&s->colored, s->s.reset_color);\n> \t\tif (next > eol)\n> \t\t\tstrbuf_add(&s->colored, plain + eol, next - eol);\n> \t\tcurrent = next;\n> \n\n"},{"id":"409868","messageId":"918a0a1d-6384-5615-c343-1f03046770f7@gmail.com","threadId":"54617","inReplyTo":"73b6d60a801766605cde9403d7a9e8451ef99e11.1605097704.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 07/11] add -p (built-in): do not color the progress indicator separately","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-11-13T12:07:47Z","receivedAt":"2020-11-13T12:07:52Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> The Perl version of this command colors the progress indicator and the\n> prompt message in one go, let's do the same in the built-in version.\n\nWhy? the C version has access to an api that means we don't have to \nremember to print the reset string each time so why move away from that? \nI don't think it matters to the user that there are some extra escape \ncodes in the prompt of the C version. The answer is probably \"so we can \nuse the same test as the perl version\" which might be a good reason - if \nit is I think it would be helpful to say so in the commit message.\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> add-patch.c | 10 +++++-----\n> 1 file changed, 5 insertions(+), 5 deletions(-)\n> \n> diff --git a/add-patch.c b/add-patch.c\n> index bf89c43145..2fad92ca37 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1461,15 +1461,15 @@ static int patch_update_file(struct add_p_state *s,\n> \t\telse\n> \t\t\tprompt_mode_type = PROMPT_HUNK;\n> \n> -\t\tcolor_fprintf(stdout, s->s.prompt_color,\n> -\t\t\t \"(%\"PRIuMAX\"/%\"PRIuMAX\") \",\n> +\t\tprintf(\"%s(%\"PRIuMAX\"/%\"PRIuMAX\") \", s->s.prompt_color,\n> \t\t\t (uintmax_t)hunk_index + 1,\n> \t\t\t (uintmax_t)(file_diff->hunk_nr\n> \t\t\t\t\t\t? file_diff->hunk_nr\n> \t\t\t\t\t\t: 1));\n> -\t\tcolor_fprintf(stdout, s->s.prompt_color,\n> -\t\t\t _(s->mode->prompt_mode[prompt_mode_type]),\n> -\t\t\t s->buf.buf);\n> +\t\tprintf(_(s->mode->prompt_mode[prompt_mode_type]),\n> +\t\t s->buf.buf);\n> +\t\tif (*s->s.reset_color)\n> +\t\t\tfputs(s->s.reset_color, stdout);\n> \t\tfflush(stdout);\n> \t\tif (read_single_character(s) == EOF)\n> \t\t\tbreak;\n> \n\n"},{"id":"409882","messageId":"70fae5a9-354d-978b-c560-d7fcabb25c76@gmail.com","threadId":"54617","inReplyTo":"304614751ea1b1db80fbbbdc909b3d198c20313e.1605097704.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 09/11] add -i (Perl version): include indentation in the colored header","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-11-13T12:11:41Z","receivedAt":"2020-11-13T12:11:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> The header is formatted by padding each column heading with spaces up to\n> the length of 12 characters. These padding spaces are naturally included\n> when coloring the entire header.\n> \n> However, the preceding five spaces indentation for non-flat lists were\n> _not_ included in the Perl version, but _were_ included in the built-in\n> version. Let's adjust the former to align with the latter's behavior.\n\nI had trouble understanding this. I think my confusion is that the \npadding was printed when the header was colored, but it was not inside \nthe colored part whereas the subject lead be to think there was no \nindentation printed when the header was colored. I assume this change is \nso that we can use the same test for both versions?\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> git-add--interactive.perl | 6 ++----\n> 1 file changed, 2 insertions(+), 4 deletions(-)\n> \n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index e713fe3d02..adbac2bc6d 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -483,10 +483,8 @@ sub list_and_choose {\n> \t\tmy $last_lf = 0;\n> \n> \t\tif ($opts->{HEADER}) {\n> -\t\t\tif (!$opts->{LIST_FLAT}) {\n> -\t\t\t\tprint \" \";\n> -\t\t\t}\n> -\t\t\tprint colored $header_color, \"$opts->{HEADER}\\n\";\n> +\t\t\tmy $indent = $opts->{LIST_FLAT} ? \"\" : \" \";\n> +\t\t\tprint colored $header_color, \"$indent$opts->{HEADER}\\n\";\n> \t\t}\n> \t\tfor ($i = 0; $i < @stuff; $i++) {\n> \t\t\tmy $chosen = $chosen[$i] ? '*' : ' ';\n> \n\n"},{"id":"409891","messageId":"nycvar.QRO.7.76.6.2011131452120.18437@tvgsbejvaqbjf.bet","threadId":"54617","inReplyTo":"6405f7c9-b084-ed48-e33e-009f388933bf@gmail.com","subject":"Re: [PATCH v2 04/11] add -i: use `reset_color` consistently","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-13T13:53:02Z","receivedAt":"2020-11-13T13:54:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Fri, 13 Nov 2020, Phillip Wood wrote:\n\n> Hi Dscho\n>\n> On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> >\n> > We already maintain a list of colors in the `add_i_state`, therefore we\n> > should use them.\n>\n> Playing devil's advocate for a moment - why do we have a reset entry in that\n> list? The next patch makes sure it cannot be customized which is a good thing\n> so why not drop the `reset` member from `add_i_state` entirely? The perl\n> version needed to get the reset string from `git config` and store it\n> somewhere but in the C version we have a perfectly good constant we can use\n> instead.\n\nRight.\n\nOn the other hand, does it hurt to keep that field for consistency with\nthe rest of the coloring stuff? It would probably cost more to make the\nchange you suggested than it would benefit us.\n\nCiao,\nDscho\n"},{"id":"409892","messageId":"nycvar.QRO.7.76.6.2011131455460.18437@tvgsbejvaqbjf.bet","threadId":"54617","inReplyTo":"918a0a1d-6384-5615-c343-1f03046770f7@gmail.com","subject":"Re: [PATCH v2 07/11] add -p (built-in): do not color the progress indicator separately","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-13T13:57:15Z","receivedAt":"2020-11-13T13:58:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Fri, 13 Nov 2020, Phillip Wood wrote:\n\n> On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> >\n> > The Perl version of this command colors the progress indicator and the\n> > prompt message in one go, let's do the same in the built-in version.\n>\n> Why? the C version has access to an api that means we don't have to remember\n> to print the reset string each time so why move away from that? I don't think\n> it matters to the user that there are some extra escape codes in the prompt of\n> the C version. The answer is probably \"so we can use the same test as the perl\n> version\" which might be a good reason - if it is I think it would be helpful\n> to say so in the commit message.\n\nHonestly, the number one reason is so that the _same_ test passes using\nthe Perl version as well as with the built-in version, something that is\nrequired by the `linux-clang` job in our CI build.\n\nI am not really willing to change this, unless I hear a goooood reason to\ncomplicate the test.\n\nCiao,\nDscho\n"},{"id":"409893","messageId":"nycvar.QRO.7.76.6.2011131457420.18437@tvgsbejvaqbjf.bet","threadId":"54617","inReplyTo":"70fae5a9-354d-978b-c560-d7fcabb25c76@gmail.com","subject":"Re: [PATCH v2 09/11] add -i (Perl version): include indentation in the colored header","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-13T13:58:57Z","receivedAt":"2020-11-13T14:00:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Fri, 13 Nov 2020, Phillip Wood wrote:\n\n> On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> >\n> > The header is formatted by padding each column heading with spaces up to\n> > the length of 12 characters. These padding spaces are naturally included\n> > when coloring the entire header.\n> >\n> > However, the preceding five spaces indentation for non-flat lists were\n> > _not_ included in the Perl version, but _were_ included in the built-in\n> > version. Let's adjust the former to align with the latter's behavior.\n>\n> I had trouble understanding this. I think my confusion is that the padding was\n> printed when the header was colored, but it was not inside the colored part\n> whereas the subject lead be to think there was no indentation printed when the\n> header was colored.\n\nRight, this is ambiguous, but I thought it was clear from the first\nparagraph that \"included\" means \"when coloring\".\n\nI'm not a native speaker, though, so I welcome suggestions to improve\nthis.\n\n> I assume this change is so that we can use the same test for both\n> versions?\n\nCorrect.\n\nThanks,\nDscho\n"},{"id":"409899","messageId":"4A02D618-34EB-4B20-A102-99B3CDAFD0BB@gmail.com","threadId":"54617","inReplyTo":"48d8e0badfec5f0e576868f7a406ed7ede6c7200.1605097704.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 10/11] add -p: prefer color.diff.context over color.diff.plain","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-11-13T14:04:58Z","receivedAt":"2020-11-13T14:05:03Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Dscho, \n\n\n> Le 11 nov. 2020 Γ  07:28, Johannes Schindelin via GitGitGadget a Γ©crit :\n> \n> From: Johannes Schindelin \n> \n> This still needs\n\ns/needs/leads/\n\n> to inconsistencies when both config names are used: the\n> initial diff will be colored by the diff machinery. Once edited by a\n> user, a hunk has to be re-colored by `git add -p`, though, which would\n> then use the other setting to color the context lines.\n\nPhilippe."},{"id":"409900","messageId":"4ad92124-de1f-df66-093b-5089ad111cb7@gmail.com","threadId":"54617","inReplyTo":"48d8e0badfec5f0e576868f7a406ed7ede6c7200.1605097704.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 10/11] add -p: prefer color.diff.context over color.diff.plain","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-11-13T14:08:22Z","receivedAt":"2020-11-13T14:08:27Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> Git's diff machinery allows users to override the colors to use in\n> diffs, even the plain-colored context lines. As of 8dbf3eb6850 (diff.h:\n> rename DIFF_PLAIN color slot to DIFF_CONTEXT, 2015-05-27), the preferred\n> name of the config setting is `color.diff.context`, although Git still\n> allows `color.diff.plain`.\n> \n> In the context of `git add -p`, this logic is a bit hard to replicate:\n> `git_diff_basic_config()` reads all config values sequentially and if it\n> sees _any_ `color.diff.context` or `color.diff.plain`, it accepts the\n> new color. The Perl version of `git add -p` needs to go through `git\n> config --get-color`, though, which allows only one key to be specified.\n> The same goes for the built-in version of `git add -p`, which has to go\n> through `repo_config_get_value()`.\n\nOne nit pick: the built-in version does not have to go through \n`repo_config_get_value()`, it could get the values in a callback using \n`git_config()` which would match the behavior of diff but chooses not to \n(presumably it is more convenient to just look up the config values). \nHaving said that I think this patch is fine and the commit message does \na good job of explaining the situation.\n\nThanks for working on this series, it's great to see a test at the end\n\nBest Wishes\n\nPhillip\n\n\n> The best we can do here is to look for `.context` and if none is found,\n> fall back to looking for `.plain`, and if still not found, fall back to\n> the hard-coded default (which in this case is simply the empty string,\n> as context lines are typically rendered without colored).\n> \n> This still needs to inconsistencies when both config names are used: the\n> initial diff will be colored by the diff machinery. Once edited by a\n> user, a hunk has to be re-colored by `git add -p`, though, which would\n> then use the other setting to color the context lines.\n> \n> In practice, this is not _all_ that bad. The `git config` manual says\n> this in the `color.diff.`:\n> \n> \t`context` (context text - `plain` is a historical synonym)\n> \n> We should therefore assume that users use either one or the other, but\n> not both names. Besides, it is relatively uncommon to look at a hunk\n> after editing it because it is immediately staged by default.\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> add-interactive.c | 6 ++++--\n> git-add--interactive.perl | 6 +++---\n> 2 files changed, 7 insertions(+), 5 deletions(-)\n> \n> diff --git a/add-interactive.c b/add-interactive.c\n> index c298a8b80f..54dfdc56f5 100644\n> --- a/add-interactive.c\n> +++ b/add-interactive.c\n> @@ -49,8 +49,10 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)\n> \n> \tinit_color(r, s, \"diff.frag\", s->fraginfo_color,\n> \t\t diff_get_color(s->use_color, DIFF_FRAGINFO));\n> -\tinit_color(r, s, \"diff.context\", s->context_color,\n> -\t\tdiff_get_color(s->use_color, DIFF_CONTEXT));\n> +\tinit_color(r, s, \"diff.context\", s->context_color, \"fall back\");\n> +\tif (!strcmp(s->context_color, \"fall back\"))\n> +\t\tinit_color(r, s, \"diff.plain\", s->context_color,\n> +\t\t\t diff_get_color(s->use_color, DIFF_CONTEXT));\n> \tinit_color(r, s, \"diff.old\", s->file_old_color,\n> \t\tdiff_get_color(s->use_color, DIFF_FILE_OLD));\n> \tinit_color(r, s, \"diff.new\", s->file_new_color,\n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index adbac2bc6d..bc3a1e8eff 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -30,9 +30,9 @@\n> \t$diff_use_color ? (\n> \t\t$repo->get_color('color.diff.frag', 'cyan'),\n> \t) : ();\n> -my ($diff_plain_color) =\n> +my ($diff_context_color) =\n> \t$diff_use_color ? (\n> -\t\t$repo->get_color('color.diff.plain', ''),\n> +\t\t$repo->get_color($repo->config('color.diff.context') ? 'color.diff.context' : 'color.diff.plain', ''),\n> \t) : ();\n> my ($diff_old_color) =\n> \t$diff_use_color ? (\n> @@ -1046,7 +1046,7 @@ sub color_diff {\n> \t\tcolored((/^@/ ? $fraginfo_color :\n> \t\t\t /^\\+/ ? $diff_new_color :\n> \t\t\t /^-/ ? $diff_old_color :\n> -\t\t\t $diff_plain_color),\n> +\t\t\t $diff_context_color),\n> \t\t\t$_);\n> \t} @_;\n> }\n> \n\n"},{"id":"409907","messageId":"820a19d2-2acb-3ee1-8927-8981de62bc7e@gmail.com","threadId":"54617","inReplyTo":"nycvar.QRO.7.76.6.2011131452120.18437@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 04/11] add -i: use `reset_color` consistently","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-11-13T16:04:20Z","receivedAt":"2020-11-13T16:04:33Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 13/11/2020 13:53, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Fri, 13 Nov 2020, Phillip Wood wrote:\n> \n>> Hi Dscho\n>>\n>> On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:\n>>> From: Johannes Schindelin \n>>>\n>>> We already maintain a list of colors in the `add_i_state`, therefore we\n>>> should use them.\n>>\n>> Playing devil's advocate for a moment - why do we have a reset entry in that\n>> list? The next patch makes sure it cannot be customized which is a good thing\n>> so why not drop the `reset` member from `add_i_state` entirely? The perl\n>> version needed to get the reset string from `git config` and store it\n>> somewhere but in the C version we have a perfectly good constant we can use\n>> instead.\n> \n> Right.\n> \n> On the other hand, does it hurt to keep that field for consistency with\n> the rest of the coloring stuff? \n\nIt creates the illusion that `reset` is similar to the others but it \nisn't as we don't want it to be customizable.\n\n> It would probably cost more to make the\n> change you suggested than it would benefit us.\n\nAt the moment there is one use of `s->s.color_reset` and two of \n`GIT_COLOR_RESET` in add-patch.c\n\nBest Wishes\n\nPhillip\n\n> Ciao,\n> Dscho\n> \n"},{"id":"409908","messageId":"d0603db3-a6d8-d71b-1ee0-4c5c51b00da2@gmail.com","threadId":"54617","inReplyTo":"nycvar.QRO.7.76.6.2011131455460.18437@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 07/11] add -p (built-in): do not color the progress indicator separately","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-11-13T16:06:25Z","receivedAt":"2020-11-13T16:06:38Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 13/11/2020 13:57, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Fri, 13 Nov 2020, Phillip Wood wrote:\n> \n>> On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:\n>>> From: Johannes Schindelin \n>>>\n>>> The Perl version of this command colors the progress indicator and the\n>>> prompt message in one go, let's do the same in the built-in version.\n>>\n>> Why? the C version has access to an api that means we don't have to remember\n>> to print the reset string each time so why move away from that? I don't think\n>> it matters to the user that there are some extra escape codes in the prompt of\n>> the C version. The answer is probably \"so we can use the same test as the perl\n>> version\" which might be a good reason - if it is I think it would be helpful\n>> to say so in the commit message.\n> \n> Honestly, the number one reason is so that the _same_ test passes using\n> the Perl version as well as with the built-in version, something that is\n> required by the `linux-clang` job in our CI build.\n\nThat's what I assumed - it would be good to document that in the commit \nmessage.\n\n> I am not really willing to change this, unless I hear a goooood reason to\n> complicate the test.\n\nI guess we could change the perl version to match the C version as it is \nthe perl version that will be retired but I'm not that fussed\n\nBest Wishes\n\nPhillip\n\n\n> Ciao,\n> Dscho\n> \n"},{"id":"409910","messageId":"041e9ff3-2447-e55d-f40d-079a84f0ad44@gmail.com","threadId":"54617","inReplyTo":"nycvar.QRO.7.76.6.2011131457420.18437@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 09/11] add -i (Perl version): include indentation in the colored header","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-11-13T16:12:00Z","receivedAt":"2020-11-13T16:12:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 13/11/2020 13:58, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Fri, 13 Nov 2020, Phillip Wood wrote:\n> \n>> On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:\n>>> From: Johannes Schindelin \n>>>\n>>> The header is formatted by padding each column heading with spaces up to\n>>> the length of 12 characters. These padding spaces are naturally included\n>>> when coloring the entire header.\n>>>\n>>> However, the preceding five spaces indentation for non-flat lists were\n>>> _not_ included in the Perl version, but _were_ included in the built-in\n>>> version. Let's adjust the former to align with the latter's behavior.\n>>\n>> I had trouble understanding this. I think my confusion is that the padding was\n>> printed when the header was colored, but it was not inside the colored part\n>> whereas the subject lead be to think there was no indentation printed when the\n>> header was colored.\n> \n> Right, this is ambiguous, but I thought it was clear from the first\n> paragraph that \"included\" means \"when coloring\".\n> \n> I'm not a native speaker, though, so I welcome suggestions to improve\n> this.\n\nMaybe something along the lines of\n\nadd -i (Perl version): color header to match the C version\n\nBoth versions of `add -i` indent non-flat lists by five spaces. However \nwhen using color the C version prints these spaces after the ANSI color \ncodes whereas the Perl version prints them before the color codes. \nChange the Perl version to match the C version to make testing easier.\n\nBest Wishes\n\nPhillip\n\n>> I assume this change is so that we can use the same test for both\n>> versions?\n> \n> Correct.\n> \n> Thanks,\n> Dscho\n> \n"},{"id":"409998","messageId":"nycvar.QRO.7.76.6.2011160027480.18437@tvgsbejvaqbjf.bet","threadId":"54617","inReplyTo":"20201112182925.GA701197@coredump.intra.peff.net","subject":"Re: [PATCH 9/9] add -i: verify in the tests that colors can be overridden","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-15T23:35:44Z","receivedAt":"2020-11-16T12:38:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 12 Nov 2020, Jeff King wrote:\n\n> On Thu, Nov 12, 2020 at 03:04:01PM +0100, Johannes Schindelin wrote:\n>\n> > > Hmm. Right, I knew about that weirdness. But I assumed that the builtin\n> > > add-interactive was doing the diffs in-core. Otherwise, why would we\n> > > have seen the failure to load diff.color.frag in the first place?\n> >\n> > Oh, that's easy to explain: as you can verify reading\n> > https://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898\n> > the Perl version of `git add -p` insists on (re-)constructing the hunk\n> > headers manually, and obviously it needs to color them manually, too. And\n> > https://github.com/git/git/blob/e31aba42fb12/add-patch.c#L649-L672 shows\n> > that the built-in version of `git add -p` slavishly follows that practice.\n>\n> But that is only when we split hunks (your link to the perl script is in\n> split_hunks()). I agree we must color manually there when creating our\n> own hunk header. But outside of that and patch-editing, the perl script\n> does not otherwise recolor or rewrite (nor even really parse beyond\n> line-splitting) what it gets from the colorized version.\n>\n> Whereas add-patch parses the colors off of the version and then\n> re-colors every hunk header. Which seems doubly weird to me. Even if we\n> were going to re-color every hunk (e.g., because we don't want to store\n> the original hunk line, but instead a parsed version of it), why bother\n> parsing the color version at all, and not just the machine-readable\n> version?\n\nLet's continue on this distraction for a bit before I go back to fixing\nthe patch series, which actually tries to fix a _different_ concern.\n\nThe reason why `add-patch.c` \"parses the colors off\" is that we want to\nshow the rest of the hunk header, in color, even after splitting hunks\n(this will only be shown for the first hunk, of course).\n\nBut given that `git add -p` is somewhat of a fringe use, and using\n`diffFilter` is _even_ more fringe, I do not really want to spend any\nfurther time on this tangent.\n\n> > > The answer seems to be that render_hunk() always _replaces_ the colors\n> > > we got from running the external diff. Whereas the perl version only\n> > > applied coloring when reading back in the results of an edit operation\n> > > (and likewise used the frag color when generating a split hunk header).\n> >\n> > No, the Perl version also insists on applying `fraginfo_color`, see\n> > https://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898\n>\n> Only when we split. Try this to give different colors between the\n> interactive script and diff:\n>\n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index e713fe3d02..862a21ff1f 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -28,8 +28,9 @@\n> my $diff_use_color = $repo->get_colorbool('color.diff');\n> my ($fraginfo_color) =\n> \t$diff_use_color ? (\n> -\t\t$repo->get_color('color.diff.frag', 'cyan'),\n> +\t\t$repo->get_color('color.diff.nonsense', 'yellow'),\n> \t) : ();\n> +# noop to create split hunk\n> my ($diff_plain_color) =\n> \t$diff_use_color ? (\n> \t\t$repo->get_color('color.diff.plain', ''),\n>\n> Running \"git add -p\" does not result in yellow hunk headers. But issuing\n> a split command does.\n>\n> The distinction is mostly academic, because diff-tree and the\n> interactive patch code should be using the same colors, so the result\n> should look the same. It could matter if the diff-filter chooses\n> different colors, though then the split headers will not match the\n> originals in style. We _could_ run the newly created hunk header\n> individually through the diff-filter, though I'm not sure how various\n> filters would handle that.\n>\n> That's true of the perl version as well as the builtin one, but I think\n> the builtin one's insistence on parsing the colored output is taking us\n> in the wrong direction to eventually fix that.\n\nMy thinking back then was: what if _I_ want to use a diffFilter? For what\nwould I use it? Probably to emphasize certain hunk headers more, by adding\nmore color to the part after the line range.\n\nAnyway. I stand by what I said above: I do not want to spend any further\ntime on this tangent, at least not right now. There are more pressing\nchallenges waiting for me, and I expect those other challenge to have a\nmuch bigger \"return on investment\".\n\nCiao,\nDscho\n\n> > > I'm not sure that what the builtin version is doing is wrong, but it\n> > > seems like it's putting a lot of extra effort into parsing colors off of\n> > > the colorized version. Whereas the perl version just assumes the lines\n> > > match up. I do wonder if there are corner cases we might hit around\n> > > filters here, though. The lines we get from a filter might bear no\n> > > resemblance at all to diff lines. The only thing we require in the perl\n> > > version is that they have correspond 1-to-1 with the unfiltered diff\n> > > lines in meaning.\n> >\n> > They do have to correspond 1-to-1 because the assumption is that the\n> > individual lines will then correspond one-to-one, too. This does not need\n> > to be true, of course, but then the filter is probably less useful than\n> > the user wants it to be.\n>\n> Right, I'm not disputing the 1-to-1 thing (I was after all the one who\n> implemented interactive.diffilter, and added the \"complain if the counts\n> don't line up\" check). But in the perl script they only need to\n> correspond _semantically_, not syntactically.\n>\n> -Peff\n>\n"},{"id":"409999","messageId":"nycvar.QRO.7.76.6.2011160036100.18437@tvgsbejvaqbjf.bet","threadId":"54617","inReplyTo":"20201112184026.GB701197@coredump.intra.peff.net","subject":"Re: [PATCH v2 00/11] Fix color handling in git add -i","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-15T23:40:32Z","receivedAt":"2020-11-16T12:38:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 12 Nov 2020, Jeff King wrote:\n\n> On Thu, Nov 12, 2020 at 03:20:38PM +0100, Johannes Schindelin wrote:\n>\n> > > $ git -c interactive.difffilter='diff-so-fancy' add -p\n> > > ──────────────────────────────────────────────────────────────────────\n> > > modified: file\n> > > ──────────────────────────────────────────────────────────────────────\n> > > @ file:1 @\n> > > old\n> > > new\n> > > (1/1) Stage this hunk [y,n,q,a,d,e,?]?\n> >\n> > It might _seem_ that it works. But try to split a hunk. I did this with\n> > the test case (interrupting it just before running `add -p`):\n>\n> Yes, there are definitely problems with how diff-so-fancy represents\n> things (because they don't maintain a 1-to-1 correspondence). You should\n> generally get a complaint like:\n>\n> $ git -c interactive.difffilter='diff-so-fancy' add -p\n> fatal: mismatched output from interactive.diffFilter\n> hint: Your filter must maintain a one-to-one correspondence\n> hint: between its input and output lines.\n>\n> The diff-so-fancy folks have asked me about this, and I've made clear\n> the problem to them, and that the solution is to have a mode which\n> maintains the line correspondence. AFAIK there's no fix yet. I don't\n> know how many people are actually using it in practice in its current\n> buggy state.\n>\n> There's a big thread here:\n>\n> https://github.com/so-fancy/diff-so-fancy/issues/35\n>\n> I don't really recommend reading the whole thing. Beyond what I just\n> said, the interesting bits are:\n>\n> - people recommend using the \"delta\" tool; it has a \"--color-only\"\n> option which does just diff-highlight-style coloring, but doesn't\n> break line correspondence\n>\n> - somebody suggested instead of using interactive.difffilter, to add\n> an option to show each hunk in an individual pager command. So\n> add-interactive would never see the \"fancy\" version at all, but it\n> would be generated on the fly when the user sees it (and that filter\n> would have to be able to handle seeing individual hunks without the\n> diff header).\n>\n> All of which is to say that the current state is a bit of a mess, and I\n> don't consider it completely necessary to fix it before the builtin\n> version becomes the default. But:\n>\n> - I think we should expect some possible complaints / bug reports\n> from fringe users of the already-somewhat-broken state\n>\n> - IMHO the parsing of the filtered version done by the builtin is\n> going in the wrong direction (see my other mail for an elaboration)\n\nIt's a little bit of a surprise that this is the first time I hear about\nthis.\n\nThe way _I_ would go about fixing this is to look for a tell-tale that\nwe're looking at a `diff-so-fancy` style output, e.g. by looking for that\nhorizontal line, then adding special code to handle that.\n\nThis is not a minor undertaking, and it would currently require _two_\nimplementations: the Perl version and the built-in version. They would\nlook _very_ different from one another. Therefore, I would probably either\nwait until the Perl version is retired, or selectively only adjust the\nbuilt-in version, if _I_ was to implement this.\n\nBut given that it does not work right now, and given that it has not been\nworking for a long time, I think it does not hurt so much if it is left in\nthe current state for a bit longer. I would love to focus on the patch\nseries that kicked off this discussion first, getting it done, before I\nthink about other `add -p` aspects.\n\nCiao,\nDscho\n\n>\n> > > While it may touch on some of the coloring code, it's otherwise\n> > > orthogonal to the fixes here. And while the fix is likely to be to make\n> > > render_hunk() stop re-coloring in the non-edit cases, your more-robust\n> > > test here will still be checking what you expect (because it really is\n> > > triggering the edit case, not relying on the extra coloring to see the\n> > > effect of in-process color config).\n> >\n> > I don't actually think that we _can_ stop re-coloring the hunk header in\n> > the general case because we need to keep this working even for split\n> > hunks. It would be a very bad idea to make it work for non-split cases and\n> > then something like `die()` only when the hunk is split. Better re-color\n> > all of them, then, to not even risk that situation.\n>\n> Yeah, obviously calling die() in the split case is bad. And the fact\n> that newly-created split hunk headers are not filtered the same way as\n> the original hunk headers isn't ideal. But it's a pretty easy fix in the\n> perl version, and not in the builtin version.\n>\n> -Peff\n>\n"},{"id":"410000","messageId":"nycvar.QRO.7.76.6.2011160042290.18437@tvgsbejvaqbjf.bet","threadId":"54617","inReplyTo":"820a19d2-2acb-3ee1-8927-8981de62bc7e@gmail.com","subject":"Re: [PATCH v2 04/11] add -i: use `reset_color` consistently","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-15T23:47:16Z","receivedAt":"2020-11-16T12:38:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Fri, 13 Nov 2020, Phillip Wood wrote:\n\n> On 13/11/2020 13:53, Johannes Schindelin wrote:\n> >\n> > On Fri, 13 Nov 2020, Phillip Wood wrote:\n> >\n> > > On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:\n> > > > From: Johannes Schindelin \n> > > >\n> > > > We already maintain a list of colors in the `add_i_state`, therefore we\n> > > > should use them.\n> > >\n> > > Playing devil's advocate for a moment - why do we have a reset entry\n> > > in that list? The next patch makes sure it cannot be customized\n> > > which is a good thing so why not drop the `reset` member from\n> > > `add_i_state` entirely? The perl version needed to get the reset\n> > > string from `git config` and store it somewhere but in the C version\n> > > we have a perfectly good constant we can use instead.\n> >\n> > Right.\n> >\n> > On the other hand, does it hurt to keep that field for consistency with\n> > the rest of the coloring stuff?\n>\n> It creates the illusion that `reset` is similar to the others but it isn't as\n> we don't want it to be customizable.\n\nIn a certain sense, however, it *is* customizable. Namely when the color\nis turned _off_. Then `reset_color` is set to the empty string, and\n`GIT_RESET_COLOR` still isn't.\n\n> > It would probably cost more to make the change you suggested than it\n> > would benefit us.\n>\n> At the moment there is one use of `s->s.color_reset` and two of\n> `GIT_COLOR_RESET` in add-patch.c\n\nI did spend the time to look at those call sites, and they seem to be\nguarded by \"if colored\" conditions.\n\nBut as I said, I don't think that it will buy us a ton to treat resetting\ncolor different from setting color.\n\nI mean, who is to say that we won't add some sort of low-level mode of\n`git add -p` at some stage that can be used by 3rd-party applications? We\nwould then have to switch back to the current code. And even if not, I\nthink that there is a benefit to the current code, where no caller has to\nwonder whether the reset sequence has been overridden (e.g. by the empty\nstring) or not, it just says semantically that this is where we switch\nback to regular color. And that's that.\n\nCiao,\nDscho\n"},{"id":"410001","messageId":"nycvar.QRO.7.76.6.2011160050300.18437@tvgsbejvaqbjf.bet","threadId":"54617","inReplyTo":"041e9ff3-2447-e55d-f40d-079a84f0ad44@gmail.com","subject":"Re: [PATCH v2 09/11] add -i (Perl version): include indentation in the colored header","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-15T23:51:10Z","receivedAt":"2020-11-16T12:38:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Fri, 13 Nov 2020, Phillip Wood wrote:\n\n> On 13/11/2020 13:58, Johannes Schindelin wrote:\n>\n> > On Fri, 13 Nov 2020, Phillip Wood wrote:\n> >\n> > > On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:\n> > > > From: Johannes Schindelin \n> > > >\n> > > > The header is formatted by padding each column heading with spaces\n> > > > up to the length of 12 characters. These padding spaces are\n> > > > naturally included when coloring the entire header.\n> > > >\n> > > > However, the preceding five spaces indentation for non-flat lists\n> > > > were _not_ included in the Perl version, but _were_ included in\n> > > > the built-in version. Let's adjust the former to align with the\n> > > > latter's behavior.\n> > >\n> > > I had trouble understanding this. I think my confusion is that the\n> > > padding was printed when the header was colored, but it was not\n> > > inside the colored part whereas the subject lead be to think there\n> > > was no indentation printed when the header was colored.\n> >\n> > Right, this is ambiguous, but I thought it was clear from the first\n> > paragraph that \"included\" means \"when coloring\".\n> >\n> > I'm not a native speaker, though, so I welcome suggestions to improve\n> > this.\n>\n> Maybe something along the lines of\n>\n> add -i (Perl version): color header to match the C version\n>\n> Both versions of `add -i` indent non-flat lists by five spaces. However when\n> using color the C version prints these spaces after the ANSI color codes\n> whereas the Perl version prints them before the color codes. Change the Perl\n> version to match the C version to make testing easier.\n\nI wrapped it, and modified it slightly:\n\n add -i (Perl version): color header to match the C version\n\n Both versions of `add -i` indent non-flat lists by five spaces. However\n when using color the C version prints these spaces after the ANSI color\n codes whereas the Perl version prints them before the color codes.\n Change the Perl version to match the C version to allow for introducing\n a test that verifies that both versions produce the exact same output.\n\nThanks,\nDscho\n"},{"id":"410002","messageId":"nycvar.QRO.7.76.6.2011160054460.18437@tvgsbejvaqbjf.bet","threadId":"54617","inReplyTo":"d0603db3-a6d8-d71b-1ee0-4c5c51b00da2@gmail.com","subject":"Re: [PATCH v2 07/11] add -p (built-in): do not color the progress indicator separately","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-15T23:55:57Z","receivedAt":"2020-11-16T12:38:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Fri, 13 Nov 2020, Phillip Wood wrote:\n\n> On 13/11/2020 13:57, Johannes Schindelin wrote:\n> >\n> > On Fri, 13 Nov 2020, Phillip Wood wrote:\n> >\n> > > On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:\n> > > > From: Johannes Schindelin \n> > > >\n> > > > The Perl version of this command colors the progress indicator and the\n> > > > prompt message in one go, let's do the same in the built-in version.\n> > >\n> > > Why? the C version has access to an api that means we don't have to\n> > > remember to print the reset string each time so why move away from\n> > > that? I don't think it matters to the user that there are some extra\n> > > escape codes in the prompt of the C version. The answer is probably\n> > > \"so we can use the same test as the perl version\" which might be a\n> > > good reason - if it is I think it would be helpful to say so in the\n> > > commit message.\n> >\n> > Honestly, the number one reason is so that the _same_ test passes using\n> > the Perl version as well as with the built-in version, something that is\n> > required by the `linux-clang` job in our CI build.\n>\n> That's what I assumed - it would be good to document that in the commit\n> message.\n\nI did that:\n\n add -p (built-in): do not color the progress indicator separately\n\n The Perl version of this command colors the progress indicator and the\n prompt message in one go.\n\n Let's do the same in the built-in version so that the same upcoming test\n (which will compare the output of `git add -p` against a known-good\n version) will pass both for the Perl version as well as for the built-in\n version.\n\n> > I am not really willing to change this, unless I hear a goooood reason to\n> > complicate the test.\n>\n> I guess we could change the perl version to match the C version as it is the\n> perl version that will be retired but I'm not that fussed\n\nWell, since we want to supersede the Perl version by the built-in version,\nthe latter needs to imitate the former, no?\n\nCiao,\nDscho\n"},{"id":"410003","messageId":"nycvar.QRO.7.76.6.2011160056450.18437@tvgsbejvaqbjf.bet","threadId":"54617","inReplyTo":"4A02D618-34EB-4B20-A102-99B3CDAFD0BB@gmail.com","subject":"Re: [PATCH v2 10/11] add -p: prefer color.diff.context over color.diff.plain","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-15T23:57:44Z","receivedAt":"2020-11-16T12:38:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Philippe,\n\n[to make me completely confused about which spelling for\n\"Phillip\"/\"Philippe\" to use, Philip Oakley needs to chime in.]\n\nOn Fri, 13 Nov 2020, Philippe Blain wrote:\n\n> > Le 11 nov. 2020 Γ  07:28, Johannes Schindelin via GitGitGadget a Γ©crit :\n> >\n> > From: Johannes Schindelin \n> >\n> > This still needs\n>\n> s/needs/leads/\n\nOf course!\n\nThanks,\nDscho\n\n> > to inconsistencies when both config names are used: the\n> > initial diff will be colored by the diff machinery. Once edited by a\n> > user, a hunk has to be re-colored by `git add -p`, though, which would\n> > then use the other setting to color the context lines.\n"},{"id":"410004","messageId":"nycvar.QRO.7.76.6.2011160057560.18437@tvgsbejvaqbjf.bet","threadId":"54617","inReplyTo":"4ad92124-de1f-df66-093b-5089ad111cb7@gmail.com","subject":"Re: [PATCH v2 10/11] add -p: prefer color.diff.context over color.diff.plain","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-16T00:02:34Z","receivedAt":"2020-11-16T12:38:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Fri, 13 Nov 2020, Phillip Wood wrote:\n\n> On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> >\n> > Git's diff machinery allows users to override the colors to use in\n> > diffs, even the plain-colored context lines. As of 8dbf3eb6850 (diff.h:\n> > rename DIFF_PLAIN color slot to DIFF_CONTEXT, 2015-05-27), the preferred\n> > name of the config setting is `color.diff.context`, although Git still\n> > allows `color.diff.plain`.\n> >\n> > In the context of `git add -p`, this logic is a bit hard to replicate:\n> > `git_diff_basic_config()` reads all config values sequentially and if it\n> > sees _any_ `color.diff.context` or `color.diff.plain`, it accepts the\n> > new color. The Perl version of `git add -p` needs to go through `git\n> > config --get-color`, though, which allows only one key to be specified.\n> > The same goes for the built-in version of `git add -p`, which has to go\n> > through `repo_config_get_value()`.\n>\n> One nit pick: the built-in version does not have to go through\n> `repo_config_get_value()`, it could get the values in a callback using\n> `git_config()` which would match the behavior of diff but chooses not to\n> (presumably it is more convenient to just look up the config values).\n\nOh, but that would require _all_ callers of the interactive patch\nfunctionality to be audited, to ensure that the correct `git_config()`\ncall back is used.\n\nThat's positively not what I intended.\n\nInstead, using `repo_config_get_value()` does not require that care, and\ndoes not open us up to future callers that may forget to include the\nrequired callback in _their_ config parsing.\n\nIn addition, using `repo_config_get_value()` already prepares this part of\nGit's code for a future where the `struct repository *` pointer is passed\ninto every code path that needs it, so that we no longer have to rely on\nglobal state at all.\n\nBesides, since this patch is really about fixing the discrepancy between\nthe Perl version's use of `.plain` and the built-in's use of `.context`,\nchanging something as unrelated as how the config is accessed would be\ninappropriate (even elsewhere in this patch series, which really is about\nfixing interactive `add`'s color handling).\n\nCiao,\nDscho\n"},{"id":"410014","messageId":"pull.785.v3.git.1605542912.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v2.git.1605097704.gitgitgadget@gmail.com","subject":"[PATCH v3 00/11] Fix color handling in git add -i","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-16T16:08:21Z","receivedAt":"2020-11-16T16:08:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series started out as a tiny fix for a bug reported by Philippe\nBlain in \nhttps://lore.kernel.org/git/313B8999-1E99-4695-A20D-E48840C30879@gmail.com/.\nAnd then I only wanted to add a regression test to make sure that this does\nnot regress. And then I just wanted to ensure that it passes both with the\nPerl version of git add -i as well as with the built-in version.\n\nAnd in no time I was looking at a real patch series.\n\nChanges since v2:\n\n * The commit messages of patches 7/11 and 9/11 now stress why we want to\n align the output of the Perl vs the built-in version so slavishly: to be\n able to validate both versions against prerecorded output.\n * A typo was fixed in the commit message of patch 10/11.\n\nChanges since v1:\n\n * The regression test now actually exercises the re-coloring (that is the\n primary purpose of git add -p looking at the color.diff.* variables).\n * The way the built-in git add -p renders hunk headers of split hunks was\n aligned with how the Perl version does things.\n * We now consistently prefer color.diff.context over color.diff.plain, no\n matter whether using the Perl or the built-in version of git add -p.\n * The commit message for the regression test no longer confuses readers by\n mentioning dash.\n * The regression test was structured a bit better, first testing error\n message coloring, then the menu in git add -i and then the diff coloring\n in git add -p.\n\nJohannes Schindelin (11):\n add -i (built-in): do show an error message for incorrect inputs\n add -i (built-in): send error messages to stderr\n add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk\n headers\n add -i: use `reset_color` consistently\n add -i (built-in): prevent the `reset` \"color\" from being configured\n add -i (built-in): use correct names to load color.diff.* config\n add -p (built-in): do not color the progress indicator separately\n add -i (built-in): use the same indentation as the Perl version\n add -i (Perl version): color header to match the C version\n add -p: prefer color.diff.context over color.diff.plain\n add -i: verify in the tests that colors can be overridden\n\n add-interactive.c | 38 ++++++++++-------\n add-patch.c | 25 +++++++-----\n git-add--interactive.perl | 12 +++---\n t/t3701-add-interactive.sh | 84 ++++++++++++++++++++++++++++++++++++++\n 4 files changed, 127 insertions(+), 32 deletions(-)\n\n\nbase-commit: e4d83eee9239207622e2b1cc43967da5051c189c\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-785%2Fdscho%2Ffix-add-i-colors-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-785/dscho/fix-add-i-colors-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/785\n\nRange-diff vs v2:\n\n 1: 6152122c04 = 1: 6152122c04 add -i (built-in): do show an error message for incorrect inputs\n 2: 068813912b = 2: 068813912b add -i (built-in): send error messages to stderr\n 3: 98deb538d9 = 3: 98deb538d9 add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headers\n 4: c857c44932 = 4: c857c44932 add -i: use `reset_color` consistently\n 5: 337b45cad8 = 5: 337b45cad8 add -i (built-in): prevent the `reset` \"color\" from being configured\n 6: dcd2ffc458 = 6: dcd2ffc458 add -i (built-in): use correct names to load color.diff.* config\n 7: 73b6d60a80 ! 7: 6a68bc5511 add -p (built-in): do not color the progress indicator separately\n @@ Commit message\n add -p (built-in): do not color the progress indicator separately\n \n The Perl version of this command colors the progress indicator and the\n - prompt message in one go, let's do the same in the built-in version.\n + prompt message in one go.\n +\n + Let's do the same in the built-in version so that the same upcoming test\n + (which will compare the output of `git add -p` against a known-good\n + version) will pass both for the Perl version as well as for the built-in\n + version.\n \n Signed-off-by: Johannes Schindelin \n \n 8: 91ded2fbbe = 8: 168891f9f8 add -i (built-in): use the same indentation as the Perl version\n 9: 304614751e ! 9: 094a4ad90c add -i (Perl version): include indentation in the colored header\n @@ Metadata\n Author: Johannes Schindelin \n \n ## Commit message ##\n - add -i (Perl version): include indentation in the colored header\n + add -i (Perl version): color header to match the C version\n \n - The header is formatted by padding each column heading with spaces up to\n - the length of 12 characters. These padding spaces are naturally included\n - when coloring the entire header.\n -\n - However, the preceding five spaces indentation for non-flat lists were\n - _not_ included in the Perl version, but _were_ included in the built-in\n - version. Let's adjust the former to align with the latter's behavior.\n + Both versions of `add -i` indent non-flat lists by five spaces. However\n + when using color the C version prints these spaces after the ANSI color\n + codes whereas the Perl version prints them before the color codes.\n + Change the Perl version to match the C version to allow for introducing\n + a test that verifies that both versions produce the exact same output.\n \n Signed-off-by: Johannes Schindelin \n \n 10: 48d8e0badf ! 10: 9a4d2a33b5 add -p: prefer color.diff.context over color.diff.plain\n @@ Commit message\n the hard-coded default (which in this case is simply the empty string,\n as context lines are typically rendered without colored).\n \n - This still needs to inconsistencies when both config names are used: the\n + This still leads to inconsistencies when both config names are used: the\n initial diff will be colored by the diff machinery. Once edited by a\n user, a hunk has to be re-colored by `git add -p`, though, which would\n then use the other setting to color the context lines.\n 11: c94abff72f = 11: 492f46833a add -i: verify in the tests that colors can be overridden\n\n-- \ngitgitgadget\n"},{"id":"410015","messageId":"6152122c04aacf8c3e3f1aae7b73995da6b9e354.1605542912.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v3.git.1605542912.gitgitgadget@gmail.com","subject":"[PATCH v3 01/11] add -i (built-in): do show an error message for incorrect inputs","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-16T16:08:22Z","receivedAt":"2020-11-16T16:08:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThere is a neat feature in `git add -i` where it allows users to select\nitems via unique prefixes.\n\nIn the built-in version of `git add -i`, we specifically sort the items\n(unless they are already sorted) and then perform a binary search to\nfigure out whether the input constitutes a unique prefix. Unfortunately,\nby mistake this code misidentifies matches even if the input string is\nnot actually a prefix of any item.\n\nFor example, in the initial menu, where there is a `status` and an\n`update` command, the input `tadaa` was mistaken as a prefix of\n`update`.\n\nLet's fix this by looking a bit closer whether the input is actually a\nprefix of the item at the found insert index.\n\nSigned-off-by: Johannes Schindelin \n---\n add-interactive.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 555c4abf32..8ca503d803 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -194,7 +194,8 @@ static ssize_t find_unique(const char *string, struct prefix_item_list *list)\n \telse if (index + 1 < list->sorted.nr &&\n \t\t starts_with(list->sorted.items[index + 1].string, string))\n \t\treturn -1;\n-\telse if (index < list->sorted.nr)\n+\telse if (index < list->sorted.nr &&\n+\t\t starts_with(list->sorted.items[index].string, string))\n \t\titem = list->sorted.items[index].util;\n \telse\n \t\treturn -1;\n-- \ngitgitgadget\n\n"},{"id":"410016","messageId":"068813912b9e5133f9d9bbacb239f7954788bf53.1605542912.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v3.git.1605542912.gitgitgadget@gmail.com","subject":"[PATCH v3 02/11] add -i (built-in): send error messages to stderr","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-16T16:08:23Z","receivedAt":"2020-11-16T16:09:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe Perl version of that command already does that since a301973641f\n(add -p: print errors in separate color, 2009-02-05). The built-in\nversion's development started by reimplementing the initial version from\n5cde71d64af (git-add --interactive, 2006-12-10) for simplicity, though,\nwhich still printed error messages to stdout.\n\nLet's fix that by imitating the Perl version's behavior in the built-in\nversion of that command.\n\nSigned-off-by: Johannes Schindelin \n---\n add-interactive.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 8ca503d803..0f24992ca4 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -365,7 +365,7 @@ static ssize_t list_and_choose(struct add_i_state *s,\n \n \t\t\tif (from < 0 || from >= items->items.nr ||\n \t\t\t (singleton && from + 1 != to)) {\n-\t\t\t\tcolor_fprintf_ln(stdout, s->error_color,\n+\t\t\t\tcolor_fprintf_ln(stderr, s->error_color,\n \t\t\t\t\t\t _(\"Huh (%s)?\"), p);\n \t\t\t\tbreak;\n \t\t\t} else if (singleton) {\n-- \ngitgitgadget\n\n"},{"id":"410017","messageId":"98deb538d91ba0ab485fa8d97cc6062bcc680938.1605542912.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v3.git.1605542912.gitgitgadget@gmail.com","subject":"[PATCH v3 03/11] add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headers","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-16T16:08:24Z","receivedAt":"2020-11-16T16:09:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn libxdiff, imitating GNU diff, the hunk headers only show the line\ncount if it is different from 1. When splitting hunks, the Perl version\nof `git add -p` already imitates this. Let's do the same in the built-in\nversion of said command.\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex be4cf6e9e5..b6d53229bb 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -661,9 +661,14 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\telse\n \t\t\tnew_offset += delta;\n \n-\t\tstrbuf_addf(out, \"@@ -%lu,%lu +%lu,%lu @@\",\n-\t\t\t old_offset, header->old_count,\n-\t\t\t new_offset, header->new_count);\n+\t\tstrbuf_addf(out, \"@@ -%lu\", old_offset);\n+\t\tif (header->old_count != 1)\n+\t\t\tstrbuf_addf(out, \",%lu\", header->old_count);\n+\t\tstrbuf_addf(out, \" +%lu\", new_offset);\n+\t\tif (header->new_count != 1)\n+\t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n+\t\tstrbuf_addstr(out, \" @@\");\n+\n \t\tif (len)\n \t\t\tstrbuf_add(out, p, len);\n \t\telse if (colored)\n-- \ngitgitgadget\n\n"},{"id":"410018","messageId":"c857c4493213bd7cc4d057487db2d1a74fa0bbd6.1605542912.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v3.git.1605542912.gitgitgadget@gmail.com","subject":"[PATCH v3 04/11] add -i: use `reset_color` consistently","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-16T16:08:25Z","receivedAt":"2020-11-16T16:09:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWe already maintain a list of colors in the `add_i_state`, therefore we\nshould use them.\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex b6d53229bb..bf89c43145 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -672,7 +672,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\tif (len)\n \t\t\tstrbuf_add(out, p, len);\n \t\telse if (colored)\n-\t\t\tstrbuf_addf(out, \"%s\\n\", GIT_COLOR_RESET);\n+\t\t\tstrbuf_addf(out, \"%s\\n\", s->s.reset_color);\n \t\telse\n \t\t\tstrbuf_addch(out, '\\n');\n \t}\n@@ -1065,7 +1065,7 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)\n \t\t\t s->s.file_new_color :\n \t\t\t s->s.context_color);\n \t\tstrbuf_add(&s->colored, plain + current, eol - current);\n-\t\tstrbuf_addstr(&s->colored, GIT_COLOR_RESET);\n+\t\tstrbuf_addstr(&s->colored, s->s.reset_color);\n \t\tif (next > eol)\n \t\t\tstrbuf_add(&s->colored, plain + eol, next - eol);\n \t\tcurrent = next;\n-- \ngitgitgadget\n\n"},{"id":"410020","messageId":"337b45cad8b876e1811099f398bf0a05ade21acf.1605542912.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v3.git.1605542912.gitgitgadget@gmail.com","subject":"[PATCH v3 05/11] add -i (built-in): prevent the `reset` \"color\" from being configured","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-16T16:08:26Z","receivedAt":"2020-11-16T16:09:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe Perl version of that command sneakily uses `git config --get-color`\nto figure out the ANSI sequence to reset the color, but passes the empty\nstring and therefore cannot actually match any config entry.\n\nThis was missed when re-implementing the command as a built-in command.\nLet's fix this, preventing the `reset` sequence from being overridden\nvia the config.\n\nSigned-off-by: Johannes Schindelin \n---\n add-interactive.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 0f24992ca4..f3a1d7456e 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -44,7 +44,6 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)\n \tinit_color(r, s, \"help\", s->help_color, GIT_COLOR_BOLD_RED);\n \tinit_color(r, s, \"prompt\", s->prompt_color, GIT_COLOR_BOLD_BLUE);\n \tinit_color(r, s, \"error\", s->error_color, GIT_COLOR_BOLD_RED);\n-\tinit_color(r, s, \"reset\", s->reset_color, GIT_COLOR_RESET);\n \tinit_color(r, s, \"fraginfo\", s->fraginfo_color,\n \t\t diff_get_color(s->use_color, DIFF_FRAGINFO));\n \tinit_color(r, s, \"context\", s->context_color,\n@@ -54,6 +53,9 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)\n \tinit_color(r, s, \"new\", s->file_new_color,\n \t\tdiff_get_color(s->use_color, DIFF_FILE_NEW));\n \n+\tstrlcpy(s->reset_color,\n+\t\ts->use_color ? GIT_COLOR_RESET : \"\", COLOR_MAXLEN);\n+\n \tFREE_AND_NULL(s->interactive_diff_filter);\n \tgit_config_get_string(\"interactive.difffilter\",\n \t\t\t &s->interactive_diff_filter);\n-- \ngitgitgadget\n\n"},{"id":"410019","messageId":"dcd2ffc4586960386eb8a517ff9478a8975d428c.1605542912.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v3.git.1605542912.gitgitgadget@gmail.com","subject":"[PATCH v3 06/11] add -i (built-in): use correct names to load color.diff.* config","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-16T16:08:27Z","receivedAt":"2020-11-16T16:09:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe builtin version of add-interactive mistakenly loads diff colors from\ncolor.interactive.* instead of color.diff.*. It also accidentally spells\n`frag` as `fraginfo`.\n\nLet's fix that.\n\nNote also that we don't respect the historical `diff.color.*`. The perl\nversion never did, and those have been deprecated since 2007.\n\nReported-by: Philippe Blain \nCo-authored-by: Jeff King \nSigned-off-by: Johannes Schindelin \n---\n add-interactive.c | 23 +++++++++++++----------\n 1 file changed, 13 insertions(+), 10 deletions(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex f3a1d7456e..9126684348 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -12,10 +12,10 @@\n #include \"prompt.h\"\n \n static void init_color(struct repository *r, struct add_i_state *s,\n-\t\t const char *slot_name, char *dst,\n+\t\t const char *section_and_slot, char *dst,\n \t\t const char *default_color)\n {\n-\tchar *key = xstrfmt(\"color.interactive.%s\", slot_name);\n+\tchar *key = xstrfmt(\"color.%s\", section_and_slot);\n \tconst char *value;\n \n \tif (!s->use_color)\n@@ -40,17 +40,20 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)\n \t\t\tgit_config_colorbool(\"color.interactive\", value);\n \ts->use_color = want_color(s->use_color);\n \n-\tinit_color(r, s, \"header\", s->header_color, GIT_COLOR_BOLD);\n-\tinit_color(r, s, \"help\", s->help_color, GIT_COLOR_BOLD_RED);\n-\tinit_color(r, s, \"prompt\", s->prompt_color, GIT_COLOR_BOLD_BLUE);\n-\tinit_color(r, s, \"error\", s->error_color, GIT_COLOR_BOLD_RED);\n-\tinit_color(r, s, \"fraginfo\", s->fraginfo_color,\n+\tinit_color(r, s, \"interactive.header\", s->header_color, GIT_COLOR_BOLD);\n+\tinit_color(r, s, \"interactive.help\", s->help_color, GIT_COLOR_BOLD_RED);\n+\tinit_color(r, s, \"interactive.prompt\", s->prompt_color,\n+\t\t GIT_COLOR_BOLD_BLUE);\n+\tinit_color(r, s, \"interactive.error\", s->error_color,\n+\t\t GIT_COLOR_BOLD_RED);\n+\n+\tinit_color(r, s, \"diff.frag\", s->fraginfo_color,\n \t\t diff_get_color(s->use_color, DIFF_FRAGINFO));\n-\tinit_color(r, s, \"context\", s->context_color,\n+\tinit_color(r, s, \"diff.context\", s->context_color,\n \t\tdiff_get_color(s->use_color, DIFF_CONTEXT));\n-\tinit_color(r, s, \"old\", s->file_old_color,\n+\tinit_color(r, s, \"diff.old\", s->file_old_color,\n \t\tdiff_get_color(s->use_color, DIFF_FILE_OLD));\n-\tinit_color(r, s, \"new\", s->file_new_color,\n+\tinit_color(r, s, \"diff.new\", s->file_new_color,\n \t\tdiff_get_color(s->use_color, DIFF_FILE_NEW));\n \n \tstrlcpy(s->reset_color,\n-- \ngitgitgadget\n\n"},{"id":"410021","messageId":"168891f9f8b61f958b7a32ab2bb744563d086357.1605542912.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v3.git.1605542912.gitgitgadget@gmail.com","subject":"[PATCH v3 08/11] add -i (built-in): use the same indentation as the Perl version","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-16T16:08:29Z","receivedAt":"2020-11-16T16:09:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen copying the spaces used to indent non-flat lists in `git add -i`,\none space was appended by mistake. This makes the output of the built-in\nversion of `git add -i` inconsistent with the Perl version. Let's adjust\nthe built-in version to produce the same output as the Perl version.\n\nSigned-off-by: Johannes Schindelin \n---\n add-interactive.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 9126684348..c298a8b80f 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -1137,7 +1137,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps)\n \tprint_file_item_data.color = data.color;\n \tprint_file_item_data.reset = data.reset;\n \n-\tstrbuf_addstr(&header, \" \");\n+\tstrbuf_addstr(&header, \" \");\n \tstrbuf_addf(&header, print_file_item_data.modified_fmt,\n \t\t _(\"staged\"), _(\"unstaged\"), _(\"path\"));\n \topts.list_opts.header = header.buf;\n-- \ngitgitgadget\n\n"},{"id":"410022","messageId":"6a68bc5511a72d25b84a243dd5d8019f81cee610.1605542912.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v3.git.1605542912.gitgitgadget@gmail.com","subject":"[PATCH v3 07/11] add -p (built-in): do not color the progress indicator separately","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-16T16:08:28Z","receivedAt":"2020-11-16T16:09:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe Perl version of this command colors the progress indicator and the\nprompt message in one go.\n\nLet's do the same in the built-in version so that the same upcoming test\n(which will compare the output of `git add -p` against a known-good\nversion) will pass both for the Perl version as well as for the built-in\nversion.\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex bf89c43145..2fad92ca37 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1461,15 +1461,15 @@ static int patch_update_file(struct add_p_state *s,\n \t\telse\n \t\t\tprompt_mode_type = PROMPT_HUNK;\n \n-\t\tcolor_fprintf(stdout, s->s.prompt_color,\n-\t\t\t \"(%\"PRIuMAX\"/%\"PRIuMAX\") \",\n+\t\tprintf(\"%s(%\"PRIuMAX\"/%\"PRIuMAX\") \", s->s.prompt_color,\n \t\t\t (uintmax_t)hunk_index + 1,\n \t\t\t (uintmax_t)(file_diff->hunk_nr\n \t\t\t\t\t\t? file_diff->hunk_nr\n \t\t\t\t\t\t: 1));\n-\t\tcolor_fprintf(stdout, s->s.prompt_color,\n-\t\t\t _(s->mode->prompt_mode[prompt_mode_type]),\n-\t\t\t s->buf.buf);\n+\t\tprintf(_(s->mode->prompt_mode[prompt_mode_type]),\n+\t\t s->buf.buf);\n+\t\tif (*s->s.reset_color)\n+\t\t\tfputs(s->s.reset_color, stdout);\n \t\tfflush(stdout);\n \t\tif (read_single_character(s) == EOF)\n \t\t\tbreak;\n-- \ngitgitgadget\n\n"},{"id":"410023","messageId":"492f46833a3fee22cef4e1ecfb341f20b5471e9c.1605542912.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v3.git.1605542912.gitgitgadget@gmail.com","subject":"[PATCH v3 11/11] add -i: verify in the tests that colors can be overridden","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-16T16:08:32Z","receivedAt":"2020-11-16T16:09:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nNow that the Perl version produces the same output as the built-in\nversion (mostly fixing bugs in the latter), let's add a regression test\nto verify that it stays this way.\n\nNote that we only `grep` for the colored error message instead of\nverifying that the entire `stderr` consists of just this one line: when\nrunning the test script using the `-x` option to trace the\ncommands, the sub-shell in `force_color` causes those commands to be\ntraced into `err.raw` (unless running in Bash where we set the\n`BASH_XTRACEFD` variable to avoid that).\n\nAlso note that the color reset in the `+new`\nline might look funny and unnecessary, as the corresponding `old` line\ndoes not reset the color after the diff marker only to turn the color\nback on right away.\n\nHowever, this is a (necessary) side effect of the white-space check: in\n`emit_line_ws_markup()`, we first emit the diff marker via\n`emit_line_0()` and then the rest of the line via `ws_check_emit()`. To\nleave them somewhat decoupled, the color has to be reset after the diff\nmarker to allow for the rest of the line to start with another color (or\ninverted, in case of white-space issues).\n\nFinally, we have to simulate hunk editing: the `git add -p` command\ncannot rely on the internal diff machinery for coloring after letting\nthe user edit a hunk; It has to \"re-color\" the edited hunk. This is the\nprimary reason why that command is interested in the exact values of the\n`color.diff.*` settings in the first place. To test this re-coloring, we\ntherefore have to pretend to edit a hunk and then show that hunk in the\nregression test.\n\nCo-authored-by: Jeff King \nSigned-off-by: Johannes Schindelin \n---\n t/t3701-add-interactive.sh | 84 ++++++++++++++++++++++++++++++++++++++\n 1 file changed, 84 insertions(+)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex ca04fac417..cc3f434a97 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -589,6 +589,90 @@ test_expect_success 'diffs can be colorized' '\n \tgrep \"$(printf \"\\\\033\")\" output\n '\n \n+test_expect_success 'colors can be overridden' '\n+\tgit reset --hard &&\n+\ttest_when_finished \"git rm -f color-test\" &&\n+\ttest_write_lines context old more-context >color-test &&\n+\tgit add color-test &&\n+\ttest_write_lines context new more-context another-one >color-test &&\n+\n+\techo trigger an error message >input &&\n+\tforce_color git \\\n+\t\t-c color.interactive.error=blue \\\n+\t\tadd -i 2>err.raw err &&\n+\tgrep \"Huh (trigger)?\" err &&\n+\n+\ttest_write_lines help quit >input &&\n+\tforce_color git \\\n+\t\t-c color.interactive.header=red \\\n+\t\t-c color.interactive.help=green \\\n+\t\t-c color.interactive.prompt=yellow \\\n+\t\tadd -i >actual.raw actual &&\n+\tcat >expect <<-\\EOF &&\n+\t staged unstaged path\n+\t 1: +3/-0 +2/-1 color-test\n+\n+\t*** Commands ***\n+\t 1: status\t 2: update\t 3: revert\t 4: add untracked\n+\t 5: patch\t 6: diff\t 7: quit\t 8: help\n+\tWhat now> status - show paths with changes\n+\tupdate - add working tree state to the staged set of changes\n+\trevert - revert staged set of changes back to the HEAD version\n+\tpatch - pick hunks and update selectively\n+\tdiff - view diff between HEAD and index\n+\tadd untracked - add contents of untracked files to the staged set of changes\n+\t*** Commands ***\n+\t 1: status\t 2: update\t 3: revert\t 4: add untracked\n+\t 5: patch\t 6: diff\t 7: quit\t 8: help\n+\tWhat now> Bye.\n+\tEOF\n+\ttest_cmp expect actual &&\n+\n+\t: exercise recolor_hunk by editing and then look at the hunk again &&\n+\ttest_write_lines s e K q >input &&\n+\tforce_color git \\\n+\t\t-c color.interactive.prompt=yellow \\\n+\t\t-c color.diff.meta=italic \\\n+\t\t-c color.diff.frag=magenta \\\n+\t\t-c color.diff.context=cyan \\\n+\t\t-c color.diff.old=bold \\\n+\t\t-c color.diff.new=blue \\\n+\t\t-c core.editor=touch \\\n+\t\tadd -p >actual.raw actual.decoded &&\n+\tsed \"s/index [0-9a-f]*\\\\.\\\\.[0-9a-f]* 100644//\" actual &&\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/color-test b/color-test\n+\t\n+\t--- a/color-test\n+\t+++ b/color-test\n+\t@@ -1,3 +1,4 @@\n+\t context\n+\t-old\n+\t+new\n+\t more-context\n+\t+another-one\n+\t(1/1) Stage this hunk [y,n,q,a,d,s,e,?]? Split into 2 hunks.\n+\t@@ -1,3 +1,3 @@\n+\t context\n+\t-old\n+\t+new\n+\t more-context\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? @@ -3 +3,2 @@\n+\t more-context\n+\t+another-one\n+\t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? @@ -1,3 +1,3 @@\n+\t context\n+\t-old\n+\t+new\n+\t more-context\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? \n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '\n \tgit reset --hard &&\n \n-- \ngitgitgadget\n"},{"id":"410024","messageId":"094a4ad90ced8756ded9a225d2fd3886574ef6fc.1605542912.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v3.git.1605542912.gitgitgadget@gmail.com","subject":"[PATCH v3 09/11] add -i (Perl version): color header to match the C version","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-16T16:08:30Z","receivedAt":"2020-11-16T16:09:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nBoth versions of `add -i` indent non-flat lists by five spaces. However\nwhen using color the C version prints these spaces after the ANSI color\ncodes whereas the Perl version prints them before the color codes.\nChange the Perl version to match the C version to allow for introducing\na test that verifies that both versions produce the exact same output.\n\nSigned-off-by: Johannes Schindelin \n---\n git-add--interactive.perl | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex e713fe3d02..adbac2bc6d 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -483,10 +483,8 @@ sub list_and_choose {\n \t\tmy $last_lf = 0;\n \n \t\tif ($opts->{HEADER}) {\n-\t\t\tif (!$opts->{LIST_FLAT}) {\n-\t\t\t\tprint \" \";\n-\t\t\t}\n-\t\t\tprint colored $header_color, \"$opts->{HEADER}\\n\";\n+\t\t\tmy $indent = $opts->{LIST_FLAT} ? \"\" : \" \";\n+\t\t\tprint colored $header_color, \"$indent$opts->{HEADER}\\n\";\n \t\t}\n \t\tfor ($i = 0; $i < @stuff; $i++) {\n \t\t\tmy $chosen = $chosen[$i] ? '*' : ' ';\n-- \ngitgitgadget\n\n"},{"id":"410025","messageId":"9a4d2a33b50344e97e2bc47e5aeb0d3fcbc18a6c.1605542912.git.gitgitgadget@gmail.com","threadId":"54617","inReplyTo":"pull.785.v3.git.1605542912.gitgitgadget@gmail.com","subject":"[PATCH v3 10/11] add -p: prefer color.diff.context over color.diff.plain","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-16T16:08:31Z","receivedAt":"2020-11-16T16:09:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nGit's diff machinery allows users to override the colors to use in\ndiffs, even the plain-colored context lines. As of 8dbf3eb6850 (diff.h:\nrename DIFF_PLAIN color slot to DIFF_CONTEXT, 2015-05-27), the preferred\nname of the config setting is `color.diff.context`, although Git still\nallows `color.diff.plain`.\n\nIn the context of `git add -p`, this logic is a bit hard to replicate:\n`git_diff_basic_config()` reads all config values sequentially and if it\nsees _any_ `color.diff.context` or `color.diff.plain`, it accepts the\nnew color. The Perl version of `git add -p` needs to go through `git\nconfig --get-color`, though, which allows only one key to be specified.\nThe same goes for the built-in version of `git add -p`, which has to go\nthrough `repo_config_get_value()`.\n\nThe best we can do here is to look for `.context` and if none is found,\nfall back to looking for `.plain`, and if still not found, fall back to\nthe hard-coded default (which in this case is simply the empty string,\nas context lines are typically rendered without colored).\n\nThis still leads to inconsistencies when both config names are used: the\ninitial diff will be colored by the diff machinery. Once edited by a\nuser, a hunk has to be re-colored by `git add -p`, though, which would\nthen use the other setting to color the context lines.\n\nIn practice, this is not _all_ that bad. The `git config` manual says\nthis in the `color.diff.`:\n\n\t`context` (context text - `plain` is a historical synonym)\n\nWe should therefore assume that users use either one or the other, but\nnot both names. Besides, it is relatively uncommon to look at a hunk\nafter editing it because it is immediately staged by default.\n\nSigned-off-by: Johannes Schindelin \n---\n add-interactive.c | 6 ++++--\n git-add--interactive.perl | 6 +++---\n 2 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex c298a8b80f..54dfdc56f5 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -49,8 +49,10 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)\n \n \tinit_color(r, s, \"diff.frag\", s->fraginfo_color,\n \t\t diff_get_color(s->use_color, DIFF_FRAGINFO));\n-\tinit_color(r, s, \"diff.context\", s->context_color,\n-\t\tdiff_get_color(s->use_color, DIFF_CONTEXT));\n+\tinit_color(r, s, \"diff.context\", s->context_color, \"fall back\");\n+\tif (!strcmp(s->context_color, \"fall back\"))\n+\t\tinit_color(r, s, \"diff.plain\", s->context_color,\n+\t\t\t diff_get_color(s->use_color, DIFF_CONTEXT));\n \tinit_color(r, s, \"diff.old\", s->file_old_color,\n \t\tdiff_get_color(s->use_color, DIFF_FILE_OLD));\n \tinit_color(r, s, \"diff.new\", s->file_new_color,\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex adbac2bc6d..bc3a1e8eff 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -30,9 +30,9 @@\n \t$diff_use_color ? (\n \t\t$repo->get_color('color.diff.frag', 'cyan'),\n \t) : ();\n-my ($diff_plain_color) =\n+my ($diff_context_color) =\n \t$diff_use_color ? (\n-\t\t$repo->get_color('color.diff.plain', ''),\n+\t\t$repo->get_color($repo->config('color.diff.context') ? 'color.diff.context' : 'color.diff.plain', ''),\n \t) : ();\n my ($diff_old_color) =\n \t$diff_use_color ? (\n@@ -1046,7 +1046,7 @@ sub color_diff {\n \t\tcolored((/^@/ ? $fraginfo_color :\n \t\t\t /^\\+/ ? $diff_new_color :\n \t\t\t /^-/ ? $diff_old_color :\n-\t\t\t $diff_plain_color),\n+\t\t\t $diff_context_color),\n \t\t\t$_);\n \t} @_;\n }\n-- \ngitgitgadget\n\n"},{"id":"410059","messageId":"20201117014435.GA19433@coredump.intra.peff.net","threadId":"54617","inReplyTo":"nycvar.QRO.7.76.6.2011160027480.18437@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 9/9] add -i: verify in the tests that colors can be overridden","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-17T01:44:35Z","receivedAt":"2020-11-17T01:44:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 16, 2020 at 12:35:44AM +0100, Johannes Schindelin wrote:\n\n> > Whereas add-patch parses the colors off of the version and then\n> > re-colors every hunk header. Which seems doubly weird to me. Even if we\n> > were going to re-color every hunk (e.g., because we don't want to store\n> > the original hunk line, but instead a parsed version of it), why bother\n> > parsing the color version at all, and not just the machine-readable\n> > version?\n> \n> Let's continue on this distraction for a bit before I go back to fixing\n> the patch series, which actually tries to fix a _different_ concern.\n> \n> The reason why `add-patch.c` \"parses the colors off\" is that we want to\n> show the rest of the hunk header, in color, even after splitting hunks\n> (this will only be shown for the first hunk, of course).\n\nOK, this is the part I didn't quite understand. My contention was: if we\nare regenerating a new hunk header after we split, why do we care what\nwas in the old one?\n\nThis \"rest of the hunk header\" is what I didn't get. You are talking\nhere about the funcname header. Which is not colored by default, but\npeople can set color.diff.func.\n\nAnd here the builtin does differ from the perl script quite a bit. The\nperl script generates the split hunk headers from scratch, and does not\nbother to include the funcname in the split header:\n\n $ git show :foo.c\n void foo()\n {\n \tcall();\n \tsome();\n \tfunctions();\n }\n \n $ cat foo.c\n void foo()\n {\n \tcall();\n \tsome();\n \tmore();\n \tfunctions();\n \treturn;\n }\n\n $ git add -p\n diff --git a/foo.c b/foo.c\n index 270ccc7..0365419 100644\n --- a/foo.c\n +++ b/foo.c\n @@ -2,5 +2,7 @@ void foo()\n {\n \tcall();\n \tsome();\n +\tmore();\n \tfunctions();\n +\treturn;\n }\n (1/1) Stage this hunk [y,n,q,a,d,s,e,?]? s\n Split into 2 hunks.\n @@ -2,4 +2,5 @@\n {\n \tcall();\n \tsome();\n +\tmore();\n \tfunctions();\n (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? n\n @@ -5,2 +6,3 @@\n \tfunctions();\n +\treturn;\n }\n (2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? n\n\nSo it does not appear at all after the split, colored or otherwise. I\nthink the behavior of the builtin version is better in this case.\n\nIt does place more restrictions on what the diffFilter can do, as we've\nbeen discussing. The same thing could be accomplished by retaining the\nuncolored func header, and then coloring it on the fly within\nadd-patch.c (exactly the same way we color the numeric part of the hunk\nheader).\n\nIt may also be worth covering this with a test (it was a surprise to me\nthat the builtin version was handling this, though as I said I do agree\nit's an improvement).\n\n> But given that `git add -p` is somewhat of a fringe use, and using\n> `diffFilter` is _even_ more fringe, I do not really want to spend any\n> further time on this tangent.\n\nI agree that diffFilter is quite fringe. And as I said before, I'm fine\npunting on it for now. Mostly this was just the first I had found out\nabout the difference, and because it is user-visible, it may be\nsomething that comes up later. So I wanted to record a description of\nthe problem for later.\n\n> My thinking back then was: what if _I_ want to use a diffFilter? For what\n> would I use it? Probably to emphasize certain hunk headers more, by adding\n> more color to the part after the line range.\n\nYes, that's what I use it for. I wrote it mostly to pipe through\ndiff-highlight, though also it was to help diff-so-fancy folks (which I\ndon't use myself, but they build on diff-highlight). However, it does\nseem that they haven't really worked out the problems in the meantime.\n\n-Peff\n"},{"id":"410060","messageId":"20201117014937.GB19433@coredump.intra.peff.net","threadId":"54617","inReplyTo":"nycvar.QRO.7.76.6.2011160036100.18437@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 00/11] Fix color handling in git add -i","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-17T01:49:37Z","receivedAt":"2020-11-17T01:49:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 16, 2020 at 12:40:32AM +0100, Johannes Schindelin wrote:\n\n> > All of which is to say that the current state is a bit of a mess, and I\n> > don't consider it completely necessary to fix it before the builtin\n> > version becomes the default. But:\n> >\n> > - I think we should expect some possible complaints / bug reports\n> > from fringe users of the already-somewhat-broken state\n> >\n> > - IMHO the parsing of the filtered version done by the builtin is\n> > going in the wrong direction (see my other mail for an elaboration)\n> \n> It's a little bit of a surprise that this is the first time I hear about\n> this.\n\nWell, it is the first time I am finding out about it, too. ;)\n\n> The way _I_ would go about fixing this is to look for a tell-tale that\n> we're looking at a `diff-so-fancy` style output, e.g. by looking for that\n> horizontal line, then adding special code to handle that.\n\nIMHO that's the wrong direction. My intent with the feature was to give\nfilters as much flexibility as possible, and beyond having 1-to-1\ncorrespondence of lines, we don't know or care what they produce. We\ndon't even know what other filters might exist out there (though I would\nbe surprised if there is anything more exotic than intra-line coloring;\nother folks have pointed to \"delta\" for this use, but only in its\ncolor-only mode).\n\nIf anybody wants to put work into it, I think a better direction is\nhaving Git keep its own colored hunks and just present single-hunks to a\ncustom display command to show to the user. Then we can continue not to\ncare about the format of the individual filters, _and_ they can get the\nopportunity to format our rewritten split-hunk headers, etc.\n\n> But given that it does not work right now, and given that it has not been\n> working for a long time, I think it does not hurt so much if it is left in\n> the current state for a bit longer. I would love to focus on the patch\n> series that kicked off this discussion first, getting it done, before I\n> think about other `add -p` aspects.\n\nYeah, absolutely.\n\n-Peff\n"},{"id":"410061","messageId":"20201117015149.GC19433@coredump.intra.peff.net","threadId":"54617","inReplyTo":"pull.785.v3.git.1605542912.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 00/11] Fix color handling in git add -i","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-17T01:51:49Z","receivedAt":"2020-11-17T01:52:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 16, 2020 at 04:08:21PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> Changes since v2:\n> \n> * The commit messages of patches 7/11 and 9/11 now stress why we want to\n> align the output of the Perl vs the built-in version so slavishly: to be\n> able to validate both versions against prerecorded output.\n> * A typo was fixed in the commit message of patch 10/11.\n\nThis version looks fine to me, and I agree is a good stopping point for\nnow (the only thing I'd consider adding is a test for the funcname in a\nsplit-hunk header, but it would have to be expect_failure for now; it is\nprobably not worth the effort to rewrite the perl version to behave like\nthe builtin here if we think it's going away soon).\n\n-Peff\n"},{"id":"410062","messageId":"20201117020522.GD19433@coredump.intra.peff.net","threadId":"54617","inReplyTo":"20201117015149.GC19433@coredump.intra.peff.net","subject":"Re: [PATCH v3 00/11] Fix color handling in git add -i","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-17T02:05:22Z","receivedAt":"2020-11-17T02:05:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 16, 2020 at 08:51:49PM -0500, Jeff King wrote:\n\n> On Mon, Nov 16, 2020 at 04:08:21PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> \n> > Changes since v2:\n> > \n> > * The commit messages of patches 7/11 and 9/11 now stress why we want to\n> > align the output of the Perl vs the built-in version so slavishly: to be\n> > able to validate both versions against prerecorded output.\n> > * A typo was fixed in the commit message of patch 10/11.\n> \n> This version looks fine to me, and I agree is a good stopping point for\n> now (the only thing I'd consider adding is a test for the funcname in a\n> split-hunk header, but it would have to be expect_failure for now; it is\n> probably not worth the effort to rewrite the perl version to behave like\n> the builtin here if we think it's going away soon).\n\nActually, I couldn't resist (but again, I don't think this has to be\npart of your series):\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex e713fe3d02..90561ea0e2 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -30,6 +30,10 @@\n \t$diff_use_color ? (\n \t\t$repo->get_color('color.diff.frag', 'cyan'),\n \t) : ();\n+my ($funcname_color) =\n+\t$diff_use_color ? (\n+\t\t$repo->get_color('color.diff.func', ''),\n+\t) : ();\n my ($diff_plain_color) =\n \t$diff_use_color ? (\n \t\t$repo->get_color('color.diff.plain', ''),\n@@ -780,11 +784,11 @@ sub hunk_splittable {\n \n sub parse_hunk_header {\n \tmy ($line) = @_;\n-\tmy ($o_ofs, $o_cnt, $n_ofs, $n_cnt) =\n-\t $line =~ /^@@ -(\\d+)(?:,(\\d+))? \\+(\\d+)(?:,(\\d+))? @@/;\n+\tmy ($o_ofs, $o_cnt, $n_ofs, $n_cnt, $funcname) =\n+\t $line =~ /^@@ -(\\d+)(?:,(\\d+))? \\+(\\d+)(?:,(\\d+))? @@ ?(.*)/;\n \t$o_cnt = 1 unless defined $o_cnt;\n \t$n_cnt = 1 unless defined $n_cnt;\n-\treturn ($o_ofs, $o_cnt, $n_ofs, $n_cnt);\n+\treturn ($o_ofs, $o_cnt, $n_ofs, $n_cnt, $funcname);\n }\n \n sub format_hunk_header {\n@@ -806,7 +810,8 @@ sub split_hunk {\n \t# it can be split, but we would need to take care of\n \t# overlaps later.\n \n-\tmy ($o_ofs, undef, $n_ofs) = parse_hunk_header($text->[0]);\n+\tmy ($o_ofs, undef, $n_ofs, undef, $funcname) =\n+\t\tparse_hunk_header($text->[0]);\n \tmy $hunk_start = 1;\n \n OUTER:\n@@ -894,6 +899,11 @@ sub split_hunk {\n \t\tif ($diff_use_color) {\n \t\t\t$display_head = colored($fraginfo_color, $head);\n \t\t}\n+\t\tif (length $funcname) {\n+\t\t\tmy $f = colored($funcname_color, $funcname);\n+\t\t\t$display_head =~ s/$/ $f/;\n+\t\t\t$funcname = '';\n+\t\t}\n \t\tunshift @{$hunk->{DISPLAY}}, $display_head;\n \t}\n \treturn @split;\n"},{"id":"410808","messageId":"xmqqtutdcd2h.fsf@gitster.c.googlers.com","threadId":"54617","inReplyTo":"20201117015149.GC19433@coredump.intra.peff.net","subject":"Re: [PATCH v3 00/11] Fix color handling in git add -i","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-25T21:59:50Z","receivedAt":"2020-11-25T22:00:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Nov 16, 2020 at 04:08:21PM +0000, Johannes Schindelin via GitGitGadget wrote:\n>\n>> Changes since v2:\n>> \n>> * The commit messages of patches 7/11 and 9/11 now stress why we want to\n>> align the output of the Perl vs the built-in version so slavishly: to be\n>> able to validate both versions against prerecorded output.\n>> * A typo was fixed in the commit message of patch 10/11.\n>\n> This version looks fine to me, and I agree is a good stopping point for\n> now (the only thing I'd consider adding is a test for the funcname in a\n> split-hunk header, but it would have to be expect_failure for now; it is\n> probably not worth the effort to rewrite the perl version to behave like\n> the builtin here if we think it's going away soon).\n\nWill mark the topic to be merged to 'next'.\n\nIt seems that these days I am too quick and eager to take more\nthings in 'seen' than the rate at I can convince myself that the\ntopics are ready and accumulate too many topics in the \"undecided\"\ncategory too fast. I probably should slow down the intake a bit to\nmatch.\n\nThanks.\n"}]}