Re: [PATCH 9/9] add -i: verify in the tests that colors can be overridden

On Mon, Nov 16, 2020 at 12:35:44AM +0100, Johannes Schindelin wrote:

quoted

Whereas add-patch parses the colors off of the version and then
re-colors every hunk header. Which seems doubly weird to me. Even if we
were going to re-color every hunk (e.g., because we don't want to store
the original hunk line, but instead a parsed version of it), why bother
parsing the color version at all, and not just the machine-readable
version?

Let's continue on this distraction for a bit before I go back to fixing
the patch series, which actually tries to fix a _different_ concern.

The reason why `add-patch.c` "parses the colors off" is that we want to
show the rest of the hunk header, in color, even after splitting hunks
(this will only be shown for the first hunk, of course).

OK, this is the part I didn't quite understand. My contention was: if we
are regenerating a new hunk header after we split, why do we care what
was in the old one?

This "rest of the hunk header" is what I didn't get. You are talking
here about the funcname header. Which is not colored by default, but
people can set color.diff.func.

And here the builtin does differ from the perl script quite a bit. The
perl script generates the split hunk headers from scratch, and does not
bother to include the funcname in the split header:

  $ git show :foo.c
  void foo()
  {
  	call();
  	some();
  	functions();
  }
  
  $ cat foo.c
  void foo()
  {
  	call();
  	some();
  	more();
  	functions();
  	return;
  }

  $ git add -p
  diff --git a/foo.c b/foo.c
  index 270ccc7..0365419 100644
  --- a/foo.c
  +++ b/foo.c
  @@ -2,5 +2,7 @@ void foo()
   {
   	call();
   	some();
  +	more();
   	functions();
  +	return;
   }
  (1/1) Stage this hunk [y,n,q,a,d,s,e,?]? s
  Split into 2 hunks.
  @@ -2,4 +2,5 @@
   {
   	call();
   	some();
  +	more();
   	functions();
  (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? n
  @@ -5,2 +6,3 @@
   	functions();
  +	return;
   }
  (2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? n

So it does not appear at all after the split, colored or otherwise. I
think the behavior of the builtin version is better in this case.

It does place more restrictions on what the diffFilter can do, as we've
been discussing. The same thing could be accomplished by retaining the
uncolored func header, and then coloring it on the fly within
add-patch.c (exactly the same way we color the numeric part of the hunk
header).

It may also be worth covering this with a test (it was a surprise to me
that the builtin version was handling this, though as I said I do agree
it's an improvement).

But given that `git add -p` is somewhat of a fringe use, and using
`diffFilter` is _even_ more fringe, I do not really want to spend any
further time on this tangent.

I agree that diffFilter is quite fringe. And as I said before, I'm fine
punting on it for now. Mostly this was just the first I had found out
about the difference, and because it is user-visible, it may be
something that comes up later. So I wanted to record a description of
the problem for later.

My thinking back then was: what if _I_ want to use a diffFilter? For what
would I use it? Probably to emphasize certain hunk headers more, by adding
more color to the part after the line range.

Yes, that's what I use it for. I wrote it mostly to pipe through
diff-highlight, though also it was to help diff-so-fancy folks (which I
don't use myself, but they build on diff-highlight). However, it does
seem that they haven't really worked out the problems in the meantime.

-Peff