built-in add -p: support diff-so-fancy better

Philippe Blain reported in
https://lore.kernel.org/git/[email protected]
that there is a problem when running the built-in version of git add -p with diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer. The symptom is this:

    error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'

This patch series addresses that and should fix
https://github.com/so-fancy/diff-so-fancy/issues/437

Le 2022-08-23 à 14:04, Johannes Schindelin via GitGitGadget a écrit :

Quoted 9 lines

Philippe Blain reported in
https://lore.kernel.org/git/[email protected]
that there is a problem when running the built-in version of git add -p with diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer. The symptom is this:

    error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'

Quoted 2 lines

This patch series addresses that and should fix
https://github.com/so-fancy/diff-so-fancy/issues/437

Thanks for the quick reaction!
I've verified that the patches fix the example in my reproducer, and they also fix it if using "delta --color-only" as interactive.diffFilter. Delta is another diff colorizer (and more) that's facing the same issue as diff-so-fancy [1].

However, I've tried it on a more "real-life" example, and then I get:

error: mismatched output from interactive.diffFilter
hint: Your filter must maintain a one-to-one correspondence
hint: between its input and output lines.

This is despite using "diff-so-fancy --patch" as interactive.diffFilter, which should keep the number of lines the same.

This ('--patch') was added in [2], about a month after Peff wrote the message you mention in https://lore.kernel.org/git/[email protected]/.

Again, when using the Perl version with this new example, it works correctly. I'll try to come up with a new reproducer for this... But this new example does work with delta with the builtin version, so it might be diff-so-fancy that's the culprit...

Cheers,

Philippe.

[1] https://github.com/dandavison/delta/issues/1114
[2] https://github.com/so-fancy/diff-so-fancy/commit/13d3f8949e15dd62f6b49bc652fe94af6a9bfc79

Hi Philippe,

Quoted 6 lines · Philippe Blain

However, I've tried it on a more "real-life" example, and then I get:

error: mismatched output from interactive.diffFilter
hint: Your filter must maintain a one-to-one correspondence
hint: between its input and output lines.

Quoted 2 lines

This is despite using "diff-so-fancy --patch" as interactive.diffFilter, which should keep the number of lines the same.

Would you mind sharing the example with me?

Thanks,
Dscho

Hi Dscho,

Le 2022-08-24 à 02:27, Johannes Schindelin a écrit :

Quoted 10 lines

Hi Philippe,

Quoted 6 lines · Philippe Blain

However, I've tried it on a more "real-life" example, and then I get:

error: mismatched output from interactive.diffFilter
hint: Your filter must maintain a one-to-one correspondence
hint: between its input and output lines.

Quoted 7 lines

Quoted 2 lines

This is despite using "diff-so-fancy --patch" as interactive.diffFilter, which should keep the number of lines the same.

Would you mind sharing the example with me?

Thanks,
Dscho

In trying to write a reproducer, I realize that this is in fact a separate "bug". I happened to have a submodule in the repository I was testing, and it had modified content. This is what was causing the "mismatched output" error, although I'm not sure why. If I use a pathspec that does not include the submodule when invoking 'git add -p', it works correctly.

I'm a bit out of time now but I'll try to write a separate reproducer for this later today.

Quoted 20 lines

Quoted 6 lines

Quoted 6 lines

However, I've tried it on a more "real-life" example, and then I get:

error: mismatched output from interactive.diffFilter
hint: Your filter must maintain a one-to-one correspondence
hint: between its input and output lines.

Quoted 7 lines

Quoted 2 lines

This is despite using "diff-so-fancy --patch" as interactive.diffFilter, which should keep the number of lines the same.

Would you mind sharing the example with me?

Thanks,
Dscho

In trying to write a reproducer, I realize that this is in fact a separate "bug". I happened to have a submodule in the repository I was testing, and it had modified content. This is what was causing the "mismatched output" error, although I'm not sure why. If I use a pathspec that does not include the submodule when invoking 'git add -p', it works correctly.

Quoted 2 lines

I'm a bit out of time now but I'll try to write a separate reproducer for this later today.

So in trying to write a new reproducer, I found the cause of the bug. ...

Quoted 17 lines · Philippe Blain

Quoted 17 lines

Quoted 10 lines

Quoted 8 lines

However, I've tried it on a more "real-life" example, and then I get:

error: mismatched output from interactive.diffFilter
hint: Your filter must maintain a one-to-one correspondence
hint: between its input and output lines.

This is despite using "diff-so-fancy --patch" as interactive.diffFilter, which should keep the number of lines the same.

Would you mind sharing the example with me?

In trying to write a reproducer, I realize that this is in fact a separate "bug". I happened to have a submodule in the repository I was testing, and it had modified content. This is what was causing the "mismatched output" error, although I'm not sure why. If I use a pathspec that does not include the submodule when invoking 'git add -p', it works correctly.

Quoted 5 lines

Quoted 2 lines

I'm a bit out of time now but I'll try to write a separate reproducer for this later today.

So in trying to write a new reproducer, I found the cause of the bug. ...

I somehow had an impression that that the C reimplementation was designed to be a bug-for-bug compatible faithful rewrite of the original scripted version,

but looking at the way how this bug is handled, I am not sure (it looks as if the initial fix was to patch the code to match the symptom, not to make the code to behave more faithfully to the scripted version).

As with any big rewrite, a complete re-implementation always has risks to introduce unintended regressions and that is why starting with faithful rewrite with the staged transition plan is valuable.

In this case, the staged transition plan, the step to flip the default without before remove the old one, is working beautifully. And it was only made possible with the actual users reporting issues before they say "the new one is broken, so let's use the knob to retain the old implementation".

One unfortunate consequence of avoiding the git diff invocations of git-add--interactive.perl that are not actually necessary is that I missed that one of those invocations specifically ignores dirty submodules, and that the Perl script then only processes files whose numstat shows up in _that_ diff output.

Fortunately, the fix is easy: simply ignore dirty submodules in _all_ git diff invocations of add -p.

I will submit the next iteration as soon as the PR builds pass.

Quoted 4 lines

As with any big rewrite, a complete re-implementation always has risks to introduce unintended regressions and that is why starting with faithful rewrite with the staged transition plan is valuable.

Quoted 5 lines

In this case, the staged transition plan, the step to flip the default without before remove the old one, is working beautifully. And it was only made possible with the actual users reporting issues before they say "the new one is broken, so let's use the knob to retain the old implementation".

Indeed, and thank you, Philippe, for bringing this to the Git mailing list so that I could do something about the bug.

Philippe Blain reported in
https://lore.kernel.org/git/[email protected]
that there is a problem when running the built-in version of git add -p with diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer. The symptom is this:

    error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'

This patch series addresses that and should fix
https://github.com/so-fancy/diff-so-fancy/issues/437

Changes since v1:

* Added a commit to ignore dirty submodules just like the Perl version does.

Philippe Blain reported in
https://lore.kernel.org/git/[email protected]
that there is a problem when running the built-in version of git add -p with diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer. The symptom is this:

    error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'

This patch series addresses that and should fix
https://github.com/so-fancy/diff-so-fancy/issues/437

Changes since v1:

* Added a commit to ignore dirty submodules just like the Perl version does.

Johannes Schindelin (4):
t3701: redefine what is "bogus" output of a diff filter add -p: gracefully ignore unparseable hunk headers in colored diffs add -p: handle diff-so-fancy's hunk headers better
add -p: ignore dirty submodules

 add-patch.c                | 24 ++++++++++++++----------
 t/t3701-add-interactive.sh | 24 +++++++++++++++++++++++-
 2 files changed, 37 insertions(+), 11 deletions(-)

base-commit: 795ea8776befc95ea2becd8020c7a284677b4161
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1336%2Fdscho%2Fdiff-so-fancy-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1336/dscho/diff-so-fancy-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1336

Range-diff vs v1:

 1:  74ab50eeb1c = 1:  74ab50eeb1c t3701: redefine what is "bogus" output of a diff filter
 2:  b07f85a0359 = 2:  b07f85a0359 add -p: gracefully ignore unparseable hunk headers in colored diffs
 3:  9dac9f74d2e = 3:  9dac9f74d2e add -p: handle `diff-so-fancy`'s hunk headers better
 -:  ----------- > 4:  540ce27c38a add -p: ignore dirty submodules

Le 2022-08-24 à 17:21, Johannes Schindelin via GitGitGadget a écrit :

Quoted 33 lines

Philippe Blain reported in
https://lore.kernel.org/git/[email protected]
that there is a problem when running the built-in version of git add -p with diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer. The symptom is this:

    error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'

This patch series addresses that and should fix
https://github.com/so-fancy/diff-so-fancy/issues/437

Changes since v1:

* Added a commit to ignore dirty submodules just like the Perl version does.

Johannes Schindelin (4):
t3701: redefine what is "bogus" output of a diff filter add -p: gracefully ignore unparseable hunk headers in colored diffs add -p: handle diff-so-fancy's hunk headers better
add -p: ignore dirty submodules

 add-patch.c                | 24 ++++++++++++++----------
 t/t3701-add-interactive.sh | 24 +++++++++++++++++++++++-
 2 files changed, 37 insertions(+), 11 deletions(-)

base-commit: 795ea8776befc95ea2becd8020c7a284677b4161
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1336%2Fdscho%2Fdiff-so-fancy-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1336/dscho/diff-so-fancy-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1336

Quoted 7 lines

Range-diff vs v1:

 1:  74ab50eeb1c = 1:  74ab50eeb1c t3701: redefine what is "bogus" output of a diff filter
 2:  b07f85a0359 = 2:  b07f85a0359 add -p: gracefully ignore unparseable hunk headers in colored diffs
 3:  9dac9f74d2e = 3:  9dac9f74d2e add -p: handle `diff-so-fancy`'s hunk headers better
 -:  ----------- > 4:  540ce27c38a add -p: ignore dirty submodules

Thanks, 4/4 fixes the mismatched output bug. Just after I sent my last email, I asked myself "but why does 'git add -p' presents dirty submodule to the user, as they can't be staged?" :)

A small question about 2/4, any reason why you did not use a "Reported-by:" trailer ? Not that I care that much, but I think using such a trailer is more standard, and makes for easier statistics as it's more parseable :)

Hi Philippe,

Quoted 46 lines · Philippe Blain

Le 2022-08-24 à 17:21, Johannes Schindelin via GitGitGadget a écrit :

Quoted 33 lines

Philippe Blain reported in
https://lore.kernel.org/git/[email protected]
that there is a problem when running the built-in version of git add -p with diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer. The symptom is this:

    error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'

This patch series addresses that and should fix
https://github.com/so-fancy/diff-so-fancy/issues/437

Changes since v1:

* Added a commit to ignore dirty submodules just like the Perl version does.

Johannes Schindelin (4):
t3701: redefine what is "bogus" output of a diff filter add -p: gracefully ignore unparseable hunk headers in colored diffs add -p: handle diff-so-fancy's hunk headers better
add -p: ignore dirty submodules

 add-patch.c                | 24 ++++++++++++++----------
 t/t3701-add-interactive.sh | 24 +++++++++++++++++++++++-
 2 files changed, 37 insertions(+), 11 deletions(-)

base-commit: 795ea8776befc95ea2becd8020c7a284677b4161
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1336%2Fdscho%2Fdiff-so-fancy-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1336/dscho/diff-so-fancy-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1336

Quoted 7 lines

Range-diff vs v1:

 1:  74ab50eeb1c = 1:  74ab50eeb1c t3701: redefine what is "bogus" output of a diff filter
 2:  b07f85a0359 = 2:  b07f85a0359 add -p: gracefully ignore unparseable hunk headers in colored diffs
 3:  9dac9f74d2e = 3:  9dac9f74d2e add -p: handle `diff-so-fancy`'s hunk headers better
 -:  ----------- > 4:  540ce27c38a add -p: ignore dirty submodules

Thanks, 4/4 fixes the mismatched output bug. Just after I sent my last email, I asked myself "but why does 'git add -p' presents dirty submodule to the user, as they can't be staged?" :)

Quoted 3 lines

A small question about 2/4, any reason why you did not use a "Reported-by:" trailer ? Not that I care that much, but I think using such a trailer is more standard, and makes for easier statistics as it's more parseable :)

Good suggestion.

How about adding your review? I'll then add a "Reviewed-by:" trailer, too ;-)

add-patch.c +0 -7

index 509ca04456b..f2fffe1af02 100644
@@ -357,16 +357,13 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)
 	eol = memchr(line, '\n', s->colored.len - hunk->colored_start);
 	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'"),

add-patch.c +5 -0

 	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)

t/t3701-add-interactive.sh +0 -0

index 7e3c1de71f5..9deb7a87f1e 100755

"Johannes Schindelin via GitGitGadget" <[email protected]>

add-patch.c static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk) · line 367

Quoted 4 lines · writes:

add-patch.c +1 -1

 	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.

Quoted 9 lines

add-patch.c +1 -0

 	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;
 

add-patch.c static void render_hunk(struct add_p_state *s, struct hunk *hunk, · line 664

Quoted 8 lines

add-patch.c +2 -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;

Quoted 17 lines

add-patch.c +2 -0

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

t/t3701-add-interactive.sh +0 -0

index 7e3c1de71f5..9deb7a87f1e 100755

t/t3701-add-interactive.sh test_expect_success 'gracefully fail to parse colored hunk header' ' · line 777

Quoted 7 lines

t/t3701-add-interactive.sh +2 -1

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

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.

* That logic was now corrected to see whether that space is really needed.
* To verify that the logic does what we need it to do, the added regression test now specifically tests for that (single) extra space that we want to be inserted.

* Rebased to the current tip of the main branch to avoid a merge conflict with 716c1f649e3 (pipe_command(): mark stdin descriptor as non-blocking, 2022-08-17).

From: Johannes Schindelin <[email protected]>

When parsing the colored version of a diff, the interactive add command really relies on the colored version having the same number of lines as the non-colored version. That is an invariant.

However, in the 'detect bogus diffFilter output' test case in t3701, we essentially required a hunk header that contains parseable @@ ... @@ hunk headers, and called all colored diffs without such hunks bogus.

The reason for this is that we would like to show the users the adjusted hunk headers _including_ the extra part after the @@ ... @@ information, which usually contains things like the function name or soms such.

Now, there is a _very_ popular diff colorizer called diff-so-fancy that does not produce such colored diffs as the built-in add command expects. Nevertheless, the Perl variant of the add command handles those nicely, essentially by ignoring the hunk header and saying "there is nothing else we can show except the original hunk header, even if we had to adjust the line range and the original hunk header might get that wrong".

In preparation for teaching the built-in interactive add to be a bit more lenient, let's change the 'detect bogus diffFilter output' test case so that it verifies that a mismatched number of lines causes the command to error out, but not an unparseable hunk header.

Signed-off-by Johannes Schindelin <[email protected]>

---
  t/t3701-add-interactive.sh | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

t/t3701-add-interactive.sh +1 -1

index 3b7df9bed5a..88d8133f38f 100755
@@ -761,7 +761,7 @@ test_expect_success 'detect bogus diffFilter output' '
  	git reset --hard &&
 
  	echo content >test &&
-	test_config interactive.diffFilter "sed 1d" &&
+	test_config interactive.diffFilter "sed q" &&
  	printf y >y &&
  	force_color test_must_fail git add -p <y
  '

From: Johannes Schindelin <[email protected]>

The diff-so-fancy diff colorizer produces hunk headers that look nothing like what 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. There is probably information the user wants to see, after all. In the diff-so-fancy case, it shows something like @ file:1 @.

If the line range could not be found in the colored hunk header, let's just show the complete hunk header.

Signed-off-by Johannes Schindelin <[email protected]>

---
  add-patch.c                | 13 +++++++++++--
  t/t3701-add-interactive.sh |  4 ++++
  2 files changed, 15 insertions(+), 2 deletions(-)

add-patch.c +11 -2

index 9d575d30ed0..0217cdd7c4a 100644
@@ -363,8 +363,17 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)
  	if (p && (p = memmem(p + 4, eol - p - 4, " @@", 3)))
  		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;
+		/*
+		 * We tried to parse the line range out of the colored hunk
+		 * header, so that we could show just the extra information
+		 * after the line range.
+		 *
+		 * At this point, we did not find that line range, but the hunk
+		 * header likely has information that the user might find
+		 * interesting. Let's just show the entire hunk header instead
+		 * in that case.
+		 */
+		header->colored_extra_start = line - s->colored.buf;
  	header->colored_extra_end = hunk->colored_start;
 
  	return 0;

t/t3701-add-interactive.sh +4 -0

index 49200b7df68..39e68b6d066 100755
@@ -775,10 +775,14 @@ test_expect_success 'handle iffy colored hunk headers' '
  		add -p <n &&
  	force_color git -c interactive.diffFilter="sed \"s/\(.*@@\).*/\1FN/\"" \
  		add -p >output 2>&1 <n &&
+	force_color git -c interactive.diffFilter="sed \"s/\(.*@@\).*/file/\"" \
+		add -p >output-so-fancy 2>&1 <n &&
  	if test_have_prereq ADD_I_USE_BUILTIN
  	then
+		grep "@ file\$" output-so-fancy &&
  		grep "@ FN\$" output
  	else
+		grep "^file\$" output-so-fancy &&
  		grep "@FN\$" output
  	fi
  '

From: Johannes Schindelin <[email protected]>

Thanks to alwyas running diff-index and diff-files with the --numstat option (the latter with --ignore-submodules=dirty) before even generating any real diff to parse, the Perl version of git add -p simply ignored dirty submodules and does not even offer them up for staging.

However, the built-in variant did not use that flag because it tries to run only one diff command, skipping the unneeded diff-index/diff-files invocation of the Perl variant and therefore only faithfully recapitulates what the Perl code does once it _does_ generate and parse the real diff.

This causes a problem when running the built-in add -p with diff-so-fancy because that diff colorizer always inserts an empty line before the diff header to ensure that it produces 4 lines as expected by git add -p (the equivalent of the non-colorized diff, index, --- and +++ lines). But git diff-files does not produce any index line for dirty submodules.

The underlying problem is not even the discrepancy in lines, but that git add -p presents diffs for dirty submodules: there is nothing that _can_ be staged for those.

Let's fix that bug, and teach the built-in add -p to ignore dirty submodules, too. This _incidentally_ also fixes the diff-so-fancy problem ;-)

Signed-off-by Johannes Schindelin <[email protected]>

---
  add-patch.c                |  3 ++-
  t/t3701-add-interactive.sh | 12 ++++++++++++
  2 files changed, 14 insertions(+), 1 deletion(-)

add-patch.c +2 -1

index 0217cdd7c4a..ee6a3d3b712 100644
@@ -426,7 +426,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
  	}
  	color_arg_index = args.nr;
  	/* Use `--no-color` explicitly, just in case `diff.color = always`. */
-	strvec_pushl(&args, "--no-color", "-p", "--", NULL);
+	strvec_pushl(&args, "--no-color", "--ignore-submodules=dirty", "-p",
+		     "--", NULL);
  	for (i = 0; i < ps->nr; i++)
  		strvec_push(&args, ps->items[i].original);
 

t/t3701-add-interactive.sh +12 -0

index 39e68b6d066..a4f45fc48a0 100755
@@ -965,6 +965,18 @@ test_expect_success 'status ignores dirty submodules (except HEAD)' '
  	! grep dirty-otherwise output
  '
 
+test_expect_success 'handle submodules' '
+	echo 123 >>for-submodules/dirty-otherwise/initial.t &&
+
+	force_color git -C for-submodules add -p dirty-otherwise >output 2>&1 &&
+	grep "No changes" output &&
+
+	force_color git -C for-submodules add -p dirty-head >output 2>&1 <y &&
+	git -C for-submodules ls-files --stage dirty-head >actual &&
+	rev="$(git -C for-submodules/dirty-head rev-parse HEAD)" &&
+	grep "$rev" actual
+'
+
  test_expect_success 'set up pathological context' '
  	git reset --hard &&
  	test_write_lines a a a a a a a a a a a >a &&

Philippe Blain reported in
https://lore.kernel.org/git/[email protected]
that there is a problem when running the built-in version of git add -p with diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer. The symptom is this:

     error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'

This patch series addresses that and should fix
https://github.com/so-fancy/diff-so-fancy/issues/437

Changes since v2:

* Added the appropriate "Reported-by" trailer to the commit message.
* Split out the logic to insert a space between the colored line range and the extra information, if needed.
* That logic was now corrected to see whether that space is really needed.
* To verify that the logic does what we need it to do, the added regression test now specifically tests for that (single) extra space that we want to be inserted.
* Reworded a stale comment that claimed that we might suppress the entire colored hunk header (which we no longer do).
* Rebased to the current tip of the main branch to avoid a merge conflict with 716c1f649e3 (pipe_command(): mark stdin descriptor as non-blocking, 2022-08-17).

Changes since v1:

* Added a commit to ignore dirty submodules just like the Perl version does.

Johannes Schindelin (5):
t3701: redefine what is "bogus" output of a diff filter add -p: gracefully ignore unparseable hunk headers in colored diffs add -p: insert space in colored hunk header as needed add -p: handle diff-so-fancy's hunk headers better
add -p: ignore dirty submodules

  add-patch.c                | 49 ++++++++++++++++++++++++++++++--------
  t/t3701-add-interactive.sh | 35 ++++++++++++++++++++++++++-
  2 files changed, 73 insertions(+), 11 deletions(-)

base-commit: 07ee72db0e97b5c233f8ada0abb412248c2f1c6f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1336%2Fdscho%2Fdiff-so-fancy-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1336/dscho/diff-so-fancy-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1336

Range-diff vs v2:

  1:  74ab50eeb1c = 1:  a01fa5d25e4 t3701: redefine what is "bogus" output of a diff filter
  2:  b07f85a0359 ! 2:  cbe833bd141 add -p: gracefully ignore unparseable hunk headers in colored diffs
      @@ Commit message
       
           [diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy
       
      +    Reported-by: Philippe Blain <[email protected]>
           Signed-off-by: Johannes Schindelin <[email protected]>
       
        ## add-patch.c ##
      @@ t/t3701-add-interactive.sh: test_expect_success 'detect bogus diffFilter output'
        	force_color test_must_fail git add -p <y
        '
        
      -+test_expect_success 'gracefully fail to parse colored hunk header' '
      ++test_expect_success 'handle iffy colored hunk headers' '
       +	git reset --hard &&
       +
       +	echo content >test &&
      -+	test_config interactive.diffFilter "sed s/@@/XX/g" &&
      -+	printf y >y &&
      -+	force_color git add -p <y
      ++	printf n >n &&
      ++	force_color git -c interactive.diffFilter="sed s/@@/XX/g" \
      ++		add -p <n
       +'
       +
      - test_expect_success 'diff.algorithm is passed to `git diff-files`' '
      + test_expect_success 'handle very large filtered diff' '
        	git reset --hard &&
      -
      + 	# The specific number here is not important, but it must

3: 9dac9f74d2e ! 3: 7a9f0b107e6 add -p: handle diff-so-fancy's hunk headers better
@@ Metadata
Author: Johannes Schindelin <[email protected]>

## Commit message ##
- add -p: handle diff-so-fancy's hunk headers better
+ add -p: insert space in colored hunk header as needed

- 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.
+ We are about to teach git add -p to show the entire hunk header if the
+ @@ ... @@ line range cannot be parsed. Previously, we showed only the
+ remainder of that hunk header as an "colored_extra" part.

- 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.
+ To prepare for that, detect if that "colored_extra" part starts with any
+ non-whitespace character (ignoring ANSI escape sequences) and insert a
+ space, to make the output much more pleasant.
+
+ Note that this has an effect already before we make git add -p more
+ lenient when parsing the hunk headers: diff filters could already remove
+ the space after the line range, which is precisely what we do in the
+ regression test introduced by this commit.

Signed-off-by Johannes Schindelin <[email protected]>

@@ add-patch.c

enum prompt_mode_type {
PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_ADDITION, PROMPT_HUNK,
-@@ add-patch.c: 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;
+@@ add-patch.c: static size_t find_next_line(struct strbuf *sb, size_t offset)
+ return eol - sb->buf + 1;
+ }

- return 0;
++static int starts_with_non_ws(const char *p, size_t len)
++{
++ for (;;) {
++ size_t skip;
++
++ if (!len || isspace(*p))
++ return 0;
++ skip = display_mode_esc_sequence_len(p);
++ if (!skip)
++ return 1;
++ if (skip > len)
++ return 0;
++ p += skip;
++ len -= skip;
++ }
++}
++
+ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
+ ssize_t delta, int colored, struct strbuf *out)
+ {
@@ add-patch.c: static void render_hunk(struct add_p_state *s, struct hunk *hunk,
size_t len;
unsigned long old_offset = header->old_offset;
@@ add-patch.c: 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;
++ needs_extra_space = starts_with_non_ws(p, len);
}

if (s->mode->is_reverse)
@@ add-patch.c: static void render_hunk(struct add_p_state *s, struct hunk *hunk,
else if (colored)

## t/t3701-add-interactive.sh ##
-@@ t/t3701-add-interactive.sh: test_expect_success 'gracefully fail to parse colored hunk header' ' +@@ t/t3701-add-interactive.sh: test_expect_success 'handle iffy colored hunk headers' '
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
+ printf n >n &&
+ force_color git -c interactive.diffFilter="sed s/@@/XX/g" \
+- add -p <n
++ add -p <n &&
++ force_color git -c interactive.diffFilter="sed \"s/\(.*@@\).*/\1FN/\"" \
++ add -p >output 2>&1 <n &&
++ if test_have_prereq ADD_I_USE_BUILTIN
++ then
++ grep "@ FN\$" output
++ else
++ grep "@FN\$" output
++ fi
'

- test_expect_success 'diff.algorithm is passed to git diff-files' '
+ test_expect_success 'handle very large filtered diff' '

  -:  ----------- > 4:  e3e3a178f98 add -p: handle `diff-so-fancy`'s hunk headers better
  4:  540ce27c38a = 5:  cfa6914aee0 add -p: ignore dirty submodules

Quoted 4 lines · Johannes Schindelin

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.

Quoted 4 lines

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.

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 4 lines · writes:

* That logic was now corrected to see whether that space is really needed.
* To verify that the logic does what we need it to do, the added regression test now specifically tests for that (single) extra space that we want to be inserted.

Quoted 3 lines

* Rebased to the current tip of the main branch to avoid a merge conflict with 716c1f649e3 (pipe_command(): mark stdin descriptor as non-blocking, 2022-08-17).

I'd prefer you (or any other contributor) not to pick a more recent base only to work around a merge conflict, [...]

We promoted the built-in one as the primary implementation in 2.37.0; I think we want to keep this fix mergeable down to the 2.37.x maintenance track (either by me, or by distro LTS folks who are motivated enough).

This time, as you can guess by my above description on the exact way how conflict happens, I've rebased the series back for 2.37.x and will resolve the conflict myself (and make sure the result matches the application of these patches directly on top of 'master').

It appears that this round is good to go down to 'next' soonish, but let's see what others find.

The existing test deletes the first line of the diff[1] - so it is removing the "diff --git ..." header not the hunk header. This patch changes the filter to delete everything except the diff header which seems like a less realistic test.

Quoted 4 lines · Phillip Wood

The existing test deletes the first line of the diff[1] - so it is removing the "diff --git ..." header not the hunk header. This patch changes the filter to delete everything except the diff header which seems like a less realistic test.

Is it that all it cares about is that the output has the same number of lines as the input? If so, I agree that it is much less realistic, but it may not matter in practice. Even an "exit 0" might do ;-)

I may be quite off the mark on this one, though.

Quoted 3 lines

Is it that all it cares about is that the output has the same number of lines as the input? If so, I agree that it is much less realistic, but it may not matter in practice. Even an "exit 0" might do ;-)

Yes I think it only cares that there are the same number of lines which begs the question "why are we changing this test in the first place?". The commit message talks about the being unable to parse the hunk header but that's only true because we would be looking in the wrong place for it - the output does in fact contain a valid hunk header. With this change there is no hunk header in the filtered output at all. In practice if the number of lines differs it is normally because the filter messed with the diff header and removed some lines from it which is what the existing test simulates.

I'm struggling to understand the need for this change from the explanation in the commit message as it seems to me to assume the line being deleted is the hunk header when in fact it is deleting the diff header.

Quoted 3 lines

I'm struggling to understand the need for this change from the explanation in the commit message as it seems to me to assume the line being deleted is the hunk header when in fact it is deleting the diff header.

FWIW, as the author of the original test, I'm also confused about why it needs to be changed. The filter I wrote in the original test was just "echo too-short". It was switched to "sed 1d" because the original did not read the input at all, which racily caused Git to see SIGPIPE:

https://lore.kernel.org/git/[email protected]/

Switching to "exit 0" would bring that problem back. But I think "sed q" potentially does, too, because sed will quit without reading all of the input. We really do want something like "sed 1d" that changes the number of lines, but ensures that the filter reads to EOF.

Quoted 9 lines · Jeff King

Quoted 3 lines

I'm struggling to understand the need for this change from the explanation in the commit message as it seems to me to assume the line being deleted is the hunk header when in fact it is deleting the diff header.

FWIW, as the author of the original test, I'm also confused about why it needs to be changed. The filter I wrote in the original test was just "echo too-short". It was switched to "sed 1d" because the original did not read the input at all, which racily caused Git to see SIGPIPE:

Quoted 6 lines

https://lore.kernel.org/git/[email protected]/

Switching to "exit 0" would bring that problem back. But I think "sed q" potentially does, too, because sed will quit without reading all of the input. We really do want something like "sed 1d" that changes the number of lines, but ensures that the filter reads to EOF.

By the way, the test change is needed for it to pass with the change in patch 2, where we become more lenient about parsing the hunk header. That implies to me that the builtin version's check for one-to-one line correspondence is broken, but we didn't notice.

In the perl version, that test fails because it triggers the mismatched-output check:

  $ GIT_TEST_ADD_I_USE_BUILTIN=false ./t3701-add-interactive.sh --verbose-only=56
  [...]
  fatal: mismatched output from interactive.diffFilter
  hint: Your filter must maintain a one-to-one correspondence
  hint: between its input and output lines.
  ok 56 - detect bogus diffFilter output

but the builtin version complains about the hunk header:

  $ GIT_TEST_ADD_I_USE_BUILTIN=true ./t3701-add-interactive.sh --verbose-only=56
  [...]
  error: could not parse colored hunk header '?[31m-10?[m'
  ok 56 - detect bogus diffFilter output

Once patch 2 is applied, we stop complaining there, and we _should_ complain that the number of lines isn't the same. But we don't:

  $ GIT_TEST_ADD_I_USE_BUILTIN=true ./t3701-add-interactive.sh --verbose-only=56
  test_must_fail: command succeeded: git add -p
  not ok 56 - detect bogus diffFilter output

Quoted 7 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

Thanks to alwyas running diff-index and diff-files with the --numstat option (the latter with --ignore-submodules=dirty) before even generating any real diff to parse, the Perl version of git add -p simply ignored dirty submodules and does not even offer them up for staging.

I had a bit of a hard time understanding this paragraph. To me the fact that the perl version is using --numstat is not that important here, what is important is that it is using --ignore-submodules=dirty when it generates its list of files to show and that information is consigned to a parenthesized aside.

The fix itself looks good.

And it is that doubling of work that I tried to avoid when implementing the built-in version. The bug came about because the full diff call wasn't using the --ignore-submodules=dirty option, and that's what I missed.

This is maybe more interesting a story for the cover letter, to be able to understand how this bug was introduced, and maybe to offer an opportunity for others (in addition to myself) to learn from my mistake.

Quoted 4 lines · Johannes Schindelin

And it is that doubling of work that I tried to avoid when implementing the built-in version. The bug came about because the full diff call wasn't using the --ignore-submodules=dirty option, and that's what I missed.

Quoted 3 lines

This is maybe more interesting a story for the cover letter, to be able to understand how this bug was introduced, and maybe to offer an opportunity for others (in addition to myself) to learn from my mistake.

Yup, the moral of the story is premature optimization bites because we are not always careful ;-)

Anyhow, I am getting an impression that the behaviour of the next iteration would be much closer to the original, which is excellent.

We have seen too many "ah, this is broken and here is a fix that is appropriate in the context of how the new C version does it", not "ok, let's make the whole thing behave more like what we used to have".

I'm not convinced about patch 1 or about showing the hunk offsets with interactive.diffFilter in patch 4 but the series looks basically sound.

From: Johannes Schindelin <[email protected]>

When parsing the colored version of a diff, the interactive add command really relies on the colored version having the same number of lines as the plain (uncolored) version. That is an invariant.

We already have code to verify correctly when the colored diff has less lines than the plain diff. Modulo an off-by-one bug: If the last diff line has no matching colored one, the code pretends to succeed, still.

To make matters worse, when we adjusted the test in 1e4ffc765db (t3701:
adjust difffilter test, 2020-01-14), we did not catch this because add -p fails for a _different_ reason: it does not find any colored hunk header that contains a parseable line range.

If we change the test case so that the line range _can_ be parsed, the bug is exposed.

Let's address all of the above by

- fixing the off-by-one,

- adjusting the test case to allow add -p to parse the line range

- making the test case more stringent by verifying that the expected error message is shown

Also adjust a misleading code comment about the now-fixed code.

Signed-off-by Johannes Schindelin <[email protected]>

---
  add-patch.c                | 5 ++++-
  t/t3701-add-interactive.sh | 5 +++--
  2 files changed, 7 insertions(+), 3 deletions(-)

add-patch.c +4 -1

index 509ca04456b..34f3807ff32 100644
@@ -592,7 +592,10 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
  			if (colored_eol)
  				colored_p = colored_eol + 1;
  			else if (p != pend)
-				/* colored shorter than non-colored? */
+				/* non-colored has more lines? */
+				goto mismatched_output;
+			else if (colored_p == colored_pend)
+				/* last line has no matching colored one? */
  				goto mismatched_output;
  			else
  				colored_p = colored_pend;

t/t3701-add-interactive.sh +3 -2

index 3b7df9bed5a..8a594700f7b 100755
@@ -761,9 +761,10 @@ test_expect_success 'detect bogus diffFilter output' '
  	git reset --hard &&
 
  	echo content >test &&
-	test_config interactive.diffFilter "sed 1d" &&
+	test_config interactive.diffFilter "sed 6d" &&
  	printf y >y &&
-	force_color test_must_fail git add -p <y
+	force_color test_must_fail git add -p <y >output 2>&1 &&
+	grep "mismatched output" output
  '
 
  test_expect_success 'handle very large filtered diff' '

From: Johannes Schindelin <[email protected]>

In
https://lore.kernel.org/git/[email protected],
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.

[diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy

Reported-by Philippe Blain <[email protected]>

Helped-by Phillip Wood <[email protected]>

Signed-off-by Johannes Schindelin <[email protected]>

---
  add-patch.c                | 42 ++++++++++++++++++++------------------
  t/t3701-add-interactive.sh | 10 +++++++++
  2 files changed, 32 insertions(+), 20 deletions(-)

add-patch.c +1 -0

index 34f3807ff32..3699ca1fcaf 100644
@@ -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)

add-patch.c +7 -12

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

t/t3701-add-interactive.sh +0 -0

index 8a594700f7b..47ed6698943 100755
@@ -767,6 +767,16 @@ test_expect_success 'detect bogus diffFilter output' '
  	grep "mismatched output" output

t/t3701-add-interactive.sh +2 -0

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

Philippe Blain reported in
https://lore.kernel.org/git/[email protected]
that there is a problem when running the built-in version of git add -p with diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer. The symptom is this:

     error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'

This patch series addresses that and should fix
https://github.com/so-fancy/diff-so-fancy/issues/437

Changes since v3:

* Instead of deviating from how the Perl version of git add -p did things, we now teach the built-in version to display hunk headers verbatim when no line range could be parsed out (instead of showing the line range anyways). This was a very good idea of Phillip's, dramatically simplifying the patch series.
* Also, this iteration drops the first patch that claims to redefine what we consider bogus, but only hides an off-by-one. In its stead, there is now a patch that fixes said off-by-one.

Changes since v2:

* Added the appropriate "Reported-by" trailer to the commit message.
* Split out the logic to insert a space between the colored line range and the extra information, if needed.
* That logic was now corrected to see whether that space is really needed.
* To verify that the logic does what we need it to do, the added regression test now specifically tests for that (single) extra space that we want to be inserted.
* Reworded a stale comment that claimed that we might suppress the entire colored hunk header (which we no longer do).
* Rebased to the current tip of the main branch to avoid a merge conflict with 716c1f649e3 (pipe_command(): mark stdin descriptor as non-blocking, 2022-08-17).

Changes since v1:

* Added a commit to ignore dirty submodules just like the Perl version does.

Johannes Schindelin (3):
add -p: detect more mismatches between plain vs colored diffs add -p: gracefully handle unparseable hunk headers in colored diffs add -p: ignore dirty submodules

  add-patch.c                | 50 +++++++++++++++++++++-----------------
  t/t3701-add-interactive.sh | 27 ++++++++++++++++++--
  2 files changed, 53 insertions(+), 24 deletions(-)

base-commit: 07ee72db0e97b5c233f8ada0abb412248c2f1c6f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1336%2Fdscho%2Fdiff-so-fancy-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1336/dscho/diff-so-fancy-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1336

Range-diff vs v3:

1: a01fa5d25e4 ! 1: 25187c3a3c2 t3701: redefine what is "bogus" output of a diff filter
@@ Metadata
Author: Johannes Schindelin <[email protected]>

## Commit message ##
- t3701: redefine what is "bogus" output of a diff filter
+ add -p: detect more mismatches between plain vs colored diffs

When parsing the colored version of a diff, the interactive add command really relies on the colored version having the same number of
- lines as the non-colored version. That is an invariant.
-
- However, in the 'detect bogus diffFilter output' test case in t3701, we
- essentially required a hunk header that contains parseable @@ ... @@
- hunk headers, and called all colored diffs without such hunks bogus.
-
- The reason for this is that we would like to show the users the adjusted
- hunk headers _including_ the extra part after the @@ ... @@
- information, which usually contains things like the function name or
- soms such.
-
- Now, there is a _very_ popular diff colorizer called diff-so-fancy
- that does not produce such colored diffs as the built-in add command
- expects. Nevertheless, the Perl variant of the add command handles
- those nicely, essentially by ignoring the hunk header and saying "there
- is nothing else we can show except the original hunk header, even if we
- had to adjust the line range and the original hunk header might get that
- wrong".
-
- In preparation for teaching the built-in interactive add to be a bit
- more lenient, let's change the 'detect bogus diffFilter output' test
- case so that it verifies that a mismatched number of lines causes the
- command to error out, but not an unparseable hunk header.
+ lines as the plain (uncolored) version. That is an invariant.
+
+ We already have code to verify correctly when the colored diff has less
+ lines than the plain diff. Modulo an off-by-one bug: If the last diff
+ line has no matching colored one, the code pretends to succeed, still.
+
+ To make matters worse, when we adjusted the test in 1e4ffc765db (t3701:
+ adjust difffilter test, 2020-01-14), we did not catch this because `add
+ -p` fails for a _different_ reason: it does not find any colored hunk
+ header that contains a parseable line range.
+
+ If we change the test case so that the line range _can_ be parsed, the
+ bug is exposed.
+
+ Let's address all of the above by
+
+ - fixing the off-by-one,
+
+ - adjusting the test case to allow add -p to parse the line range
+
+ - making the test case more stringent by verifying that the expected
+ error message is shown
+
+ Also adjust a misleading code comment about the now-fixed code.

Signed-off-by Johannes Schindelin <[email protected]>

+ ## add-patch.c ##
+@@ add-patch.c: static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
+ if (colored_eol)
+ colored_p = colored_eol + 1;
+ else if (p != pend)
+- /* colored shorter than non-colored? */
++ /* non-colored has more lines? */
++ goto mismatched_output;
++ else if (colored_p == colored_pend)
++ /* last line has no matching colored one? */
+ goto mismatched_output;
+ else
+ colored_p = colored_pend;
+
## t/t3701-add-interactive.sh ##
@@ t/t3701-add-interactive.sh: test_expect_success 'detect bogus diffFilter output' '
git reset --hard &&

echo content >test &&
- test_config interactive.diffFilter "sed 1d" &&
-+ test_config interactive.diffFilter "sed q" &&
++ test_config interactive.diffFilter "sed 6d" &&
printf y >y &&
- force_color test_must_fail git add -p <y
+- force_color test_must_fail git add -p <y
++ force_color test_must_fail git add -p <y >output 2>&1 &&
++ grep "mismatched output" output
'
+
+ test_expect_success 'handle very large filtered diff' '
2: cbe833bd141 ! 2: cd1c5100506 add -p: gracefully ignore unparseable hunk headers in colored diffs

      @@ Metadata
       Author: Johannes Schindelin <[email protected]>
       
        ## Commit message ##
      -    add -p: gracefully ignore unparseable hunk headers in colored diffs
      +    add -p: gracefully handle unparseable hunk headers in colored diffs
       
           In
           https://lore.kernel.org/git/[email protected],
      @@ Commit message
           and therefore we cannot detect any part in that header that comes after
           the line range.
       
      -    Let's punt for now and simply show nothing apart from the line range in
      -    that case.
      +    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`.
       
           [diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy
       
           Reported-by: Philippe Blain <[email protected]>
      +    Helped-by: Phillip Wood <[email protected]>
           Signed-off-by: Johannes Schindelin <[email protected]>
       
        ## add-patch.c ##
      +@@ add-patch.c: 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 {
       @@ add-patch.c: static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)
      - 	eol = memchr(line, '\n', s->colored.len - hunk->colored_start);
        	if (!eol)
        		eol = s->colored.buf + s->colored.len;
      --	p = memmem(line, eol - line, "@@ -", 4);
      + 	p = memmem(line, eol - line, "@@ -", 4);
       -	if (!p)
       -		return error(_("could not parse colored hunk header '%.*s'"),
       -			     (int)(eol - line), line);
      @@ add-patch.c: static int parse_hunk_header(struct add_p_state *s, struct hunk *hu
       -	if (!p)
       -		return error(_("could not parse colored hunk header '%.*s'"),
       -			     (int)(eol - line), line);
      - 	hunk->colored_start = eol - s->colored.buf + (*eol == '\n');
      --	header->colored_extra_start = p + 3 - s->colored.buf;
      -+	p = memmem(line, eol - line, "@@ -", 4);
       +	if (p && (p = memmem(p + 4, eol - p - 4, " @@", 3)))
       +		header->colored_extra_start = p + 3 - s->colored.buf;
      -+	else
      -+		/* could not parse colored hunk header, showing nothing */
      ++	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;
      +@@ add-patch.c: static void render_hunk(struct add_p_state *s, struct hunk *hunk,
      + 				- header->colored_extra_start;
      + 		}
      +
      +-		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);
       
        ## t/t3701-add-interactive.sh ##
       @@ t/t3701-add-interactive.sh: test_expect_success 'detect bogus diffFilter output' '
      - 	force_color test_must_fail git add -p <y
      + 	grep "mismatched output" output
        '
        
       +test_expect_success 'handle iffy colored hunk headers' '
      @@ t/t3701-add-interactive.sh: test_expect_success 'detect bogus diffFilter output'
       +
       +	echo content >test &&
       +	printf n >n &&
      -+	force_color git -c interactive.diffFilter="sed s/@@/XX/g" \
      -+		add -p <n
      ++	force_color git -c interactive.diffFilter="sed s/.*@@.*/XX/" \
      ++		add -p >output 2>&1 <n &&
      ++	grep "^[^@]*XX[^@]*$" output
       +'
       +
        test_expect_success 'handle very large filtered diff' '
  3:  7a9f0b107e6 < -:  ----------- add -p: insert space in colored hunk header as needed
  4:  e3e3a178f98 < -:  ----------- add -p: handle `diff-so-fancy`'s hunk headers better
  5:  cfa6914aee0 = 3:  116f0cf5cab add -p: ignore dirty submodules

On 31/08/2022 21:31, Johannes Schindelin via GitGitGadget wrote:
[...]

add-patch.c static int parse_hunk_header(struct add_p_state *s, · line 363

Quoted 13 lines

add-patch.c +1 -7

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

Review static void render_hunk(struct add_p_state *s, struct · line 666

Quoted 2 lines

(quoted diff) +0 -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...

                }

Quoted 19 lines

add-patch.c +7 -11

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

add-patch.c static void render_hunk(struct add_p_state *s, struct hunk *hunk, · line 685

Quoted 10 lines

add-patch.c +7 -0

+			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');

        }

Quoted 8 lines

t/t3701-add-interactive.sh +0 -0

index 8a594700f7b..47ed6698943 100755
@@ -767,6 +767,16 @@ test_expect_success 'detect bogus diffFilter output' '
  	grep "mismatched output" output
  '
 

t/t3701-add-interactive.sh test_expect_success 'detect bogus diffFilter output' ' · line 777

Quoted 8 lines

t/t3701-add-interactive.sh +8 -0

+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

Quoted 5 lines

t/t3701-add-interactive.sh +2 -0

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

Thanks to alwyas running diff-index and diff-files with the

Quoted 1 line · Johannes Schindelin via GitGitGadget

Thanks to alwyas running diff-index and diff-files with the

Not a big deal, but since it sounds like you're going to re-roll anyway:

s/alwyas/always/

From: Johannes Schindelin <[email protected]>

In
https://lore.kernel.org/git/[email protected],
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.

[diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy

Reported-by Philippe Blain <[email protected]>

Helped-by Phillip Wood <[email protected]>

Signed-off-by Johannes Schindelin <[email protected]>

---

add-patch.c +0 -0

@@ -659,6 +659,15 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
  		if (!colored) {
  			p = s->plain.buf + header->extra_start;

t/t3701-add-interactive.sh +0 -0

index 8a594700f7b..a94e7c53c8a 100755
@@ -767,6 +767,16 @@ test_expect_success 'detect bogus diffFilter output' '
  	grep "mismatched output" output

Philippe Blain reported in
https://lore.kernel.org/git/[email protected]
that there is a problem when running the built-in version of git add -p with diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer. The symptom is this:

     error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'

This patch series addresses that and should fix
https://github.com/so-fancy/diff-so-fancy/issues/437

Changes since v4:

* The second patch was further simplified, avoiding to print extra ANSI sequences if the hunk header is shown verbatim.

Changes since v3:

* Instead of deviating from how the Perl version of git add -p did things, we now teach the built-in version to display hunk headers verbatim when no line range could be parsed out (instead of showing the line range anyways). This was a very good idea of Phillip's, dramatically simplifying the patch series.
* Also, this iteration drops the first patch that claims to redefine what we consider bogus, but only hides an off-by-one. In its stead, there is now a patch that fixes said off-by-one.

Changes since v2:

* Added the appropriate "Reported-by" trailer to the commit message.
* Split out the logic to insert a space between the colored line range and the extra information, if needed.
* That logic was now corrected to see whether that space is really needed.
* To verify that the logic does what we need it to do, the added regression test now specifically tests for that (single) extra space that we want to be inserted.
* Reworded a stale comment that claimed that we might suppress the entire colored hunk header (which we no longer do).
* Rebased to the current tip of the main branch to avoid a merge conflict with 716c1f649e3 (pipe_command(): mark stdin descriptor as non-blocking, 2022-08-17).

Changes since v1:

* Added a commit to ignore dirty submodules just like the Perl version does.

Johannes Schindelin (3):
add -p: detect more mismatches between plain vs colored diffs add -p: gracefully handle unparseable hunk headers in colored diffs add -p: ignore dirty submodules

  add-patch.c                | 33 +++++++++++++++++++++++----------
  t/t3701-add-interactive.sh | 27 +++++++++++++++++++++++++--
  2 files changed, 48 insertions(+), 12 deletions(-)

base-commit: 07ee72db0e97b5c233f8ada0abb412248c2f1c6f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1336%2Fdscho%2Fdiff-so-fancy-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1336/dscho/diff-so-fancy-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1336

Range-diff vs v4:

  1:  25187c3a3c2 = 1:  25187c3a3c2 add -p: detect more mismatches between plain vs colored diffs
  2:  cd1c5100506 ! 2:  93d0e3b4d2a add -p: gracefully handle unparseable hunk headers in colored diffs
      @@ Commit message
           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`.
      -
           [diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy
       
           Reported-by: Philippe Blain <[email protected]>
      @@ add-patch.c: static int parse_hunk_header(struct add_p_state *s, struct hunk *hu
       -	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)))
      ++	if (p && (p = memmem(p + 4, eol - p - 4, " @@", 3))) {
       +		header->colored_extra_start = p + 3 - s->colored.buf;
      -+	else {
      ++	} else {
       +		/* could not parse colored hunk header, leave as-is */
       +		header->colored_extra_start = hunk->colored_start;
       +		header->suppress_colored_line_range = 1;
      @@ add-patch.c: static int parse_hunk_header(struct add_p_state *s, struct hunk *hu
        
        	return 0;
       @@ add-patch.c: static void render_hunk(struct add_p_state *s, struct hunk *hunk,
      - 				- header->colored_extra_start;
      - 		}
      -
      --		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;
      + 		if (!colored) {
      + 			p = s->plain.buf + header->extra_start;
      + 			len = header->extra_end - header->extra_start;
      ++		} else if (header->suppress_colored_line_range) {
      ++			strbuf_add(out,
      ++				   s->colored.buf + header->colored_extra_start,
      ++				   header->colored_extra_end -
      ++				   header->colored_extra_start);
       +
      -+			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);
      ++			strbuf_add(out, s->colored.buf + hunk->colored_start,
      ++				   hunk->colored_end - hunk->colored_start);
      ++			return;
      + 		} else {
      + 			strbuf_addstr(out, s->s.fraginfo_color);
      + 			p = s->colored.buf + header->colored_extra_start;
       
        ## t/t3701-add-interactive.sh ##
       @@ t/t3701-add-interactive.sh: test_expect_success 'detect bogus diffFilter output' '
      @@ t/t3701-add-interactive.sh: test_expect_success 'detect bogus diffFilter output'
       +	printf n >n &&
       +	force_color git -c interactive.diffFilter="sed s/.*@@.*/XX/" \
       +		add -p >output 2>&1 <n &&
      -+	grep "^[^@]*XX[^@]*$" output
      ++	grep "^XX$" output
       +'
       +
        test_expect_success 'handle very large filtered diff' '

3: 116f0cf5cab = 3: 47943b603b1 add -p: ignore dirty submodules

From: Johannes Schindelin <[email protected]>

Thanks to alwyas running diff-index and diff-files with the

This version is looking much simpler and nicer, I've left a couple of comments on the second patch as we're still not quite printing the output of the diff filter verbatim.

Hi Dscho

Quoted 5 lines · Johannes Schindelin via GitGitGadget

Philippe Blain reported in
https://lore.kernel.org/git/[email protected]
that there is a problem when running the built-in version of git add -p with diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer. The symptom is this:

Quoted 8 lines

error: could not parse colored hunk header
'?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'
This patch series addresses that and should fix
https://github.com/so-fancy/diff-so-fancy/issues/437
Changes since v4:
* The second patch was further simplified, avoiding to print extra
ANSI
sequences if the hunk header is shown verbatim.

That was a quick re-roll! I'm very happy with this version

From: Johannes Schindelin <[email protected]>

Thanks to alwyas running diff-index and diff-files with the

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 2 lines · writes:

From: Johannes Schindelin <[email protected]>

Quoted 1 line

Thanks to alwyas running diff-index and diff-files with the

"always".

I notice that this round (like the previous one, v3) is made not to apply on 'maint'. As I said in the earlier review, in general I prefer to fork a bugfix topic out of 'maint' unless there is a strong reason not to.

It is a different matter if all these bugfix topics are actually merged down to 'maint' and tagged a new maintenance release by me. But I'd prefer to make it easier for motivated distro packagers to adopt fixes that proves good in our 'master' front to their maint releases, and it would be much easier to just merge a topic based on 'maint' than having to cherry-pick a topic based on 'master'.

Thanks.