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

Hi Peff,

On Thu, 12 Nov 2020, Jeff King wrote:

On Thu, Nov 12, 2020 at 03:04:01PM +0100, Johannes Schindelin wrote:

quoted

quoted

Hmm. Right, I knew about that weirdness. But I assumed that the builtin
add-interactive was doing the diffs in-core. Otherwise, why would we
have seen the failure to load diff.color.frag in the first place?

Oh, that's easy to explain: as you can verify reading
https://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898
the Perl version of `git add -p` insists on (re-)constructing the hunk
headers manually, and obviously it needs to color them manually, too. And
https://github.com/git/git/blob/e31aba42fb12/add-patch.c#L649-L672 shows
that the built-in version of `git add -p` slavishly follows that practice.

But that is only when we split hunks (your link to the perl script is in
split_hunks()). I agree we must color manually there when creating our
own hunk header. But outside of that and patch-editing, the perl script
does not otherwise recolor or rewrite (nor even really parse beyond
line-splitting) what it gets from the colorized version.

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

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.

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.

Anyway. I stand by what I said above: I do not want to spend any further
time on this tangent, at least not right now. There are more pressing
challenges waiting for me, and I expect those other challenge to have a
much bigger "return on investment".

Ciao,
Dscho

quoted

quoted

I'm not sure that what the builtin version is doing is wrong, but it
seems like it's putting a lot of extra effort into parsing colors off of
the colorized version. Whereas the perl version just assumes the lines
match up. I do wonder if there are corner cases we might hit around
filters here, though. The lines we get from a filter might bear no
resemblance at all to diff lines. The only thing we require in the perl
version is that they have correspond 1-to-1 with the unfiltered diff
lines in meaning.

They do have to correspond 1-to-1 because the assumption is that the
individual lines will then correspond one-to-one, too. This does not need
to be true, of course, but then the filter is probably less useful than
the user wants it to be.

Right, I'm not disputing the 1-to-1 thing (I was after all the one who
implemented interactive.diffilter, and added the "complain if the counts
don't line up" check). But in the perl script they only need to
correspond _semantically_, not syntactically.

-Peff