Re: [PATCH v2 00/11] Fix color handling in git add -i

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Jeff King <peff@peff.net>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org,
	Philippe Blain <levraiphilippeblain@gmail.com>
Subject: Re: [PATCH v2 00/11] Fix color handling in git add -i
Date: Thu, 12 Nov 2020 15:20:38 +0100 (CET)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.2011121504110.18437@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20201111184527.GD9902@coredump.intra.peff.net>

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

Hi Peff,

On Wed, 11 Nov 2020, Jeff King wrote:

> On Wed, Nov 11, 2020 at 12:28:13PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > 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.
>
> 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,?]?

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.

> 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'

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

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

Oh, _that_ I wholeheartedly agree with.

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

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