From: Phillip Wood <phillip.wood123@gmail.com>
To: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org
Cc: Philippe Blain <levraiphilippeblain@gmail.com>,
Jeff King <peff@peff.net>,
Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH v5 0/3] built-in add -p: support diff-so-fancy better
Date: Thu, 1 Sep 2022 17:04:15 +0100 [thread overview]
Message-ID: <ced55750-21bb-c264-ef2d-666aca77fa2b@gmail.com> (raw)
In-Reply-To: <pull.1336.v5.git.1662046939.gitgitgadget@gmail.com>
Hi Dscho
On 01/09/2022 16:42, Johannes Schindelin via GitGitGadget wrote:
> Philippe Blain reported in
> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com
> that there is a problem when running the built-in version of git add -p with
> diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer.
> The symptom is this:
>
> error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'
>
>
> This patch series addresses that and should fix
> https://github.com/so-fancy/diff-so-fancy/issues/437
>
> Changes since v4:
>
> * The second patch was further simplified, avoiding to print extra ANSI
> sequences if the hunk header is shown verbatim.
That was a quick re-roll! I'm very happy with this version
Thanks
Phillip
> Changes since v3:
>
> * Instead of deviating from how the Perl version of git add -p did things,
> we now teach the built-in version to display hunk headers verbatim when
> no line range could be parsed out (instead of showing the line range
> anyways). This was a very good idea of Phillip's, dramatically
> simplifying the patch series.
> * Also, this iteration drops the first patch that claims to redefine what
> we consider bogus, but only hides an off-by-one. In its stead, there is
> now a patch that fixes said off-by-one.
>
> Changes since v2:
>
> * Added the appropriate "Reported-by" trailer to the commit message.
> * Split out the logic to insert a space between the colored line range and
> the extra information, if needed.
> * That logic was now corrected to see whether that space is really needed.
> * To verify that the logic does what we need it to do, the added regression
> test now specifically tests for that (single) extra space that we want to
> be inserted.
> * Reworded a stale comment that claimed that we might suppress the entire
> colored hunk header (which we no longer do).
> * Rebased to the current tip of the main branch to avoid a merge conflict
> with 716c1f649e3 (pipe_command(): mark stdin descriptor as non-blocking,
> 2022-08-17).
>
> Changes since v1:
>
> * Added a commit to ignore dirty submodules just like the Perl version
> does.
>
> Johannes Schindelin (3):
> add -p: detect more mismatches between plain vs colored diffs
> add -p: gracefully handle unparseable hunk headers in colored diffs
> add -p: ignore dirty submodules
>
> add-patch.c | 33 +++++++++++++++++++++++----------
> t/t3701-add-interactive.sh | 27 +++++++++++++++++++++++++--
> 2 files changed, 48 insertions(+), 12 deletions(-)
>
>
> base-commit: 07ee72db0e97b5c233f8ada0abb412248c2f1c6f
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1336%2Fdscho%2Fdiff-so-fancy-v5
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1336/dscho/diff-so-fancy-v5
> Pull-Request: https://github.com/gitgitgadget/git/pull/1336
>
> Range-diff vs v4:
>
> 1: 25187c3a3c2 = 1: 25187c3a3c2 add -p: detect more mismatches between plain vs colored diffs
> 2: cd1c5100506 ! 2: 93d0e3b4d2a add -p: gracefully handle unparseable hunk headers in colored diffs
> @@ Commit message
> we should show the hunk headers verbatim. This is what the Perl version
> of the interactive `add` command did, too.
>
> - This commit is best viewed with `--color-moved --ignore-space-change`.
> -
> [diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy
>
> Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
> @@ add-patch.c: static int parse_hunk_header(struct add_p_state *s, struct hunk *hu
> - if (!p)
> - return error(_("could not parse colored hunk header '%.*s'"),
> - (int)(eol - line), line);
> -+ if (p && (p = memmem(p + 4, eol - p - 4, " @@", 3)))
> ++ if (p && (p = memmem(p + 4, eol - p - 4, " @@", 3))) {
> + header->colored_extra_start = p + 3 - s->colored.buf;
> -+ else {
> ++ } else {
> + /* could not parse colored hunk header, leave as-is */
> + header->colored_extra_start = hunk->colored_start;
> + header->suppress_colored_line_range = 1;
> @@ add-patch.c: static int parse_hunk_header(struct add_p_state *s, struct hunk *hu
>
> return 0;
> @@ add-patch.c: static void render_hunk(struct add_p_state *s, struct hunk *hunk,
> - - header->colored_extra_start;
> - }
> -
> -- if (s->mode->is_reverse)
> -- old_offset -= delta;
> -- else
> -- new_offset += delta;
> --
> -- strbuf_addf(out, "@@ -%lu", old_offset);
> -- if (header->old_count != 1)
> -- strbuf_addf(out, ",%lu", header->old_count);
> -- strbuf_addf(out, " +%lu", new_offset);
> -- if (header->new_count != 1)
> -- strbuf_addf(out, ",%lu", header->new_count);
> -- strbuf_addstr(out, " @@");
> -+ if (!colored || !header->suppress_colored_line_range) {
> -+ if (s->mode->is_reverse)
> -+ old_offset -= delta;
> -+ else
> -+ new_offset += delta;
> + if (!colored) {
> + p = s->plain.buf + header->extra_start;
> + len = header->extra_end - header->extra_start;
> ++ } else if (header->suppress_colored_line_range) {
> ++ strbuf_add(out,
> ++ s->colored.buf + header->colored_extra_start,
> ++ header->colored_extra_end -
> ++ header->colored_extra_start);
> +
> -+ strbuf_addf(out, "@@ -%lu", old_offset);
> -+ if (header->old_count != 1)
> -+ strbuf_addf(out, ",%lu", header->old_count);
> -+ strbuf_addf(out, " +%lu", new_offset);
> -+ if (header->new_count != 1)
> -+ strbuf_addf(out, ",%lu", header->new_count);
> -+ strbuf_addstr(out, " @@");
> -+ }
> -
> - if (len)
> - strbuf_add(out, p, len);
> ++ strbuf_add(out, s->colored.buf + hunk->colored_start,
> ++ hunk->colored_end - hunk->colored_start);
> ++ return;
> + } else {
> + strbuf_addstr(out, s->s.fraginfo_color);
> + p = s->colored.buf + header->colored_extra_start;
>
> ## t/t3701-add-interactive.sh ##
> @@ t/t3701-add-interactive.sh: test_expect_success 'detect bogus diffFilter output' '
> @@ t/t3701-add-interactive.sh: test_expect_success 'detect bogus diffFilter output'
> + printf n >n &&
> + force_color git -c interactive.diffFilter="sed s/.*@@.*/XX/" \
> + add -p >output 2>&1 <n &&
> -+ grep "^[^@]*XX[^@]*$" output
> ++ grep "^XX$" output
> +'
> +
> test_expect_success 'handle very large filtered diff' '
> 3: 116f0cf5cab = 3: 47943b603b1 add -p: ignore dirty submodules
>
next prev parent reply other threads:[~2022-09-01 16:04 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 ` [PATCH v4 0/3] " Johannes Schindelin via GitGitGadget
2022-08-31 20:31 ` [PATCH v4 1/3] add -p: detect more mismatches between plain vs colored diffs Johannes Schindelin via GitGitGadget
2022-09-01 13:19 ` Phillip Wood
2022-08-31 20:31 ` [PATCH v4 2/3] add -p: gracefully handle unparseable hunk headers in " Johannes Schindelin via GitGitGadget
2022-09-01 13:53 ` Phillip Wood
2022-09-01 15:09 ` Johannes Schindelin
2022-08-31 20:31 ` [PATCH v4 3/3] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
2022-09-01 15:45 ` Jeff King
2022-09-01 15:49 ` Jeff King
2022-09-01 16:17 ` Junio C Hamano
2022-09-02 8:53 ` Johannes Schindelin
2022-09-01 13:55 ` [PATCH v4 0/3] built-in add -p: support diff-so-fancy better Phillip Wood
2022-09-01 16:19 ` Junio C Hamano
2022-09-01 15:42 ` [PATCH v5 " Johannes Schindelin via GitGitGadget
2022-09-01 15:42 ` [PATCH v5 1/3] add -p: detect more mismatches between plain vs colored diffs Johannes Schindelin via GitGitGadget
2022-09-01 15:42 ` [PATCH v5 2/3] add -p: gracefully handle unparseable hunk headers in " Johannes Schindelin via GitGitGadget
2022-09-01 16:03 ` Phillip Wood
2022-09-01 15:42 ` [PATCH v5 3/3] add -p: ignore dirty submodules Johannes Schindelin via GitGitGadget
2022-09-01 16:55 ` Junio C Hamano
2022-09-01 16:04 ` Phillip Wood [this message]
2022-09-01 16:54 ` [PATCH v5 0/3] built-in add -p: support diff-so-fancy better 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=ced55750-21bb-c264-ef2d-666aca77fa2b@gmail.com \
--to=phillip.wood123@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=johannes.schindelin@gmx.de \
--cc=levraiphilippeblain@gmail.com \
--cc=peff@peff.net \
--cc=phillip.wood@dunelm.org.uk \
/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).