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

Commit Message

Johannes Schindelin Aug. 24, 2022, 9:21 p.m. UTC

From: Johannes Schindelin <[e-mail]>

The `diff-so-fancy` diff colorizer produces hunk headers that look
nothing like the built-in `add -p` expects: there is no `@@ ... @@` line
range, and therefore the parser cannot determine where any extra
information starts, such as the function name that is often added to
those hunk header lines.

However, we can do better than simply swallowing the unparseable hunk
header. In the `diff-so-fancy` case, it shows something like `@ file:1
@`. Let's just show the complete hunk header because it probably offers
useful information.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 add-patch.c                | 8 +++++++-
 t/t3701-add-interactive.sh | 3 ++-
 2 files changed, 9 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Aug. 29, 2022, 8:06 a.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
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.

Johannes Schindelin Aug. 29, 2022, 1:32 p.m. UTC | #2

Hi Junio,

On Mon, 29 Aug 2022, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <[e-mail]>
> 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.

Correct.

> Everything else in this patch is about adding an extra space
> depending on what is in the "funcname" part.

... because that logic was not relevant before this commit.

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

I've opted to split the changes out into their own patch because it
improves the reviewability of the patch series.

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

Let me add a review of my own: This hunk is incorrect ;-)

Here is why: in the _regular_ case, i.e. when we have a colored hunk
header like `@@ -936 +936 @@ wow()`, we manage to parse the line range,
and then record the offset of the extra part that starts afterwards.

This extra part is non-empty, therefore we add an extra space.

But that part already starts with a space, so now we end up with two
spaces.

A fix for this will be part of the next iteration.

Ciao,
Dscho

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

Junio C Hamano Aug. 29, 2022, 5:19 p.m. UTC | #3

Johannes Schindelin <[e-mail]> writes:

> Here is why: in the _regular_ case, i.e. when we have a colored hunk
> header like `@@ -936 +936 @@ wow()`, we manage to parse the line range,
> and then record the offset of the extra part that starts afterwards.
>
> This extra part is non-empty, therefore we add an extra space.
>
> But that part already starts with a space, so now we end up with two
> spaces.

In other words, this breaks because the original code depended on
having the extra whitespace before the "funcname" part.

Stepping back a bit, if the final goal for the UI generation out of
this string is to append the material with a whitespace before it
for better visual separation, then the original should probably have
(at least conceptually) lstrip'ed the leading whitespaces from the
string it found after " @@" and then appended the result to where it
is showing, with its own single whitespace as a prefix.  It would
have prevented this bug from happening by future-proofing, and made
the final output nicer, as any excess whitespaces between the " @@"
and "funcname" would have been turned into a single SP.

The "prepend one iff it does not already begin with a whitespace" is
a (at least mentally to the developer) less expensive approach that
is fine, too.

Thanks.

Johannes Schindelin Aug. 30, 2022, 2:14 p.m. UTC | #4

Hi Junio,

On Mon, 29 Aug 2022, Junio C Hamano wrote:

> Johannes Schindelin <[e-mail]> writes:
>
> > Here is why: in the _regular_ case, i.e. when we have a colored hunk
> > header like `@@ -936 +936 @@ wow()`, we manage to parse the line range,
> > and then record the offset of the extra part that starts afterwards.
> >
> > This extra part is non-empty, therefore we add an extra space.
> >
> > But that part already starts with a space, so now we end up with two
> > spaces.
>
> In other words, this breaks because the original code depended on
> having the extra whitespace before the "funcname" part.

Yes.

> Stepping back a bit, if the final goal for the UI generation out of
> this string is to append the material with a whitespace before it
> for better visual separation, then the original should probably have
> (at least conceptually) lstrip'ed the leading whitespaces from the
> string it found after " @@" and then appended the result to where it
> is showing, with its own single whitespace as a prefix.

Yes, with one twist: ANSI escape sequences can make lstrip'ing non-trivial
(granted, the line range parser totally ignores the fact that `@@<RESET> `
is a totally legitimate end of a colored line range).

> It would have prevented this bug from happening by future-proofing, and
> made the final output nicer, as any excess whitespaces between the " @@"
> and "funcname" would have been turned into a single SP.
>
> The "prepend one iff it does not already begin with a whitespace" is
> a (at least mentally to the developer) less expensive approach that
> is fine, too.

Yes, it is definitely less expensive.

Ciao,
Dscho

Patch

diff --git a/add-patch.c b/add-patch.c
index f2fffe1af02..1f3f3611ee9 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -8,6 +8,7 @@ 
 #include "diff.h"
 #include "compat/terminal.h"
 #include "prompt.h"
+#include "utf8.h"
 
 enum prompt_mode_type {
 	PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_ADDITION, PROMPT_HUNK,
@@ -363,7 +364,7 @@  static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)
 		header->colored_extra_start = p + 3 - s->colored.buf;
 	else
 		/* could not parse colored hunk header, showing nothing */
-		header->colored_extra_start = hunk->colored_start;
+		header->colored_extra_start = line - s->colored.buf;
 	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
 '
 
 test_expect_success 'diff.algorithm is passed to `git diff-files`' '