git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] built-in add -p: support diff-so-fancy better
@ 2022-08-23 18:04 Johannes Schindelin via GitGitGadget
  2022-08-23 18:04 ` [PATCH 1/3] t3701: redefine what is "bogus" output of a diff filter Johannes Schindelin via GitGitGadget
                   ` (4 more replies)
  0 siblings, 5 replies; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-23 18:04 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Johannes Schindelin

Philippe Blain reported in
https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com
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

Johannes Schindelin (3):
  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-patch.c                | 21 ++++++++++++---------
 t/t3701-add-interactive.sh | 12 +++++++++++-
 2 files changed, 23 insertions(+), 10 deletions(-)


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

^ permalink raw reply	[flat|nested] 67+ messages in thread

* [PATCH 1/3] t3701: redefine what is "bogus" output of a diff filter
  2022-08-23 18:04 [PATCH 0/3] built-in add -p: support diff-so-fancy better Johannes Schindelin via GitGitGadget
@ 2022-08-23 18:04 ` Johannes Schindelin via GitGitGadget
  2022-08-23 18:04 ` [PATCH 2/3] add -p: gracefully ignore unparseable hunk headers in colored diffs Johannes Schindelin via GitGitGadget
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-23 18:04 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 t/t3701-add-interactive.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index b354fb39de8..b40d1c94d99 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -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
 '
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 2/3] add -p: gracefully ignore unparseable hunk headers in colored diffs
  2022-08-23 18:04 [PATCH 0/3] built-in add -p: support diff-so-fancy better Johannes Schindelin via GitGitGadget
  2022-08-23 18:04 ` [PATCH 1/3] t3701: redefine what is "bogus" output of a diff filter Johannes Schindelin via GitGitGadget
@ 2022-08-23 18:04 ` Johannes Schindelin via GitGitGadget
  2022-08-23 18:04 ` [PATCH 3/3] add -p: handle `diff-so-fancy`'s hunk headers better Johannes Schindelin via GitGitGadget
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-23 18:04 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In
https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com,
Phillipe Blain reported that the built-in `git add -p` command fails
when asked to use [`diff-so-fancy`][diff-so-fancy] to colorize the diff.

The reason is that this tool produces colored diffs with a hunk header
that does not contain any parseable `@@ ... @@` line range information,
and therefore we cannot detect any part in that header that comes after
the line range.

Let's punt for now and simply show nothing apart from the line range in
that case.

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c                | 15 ++++++---------
 t/t3701-add-interactive.sh |  9 +++++++++
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 509ca04456b..f2fffe1af02 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -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'"),
-			     (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 */
+		header->colored_extra_start = hunk->colored_start;
 	header->colored_extra_end = hunk->colored_start;
 
 	return 0;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index b40d1c94d99..7e3c1de71f5 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -766,6 +766,15 @@ 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' '
+	git reset --hard &&
+
+	echo content >test &&
+	test_config interactive.diffFilter "sed s/@@/XX/g" &&
+	printf y >y &&
+	force_color git add -p <y
+'
+
 test_expect_success 'diff.algorithm is passed to `git diff-files`' '
 	git reset --hard &&
 
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 3/3] add -p: handle `diff-so-fancy`'s hunk headers better
  2022-08-23 18:04 [PATCH 0/3] built-in add -p: support diff-so-fancy better Johannes Schindelin via GitGitGadget
  2022-08-23 18:04 ` [PATCH 1/3] t3701: redefine what is "bogus" output of a diff filter Johannes Schindelin via GitGitGadget
  2022-08-23 18:04 ` [PATCH 2/3] add -p: gracefully ignore unparseable hunk headers in colored diffs Johannes Schindelin via GitGitGadget
@ 2022-08-23 18:04 ` Johannes Schindelin via GitGitGadget
  2022-08-24  3:49 ` [PATCH 0/3] built-in add -p: support diff-so-fancy better Philippe Blain
  2022-08-24 21:21 ` [PATCH v2 0/4] " Johannes Schindelin via GitGitGadget
  4 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-23 18:04 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 add-patch.c                | 8 +++++++-
 t/t3701-add-interactive.sh | 3 ++-
 2 files changed, 9 insertions(+), 2 deletions(-)

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`' '
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 67+ messages in thread

* Re: [PATCH 0/3] built-in add -p: support diff-so-fancy better
  2022-08-23 18:04 [PATCH 0/3] built-in add -p: support diff-so-fancy better Johannes Schindelin via GitGitGadget
                   ` (2 preceding siblings ...)
  2022-08-23 18:04 ` [PATCH 3/3] add -p: handle `diff-so-fancy`'s hunk headers better Johannes Schindelin via GitGitGadget
@ 2022-08-24  3:49 ` Philippe Blain
  2022-08-24  6:27   ` Johannes Schindelin
  2022-08-24 21:21 ` [PATCH v2 0/4] " Johannes Schindelin via GitGitGadget
  4 siblings, 1 reply; 67+ messages in thread
From: Philippe Blain @ 2022-08-24  3:49 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget, git; +Cc: Johannes Schindelin

Hi Dscho,

Le 2022-08-23 à 14:04, Johannes Schindelin via GitGitGadget a écrit :
> Philippe Blain reported in
> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com
> 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

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/s40ss309-3311-o08s-38r2-9144r33pq549@tzk.qr/.

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

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 0/3] built-in add -p: support diff-so-fancy better
  2022-08-24  3:49 ` [PATCH 0/3] built-in add -p: support diff-so-fancy better Philippe Blain
@ 2022-08-24  6:27   ` Johannes Schindelin
  2022-08-24 13:21     ` Philippe Blain
  0 siblings, 1 reply; 67+ messages in thread
From: Johannes Schindelin @ 2022-08-24  6:27 UTC (permalink / raw)
  To: Philippe Blain; +Cc: Johannes Schindelin via GitGitGadget, git

[-- Attachment #1: Type: text/plain, Size: 2026 bytes --]

Hi Philippe,

On Tue, 23 Aug 2022, Philippe Blain wrote:

> Le 2022-08-23 à 14:04, Johannes Schindelin via GitGitGadget a écrit :
> > Philippe Blain reported in
> > https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com
> > 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
>
> 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].

Great!

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

Thanks,
Dscho

> This ('--patch') was added in [2], about a month after Peff wrote the message
> you mention in https://lore.kernel.org/git/s40ss309-3311-o08s-38r2-9144r33pq549@tzk.qr/.
>
> 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
>

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 0/3] built-in add -p: support diff-so-fancy better
  2022-08-24  6:27   ` Johannes Schindelin
@ 2022-08-24 13:21     ` Philippe Blain
  2022-08-24 17:49       ` Philippe Blain
  0 siblings, 1 reply; 67+ messages in thread
From: Philippe Blain @ 2022-08-24 13:21 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Johannes Schindelin via GitGitGadget, git

Hi Dscho,

Le 2022-08-24 à 02:27, Johannes Schindelin a écrit :
> Hi Philippe,
> 
> On Tue, 23 Aug 2022, Philippe Blain wrote:
> 
>> 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?
> 
> 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.

As for my real-life example, I noticed a little change in the output. 
Here is what it looks like with the Perl version:

$ git -c interactive.diffFilter="diff-so-fancy --patch" -c add.interactive.useBuiltin=false -add p ice_dyn_vp.F90

───────────────────────────────────────────────────────────────────────
modified: ice_dyn_vp.F90
───────────────────────────────────────────────────────────────────────
@ ice_dyn_vp.F90:274 @ subroutine implicit_solver (dt)
         do i = 1, nx_block
            rdg_conv (i,j,iblk) = c0
            rdg_shear(i,j,iblk) = c0
            divu (i,j,iblk) = c0
            divv (i,j,iblk) = c0
            shear(i,j,iblk) = c0
         enddo
         enddo
(1/1) Stage this hunk [y,n,q,a,d,e,?]? 

The first part of the hunk header "ice_dyn_vp.F90:274" is in pink and the subroutine
name is in light mauve (default diff-so-fancy colors). Here is what it looks like
with your patches with the builtin version:

$ git -c interactive.diffFilter="diff-so-fancy --patch" add -p ice_dyn_vp.F90

───────────────────────────────────────────────────────────────────────
modified: ice_dyn_vp.F90
───────────────────────────────────────────────────────────────────────
@@ -271,7 +271,7 @@ @ ice_dyn_vp.F90:274 @ subroutine implicit_solver (dt)
         do i = 1, nx_block
            rdg_conv (i,j,iblk) = c0
            rdg_shear(i,j,iblk) = c0
            divu (i,j,iblk) = c0
            divv (i,j,iblk) = c0
            shear(i,j,iblk) = c0
         enddo
         enddo
(1/1) Stage this hunk [y,n,q,a,d,e,?]? 

Notice we have the line numbers (-271,7 +271,7) in cyan, then the first part
of the hunk header modified by diff-so-fancy (ice_dyn_vp.F90:274) in pink, and
then the subroutine name (in light mauve). 

We can see the same thing with my earlier reproducer, only there is no function
matched. I really don't mind having this first part with the line numbers, I
just wanted to share my observation.

Thanks,

Philippe.

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 0/3] built-in add -p: support diff-so-fancy better
  2022-08-24 13:21     ` Philippe Blain
@ 2022-08-24 17:49       ` Philippe Blain
  2022-08-24 18:24         ` Junio C Hamano
  0 siblings, 1 reply; 67+ messages in thread
From: Philippe Blain @ 2022-08-24 17:49 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Johannes Schindelin via GitGitGadget, git

Hi Dscho,

Le 2022-08-24 à 09:21, Philippe Blain a écrit :
> Hi Dscho,
> 
> Le 2022-08-24 à 02:27, Johannes Schindelin a écrit :
>> Hi Philippe,
>>
>> On Tue, 23 Aug 2022, Philippe Blain wrote:
>>
>>> 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?
>>
>> 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.

So in trying to write a new reproducer, I found the cause of the bug. A submodule in "modified 
content" or "untracked content" state (i.e. no new commit) has no "index" line in its diff
header. diff-so-fancy, with --patch, always *adds* a blank line before the diff-header 
because its modified diff header is 3 lines, whereas Git's is 4. So the count is off 
specifically for submodules in  "modified/untracked content" state. 

It seems the C version is less lenient than the Perl version, because the Perl version does
not complain. But I'd say that it's more of a bug on the diff-so-fancy side, even if it 
is a regression of the builtin version...
Delta does not alter the header (in its default configuration) and so it is not affected here.

For what it's worth, here is the updated reproducer script. It also downloads the 'delta' executable (for Linux)
and runs it. With delta, both Perl and C versions work. With diff-so-fancy, the C version 
complains bout mismatched output.

```bash
#!/bin/bash

set -euEo pipefail

repo=repro
rm -rf $repo

TEST_AUTHOR_LOCALNAME=author
TEST_AUTHOR_DOMAIN=example.com
GIT_AUTHOR_EMAIL=${TEST_AUTHOR_LOCALNAME}@${TEST_AUTHOR_DOMAIN}
GIT_AUTHOR_NAME='A U Thor'
GIT_AUTHOR_DATE='1112354055 +0200'
TEST_COMMITTER_LOCALNAME=committer
TEST_COMMITTER_DOMAIN=example.com
GIT_COMMITTER_EMAIL=${TEST_COMMITTER_LOCALNAME}@${TEST_COMMITTER_DOMAIN}
GIT_COMMITTER_NAME='C O Mitter'
GIT_COMMITTER_DATE='1112354055 +0200'
export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME
export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME
export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
export HOME=

git -c init.defaultBranch=unimportant init $repo
cd $repo
date>file
git add file
git commit -m file
git clone https://github.com/so-fancy/diff-so-fancy.git
date>>file
export PATH="diff-so-fancy:$PATH"
git submodule add ./diff-so-fancy
git commit -m "add sub"
date>diff-so-fancy/README.md
curl -sSLO https://github.com/dandavison/delta/releases/download/0.12.1/delta-0.12.1-x86_64-unknown-linux-gnu.tar.gz
tar xzf delta-0.12.1-x86_64-unknown-linux-gnu.tar.gz
export PATH="delta-0.12.1-x86_64-unknown-linux-gnu:$PATH"
echo -----
git -c interactive.diffFilter="delta --color-only" -c add.interactive.useBuiltin=false add -p < <(echo n)
echo -----
git -c interactive.diffFilter="delta --color-only" add -p < <(echo n)
echo -----
git -c interactive.diffFilter="diff-so-fancy --patch" -c add.interactive.useBuiltin=false add -p < <(echo n)
echo -----
git -c interactive.diffFilter="diff-so-fancy --patch" add -p < <(echo n) 
```

Cheers,

Philippe.

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 0/3] built-in add -p: support diff-so-fancy better
  2022-08-24 17:49       ` Philippe Blain
@ 2022-08-24 18:24         ` Junio C Hamano
  2022-08-24 21:05           ` Johannes Schindelin
  0 siblings, 1 reply; 67+ messages in thread
From: Junio C Hamano @ 2022-08-24 18:24 UTC (permalink / raw)
  To: Philippe Blain
  Cc: Johannes Schindelin, Johannes Schindelin via GitGitGadget, git

Philippe Blain <levraiphilippeblain@gmail.com> writes:

>>>> 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?
>>>
>>> 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.
>
> 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".  Please thank those in diff-so-fancy
land for reporting the issue.

Thanks, you two, for working on this.

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 0/3] built-in add -p: support diff-so-fancy better
  2022-08-24 18:24         ` Junio C Hamano
@ 2022-08-24 21:05           ` Johannes Schindelin
  2022-08-24 21:37             ` Junio C Hamano
  0 siblings, 1 reply; 67+ messages in thread
From: Johannes Schindelin @ 2022-08-24 21:05 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Philippe Blain, Johannes Schindelin via GitGitGadget, git

Hi Junio & Philippe,

On Wed, 24 Aug 2022, Junio C Hamano wrote:

> Philippe Blain <levraiphilippeblain@gmail.com> writes:
>
> >>>> 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.
> >>
> >> 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. ...

Ah. Submodules. The gift that keeps on giving.

I'm not _really_ complaining, though. It seems that with beautiful
regularity, submodules come up as crucial parts of attack vectors in
exploits that are reported to the git-security mailing list, requiring
somebody with my abilities to implement fixes. Therefore, submodules award
me with a sort of job security. :-)

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

Yes, it was designed as that, with a few things done differently, though,
such as avoiding unnecessary `git diff` invocations, and using C-native
data structures and memory management.

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.

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

The built-in `add -p` does something slightly different from the Perl
version when it comes to the line ranges in hunk headers: instead of
storing stringified versions of them and parsing & re-rendering them when
the hunks are modified, the C version stores the actual numbers, adjusts
them as needed (e.g. when hunks are split), and, crucially, renders them
on the fly when displaying the hunks.

That means that also the colorized hunk headers are generated on the fly
whenever the hunks are displayed, and they are never stored in rendered
form, neither while parsing diffs nor when hunks are split or otherwise
edited. That's why the built-in `add -p` looked for a `@@ ... @@` part in
the colorized diff, so that it can display the remainder of that line
after showing the rendered line range.

And yes, this is a deviation from the bug-for-bug principle I follow for
conversions from scripted commands to built-in C code, but it was for a
good reason: memory management in C is less trivial than in Perl (where
one essentially YOLOs memory management), and I was not prepared to invent
a rope data structure just to be able to replace parts of a long string
buffer with rendered text of a different length, nor was I prepared to
call `memmove()` tons of times.

Note that the Perl version does something slightly iffy, too: if a diff
colorizer is configured, it initially shows the original hunk headers
(e.g. "@ file:16 @" in the `diff-so-fancy` case) but when the hunk is
modified in any way, it generates "@@ -1,6 +1,7 @@"-style hunk headers and
does not show the ones generated by the diff colorizer _at all_ anymore.

In this patch series, I teach the built-in `add -p` to be more lenient and
simply show the entire original hunk header after the rendered line range
if no `@@ ... @@` part was found. If that part was found, we still replace
it with the rendered line range.

Yes, it is a deviation from what the Perl version does. I consider it an
improvement, though.

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

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

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 67+ messages in thread

* [PATCH v2 0/4] built-in add -p: support diff-so-fancy better
  2022-08-23 18:04 [PATCH 0/3] built-in add -p: support diff-so-fancy better Johannes Schindelin via GitGitGadget
                   ` (3 preceding siblings ...)
  2022-08-24  3:49 ` [PATCH 0/3] built-in add -p: support diff-so-fancy better Philippe Blain
@ 2022-08-24 21:21 ` Johannes Schindelin via GitGitGadget
  2022-08-24 21:21   ` [PATCH v2 1/4] t3701: redefine what is "bogus" output of a diff filter Johannes Schindelin via GitGitGadget
                     ` (6 more replies)
  4 siblings, 7 replies; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-24 21:21 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Johannes Schindelin

Philippe Blain reported in
https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com
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

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 67+ messages in thread

* [PATCH v2 1/4] t3701: redefine what is "bogus" output of a diff filter
  2022-08-24 21:21 ` [PATCH v2 0/4] " Johannes Schindelin via GitGitGadget
@ 2022-08-24 21:21   ` Johannes Schindelin via GitGitGadget
  2022-08-24 21:21   ` [PATCH v2 2/4] add -p: gracefully ignore unparseable hunk headers in colored diffs Johannes Schindelin via GitGitGadget
                     ` (5 subsequent siblings)
  6 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-24 21:21 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 t/t3701-add-interactive.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index b354fb39de8..b40d1c94d99 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -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
 '
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH v2 2/4] add -p: gracefully ignore unparseable hunk headers in colored diffs
  2022-08-24 21:21 ` [PATCH v2 0/4] " Johannes Schindelin via GitGitGadget
  2022-08-24 21:21   ` [PATCH v2 1/4] t3701: redefine what is "bogus" output of a diff filter Johannes Schindelin via GitGitGadget
@ 2022-08-24 21:21   ` Johannes Schindelin via GitGitGadget
  2022-08-29  7:56     ` Junio C Hamano
  2022-08-24 21:21   ` [PATCH v2 3/4] add -p: handle `diff-so-fancy`'s hunk headers better Johannes Schindelin via GitGitGadget
                     ` (4 subsequent siblings)
  6 siblings, 1 reply; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-24 21:21 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In
https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com,
Phillipe Blain reported that the built-in `git add -p` command fails
when asked to use [`diff-so-fancy`][diff-so-fancy] to colorize the diff.

The reason is that this tool produces colored diffs with a hunk header
that does not contain any parseable `@@ ... @@` line range information,
and therefore we cannot detect any part in that header that comes after
the line range.

Let's punt for now and simply show nothing apart from the line range in
that case.

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c                | 15 ++++++---------
 t/t3701-add-interactive.sh |  9 +++++++++
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 509ca04456b..f2fffe1af02 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -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'"),
-			     (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 */
+		header->colored_extra_start = hunk->colored_start;
 	header->colored_extra_end = hunk->colored_start;
 
 	return 0;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index b40d1c94d99..7e3c1de71f5 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -766,6 +766,15 @@ 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' '
+	git reset --hard &&
+
+	echo content >test &&
+	test_config interactive.diffFilter "sed s/@@/XX/g" &&
+	printf y >y &&
+	force_color git add -p <y
+'
+
 test_expect_success 'diff.algorithm is passed to `git diff-files`' '
 	git reset --hard &&
 
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH v2 3/4] add -p: handle `diff-so-fancy`'s hunk headers better
  2022-08-24 21:21 ` [PATCH v2 0/4] " Johannes Schindelin via GitGitGadget
  2022-08-24 21:21   ` [PATCH v2 1/4] t3701: redefine what is "bogus" output of a diff filter Johannes Schindelin via GitGitGadget
  2022-08-24 21:21   ` [PATCH v2 2/4] add -p: gracefully ignore unparseable hunk headers in colored diffs Johannes Schindelin via GitGitGadget
@ 2022-08-24 21:21   ` Johannes Schindelin via GitGitGadget
  2022-08-29  8:06     ` Junio C Hamano
  2022-08-24 21:21   ` [PATCH v2 4/4] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
                     ` (3 subsequent siblings)
  6 siblings, 1 reply; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-24 21:21 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 add-patch.c                | 8 +++++++-
 t/t3701-add-interactive.sh | 3 ++-
 2 files changed, 9 insertions(+), 2 deletions(-)

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`' '
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH v2 4/4] add -p: ignore dirty submodules
  2022-08-24 21:21 ` [PATCH v2 0/4] " Johannes Schindelin via GitGitGadget
                     ` (2 preceding siblings ...)
  2022-08-24 21:21   ` [PATCH v2 3/4] add -p: handle `diff-so-fancy`'s hunk headers better Johannes Schindelin via GitGitGadget
@ 2022-08-24 21:21   ` Johannes Schindelin via GitGitGadget
  2022-08-24 22:11   ` [PATCH v2 0/4] built-in add -p: support diff-so-fancy better Junio C Hamano
                     ` (2 subsequent siblings)
  6 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-24 21:21 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 add-patch.c                |  3 ++-
 t/t3701-add-interactive.sh | 12 ++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/add-patch.c b/add-patch.c
index 1f3f3611ee9..64807e7cdc0 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -417,7 +417,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);
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 9deb7a87f1e..10058b7ce42 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -941,6 +941,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 &&
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 67+ messages in thread

* Re: [PATCH 0/3] built-in add -p: support diff-so-fancy better
  2022-08-24 21:05           ` Johannes Schindelin
@ 2022-08-24 21:37             ` Junio C Hamano
  0 siblings, 0 replies; 67+ messages in thread
From: Junio C Hamano @ 2022-08-24 21:37 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Philippe Blain, Johannes Schindelin via GitGitGadget, git

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

OK, so it wasn't removing what is useless, but removing something
whose result was used.  And our next move is not to bring us closer
to how we used to correctly handle third-party diff filters by
mimicking the original better but doing the handling of this
particular piece differently.

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

OK.

>> 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".
>
> Indeed, and thank you, Philippe, for bringing this to the Git mailing list
> so that I could do something about the bug.

Yup.

To some people, "refatoring to match my subjective code aesthetics"
may be personally the most important, and to some other people,
"getting rid of scripted Porcelains" may be, but they are both wrong
and these are their second most important goals ;-)  Not regressing
end-user experience is, and we learned a valuable lesson here.  Be
conservative and make sure we can revert to buy time when making a
big rewrite like this one.

Thanks, all.

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v2 0/4] built-in add -p: support diff-so-fancy better
  2022-08-24 21:21 ` [PATCH v2 0/4] " Johannes Schindelin via GitGitGadget
                     ` (3 preceding siblings ...)
  2022-08-24 21:21   ` [PATCH v2 4/4] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
@ 2022-08-24 22:11   ` Junio C Hamano
  2022-08-25  0:18   ` Philippe Blain
  2022-08-29 15:11   ` [PATCH v3 0/5] " Johannes Schindelin via GitGitGadget
  6 siblings, 0 replies; 67+ messages in thread
From: Junio C Hamano @ 2022-08-24 22:11 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Philippe Blain, Johannes Schindelin

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

> Philippe Blain reported in
> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com
> 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.

Thanks, all.  Will queue, but it may miss this afternoon's
integration I am preparing right now for pushing out.


^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v2 0/4] built-in add -p: support diff-so-fancy better
  2022-08-24 21:21 ` [PATCH v2 0/4] " Johannes Schindelin via GitGitGadget
                     ` (4 preceding siblings ...)
  2022-08-24 22:11   ` [PATCH v2 0/4] built-in add -p: support diff-so-fancy better Junio C Hamano
@ 2022-08-25  0:18   ` Philippe Blain
  2022-08-26 11:43     ` Johannes Schindelin
  2022-08-29 15:11   ` [PATCH v3 0/5] " Johannes Schindelin via GitGitGadget
  6 siblings, 1 reply; 67+ messages in thread
From: Philippe Blain @ 2022-08-25  0:18 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget, git; +Cc: Johannes Schindelin

Hi Dscho,

Le 2022-08-24 à 17:21, Johannes Schindelin via GitGitGadget a écrit :
> Philippe Blain reported in
> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com
> 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
> 

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

Thanks again for the quick fixes,

Philippe.

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v2 0/4] built-in add -p: support diff-so-fancy better
  2022-08-25  0:18   ` Philippe Blain
@ 2022-08-26 11:43     ` Johannes Schindelin
  2022-08-26 23:15       ` Philippe Blain
  0 siblings, 1 reply; 67+ messages in thread
From: Johannes Schindelin @ 2022-08-26 11:43 UTC (permalink / raw)
  To: Philippe Blain; +Cc: Johannes Schindelin via GitGitGadget, git

[-- Attachment #1: Type: text/plain, Size: 2533 bytes --]

Hi Philippe,

On Wed, 24 Aug 2022, Philippe Blain wrote:

> Le 2022-08-24 à 17:21, Johannes Schindelin via GitGitGadget a écrit :
> > Philippe Blain reported in
> > https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com
> > 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
> >
>
> 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 :)

Good suggestion.

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

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v2 0/4] built-in add -p: support diff-so-fancy better
  2022-08-26 11:43     ` Johannes Schindelin
@ 2022-08-26 23:15       ` Philippe Blain
  0 siblings, 0 replies; 67+ messages in thread
From: Philippe Blain @ 2022-08-26 23:15 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Johannes Schindelin via GitGitGadget, git

Hi Dscho,

Le 2022-08-26 à 07:43, Johannes Schindelin a écrit :
> Hi Philippe,
> 
> On Wed, 24 Aug 2022, Philippe Blain wrote:
> 
>> Le 2022-08-24 à 17:21, Johannes Schindelin via GitGitGadget a écrit :
>>> Philippe Blain reported in
>>> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com
>>> 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
>>>
>>
>> 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 :)
> 
> Good suggestion.
> 
> How about adding your review? I'll then add a "Reviewed-by:" trailer, too
> ;-)

I won't have time to look at the code changes themselves, and I've never looked 
at the builtin add -p code (nor the original Perl one!) so it would take some time.

Cheers,
Philippe.

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v2 2/4] add -p: gracefully ignore unparseable hunk headers in colored diffs
  2022-08-24 21:21   ` [PATCH v2 2/4] add -p: gracefully ignore unparseable hunk headers in colored diffs Johannes Schindelin via GitGitGadget
@ 2022-08-29  7:56     ` Junio C Hamano
  0 siblings, 0 replies; 67+ messages in thread
From: Junio C Hamano @ 2022-08-29  7:56 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Philippe Blain, Johannes Schindelin

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

> diff --git a/add-patch.c b/add-patch.c
> index 509ca04456b..f2fffe1af02 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -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'"),
> -			     (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 */
> +		header->colored_extra_start = hunk->colored_start;
>  	header->colored_extra_end = hunk->colored_start;

OK.  We keep two copies s->plain and s->colored of the line in the
patch, and the real information on the hunk header has already been
parsed out of the "plain" version.

Here, w want to find the end of "@@ -<range>, +<range> @@"" on the
colored variant, presumably because that is where the "funcname"
may appear and more useful pieces of information can be gleaned.

The original code assumed that we should be able to find such a
place (after all, we successfully parsed the "corresponding" plain
version to find it) and threw an error otherwise.  Otherwise,

 * hunk->colored_start is set to the beginning of the next line.

 * header->colored_extra_start is set to the byte after the " @@",
   i.e. where the "funcname" ought to begin.

 * header->colored_extra_end is set to the beginning of the next
   line (i.e. "funcname" is the remainder of the line starting at
   "colored_extra_start").

The new code loosens the requirement that colored one has the same
hunk header on the line (presumably prefixed by some color codes).
When the colored line does not look like a hunk header and we cannot
tell where the "funcname" part begins, we can pretend that the entire
line was just the hunk header without "funcname" part by setting the
extra_start and extra_end both at the end of the line.

There is nothing "structurally" important on the "funcname" part, so
presumably we will only use the information to show to the display
and nothing else, so I think this is perfectly OK to do.

Another obvious possibility is to show everything on that
unparseable line.  We know it corresponds to the hunk header in the
"real" patch (i.e. s->plain), and it may have something human can
understand.  Perhaps that may be changed in the later part of this
series (I haven't looked, but if that happens "showing nothing"
comment needs to be updated).

So far, looking good.


^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v2 3/4] add -p: handle `diff-so-fancy`'s hunk headers better
  2022-08-24 21:21   ` [PATCH v2 3/4] add -p: handle `diff-so-fancy`'s hunk headers better Johannes Schindelin via GitGitGadget
@ 2022-08-29  8:06     ` Junio C Hamano
  2022-08-29 13:32       ` Johannes Schindelin
  0 siblings, 1 reply; 67+ messages in thread
From: Junio C Hamano @ 2022-08-29  8:06 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Philippe Blain, Johannes Schindelin

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

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v2 3/4] add -p: handle `diff-so-fancy`'s hunk headers better
  2022-08-29  8:06     ` Junio C Hamano
@ 2022-08-29 13:32       ` Johannes Schindelin
  2022-08-29 17:19         ` Junio C Hamano
  0 siblings, 1 reply; 67+ messages in thread
From: Johannes Schindelin @ 2022-08-29 13:32 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain

Hi Junio,

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

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

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

^ permalink raw reply	[flat|nested] 67+ messages in thread

* [PATCH v3 0/5] built-in add -p: support diff-so-fancy better
  2022-08-24 21:21 ` [PATCH v2 0/4] " Johannes Schindelin via GitGitGadget
                     ` (5 preceding siblings ...)
  2022-08-25  0:18   ` Philippe Blain
@ 2022-08-29 15:11   ` Johannes Schindelin via GitGitGadget
  2022-08-29 15:11     ` [PATCH v3 1/5] t3701: redefine what is "bogus" output of a diff filter Johannes Schindelin via GitGitGadget
                       ` (7 more replies)
  6 siblings, 8 replies; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-29 15:11 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Johannes Schindelin

Philippe Blain reported in
https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com
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 <levraiphilippeblain@gmail.com>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## 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 <Johannes.Schindelin@gmx.de>
      
       ## 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 <johannes.schindelin@gmx.de>
      
     @@ 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

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 67+ messages in thread

* [PATCH v3 1/5] t3701: redefine what is "bogus" output of a diff filter
  2022-08-29 15:11   ` [PATCH v3 0/5] " Johannes Schindelin via GitGitGadget
@ 2022-08-29 15:11     ` Johannes Schindelin via GitGitGadget
  2022-08-30 13:17       ` Phillip Wood
  2022-08-29 15:11     ` [PATCH v3 2/5] add -p: gracefully ignore unparseable hunk headers in colored diffs Johannes Schindelin via GitGitGadget
                       ` (6 subsequent siblings)
  7 siblings, 1 reply; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-29 15:11 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 t/t3701-add-interactive.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 3b7df9bed5a..88d8133f38f 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -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
 '
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH v3 2/5] add -p: gracefully ignore unparseable hunk headers in colored diffs
  2022-08-29 15:11   ` [PATCH v3 0/5] " Johannes Schindelin via GitGitGadget
  2022-08-29 15:11     ` [PATCH v3 1/5] t3701: redefine what is "bogus" output of a diff filter Johannes Schindelin via GitGitGadget
@ 2022-08-29 15:11     ` Johannes Schindelin via GitGitGadget
  2022-08-29 15:11     ` [PATCH v3 3/5] add -p: insert space in colored hunk header as needed Johannes Schindelin via GitGitGadget
                       ` (5 subsequent siblings)
  7 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-29 15:11 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In
https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com,
Phillipe Blain reported that the built-in `git add -p` command fails
when asked to use [`diff-so-fancy`][diff-so-fancy] to colorize the diff.

The reason is that this tool produces colored diffs with a hunk header
that does not contain any parseable `@@ ... @@` line range information,
and therefore we cannot detect any part in that header that comes after
the line range.

Let's punt for now and simply show nothing apart from the line range in
that case.

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

Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c                | 15 ++++++---------
 t/t3701-add-interactive.sh |  9 +++++++++
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 509ca04456b..f2fffe1af02 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -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'"),
-			     (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 */
+		header->colored_extra_start = hunk->colored_start;
 	header->colored_extra_end = hunk->colored_start;
 
 	return 0;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 88d8133f38f..c2187f9cec8 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -766,6 +766,15 @@ test_expect_success 'detect bogus diffFilter output' '
 	force_color test_must_fail git add -p <y
 '
 
+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/g" \
+		add -p <n
+'
+
 test_expect_success 'handle very large filtered diff' '
 	git reset --hard &&
 	# The specific number here is not important, but it must
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH v3 3/5] add -p: insert space in colored hunk header as needed
  2022-08-29 15:11   ` [PATCH v3 0/5] " Johannes Schindelin via GitGitGadget
  2022-08-29 15:11     ` [PATCH v3 1/5] t3701: redefine what is "bogus" output of a diff filter Johannes Schindelin via GitGitGadget
  2022-08-29 15:11     ` [PATCH v3 2/5] add -p: gracefully ignore unparseable hunk headers in colored diffs Johannes Schindelin via GitGitGadget
@ 2022-08-29 15:11     ` Johannes Schindelin via GitGitGadget
  2022-08-29 15:11     ` [PATCH v3 4/5] add -p: handle `diff-so-fancy`'s hunk headers better Johannes Schindelin via GitGitGadget
                       ` (4 subsequent siblings)
  7 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-29 15:11 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

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 <johannes.schindelin@gmx.de>
---
 add-patch.c                | 22 ++++++++++++++++++++++
 t/t3701-add-interactive.sh | 10 +++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/add-patch.c b/add-patch.c
index f2fffe1af02..9d575d30ed0 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,
@@ -635,6 +636,23 @@ static size_t find_next_line(struct strbuf *sb, size_t offset)
 	return eol - sb->buf + 1;
 }
 
+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)
 {
@@ -649,6 +667,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 +677,7 @@ 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;
+			needs_extra_space = starts_with_non_ws(p, len);
 		}
 
 		if (s->mode->is_reverse)
@@ -673,6 +693,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 c2187f9cec8..49200b7df68 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -772,7 +772,15 @@ test_expect_success 'handle iffy colored hunk headers' '
 	echo content >test &&
 	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 'handle very large filtered diff' '
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH v3 4/5] add -p: handle `diff-so-fancy`'s hunk headers better
  2022-08-29 15:11   ` [PATCH v3 0/5] " Johannes Schindelin via GitGitGadget
                       ` (2 preceding siblings ...)
  2022-08-29 15:11     ` [PATCH v3 3/5] add -p: insert space in colored hunk header as needed Johannes Schindelin via GitGitGadget
@ 2022-08-29 15:11     ` Johannes Schindelin via GitGitGadget
  2022-08-30 13:23       ` Phillip Wood
  2022-08-29 15:11     ` [PATCH v3 5/5] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
                       ` (3 subsequent siblings)
  7 siblings, 1 reply; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-29 15:11 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 add-patch.c                | 13 +++++++++++--
 t/t3701-add-interactive.sh |  4 ++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 9d575d30ed0..0217cdd7c4a 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -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;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 49200b7df68..39e68b6d066 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -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
 '
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH v3 5/5] add -p: ignore dirty submodules
  2022-08-29 15:11   ` [PATCH v3 0/5] " Johannes Schindelin via GitGitGadget
                       ` (3 preceding siblings ...)
  2022-08-29 15:11     ` [PATCH v3 4/5] add -p: handle `diff-so-fancy`'s hunk headers better Johannes Schindelin via GitGitGadget
@ 2022-08-29 15:11     ` Johannes Schindelin via GitGitGadget
  2022-08-30 13:26       ` Phillip Wood
  2022-08-29 18:01     ` [PATCH v3 0/5] built-in add -p: support diff-so-fancy better Junio C Hamano
                       ` (2 subsequent siblings)
  7 siblings, 1 reply; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-29 15:11 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 add-patch.c                |  3 ++-
 t/t3701-add-interactive.sh | 12 ++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/add-patch.c b/add-patch.c
index 0217cdd7c4a..ee6a3d3b712 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -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);
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 39e68b6d066..a4f45fc48a0 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -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 &&
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 67+ messages in thread

* Re: [PATCH v2 3/4] add -p: handle `diff-so-fancy`'s hunk headers better
  2022-08-29 13:32       ` Johannes Schindelin
@ 2022-08-29 17:19         ` Junio C Hamano
  2022-08-30 14:14           ` Johannes Schindelin
  0 siblings, 1 reply; 67+ messages in thread
From: Junio C Hamano @ 2022-08-29 17:19 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 0/5] built-in add -p: support diff-so-fancy better
  2022-08-29 15:11   ` [PATCH v3 0/5] " Johannes Schindelin via GitGitGadget
                       ` (4 preceding siblings ...)
  2022-08-29 15:11     ` [PATCH v3 5/5] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
@ 2022-08-29 18:01     ` Junio C Hamano
  2022-08-30 14:22       ` Johannes Schindelin
  2022-08-30 13:29     ` Phillip Wood
  2022-08-31 20:31     ` [PATCH v4 0/3] " Johannes Schindelin via GitGitGadget
  7 siblings, 1 reply; 67+ messages in thread
From: Junio C Hamano @ 2022-08-29 18:01 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Philippe Blain, Johannes Schindelin

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
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.


>  * 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, especially when it is
something simple (like this case, where both sides added a new test
piece each in a way that semantically do not conflict).  I'll ask
the author to rebase if the conflicts becomes unmanageable.

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.

Thanks.

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 1/5] t3701: redefine what is "bogus" output of a diff filter
  2022-08-29 15:11     ` [PATCH v3 1/5] t3701: redefine what is "bogus" output of a diff filter Johannes Schindelin via GitGitGadget
@ 2022-08-30 13:17       ` Phillip Wood
  2022-08-30 21:36         ` Junio C Hamano
  0 siblings, 1 reply; 67+ messages in thread
From: Phillip Wood @ 2022-08-30 13:17 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget, git
  Cc: Philippe Blain, Johannes Schindelin, Junio C Hamano

Hi Dscho

On 29/08/2022 16:11, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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.

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.

Best Wishes

Phillip

[1] To verify this I changed the filter to  "sed 1d | tee 
filtered-diff". filtered diff-contains
index 0889435..d95f3ad 100644

--- a/test

+++ b/test

@@ -1,6 +1 @@

-10

-20

-30

-40

-50

-60

+content



> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   t/t3701-add-interactive.sh | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 3b7df9bed5a..88d8133f38f 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -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
>   '

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 4/5] add -p: handle `diff-so-fancy`'s hunk headers better
  2022-08-29 15:11     ` [PATCH v3 4/5] add -p: handle `diff-so-fancy`'s hunk headers better Johannes Schindelin via GitGitGadget
@ 2022-08-30 13:23       ` Phillip Wood
  0 siblings, 0 replies; 67+ messages in thread
From: Phillip Wood @ 2022-08-30 13:23 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget, git
  Cc: Philippe Blain, Johannes Schindelin

Hi Dscho

On 29/08/2022 16:11, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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.

Looking at the tests, I don't think we just show the complete hunk 
header, we show the offsets from the unfiltered diff as well. I think 
that is unfortunate as it kind of defeats the purpose of 
interactive.diffFilter which is to the the user see the diff how they 
want to (so long as it has the same number of lines)

Best Wishes

Phillip

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   add-patch.c                | 13 +++++++++++--
>   t/t3701-add-interactive.sh |  4 ++++
>   2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/add-patch.c b/add-patch.c
> index 9d575d30ed0..0217cdd7c4a 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -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;
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 49200b7df68..39e68b6d066 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -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
>   '

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 5/5] add -p: ignore dirty submodules
  2022-08-29 15:11     ` [PATCH v3 5/5] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
@ 2022-08-30 13:26       ` Phillip Wood
  2022-08-31 20:05         ` Johannes Schindelin
  0 siblings, 1 reply; 67+ messages in thread
From: Phillip Wood @ 2022-08-30 13:26 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget, git
  Cc: Philippe Blain, Johannes Schindelin, Junio C Hamano

Hi Dscho

On 29/08/2022 16:11, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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.

Best Wishes

Phillip

> 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 <johannes.schindelin@gmx.de>
> ---
>   add-patch.c                |  3 ++-
>   t/t3701-add-interactive.sh | 12 ++++++++++++
>   2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/add-patch.c b/add-patch.c
> index 0217cdd7c4a..ee6a3d3b712 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -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);
>   
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 39e68b6d066..a4f45fc48a0 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -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 &&

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 0/5] built-in add -p: support diff-so-fancy better
  2022-08-29 15:11   ` [PATCH v3 0/5] " Johannes Schindelin via GitGitGadget
                       ` (5 preceding siblings ...)
  2022-08-29 18:01     ` [PATCH v3 0/5] built-in add -p: support diff-so-fancy better Junio C Hamano
@ 2022-08-30 13:29     ` Phillip Wood
  2022-08-31 20:44       ` Johannes Schindelin
  2022-08-31 20:31     ` [PATCH v4 0/3] " Johannes Schindelin via GitGitGadget
  7 siblings, 1 reply; 67+ messages in thread
From: Phillip Wood @ 2022-08-30 13:29 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget, git
  Cc: Philippe Blain, Johannes Schindelin, Junio C Hamano

Hi Dscho

Thanks for working on this, I've left a few comments on the patches - 
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.

Best Wishes

Phillip

On 29/08/2022 16:11, Johannes Schindelin via GitGitGadget wrote:
> Philippe Blain reported in
> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com
> 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 <levraiphilippeblain@gmail.com>
>            Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>        
>         ## 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 <Johannes.Schindelin@gmx.de>
>        
>         ## 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 <johannes.schindelin@gmx.de>
>        
>       @@ 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
> 

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v2 3/4] add -p: handle `diff-so-fancy`'s hunk headers better
  2022-08-29 17:19         ` Junio C Hamano
@ 2022-08-30 14:14           ` Johannes Schindelin
  0 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2022-08-30 14:14 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain

Hi Junio,

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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 0/5] built-in add -p: support diff-so-fancy better
  2022-08-29 18:01     ` [PATCH v3 0/5] built-in add -p: support diff-so-fancy better Junio C Hamano
@ 2022-08-30 14:22       ` Johannes Schindelin
  0 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2022-08-30 14:22 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain

Hi Junio,

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

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> 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.
>
>
> >  * 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, [...]

Noted. I just wanted to be nice.

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

Sounds good.

Thanks,
Dscho

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 1/5] t3701: redefine what is "bogus" output of a diff filter
  2022-08-30 13:17       ` Phillip Wood
@ 2022-08-30 21:36         ` Junio C Hamano
  2022-08-31  9:26           ` Phillip Wood
  0 siblings, 1 reply; 67+ messages in thread
From: Junio C Hamano @ 2022-08-30 21:36 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain,
	Johannes Schindelin

Phillip Wood <phillip.wood123@gmail.com> writes:

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


^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 1/5] t3701: redefine what is "bogus" output of a diff filter
  2022-08-30 21:36         ` Junio C Hamano
@ 2022-08-31  9:26           ` Phillip Wood
  2022-08-31 15:36             ` Jeff King
  0 siblings, 1 reply; 67+ messages in thread
From: Phillip Wood @ 2022-08-31  9:26 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain,
	Johannes Schindelin

Hi Junio

On 30/08/2022 22:36, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> 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 ;-)

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.

Best Wishes

Phillip

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

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 1/5] t3701: redefine what is "bogus" output of a diff filter
  2022-08-31  9:26           ` Phillip Wood
@ 2022-08-31 15:36             ` Jeff King
  2022-08-31 15:47               ` Jeff King
  0 siblings, 1 reply; 67+ messages in thread
From: Jeff King @ 2022-08-31 15:36 UTC (permalink / raw)
  To: phillip.wood
  Cc: Junio C Hamano, Johannes Schindelin via GitGitGadget, git,
	Philippe Blain, Johannes Schindelin

On Wed, Aug 31, 2022 at 10:26:17AM +0100, Phillip Wood wrote:

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

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/20200113170417.GK32750@szeder.dev/

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.

-Peff

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 1/5] t3701: redefine what is "bogus" output of a diff filter
  2022-08-31 15:36             ` Jeff King
@ 2022-08-31 15:47               ` Jeff King
  2022-08-31 19:57                 ` Johannes Schindelin
  0 siblings, 1 reply; 67+ messages in thread
From: Jeff King @ 2022-08-31 15:47 UTC (permalink / raw)
  To: phillip.wood
  Cc: Junio C Hamano, Johannes Schindelin via GitGitGadget, git,
	Philippe Blain, Johannes Schindelin

On Wed, Aug 31, 2022 at 11:36:19AM -0400, Jeff King wrote:

> > 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/20200113170417.GK32750@szeder.dev/
> 
> 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

-Peff

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 1/5] t3701: redefine what is "bogus" output of a diff filter
  2022-08-31 15:47               ` Jeff King
@ 2022-08-31 19:57                 ` Johannes Schindelin
  0 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2022-08-31 19:57 UTC (permalink / raw)
  To: Jeff King
  Cc: phillip.wood, Junio C Hamano,
	Johannes Schindelin via GitGitGadget, git, Philippe Blain

Hi Peff,

On Wed, 31 Aug 2022, Jeff King wrote:

> On Wed, Aug 31, 2022 at 11:36:19AM -0400, Jeff King wrote:
>
> > > 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/20200113170417.GK32750@szeder.dev/
> >
> > 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.

And that's exactly the case. It was an off-by-one bug ;-)

> 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

Yes, that matches my analysis, and I already pushed a new iteration before
even noticing your mail, only waiting for the CI run to finish before I
submit it.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 5/5] add -p: ignore dirty submodules
  2022-08-30 13:26       ` Phillip Wood
@ 2022-08-31 20:05         ` Johannes Schindelin
  2022-08-31 20:19           ` Junio C Hamano
  0 siblings, 1 reply; 67+ messages in thread
From: Johannes Schindelin @ 2022-08-31 20:05 UTC (permalink / raw)
  To: phillip.wood
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain,
	Junio C Hamano

Hi Phillip,

On Tue, 30 Aug 2022, Phillip Wood wrote:

> On 29/08/2022 16:11, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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.

Yes, the `--ignore-submodules=dirty` part is important, but so is the
`--numstat` part.

It is important because the Perl script would parse that numstat part and
only offer files up for further processing for which it saw those numbers.
Then it would go ahead and run a full diff on those files, which basically
doubled the work.

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.

> The fix itself looks good.

Thanks!
Dscho

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 5/5] add -p: ignore dirty submodules
  2022-08-31 20:05         ` Johannes Schindelin
@ 2022-08-31 20:19           ` Junio C Hamano
  2022-08-31 20:38             ` Johannes Schindelin
  0 siblings, 1 reply; 67+ messages in thread
From: Junio C Hamano @ 2022-08-31 20:19 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: phillip.wood, Johannes Schindelin via GitGitGadget, git, Philippe Blain

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

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

Thanks.

^ permalink raw reply	[flat|nested] 67+ messages in thread

* [PATCH v4 0/3] built-in add -p: support diff-so-fancy better
  2022-08-29 15:11   ` [PATCH v3 0/5] " Johannes Schindelin via GitGitGadget
                       ` (6 preceding siblings ...)
  2022-08-30 13:29     ` Phillip Wood
@ 2022-08-31 20:31     ` Johannes Schindelin via GitGitGadget
  2022-08-31 20:31       ` [PATCH v4 1/3] add -p: detect more mismatches between plain vs colored diffs Johannes Schindelin via GitGitGadget
                         ` (4 more replies)
  7 siblings, 5 replies; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-31 20:31 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Phillip Wood, Jeff King, Johannes Schindelin

Philippe Blain reported in
https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com
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 <Johannes.Schindelin@gmx.de>
      
       ## 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 <johannes.schindelin@gmx.de>
      
     + ## 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 <Johannes.Schindelin@gmx.de>
      
       ## 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/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com,
     @@ 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 <levraiphilippeblain@gmail.com>
     +    Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## 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

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 67+ messages in thread

* [PATCH v4 1/3] add -p: detect more mismatches between plain vs colored diffs
  2022-08-31 20:31     ` [PATCH v4 0/3] " Johannes Schindelin via GitGitGadget
@ 2022-08-31 20:31       ` Johannes Schindelin via GitGitGadget
  2022-09-01 13:19         ` Phillip Wood
  2022-08-31 20:31       ` [PATCH v4 2/3] add -p: gracefully handle unparseable hunk headers in " Johannes Schindelin via GitGitGadget
                         ` (3 subsequent siblings)
  4 siblings, 1 reply; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-31 20:31 UTC (permalink / raw)
  To: git
  Cc: Philippe Blain, Phillip Wood, Jeff King, Johannes Schindelin,
	Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 add-patch.c                | 5 ++++-
 t/t3701-add-interactive.sh | 5 +++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 509ca04456b..34f3807ff32 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -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;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 3b7df9bed5a..8a594700f7b 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -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' '
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH v4 2/3] add -p: gracefully handle unparseable hunk headers in colored diffs
  2022-08-31 20:31     ` [PATCH v4 0/3] " Johannes Schindelin via GitGitGadget
  2022-08-31 20:31       ` [PATCH v4 1/3] add -p: detect more mismatches between plain vs colored diffs Johannes Schindelin via GitGitGadget
@ 2022-08-31 20:31       ` Johannes Schindelin via GitGitGadget
  2022-09-01 13:53         ` Phillip Wood
  2022-08-31 20:31       ` [PATCH v4 3/3] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
                         ` (2 subsequent siblings)
  4 siblings, 1 reply; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-31 20:31 UTC (permalink / raw)
  To: git
  Cc: Philippe Blain, Phillip Wood, Jeff King, Johannes Schindelin,
	Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In
https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com,
Phillipe Blain reported that the built-in `git add -p` command fails
when asked to use [`diff-so-fancy`][diff-so-fancy] to colorize the diff.

The reason is that this tool produces colored diffs with a hunk header
that does not contain any parseable `@@ ... @@` line range information,
and therefore we cannot detect any part in that header that comes after
the line range.

As proposed by Phillip Wood, let's take that for a clear indicator that
we should show the hunk headers verbatim. This is what the Perl version
of the interactive `add` command did, too.

This commit is best viewed with `--color-moved --ignore-space-change`.

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

Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c                | 42 ++++++++++++++++++++------------------
 t/t3701-add-interactive.sh | 10 +++++++++
 2 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 34f3807ff32..3699ca1fcaf 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -238,6 +238,7 @@ struct hunk_header {
 	 * include the newline.
 	 */
 	size_t extra_start, extra_end, colored_extra_start, colored_extra_end;
+	unsigned suppress_colored_line_range:1;
 };
 
 struct hunk {
@@ -358,15 +359,14 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)
 	if (!eol)
 		eol = s->colored.buf + s->colored.len;
 	p = memmem(line, eol - line, "@@ -", 4);
-	if (!p)
-		return error(_("could not parse colored hunk header '%.*s'"),
-			     (int)(eol - line), line);
-	p = memmem(p + 4, eol - p - 4, " @@", 3);
-	if (!p)
-		return error(_("could not parse colored hunk header '%.*s'"),
-			     (int)(eol - line), line);
+	if (p && (p = memmem(p + 4, eol - p - 4, " @@", 3)))
+		header->colored_extra_start = p + 3 - s->colored.buf;
+	else {
+		/* could not parse colored hunk header, leave as-is */
+		header->colored_extra_start = hunk->colored_start;
+		header->suppress_colored_line_range = 1;
+	}
 	hunk->colored_start = eol - s->colored.buf + (*eol == '\n');
-	header->colored_extra_start = p + 3 - s->colored.buf;
 	header->colored_extra_end = hunk->colored_start;
 
 	return 0;
@@ -666,18 +666,20 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
 				- 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);
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 8a594700f7b..47ed6698943 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -767,6 +767,16 @@ test_expect_success 'detect bogus diffFilter output' '
 	grep "mismatched output" output
 '
 
+test_expect_success 'handle iffy colored hunk headers' '
+	git reset --hard &&
+
+	echo content >test &&
+	printf n >n &&
+	force_color git -c interactive.diffFilter="sed s/.*@@.*/XX/" \
+		add -p >output 2>&1 <n &&
+	grep "^[^@]*XX[^@]*$" output
+'
+
 test_expect_success 'handle very large filtered diff' '
 	git reset --hard &&
 	# The specific number here is not important, but it must
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH v4 3/3] add -p: ignore dirty submodules
  2022-08-31 20:31     ` [PATCH v4 0/3] " Johannes Schindelin via GitGitGadget
  2022-08-31 20:31       ` [PATCH v4 1/3] add -p: detect more mismatches between plain vs colored diffs Johannes Schindelin via GitGitGadget
  2022-08-31 20:31       ` [PATCH v4 2/3] add -p: gracefully handle unparseable hunk headers in " Johannes Schindelin via GitGitGadget
@ 2022-08-31 20:31       ` Johannes Schindelin via GitGitGadget
  2022-09-01 15:45         ` Jeff King
  2022-09-01 16:17         ` Junio C Hamano
  2022-09-01 13:55       ` [PATCH v4 0/3] built-in add -p: support diff-so-fancy better Phillip Wood
  2022-09-01 15:42       ` [PATCH v5 " Johannes Schindelin via GitGitGadget
  4 siblings, 2 replies; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-31 20:31 UTC (permalink / raw)
  To: git
  Cc: Philippe Blain, Phillip Wood, Jeff King, Johannes Schindelin,
	Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 add-patch.c                |  3 ++-
 t/t3701-add-interactive.sh | 12 ++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/add-patch.c b/add-patch.c
index 3699ca1fcaf..c7eebe9de89 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -419,7 +419,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);
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 47ed6698943..2d710645719 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -955,6 +955,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 &&
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 5/5] add -p: ignore dirty submodules
  2022-08-31 20:19           ` Junio C Hamano
@ 2022-08-31 20:38             ` Johannes Schindelin
  0 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2022-08-31 20:38 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: phillip.wood, Johannes Schindelin via GitGitGadget, git, Philippe Blain

Hi Junio,

On Wed, 31 Aug 2022, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > 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.
>
> Yup, the moral of the story is premature optimization bites because
> we are not always careful ;-)

Yes. Maybe even more generally: trying to do too many things at once is
prone to introduce bugs.

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

Yep!

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

Unfortunately so.

It is a fine line, and not always possible to tread safely, between trying
to stay close to the original and trying to learn the lesson from Elijah
that converting scripts to C without playing to C's strengths causes a lot
of hiccups later on. There are probably better alternatives for
prototyping Git functionality than using either an untyped language that
cannot hold complex data structures in memory or a language to which
everything looks like a map ;-)

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 0/5] built-in add -p: support diff-so-fancy better
  2022-08-30 13:29     ` Phillip Wood
@ 2022-08-31 20:44       ` Johannes Schindelin
  0 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2022-08-31 20:44 UTC (permalink / raw)
  To: phillip.wood
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain,
	Junio C Hamano

Hi Phillip,

On Tue, 30 Aug 2022, Phillip Wood wrote:

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

The fourth iteration of this patch series that I just sent out should
address both concerns.

Thank you!
Dscho

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v4 1/3] add -p: detect more mismatches between plain vs colored diffs
  2022-08-31 20:31       ` [PATCH v4 1/3] add -p: detect more mismatches between plain vs colored diffs Johannes Schindelin via GitGitGadget
@ 2022-09-01 13:19         ` Phillip Wood
  0 siblings, 0 replies; 67+ messages in thread
From: Phillip Wood @ 2022-09-01 13:19 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget, git
  Cc: Philippe Blain, Jeff King, Johannes Schindelin

Hi Dscho

On 31/08/2022 21:31, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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.

Thanks for re-rolling, this looks good. The commit message explains the 
problem well and the fix is good, I especially like the fact that you've 
added a grep for the correct error message.

Related to this we also have code that detects over-long output from the 
filter, I'm not sure if we have a test for that but I think the 
implementation looks ok.

Best Wishes

Phillip

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   add-patch.c                | 5 ++++-
>   t/t3701-add-interactive.sh | 5 +++--
>   2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/add-patch.c b/add-patch.c
> index 509ca04456b..34f3807ff32 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -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;
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 3b7df9bed5a..8a594700f7b 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -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' '

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v4 2/3] add -p: gracefully handle unparseable hunk headers in colored diffs
  2022-08-31 20:31       ` [PATCH v4 2/3] add -p: gracefully handle unparseable hunk headers in " Johannes Schindelin via GitGitGadget
@ 2022-09-01 13:53         ` Phillip Wood
  2022-09-01 15:09           ` Johannes Schindelin
  0 siblings, 1 reply; 67+ messages in thread
From: Phillip Wood @ 2022-09-01 13:53 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget, git
  Cc: Philippe Blain, Jeff King, Johannes Schindelin

Hi Dscho

On 31/08/2022 21:31, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> In
> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com,
> Phillipe Blain reported that the built-in `git add -p` command fails
> when asked to use [`diff-so-fancy`][diff-so-fancy] to colorize the diff.
> 
> The reason is that this tool produces colored diffs with a hunk header
> that does not contain any parseable `@@ ... @@` line range information,
> and therefore we cannot detect any part in that header that comes after
> the line range.
> 
> As proposed by Phillip Wood, let's take that for a clear indicator that
> we should show the hunk headers verbatim. This is what the Perl version
> of the interactive `add` command did, too.
> 
> This commit is best viewed with `--color-moved --ignore-space-change`.

or --color-moved-ws=allow-indentation-change

This looks pretty good, unsurprisingly I like the fact that the output 
now matches the perl version. I was confused by the grep in the test 
which lead me to realize we're printing an color escape code when we 
shouldn't.

> [diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy
> 
> Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   add-patch.c                | 42 ++++++++++++++++++++------------------
>   t/t3701-add-interactive.sh | 10 +++++++++
>   2 files changed, 32 insertions(+), 20 deletions(-)
> 
> diff --git a/add-patch.c b/add-patch.c
> index 34f3807ff32..3699ca1fcaf 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -238,6 +238,7 @@ struct hunk_header {
>   	 * include the newline.
>   	 */
>   	size_t extra_start, extra_end, colored_extra_start, colored_extra_end;
> +	unsigned suppress_colored_line_range:1;
>   };
>   
>   struct hunk {
> @@ -358,15 +359,14 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)
>   	if (!eol)
>   		eol = s->colored.buf + s->colored.len;
>   	p = memmem(line, eol - line, "@@ -", 4);
> -	if (!p)
> -		return error(_("could not parse colored hunk header '%.*s'"),
> -			     (int)(eol - line), line);
> -	p = memmem(p + 4, eol - p - 4, " @@", 3);
> -	if (!p)
> -		return error(_("could not parse colored hunk header '%.*s'"),
> -			     (int)(eol - line), line);
> +	if (p && (p = memmem(p + 4, eol - p - 4, " @@", 3)))

nit: there should be braces round both arms of the if, but it's hardly 
the first one that does not follow our official style.

> +		header->colored_extra_start = p + 3 - s->colored.buf;
> +	else {
> +		/* could not parse colored hunk header, leave as-is */
> +		header->colored_extra_start = hunk->colored_start;
> +		header->suppress_colored_line_range = 1;
> +	}
>   	hunk->colored_start = eol - s->colored.buf + (*eol == '\n');
> -	header->colored_extra_start = p + 3 - s->colored.buf;
>   	header->colored_extra_end = hunk->colored_start;
>   
>   	return 0;
> @@ -666,18 +666,20 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
                 if (!colored) {
                         p = s->plain.buf + header->extra_start;
                         len = header->extra_end - header->extra_start;
                 } else {
                         strbuf_addstr(out, s->s.fraginfo_color);

I don't think we want to add this escape sequence unless we're going to 
dynamically generate the hunk header

                         p = s->colored.buf + header->colored_extra_start;
                         len = header->colored_extra_end
                                 - header->colored_extra_start;

If we cannot parse the hunk header then len is non-zero...

                 }
>   
> -		if (s->mode->is_reverse)
> -			old_offset -= delta;
> -		else
> -			new_offset += delta;
> -
> -		strbuf_addf(out, "@@ -%lu", old_offset);
> -		if (header->old_count != 1)
> -			strbuf_addf(out, ",%lu", header->old_count);
> -		strbuf_addf(out, " +%lu", new_offset);
> -		if (header->new_count != 1)
> -			strbuf_addf(out, ",%lu", header->new_count);
> -		strbuf_addstr(out, " @@");
> +		if (!colored || !header->suppress_colored_line_range) {
> +			if (s->mode->is_reverse)
> +				old_offset -= delta;
> +			else
> +				new_offset += delta;
> +
> +			strbuf_addf(out, "@@ -%lu", old_offset);
> +			if (header->old_count != 1)
> +				strbuf_addf(out, ",%lu", header->old_count);
> +			strbuf_addf(out, " +%lu", new_offset);
> +			if (header->new_count != 1)
> +				strbuf_addf(out, ",%lu", header->new_count);
> +			strbuf_addstr(out, " @@");
> +		}
>   
>   		if (len)
>   			strbuf_add(out, p, len);

... and so we print the filtered hunk header here and do not reset the 
color below. That is probably ok as the filter should be resetting it's 
own colors but we shouldn't be printing the color at the beginning of 
the line above in that case.

                 else if (colored)
                         strbuf_addf(out, "%s\n", s->s.reset_color);
                 else
                         strbuf_addch(out, '\n');
         }


> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 8a594700f7b..47ed6698943 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -767,6 +767,16 @@ test_expect_success 'detect bogus diffFilter output' '
>   	grep "mismatched output" output
>   '
>   
> +test_expect_success 'handle iffy colored hunk headers' '
> +	git reset --hard &&
> +
> +	echo content >test &&
> +	printf n >n &&
> +	force_color git -c interactive.diffFilter="sed s/.*@@.*/XX/" \
> +		add -p >output 2>&1 <n &&
> +	grep "^[^@]*XX[^@]*$" output

I was wondering why this wasn't just `grep "^XX$"` as we should be 
printing the output of the diff filter verbatim. That lead to my 
comments about outputting escape codes above. Apart from that this patch 
looks good.

Best Wishes

Phillip

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

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v4 0/3] built-in add -p: support diff-so-fancy better
  2022-08-31 20:31     ` [PATCH v4 0/3] " Johannes Schindelin via GitGitGadget
                         ` (2 preceding siblings ...)
  2022-08-31 20:31       ` [PATCH v4 3/3] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
@ 2022-09-01 13:55       ` Phillip Wood
  2022-09-01 16:19         ` Junio C Hamano
  2022-09-01 15:42       ` [PATCH v5 " Johannes Schindelin via GitGitGadget
  4 siblings, 1 reply; 67+ messages in thread
From: Phillip Wood @ 2022-09-01 13:55 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget, git
  Cc: Philippe Blain, Jeff King, Johannes Schindelin

Hi Dscho

On 31/08/2022 21:31, Johannes Schindelin via GitGitGadget wrote:
> Philippe Blain reported in
> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com
> 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.

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.

Thanks

Phillip

> 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 <Johannes.Schindelin@gmx.de>
>        
>         ## 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 <johannes.schindelin@gmx.de>
>        
>       + ## 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 <Johannes.Schindelin@gmx.de>
>        
>         ## 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/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com,
>       @@ 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 <levraiphilippeblain@gmail.com>
>       +    Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>            Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>        
>         ## 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
> 

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v4 2/3] add -p: gracefully handle unparseable hunk headers in colored diffs
  2022-09-01 13:53         ` Phillip Wood
@ 2022-09-01 15:09           ` Johannes Schindelin
  0 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2022-09-01 15:09 UTC (permalink / raw)
  To: phillip.wood
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain, Jeff King

Hi Phillip,

On Thu, 1 Sep 2022, Phillip Wood wrote:

> On 31/08/2022 21:31, Johannes Schindelin via GitGitGadget wrote:
> [...]
> > @@ -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.

Fixed.

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

True.

>
>                         p = s->colored.buf + header->colored_extra_start;
>                         len = header->colored_extra_end
>                                 - header->colored_extra_start;
>
> If we cannot parse the hunk header then len is non-zero...
>
>                 }
> >   -		if (s->mode->is_reverse)
> > -			old_offset -= delta;
> > -		else
> > -			new_offset += delta;
> > -
> > -		strbuf_addf(out, "@@ -%lu", old_offset);
> > -		if (header->old_count != 1)
> > -			strbuf_addf(out, ",%lu", header->old_count);
> > -		strbuf_addf(out, " +%lu", new_offset);
> > -		if (header->new_count != 1)
> > -			strbuf_addf(out, ",%lu", header->new_count);
> > -		strbuf_addstr(out, " @@");
> > +		if (!colored || !header->suppress_colored_line_range) {
> > +			if (s->mode->is_reverse)
> > +				old_offset -= delta;
> > +			else
> > +				new_offset += delta;
> > +
> > +			strbuf_addf(out, "@@ -%lu", old_offset);
> > +			if (header->old_count != 1)
> > +				strbuf_addf(out, ",%lu", header->old_count);
> > +			strbuf_addf(out, " +%lu", new_offset);
> > +			if (header->new_count != 1)
> > +				strbuf_addf(out, ",%lu", header->new_count);
> > +			strbuf_addstr(out, " @@");
> > +		}
> >
> >     if (len)
> >      strbuf_add(out, p, len);
>
> ... and so we print the filtered hunk header here and do not reset the color
> below. That is probably ok as the filter should be resetting it's own colors
> but we shouldn't be printing the color at the beginning of the line above in
> that case.
>
>                 else if (colored)
>                         strbuf_addf(out, "%s\n", s->s.reset_color);
>                 else
>                         strbuf_addch(out, '\n');

I've changed the code so that the `suppress_colored_line_range` code path
prints no extra sequence but the hunk header and the hunk verbatim (they
still have to be two separate `strbuf*()` calls because the hunk could be
edited).

Since this code path now also returns early, the patch avoids the
indentation change altogether.

>         }
>
>
> > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> > index 8a594700f7b..47ed6698943 100755
> > --- a/t/t3701-add-interactive.sh
> > +++ b/t/t3701-add-interactive.sh
> > @@ -767,6 +767,16 @@ test_expect_success 'detect bogus diffFilter output' '
> >   	grep "mismatched output" output
> >   '
> >
> > +test_expect_success 'handle iffy colored hunk headers' '
> > +	git reset --hard &&
> > +
> > +	echo content >test &&
> > +	printf n >n &&
> > +	force_color git -c interactive.diffFilter="sed s/.*@@.*/XX/" \
> > +		add -p >output 2>&1 <n &&
> > +	grep "^[^@]*XX[^@]*$" output
>
> I was wondering why this wasn't just `grep "^XX$"` as we should be printing
> the output of the diff filter verbatim. That lead to my comments about
> outputting escape codes above. Apart from that this patch looks good.

I changed it to `^XX$` as you suggested.

Thank you for your excellent review,
Dscho

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

^ permalink raw reply	[flat|nested] 67+ messages in thread

* [PATCH v5 0/3] built-in add -p: support diff-so-fancy better
  2022-08-31 20:31     ` [PATCH v4 0/3] " Johannes Schindelin via GitGitGadget
                         ` (3 preceding siblings ...)
  2022-09-01 13:55       ` [PATCH v4 0/3] built-in add -p: support diff-so-fancy better Phillip Wood
@ 2022-09-01 15:42       ` Johannes Schindelin via GitGitGadget
  2022-09-01 15:42         ` [PATCH v5 1/3] add -p: detect more mismatches between plain vs colored diffs Johannes Schindelin via GitGitGadget
                           ` (3 more replies)
  4 siblings, 4 replies; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-09-01 15:42 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Phillip Wood, Jeff King, Johannes Schindelin

Philippe Blain reported in
https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com
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 <levraiphilippeblain@gmail.com>
     @@ 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

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 67+ messages in thread

* [PATCH v5 1/3] add -p: detect more mismatches between plain vs colored diffs
  2022-09-01 15:42       ` [PATCH v5 " Johannes Schindelin via GitGitGadget
@ 2022-09-01 15:42         ` Johannes Schindelin via GitGitGadget
  2022-09-01 15:42         ` [PATCH v5 2/3] add -p: gracefully handle unparseable hunk headers in " Johannes Schindelin via GitGitGadget
                           ` (2 subsequent siblings)
  3 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-09-01 15:42 UTC (permalink / raw)
  To: git
  Cc: Philippe Blain, Phillip Wood, Jeff King, Johannes Schindelin,
	Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 add-patch.c                | 5 ++++-
 t/t3701-add-interactive.sh | 5 +++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 509ca04456b..34f3807ff32 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -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;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 3b7df9bed5a..8a594700f7b 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -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' '
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH v5 2/3] add -p: gracefully handle unparseable hunk headers in colored diffs
  2022-09-01 15:42       ` [PATCH v5 " Johannes Schindelin via GitGitGadget
  2022-09-01 15:42         ` [PATCH v5 1/3] add -p: detect more mismatches between plain vs colored diffs Johannes Schindelin via GitGitGadget
@ 2022-09-01 15:42         ` Johannes Schindelin via GitGitGadget
  2022-09-01 16:03           ` Phillip Wood
  2022-09-01 15:42         ` [PATCH v5 3/3] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
  2022-09-01 16:04         ` [PATCH v5 0/3] built-in add -p: support diff-so-fancy better Phillip Wood
  3 siblings, 1 reply; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-09-01 15:42 UTC (permalink / raw)
  To: git
  Cc: Philippe Blain, Phillip Wood, Jeff King, Johannes Schindelin,
	Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In
https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com,
Phillipe Blain reported that the built-in `git add -p` command fails
when asked to use [`diff-so-fancy`][diff-so-fancy] to colorize the diff.

The reason is that this tool produces colored diffs with a hunk header
that does not contain any parseable `@@ ... @@` line range information,
and therefore we cannot detect any part in that header that comes after
the line range.

As proposed by Phillip Wood, let's take that for a clear indicator that
we should show the hunk headers verbatim. This is what the Perl version
of the interactive `add` command did, too.

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

Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c                | 25 +++++++++++++++++--------
 t/t3701-add-interactive.sh | 10 ++++++++++
 2 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 34f3807ff32..a6bd150de51 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -238,6 +238,7 @@ struct hunk_header {
 	 * include the newline.
 	 */
 	size_t extra_start, extra_end, colored_extra_start, colored_extra_end;
+	unsigned suppress_colored_line_range:1;
 };
 
 struct hunk {
@@ -358,15 +359,14 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)
 	if (!eol)
 		eol = s->colored.buf + s->colored.len;
 	p = memmem(line, eol - line, "@@ -", 4);
-	if (!p)
-		return error(_("could not parse colored hunk header '%.*s'"),
-			     (int)(eol - line), line);
-	p = memmem(p + 4, eol - p - 4, " @@", 3);
-	if (!p)
-		return error(_("could not parse colored hunk header '%.*s'"),
-			     (int)(eol - line), line);
+	if (p && (p = memmem(p + 4, eol - p - 4, " @@", 3))) {
+		header->colored_extra_start = p + 3 - s->colored.buf;
+	} else {
+		/* could not parse colored hunk header, leave as-is */
+		header->colored_extra_start = hunk->colored_start;
+		header->suppress_colored_line_range = 1;
+	}
 	hunk->colored_start = eol - s->colored.buf + (*eol == '\n');
-	header->colored_extra_start = p + 3 - s->colored.buf;
 	header->colored_extra_end = hunk->colored_start;
 
 	return 0;
@@ -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;
 			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_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;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 8a594700f7b..a94e7c53c8a 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -767,6 +767,16 @@ test_expect_success 'detect bogus diffFilter output' '
 	grep "mismatched output" output
 '
 
+test_expect_success 'handle iffy colored hunk headers' '
+	git reset --hard &&
+
+	echo content >test &&
+	printf n >n &&
+	force_color git -c interactive.diffFilter="sed s/.*@@.*/XX/" \
+		add -p >output 2>&1 <n &&
+	grep "^XX$" output
+'
+
 test_expect_success 'handle very large filtered diff' '
 	git reset --hard &&
 	# The specific number here is not important, but it must
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH v5 3/3] add -p: ignore dirty submodules
  2022-09-01 15:42       ` [PATCH v5 " Johannes Schindelin via GitGitGadget
  2022-09-01 15:42         ` [PATCH v5 1/3] add -p: detect more mismatches between plain vs colored diffs Johannes Schindelin via GitGitGadget
  2022-09-01 15:42         ` [PATCH v5 2/3] add -p: gracefully handle unparseable hunk headers in " Johannes Schindelin via GitGitGadget
@ 2022-09-01 15:42         ` Johannes Schindelin via GitGitGadget
  2022-09-01 16:55           ` Junio C Hamano
  2022-09-01 16:04         ` [PATCH v5 0/3] built-in add -p: support diff-so-fancy better Phillip Wood
  3 siblings, 1 reply; 67+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-09-01 15:42 UTC (permalink / raw)
  To: git
  Cc: Philippe Blain, Phillip Wood, Jeff King, Johannes Schindelin,
	Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 add-patch.c                |  3 ++-
 t/t3701-add-interactive.sh | 12 ++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/add-patch.c b/add-patch.c
index a6bd150de51..a6596532867 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -419,7 +419,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);
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index a94e7c53c8a..5a7a0ea7e83 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -955,6 +955,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 &&
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 67+ messages in thread

* Re: [PATCH v4 3/3] add -p: ignore dirty submodules
  2022-08-31 20:31       ` [PATCH v4 3/3] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
@ 2022-09-01 15:45         ` Jeff King
  2022-09-01 15:49           ` Jeff King
  2022-09-01 16:17         ` Junio C Hamano
  1 sibling, 1 reply; 67+ messages in thread
From: Jeff King @ 2022-09-01 15:45 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Philippe Blain, Phillip Wood, Johannes Schindelin

On Wed, Aug 31, 2022 at 08:31:17PM +0000, Johannes Schindelin via GitGitGadget wrote:

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

I hadn't been closely following the earlier iterations of this series,
but the approach of this version definitely matches what I'd expected to
see based on our previous discussions about filters.

Modulo the coloring thing Phillip brought up in patch 2, it all looks
good to me. Thanks for working on it. I know you said earlier that the
bugs we've found in the conversion make you hesitant to remove the perl
version for now. But to me, your responsiveness in fixing those bugs
gives me more confidence in dumping the perl one. ;) (eventually, at
least)

-Peff

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v4 3/3] add -p: ignore dirty submodules
  2022-09-01 15:45         ` Jeff King
@ 2022-09-01 15:49           ` Jeff King
  0 siblings, 0 replies; 67+ messages in thread
From: Jeff King @ 2022-09-01 15:49 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Philippe Blain, Phillip Wood, Johannes Schindelin

On Thu, Sep 01, 2022 at 11:45:49AM -0400, Jeff King wrote:

> On Wed, Aug 31, 2022 at 08:31:17PM +0000, Johannes Schindelin via GitGitGadget wrote:
> 
> > 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/

Doh, looks like I raced you sending the new version. :)

FWIW, the new iteration looks good to me.

-Peff

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v5 2/3] add -p: gracefully handle unparseable hunk headers in colored diffs
  2022-09-01 15:42         ` [PATCH v5 2/3] add -p: gracefully handle unparseable hunk headers in " Johannes Schindelin via GitGitGadget
@ 2022-09-01 16:03           ` Phillip Wood
  0 siblings, 0 replies; 67+ messages in thread
From: Phillip Wood @ 2022-09-01 16:03 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget, git
  Cc: Philippe Blain, Jeff King, Johannes Schindelin

Hi Dscho

On 01/09/2022 16:42, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> In
> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com,
> Phillipe Blain reported that the built-in `git add -p` command fails
> when asked to use [`diff-so-fancy`][diff-so-fancy] to colorize the diff.
> 
> The reason is that this tool produces colored diffs with a hunk header
> that does not contain any parseable `@@ ... @@` line range information,
> and therefore we cannot detect any part in that header that comes after
> the line range.
> 
> As proposed by Phillip Wood, let's take that for a clear indicator that
> we should show the hunk headers verbatim. This is what the Perl version
> of the interactive `add` command did, too.
> 
> [diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy
> 
> Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

> @@ -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;
>   			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_add(out, s->colored.buf + hunk->colored_start,
> +				   hunk->colored_end - hunk->colored_start);
> +			return;

Having an extra branch for this that returns early makes it easy to see 
what is being printed when we cannot parse the hunk header.

> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 8a594700f7b..a94e7c53c8a 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -767,6 +767,16 @@ test_expect_success 'detect bogus diffFilter output' '
>   	grep "mismatched output" output
>   '
>   
> +test_expect_success 'handle iffy colored hunk headers' '
> +	git reset --hard &&
> +
> +	echo content >test &&
> +	printf n >n &&
> +	force_color git -c interactive.diffFilter="sed s/.*@@.*/XX/" \
> +		add -p >output 2>&1 <n &&
> +	grep "^XX$" output

We check that the hunk header line from filtered diff was reproduced 
verbatim

This all looks good to me

Best Wishes

Phillip

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v5 0/3] built-in add -p: support diff-so-fancy better
  2022-09-01 15:42       ` [PATCH v5 " Johannes Schindelin via GitGitGadget
                           ` (2 preceding siblings ...)
  2022-09-01 15:42         ` [PATCH v5 3/3] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
@ 2022-09-01 16:04         ` Phillip Wood
  2022-09-01 16:54           ` Junio C Hamano
  3 siblings, 1 reply; 67+ messages in thread
From: Phillip Wood @ 2022-09-01 16:04 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget, git
  Cc: Philippe Blain, Jeff King, Johannes Schindelin

Hi Dscho

On 01/09/2022 16:42, Johannes Schindelin via GitGitGadget wrote:
> Philippe Blain reported in
> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com
> 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.

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

Thanks

Phillip

> 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 <levraiphilippeblain@gmail.com>
>       @@ 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
> 

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v4 3/3] add -p: ignore dirty submodules
  2022-08-31 20:31       ` [PATCH v4 3/3] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
  2022-09-01 15:45         ` Jeff King
@ 2022-09-01 16:17         ` Junio C Hamano
  2022-09-02  8:53           ` Johannes Schindelin
  1 sibling, 1 reply; 67+ messages in thread
From: Junio C Hamano @ 2022-09-01 16:17 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Philippe Blain, Phillip Wood, Jeff King, Johannes Schindelin

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

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v4 0/3] built-in add -p: support diff-so-fancy better
  2022-09-01 13:55       ` [PATCH v4 0/3] built-in add -p: support diff-so-fancy better Phillip Wood
@ 2022-09-01 16:19         ` Junio C Hamano
  0 siblings, 0 replies; 67+ messages in thread
From: Junio C Hamano @ 2022-09-01 16:19 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain,
	Jeff King, Johannes Schindelin

Phillip Wood <phillip.wood123@gmail.com> writes:

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

Yes, this round looks a lot more sensible.  Thanks, all.

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v5 0/3] built-in add -p: support diff-so-fancy better
  2022-09-01 16:04         ` [PATCH v5 0/3] built-in add -p: support diff-so-fancy better Phillip Wood
@ 2022-09-01 16:54           ` Junio C Hamano
  0 siblings, 0 replies; 67+ messages in thread
From: Junio C Hamano @ 2022-09-01 16:54 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain,
	Jeff King, Johannes Schindelin

Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Dscho
>
> On 01/09/2022 16:42, Johannes Schindelin via GitGitGadget wrote:
>> Philippe Blain reported in
>> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com
>> 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.
>
> That was a quick re-roll! I'm very happy with this version

Yeah, looking good.

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v5 3/3] add -p: ignore dirty submodules
  2022-09-01 15:42         ` [PATCH v5 3/3] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
@ 2022-09-01 16:55           ` Junio C Hamano
  0 siblings, 0 replies; 67+ messages in thread
From: Junio C Hamano @ 2022-09-01 16:55 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Philippe Blain, Phillip Wood, Jeff King, Johannes Schindelin

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

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Thanks to alwyas running `diff-index` and `diff-files` with the

"always".  If I do not forget I'll amend before merging it to 'seen'.

Thanks.

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v4 3/3] add -p: ignore dirty submodules
  2022-09-01 16:17         ` Junio C Hamano
@ 2022-09-02  8:53           ` Johannes Schindelin
  0 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2022-09-02  8:53 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain,
	Phillip Wood, Jeff King

Hi Junio,

On Thu, 1 Sep 2022, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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.

Sorry, I thought that your comment was more about changing the base commit
too often, not meant as "please change it back for your next iteration".

I have changed it back, fixed the typo and force-pushed. In case a new
iteration is still required, that iteration will have these changes.

Ciao,
Dscho

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

^ permalink raw reply	[flat|nested] 67+ messages in thread

end of thread, other threads:[~2022-09-02  8:53 UTC | newest]

Thread overview: 67+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-23 18:04 [PATCH 0/3] built-in add -p: support diff-so-fancy better Johannes Schindelin via GitGitGadget
2022-08-23 18:04 ` [PATCH 1/3] t3701: redefine what is "bogus" output of a diff filter Johannes Schindelin via GitGitGadget
2022-08-23 18:04 ` [PATCH 2/3] add -p: gracefully ignore unparseable hunk headers in colored diffs Johannes Schindelin via GitGitGadget
2022-08-23 18:04 ` [PATCH 3/3] add -p: handle `diff-so-fancy`'s hunk headers better Johannes Schindelin via GitGitGadget
2022-08-24  3:49 ` [PATCH 0/3] built-in add -p: support diff-so-fancy better Philippe Blain
2022-08-24  6:27   ` Johannes Schindelin
2022-08-24 13:21     ` Philippe Blain
2022-08-24 17:49       ` Philippe Blain
2022-08-24 18:24         ` Junio C Hamano
2022-08-24 21:05           ` Johannes Schindelin
2022-08-24 21:37             ` Junio C Hamano
2022-08-24 21:21 ` [PATCH v2 0/4] " Johannes Schindelin via GitGitGadget
2022-08-24 21:21   ` [PATCH v2 1/4] t3701: redefine what is "bogus" output of a diff filter Johannes Schindelin via GitGitGadget
2022-08-24 21:21   ` [PATCH v2 2/4] add -p: gracefully ignore unparseable hunk headers in colored diffs Johannes Schindelin via GitGitGadget
2022-08-29  7:56     ` Junio C Hamano
2022-08-24 21:21   ` [PATCH v2 3/4] add -p: handle `diff-so-fancy`'s hunk headers better Johannes Schindelin via GitGitGadget
2022-08-29  8:06     ` Junio C Hamano
2022-08-29 13:32       ` Johannes Schindelin
2022-08-29 17:19         ` Junio C Hamano
2022-08-30 14:14           ` Johannes Schindelin
2022-08-24 21:21   ` [PATCH v2 4/4] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
2022-08-24 22:11   ` [PATCH v2 0/4] built-in add -p: support diff-so-fancy better Junio C Hamano
2022-08-25  0:18   ` Philippe Blain
2022-08-26 11:43     ` Johannes Schindelin
2022-08-26 23:15       ` Philippe Blain
2022-08-29 15:11   ` [PATCH v3 0/5] " Johannes Schindelin via GitGitGadget
2022-08-29 15:11     ` [PATCH v3 1/5] t3701: redefine what is "bogus" output of a diff filter Johannes Schindelin via GitGitGadget
2022-08-30 13:17       ` Phillip Wood
2022-08-30 21:36         ` Junio C Hamano
2022-08-31  9:26           ` Phillip Wood
2022-08-31 15:36             ` Jeff King
2022-08-31 15:47               ` Jeff King
2022-08-31 19:57                 ` Johannes Schindelin
2022-08-29 15:11     ` [PATCH v3 2/5] add -p: gracefully ignore unparseable hunk headers in colored diffs Johannes Schindelin via GitGitGadget
2022-08-29 15:11     ` [PATCH v3 3/5] add -p: insert space in colored hunk header as needed Johannes Schindelin via GitGitGadget
2022-08-29 15:11     ` [PATCH v3 4/5] add -p: handle `diff-so-fancy`'s hunk headers better Johannes Schindelin via GitGitGadget
2022-08-30 13:23       ` Phillip Wood
2022-08-29 15:11     ` [PATCH v3 5/5] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
2022-08-30 13:26       ` Phillip Wood
2022-08-31 20:05         ` Johannes Schindelin
2022-08-31 20:19           ` Junio C Hamano
2022-08-31 20:38             ` Johannes Schindelin
2022-08-29 18:01     ` [PATCH v3 0/5] built-in add -p: support diff-so-fancy better Junio C Hamano
2022-08-30 14:22       ` Johannes Schindelin
2022-08-30 13:29     ` Phillip Wood
2022-08-31 20:44       ` Johannes Schindelin
2022-08-31 20:31     ` [PATCH v4 0/3] " Johannes Schindelin via GitGitGadget
2022-08-31 20:31       ` [PATCH v4 1/3] add -p: detect more mismatches between plain vs colored diffs Johannes Schindelin via GitGitGadget
2022-09-01 13:19         ` Phillip Wood
2022-08-31 20:31       ` [PATCH v4 2/3] add -p: gracefully handle unparseable hunk headers in " Johannes Schindelin via GitGitGadget
2022-09-01 13:53         ` Phillip Wood
2022-09-01 15:09           ` Johannes Schindelin
2022-08-31 20:31       ` [PATCH v4 3/3] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
2022-09-01 15:45         ` Jeff King
2022-09-01 15:49           ` Jeff King
2022-09-01 16:17         ` Junio C Hamano
2022-09-02  8:53           ` Johannes Schindelin
2022-09-01 13:55       ` [PATCH v4 0/3] built-in add -p: support diff-so-fancy better Phillip Wood
2022-09-01 16:19         ` Junio C Hamano
2022-09-01 15:42       ` [PATCH v5 " Johannes Schindelin via GitGitGadget
2022-09-01 15:42         ` [PATCH v5 1/3] add -p: detect more mismatches between plain vs colored diffs Johannes Schindelin via GitGitGadget
2022-09-01 15:42         ` [PATCH v5 2/3] add -p: gracefully handle unparseable hunk headers in " Johannes Schindelin via GitGitGadget
2022-09-01 16:03           ` Phillip Wood
2022-09-01 15:42         ` [PATCH v5 3/3] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
2022-09-01 16:55           ` Junio C Hamano
2022-09-01 16:04         ` [PATCH v5 0/3] built-in add -p: support diff-so-fancy better Phillip Wood
2022-09-01 16:54           ` Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).