diff mbox series

[v4,1/3] add -p: detect more mismatches between plain vs colored diffs

Message ID 25187c3a3c2bb440ab0af34011db41361d4e2496.1661977877.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. 31, 2022, 8:31 p.m. UTC 
From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 add-patch.c                | 5 ++++-
 t/t3701-add-interactive.sh | 5 +++--
 2 files changed, 7 insertions(+), 3 deletions(-)

Comments

Phillip Wood Sept. 1, 2022, 1:19 p.m. UTC | #1 
Hi Dscho

On 31/08/2022 21:31, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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.

Thanks for re-rolling, this looks good. The commit message explains the 
problem well and the fix is good, I especially like the fact that you've 
added a grep for the correct error message.

Related to this we also have code that detects over-long output from the 
filter, I'm not sure if we have a test for that but I think the 
implementation looks ok.

Best Wishes

Phillip

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   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 509ca04456b..34f3807ff32 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 3b7df9bed5a..8a594700f7b 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 <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' '
diff mbox series

Patch

diff --git a/add-patch.c b/add-patch.c
index 509ca04456b..34f3807ff32 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 3b7df9bed5a..8a594700f7b 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 <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' '