Re: [PATCH v4 2/3] add -p: gracefully handle unparseable hunk headers in colored diffs
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Sep 1, 2022, 13:53 UTC
- Message-ID
- <19d4c44b-868e-628d-5715-54f37ef56c0d@gmail.com>
- In-Reply-To
- <cd1c51005068247fc92f1c515469bcd384bfe589.1661977877.git.gitgitgadget@gmail.com>
Hi Dscho
On 31/08/2022 21:31, Johannes Schindelin via GitGitGadget wrote:
Show 17 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de> > > In > https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com, > Phillipe Blain reported that the built-in `git add -p` command fails > when asked to use [`diff-so-fancy`][diff-so-fancy] to colorize the diff. > > The reason is that this tool produces colored diffs with a hunk header > that does not contain any parseable `@@ ... @@` line range information, > and therefore we cannot detect any part in that header that comes after > the line range. > > As proposed by Phillip Wood, let's take that for a clear indicator that > we should show the hunk headers verbatim. This is what the Perl version > of the interactive `add` command did, too. > > This commit is best viewed with `--color-moved --ignore-space-change`.
or --color-moved-ws=allow-indentation-change
This looks pretty good, unsurprisingly I like the fact that the output now matches the perl version. I was confused by the grep in the test which lead me to realize we're printing an color escape code when we shouldn't.
Show 34 quoted lines
> [diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy
>
> Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> add-patch.c | 42 ++++++++++++++++++++------------------
> t/t3701-add-interactive.sh | 10 +++++++++
> 2 files changed, 32 insertions(+), 20 deletions(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index 34f3807ff32..3699ca1fcaf 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -238,6 +238,7 @@ struct hunk_header {
> * include the newline.
> */
> size_t extra_start, extra_end, colored_extra_start, colored_extra_end;
> + unsigned suppress_colored_line_range:1;
> };
>
> struct hunk {
> @@ -358,15 +359,14 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)
> if (!eol)
> eol = s->colored.buf + s->colored.len;
> p = memmem(line, eol - line, "@@ -", 4);
> - if (!p)
> - return error(_("could not parse colored hunk header '%.*s'"),
> - (int)(eol - line), line);
> - p = memmem(p + 4, eol - p - 4, " @@", 3);
> - if (!p)
> - return error(_("could not parse colored hunk header '%.*s'"),
> - (int)(eol - line), line);
> + if (p && (p = memmem(p + 4, eol - p - 4, " @@", 3)))nit: there should be braces round both arms of the if, but it's hardly the first one that does not follow our official style.
Show 12 quoted lines
> + header->colored_extra_start = p + 3 - s->colored.buf;
> + else {
> + /* could not parse colored hunk header, leave as-is */
> + header->colored_extra_start = hunk->colored_start;
> + header->suppress_colored_line_range = 1;
> + }
> hunk->colored_start = eol - s->colored.buf + (*eol == '\n');
> - header->colored_extra_start = p + 3 - s->colored.buf;
> header->colored_extra_end = hunk->colored_start;
>
> return 0;
> @@ -666,18 +666,20 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk, if (!colored) {
p = s->plain.buf + header->extra_start;
len = header->extra_end - header->extra_start;
} else {
strbuf_addstr(out, s->s.fraginfo_color);I don't think we want to add this escape sequence unless we're going to dynamically generate the hunk header
p = s->colored.buf + header->colored_extra_start;
len = header->colored_extra_end
- header->colored_extra_start;If we cannot parse the hunk header then len is non-zero...
}
Show 30 quoted lines
>
> - if (s->mode->is_reverse)
> - old_offset -= delta;
> - else
> - new_offset += delta;
> -
> - strbuf_addf(out, "@@ -%lu", old_offset);
> - if (header->old_count != 1)
> - strbuf_addf(out, ",%lu", header->old_count);
> - strbuf_addf(out, " +%lu", new_offset);
> - if (header->new_count != 1)
> - strbuf_addf(out, ",%lu", header->new_count);
> - strbuf_addstr(out, " @@");
> + if (!colored || !header->suppress_colored_line_range) {
> + if (s->mode->is_reverse)
> + old_offset -= delta;
> + else
> + new_offset += delta;
> +
> + strbuf_addf(out, "@@ -%lu", old_offset);
> + if (header->old_count != 1)
> + strbuf_addf(out, ",%lu", header->old_count);
> + strbuf_addf(out, " +%lu", new_offset);
> + if (header->new_count != 1)
> + strbuf_addf(out, ",%lu", header->new_count);
> + strbuf_addstr(out, " @@");
> + }
>
> if (len)
> strbuf_add(out, p, len);... and so we print the filtered hunk header here and do not reset the color below. That is probably ok as the filter should be resetting it's own colors but we shouldn't be printing the color at the beginning of the line above in that case.
else if (colored)
strbuf_addf(out, "%s\n", s->s.reset_color);
else
strbuf_addch(out, '\n');
}Show 16 quoted lines
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > index 8a594700f7b..47ed6698943 100755 > --- a/t/t3701-add-interactive.sh > +++ b/t/t3701-add-interactive.sh > @@ -767,6 +767,16 @@ test_expect_success 'detect bogus diffFilter output' ' > grep "mismatched output" output > ' > > +test_expect_success 'handle iffy colored hunk headers' ' > + git reset --hard && > + > + echo content >test && > + printf n >n && > + force_color git -c interactive.diffFilter="sed s/.*@@.*/XX/" \ > + add -p >output 2>&1 <n && > + grep "^[^@]*XX[^@]*$" output
I was wondering why this wasn't just `grep "^XX$"` as we should be printing the output of the diff filter verbatim. That lead to my comments about outputting escape codes above. Apart from that this patch looks good.
Best Wishes
Phillip
Show 5 quoted lines
> +' > + > test_expect_success 'handle very large filtered diff' ' > git reset --hard && > # The specific number here is not important, but it must