git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Jeff King <peff@peff.net>
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 16:53:13 +0100 (CET)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.2011111635140.18437@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20201111023549.GB806755@coredump.intra.peff.net>

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

> 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

  reply	other threads:[~2020-11-11 15:53 UTC|newest]

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-10 23:42 [PATCH 0/9] Fix color handling in git add -i Johannes Schindelin via GitGitGadget
2020-11-10 23:42 ` [PATCH 1/9] add -i (built-in): do show an error message for incorrect inputs Johannes Schindelin via GitGitGadget
2020-11-10 23:42 ` [PATCH 2/9] add -i (built-in): send error messages to stderr Johannes Schindelin via GitGitGadget
2020-11-10 23:42 ` [PATCH 3/9] add -i: use `reset_color` consistently Johannes Schindelin via GitGitGadget
2020-11-10 23:42 ` [PATCH 4/9] add -i (built-in): prevent the `reset` "color" from being configured Johannes Schindelin via GitGitGadget
2020-11-10 23:42 ` [PATCH 5/9] add -i (built-in): use correct names to load color.diff.* config Johannes Schindelin via GitGitGadget
2020-11-10 23:42 ` [PATCH 6/9] add -p (built-in): do not color the progress indicator separately Johannes Schindelin via GitGitGadget
2020-11-10 23:42 ` [PATCH 7/9] add -i (built-in): use the same indentation as the Perl version Johannes Schindelin via GitGitGadget
2020-11-10 23:42 ` [PATCH 8/9] add -i (Perl version): include indentation in the colored header Johannes Schindelin via GitGitGadget
2020-11-10 23:42 ` [PATCH 9/9] add -i: verify in the tests that colors can be overridden Johannes Schindelin via GitGitGadget
2020-11-11  2:35   ` Jeff King
2020-11-11 15:53     ` Johannes Schindelin [this message]
2020-11-11 18:07       ` Jeff King
2020-11-12 14:04         ` Johannes Schindelin
2020-11-12 18:29           ` Jeff King
2020-11-15 23:35             ` Johannes Schindelin
2020-11-17  1:44               ` Jeff King
2020-11-11  2:36 ` [PATCH 0/9] Fix color handling in git add -i Jeff King
2020-11-11 12:28 ` [PATCH v2 00/11] " Johannes Schindelin via GitGitGadget
2020-11-11 12:28   ` [PATCH v2 01/11] add -i (built-in): do show an error message for incorrect inputs Johannes Schindelin via GitGitGadget
2020-11-11 12:28   ` [PATCH v2 02/11] add -i (built-in): send error messages to stderr Johannes Schindelin via GitGitGadget
2020-11-11 12:28   ` [PATCH v2 03/11] add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headers Johannes Schindelin via GitGitGadget
2020-11-11 12:28   ` [PATCH v2 04/11] add -i: use `reset_color` consistently Johannes Schindelin via GitGitGadget
2020-11-13 12:03     ` Phillip Wood
2020-11-13 13:53       ` Johannes Schindelin
2020-11-13 16:04         ` Phillip Wood
2020-11-15 23:47           ` Johannes Schindelin
2020-11-11 12:28   ` [PATCH v2 05/11] add -i (built-in): prevent the `reset` "color" from being configured Johannes Schindelin via GitGitGadget
2020-11-11 12:28   ` [PATCH v2 06/11] add -i (built-in): use correct names to load color.diff.* config Johannes Schindelin via GitGitGadget
2020-11-11 12:28   ` [PATCH v2 07/11] add -p (built-in): do not color the progress indicator separately Johannes Schindelin via GitGitGadget
2020-11-13 12:07     ` Phillip Wood
2020-11-13 13:57       ` Johannes Schindelin
2020-11-13 16:06         ` Phillip Wood
2020-11-15 23:55           ` Johannes Schindelin
2020-11-11 12:28   ` [PATCH v2 08/11] add -i (built-in): use the same indentation as the Perl version Johannes Schindelin via GitGitGadget
2020-11-11 12:28   ` [PATCH v2 09/11] add -i (Perl version): include indentation in the colored header Johannes Schindelin via GitGitGadget
2020-11-13 12:11     ` Phillip Wood
2020-11-13 13:58       ` Johannes Schindelin
2020-11-13 16:12         ` Phillip Wood
2020-11-15 23:51           ` Johannes Schindelin
2020-11-11 12:28   ` [PATCH v2 10/11] add -p: prefer color.diff.context over color.diff.plain Johannes Schindelin via GitGitGadget
2020-11-13 14:04     ` Philippe Blain
2020-11-15 23:57       ` Johannes Schindelin
2020-11-13 14:08     ` Phillip Wood
2020-11-16  0:02       ` Johannes Schindelin
2020-11-11 12:28   ` [PATCH v2 11/11] add -i: verify in the tests that colors can be overridden Johannes Schindelin via GitGitGadget
2020-11-11 18:45   ` [PATCH v2 00/11] Fix color handling in git add -i Jeff King
2020-11-12 14:20     ` Johannes Schindelin
2020-11-12 18:40       ` Jeff King
2020-11-15 23:40         ` Johannes Schindelin
2020-11-17  1:49           ` Jeff King
2020-11-16 16:08   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 01/11] add -i (built-in): do show an error message for incorrect inputs Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 02/11] add -i (built-in): send error messages to stderr Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 03/11] add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headers Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 04/11] add -i: use `reset_color` consistently Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 05/11] add -i (built-in): prevent the `reset` "color" from being configured Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 06/11] add -i (built-in): use correct names to load color.diff.* config Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 07/11] add -p (built-in): do not color the progress indicator separately Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 08/11] add -i (built-in): use the same indentation as the Perl version Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 09/11] add -i (Perl version): color header to match the C version Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 10/11] add -p: prefer color.diff.context over color.diff.plain Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 11/11] add -i: verify in the tests that colors can be overridden Johannes Schindelin via GitGitGadget
2020-11-17  1:51     ` [PATCH v3 00/11] Fix color handling in git add -i Jeff King
2020-11-17  2:05       ` Jeff King
2020-11-25 21:59       ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=nycvar.QRO.7.76.6.2011111635140.18437@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).