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