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

From: Jeff King <peff@peff.net>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Subject: Re: [PATCH 9/9] add -i: verify in the tests that colors can be overridden
Date: Wed, 11 Nov 2020 13:07:13 -0500	[thread overview]
Message-ID: <20201111180713.GC9902@coredump.intra.peff.net> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.2011111635140.18437@tvgsbejvaqbjf.bet>

On Wed, Nov 11, 2020 at 04:53:13PM +0100, Johannes Schindelin wrote:

> > If we are using dash, then surely BASH_XTRACEFD does not matter either
> > way?
> 
> It kinda does, though. Dash _does_ respect the `BASH_XTRACEFD` variable,
> funnily enough, but does not hand the settings through to sub-shells,
> whereas Bash does.

Really? That's news to me, and doesn't seem to trigger:

  [bash uses it]
  $ bash -xc 'BASH_XTRACEFD=3; echo foo' 3>trace
  + BASH_XTRACEFD=3
  foo
  $ cat trace
  + echo foo

  [dash does not]
  $ dash -xc 'BASH_XTRACEFD=3; echo foo' 3>trace
  + BASH_XTRACEFD=3
  + echo foo
  foo
  $ cat trace

> > Perhaps the subshell is important if we are running bash, though.
> 
> The most important thing, which I of course _failed_ to describe most
> prominently, is that _in general_ `-x` will mess up the `err` file, i.e.
> when running with anything but Bash. Therefoer we need the `grep` instead
> of a `test_cmp`, and that's what I tried to convey in v2.

Yeah. I understood that part (because it has bit me so many times ;) ),
but I agree it's good to make it clear.

> Oh my. I really had tried to avoid going _this_ deep. The `.meta` setting
> is not even read by the interactive add command:
> 
> 	$ git grep -w meta git-add--interactive.perl add-interactive.c \
> 		add-patch.c
> 
> comes up empty.
> [how and why add--interactive.perl reads color config]

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?
Philippe's simple example just did "git add -p". So now I'm doubly
confused.

The answer seems to be that render_hunk() always _replaces_ the colors
we got from running the external diff. Whereas the perl version only
applied coloring when reading back in the results of an edit operation
(and likewise used the frag color when generating a split hunk header).

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.

> For those reasons, v2 brings more changes than I had hoped for. In the
> end, it is a better patch series, obviously. So even if I was reluctant to
> work on all this: thank you for prodding me.

Heh. Sorry and thanks, I guess? :) I'll try to read over v2 carefully.

-Peff