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
────────────────────────────────────────────────────────────────────────────────
modified: color-test
────────────────────────────────────────────────────────────────────────────────
@ color-test:1 @
context
old
new
more-context
another-one
(1/1) Stage this hunk [y,n,q,a,d,s,e,?]? s
Split into 2 hunks.
@@ -1,3 +1,3 @@
old
new
more-context
another-one
(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]?
-- 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