[GSoC PATCH] apply: report input location in binary and garbage patch errors

[GSoC PATCH] apply: report input location in binary and garbage patch errors

[Jialong Wang]

Several binary parsing paths in apply.c still report only line
numbers. When more than one patch input is fed to a single
invocation, that does not tell the user which input the line belongs
to.

Report the patch input location for corrupt and unrecognized binary
patches, as well as the "patch with only garbage" case, and update
the related tests.

Signed-off-by: Jialong Wang <jerrywang183@yahoo.com>
---
 apply.c                 | 10 ++++++----
 t/t4100-apply-stat.sh   | 12 ++++++++++++
 t/t4103-apply-binary.sh | 20 +++++++++++++++++++-
 3 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/apply.c b/apply.c
index 700809f3e6..84b4a569c5 100644
--- a/apply.c
+++ b/apply.c
@@ -2110,8 +2110,8 @@ static struct fragment *parse_binary_hunk(struct apply_state *state,
  corrupt:
 	free(data);
 	*status_p = -1;
-	error(_("corrupt binary patch at line %d: %.*s"),
-	      state->linenr-1, llen-1, buffer);
+	error(_("corrupt binary patch at %s:%d: %.*s"),
+	      state->patch_input_file, state->linenr-1, llen-1, buffer);
 	return NULL;
 }
 
@@ -2147,7 +2147,8 @@ static int parse_binary(struct apply_state *state,
 	forward = parse_binary_hunk(state, &buffer, &size, &status, &used);
 	if (!forward && !status)
 		/* there has to be one hunk (forward hunk) */
-		return error(_("unrecognized binary patch at line %d"), state->linenr-1);
+		return error(_("unrecognized binary patch at %s:%d"),
+			     state->patch_input_file, state->linenr-1);
 	if (status)
 		/* otherwise we already gave an error message */
 		return status;
@@ -2309,7 +2310,8 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si
 		 */
 		if ((state->apply || state->check) &&
 		    (!patch->is_binary && !metadata_changes(patch))) {
-			error(_("patch with only garbage at line %d"), state->linenr);
+			error(_("patch with only garbage at %s:%d"),
+			      state->patch_input_file, state->linenr);
 			return -128;
 		}
 	}
diff --git a/t/t4100-apply-stat.sh b/t/t4100-apply-stat.sh
index b3d93d8ed6..8393076469 100755
--- a/t/t4100-apply-stat.sh
+++ b/t/t4100-apply-stat.sh
@@ -125,4 +125,16 @@ test_expect_success 'applying a patch with an invalid mode reports the input' '
 	EOF
 	test_cmp expect err
 '
+
+test_expect_success 'applying a patch with only garbage reports the input' '
+	cat >garbage.patch <<-\EOF &&
+	diff --git a/f b/f
+	--- a/f
+	+++ b/f
+	this is garbage
+	EOF
+	test_must_fail git apply garbage.patch 2>err &&
+	echo "error: patch with only garbage at garbage.patch:4" >expect &&
+	test_cmp expect err
+'
 test_done
diff --git a/t/t4103-apply-binary.sh b/t/t4103-apply-binary.sh
index 8e302a5a57..f2d41e06bc 100755
--- a/t/t4103-apply-binary.sh
+++ b/t/t4103-apply-binary.sh
@@ -179,6 +179,24 @@ test_expect_success PERL_TEST_HELPERS 'reject truncated binary diff' '
 	" <patch >patch.trunc &&
 
 	do_reset &&
-	test_must_fail git apply patch.trunc
+	test_must_fail git apply patch.trunc 2>err &&
+	line=$(awk "END { print NR + 1 }" patch.trunc) &&
+	grep "error: corrupt binary patch at patch.trunc:$line: " err
+'
+
+test_expect_success 'reject unrecognized binary diff' '
+	cat >patch.bad <<-\EOF &&
+	diff --git a/f b/f
+	new file mode 100644
+	index 0000000..7898192
+	GIT binary patch
+	bogus
+	EOF
+	test_must_fail git apply patch.bad 2>err &&
+	cat >expect <<-\EOF &&
+	error: unrecognized binary patch at patch.bad:4
+	error: No valid patches in input (allow with "--allow-empty")
+	EOF
+	test_cmp expect err
 '
 test_done
-- 
2.51.0

Re: [GSoC PATCH] apply: report input location in binary and garbage patch errors

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 3806 bytes --]

Jialong Wang <jerrywang183@yahoo.com> writes:

> Several binary parsing paths in apply.c still report only line
> numbers. When more than one patch input is fed to a single
> invocation, that does not tell the user which input the line belongs
> to.
>
> Report the patch input location for corrupt and unrecognized binary
> patches, as well as the "patch with only garbage" case, and update
> the related tests.
>
> Signed-off-by: Jialong Wang <jerrywang183@yahoo.com>
> ---
>  apply.c                 | 10 ++++++----
>  t/t4100-apply-stat.sh   | 12 ++++++++++++
>  t/t4103-apply-binary.sh | 20 +++++++++++++++++++-
>  3 files changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index 700809f3e6..84b4a569c5 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -2110,8 +2110,8 @@ static struct fragment *parse_binary_hunk(struct apply_state *state,
>   corrupt:
>  	free(data);
>  	*status_p = -1;
> -	error(_("corrupt binary patch at line %d: %.*s"),
> -	      state->linenr-1, llen-1, buffer);
> +	error(_("corrupt binary patch at %s:%d: %.*s"),
> +	      state->patch_input_file, state->linenr-1, llen-1, buffer);
>  	return NULL;
>  }
>
> @@ -2147,7 +2147,8 @@ static int parse_binary(struct apply_state *state,
>  	forward = parse_binary_hunk(state, &buffer, &size, &status, &used);
>  	if (!forward && !status)
>  		/* there has to be one hunk (forward hunk) */
> -		return error(_("unrecognized binary patch at line %d"), state->linenr-1);
> +		return error(_("unrecognized binary patch at %s:%d"),
> +			     state->patch_input_file, state->linenr-1);
>  	if (status)
>  		/* otherwise we already gave an error message */
>  		return status;
> @@ -2309,7 +2310,8 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si
>  		 */
>  		if ((state->apply || state->check) &&
>  		    (!patch->is_binary && !metadata_changes(patch))) {
> -			error(_("patch with only garbage at line %d"), state->linenr);
> +			error(_("patch with only garbage at %s:%d"),
> +			      state->patch_input_file, state->linenr);
>  			return -128;
>  		}
>  	}
> diff --git a/t/t4100-apply-stat.sh b/t/t4100-apply-stat.sh
> index b3d93d8ed6..8393076469 100755
> --- a/t/t4100-apply-stat.sh
> +++ b/t/t4100-apply-stat.sh
> @@ -125,4 +125,16 @@ test_expect_success 'applying a patch with an invalid mode reports the input' '
>  	EOF
>  	test_cmp expect err
>  '
> +
> +test_expect_success 'applying a patch with only garbage reports the input' '
> +	cat >garbage.patch <<-\EOF &&
> +	diff --git a/f b/f
> +	--- a/f
> +	+++ b/f
> +	this is garbage
> +	EOF
> +	test_must_fail git apply garbage.patch 2>err &&
> +	echo "error: patch with only garbage at garbage.patch:4" >expect &&
> +	test_cmp expect err
> +'
>  test_done
> diff --git a/t/t4103-apply-binary.sh b/t/t4103-apply-binary.sh
> index 8e302a5a57..f2d41e06bc 100755
> --- a/t/t4103-apply-binary.sh
> +++ b/t/t4103-apply-binary.sh
> @@ -179,6 +179,24 @@ test_expect_success PERL_TEST_HELPERS 'reject truncated binary diff' '
>  	" <patch >patch.trunc &&
>
>  	do_reset &&
> -	test_must_fail git apply patch.trunc
> +	test_must_fail git apply patch.trunc 2>err &&
> +	line=$(awk "END { print NR + 1 }" patch.trunc) &&
> +	grep "error: corrupt binary patch at patch.trunc:$line: " err
> +'
> +
> +test_expect_success 'reject unrecognized binary diff' '
> +	cat >patch.bad <<-\EOF &&
> +	diff --git a/f b/f
> +	new file mode 100644
> +	index 0000000..7898192
> +	GIT binary patch
> +	bogus
> +	EOF
> +	test_must_fail git apply patch.bad 2>err &&
> +	cat >expect <<-\EOF &&
> +	error: unrecognized binary patch at patch.bad:4
> +	error: No valid patches in input (allow with "--allow-empty")
> +	EOF
> +	test_cmp expect err
>  '
>  test_done
> --
> 2.51.0

This patch looks good to me!

Thanks for picking this up.

- Karthik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [GSoC PATCH] apply: report input location in binary and garbage patch errors

[Junio C Hamano]

Jialong Wang <jerrywang183@yahoo.com> writes:

> diff --git a/t/t4100-apply-stat.sh b/t/t4100-apply-stat.sh
> index b3d93d8ed6..8393076469 100755
> --- a/t/t4100-apply-stat.sh
> +++ b/t/t4100-apply-stat.sh
> @@ -125,4 +125,16 @@ test_expect_success 'applying a patch with an invalid mode reports the input' '
>  	EOF
>  	test_cmp expect err
>  '
> +
> +test_expect_success 'applying a patch with only garbage reports the input' '
> +	cat >garbage.patch <<-\EOF &&
> +	diff --git a/f b/f
> +	--- a/f
> +	+++ b/f
> +	this is garbage
> +	EOF
> +	test_must_fail git apply garbage.patch 2>err &&
> +	echo "error: patch with only garbage at garbage.patch:4" >expect &&
> +	test_cmp expect err
> +'
>  test_done

What is this patch based on?  Can we have a consolidated either a
single patch or a series of patches that form a single topic?

> diff --git a/t/t4103-apply-binary.sh b/t/t4103-apply-binary.sh
> index 8e302a5a57..f2d41e06bc 100755
> --- a/t/t4103-apply-binary.sh
> +++ b/t/t4103-apply-binary.sh
> @@ -179,6 +179,24 @@ test_expect_success PERL_TEST_HELPERS 'reject truncated binary diff' '
>  	" <patch >patch.trunc &&
>  
>  	do_reset &&
> -	test_must_fail git apply patch.trunc
> +	test_must_fail git apply patch.trunc 2>err &&
> +	line=$(awk "END { print NR + 1 }" patch.trunc) &&
> +	grep "error: corrupt binary patch at patch.trunc:$line: " err
> +'
> +
> +test_expect_success 'reject unrecognized binary diff' '
> +	cat >patch.bad <<-\EOF &&
> +	diff --git a/f b/f
> +	new file mode 100644
> +	index 0000000..7898192
> +	GIT binary patch
> +	bogus
> +	EOF
> +	test_must_fail git apply patch.bad 2>err &&
> +	cat >expect <<-\EOF &&
> +	error: unrecognized binary patch at patch.bad:4
> +	error: No valid patches in input (allow with "--allow-empty")
> +	EOF
> +	test_cmp expect err
>  '
>  test_done

Re: [GSoC PATCH] apply: report input location in binary and garbage patch errors

[Jialong Wang]

Hi Junio,

Yes, these follow-up patches are based on my earlier input-location
reporting change in apply.c.

I split the remaining error sites while working through them, but I
agree they are better presented as a single topic. I'll reroll them as
a small series on top of the original patch.

Thanks,
Jialong