patch, 9 partsFix 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/313B8999-1E99-4695-A20D-E48840C30879@gmail.com/. 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

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-add--interactive.perl | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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`).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3701-add-interactive.sh | 53 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

On Tue, Nov 10, 2020 at 11:42:19PM +0000, Johannes Schindelin via GitGitGadget wrote:

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.

Since you are being so thorough, and since it is already in the output, maybe color color.diff.meta, too? Like:

Hi Peff,

On Tue, 10 Nov 2020, Jeff King wrote:

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.

Oh my. I really had tried to avoid going _this_ deep. The `.meta` setting is not even read by the interactive add command:

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

comes up empty.

The reason is that we actually let the diff machinery do all the coloring. So why, then, you might ask, do we bother to read those values in the first place?

Well, you know, in `git add -p`, you can split hunks. That's still fine, we basically just have to color the hunk header that we insert, and can reuse the original coloring of the remaining lines, because they are unchanged.

But. But you can also `edit` hunks.

And after that `edit` step, those hunks have to be re-colored. And _that_ is why `git-add--interactive` bothers to read those `color.diff.*` values to begin with.

Now, how do you see this re-coloring in action?

I am almost glad that you asked. By simulating that `edit` in the regression test. But that is not enough because the hunk is staged immediately after editing. But you _can_ go back to that hunk, even if it already has been changed, and make up your mind to _not_ stage it, but that only works if `git add -p` hasn't quit already, which it would do with a single hunk, so we have to have _two_ hunks.

This not only complicates the regression test, but _of course_ (because _why would I already be done with this patch series???_) it shows further differences between the Perl version and the built-in version. It's almost as if `git add -i` said "Johannes, you think those 500 hours were all you would spend on converting me to a built-in? Think again!".

One of the issues I found (and fixed in v2) was that the built-in version tried to be practical and _not_ special-case a count of one in the hunk header (`@@ -3 +3 @@` is equivalent to `@@ -3,1 +3,1 @@`). Well, now it does, just like libxdiff and the Perl version.

The other issue was that the Perl version still used `color.diff.plain` instead of `color.diff.context`. I hope I found a good-enough solution to the problem that we simply cannot call `git config --get-color` or `repo_get_config()` with _two_ keys to look for (and the last one wins).

For those reasons, v2 brings more changes than I had hoped for. In the end, it is a better patch series, obviously. So even if I was reluctant to work on all this: thank you for prodding me.

Ciao, Dscho

On Wed, Nov 11, 2020 at 04:53:13PM +0100, Johannes Schindelin wrote:

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

Hi Peff,

On Wed, 11 Nov 2020, Jeff King wrote:

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

On Thu, Nov 12, 2020 at 03:04:01PM +0100, Johannes Schindelin wrote:

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:

Hi Peff,

On Thu, 12 Nov 2020, Jeff King wrote:

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

On Mon, Nov 16, 2020 at 12:35:44AM +0100, Johannes Schindelin wrote:

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

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

On Tue, Nov 10, 2020 at 11:42:10PM +0000, Johannes Schindelin via GitGitGadget wrote:

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/313B8999-1E99-4695-A20D-E48840C30879@gmail.com/. 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 <peff@peff.net>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## 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
      +'
      +

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Hi Dscho

On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:

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 Phillip,

On Fri, 13 Nov 2020, Phillip Wood wrote:

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 Dscho

On 13/11/2020 13:57, Johannes Schindelin wrote:

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 Phillip,

On Fri, 13 Nov 2020, Phillip Wood wrote:

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

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c         | 6 ++++--
 git-add--interactive.perl | 6 +++---
 2 files changed, 7 insertions(+), 5 deletions(-)

Hi Dscho, 

s/needs/leads/

Philippe.

Hi Philippe,

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

On Fri, 13 Nov 2020, Philippe Blain wrote:

Of course!

Thanks, Dscho

Hi Dscho

On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:

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 Phillip,

On Fri, 13 Nov 2020, Phillip Wood wrote:

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

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-add--interactive.perl | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Hi Dscho

On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:

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,

On Fri, 13 Nov 2020, Phillip Wood wrote:

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

On 13/11/2020 13:58, Johannes Schindelin wrote:

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 Phillip,

On Fri, 13 Nov 2020, Phillip Wood wrote:

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

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3701-add-interactive.sh | 84 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Hi Dscho

On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:

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 Phillip,

On Fri, 13 Nov 2020, Phillip Wood wrote:

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 Dscho

On 13/11/2020 13:53, Johannes Schindelin wrote:

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 Phillip,

On Fri, 13 Nov 2020, Phillip Wood wrote:

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

On Wed, Nov 11, 2020 at 12:28:13PM +0000, Johannes Schindelin via GitGitGadget wrote:

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,

On Wed, 11 Nov 2020, Jeff King wrote:

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

On Thu, Nov 12, 2020 at 03:20:38PM +0100, Johannes Schindelin wrote:

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 Peff,

On Thu, 12 Nov 2020, Jeff King wrote:

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

On Mon, Nov 16, 2020 at 12:40:32AM +0100, Johannes Schindelin wrote:

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 patch series started out as a tiny fix for a bug reported by Philippe Blain in https://lore.kernel.org/git/313B8999-1E99-4695-A20D-E48840C30879@gmail.com/. 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.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
  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 <Johannes.Schindelin@gmx.de>
      
       ## 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.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
 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

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3701-add-interactive.sh | 84 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-add--interactive.perl | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c         | 6 ++++--
 git-add--interactive.perl | 6 +++---
 2 files changed, 7 insertions(+), 5 deletions(-)

On Mon, Nov 16, 2020 at 04:08:21PM +0000, Johannes Schindelin via GitGitGadget wrote:

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

On Mon, Nov 16, 2020 at 08:51:49PM -0500, Jeff King wrote:

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

Jeff King <peff@peff.net> writes:

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.