Re: [PATCH 9/9] add -i: verify in the tests that colors can be overridden
- From
Jeff King <peff@peff.net>
- Date
- Nov 17, 2020, 01:44 UTC
- Message-ID
- <20201117014435.GA19433@coredump.intra.peff.net>
- In-Reply-To
- <nycvar.QRO.7.76.6.2011160027480.18437@tvgsbejvaqbjf.bet>
On Mon, Nov 16, 2020 at 12:35:44AM +0100, Johannes Schindelin wrote:
Show 13 quoted lines
> > 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,?]? nSo 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