patch, 3 partsadd -p: handle `diff-so-fancy`'s hunk headers better

Philippe Blain reported in https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com that there is a problem when running the built-in version of git add -p with diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer. The symptom is this:

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

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

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

 add-patch.c                | 21 ++++++++++++---------
 t/t3701-add-interactive.sh | 12 +++++++++++-
 2 files changed, 23 insertions(+), 10 deletions(-)

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

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

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

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

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

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

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

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

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

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

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

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

Hi Dscho,

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

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

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

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

This is despite using "diff-so-fancy --patch" as interactive.diffFilter, which should keep the number of lines the same. This ('--patch') was added in [2], about a month after Peff wrote the message you mention in https://lore.kernel.org/git/s40ss309-3311-o08s-38r2-9144r33pq549@tzk.qr/.

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

Cheers,

Philippe.

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

Hi Philippe,

On Tue, 23 Aug 2022, Philippe Blain wrote:

Great!

Would you mind sharing the example with me?

Thanks, Dscho

Hi Dscho,

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

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

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

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

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

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

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

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

─────────────────────────────────────────────────────────────────────── modified: ice_dyn_vp.F90 ───────────────────────────────────────────────────────────────────────

Hi Dscho,

Le 2022-08-24 à 09:21, Philippe Blain a écrit :

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

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

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

```bash #!/bin/bash

set -euEo pipefail

repo=repro rm -rf $repo

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

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

Cheers,

Philippe.

Philippe Blain <levraiphilippeblain@gmail.com> writes:

I somehow had an impression that that the C reimplementation was designed to be a bug-for-bug compatible faithful rewrite of the original scripted version, but looking at the way how this bug is handled, I am not sure (it looks as if the initial fix was to patch the code to match the symptom, not to make the code to behave more faithfully to the scripted version). As with any big rewrite, a complete re-implementation always has risks to introduce unintended regressions and that is why starting with faithful rewrite with the staged transition plan is valuable.

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

Thanks, you two, for working on this.

Hi Junio & Philippe,

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

Ah. Submodules. The gift that keeps on giving.

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

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

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

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

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

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

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

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

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

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

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

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

Ciao, Dscho

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

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

OK.

Yup.

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

Thanks, all.

Philippe Blain reported in https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com that there is a problem when running the built-in version of git add -p with diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer. The symptom is this:

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

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

Changes since v1:

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

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

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

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

Range-diff vs v1:

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

So far, looking good.

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

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

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

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

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

This is the only thing that corresponds to the proposed log message. The comment that says "showing nothing" is no longer correct, and needs to be updated.

Everything else in this patch is about adding an extra space depending on what is in the "funcname" part. The code does not know or care if it is an attempt to do diff-so-fancy's headers better by doing something we didn't do to the normal header we managed to have parsed; rather the extra space thing is done unconditionally and does not know or care if extra_start is truly after " @@" or a place in the line the new code computed.

So the following three hunks either need to be made into a separate patch, or deserves to be explained in a new paragraph in the proposed log message.

It is good to make sure that XX that was munged appears in the output. This however does not check anything about the needs-extra-space logic. It should. If the extra-space logic is moved to a separate patch, then this step can have the above test, but then the separate patch needs to update it to check the additional logic.

Other than that, looking very good.

Hi Junio,

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

Correct.

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

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

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

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

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

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

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

Ciao, Dscho

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

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

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

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

Thanks.

Hi Junio,

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

Yes.

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

Yes, it is definitely less expensive.

Ciao, Dscho

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

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

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

Hi Dscho,

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

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

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

Thanks again for the quick fixes,

Philippe.

Hi Philippe,

On Wed, 24 Aug 2022, Philippe Blain wrote:

Good suggestion.

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

Ciao, Dscho

Hi Dscho,

Le 2022-08-26 à 07:43, Johannes Schindelin a écrit :

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

Cheers, Philippe.

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

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

Hi Dscho

On 29/08/2022 16:11, Johannes Schindelin via GitGitGadget wrote:

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

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

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.

Hi Junio

On 30/08/2022 22:36, Junio C Hamano wrote:

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

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

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

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

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

Hi Peff,

On Wed, 31 Aug 2022, Jeff King wrote:

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

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

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

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

Hi Dscho

On 29/08/2022 16:11, Johannes Schindelin via GitGitGadget wrote:

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

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

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

Hi Dscho

On 29/08/2022 16:11, Johannes Schindelin via GitGitGadget wrote:

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

Hi Phillip,

On Tue, 30 Aug 2022, Phillip Wood wrote:

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.

Thanks! Dscho

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

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.

Hi Junio,

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

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

Yep!

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

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

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.

Hi Junio,

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

Noted. I just wanted to be nice.

Sounds good.

Thanks, Dscho

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:

Hi Phillip,

On Tue, 30 Aug 2022, Phillip Wood wrote:

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

Thank you! Dscho

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

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

Hi Dscho

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

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.

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.

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

                 }

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

Hi Phillip,

On Thu, 1 Sep 2022, Phillip Wood wrote:

Fixed.

True.

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.

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

Thank you for your excellent review, Dscho

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

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

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

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

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

FWIW, the new iteration looks good to me.

-Peff

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

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

Hi Junio,

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

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

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

Hi Dscho

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

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

Hi Dscho

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

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

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

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

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

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

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

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

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

Thanks.

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

Hi Dscho

On 01/09/2022 16:42, Johannes Schindelin via GitGitGadget wrote:

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.

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

This all looks good to me

Best Wishes

Phillip

Hi Dscho

On 01/09/2022 16:42, Johannes Schindelin via GitGitGadget wrote:

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

Thanks

Phillip

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

Yeah, looking good.