* [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
* 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 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
* [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
* 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
* [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 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 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
* 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-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 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 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 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
* [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
* 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
* [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
* 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 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 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 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 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 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
* 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
* [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
* 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
* [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 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