Fix color handling in git add -i

This patch series started out as a tiny fix for a bug reported in
https://lore.kernel.org/git/[email protected]/.
And then I only wanted to add a regression test to make sure that this does not regress. And then I just wanted to ensure that it passes both with the Perl version of git add -i as well as with the built-in version.

And in no time I was looking at a real patch series.

Johannes Schindelin (9):
add -i (built-in): do show an error message for incorrect inputs add -i (built-in): send error messages to stderr
add -i: use reset_color consistently
add -i (built-in): prevent the reset "color" from being configured add -i (built-in): use correct names to load color.diff.* config add -p (built-in): do not color the progress indicator separately add -i (built-in): use the same indentation as the Perl version add -i (Perl version): include indentation in the colored header add -i: verify in the tests that colors can be overridden

 add-interactive.c          | 34 ++++++++++++++----------
 add-patch.c                | 14 +++++-----
 git-add--interactive.perl  |  6 ++---
 t/t3701-add-interactive.sh | 53 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 82 insertions(+), 25 deletions(-)

base-commit: e4d83eee9239207622e2b1cc43967da5051c189c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-785%2Fdscho%2Ffix-add-i-colors-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-785/dscho/fix-add-i-colors-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/785

gitgitgadget

From: Johannes Schindelin <[email protected]>

There is a neat feature in git add -i where it allows users to select items via unique prefixes.

In the built-in version of git add -i, we specifically sort the items (unless they are already sorted) and then perform a binary search to figure out whether the input constitutes a unique prefix. Unfortunately, by mistake this code misidentifies matches even if the input string is not actually a prefix of any item.

For example, in the initial menu, where there is a status and an update command, the input tadaa was mistaken as a prefix of update.

Let's fix this by looking a bit closer whether the input is actually a prefix of the item at the found insert index.

---
 add-interactive.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 555c4abf32..8ca503d803 100644
@@ -194,7 +194,8 @@ static ssize_t find_unique(const char *string, struct prefix_item_list *list)
 	else if (index + 1 < list->sorted.nr &&
 		 starts_with(list->sorted.items[index + 1].string, string))
 		return -1;
-	else if (index < list->sorted.nr)
+	else if (index < list->sorted.nr &&
+		 starts_with(list->sorted.items[index].string, string))
 		item = list->sorted.items[index].util;
 	else
 		return -1;

gitgitgadget

From: Johannes Schindelin <[email protected]>

The Perl version of that command already does that since a301973641f (add -p: print errors in separate color, 2009-02-05). The built-in version's development started by reimplementing the initial version from 5cde71d64af (git-add --interactive, 2006-12-10) for simplicity, though, which still printed error messages to stdout.

Let's fix that by imitating the Perl version's behavior in the built-in version of that command.

---
 add-interactive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 8ca503d803..0f24992ca4 100644
@@ -365,7 +365,7 @@ static ssize_t list_and_choose(struct add_i_state *s,
 
 			if (from < 0 || from >= items->items.nr ||
 			    (singleton && from + 1 != to)) {
-				color_fprintf_ln(stdout, s->error_color,
+				color_fprintf_ln(stderr, s->error_color,
 						 _("Huh (%s)?"), p);
 				break;
 			} else if (singleton) {

gitgitgadget

From: Johannes Schindelin <[email protected]>

When copying the spaces used to indent non-flat lists in git add -i, one space was appended by mistake. This makes the output of the built-in version of git add -i inconsistent with the Perl version. Let's adjust the built-in version to produce the same output as the Perl version.

---
 add-interactive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 9126684348..c298a8b80f 100644
@@ -1137,7 +1137,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps)
 	print_file_item_data.color = data.color;
 	print_file_item_data.reset = data.reset;
 
-	strbuf_addstr(&header, "      ");
+	strbuf_addstr(&header, "     ");
 	strbuf_addf(&header, print_file_item_data.modified_fmt,
 		    _("staged"), _("unstaged"), _("path"));
 	opts.list_opts.header = header.buf;

gitgitgadget

From: Johannes Schindelin <[email protected]>

The header is formatted by padding each column heading with spaces up to the length of 12 characters. These padding spaces are naturally included when coloring the entire header.

However, the preceding five spaces indentation for non-flat lists were _not_ included in the Perl version, but _were_ included in the built-in version. Let's adjust the former to align with the latter's behavior.

---
 git-add--interactive.perl | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index e713fe3d02..adbac2bc6d 100755
@@ -483,10 +483,8 @@ sub list_and_choose {
 		my $last_lf = 0;
 
 		if ($opts->{HEADER}) {
-			if (!$opts->{LIST_FLAT}) {
-				print "     ";
-			}
-			print colored $header_color, "$opts->{HEADER}\n";
+			my $indent = $opts->{LIST_FLAT} ? "" : "     ";
+			print colored $header_color, "$indent$opts->{HEADER}\n";
 		}
 		for ($i = 0; $i < @stuff; $i++) {
 			my $chosen = $chosen[$i] ? '*' : ' ';

gitgitgadget

From: Johannes Schindelin <[email protected]>

We already maintain a list of colors in the add_i_state, therefore we should use them.

---
 add-patch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index be4cf6e9e5..33a24f58fe 100644
@@ -667,7 +667,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
 		if (len)
 			strbuf_add(out, p, len);
 		else if (colored)
-			strbuf_addf(out, "%s\n", GIT_COLOR_RESET);
+			strbuf_addf(out, "%s\n", s->s.reset_color);
 		else
 			strbuf_addch(out, '\n');
 	}

@@ -1060,7 +1060,7 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)
 			      s->s.file_new_color :
 			      s->s.context_color);
 		strbuf_add(&s->colored, plain + current, eol - current);
-		strbuf_addstr(&s->colored, GIT_COLOR_RESET);
+		strbuf_addstr(&s->colored, s->s.reset_color);
 		if (next > eol)
 			strbuf_add(&s->colored, plain + eol, next - eol);
 		current = next;

gitgitgadget

From: Johannes Schindelin <[email protected]>

The builtin version of add-interactive mistakenly loads diff colors from color.interactive.* instead of color.diff.*. It also accidentally spells frag as fraginfo.

Let's fix that.

Note also that we don't respect the historical diff.color.*. The perl version never did, and those have been deprecated since 2007.

---
 add-interactive.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

index f3a1d7456e..9126684348 100644
@@ -12,10 +12,10 @@
 #include "prompt.h"
 
 static void init_color(struct repository *r, struct add_i_state *s,
-		       const char *slot_name, char *dst,
+		       const char *section_and_slot, char *dst,
 		       const char *default_color)
 {
-	char *key = xstrfmt("color.interactive.%s", slot_name);
+	char *key = xstrfmt("color.%s", section_and_slot);
 	const char *value;
 
 	if (!s->use_color)

@@ -40,17 +40,20 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 			git_config_colorbool("color.interactive", value);
 	s->use_color = want_color(s->use_color);
 
-	init_color(r, s, "header", s->header_color, GIT_COLOR_BOLD);
-	init_color(r, s, "help", s->help_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE);
-	init_color(r, s, "error", s->error_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "fraginfo", s->fraginfo_color,
+	init_color(r, s, "interactive.header", s->header_color, GIT_COLOR_BOLD);
+	init_color(r, s, "interactive.help", s->help_color, GIT_COLOR_BOLD_RED);
+	init_color(r, s, "interactive.prompt", s->prompt_color,
+		   GIT_COLOR_BOLD_BLUE);
+	init_color(r, s, "interactive.error", s->error_color,
+		   GIT_COLOR_BOLD_RED);
+
+	init_color(r, s, "diff.frag", s->fraginfo_color,
 		   diff_get_color(s->use_color, DIFF_FRAGINFO));
-	init_color(r, s, "context", s->context_color,
+	init_color(r, s, "diff.context", s->context_color,
 		diff_get_color(s->use_color, DIFF_CONTEXT));
-	init_color(r, s, "old", s->file_old_color,
+	init_color(r, s, "diff.old", s->file_old_color,
 		diff_get_color(s->use_color, DIFF_FILE_OLD));
-	init_color(r, s, "new", s->file_new_color,
+	init_color(r, s, "diff.new", s->file_new_color,
 		diff_get_color(s->use_color, DIFF_FILE_NEW));
 
 	strlcpy(s->reset_color,

gitgitgadget

From: Johannes Schindelin <[email protected]>

Now that the Perl version produces the same output as the built-in version (mostly fixing bugs in the latter), let's add a regression test to verify that it stays this way.

Note that we only grep for the colored error message instead of verifying that the entire stderr consists of just this one line: when running the test script via dash, using the -x option to trace the commands, the sub-shell in force_color causes those commands to be traced into err.raw because we set, but do not export BASH_XTRACEFD).

---
 t/t3701-add-interactive.sh | 53 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

index ca04fac417..28549a41a2 100755
@@ -589,6 +589,59 @@ test_expect_success 'diffs can be colorized' '
 	grep "$(printf "\\033")" output
 '
 
+test_expect_success 'colors can be overridden' '
+	test_config color.interactive.header red &&
+	test_config color.interactive.help green &&
+	test_config color.interactive.prompt yellow &&
+	test_config color.interactive.error blue &&
+	test_config color.diff.frag magenta &&
+	test_config color.diff.context cyan &&
+	test_config color.diff.old bold &&
+	test_config color.diff.new "bold red" &&
+
+	git reset --hard &&
+	test_when_finished "git rm -f color-test" &&
+	test_write_lines context old more-context >color-test &&
+	git add color-test &&
+	test_write_lines context new more-context >color-test &&
+
+	echo trigger an error message >input &&
+	force_color git add -i 2>err.raw <input &&
+	test_decode_color <err.raw >err &&
+	grep "<BLUE>Huh (trigger)?<RESET>" err &&
+
+	test_write_lines patch color-test "" y quit >input &&
+	force_color git add -i >actual.raw <input &&
+	test_decode_color <actual.raw >actual.decoded &&
+	sed "/index [0-9a-f]*\\.\\.[0-9a-f]* 100644/d" <actual.decoded >actual &&
+	cat >expect <<-\EOF &&
+	<RED>           staged     unstaged path<RESET>
+	  1:        +3/-0        +1/-1 color-test
+
+	<RED>*** Commands ***<RESET>
+	  1: <YELLOW>s<RESET>tatus	  2: <YELLOW>u<RESET>pdate	  3: <YELLOW>r<RESET>evert	  4: <YELLOW>a<RESET>dd untracked
+	  5: <YELLOW>p<RESET>atch	  6: <YELLOW>d<RESET>iff	  7: <YELLOW>q<RESET>uit	  8: <YELLOW>h<RESET>elp
+	<YELLOW>What now<RESET>> <RED>           staged     unstaged path<RESET>
+	  1:        +3/-0        +1/-1 <YELLOW>c<RESET>olor-test
+	<YELLOW>Patch update<RESET>>> <RED>           staged     unstaged path<RESET>
+	* 1:        +3/-0        +1/-1 <YELLOW>c<RESET>olor-test
+	<YELLOW>Patch update<RESET>>> <BOLD>diff --git a/color-test b/color-test<RESET>
+	<BOLD>--- a/color-test<RESET>
+	<BOLD>+++ b/color-test<RESET>
+	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
+	<CYAN> context<RESET>
+	<BOLD>-old<RESET>
+	<BOLD;RED>+<RESET><BOLD;RED>new<RESET>
+	<CYAN> more-context<RESET>
+	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,e,?]? <RESET>
+	<RED>*** Commands ***<RESET>
+	  1: <YELLOW>s<RESET>tatus	  2: <YELLOW>u<RESET>pdate	  3: <YELLOW>r<RESET>evert	  4: <YELLOW>a<RESET>dd untracked
+	  5: <YELLOW>p<RESET>atch	  6: <YELLOW>d<RESET>iff	  7: <YELLOW>q<RESET>uit	  8: <YELLOW>h<RESET>elp
+	<YELLOW>What now<RESET>> Bye.
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '
 	git reset --hard &&
 

gitgitgadget

From: Johannes Schindelin <[email protected]>

The Perl version of this command colors the progress indicator and the prompt message in one go, let's do the same in the built-in version.

---
 add-patch.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index 33a24f58fe..6f4c187238 100644
@@ -1456,15 +1456,15 @@ static int patch_update_file(struct add_p_state *s,
 		else
 			prompt_mode_type = PROMPT_HUNK;
 
-		color_fprintf(stdout, s->s.prompt_color,
-			      "(%"PRIuMAX"/%"PRIuMAX") ",
+		printf("%s(%"PRIuMAX"/%"PRIuMAX") ", s->s.prompt_color,
 			      (uintmax_t)hunk_index + 1,
 			      (uintmax_t)(file_diff->hunk_nr
 						? file_diff->hunk_nr
 						: 1));
-		color_fprintf(stdout, s->s.prompt_color,
-			      _(s->mode->prompt_mode[prompt_mode_type]),
-			      s->buf.buf);
+		printf(_(s->mode->prompt_mode[prompt_mode_type]),
+		       s->buf.buf);
+		if (*s->s.reset_color)
+			fputs(s->s.reset_color, stdout);
 		fflush(stdout);
 		if (read_single_character(s) == EOF)
 			break;

gitgitgadget

From: Johannes Schindelin <[email protected]>

The Perl version of that command sneakily uses git config --get-color to figure out the ANSI sequence to reset the color, but passes the empty string and therefore cannot actually match any config entry.

This was missed when re-implementing the command as a built-in command. Let's fix this, preventing the reset sequence from being overridden via the config.

---
 add-interactive.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 0f24992ca4..f3a1d7456e 100644
@@ -44,7 +44,6 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 	init_color(r, s, "help", s->help_color, GIT_COLOR_BOLD_RED);
 	init_color(r, s, "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE);
 	init_color(r, s, "error", s->error_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "reset", s->reset_color, GIT_COLOR_RESET);
 	init_color(r, s, "fraginfo", s->fraginfo_color,
 		   diff_get_color(s->use_color, DIFF_FRAGINFO));
 	init_color(r, s, "context", s->context_color,

@@ -54,6 +53,9 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 	init_color(r, s, "new", s->file_new_color,
 		diff_get_color(s->use_color, DIFF_FILE_NEW));
 
+	strlcpy(s->reset_color,
+		s->use_color ? GIT_COLOR_RESET : "", COLOR_MAXLEN);
+
 	FREE_AND_NULL(s->interactive_diff_filter);
 	git_config_get_string("interactive.difffilter",
 			      &s->interactive_diff_filter);

gitgitgadget

Your reasoning here confuses me. If we are using dash, then surely BASH_XTRACEFD does not matter either way? Perhaps the subshell is important if we are running bash, though.

Either way, being forgiving to the use of "-x" is a nice convenience and I approve.

index 28549a41a2..cca866c8f4 100755
@@ -594,6 +594,7 @@ test_expect_success 'colors can be overridden' '
 	test_config color.interactive.help green &&
 	test_config color.interactive.prompt yellow &&
 	test_config color.interactive.error blue &&
+	test_config color.diff.meta italic &&
 	test_config color.diff.frag magenta &&
 	test_config color.diff.context cyan &&
 	test_config color.diff.old bold &&

@@ -625,9 +626,9 @@ test_expect_success 'colors can be overridden' '
 	  1:        +3/-0        +1/-1 <YELLOW>c<RESET>olor-test
 	<YELLOW>Patch update<RESET>>> <RED>           staged     unstaged path<RESET>
 	* 1:        +3/-0        +1/-1 <YELLOW>c<RESET>olor-test
-	<YELLOW>Patch update<RESET>>> <BOLD>diff --git a/color-test b/color-test<RESET>
-	<BOLD>--- a/color-test<RESET>
-	<BOLD>+++ b/color-test<RESET>
+	<YELLOW>Patch update<RESET>>> <ITALIC>diff --git a/color-test b/color-test<RESET>
+	<ITALIC>--- a/color-test<RESET>
+	<ITALIC>+++ b/color-test<RESET>
 	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
 	<CYAN> context<RESET>
 	<BOLD>-old<RESET>

OK, I see now why you wanted to roll my work in. There was quite a bit more here than I expected. :)

They all look good to me. I left some minor comments on the final patch.

-Peff

This patch series started out as a tiny fix for a bug reported by Philippe Blain in
https://lore.kernel.org/git/[email protected]/.
And then I only wanted to add a regression test to make sure that this does not regress. And then I just wanted to ensure that it passes both with the Perl version of git add -i as well as with the built-in version.

And in no time I was looking at a real patch series.

Changes since v1:

* The regression test now actually exercises the re-coloring (that is the primary purpose of git add -p looking at the color.diff.* variables).
* The way the built-in git add -p renders hunk headers of split hunks was aligned with how the Perl version does things.
* We now consistently prefer color.diff.context over color.diff.plain, no matter whether using the Perl or the built-in version of git add -p.
* The commit message for the regression test no longer confuses readers by mentioning dash.
* The regression test was structured a bit better, first testing error message coloring, then the menu in git add -i and then the diff coloring in git add -p.

Johannes Schindelin (11):
add -i (built-in): do show an error message for incorrect inputs add -i (built-in): send error messages to stderr
add -p (built-in): imitate xdl_format_hunk_hdr() generating hunk
headers
add -i: use reset_color consistently
add -i (built-in): prevent the reset "color" from being configured add -i (built-in): use correct names to load color.diff.* config add -p (built-in): do not color the progress indicator separately add -i (built-in): use the same indentation as the Perl version add -i (Perl version): include indentation in the colored header add -p: prefer color.diff.context over color.diff.plain add -i: verify in the tests that colors can be overridden

 add-interactive.c          | 38 ++++++++++-------
 add-patch.c                | 25 +++++++-----
 git-add--interactive.perl  | 12 +++---
 t/t3701-add-interactive.sh | 84 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 127 insertions(+), 32 deletions(-)

base-commit: e4d83eee9239207622e2b1cc43967da5051c189c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-785%2Fdscho%2Ffix-add-i-colors-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-785/dscho/fix-add-i-colors-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/785

Range-diff vs v1:

  1:  6152122c04 =  1:  6152122c04 add -i (built-in): do show an error message for incorrect inputs
  2:  068813912b =  2:  068813912b add -i (built-in): send error messages to stderr
  -:  ---------- >  3:  98deb538d9 add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headers
  3:  9a1ba71977 =  4:  c857c44932 add -i: use `reset_color` consistently
  4:  b48c878fad =  5:  337b45cad8 add -i (built-in): prevent the `reset` "color" from being configured
  5:  85b0d27d76 =  6:  dcd2ffc458 add -i (built-in): use correct names to load color.diff.* config
  6:  059344bfaf =  7:  73b6d60a80 add -p (built-in): do not color the progress indicator separately
  7:  8df87e6395 =  8:  91ded2fbbe add -i (built-in): use the same indentation as the Perl version
  8:  42113e20dd =  9:  304614751e add -i (Perl version): include indentation in the colored header
  -:  ---------- > 10:  48d8e0badf add -p: prefer color.diff.context over color.diff.plain
  9:  38355ec98f ! 11:  c94abff72f add -i: verify in the tests that colors can be overridden
     @@ Commit message
      
          Note that we only `grep` for the colored error message instead of
          verifying that the entire `stderr` consists of just this one line: when
     -    running the test script via `dash`, using the `-x` option to trace the
     +    running the test script using the `-x` option to trace the
          commands, the sub-shell in `force_color` causes those commands to be
     -    traced into `err.raw` because we set, but do not export
     -    `BASH_XTRACEFD`).
     +    traced into `err.raw` (unless running in Bash where we set the
     +    `BASH_XTRACEFD` variable to avoid that).
      
     +    Also note that the color reset in the `<BLUE>+<RESET><BLUE>new<RESET>`
     +    line might look funny and unnecessary, as the corresponding `old` line
     +    does not reset the color after the diff marker only to turn the color
     +    back on right away.
     +
     +    However, this is a (necessary) side effect of the white-space check: in
     +    `emit_line_ws_markup()`, we first emit the diff marker via
     +    `emit_line_0()` and then the rest of the line via `ws_check_emit()`. To
     +    leave them somewhat decoupled, the color has to be reset after the diff
     +    marker to allow for the rest of the line to start with another color (or
     +    inverted, in case of white-space issues).
     +
     +    Finally, we have to simulate hunk editing: the `git add -p` command
     +    cannot rely on the internal diff machinery for coloring after letting
     +    the user edit a hunk; It has to "re-color" the edited hunk. This is the
     +    primary reason why that command is interested in the exact values of the
     +    `color.diff.*` settings in the first place. To test this re-coloring, we
     +    therefore have to pretend to edit a hunk and then show that hunk in the
     +    regression test.
     +
     +    Co-authored-by: Jeff King <[email protected]>
          Signed-off-by: Johannes Schindelin <[email protected]>
      
       ## t/t3701-add-interactive.sh ##
     @@ t/t3701-add-interactive.sh: test_expect_success 'diffs can be colorized' '
       '
       
      +test_expect_success 'colors can be overridden' '
     -+	test_config color.interactive.header red &&
     -+	test_config color.interactive.help green &&
     -+	test_config color.interactive.prompt yellow &&
     -+	test_config color.interactive.error blue &&
     -+	test_config color.diff.frag magenta &&
     -+	test_config color.diff.context cyan &&
     -+	test_config color.diff.old bold &&
     -+	test_config color.diff.new "bold red" &&
     -+
      +	git reset --hard &&
      +	test_when_finished "git rm -f color-test" &&
      +	test_write_lines context old more-context >color-test &&
      +	git add color-test &&
     -+	test_write_lines context new more-context >color-test &&
     ++	test_write_lines context new more-context another-one >color-test &&
      +
      +	echo trigger an error message >input &&
     -+	force_color git add -i 2>err.raw <input &&
     ++	force_color git \
     ++		-c color.interactive.error=blue \
     ++		add -i 2>err.raw <input &&
      +	test_decode_color <err.raw >err &&
      +	grep "<BLUE>Huh (trigger)?<RESET>" err &&
      +
     -+	test_write_lines patch color-test "" y quit >input &&
     -+	force_color git add -i >actual.raw <input &&
     -+	test_decode_color <actual.raw >actual.decoded &&
     -+	sed "/index [0-9a-f]*\\.\\.[0-9a-f]* 100644/d" <actual.decoded >actual &&
     ++	test_write_lines help quit >input &&
     ++	force_color git \
     ++		-c color.interactive.header=red \
     ++		-c color.interactive.help=green \
     ++		-c color.interactive.prompt=yellow \
     ++		add -i >actual.raw <input &&
     ++	test_decode_color <actual.raw >actual &&
      +	cat >expect <<-\EOF &&
      +	<RED>           staged     unstaged path<RESET>
     -+	  1:        +3/-0        +1/-1 color-test
     ++	  1:        +3/-0        +2/-1 color-test
      +
      +	<RED>*** Commands ***<RESET>
      +	  1: <YELLOW>s<RESET>tatus	  2: <YELLOW>u<RESET>pdate	  3: <YELLOW>r<RESET>evert	  4: <YELLOW>a<RESET>dd untracked
      +	  5: <YELLOW>p<RESET>atch	  6: <YELLOW>d<RESET>iff	  7: <YELLOW>q<RESET>uit	  8: <YELLOW>h<RESET>elp
     -+	<YELLOW>What now<RESET>> <RED>           staged     unstaged path<RESET>
     -+	  1:        +3/-0        +1/-1 <YELLOW>c<RESET>olor-test
     -+	<YELLOW>Patch update<RESET>>> <RED>           staged     unstaged path<RESET>
     -+	* 1:        +3/-0        +1/-1 <YELLOW>c<RESET>olor-test
     -+	<YELLOW>Patch update<RESET>>> <BOLD>diff --git a/color-test b/color-test<RESET>
     -+	<BOLD>--- a/color-test<RESET>
     -+	<BOLD>+++ b/color-test<RESET>
     -+	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
     -+	<CYAN> context<RESET>
     -+	<BOLD>-old<RESET>
     -+	<BOLD;RED>+<RESET><BOLD;RED>new<RESET>
     -+	<CYAN> more-context<RESET>
     -+	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,e,?]? <RESET>
     ++	<YELLOW>What now<RESET>> <GREEN>status        - show paths with changes<RESET>
     ++	<GREEN>update        - add working tree state to the staged set of changes<RESET>
     ++	<GREEN>revert        - revert staged set of changes back to the HEAD version<RESET>
     ++	<GREEN>patch         - pick hunks and update selectively<RESET>
     ++	<GREEN>diff          - view diff between HEAD and index<RESET>
     ++	<GREEN>add untracked - add contents of untracked files to the staged set of changes<RESET>
      +	<RED>*** Commands ***<RESET>
      +	  1: <YELLOW>s<RESET>tatus	  2: <YELLOW>u<RESET>pdate	  3: <YELLOW>r<RESET>evert	  4: <YELLOW>a<RESET>dd untracked
      +	  5: <YELLOW>p<RESET>atch	  6: <YELLOW>d<RESET>iff	  7: <YELLOW>q<RESET>uit	  8: <YELLOW>h<RESET>elp
      +	<YELLOW>What now<RESET>> Bye.
      +	EOF
     ++	test_cmp expect actual &&
     ++
     ++	: exercise recolor_hunk by editing and then look at the hunk again &&
     ++	test_write_lines s e K q >input &&
     ++	force_color git \
     ++		-c color.interactive.prompt=yellow \
     ++		-c color.diff.meta=italic \
     ++		-c color.diff.frag=magenta \
     ++		-c color.diff.context=cyan \
     ++		-c color.diff.old=bold \
     ++		-c color.diff.new=blue \
     ++		-c core.editor=touch \
     ++		add -p >actual.raw <input &&
     ++	test_decode_color <actual.raw >actual.decoded &&
     ++	sed "s/index [0-9a-f]*\\.\\.[0-9a-f]* 100644/<INDEX-LINE>/" <actual.decoded >actual &&
     ++	cat >expect <<-\EOF &&
     ++	<ITALIC>diff --git a/color-test b/color-test<RESET>
     ++	<ITALIC><INDEX-LINE><RESET>
     ++	<ITALIC>--- a/color-test<RESET>
     ++	<ITALIC>+++ b/color-test<RESET>
     ++	<MAGENTA>@@ -1,3 +1,4 @@<RESET>
     ++	<CYAN> context<RESET>
     ++	<BOLD>-old<RESET>
     ++	<BLUE>+<RESET><BLUE>new<RESET>
     ++	<CYAN> more-context<RESET>
     ++	<BLUE>+<RESET><BLUE>another-one<RESET>
     ++	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,s,e,?]? <RESET><BOLD>Split into 2 hunks.<RESET>
     ++	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
     ++	<CYAN> context<RESET>
     ++	<BOLD>-old<RESET>
     ++	<BLUE>+<RESET><BLUE>new<RESET>
     ++	<CYAN> more-context<RESET>
     ++	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
     ++	<CYAN> more-context<RESET>
     ++	<BLUE>+<RESET><BLUE>another-one<RESET>
     ++	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
     ++	<CYAN> context<RESET>
     ++	<BOLD>-old<RESET>
     ++	<BLUE>+new<RESET>
     ++	<CYAN> more-context<RESET>
     ++	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? <RESET>
     ++	EOF
      +	test_cmp expect actual
      +'
      +

gitgitgadget

From: Johannes Schindelin <[email protected]>

There is a neat feature in git add -i where it allows users to select items via unique prefixes.

In the built-in version of git add -i, we specifically sort the items (unless they are already sorted) and then perform a binary search to figure out whether the input constitutes a unique prefix. Unfortunately, by mistake this code misidentifies matches even if the input string is not actually a prefix of any item.

For example, in the initial menu, where there is a status and an update command, the input tadaa was mistaken as a prefix of update.

Let's fix this by looking a bit closer whether the input is actually a prefix of the item at the found insert index.

---
 add-interactive.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 555c4abf32..8ca503d803 100644
@@ -194,7 +194,8 @@ static ssize_t find_unique(const char *string, struct prefix_item_list *list)
 	else if (index + 1 < list->sorted.nr &&
 		 starts_with(list->sorted.items[index + 1].string, string))
 		return -1;
-	else if (index < list->sorted.nr)
+	else if (index < list->sorted.nr &&
+		 starts_with(list->sorted.items[index].string, string))
 		item = list->sorted.items[index].util;
 	else
 		return -1;

gitgitgadget

From: Johannes Schindelin <[email protected]>

The Perl version of this command colors the progress indicator and the prompt message in one go, let's do the same in the built-in version.

---
 add-patch.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index bf89c43145..2fad92ca37 100644
@@ -1461,15 +1461,15 @@ static int patch_update_file(struct add_p_state *s,
 		else
 			prompt_mode_type = PROMPT_HUNK;
 
-		color_fprintf(stdout, s->s.prompt_color,
-			      "(%"PRIuMAX"/%"PRIuMAX") ",
+		printf("%s(%"PRIuMAX"/%"PRIuMAX") ", s->s.prompt_color,
 			      (uintmax_t)hunk_index + 1,
 			      (uintmax_t)(file_diff->hunk_nr
 						? file_diff->hunk_nr
 						: 1));
-		color_fprintf(stdout, s->s.prompt_color,
-			      _(s->mode->prompt_mode[prompt_mode_type]),
-			      s->buf.buf);
+		printf(_(s->mode->prompt_mode[prompt_mode_type]),
+		       s->buf.buf);
+		if (*s->s.reset_color)
+			fputs(s->s.reset_color, stdout);
 		fflush(stdout);
 		if (read_single_character(s) == EOF)
 			break;

gitgitgadget

From: Johannes Schindelin <[email protected]>

When copying the spaces used to indent non-flat lists in git add -i, one space was appended by mistake. This makes the output of the built-in version of git add -i inconsistent with the Perl version. Let's adjust the built-in version to produce the same output as the Perl version.

---
 add-interactive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 9126684348..c298a8b80f 100644
@@ -1137,7 +1137,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps)
 	print_file_item_data.color = data.color;
 	print_file_item_data.reset = data.reset;
 
-	strbuf_addstr(&header, "      ");
+	strbuf_addstr(&header, "     ");
 	strbuf_addf(&header, print_file_item_data.modified_fmt,
 		    _("staged"), _("unstaged"), _("path"));
 	opts.list_opts.header = header.buf;

gitgitgadget

From: Johannes Schindelin <[email protected]>

The header is formatted by padding each column heading with spaces up to the length of 12 characters. These padding spaces are naturally included when coloring the entire header.

However, the preceding five spaces indentation for non-flat lists were _not_ included in the Perl version, but _were_ included in the built-in version. Let's adjust the former to align with the latter's behavior.

---
 git-add--interactive.perl | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index e713fe3d02..adbac2bc6d 100755
@@ -483,10 +483,8 @@ sub list_and_choose {
 		my $last_lf = 0;
 
 		if ($opts->{HEADER}) {
-			if (!$opts->{LIST_FLAT}) {
-				print "     ";
-			}
-			print colored $header_color, "$opts->{HEADER}\n";
+			my $indent = $opts->{LIST_FLAT} ? "" : "     ";
+			print colored $header_color, "$indent$opts->{HEADER}\n";
 		}
 		for ($i = 0; $i < @stuff; $i++) {
 			my $chosen = $chosen[$i] ? '*' : ' ';

gitgitgadget

From: Johannes Schindelin <[email protected]>

Git's diff machinery allows users to override the colors to use in diffs, even the plain-colored context lines. As of 8dbf3eb6850 (diff.h:
rename DIFF_PLAIN color slot to DIFF_CONTEXT, 2015-05-27), the preferred name of the config setting is color.diff.context, although Git still allows color.diff.plain.

In the context of git add -p, this logic is a bit hard to replicate:
git_diff_basic_config() reads all config values sequentially and if it sees _any_ color.diff.context or color.diff.plain, it accepts the new color. The Perl version of git add -p needs to go through git config --get-color, though, which allows only one key to be specified. The same goes for the built-in version of git add -p, which has to go through repo_config_get_value().

The best we can do here is to look for .context and if none is found, fall back to looking for .plain, and if still not found, fall back to the hard-coded default (which in this case is simply the empty string, as context lines are typically rendered without colored).

This still needs to inconsistencies when both config names are used: the initial diff will be colored by the diff machinery. Once edited by a user, a hunk has to be re-colored by git add -p, though, which would then use the other setting to color the context lines.

In practice, this is not _all_ that bad. The git config manual says this in the color.diff.<slot>:

context (context text - plain is a historical synonym)

We should therefore assume that users use either one or the other, but not both names. Besides, it is relatively uncommon to look at a hunk after editing it because it is immediately staged by default.

---
 add-interactive.c         | 6 ++++--
 git-add--interactive.perl | 6 +++---
 2 files changed, 7 insertions(+), 5 deletions(-)

index c298a8b80f..54dfdc56f5 100644
@@ -49,8 +49,10 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 
 	init_color(r, s, "diff.frag", s->fraginfo_color,
 		   diff_get_color(s->use_color, DIFF_FRAGINFO));
-	init_color(r, s, "diff.context", s->context_color,
-		diff_get_color(s->use_color, DIFF_CONTEXT));
+	init_color(r, s, "diff.context", s->context_color, "fall back");
+	if (!strcmp(s->context_color, "fall back"))
+		init_color(r, s, "diff.plain", s->context_color,
+			   diff_get_color(s->use_color, DIFF_CONTEXT));
 	init_color(r, s, "diff.old", s->file_old_color,
 		diff_get_color(s->use_color, DIFF_FILE_OLD));
 	init_color(r, s, "diff.new", s->file_new_color,

index adbac2bc6d..bc3a1e8eff 100755
@@ -30,9 +30,9 @@
 	$diff_use_color ? (
 		$repo->get_color('color.diff.frag', 'cyan'),
 	) : ();
-my ($diff_plain_color) =
+my ($diff_context_color) =
 	$diff_use_color ? (
-		$repo->get_color('color.diff.plain', ''),
+		$repo->get_color($repo->config('color.diff.context') ? 'color.diff.context' : 'color.diff.plain', ''),
 	) : ();
 my ($diff_old_color) =
 	$diff_use_color ? (

@@ -1046,7 +1046,7 @@ sub color_diff {
 		colored((/^@/  ? $fraginfo_color :
 			 /^\+/ ? $diff_new_color :
 			 /^-/  ? $diff_old_color :
-			 $diff_plain_color),
+			 $diff_context_color),
 			$_);
 	} @_;
 }

gitgitgadget

From: Johannes Schindelin <[email protected]>

The builtin version of add-interactive mistakenly loads diff colors from color.interactive.* instead of color.diff.*. It also accidentally spells frag as fraginfo.

Let's fix that.

Note also that we don't respect the historical diff.color.*. The perl version never did, and those have been deprecated since 2007.

---
 add-interactive.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

index f3a1d7456e..9126684348 100644
@@ -12,10 +12,10 @@
 #include "prompt.h"
 
 static void init_color(struct repository *r, struct add_i_state *s,
-		       const char *slot_name, char *dst,
+		       const char *section_and_slot, char *dst,
 		       const char *default_color)
 {
-	char *key = xstrfmt("color.interactive.%s", slot_name);
+	char *key = xstrfmt("color.%s", section_and_slot);
 	const char *value;
 
 	if (!s->use_color)

@@ -40,17 +40,20 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 			git_config_colorbool("color.interactive", value);
 	s->use_color = want_color(s->use_color);
 
-	init_color(r, s, "header", s->header_color, GIT_COLOR_BOLD);
-	init_color(r, s, "help", s->help_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE);
-	init_color(r, s, "error", s->error_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "fraginfo", s->fraginfo_color,
+	init_color(r, s, "interactive.header", s->header_color, GIT_COLOR_BOLD);
+	init_color(r, s, "interactive.help", s->help_color, GIT_COLOR_BOLD_RED);
+	init_color(r, s, "interactive.prompt", s->prompt_color,
+		   GIT_COLOR_BOLD_BLUE);
+	init_color(r, s, "interactive.error", s->error_color,
+		   GIT_COLOR_BOLD_RED);
+
+	init_color(r, s, "diff.frag", s->fraginfo_color,
 		   diff_get_color(s->use_color, DIFF_FRAGINFO));
-	init_color(r, s, "context", s->context_color,
+	init_color(r, s, "diff.context", s->context_color,
 		diff_get_color(s->use_color, DIFF_CONTEXT));
-	init_color(r, s, "old", s->file_old_color,
+	init_color(r, s, "diff.old", s->file_old_color,
 		diff_get_color(s->use_color, DIFF_FILE_OLD));
-	init_color(r, s, "new", s->file_new_color,
+	init_color(r, s, "diff.new", s->file_new_color,
 		diff_get_color(s->use_color, DIFF_FILE_NEW));
 
 	strlcpy(s->reset_color,

gitgitgadget

From: Johannes Schindelin <[email protected]>

The Perl version of that command sneakily uses git config --get-color to figure out the ANSI sequence to reset the color, but passes the empty string and therefore cannot actually match any config entry.

This was missed when re-implementing the command as a built-in command. Let's fix this, preventing the reset sequence from being overridden via the config.

---
 add-interactive.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 0f24992ca4..f3a1d7456e 100644
@@ -44,7 +44,6 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 	init_color(r, s, "help", s->help_color, GIT_COLOR_BOLD_RED);
 	init_color(r, s, "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE);
 	init_color(r, s, "error", s->error_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "reset", s->reset_color, GIT_COLOR_RESET);
 	init_color(r, s, "fraginfo", s->fraginfo_color,
 		   diff_get_color(s->use_color, DIFF_FRAGINFO));
 	init_color(r, s, "context", s->context_color,

@@ -54,6 +53,9 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 	init_color(r, s, "new", s->file_new_color,
 		diff_get_color(s->use_color, DIFF_FILE_NEW));
 
+	strlcpy(s->reset_color,
+		s->use_color ? GIT_COLOR_RESET : "", COLOR_MAXLEN);
+
 	FREE_AND_NULL(s->interactive_diff_filter);
 	git_config_get_string("interactive.difffilter",
 			      &s->interactive_diff_filter);

gitgitgadget

From: Johannes Schindelin <[email protected]>

Now that the Perl version produces the same output as the built-in version (mostly fixing bugs in the latter), let's add a regression test to verify that it stays this way.

Note that we only grep for the colored error message instead of verifying that the entire stderr consists of just this one line: when running the test script using the -x option to trace the commands, the sub-shell in force_color causes those commands to be traced into err.raw (unless running in Bash where we set the BASH_XTRACEFD variable to avoid that).

Also note that the color reset in the <BLUE>+<RESET><BLUE>new<RESET> line might look funny and unnecessary, as the corresponding old line does not reset the color after the diff marker only to turn the color back on right away.

However, this is a (necessary) side effect of the white-space check: in emit_line_ws_markup(), we first emit the diff marker via emit_line_0() and then the rest of the line via ws_check_emit(). To leave them somewhat decoupled, the color has to be reset after the diff marker to allow for the rest of the line to start with another color (or inverted, in case of white-space issues).

Finally, we have to simulate hunk editing: the git add -p command cannot rely on the internal diff machinery for coloring after letting the user edit a hunk; It has to "re-color" the edited hunk. This is the primary reason why that command is interested in the exact values of the color.diff.* settings in the first place. To test this re-coloring, we therefore have to pretend to edit a hunk and then show that hunk in the regression test.

---
 t/t3701-add-interactive.sh | 84 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

index ca04fac417..cc3f434a97 100755
@@ -589,6 +589,90 @@ test_expect_success 'diffs can be colorized' '
 	grep "$(printf "\\033")" output
 '
 
+test_expect_success 'colors can be overridden' '
+	git reset --hard &&
+	test_when_finished "git rm -f color-test" &&
+	test_write_lines context old more-context >color-test &&
+	git add color-test &&
+	test_write_lines context new more-context another-one >color-test &&
+
+	echo trigger an error message >input &&
+	force_color git \
+		-c color.interactive.error=blue \
+		add -i 2>err.raw <input &&
+	test_decode_color <err.raw >err &&
+	grep "<BLUE>Huh (trigger)?<RESET>" err &&
+
+	test_write_lines help quit >input &&
+	force_color git \
+		-c color.interactive.header=red \
+		-c color.interactive.help=green \
+		-c color.interactive.prompt=yellow \
+		add -i >actual.raw <input &&
+	test_decode_color <actual.raw >actual &&
+	cat >expect <<-\EOF &&
+	<RED>           staged     unstaged path<RESET>
+	  1:        +3/-0        +2/-1 color-test
+
+	<RED>*** Commands ***<RESET>
+	  1: <YELLOW>s<RESET>tatus	  2: <YELLOW>u<RESET>pdate	  3: <YELLOW>r<RESET>evert	  4: <YELLOW>a<RESET>dd untracked
+	  5: <YELLOW>p<RESET>atch	  6: <YELLOW>d<RESET>iff	  7: <YELLOW>q<RESET>uit	  8: <YELLOW>h<RESET>elp
+	<YELLOW>What now<RESET>> <GREEN>status        - show paths with changes<RESET>
+	<GREEN>update        - add working tree state to the staged set of changes<RESET>
+	<GREEN>revert        - revert staged set of changes back to the HEAD version<RESET>
+	<GREEN>patch         - pick hunks and update selectively<RESET>
+	<GREEN>diff          - view diff between HEAD and index<RESET>
+	<GREEN>add untracked - add contents of untracked files to the staged set of changes<RESET>
+	<RED>*** Commands ***<RESET>
+	  1: <YELLOW>s<RESET>tatus	  2: <YELLOW>u<RESET>pdate	  3: <YELLOW>r<RESET>evert	  4: <YELLOW>a<RESET>dd untracked
+	  5: <YELLOW>p<RESET>atch	  6: <YELLOW>d<RESET>iff	  7: <YELLOW>q<RESET>uit	  8: <YELLOW>h<RESET>elp
+	<YELLOW>What now<RESET>> Bye.
+	EOF
+	test_cmp expect actual &&
+
+	: exercise recolor_hunk by editing and then look at the hunk again &&
+	test_write_lines s e K q >input &&
+	force_color git \
+		-c color.interactive.prompt=yellow \
+		-c color.diff.meta=italic \
+		-c color.diff.frag=magenta \
+		-c color.diff.context=cyan \
+		-c color.diff.old=bold \
+		-c color.diff.new=blue \
+		-c core.editor=touch \
+		add -p >actual.raw <input &&
+	test_decode_color <actual.raw >actual.decoded &&
+	sed "s/index [0-9a-f]*\\.\\.[0-9a-f]* 100644/<INDEX-LINE>/" <actual.decoded >actual &&
+	cat >expect <<-\EOF &&
+	<ITALIC>diff --git a/color-test b/color-test<RESET>
+	<ITALIC><INDEX-LINE><RESET>
+	<ITALIC>--- a/color-test<RESET>
+	<ITALIC>+++ b/color-test<RESET>
+	<MAGENTA>@@ -1,3 +1,4 @@<RESET>
+	<CYAN> context<RESET>
+	<BOLD>-old<RESET>
+	<BLUE>+<RESET><BLUE>new<RESET>
+	<CYAN> more-context<RESET>
+	<BLUE>+<RESET><BLUE>another-one<RESET>
+	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,s,e,?]? <RESET><BOLD>Split into 2 hunks.<RESET>
+	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
+	<CYAN> context<RESET>
+	<BOLD>-old<RESET>
+	<BLUE>+<RESET><BLUE>new<RESET>
+	<CYAN> more-context<RESET>
+	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
+	<CYAN> more-context<RESET>
+	<BLUE>+<RESET><BLUE>another-one<RESET>
+	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
+	<CYAN> context<RESET>
+	<BOLD>-old<RESET>
+	<BLUE>+new<RESET>
+	<CYAN> more-context<RESET>
+	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? <RESET>
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '
 	git reset --hard &&
 

gitgitgadget

From: Johannes Schindelin <[email protected]>

The Perl version of that command already does that since a301973641f (add -p: print errors in separate color, 2009-02-05). The built-in version's development started by reimplementing the initial version from 5cde71d64af (git-add --interactive, 2006-12-10) for simplicity, though, which still printed error messages to stdout.

Let's fix that by imitating the Perl version's behavior in the built-in version of that command.

---
 add-interactive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 8ca503d803..0f24992ca4 100644
@@ -365,7 +365,7 @@ static ssize_t list_and_choose(struct add_i_state *s,
 
 			if (from < 0 || from >= items->items.nr ||
 			    (singleton && from + 1 != to)) {
-				color_fprintf_ln(stdout, s->error_color,
+				color_fprintf_ln(stderr, s->error_color,
 						 _("Huh (%s)?"), p);
 				break;
 			} else if (singleton) {

gitgitgadget

From: Johannes Schindelin <[email protected]>

In libxdiff, imitating GNU diff, the hunk headers only show the line count if it is different from 1. When splitting hunks, the Perl version of git add -p already imitates this. Let's do the same in the built-in version of said command.

---
 add-patch.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

index be4cf6e9e5..b6d53229bb 100644
@@ -661,9 +661,14 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
 		else
 			new_offset += delta;
 
-		strbuf_addf(out, "@@ -%lu,%lu +%lu,%lu @@",
-			    old_offset, header->old_count,
-			    new_offset, header->new_count);
+		strbuf_addf(out, "@@ -%lu", old_offset);
+		if (header->old_count != 1)
+			strbuf_addf(out, ",%lu", header->old_count);
+		strbuf_addf(out, " +%lu", new_offset);
+		if (header->new_count != 1)
+			strbuf_addf(out, ",%lu", header->new_count);
+		strbuf_addstr(out, " @@");
+
 		if (len)
 			strbuf_add(out, p, len);
 		else if (colored)

gitgitgadget

From: Johannes Schindelin <[email protected]>

We already maintain a list of colors in the add_i_state, therefore we should use them.

---
 add-patch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index b6d53229bb..bf89c43145 100644
@@ -672,7 +672,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
 		if (len)
 			strbuf_add(out, p, len);
 		else if (colored)
-			strbuf_addf(out, "%s\n", GIT_COLOR_RESET);
+			strbuf_addf(out, "%s\n", s->s.reset_color);
 		else
 			strbuf_addch(out, '\n');
 	}

@@ -1065,7 +1065,7 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)
 			      s->s.file_new_color :
 			      s->s.context_color);
 		strbuf_add(&s->colored, plain + current, eol - current);
-		strbuf_addstr(&s->colored, GIT_COLOR_RESET);
+		strbuf_addstr(&s->colored, s->s.reset_color);
 		if (next > eol)
 			strbuf_add(&s->colored, plain + eol, next - eol);
 		current = next;

gitgitgadget

Hi Peff,

Sorry!

It kinda does, though. Dash _does_ respect the BASH_XTRACEFD variable, funnily enough, but does not hand the settings through to sub-shells, whereas Bash does.

The most important thing, which I of course _failed_ to describe most prominently, is that _in general_ -x will mess up the err file, i.e. when running with anything but Bash. Therefoer we need the grep instead of a test_cmp, and that's what I tried to convey in v2.

	$ git grep -w meta git-add--interactive.perl add-interactive.c \
		add-patch.c

Really? That's news to me, and doesn't seem to trigger:

[bash uses it]

  $ bash -xc 'BASH_XTRACEFD=3; echo foo' 3>trace
  + BASH_XTRACEFD=3
  foo
  $ cat trace
  + echo foo

[dash does not]

  $ dash -xc 'BASH_XTRACEFD=3; echo foo' 3>trace
  + BASH_XTRACEFD=3
  + echo foo
  foo
  $ cat trace

Yeah. I understood that part (because it has bit me so many times ;) ), but I agree it's good to make it clear.

Hmm. Right, I knew about that weirdness. But I assumed that the builtin add-interactive was doing the diffs in-core. Otherwise, why would we have seen the failure to load diff.color.frag in the first place? Philippe's simple example just did "git add -p". So now I'm doubly confused.

The answer seems to be that render_hunk() always _replaces_ the colors we got from running the external diff. Whereas the perl version only applied coloring when reading back in the results of an edit operation (and likewise used the frag color when generating a split hunk header).

I'm not sure that what the builtin version is doing is wrong, but it seems like it's putting a lot of extra effort into parsing colors off of the colorized version. Whereas the perl version just assumes the lines match up. I do wonder if there are corner cases we might hit around filters here, though. The lines we get from a filter might bear no resemblance at all to diff lines. The only thing we require in the perl version is that they have correspond 1-to-1 with the unfiltered diff lines in meaning.

Heh. Sorry and thanks, I guess? :) I'll try to read over v2 carefully.

-Peff

The changes were less scary than I was led to believe after reading your earlier response. :)

Everything here looks sensible. As I said elsewhere, I do worry there may be a lingering issue with how parse_diff() looks at the filtered diffs. Let me see if I can get diff-so-fancy working...

Aha, yes. I think using diff-so-fancy here isn't entirely robust, because it has some cases where it fails at the 1-to-1 line correspondence, but they're aware of the issue. But it does work in simples cases now (there's some coloring which makes the output more meaningful, but I obviously won't paste it here):

  $ git -c interactive.difffilter='diff-so-fancy' add -p
  ──────────────────────────────────────────────────────────────────────
  modified: file
  ──────────────────────────────────────────────────────────────────────
  @ file:1 @
  old
  new
  (1/1) Stage this hunk [y,n,q,a,d,e,?]? 

But with the builtin:

  $ git -c interactive.difffilter='diff-so-fancy' \
        -c add.interactive.usebuiltin=true \
	add -p
  error: could not parse colored hunk header '?[1m?[38;5;1m?[31mold?[m'

I don't use it myself, and as I said, I think they have some fixes to make for it to be robust as a diff-filter. But I suspect this may be a problem once the builtin version becomes the default.

I don't think it should be dealt with in this patch series, though. While it may touch on some of the coloring code, it's otherwise orthogonal to the fixes here. And while the fix is likely to be to make render_hunk() stop re-coloring in the non-edit cases, your more-robust test here will still be checking what you expect (because it really is triggering the edit case, not relying on the extra coloring to see the effect of in-process color config).

-Peff

Hi Peff,

Gaaah! I totally forgot that BASH_XTRACEFD is only heeded by BusyBox' ash (and only when built with ENABLE_ASH_BASH_COMPAT), not by dash.

Sorry for the noise.

Oh, that's easy to explain: as you can verify reading
https://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898
the Perl version of git add -p insists on (re-)constructing the hunk headers manually, and obviously it needs to color them manually, too. And
https://github.com/git/git/blob/e31aba42fb12/add-patch.c#L649-L672 shows that the built-in version of git add -p slavishly follows that practice.

Right. I should have been more precise in what parts are used of the diff that is colored via the internal diff machinery. The hunk headers are not used. The hunks themselves are, unless edited. The file header is, too:
https://github.com/git/git/blob/e31aba42fb12/add-patch.c#L683-L714

No, the Perl version also insists on applying fraginfo_color, see
https://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898

They do have to correspond 1-to-1 because the assumption is that the individual lines will then correspond one-to-one, too. This does not need to be true, of course, but then the filter is probably less useful than the user wants it to be.

No need to be sorry on your side. _I_ am sorry that I did not add that test case during my re-implementation efforts in the first place.

Thanks,
DScho

Hi Peff,

It might _seem_ that it works. But try to split a hunk. I did this with the test case (interrupting it just before running add -p):

-- snip --

$ git -C ./t/trash\ directory.t3701-add-interactive/ -c
interactive.difffilter='diff-so-fancy' -c add.interactive.usebuiltin=false add -p
<BOLD>────────────────────────────────────────────────────────────────────────────────</BOLD>
<BOLD>modified: color-test</BOLD>
<BOLD>────────────────────────────────────────────────────────────────────────────────</BOLD>
<MAGENTA>@ color-test:1 @</MAGENTA>
context
<RED>old</RED>
<GREEN>new</GREEN>
more-context
<GREEN>another-one</GREEN>
<BLUE>(1/1) Stage this hunk [y,n,q,a,d,s,e,?]?</BLUE> s
<BOLD>Split into 2 hunks.</BOLD>
<MAGENTA>@@ -1,3 +1,3 @@</MAGENTA>
<RED>old</RED>
<GREEN>new</GREEN>
more-context
<GREEN>another-one</GREEN>
<BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]?</BLUE>
-- snap --

Obviously, I marked it up so you can see what parts were colored.

See how it first _looks_ as if it works? But then you split the hunk, but instead of showing only the old/new part, it shows the old/new/another-one part!

In other words, it does not work at all, and the fact that it does not even warn you about it is misleading, and that's pretty much all I will say about it.

Granted, this is not quite helpful. I _think_ what is happening is that the number of lines is different, and add -p goes like: noooope! But it could probably be better at describing what the issue is. Or it could cater to diff-so-fancy, if that is what people use these days, and special-case it by falling back to detecting the hunk boundaries in a manner that is compatible with diff-so-fancy.

Or we might be able to come up with a strategy that is not so limited and that works for more than just diff-so-fancy.

Oh, _that_ I wholeheartedly agree with.

I don't actually think that we _can_ stop re-coloring the hunk header in the general case because we need to keep this working even for split hunks. It would be a very bad idea to make it work for non-split cases and then something like die() only when the hunk is split. Better re-color all of them, then, to not even risk that situation.

Ciao,
Dscho

Ah, that makes more sense. And I learned something new about busybox. :)

But that is only when we split hunks (your link to the perl script is in split_hunks()). I agree we must color manually there when creating our own hunk header. But outside of that and patch-editing, the perl script does not otherwise recolor or rewrite (nor even really parse beyond line-splitting) what it gets from the colorized version.

Whereas add-patch parses the colors off of the version and then re-colors every hunk header. Which seems doubly weird to me. Even if we were going to re-color every hunk (e.g., because we don't want to store the original hunk line, but instead a parsed version of it), why bother parsing the color version at all, and not just the machine-readable version?

Only when we split. Try this to give different colors between the interactive script and diff:

index e713fe3d02..862a21ff1f 100755
@@ -28,8 +28,9 @@
 my $diff_use_color = $repo->get_colorbool('color.diff');
 my ($fraginfo_color) =
 	$diff_use_color ? (
-		$repo->get_color('color.diff.frag', 'cyan'),
+		$repo->get_color('color.diff.nonsense', 'yellow'),
 	) : ();
+# noop to create split hunk
 my ($diff_plain_color) =
 	$diff_use_color ? (
 		$repo->get_color('color.diff.plain', ''),

Running "git add -p" does not result in yellow hunk headers. But issuing a split command does.

The distinction is mostly academic, because diff-tree and the interactive patch code should be using the same colors, so the result should look the same. It could matter if the diff-filter chooses different colors, though then the split headers will not match the originals in style. We _could_ run the newly created hunk header individually through the diff-filter, though I'm not sure how various filters would handle that.

That's true of the perl version as well as the builtin one, but I think the builtin one's insistence on parsing the colored output is taking us in the wrong direction to eventually fix that.

Right, I'm not disputing the 1-to-1 thing (I was after all the one who implemented interactive.diffilter, and added the "complain if the counts don't line up" check). But in the perl script they only need to correspond _semantically_, not syntactically.

-Peff

Yes, there are definitely problems with how diff-so-fancy represents things (because they don't maintain a 1-to-1 correspondence). You should generally get a complaint like:

  $ git -c interactive.difffilter='diff-so-fancy' add -p
  fatal: mismatched output from interactive.diffFilter
  hint: Your filter must maintain a one-to-one correspondence
  hint: between its input and output lines.

The diff-so-fancy folks have asked me about this, and I've made clear the problem to them, and that the solution is to have a mode which maintains the line correspondence. AFAIK there's no fix yet. I don't know how many people are actually using it in practice in its current buggy state.

There's a big thread here:

https://github.com/so-fancy/diff-so-fancy/issues/35

I don't really recommend reading the whole thing. Beyond what I just said, the interesting bits are:

- people recommend using the "delta" tool; it has a "--color-only" option which does just diff-highlight-style coloring, but doesn't break line correspondence

- somebody suggested instead of using interactive.difffilter, to add an option to show each hunk in an individual pager command. So add-interactive would never see the "fancy" version at all, but it would be generated on the fly when the user sees it (and that filter would have to be able to handle seeing individual hunks without the diff header).

All of which is to say that the current state is a bit of a mess, and I don't consider it completely necessary to fix it before the builtin version becomes the default. But:

- I think we should expect some possible complaints / bug reports from fringe users of the already-somewhat-broken state

- IMHO the parsing of the filtered version done by the builtin is going in the wrong direction (see my other mail for an elaboration)

Yeah, obviously calling die() in the split case is bad. And the fact that newly-created split hunk headers are not filtered the same way as the original hunk headers isn't ideal. But it's a pretty easy fix in the perl version, and not in the builtin version.

-Peff

Hi Dscho

Playing devil's advocate for a moment - why do we have a reset entry in that list? The next patch makes sure it cannot be customized which is a good thing so why not drop the reset member from add_i_state entirely? The perl version needed to get the reset string from git config and store it somewhere but in the C version we have a perfectly good constant we can use instead.

Best Wishes

Phillip

Hi Dscho

Why? the C version has access to an api that means we don't have to remember to print the reset string each time so why move away from that? I don't think it matters to the user that there are some extra escape codes in the prompt of the C version. The answer is probably "so we can use the same test as the perl version" which might be a good reason - if it is I think it would be helpful to say so in the commit message.

Best Wishes

Phillip

Hi Dscho

I had trouble understanding this. I think my confusion is that the padding was printed when the header was colored, but it was not inside the colored part whereas the subject lead be to think there was no indentation printed when the header was colored. I assume this change is so that we can use the same test for both versions?

Best Wishes

Phillip

Hi Phillip,

Right.

On the other hand, does it hurt to keep that field for consistency with the rest of the coloring stuff? It would probably cost more to make the change you suggested than it would benefit us.

Ciao,
Dscho

Hi Phillip,

Honestly, the number one reason is so that the _same_ test passes using the Perl version as well as with the built-in version, something that is required by the linux-clang job in our CI build.

I am not really willing to change this, unless I hear a goooood reason to complicate the test.

Ciao,
Dscho

Hi Phillip,

Right, this is ambiguous, but I thought it was clear from the first paragraph that "included" means "when coloring".

I'm not a native speaker, though, so I welcome suggestions to improve this.

Correct.

Thanks,
Dscho

Hi Dscho,

s/needs/leads/

Philippe.

Hi Dscho

One nit pick: the built-in version does not have to go through repo_config_get_value(), it could get the values in a callback using git_config() which would match the behavior of diff but chooses not to (presumably it is more convenient to just look up the config values). Having said that I think this patch is fine and the commit message does a good job of explaining the situation.

Thanks for working on this series, it's great to see a test at the end

Best Wishes

Phillip

Hi Dscho

It creates the illusion that reset is similar to the others but it isn't as we don't want it to be customizable.

At the moment there is one use of s->s.color_reset and two of GIT_COLOR_RESET in add-patch.c

Best Wishes

Phillip

Hi Dscho

That's what I assumed - it would be good to document that in the commit message.

I guess we could change the perl version to match the C version as it is the perl version that will be retired but I'm not that fussed

Best Wishes

Phillip

Hi Dscho

Maybe something along the lines of

add -i (Perl version): color header to match the C version

Both versions of add -i indent non-flat lists by five spaces. However when using color the C version prints these spaces after the ANSI color codes whereas the Perl version prints them before the color codes. Change the Perl version to match the C version to make testing easier.

Best Wishes

Phillip

Hi Peff,

Let's continue on this distraction for a bit before I go back to fixing the patch series, which actually tries to fix a _different_ concern.

The reason why add-patch.c "parses the colors off" is that we want to show the rest of the hunk header, in color, even after splitting hunks (this will only be shown for the first hunk, of course).

But given that git add -p is somewhat of a fringe use, and using diffFilter is _even_ more fringe, I do not really want to spend any further time on this tangent.

My thinking back then was: what if _I_ want to use a diffFilter? For what would I use it? Probably to emphasize certain hunk headers more, by adding more color to the part after the line range.

Anyway. I stand by what I said above: I do not want to spend any further time on this tangent, at least not right now. There are more pressing challenges waiting for me, and I expect those other challenge to have a much bigger "return on investment".

Ciao,
Dscho

Hi Peff,

It's a little bit of a surprise that this is the first time I hear about this.

The way _I_ would go about fixing this is to look for a tell-tale that we're looking at a diff-so-fancy style output, e.g. by looking for that horizontal line, then adding special code to handle that.

This is not a minor undertaking, and it would currently require _two_
implementations: the Perl version and the built-in version. They would look _very_ different from one another. Therefore, I would probably either wait until the Perl version is retired, or selectively only adjust the built-in version, if _I_ was to implement this.

But given that it does not work right now, and given that it has not been working for a long time, I think it does not hurt so much if it is left in the current state for a bit longer. I would love to focus on the patch series that kicked off this discussion first, getting it done, before I think about other add -p aspects.

Ciao,
Dscho

Hi Phillip,

In a certain sense, however, it *is* customizable. Namely when the color is turned _off_. Then reset_color is set to the empty string, and GIT_RESET_COLOR still isn't.

I did spend the time to look at those call sites, and they seem to be guarded by "if colored" conditions.

But as I said, I don't think that it will buy us a ton to treat resetting color different from setting color.

I mean, who is to say that we won't add some sort of low-level mode of git add -p at some stage that can be used by 3rd-party applications? We would then have to switch back to the current code. And even if not, I think that there is a benefit to the current code, where no caller has to wonder whether the reset sequence has been overridden (e.g. by the empty string) or not, it just says semantically that this is where we switch back to regular color. And that's that.

Ciao,
Dscho

Hi Phillip,

I wrapped it, and modified it slightly:

add -i (Perl version): color header to match the C version

Both versions of add -i indent non-flat lists by five spaces. However when using color the C version prints these spaces after the ANSI color codes whereas the Perl version prints them before the color codes. Change the Perl version to match the C version to allow for introducing a test that verifies that both versions produce the exact same output.

Thanks,
Dscho

Hi Phillip,

I did that:

add -p (built-in): do not color the progress indicator separately

The Perl version of this command colors the progress indicator and the prompt message in one go.

Let's do the same in the built-in version so that the same upcoming test (which will compare the output of git add -p against a known-good version) will pass both for the Perl version as well as for the built-in version.

Well, since we want to supersede the Perl version by the built-in version, the latter needs to imitate the former, no?

Ciao,
Dscho

Hi Philippe,

[to make me completely confused about which spelling for "Phillip"/"Philippe" to use, Philip Oakley needs to chime in.]

Of course!

Thanks,
Dscho

Hi Phillip,

Oh, but that would require _all_ callers of the interactive patch functionality to be audited, to ensure that the correct git_config() call back is used.

That's positively not what I intended.

Instead, using repo_config_get_value() does not require that care, and does not open us up to future callers that may forget to include the required callback in _their_ config parsing.

In addition, using repo_config_get_value() already prepares this part of Git's code for a future where the struct repository * pointer is passed into every code path that needs it, so that we no longer have to rely on global state at all.

Besides, since this patch is really about fixing the discrepancy between the Perl version's use of .plain and the built-in's use of .context, changing something as unrelated as how the config is accessed would be inappropriate (even elsewhere in this patch series, which really is about fixing interactive add's color handling).

Ciao,
Dscho

This patch series started out as a tiny fix for a bug reported by Philippe Blain in
https://lore.kernel.org/git/[email protected]/.
And then I only wanted to add a regression test to make sure that this does not regress. And then I just wanted to ensure that it passes both with the Perl version of git add -i as well as with the built-in version.

And in no time I was looking at a real patch series.

Changes since v2:

* The commit messages of patches 7/11 and 9/11 now stress why we want to align the output of the Perl vs the built-in version so slavishly: to be able to validate both versions against prerecorded output.
* A typo was fixed in the commit message of patch 10/11.

Changes since v1:

* The regression test now actually exercises the re-coloring (that is the primary purpose of git add -p looking at the color.diff.* variables).
* The way the built-in git add -p renders hunk headers of split hunks was aligned with how the Perl version does things.
* We now consistently prefer color.diff.context over color.diff.plain, no matter whether using the Perl or the built-in version of git add -p.
* The commit message for the regression test no longer confuses readers by mentioning dash.
* The regression test was structured a bit better, first testing error message coloring, then the menu in git add -i and then the diff coloring in git add -p.

Johannes Schindelin (11):
add -i (built-in): do show an error message for incorrect inputs add -i (built-in): send error messages to stderr
add -p (built-in): imitate xdl_format_hunk_hdr() generating hunk
headers
add -i: use reset_color consistently
add -i (built-in): prevent the reset "color" from being configured add -i (built-in): use correct names to load color.diff.* config add -p (built-in): do not color the progress indicator separately add -i (built-in): use the same indentation as the Perl version add -i (Perl version): color header to match the C version add -p: prefer color.diff.context over color.diff.plain add -i: verify in the tests that colors can be overridden

 add-interactive.c          | 38 ++++++++++-------
 add-patch.c                | 25 +++++++-----
 git-add--interactive.perl  | 12 +++---
 t/t3701-add-interactive.sh | 84 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 127 insertions(+), 32 deletions(-)

base-commit: e4d83eee9239207622e2b1cc43967da5051c189c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-785%2Fdscho%2Ffix-add-i-colors-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-785/dscho/fix-add-i-colors-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/785

Range-diff vs v2:

  1:  6152122c04 =  1:  6152122c04 add -i (built-in): do show an error message for incorrect inputs
  2:  068813912b =  2:  068813912b add -i (built-in): send error messages to stderr
  3:  98deb538d9 =  3:  98deb538d9 add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headers
  4:  c857c44932 =  4:  c857c44932 add -i: use `reset_color` consistently
  5:  337b45cad8 =  5:  337b45cad8 add -i (built-in): prevent the `reset` "color" from being configured
  6:  dcd2ffc458 =  6:  dcd2ffc458 add -i (built-in): use correct names to load color.diff.* config
  7:  73b6d60a80 !  7:  6a68bc5511 add -p (built-in): do not color the progress indicator separately

@@ Commit message
add -p (built-in): do not color the progress indicator separately

The Perl version of this command colors the progress indicator and the
- prompt message in one go, let's do the same in the built-in version.
+ prompt message in one go.
+
+ Let's do the same in the built-in version so that the same upcoming test
+ (which will compare the output of git add -p against a known-good
+ version) will pass both for the Perl version as well as for the built-in
+ version.

  8:  91ded2fbbe =  8:  168891f9f8 add -i (built-in): use the same indentation as the Perl version
  9:  304614751e !  9:  094a4ad90c add -i (Perl version): include indentation in the colored header

@@ Metadata
Author: Johannes Schindelin <[email protected]>

## Commit message ##
- add -i (Perl version): include indentation in the colored header
+ add -i (Perl version): color header to match the C version

- The header is formatted by padding each column heading with spaces up to
- the length of 12 characters. These padding spaces are naturally included
- when coloring the entire header.
-
- However, the preceding five spaces indentation for non-flat lists were
- _not_ included in the Perl version, but _were_ included in the built-in
- version. Let's adjust the former to align with the latter's behavior.
+ Both versions of add -i indent non-flat lists by five spaces. However
+ when using color the C version prints these spaces after the ANSI color
+ codes whereas the Perl version prints them before the color codes.
+ Change the Perl version to match the C version to allow for introducing
+ a test that verifies that both versions produce the exact same output.

10: 48d8e0badf ! 10: 9a4d2a33b5 add -p: prefer color.diff.context over color.diff.plain
@@ Commit message
the hard-coded default (which in this case is simply the empty string, as context lines are typically rendered without colored).

- This still needs to inconsistencies when both config names are used: the
+ This still leads to inconsistencies when both config names are used: the
initial diff will be colored by the diff machinery. Once edited by a user, a hunk has to be re-colored by git add -p, though, which would then use the other setting to color the context lines.
11: c94abff72f = 11: 492f46833a add -i: verify in the tests that colors can be overridden

gitgitgadget

From: Johannes Schindelin <[email protected]>

There is a neat feature in git add -i where it allows users to select items via unique prefixes.

In the built-in version of git add -i, we specifically sort the items (unless they are already sorted) and then perform a binary search to figure out whether the input constitutes a unique prefix. Unfortunately, by mistake this code misidentifies matches even if the input string is not actually a prefix of any item.

For example, in the initial menu, where there is a status and an update command, the input tadaa was mistaken as a prefix of update.

Let's fix this by looking a bit closer whether the input is actually a prefix of the item at the found insert index.

---
 add-interactive.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 555c4abf32..8ca503d803 100644
@@ -194,7 +194,8 @@ static ssize_t find_unique(const char *string, struct prefix_item_list *list)
 	else if (index + 1 < list->sorted.nr &&
 		 starts_with(list->sorted.items[index + 1].string, string))
 		return -1;
-	else if (index < list->sorted.nr)
+	else if (index < list->sorted.nr &&
+		 starts_with(list->sorted.items[index].string, string))
 		item = list->sorted.items[index].util;
 	else
 		return -1;

gitgitgadget

From: Johannes Schindelin <[email protected]>

The Perl version of that command already does that since a301973641f (add -p: print errors in separate color, 2009-02-05). The built-in version's development started by reimplementing the initial version from 5cde71d64af (git-add --interactive, 2006-12-10) for simplicity, though, which still printed error messages to stdout.

Let's fix that by imitating the Perl version's behavior in the built-in version of that command.

---
 add-interactive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 8ca503d803..0f24992ca4 100644
@@ -365,7 +365,7 @@ static ssize_t list_and_choose(struct add_i_state *s,
 
 			if (from < 0 || from >= items->items.nr ||
 			    (singleton && from + 1 != to)) {
-				color_fprintf_ln(stdout, s->error_color,
+				color_fprintf_ln(stderr, s->error_color,
 						 _("Huh (%s)?"), p);
 				break;
 			} else if (singleton) {

gitgitgadget

From: Johannes Schindelin <[email protected]>

In libxdiff, imitating GNU diff, the hunk headers only show the line count if it is different from 1. When splitting hunks, the Perl version of git add -p already imitates this. Let's do the same in the built-in version of said command.

---
 add-patch.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

index be4cf6e9e5..b6d53229bb 100644
@@ -661,9 +661,14 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
 		else
 			new_offset += delta;
 
-		strbuf_addf(out, "@@ -%lu,%lu +%lu,%lu @@",
-			    old_offset, header->old_count,
-			    new_offset, header->new_count);
+		strbuf_addf(out, "@@ -%lu", old_offset);
+		if (header->old_count != 1)
+			strbuf_addf(out, ",%lu", header->old_count);
+		strbuf_addf(out, " +%lu", new_offset);
+		if (header->new_count != 1)
+			strbuf_addf(out, ",%lu", header->new_count);
+		strbuf_addstr(out, " @@");
+
 		if (len)
 			strbuf_add(out, p, len);
 		else if (colored)

gitgitgadget

From: Johannes Schindelin <[email protected]>

We already maintain a list of colors in the add_i_state, therefore we should use them.

---
 add-patch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index b6d53229bb..bf89c43145 100644
@@ -672,7 +672,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
 		if (len)
 			strbuf_add(out, p, len);
 		else if (colored)
-			strbuf_addf(out, "%s\n", GIT_COLOR_RESET);
+			strbuf_addf(out, "%s\n", s->s.reset_color);
 		else
 			strbuf_addch(out, '\n');
 	}

@@ -1065,7 +1065,7 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)
 			      s->s.file_new_color :
 			      s->s.context_color);
 		strbuf_add(&s->colored, plain + current, eol - current);
-		strbuf_addstr(&s->colored, GIT_COLOR_RESET);
+		strbuf_addstr(&s->colored, s->s.reset_color);
 		if (next > eol)
 			strbuf_add(&s->colored, plain + eol, next - eol);
 		current = next;

gitgitgadget

From: Johannes Schindelin <[email protected]>

The Perl version of that command sneakily uses git config --get-color to figure out the ANSI sequence to reset the color, but passes the empty string and therefore cannot actually match any config entry.

This was missed when re-implementing the command as a built-in command. Let's fix this, preventing the reset sequence from being overridden via the config.

---
 add-interactive.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 0f24992ca4..f3a1d7456e 100644
@@ -44,7 +44,6 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 	init_color(r, s, "help", s->help_color, GIT_COLOR_BOLD_RED);
 	init_color(r, s, "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE);
 	init_color(r, s, "error", s->error_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "reset", s->reset_color, GIT_COLOR_RESET);
 	init_color(r, s, "fraginfo", s->fraginfo_color,
 		   diff_get_color(s->use_color, DIFF_FRAGINFO));
 	init_color(r, s, "context", s->context_color,

@@ -54,6 +53,9 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 	init_color(r, s, "new", s->file_new_color,
 		diff_get_color(s->use_color, DIFF_FILE_NEW));
 
+	strlcpy(s->reset_color,
+		s->use_color ? GIT_COLOR_RESET : "", COLOR_MAXLEN);
+
 	FREE_AND_NULL(s->interactive_diff_filter);
 	git_config_get_string("interactive.difffilter",
 			      &s->interactive_diff_filter);

gitgitgadget

From: Johannes Schindelin <[email protected]>

The builtin version of add-interactive mistakenly loads diff colors from color.interactive.* instead of color.diff.*. It also accidentally spells frag as fraginfo.

Let's fix that.

Note also that we don't respect the historical diff.color.*. The perl version never did, and those have been deprecated since 2007.

---
 add-interactive.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

index f3a1d7456e..9126684348 100644
@@ -12,10 +12,10 @@
 #include "prompt.h"
 
 static void init_color(struct repository *r, struct add_i_state *s,
-		       const char *slot_name, char *dst,
+		       const char *section_and_slot, char *dst,
 		       const char *default_color)
 {
-	char *key = xstrfmt("color.interactive.%s", slot_name);
+	char *key = xstrfmt("color.%s", section_and_slot);
 	const char *value;
 
 	if (!s->use_color)

@@ -40,17 +40,20 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 			git_config_colorbool("color.interactive", value);
 	s->use_color = want_color(s->use_color);
 
-	init_color(r, s, "header", s->header_color, GIT_COLOR_BOLD);
-	init_color(r, s, "help", s->help_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE);
-	init_color(r, s, "error", s->error_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "fraginfo", s->fraginfo_color,
+	init_color(r, s, "interactive.header", s->header_color, GIT_COLOR_BOLD);
+	init_color(r, s, "interactive.help", s->help_color, GIT_COLOR_BOLD_RED);
+	init_color(r, s, "interactive.prompt", s->prompt_color,
+		   GIT_COLOR_BOLD_BLUE);
+	init_color(r, s, "interactive.error", s->error_color,
+		   GIT_COLOR_BOLD_RED);
+
+	init_color(r, s, "diff.frag", s->fraginfo_color,
 		   diff_get_color(s->use_color, DIFF_FRAGINFO));
-	init_color(r, s, "context", s->context_color,
+	init_color(r, s, "diff.context", s->context_color,
 		diff_get_color(s->use_color, DIFF_CONTEXT));
-	init_color(r, s, "old", s->file_old_color,
+	init_color(r, s, "diff.old", s->file_old_color,
 		diff_get_color(s->use_color, DIFF_FILE_OLD));
-	init_color(r, s, "new", s->file_new_color,
+	init_color(r, s, "diff.new", s->file_new_color,
 		diff_get_color(s->use_color, DIFF_FILE_NEW));
 
 	strlcpy(s->reset_color,

gitgitgadget

From: Johannes Schindelin <[email protected]>

The Perl version of this command colors the progress indicator and the prompt message in one go.

Let's do the same in the built-in version so that the same upcoming test (which will compare the output of git add -p against a known-good version) will pass both for the Perl version as well as for the built-in version.

---
 add-patch.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index bf89c43145..2fad92ca37 100644
@@ -1461,15 +1461,15 @@ static int patch_update_file(struct add_p_state *s,
 		else
 			prompt_mode_type = PROMPT_HUNK;
 
-		color_fprintf(stdout, s->s.prompt_color,
-			      "(%"PRIuMAX"/%"PRIuMAX") ",
+		printf("%s(%"PRIuMAX"/%"PRIuMAX") ", s->s.prompt_color,
 			      (uintmax_t)hunk_index + 1,
 			      (uintmax_t)(file_diff->hunk_nr
 						? file_diff->hunk_nr
 						: 1));
-		color_fprintf(stdout, s->s.prompt_color,
-			      _(s->mode->prompt_mode[prompt_mode_type]),
-			      s->buf.buf);
+		printf(_(s->mode->prompt_mode[prompt_mode_type]),
+		       s->buf.buf);
+		if (*s->s.reset_color)
+			fputs(s->s.reset_color, stdout);
 		fflush(stdout);
 		if (read_single_character(s) == EOF)
 			break;

gitgitgadget

From: Johannes Schindelin <[email protected]>

When copying the spaces used to indent non-flat lists in git add -i, one space was appended by mistake. This makes the output of the built-in version of git add -i inconsistent with the Perl version. Let's adjust the built-in version to produce the same output as the Perl version.

---
 add-interactive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 9126684348..c298a8b80f 100644
@@ -1137,7 +1137,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps)
 	print_file_item_data.color = data.color;
 	print_file_item_data.reset = data.reset;
 
-	strbuf_addstr(&header, "      ");
+	strbuf_addstr(&header, "     ");
 	strbuf_addf(&header, print_file_item_data.modified_fmt,
 		    _("staged"), _("unstaged"), _("path"));
 	opts.list_opts.header = header.buf;

gitgitgadget

From: Johannes Schindelin <[email protected]>

Both versions of add -i indent non-flat lists by five spaces. However when using color the C version prints these spaces after the ANSI color codes whereas the Perl version prints them before the color codes. Change the Perl version to match the C version to allow for introducing a test that verifies that both versions produce the exact same output.

---
 git-add--interactive.perl | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index e713fe3d02..adbac2bc6d 100755
@@ -483,10 +483,8 @@ sub list_and_choose {
 		my $last_lf = 0;
 
 		if ($opts->{HEADER}) {
-			if (!$opts->{LIST_FLAT}) {
-				print "     ";
-			}
-			print colored $header_color, "$opts->{HEADER}\n";
+			my $indent = $opts->{LIST_FLAT} ? "" : "     ";
+			print colored $header_color, "$indent$opts->{HEADER}\n";
 		}
 		for ($i = 0; $i < @stuff; $i++) {
 			my $chosen = $chosen[$i] ? '*' : ' ';

gitgitgadget

From: Johannes Schindelin <[email protected]>

Git's diff machinery allows users to override the colors to use in diffs, even the plain-colored context lines. As of 8dbf3eb6850 (diff.h:
rename DIFF_PLAIN color slot to DIFF_CONTEXT, 2015-05-27), the preferred name of the config setting is color.diff.context, although Git still allows color.diff.plain.

In the context of git add -p, this logic is a bit hard to replicate:
git_diff_basic_config() reads all config values sequentially and if it sees _any_ color.diff.context or color.diff.plain, it accepts the new color. The Perl version of git add -p needs to go through git config --get-color, though, which allows only one key to be specified. The same goes for the built-in version of git add -p, which has to go through repo_config_get_value().

The best we can do here is to look for .context and if none is found, fall back to looking for .plain, and if still not found, fall back to the hard-coded default (which in this case is simply the empty string, as context lines are typically rendered without colored).

This still leads to inconsistencies when both config names are used: the initial diff will be colored by the diff machinery. Once edited by a user, a hunk has to be re-colored by git add -p, though, which would then use the other setting to color the context lines.

In practice, this is not _all_ that bad. The git config manual says this in the color.diff.<slot>:

context (context text - plain is a historical synonym)

We should therefore assume that users use either one or the other, but not both names. Besides, it is relatively uncommon to look at a hunk after editing it because it is immediately staged by default.

---
 add-interactive.c         | 6 ++++--
 git-add--interactive.perl | 6 +++---
 2 files changed, 7 insertions(+), 5 deletions(-)

index c298a8b80f..54dfdc56f5 100644
@@ -49,8 +49,10 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 
 	init_color(r, s, "diff.frag", s->fraginfo_color,
 		   diff_get_color(s->use_color, DIFF_FRAGINFO));
-	init_color(r, s, "diff.context", s->context_color,
-		diff_get_color(s->use_color, DIFF_CONTEXT));
+	init_color(r, s, "diff.context", s->context_color, "fall back");
+	if (!strcmp(s->context_color, "fall back"))
+		init_color(r, s, "diff.plain", s->context_color,
+			   diff_get_color(s->use_color, DIFF_CONTEXT));
 	init_color(r, s, "diff.old", s->file_old_color,
 		diff_get_color(s->use_color, DIFF_FILE_OLD));
 	init_color(r, s, "diff.new", s->file_new_color,

index adbac2bc6d..bc3a1e8eff 100755
@@ -30,9 +30,9 @@
 	$diff_use_color ? (
 		$repo->get_color('color.diff.frag', 'cyan'),
 	) : ();
-my ($diff_plain_color) =
+my ($diff_context_color) =
 	$diff_use_color ? (
-		$repo->get_color('color.diff.plain', ''),
+		$repo->get_color($repo->config('color.diff.context') ? 'color.diff.context' : 'color.diff.plain', ''),
 	) : ();
 my ($diff_old_color) =
 	$diff_use_color ? (

@@ -1046,7 +1046,7 @@ sub color_diff {
 		colored((/^@/  ? $fraginfo_color :
 			 /^\+/ ? $diff_new_color :
 			 /^-/  ? $diff_old_color :
-			 $diff_plain_color),
+			 $diff_context_color),
 			$_);
 	} @_;
 }

gitgitgadget

From: Johannes Schindelin <[email protected]>

Now that the Perl version produces the same output as the built-in version (mostly fixing bugs in the latter), let's add a regression test to verify that it stays this way.

Note that we only grep for the colored error message instead of verifying that the entire stderr consists of just this one line: when running the test script using the -x option to trace the commands, the sub-shell in force_color causes those commands to be traced into err.raw (unless running in Bash where we set the BASH_XTRACEFD variable to avoid that).

Also note that the color reset in the <BLUE>+<RESET><BLUE>new<RESET> line might look funny and unnecessary, as the corresponding old line does not reset the color after the diff marker only to turn the color back on right away.

However, this is a (necessary) side effect of the white-space check: in emit_line_ws_markup(), we first emit the diff marker via emit_line_0() and then the rest of the line via ws_check_emit(). To leave them somewhat decoupled, the color has to be reset after the diff marker to allow for the rest of the line to start with another color (or inverted, in case of white-space issues).

Finally, we have to simulate hunk editing: the git add -p command cannot rely on the internal diff machinery for coloring after letting the user edit a hunk; It has to "re-color" the edited hunk. This is the primary reason why that command is interested in the exact values of the color.diff.* settings in the first place. To test this re-coloring, we therefore have to pretend to edit a hunk and then show that hunk in the regression test.

---
 t/t3701-add-interactive.sh | 84 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

index ca04fac417..cc3f434a97 100755
@@ -589,6 +589,90 @@ test_expect_success 'diffs can be colorized' '
 	grep "$(printf "\\033")" output
 '
 
+test_expect_success 'colors can be overridden' '
+	git reset --hard &&
+	test_when_finished "git rm -f color-test" &&
+	test_write_lines context old more-context >color-test &&
+	git add color-test &&
+	test_write_lines context new more-context another-one >color-test &&
+
+	echo trigger an error message >input &&
+	force_color git \
+		-c color.interactive.error=blue \
+		add -i 2>err.raw <input &&
+	test_decode_color <err.raw >err &&
+	grep "<BLUE>Huh (trigger)?<RESET>" err &&
+
+	test_write_lines help quit >input &&
+	force_color git \
+		-c color.interactive.header=red \
+		-c color.interactive.help=green \
+		-c color.interactive.prompt=yellow \
+		add -i >actual.raw <input &&
+	test_decode_color <actual.raw >actual &&
+	cat >expect <<-\EOF &&
+	<RED>           staged     unstaged path<RESET>
+	  1:        +3/-0        +2/-1 color-test
+
+	<RED>*** Commands ***<RESET>
+	  1: <YELLOW>s<RESET>tatus	  2: <YELLOW>u<RESET>pdate	  3: <YELLOW>r<RESET>evert	  4: <YELLOW>a<RESET>dd untracked
+	  5: <YELLOW>p<RESET>atch	  6: <YELLOW>d<RESET>iff	  7: <YELLOW>q<RESET>uit	  8: <YELLOW>h<RESET>elp
+	<YELLOW>What now<RESET>> <GREEN>status        - show paths with changes<RESET>
+	<GREEN>update        - add working tree state to the staged set of changes<RESET>
+	<GREEN>revert        - revert staged set of changes back to the HEAD version<RESET>
+	<GREEN>patch         - pick hunks and update selectively<RESET>
+	<GREEN>diff          - view diff between HEAD and index<RESET>
+	<GREEN>add untracked - add contents of untracked files to the staged set of changes<RESET>
+	<RED>*** Commands ***<RESET>
+	  1: <YELLOW>s<RESET>tatus	  2: <YELLOW>u<RESET>pdate	  3: <YELLOW>r<RESET>evert	  4: <YELLOW>a<RESET>dd untracked
+	  5: <YELLOW>p<RESET>atch	  6: <YELLOW>d<RESET>iff	  7: <YELLOW>q<RESET>uit	  8: <YELLOW>h<RESET>elp
+	<YELLOW>What now<RESET>> Bye.
+	EOF
+	test_cmp expect actual &&
+
+	: exercise recolor_hunk by editing and then look at the hunk again &&
+	test_write_lines s e K q >input &&
+	force_color git \
+		-c color.interactive.prompt=yellow \
+		-c color.diff.meta=italic \
+		-c color.diff.frag=magenta \
+		-c color.diff.context=cyan \
+		-c color.diff.old=bold \
+		-c color.diff.new=blue \
+		-c core.editor=touch \
+		add -p >actual.raw <input &&
+	test_decode_color <actual.raw >actual.decoded &&
+	sed "s/index [0-9a-f]*\\.\\.[0-9a-f]* 100644/<INDEX-LINE>/" <actual.decoded >actual &&
+	cat >expect <<-\EOF &&
+	<ITALIC>diff --git a/color-test b/color-test<RESET>
+	<ITALIC><INDEX-LINE><RESET>
+	<ITALIC>--- a/color-test<RESET>
+	<ITALIC>+++ b/color-test<RESET>
+	<MAGENTA>@@ -1,3 +1,4 @@<RESET>
+	<CYAN> context<RESET>
+	<BOLD>-old<RESET>
+	<BLUE>+<RESET><BLUE>new<RESET>
+	<CYAN> more-context<RESET>
+	<BLUE>+<RESET><BLUE>another-one<RESET>
+	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,s,e,?]? <RESET><BOLD>Split into 2 hunks.<RESET>
+	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
+	<CYAN> context<RESET>
+	<BOLD>-old<RESET>
+	<BLUE>+<RESET><BLUE>new<RESET>
+	<CYAN> more-context<RESET>
+	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
+	<CYAN> more-context<RESET>
+	<BLUE>+<RESET><BLUE>another-one<RESET>
+	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
+	<CYAN> context<RESET>
+	<BOLD>-old<RESET>
+	<BLUE>+new<RESET>
+	<CYAN> more-context<RESET>
+	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? <RESET>
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '
 	git reset --hard &&
 

gitgitgadget

OK, this is the part I didn't quite understand. My contention was: if we are regenerating a new hunk header after we split, why do we care what was in the old one?

This "rest of the hunk header" is what I didn't get. You are talking here about the funcname header. Which is not colored by default, but people can set color.diff.func.

And here the builtin does differ from the perl script quite a bit. The perl script generates the split hunk headers from scratch, and does not bother to include the funcname in the split header:

  $ git show :foo.c
  void foo()
  {
  	call();
  	some();
  	functions();
  }

  $ cat foo.c
  void foo()
  {
  	call();
  	some();
  	more();
  	functions();
  	return;
  }

  $ git add -p
  diff --git a/foo.c b/foo.c
  index 270ccc7..0365419 100644
  --- a/foo.c
  +++ b/foo.c
  @@ -2,5 +2,7 @@ void foo()
   {
   	call();
   	some();
  +	more();
   	functions();
  +	return;
   }
  (1/1) Stage this hunk [y,n,q,a,d,s,e,?]? s
  Split into 2 hunks.
  @@ -2,4 +2,5 @@
   {
   	call();
   	some();
  +	more();
   	functions();
  (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? n
  @@ -5,2 +6,3 @@
   	functions();
  +	return;
   }
  (2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? n

So it does not appear at all after the split, colored or otherwise. I think the behavior of the builtin version is better in this case.

It does place more restrictions on what the diffFilter can do, as we've been discussing. The same thing could be accomplished by retaining the uncolored func header, and then coloring it on the fly within add-patch.c (exactly the same way we color the numeric part of the hunk header).

It may also be worth covering this with a test (it was a surprise to me that the builtin version was handling this, though as I said I do agree it's an improvement).

I agree that diffFilter is quite fringe. And as I said before, I'm fine punting on it for now. Mostly this was just the first I had found out about the difference, and because it is user-visible, it may be something that comes up later. So I wanted to record a description of the problem for later.

Yes, that's what I use it for. I wrote it mostly to pipe through diff-highlight, though also it was to help diff-so-fancy folks (which I don't use myself, but they build on diff-highlight). However, it does seem that they haven't really worked out the problems in the meantime.

-Peff

Well, it is the first time I am finding out about it, too. ;)

IMHO that's the wrong direction. My intent with the feature was to give filters as much flexibility as possible, and beyond having 1-to-1 correspondence of lines, we don't know or care what they produce. We don't even know what other filters might exist out there (though I would be surprised if there is anything more exotic than intra-line coloring; other folks have pointed to "delta" for this use, but only in its color-only mode).

If anybody wants to put work into it, I think a better direction is having Git keep its own colored hunks and just present single-hunks to a custom display command to show to the user. Then we can continue not to care about the format of the individual filters, _and_ they can get the opportunity to format our rewritten split-hunk headers, etc.

Yeah, absolutely.

-Peff

This version looks fine to me, and I agree is a good stopping point for now (the only thing I'd consider adding is a test for the funcname in a split-hunk header, but it would have to be expect_failure for now; it is probably not worth the effort to rewrite the perl version to behave like the builtin here if we think it's going away soon).

-Peff

Actually, I couldn't resist (but again, I don't think this has to be part of your series):

index e713fe3d02..90561ea0e2 100755
@@ -30,6 +30,10 @@
 	$diff_use_color ? (
 		$repo->get_color('color.diff.frag', 'cyan'),
 	) : ();
+my ($funcname_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.func', ''),
+	) : ();
 my ($diff_plain_color) =
 	$diff_use_color ? (
 		$repo->get_color('color.diff.plain', ''),

@@ -780,11 +784,11 @@ sub hunk_splittable {
 
 sub parse_hunk_header {
 	my ($line) = @_;
-	my ($o_ofs, $o_cnt, $n_ofs, $n_cnt) =
-	    $line =~ /^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/;
+	my ($o_ofs, $o_cnt, $n_ofs, $n_cnt, $funcname) =
+	    $line =~ /^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@ ?(.*)/;
 	$o_cnt = 1 unless defined $o_cnt;
 	$n_cnt = 1 unless defined $n_cnt;
-	return ($o_ofs, $o_cnt, $n_ofs, $n_cnt);
+	return ($o_ofs, $o_cnt, $n_ofs, $n_cnt, $funcname);
 }
 
 sub format_hunk_header {

@@ -806,7 +810,8 @@ sub split_hunk {
 	# it can be split, but we would need to take care of
 	# overlaps later.
 
-	my ($o_ofs, undef, $n_ofs) = parse_hunk_header($text->[0]);
+	my ($o_ofs, undef, $n_ofs, undef, $funcname) =
+		parse_hunk_header($text->[0]);
 	my $hunk_start = 1;
 
       OUTER:

@@ -894,6 +899,11 @@ sub split_hunk {
 		if ($diff_use_color) {
 			$display_head = colored($fraginfo_color, $head);
 		}
+		if (length $funcname) {
+			my $f = colored($funcname_color, $funcname);
+			$display_head =~ s/$/ $f/;
+			$funcname = '';
+		}
 		unshift @{$hunk->{DISPLAY}}, $display_head;
 	}
 	return @split;

Will mark the topic to be merged to 'next'.

It seems that these days I am too quick and eager to take more things in 'seen' than the rate at I can convince myself that the topics are ready and accumulate too many topics in the "undecided" category too fast. I probably should slow down the intake a bit to match.

Thanks.