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

[Phillip Wood <phillip.wood123@gmail.com>]

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