From: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Philippe Blain <levraiphilippeblain@gmail.com>,
Phillip Wood <phillip.wood123@gmail.com>,
Jeff King <peff@peff.net>,
Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: [PATCH v4 0/3] built-in add -p: support diff-so-fancy better
Date: Wed, 31 Aug 2022 20:31:14 +0000 [thread overview]
Message-ID: <pull.1336.v4.git.1661977877.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1336.v3.git.1661785916.gitgitgadget@gmail.com>
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
next prev parent reply other threads:[~2022-08-31 20:31 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-23 18:04 [PATCH 0/3] built-in add -p: support diff-so-fancy better Johannes Schindelin via GitGitGadget
2022-08-23 18:04 ` [PATCH 1/3] t3701: redefine what is "bogus" output of a diff filter Johannes Schindelin via GitGitGadget
2022-08-23 18:04 ` [PATCH 2/3] add -p: gracefully ignore unparseable hunk headers in colored diffs Johannes Schindelin via GitGitGadget
2022-08-23 18:04 ` [PATCH 3/3] add -p: handle `diff-so-fancy`'s hunk headers better Johannes Schindelin via GitGitGadget
2022-08-24 3:49 ` [PATCH 0/3] built-in add -p: support diff-so-fancy better Philippe Blain
2022-08-24 6:27 ` Johannes Schindelin
2022-08-24 13:21 ` Philippe Blain
2022-08-24 17:49 ` Philippe Blain
2022-08-24 18:24 ` Junio C Hamano
2022-08-24 21:05 ` Johannes Schindelin
2022-08-24 21:37 ` Junio C Hamano
2022-08-24 21:21 ` [PATCH v2 0/4] " Johannes Schindelin via GitGitGadget
2022-08-24 21:21 ` [PATCH v2 1/4] t3701: redefine what is "bogus" output of a diff filter Johannes Schindelin via GitGitGadget
2022-08-24 21:21 ` [PATCH v2 2/4] add -p: gracefully ignore unparseable hunk headers in colored diffs Johannes Schindelin via GitGitGadget
2022-08-29 7:56 ` Junio C Hamano
2022-08-24 21:21 ` [PATCH v2 3/4] add -p: handle `diff-so-fancy`'s hunk headers better Johannes Schindelin via GitGitGadget
2022-08-29 8:06 ` Junio C Hamano
2022-08-29 13:32 ` Johannes Schindelin
2022-08-29 17:19 ` Junio C Hamano
2022-08-30 14:14 ` Johannes Schindelin
2022-08-24 21:21 ` [PATCH v2 4/4] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
2022-08-24 22:11 ` [PATCH v2 0/4] built-in add -p: support diff-so-fancy better Junio C Hamano
2022-08-25 0:18 ` Philippe Blain
2022-08-26 11:43 ` Johannes Schindelin
2022-08-26 23:15 ` Philippe Blain
2022-08-29 15:11 ` [PATCH v3 0/5] " Johannes Schindelin via GitGitGadget
2022-08-29 15:11 ` [PATCH v3 1/5] t3701: redefine what is "bogus" output of a diff filter Johannes Schindelin via GitGitGadget
2022-08-30 13:17 ` Phillip Wood
2022-08-30 21:36 ` Junio C Hamano
2022-08-31 9:26 ` Phillip Wood
2022-08-31 15:36 ` Jeff King
2022-08-31 15:47 ` Jeff King
2022-08-31 19:57 ` Johannes Schindelin
2022-08-29 15:11 ` [PATCH v3 2/5] add -p: gracefully ignore unparseable hunk headers in colored diffs Johannes Schindelin via GitGitGadget
2022-08-29 15:11 ` [PATCH v3 3/5] add -p: insert space in colored hunk header as needed Johannes Schindelin via GitGitGadget
2022-08-29 15:11 ` [PATCH v3 4/5] add -p: handle `diff-so-fancy`'s hunk headers better Johannes Schindelin via GitGitGadget
2022-08-30 13:23 ` Phillip Wood
2022-08-29 15:11 ` [PATCH v3 5/5] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
2022-08-30 13:26 ` Phillip Wood
2022-08-31 20:05 ` Johannes Schindelin
2022-08-31 20:19 ` Junio C Hamano
2022-08-31 20:38 ` Johannes Schindelin
2022-08-29 18:01 ` [PATCH v3 0/5] built-in add -p: support diff-so-fancy better Junio C Hamano
2022-08-30 14:22 ` Johannes Schindelin
2022-08-30 13:29 ` Phillip Wood
2022-08-31 20:44 ` Johannes Schindelin
2022-08-31 20:31 ` Johannes Schindelin via GitGitGadget [this message]
2022-08-31 20:31 ` [PATCH v4 1/3] add -p: detect more mismatches between plain vs colored diffs Johannes Schindelin via GitGitGadget
2022-09-01 13:19 ` Phillip Wood
2022-08-31 20:31 ` [PATCH v4 2/3] add -p: gracefully handle unparseable hunk headers in " Johannes Schindelin via GitGitGadget
2022-09-01 13:53 ` Phillip Wood
2022-09-01 15:09 ` Johannes Schindelin
2022-08-31 20:31 ` [PATCH v4 3/3] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
2022-09-01 15:45 ` Jeff King
2022-09-01 15:49 ` Jeff King
2022-09-01 16:17 ` Junio C Hamano
2022-09-02 8:53 ` Johannes Schindelin
2022-09-01 13:55 ` [PATCH v4 0/3] built-in add -p: support diff-so-fancy better Phillip Wood
2022-09-01 16:19 ` Junio C Hamano
2022-09-01 15:42 ` [PATCH v5 " Johannes Schindelin via GitGitGadget
2022-09-01 15:42 ` [PATCH v5 1/3] add -p: detect more mismatches between plain vs colored diffs Johannes Schindelin via GitGitGadget
2022-09-01 15:42 ` [PATCH v5 2/3] add -p: gracefully handle unparseable hunk headers in " Johannes Schindelin via GitGitGadget
2022-09-01 16:03 ` Phillip Wood
2022-09-01 15:42 ` [PATCH v5 3/3] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
2022-09-01 16:55 ` Junio C Hamano
2022-09-01 16:04 ` [PATCH v5 0/3] built-in add -p: support diff-so-fancy better Phillip Wood
2022-09-01 16:54 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=pull.1336.v4.git.1661977877.gitgitgadget@gmail.com \
--to=gitgitgadget@gmail.com \
--cc=git@vger.kernel.org \
--cc=johannes.schindelin@gmx.de \
--cc=levraiphilippeblain@gmail.com \
--cc=peff@peff.net \
--cc=phillip.wood123@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).