Hi Dscho
On 31/08/2022 21:31, Johannes Schindelin via GitGitGadget wrote:
From: Johannes Schindelin <johannes.schindelin@xxxxxx>
In
https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@xxxxxxxxx,
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.
[diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy
Reported-by: Philippe Blain <levraiphilippeblain@xxxxxxxxx>
Helped-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
---
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.
+ 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...
}
- 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');
}
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
+'
+
test_expect_success 'handle very large filtered diff' '
git reset --hard &&
# The specific number here is not important, but it must
