diff mbox series

[v3,4/5] add -p: handle `diff-so-fancy`'s hunk headers better

Message ID e3e3a178f98183032b3df8ad9c81a096fe4af556.1661785916.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. 29, 2022, 3:11 p.m. UTC 
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(-)

Comments

Phillip Wood Aug. 30, 2022, 1:23 p.m. UTC | #1 
Hi Dscho

On 29/08/2022 16:11, Johannes Schindelin via GitGitGadget wrote:
> 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.

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

> 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(-)
> 
> diff --git a/add-patch.c b/add-patch.c
> index 9d575d30ed0..0217cdd7c4a 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -363,8 +363,17 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)
>   	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;
> +		/*
> +		 * We tried to parse the line range out of the colored hunk
> +		 * header, so that we could show just the extra information
> +		 * after the line range.
> +		 *
> +		 * At this point, we did not find that line range, but the hunk
> +		 * header likely has information that the user might find
> +		 * interesting. Let's just show the entire hunk header instead
> +		 * in that case.
> +		 */
> +		header->colored_extra_start = line - s->colored.buf;
>   	header->colored_extra_end = hunk->colored_start;
>   
>   	return 0;
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 49200b7df68..39e68b6d066 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -775,10 +775,14 @@ test_expect_success 'handle iffy colored hunk headers' '
>   		add -p <n &&
>   	force_color git -c interactive.diffFilter="sed \"s/\(.*@@\).*/\1FN/\"" \
>   		add -p >output 2>&1 <n &&
> +	force_color git -c interactive.diffFilter="sed \"s/\(.*@@\).*/file/\"" \
> +		add -p >output-so-fancy 2>&1 <n &&
>   	if test_have_prereq ADD_I_USE_BUILTIN
>   	then
> +		grep "@ file\$" output-so-fancy &&
>   		grep "@ FN\$" output
>   	else
> +		grep "^file\$" output-so-fancy &&
>   		grep "@FN\$" output
>   	fi
>   '
diff mbox series

Patch

diff --git a/add-patch.c b/add-patch.c
index 9d575d30ed0..0217cdd7c4a 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -363,8 +363,17 @@  static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)
 	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;
+		/*
+		 * We tried to parse the line range out of the colored hunk
+		 * header, so that we could show just the extra information
+		 * after the line range.
+		 *
+		 * At this point, we did not find that line range, but the hunk
+		 * header likely has information that the user might find
+		 * interesting. Let's just show the entire hunk header instead
+		 * in that case.
+		 */
+		header->colored_extra_start = line - s->colored.buf;
 	header->colored_extra_end = hunk->colored_start;
 
 	return 0;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 49200b7df68..39e68b6d066 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -775,10 +775,14 @@  test_expect_success 'handle iffy colored hunk headers' '
 		add -p <n &&
 	force_color git -c interactive.diffFilter="sed \"s/\(.*@@\).*/\1FN/\"" \
 		add -p >output 2>&1 <n &&
+	force_color git -c interactive.diffFilter="sed \"s/\(.*@@\).*/file/\"" \
+		add -p >output-so-fancy 2>&1 <n &&
 	if test_have_prereq ADD_I_USE_BUILTIN
 	then
+		grep "@ file\$" output-so-fancy &&
 		grep "@ FN\$" output
 	else
+		grep "^file\$" output-so-fancy &&
 		grep "@FN\$" output
 	fi
 '