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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 12, 2020, 14:20 UTC
Message-ID: <nycvar.QRO.7.76.6.2011121504110.18437@tvgsbejvaqbjf.bet>
In-Reply-To: <20201111184527.GD9902@coredump.intra.peff.net>

Hi Peff,

On Wed, 11 Nov 2020, Jeff King wrote:

Show 37 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 6 quoted lines

> 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