mbox series

[v2,00/11] Fix color handling in git add -i

Message ID pull.785.v2.git.1605097704.gitgitgadget@gmail.com (mailing list archive)
Headers show
Series Fix color handling in git add -i | expand

Message

Kazuhiro Kato via GitGitGadget Nov. 11, 2020, 12:28 p.m. UTC
This patch series started out as a tiny fix for a bug reported by Philippe
Blain in 
https://lore.kernel.org/git/313B8999-1E99-4695-A20D-E48840C30879@gmail.com/.
And then I only wanted to add a regression test to make sure that this does
not regress. And then I just wanted to ensure that it passes both with the
Perl version of git add -i as well as with the built-in version.

And in no time I was looking at a real patch series.

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.

Johannes Schindelin (11):
  add -i (built-in): do show an error message for incorrect inputs
  add -i (built-in): send error messages to stderr
  add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk
    headers
  add -i: use `reset_color` consistently
  add -i (built-in): prevent the `reset` "color" from being configured
  add -i (built-in): use correct names to load color.diff.* config
  add -p (built-in): do not color the progress indicator separately
  add -i (built-in): use the same indentation as the Perl version
  add -i (Perl version): include indentation in the colored header
  add -p: prefer color.diff.context over color.diff.plain
  add -i: verify in the tests that colors can be overridden

 add-interactive.c          | 38 ++++++++++-------
 add-patch.c                | 25 +++++++-----
 git-add--interactive.perl  | 12 +++---
 t/t3701-add-interactive.sh | 84 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 127 insertions(+), 32 deletions(-)


base-commit: e4d83eee9239207622e2b1cc43967da5051c189c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-785%2Fdscho%2Ffix-add-i-colors-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-785/dscho/fix-add-i-colors-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/785

Range-diff vs v1:

  1:  6152122c04 =  1:  6152122c04 add -i (built-in): do show an error message for incorrect inputs
  2:  068813912b =  2:  068813912b add -i (built-in): send error messages to stderr
  -:  ---------- >  3:  98deb538d9 add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headers
  3:  9a1ba71977 =  4:  c857c44932 add -i: use `reset_color` consistently
  4:  b48c878fad =  5:  337b45cad8 add -i (built-in): prevent the `reset` "color" from being configured
  5:  85b0d27d76 =  6:  dcd2ffc458 add -i (built-in): use correct names to load color.diff.* config
  6:  059344bfaf =  7:  73b6d60a80 add -p (built-in): do not color the progress indicator separately
  7:  8df87e6395 =  8:  91ded2fbbe add -i (built-in): use the same indentation as the Perl version
  8:  42113e20dd =  9:  304614751e add -i (Perl version): include indentation in the colored header
  -:  ---------- > 10:  48d8e0badf add -p: prefer color.diff.context over color.diff.plain
  9:  38355ec98f ! 11:  c94abff72f add -i: verify in the tests that colors can be overridden
     @@ Commit message
      
          Note that we only `grep` for the colored error message instead of
          verifying that the entire `stderr` consists of just this one line: when
     -    running the test script via `dash`, using the `-x` option to trace the
     +    running the test script using the `-x` option to trace the
          commands, the sub-shell in `force_color` causes those commands to be
     -    traced into `err.raw` because we set, but do not export
     -    `BASH_XTRACEFD`).
     +    traced into `err.raw` (unless running in Bash where we set the
     +    `BASH_XTRACEFD` variable to avoid that).
      
     +    Also note that the color reset in the `<BLUE>+<RESET><BLUE>new<RESET>`
     +    line might look funny and unnecessary, as the corresponding `old` line
     +    does not reset the color after the diff marker only to turn the color
     +    back on right away.
     +
     +    However, this is a (necessary) side effect of the white-space check: in
     +    `emit_line_ws_markup()`, we first emit the diff marker via
     +    `emit_line_0()` and then the rest of the line via `ws_check_emit()`. To
     +    leave them somewhat decoupled, the color has to be reset after the diff
     +    marker to allow for the rest of the line to start with another color (or
     +    inverted, in case of white-space issues).
     +
     +    Finally, we have to simulate hunk editing: the `git add -p` command
     +    cannot rely on the internal diff machinery for coloring after letting
     +    the user edit a hunk; It has to "re-color" the edited hunk. This is the
     +    primary reason why that command is interested in the exact values of the
     +    `color.diff.*` settings in the first place. To test this re-coloring, we
     +    therefore have to pretend to edit a hunk and then show that hunk in the
     +    regression test.
     +
     +    Co-authored-by: Jeff King <peff@peff.net>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## t/t3701-add-interactive.sh ##
     @@ t/t3701-add-interactive.sh: test_expect_success 'diffs can be colorized' '
       '
       
      +test_expect_success 'colors can be overridden' '
     -+	test_config color.interactive.header red &&
     -+	test_config color.interactive.help green &&
     -+	test_config color.interactive.prompt yellow &&
     -+	test_config color.interactive.error blue &&
     -+	test_config color.diff.frag magenta &&
     -+	test_config color.diff.context cyan &&
     -+	test_config color.diff.old bold &&
     -+	test_config color.diff.new "bold red" &&
     -+
      +	git reset --hard &&
      +	test_when_finished "git rm -f color-test" &&
      +	test_write_lines context old more-context >color-test &&
      +	git add color-test &&
     -+	test_write_lines context new more-context >color-test &&
     ++	test_write_lines context new more-context another-one >color-test &&
      +
      +	echo trigger an error message >input &&
     -+	force_color git add -i 2>err.raw <input &&
     ++	force_color git \
     ++		-c color.interactive.error=blue \
     ++		add -i 2>err.raw <input &&
      +	test_decode_color <err.raw >err &&
      +	grep "<BLUE>Huh (trigger)?<RESET>" err &&
      +
     -+	test_write_lines patch color-test "" y quit >input &&
     -+	force_color git add -i >actual.raw <input &&
     -+	test_decode_color <actual.raw >actual.decoded &&
     -+	sed "/index [0-9a-f]*\\.\\.[0-9a-f]* 100644/d" <actual.decoded >actual &&
     ++	test_write_lines help quit >input &&
     ++	force_color git \
     ++		-c color.interactive.header=red \
     ++		-c color.interactive.help=green \
     ++		-c color.interactive.prompt=yellow \
     ++		add -i >actual.raw <input &&
     ++	test_decode_color <actual.raw >actual &&
      +	cat >expect <<-\EOF &&
      +	<RED>           staged     unstaged path<RESET>
     -+	  1:        +3/-0        +1/-1 color-test
     ++	  1:        +3/-0        +2/-1 color-test
      +
      +	<RED>*** Commands ***<RESET>
      +	  1: <YELLOW>s<RESET>tatus	  2: <YELLOW>u<RESET>pdate	  3: <YELLOW>r<RESET>evert	  4: <YELLOW>a<RESET>dd untracked
      +	  5: <YELLOW>p<RESET>atch	  6: <YELLOW>d<RESET>iff	  7: <YELLOW>q<RESET>uit	  8: <YELLOW>h<RESET>elp
     -+	<YELLOW>What now<RESET>> <RED>           staged     unstaged path<RESET>
     -+	  1:        +3/-0        +1/-1 <YELLOW>c<RESET>olor-test
     -+	<YELLOW>Patch update<RESET>>> <RED>           staged     unstaged path<RESET>
     -+	* 1:        +3/-0        +1/-1 <YELLOW>c<RESET>olor-test
     -+	<YELLOW>Patch update<RESET>>> <BOLD>diff --git a/color-test b/color-test<RESET>
     -+	<BOLD>--- a/color-test<RESET>
     -+	<BOLD>+++ b/color-test<RESET>
     -+	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
     -+	<CYAN> context<RESET>
     -+	<BOLD>-old<RESET>
     -+	<BOLD;RED>+<RESET><BOLD;RED>new<RESET>
     -+	<CYAN> more-context<RESET>
     -+	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,e,?]? <RESET>
     ++	<YELLOW>What now<RESET>> <GREEN>status        - show paths with changes<RESET>
     ++	<GREEN>update        - add working tree state to the staged set of changes<RESET>
     ++	<GREEN>revert        - revert staged set of changes back to the HEAD version<RESET>
     ++	<GREEN>patch         - pick hunks and update selectively<RESET>
     ++	<GREEN>diff          - view diff between HEAD and index<RESET>
     ++	<GREEN>add untracked - add contents of untracked files to the staged set of changes<RESET>
      +	<RED>*** Commands ***<RESET>
      +	  1: <YELLOW>s<RESET>tatus	  2: <YELLOW>u<RESET>pdate	  3: <YELLOW>r<RESET>evert	  4: <YELLOW>a<RESET>dd untracked
      +	  5: <YELLOW>p<RESET>atch	  6: <YELLOW>d<RESET>iff	  7: <YELLOW>q<RESET>uit	  8: <YELLOW>h<RESET>elp
      +	<YELLOW>What now<RESET>> Bye.
      +	EOF
     ++	test_cmp expect actual &&
     ++
     ++	: exercise recolor_hunk by editing and then look at the hunk again &&
     ++	test_write_lines s e K q >input &&
     ++	force_color git \
     ++		-c color.interactive.prompt=yellow \
     ++		-c color.diff.meta=italic \
     ++		-c color.diff.frag=magenta \
     ++		-c color.diff.context=cyan \
     ++		-c color.diff.old=bold \
     ++		-c color.diff.new=blue \
     ++		-c core.editor=touch \
     ++		add -p >actual.raw <input &&
     ++	test_decode_color <actual.raw >actual.decoded &&
     ++	sed "s/index [0-9a-f]*\\.\\.[0-9a-f]* 100644/<INDEX-LINE>/" <actual.decoded >actual &&
     ++	cat >expect <<-\EOF &&
     ++	<ITALIC>diff --git a/color-test b/color-test<RESET>
     ++	<ITALIC><INDEX-LINE><RESET>
     ++	<ITALIC>--- a/color-test<RESET>
     ++	<ITALIC>+++ b/color-test<RESET>
     ++	<MAGENTA>@@ -1,3 +1,4 @@<RESET>
     ++	<CYAN> context<RESET>
     ++	<BOLD>-old<RESET>
     ++	<BLUE>+<RESET><BLUE>new<RESET>
     ++	<CYAN> more-context<RESET>
     ++	<BLUE>+<RESET><BLUE>another-one<RESET>
     ++	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,s,e,?]? <RESET><BOLD>Split into 2 hunks.<RESET>
     ++	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
     ++	<CYAN> context<RESET>
     ++	<BOLD>-old<RESET>
     ++	<BLUE>+<RESET><BLUE>new<RESET>
     ++	<CYAN> more-context<RESET>
     ++	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
     ++	<CYAN> more-context<RESET>
     ++	<BLUE>+<RESET><BLUE>another-one<RESET>
     ++	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
     ++	<CYAN> context<RESET>
     ++	<BOLD>-old<RESET>
     ++	<BLUE>+new<RESET>
     ++	<CYAN> more-context<RESET>
     ++	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? <RESET>
     ++	EOF
      +	test_cmp expect actual
      +'
      +

Comments

Jeff King Nov. 11, 2020, 6:45 p.m. UTC | #1
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
Johannes Schindelin Nov. 12, 2020, 2:20 p.m. UTC | #2
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
Jeff King Nov. 12, 2020, 6:40 p.m. UTC | #3
On Thu, Nov 12, 2020 at 03:20:38PM +0100, Johannes Schindelin wrote:

> >   $ 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`):

Yes, there are definitely problems with how diff-so-fancy represents
things (because they don't maintain a 1-to-1 correspondence). You should
generally get a complaint like:

  $ git -c interactive.difffilter='diff-so-fancy' add -p
  fatal: mismatched output from interactive.diffFilter
  hint: Your filter must maintain a one-to-one correspondence
  hint: between its input and output lines.

The diff-so-fancy folks have asked me about this, and I've made clear
the problem to them, and that the solution is to have a mode which
maintains the line correspondence. AFAIK there's no fix yet. I don't
know how many people are actually using it in practice in its current
buggy state.

There's a big thread here:

  https://github.com/so-fancy/diff-so-fancy/issues/35

I don't really recommend reading the whole thing. Beyond what I just
said, the interesting bits are:

  - people recommend using the "delta" tool; it has a "--color-only"
    option which does just diff-highlight-style coloring, but doesn't
    break line correspondence

  - somebody suggested instead of using interactive.difffilter, to add
    an option to show each hunk in an individual pager command. So
    add-interactive would never see the "fancy" version at all, but it
    would be generated on the fly when the user sees it (and that filter
    would have to be able to handle seeing individual hunks without the
    diff header).

All of which is to say that the current state is a bit of a mess, and I
don't consider it completely necessary to fix it before the builtin
version becomes the default. But:

  - I think we should expect some possible complaints / bug reports
    from fringe users of the already-somewhat-broken state

  - IMHO the parsing of the filtered version done by the builtin is
    going in the wrong direction (see my other mail for an elaboration)

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

Yeah, obviously calling die() in the split case is bad. And the fact
that newly-created split hunk headers are not filtered the same way as
the original hunk headers isn't ideal. But it's a pretty easy fix in the
perl version, and not in the builtin version.

-Peff
Johannes Schindelin Nov. 15, 2020, 11:40 p.m. UTC | #4
Hi Peff,

On Thu, 12 Nov 2020, Jeff King wrote:

> On Thu, Nov 12, 2020 at 03:20:38PM +0100, Johannes Schindelin wrote:
>
> > >   $ 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`):
>
> Yes, there are definitely problems with how diff-so-fancy represents
> things (because they don't maintain a 1-to-1 correspondence). You should
> generally get a complaint like:
>
>   $ git -c interactive.difffilter='diff-so-fancy' add -p
>   fatal: mismatched output from interactive.diffFilter
>   hint: Your filter must maintain a one-to-one correspondence
>   hint: between its input and output lines.
>
> The diff-so-fancy folks have asked me about this, and I've made clear
> the problem to them, and that the solution is to have a mode which
> maintains the line correspondence. AFAIK there's no fix yet. I don't
> know how many people are actually using it in practice in its current
> buggy state.
>
> There's a big thread here:
>
>   https://github.com/so-fancy/diff-so-fancy/issues/35
>
> I don't really recommend reading the whole thing. Beyond what I just
> said, the interesting bits are:
>
>   - people recommend using the "delta" tool; it has a "--color-only"
>     option which does just diff-highlight-style coloring, but doesn't
>     break line correspondence
>
>   - somebody suggested instead of using interactive.difffilter, to add
>     an option to show each hunk in an individual pager command. So
>     add-interactive would never see the "fancy" version at all, but it
>     would be generated on the fly when the user sees it (and that filter
>     would have to be able to handle seeing individual hunks without the
>     diff header).
>
> All of which is to say that the current state is a bit of a mess, and I
> don't consider it completely necessary to fix it before the builtin
> version becomes the default. But:
>
>   - I think we should expect some possible complaints / bug reports
>     from fringe users of the already-somewhat-broken state
>
>   - IMHO the parsing of the filtered version done by the builtin is
>     going in the wrong direction (see my other mail for an elaboration)

It's a little bit of a surprise that this is the first time I hear about
this.

The way _I_ would go about fixing this is to look for a tell-tale that
we're looking at a `diff-so-fancy` style output, e.g. by looking for that
horizontal line, then adding special code to handle that.

This is not a minor undertaking, and it would currently require _two_
implementations: the Perl version and the built-in version. They would
look _very_ different from one another. Therefore, I would probably either
wait until the Perl version is retired, or selectively only adjust the
built-in version, if _I_ was to implement this.

But given that it does not work right now, and given that it has not been
working for a long time, I think it does not hurt so much if it is left in
the current state for a bit longer. I would love to focus on the patch
series that kicked off this discussion first, getting it done, before I
think about other `add -p` aspects.

Ciao,
Dscho

>
> > > 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.
>
> Yeah, obviously calling die() in the split case is bad. And the fact
> that newly-created split hunk headers are not filtered the same way as
> the original hunk headers isn't ideal. But it's a pretty easy fix in the
> perl version, and not in the builtin version.
>
> -Peff
>
Jeff King Nov. 17, 2020, 1:49 a.m. UTC | #5
On Mon, Nov 16, 2020 at 12:40:32AM +0100, Johannes Schindelin wrote:

> > All of which is to say that the current state is a bit of a mess, and I
> > don't consider it completely necessary to fix it before the builtin
> > version becomes the default. But:
> >
> >   - I think we should expect some possible complaints / bug reports
> >     from fringe users of the already-somewhat-broken state
> >
> >   - IMHO the parsing of the filtered version done by the builtin is
> >     going in the wrong direction (see my other mail for an elaboration)
> 
> It's a little bit of a surprise that this is the first time I hear about
> this.

Well, it is the first time I am finding out about it, too. ;)

> The way _I_ would go about fixing this is to look for a tell-tale that
> we're looking at a `diff-so-fancy` style output, e.g. by looking for that
> horizontal line, then adding special code to handle that.

IMHO that's the wrong direction. My intent with the feature was to give
filters as much flexibility as possible, and beyond having 1-to-1
correspondence of lines, we don't know or care what they produce. We
don't even know what other filters might exist out there (though I would
be surprised if there is anything more exotic than intra-line coloring;
other folks have pointed to "delta" for this use, but only in its
color-only mode).

If anybody wants to put work into it, I think a better direction is
having Git keep its own colored hunks and just present single-hunks to a
custom display command to show to the user. Then we can continue not to
care about the format of the individual filters, _and_ they can get the
opportunity to format our rewritten split-hunk headers, etc.

> But given that it does not work right now, and given that it has not been
> working for a long time, I think it does not hurt so much if it is left in
> the current state for a bit longer. I would love to focus on the patch
> series that kicked off this discussion first, getting it done, before I
> think about other `add -p` aspects.

Yeah, absolutely.

-Peff