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,
	Philippe Blain <levraiphilippeblain@gmail.com>
Subject: Re: [PATCH v2 00/11] Fix color handling in git add -i
Date: Thu, 12 Nov 2020 15:20:38 +0100 (CET)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.2011121504110.18437@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20201111184527.GD9902@coredump.intra.peff.net>

[-- Attachment #1: Type: text/plain, Size: 5622 bytes --]

Hi Peff,

On Wed, 11 Nov 2020, Jeff King wrote:

> On Wed, Nov 11, 2020 at 12:28:13PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > Changes since v1:
> >
> >  * The regression test now actually exercises the re-coloring (that is the
> >    primary purpose of git add -p looking at the color.diff.* variables).
> >  * The way the built-in git add -p renders hunk headers of split hunks was
> >    aligned with how the Perl version does things.
> >  * We now consistently prefer color.diff.context over color.diff.plain, no
> >    matter whether using the Perl or the built-in version of git add -p.
> >  * The commit message for the regression test no longer confuses readers by
> >    mentioning dash.
> >  * The regression test was structured a bit better, first testing error
> >    message coloring, then the menu in git add -i and then the diff coloring
> >    in git add -p.
>
> The changes were less scary than I was led to believe after reading your
> earlier response. :)
>
> Everything here looks sensible. As I said elsewhere, I do worry there
> may be a lingering issue with how parse_diff() looks at the filtered
> diffs. Let me see if I can get diff-so-fancy working...
>
> Aha, yes. I think using diff-so-fancy here isn't entirely robust,
> because it has some cases where it fails at the 1-to-1 line
> correspondence, but they're aware of the issue. But it does work in
> simples cases now (there's some coloring which makes the output more
> meaningful, but I obviously won't paste it here):
>
>   $ git -c interactive.difffilter='diff-so-fancy' add -p
>   ──────────────────────────────────────────────────────────────────────
>   modified: file
>   ──────────────────────────────────────────────────────────────────────
>   @ file:1 @
>   old
>   new
>   (1/1) Stage this hunk [y,n,q,a,d,e,?]?

It might _seem_ that it works. But try to split a hunk. I did this with
the test case (interrupting it just before running `add -p`):

-- snip --
$ git -C ./t/trash\ directory.t3701-add-interactive/ -c
interactive.difffilter='diff-so-fancy' -c add.interactive.usebuiltin=false add -p
<BOLD>────────────────────────────────────────────────────────────────────────────────</BOLD>
<BOLD>modified: color-test</BOLD>
<BOLD>────────────────────────────────────────────────────────────────────────────────</BOLD>
<MAGENTA>@ color-test:1 @</MAGENTA>
context
<RED>old</RED>
<GREEN>new</GREEN>
more-context
<GREEN>another-one</GREEN>
<BLUE>(1/1) Stage this hunk [y,n,q,a,d,s,e,?]?</BLUE> s
<BOLD>Split into 2 hunks.</BOLD>
<MAGENTA>@@ -1,3 +1,3 @@</MAGENTA>
<RED>old</RED>
<GREEN>new</GREEN>
more-context
<GREEN>another-one</GREEN>
<BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]?</BLUE>
-- snap --

Obviously, I marked it up so you can see what parts were colored.

See how it first _looks_ as if it works? But then you split the hunk, but
instead of showing only the old/new part, it shows the old/new/another-one
part!

In other words, it does not work at all, and the fact that it does not
even warn you about it is misleading, and that's pretty much all I will
say about it.

> But with the builtin:
>
>   $ git -c interactive.difffilter='diff-so-fancy' \
>         -c add.interactive.usebuiltin=true \
> 	add -p
>   error: could not parse colored hunk header '?[1m?[38;5;1m?[31mold?[m'

Granted, this is not quite helpful. I _think_ what is happening is that
the number of lines is different, and `add -p` goes like: noooope! But it
could probably be better at describing what the issue is. Or it could
cater to `diff-so-fancy`, if that is what people use these days, and
special-case it by falling back to detecting the hunk boundaries in a
manner that is compatible with `diff-so-fancy`.

Or we might be able to come up with a strategy that is not so limited and
that works for more than just `diff-so-fancy`.

> I don't use it myself, and as I said, I think they have some fixes to
> make for it to be robust as a diff-filter. But I suspect this may be a
> problem once the builtin version becomes the default.
>
> I don't think it should be dealt with in this patch series, though.

Oh, _that_ I wholeheartedly agree with.

> While it may touch on some of the coloring code, it's otherwise
> orthogonal to the fixes here. And while the fix is likely to be to make
> render_hunk() stop re-coloring in the non-edit cases, your more-robust
> test here will still be checking what you expect (because it really is
> triggering the edit case, not relying on the extra coloring to see the
> effect of in-process color config).

I don't actually think that we _can_ stop re-coloring the hunk header in
the general case because we need to keep this working even for split
hunks. It would be a very bad idea to make it work for non-split cases and
then something like `die()` only when the hunk is split. Better re-color
all of them, then, to not even risk that situation.

Ciao,
Dscho

  reply	other threads:[~2020-11-12 14:21 UTC|newest]

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-10 23:42 [PATCH 0/9] " 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
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 [this message]
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.2011121504110.18437@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=levraiphilippeblain@gmail.com \
    --cc=peff@peff.net \
    --subject='Re: [PATCH v2 00/11] Fix color handling in git add -i' \
    /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).