git@vger.kernel.org list mirror (unofficial, 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: Mon, 16 Nov 2020 00:35:44 +0100 (CET)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.2011160027480.18437@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20201112182925.GA701197@coredump.intra.peff.net>

Hi Peff,

On Thu, 12 Nov 2020, Jeff King wrote:

> On Thu, Nov 12, 2020 at 03:04:01PM +0100, Johannes Schindelin wrote:
>
> > > 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.

> > > 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).
> >
> > No, the Perl version also insists on applying `fraginfo_color`, see
> > https://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898
>
> Only when we split. Try this to give different colors between the
> interactive script and diff:
>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index e713fe3d02..862a21ff1f 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -28,8 +28,9 @@
>  my $diff_use_color = $repo->get_colorbool('color.diff');
>  my ($fraginfo_color) =
>  	$diff_use_color ? (
> -		$repo->get_color('color.diff.frag', 'cyan'),
> +		$repo->get_color('color.diff.nonsense', 'yellow'),
>  	) : ();
> +# noop to create split hunk
>  my ($diff_plain_color) =
>  	$diff_use_color ? (
>  		$repo->get_color('color.diff.plain', ''),
>
> Running "git add -p" does not result in yellow hunk headers. But issuing
> a split command does.
>
> The distinction is mostly academic, because diff-tree and the
> interactive patch code should be using the same colors, so the result
> should look the same. It could matter if the diff-filter chooses
> different colors, though then the split headers will not match the
> originals in style. We _could_ run the newly created hunk header
> individually through the diff-filter, though I'm not sure how various
> filters would handle that.
>
> That's true of the perl version as well as the builtin one, but I think
> the builtin one's insistence on parsing the colored output is taking us
> in the wrong direction to eventually fix that.

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

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

  reply	other threads:[~2020-11-16 11: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
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 [this message]
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.2011160027480.18437@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=peff@peff.net \
    --subject='Re: [PATCH 9/9] add -i: verify in the tests that colors can be overridden' \
    /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

Code repositories for project(s) associated with this 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).