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

From: Jeff King <peff@peff.net>
Date: Nov 11, 2020, 18:45 UTC
Message-ID: <20201111184527.GD9902@coredump.intra.peff.net>
In-Reply-To: <pull.785.v2.git.1605097704.gitgitgadget@gmail.com>

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

Show 13 quoted lines

> 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