| Message ID | cd1c51005068247fc92f1c515469bcd384bfe589.1661977877.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | built-in add -p: support diff-so-fancy better | expand |
Hi Dscho On 31/08/2022 21:31, Johannes Schindelin via GitGitGadget wrote: > 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. > [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. > + 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
Hi Phillip, On Thu, 1 Sep 2022, Phillip Wood wrote: > On 31/08/2022 21:31, Johannes Schindelin via GitGitGadget wrote: > [...] > > @@ -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. Fixed. > > @@ -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 True. > > 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'); I've changed the code so that the `suppress_colored_line_range` code path prints no extra sequence but the hunk header and the hunk verbatim (they still have to be two separate `strbuf*()` calls because the hunk could be edited). Since this code path now also returns early, the patch avoids the indentation change altogether. > } > > > > 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. I changed it to `^XX$` as you suggested. Thank you for your excellent review, Dscho > > Best Wishes > > Phillip > > > +' > > + > > test_expect_success 'handle very large filtered diff' ' > > git reset --hard && > > # The specific number here is not important, but it must > >
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))) + 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, - header->colored_extra_start; } - 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); 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 +' + test_expect_success 'handle very large filtered diff' ' git reset --hard && # The specific number here is not important, but it must