Re: [PATCH v2 3/4] add -p: handle `diff-so-fancy`'s hunk headers better

[Junio C Hamano <gitster@pobox.com>]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

>  	else
>  		/* could not parse colored hunk header, showing nothing */
> -		header->colored_extra_start = hunk->colored_start;
> +		header->colored_extra_start = line - s->colored.buf;

This is the only thing that corresponds to the proposed log message.
The comment that says "showing nothing" is no longer correct, and
needs to be updated.

Everything else in this patch is about adding an extra space
depending on what is in the "funcname" part.  The code does not know
or care if it is an attempt to do diff-so-fancy's headers better by
doing something we didn't do to the normal header we managed to have
parsed; rather the extra space thing is done unconditionally and
does not know or care if extra_start is truly after " @@" or a place
in the line the new code computed.

So the following three hunks either need to be made into a separate
patch, or deserves to be explained in a new paragraph in the
proposed log message.

>  	header->colored_extra_end = hunk->colored_start;
>  
>  	return 0;
> @@ -649,6 +650,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
>  		size_t len;
>  		unsigned long old_offset = header->old_offset;
>  		unsigned long new_offset = header->new_offset;
> +		int needs_extra_space = 0;
>  
>  		if (!colored) {
>  			p = s->plain.buf + header->extra_start;
> @@ -658,6 +660,8 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
>  			p = s->colored.buf + header->colored_extra_start;
>  			len = header->colored_extra_end
>  				- header->colored_extra_start;
> +			if (utf8_strnwidth(p, len, 1 /* skip ANSI */) > 0)
> +				needs_extra_space = 1;
>  		}
>  
>  		if (s->mode->is_reverse)
> @@ -673,6 +677,8 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
>  			strbuf_addf(out, ",%lu", header->new_count);
>  		strbuf_addstr(out, " @@");
>  
> +		if (needs_extra_space)
> +			strbuf_addch(out, ' ');
>  		if (len)
>  			strbuf_add(out, p, len);
>  		else if (colored)
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 7e3c1de71f5..9deb7a87f1e 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -772,7 +772,8 @@ test_expect_success 'gracefully fail to parse colored hunk header' '
>  	echo content >test &&
>  	test_config interactive.diffFilter "sed s/@@/XX/g" &&
>  	printf y >y &&
> -	force_color git add -p <y
> +	force_color git add -p >output 2>&1 <y &&
> +	grep XX output
>  '

It is good to make sure that XX that was munged appears in the
output.  This however does not check anything about the
needs-extra-space logic.  It should.  If the extra-space logic is
moved to a separate patch, then this step can have the above test,
but then the separate patch needs to update it to check the
additional logic.

Other than that, looking very good.