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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Peff,

On Tue, 10 Nov 2020, Jeff King wrote:

> On Tue, Nov 10, 2020 at 11:42:19PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@xxxxxx>
> >
> > 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.

> 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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux