diff mbox series

[v2,2/4] add -p: gracefully ignore unparseable hunk headers in colored diffs

Message ID b07f85a035954bb9145a0fe034e6f17aeb128f52.1661376112.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series built-in add -p: support diff-so-fancy better | expand

Commit Message

Johannes Schindelin Aug. 24, 2022, 9:21 p.m. UTC 
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(-)

Comments

Junio C Hamano Aug. 29, 2022, 7:56 a.m. UTC | #1 
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> diff --git a/add-patch.c b/add-patch.c
> index 509ca04456b..f2fffe1af02 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -357,16 +357,13 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)
>  	eol = memchr(line, '\n', s->colored.len - hunk->colored_start);
>  	if (!eol)
>  		eol = s->colored.buf + s->colored.len;
> -	p = memmem(line, eol - line, "@@ -", 4);
> -	if (!p)
> -		return error(_("could not parse colored hunk header '%.*s'"),
> -			     (int)(eol - line), line);
> -	p = memmem(p + 4, eol - p - 4, " @@", 3);
> -	if (!p)
> -		return error(_("could not parse colored hunk header '%.*s'"),
> -			     (int)(eol - line), line);
>  	hunk->colored_start = eol - s->colored.buf + (*eol == '\n');
> -	header->colored_extra_start = p + 3 - s->colored.buf;
> +	p = memmem(line, eol - line, "@@ -", 4);
> +	if (p && (p = memmem(p + 4, eol - p - 4, " @@", 3)))
> +		header->colored_extra_start = p + 3 - s->colored.buf;
> +	else
> +		/* could not parse colored hunk header, showing nothing */
> +		header->colored_extra_start = hunk->colored_start;
>  	header->colored_extra_end = hunk->colored_start;

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.
diff mbox series

Patch

diff --git a/add-patch.c b/add-patch.c
index 509ca04456b..f2fffe1af02 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -357,16 +357,13 @@  static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)
 	eol = memchr(line, '\n', s->colored.len - hunk->colored_start);
 	if (!eol)
 		eol = s->colored.buf + s->colored.len;
-	p = memmem(line, eol - line, "@@ -", 4);
-	if (!p)
-		return error(_("could not parse colored hunk header '%.*s'"),
-			     (int)(eol - line), line);
-	p = memmem(p + 4, eol - p - 4, " @@", 3);
-	if (!p)
-		return error(_("could not parse colored hunk header '%.*s'"),
-			     (int)(eol - line), line);
 	hunk->colored_start = eol - s->colored.buf + (*eol == '\n');
-	header->colored_extra_start = p + 3 - s->colored.buf;
+	p = memmem(line, eol - line, "@@ -", 4);
+	if (p && (p = memmem(p + 4, eol - p - 4, " @@", 3)))
+		header->colored_extra_start = p + 3 - s->colored.buf;
+	else
+		/* could not parse colored hunk header, showing nothing */
+		header->colored_extra_start = hunk->colored_start;
 	header->colored_extra_end = hunk->colored_start;
 
 	return 0;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index b40d1c94d99..7e3c1de71f5 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -766,6 +766,15 @@  test_expect_success 'detect bogus diffFilter output' '
 	force_color test_must_fail git add -p <y
 '
 
+test_expect_success 'gracefully fail to parse colored hunk header' '
+	git reset --hard &&
+
+	echo content >test &&
+	test_config interactive.diffFilter "sed s/@@/XX/g" &&
+	printf y >y &&
+	force_color git add -p <y
+'
+
 test_expect_success 'diff.algorithm is passed to `git diff-files`' '
 	git reset --hard &&