From b6633a005384a8c120d34a8a79a5a5fe9d8719d7 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 1 Sep 2022 15:42:17 +0000 Subject: 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 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 Signed-off-by: Junio C Hamano --- 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 509ca04456..34f3807ff3 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 b354fb39de..6b1137e7c9 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 output 2>&1 && + grep "mismatched output" output ' test_expect_success 'diff.algorithm is passed to `git diff-files`' ' -- cgit v1.3 From fd3f7f619aa97ce577b29a19cfd056e20680f62a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 1 Sep 2022 15:42:18 +0000 Subject: add -p: gracefully handle unparseable hunk headers in colored diffs 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 Helped-by: Phillip Wood Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- 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 34f3807ff3..a6bd150de5 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 6b1137e7c9..447fc3380a 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 Date: Thu, 1 Sep 2022 15:42:19 +0000 Subject: add -p: ignore dirty submodules Thanks to always 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 Signed-off-by: Junio C Hamano --- 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 a6bd150de5..a659653286 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 447fc3380a..b6b309414e 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -942,6 +942,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 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 && -- cgit v1.3