Hi Junio,

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

"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:

quoted

 	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.

quoted

 	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

quoted

 		}

 		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.