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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 11, 2020, 15:53 UTC
Message-ID: <nycvar.QRO.7.76.6.2011111635140.18437@tvgsbejvaqbjf.bet>
In-Reply-To: <20201111023549.GB806755@coredump.intra.peff.net>

Hi Peff,

On Tue, 10 Nov 2020, Jeff King wrote:

Show 16 quoted lines

> On Tue, Nov 10, 2020 at 11:42:19PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > Now that the Perl version produces the same output as the built-in
> > version (mostly fixing bugs in the latter), let's add a regression test
> > to verify that it stays this way.
> >
> > 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
> > 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`).
>
> Your reasoning here confuses me.

Sorry!

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

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

Show 41 quoted lines

> Either way, being forgiving to the use of "-x" is a nice convenience and
> I approve.
>
> > +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" &&
>
> Since you are being so thorough, and since it is already in the output,
> maybe color color.diff.meta, too? Like:
>
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 28549a41a2..cca866c8f4 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -594,6 +594,7 @@ test_expect_success 'colors can be overridden' '
>  	test_config color.interactive.help green &&
>  	test_config color.interactive.prompt yellow &&
>  	test_config color.interactive.error blue &&
> +	test_config color.diff.meta italic &&
>  	test_config color.diff.frag magenta &&
>  	test_config color.diff.context cyan &&
>  	test_config color.diff.old bold &&
> @@ -625,9 +626,9 @@ test_expect_success 'colors can be overridden' '
>  	  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>
> +	<YELLOW>Patch update<RESET>>> <ITALIC>diff --git a/color-test b/color-test<RESET>
> +	<ITALIC>--- a/color-test<RESET>
> +	<ITALIC>+++ b/color-test<RESET>
>  	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
>  	<CYAN> context<RESET>
>  	<BOLD>-old<RESET>

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.

The reason is that we actually let the diff machinery do all the coloring. So why, then, you might ask, do we bother to read those values in the first place?

Well, you know, in `git add -p`, you can split hunks. That's still fine, we basically just have to color the hunk header that we insert, and can reuse the original coloring of the remaining lines, because they are unchanged.

But. But you can also `edit` hunks.

And after that `edit` step, those hunks have to be re-colored. And _that_ is why `git-add--interactive` bothers to read those `color.diff.*` values to begin with.

Now, how do you see this re-coloring in action?

I am almost glad that you asked. By simulating that `edit` in the regression test. But that is not enough because the hunk is staged immediately after editing. But you _can_ go back to that hunk, even if it already has been changed, and make up your mind to _not_ stage it, but that only works if `git add -p` hasn't quit already, which it would do with a single hunk, so we have to have _two_ hunks.

This not only complicates the regression test, but _of course_ (because _why would I already be done with this patch series???_) it shows further differences between the Perl version and the built-in version. It's almost as if `git add -i` said "Johannes, you think those 500 hours were all you would spend on converting me to a built-in? Think again!".

One of the issues I found (and fixed in v2) was that the built-in version tried to be practical and _not_ special-case a count of one in the hunk header (`@@ -3 +3 @@` is equivalent to `@@ -3,1 +3,1 @@`). Well, now it does, just like libxdiff and the Perl version.

The other issue was that the Perl version still used `color.diff.plain` instead of `color.diff.context`. I hope I found a good-enough solution to the problem that we simply cannot call `git config --get-color` or `repo_get_config()` with _two_ keys to look for (and the last one wins).

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.

Ciao, Dscho