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,?]?
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
