Re: [PATCH v4 2/3] add -p: gracefully handle unparseable hunk headers in colored diffs

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sep 1, 2022, 15:09 UTC
Message-ID: <1q42767p-7oqo-np69-0099-2r5784q5q140@tzk.qr>
In-Reply-To: <19d4c44b-868e-628d-5715-54f37ef56c0d@gmail.com>

Hi Phillip,

On Thu, 1 Sep 2022, Phillip Wood wrote:

Show 18 quoted lines

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

Show 10 quoted lines

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

Show 47 quoted lines

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

Show 23 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.

I changed it to `^XX$` as you suggested.

Thank you for your excellent review, Dscho

Show 12 quoted lines

>
> Best Wishes
>
> Phillip
>
> > +'
> > +
> >   test_expect_success 'handle very large filtered diff' '
> >    git reset --hard &&
> >    # The specific number here is not important, but it must
>
>