threads / patch / 57308

patchpatch-id: fix scan_hunk_header on diffs with -U1

Subject: [PATCH] patch-id: fix scan_hunk_header on diffs with -U1

## tl;dr

6 messages between Jan 25, 2022 and Jan 31, 2022. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Jerry Zhang· Jan 25, 2022, 22:16 UTC · lore

Normally diffs will contain a hunk header of the format "@@ -2,2 +2,15 @@ code". However when git diff is given "-U1" indicating that diffs are surrounded by only one line of context, it can omit the second comma separated value in either before or after line counts in the case where there is only one line.

This can produce hunk headers that look like "@@ -2 +2,18 @@ code" or "@@ -2,2 +2 @@ code". As a result, scan_hunk_header mistakenly returns the line number as line count, which then results in unpredictable parsing errors with the rest of the patch, including giving multiple lines of output for a single commit.

Fix by explicitly setting line count to 1 when there is no comma, and add a test.

Signed-off-by: Jerry Zhang <jerry@skydio.com>
---
 builtin/patch-id.c  |  9 +++++++--
 t/t4204-patch-id.sh | 32 +++++++++++++++++++++++++++++++-
 2 files changed, 38 insertions(+), 3 deletions(-)
Show changes to 2 files +35 −3

builtin/patch-id.c, t/t4204-patch-id.sh

diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 822ffff51f..881fcf3273 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -30,26 +30,31 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)
 
 	q = p + 4;
 	n = strspn(q, digits);
 	if (q[n] == ',') {
 		q += n + 1;
+		*p_before = atoi(q);
 		n = strspn(q, digits);
+	} else {
+		*p_before = 1;
 	}
+
 	if (n == 0 || q[n] != ' ' || q[n+1] != '+')
 		return 0;
 
 	r = q + n + 2;
 	n = strspn(r, digits);
 	if (r[n] == ',') {
 		r += n + 1;
+		*p_after = atoi(r);
 		n = strspn(r, digits);
+	} else {
+		*p_after = 1;
 	}
 	if (n == 0)
 		return 0;
 
-	*p_before = atoi(q);
-	*p_after = atoi(r);
 	return 1;
 }
 
 static int get_one_patchid(struct object_id *next_oid, struct object_id *result,
 			   struct strbuf *line_buf, int stable)
diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
index 80f4a65b28..08d10dfc3e 100755
--- a/t/t4204-patch-id.sh
+++ b/t/t4204-patch-id.sh
@@ -36,11 +36,11 @@ test_expect_success 'patch-id output is well-formed' '
 calc_patch_id () {
 	patch_name="$1"
 	shift
 	git patch-id "$@" >patch-id.output &&
 	sed "s/ .*//" patch-id.output >patch-id_"$patch_name" &&
-	test_line_count -gt 0 patch-id_"$patch_name"
+	test_line_count -eq 1 patch-id_"$patch_name"
 }
 
 get_top_diff () {
 	git log -p -1 "$@" -O bar-then-foo --
 }
@@ -200,6 +200,36 @@ EOF
 test_expect_success 'patch-id handles no-nl-at-eof markers' '
 	cat nonl | calc_patch_id nonl &&
 	cat withnl | calc_patch_id withnl &&
 	test_cmp patch-id_nonl patch-id_withnl
 '
+
+cat >diffu1 <<\EOF
+diff --git a/bar b/bar
+index bdaf90f..31051f6 100644
+--- a/bar
++++ b/bar
+@@ -2 +2,2 @@
+ b
++c
+diff --git a/car b/car
+index 00750ed..2ae5e34 100644
+--- a/car
++++ b/car
+@@ -1 +1,2 @@
+ 3
++d
+diff --git a/foo b/foo
+index e439850..7146eb8 100644
+--- a/foo
++++ b/foo
+@@ -2 +2,2 @@
+ a
++e
+EOF
+
+test_expect_success 'patch-id handles diffs with -U1' '
+	cat diffu1 | calc_patch_id diffu1 &&
+	test_config patchid.stable true &&
+	cat diffu1 | calc_patch_id diffu1stable
+'
 test_done

-- 
2.32.0.1314.g6ed4fcc4cc
Junio C Hamano· Jan 26, 2022, 05:13 UTC · re: Jerry Zhang · lore

Re: [PATCH] patch-id: fix scan_hunk_header on diffs with -U1

Jerry Zhang <jerry@skydio.com> writes:
Show 6 quoted lines
> Normally diffs will contain a hunk header of the format
> "@@ -2,2 +2,15 @@ code". However when git diff is given "-U1"
> indicating that diffs are surrounded by only one line of
> context, it can omit the second comma separated value in
> either before or after line counts in the case where there
> is only one line.

Yes, it is possible that l and n in "@@ -k,l +m,n @@" is 1 in which case ",1" is omitted altogether. I however do not think it is limited to "-U1". The above description is inaccurate.

	$ echo one >foo
	$ git add foo
	$ echo two >foo
	$ git diff foo
	diff --git i/foo w/foo
	index 257cc5642c..5716ca5987 100644
	--- i/foo
	+++ w/foo
	@@ -1 +1 @@
	-one
	+two
> Fix by explicitly setting line count to 1 when there is
> no comma, and add a test.

It is correct to assume missing ,l and ,n as 1. We should have the same logic in apply.c (I wonder if there is a way to share the parser, but I do not know if it is worth it).

Show 13 quoted lines
> Signed-off-by: Jerry Zhang <jerry@skydio.com>
> ---
>  builtin/patch-id.c  |  9 +++++++--
>  t/t4204-patch-id.sh | 32 +++++++++++++++++++++++++++++++-
>  2 files changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/patch-id.c b/builtin/patch-id.c
> index 822ffff51f..881fcf3273 100644
> --- a/builtin/patch-id.c
> +++ b/builtin/patch-id.c
> @@ -30,26 +30,31 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)
>  
>  	q = p + 4;
I wonder if the repeated code from here ...
Show 9 quoted lines
>  	n = strspn(q, digits);
>  	if (q[n] == ',') {
>  		q += n + 1;
> +		*p_before = atoi(q);
>  		n = strspn(q, digits);
> +	} else {
> +		*p_before = 1;
>  	}
> +

... down to here ought to be in a helper function to parse l out of k[,l]. But that can and should be done as a post-fix cleanup.

Show 76 quoted lines
>  	if (n == 0 || q[n] != ' ' || q[n+1] != '+')
>  		return 0;
>  
>  	r = q + n + 2;
>  	n = strspn(r, digits);
>  	if (r[n] == ',') {
>  		r += n + 1;
> +		*p_after = atoi(r);
>  		n = strspn(r, digits);
> +	} else {
> +		*p_after = 1;
>  	}
>  	if (n == 0)
>  		return 0;
>  
> -	*p_before = atoi(q);
> -	*p_after = atoi(r);
>  	return 1;
>  }
>  
>  static int get_one_patchid(struct object_id *next_oid, struct object_id *result,
>  			   struct strbuf *line_buf, int stable)
> diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
> index 80f4a65b28..08d10dfc3e 100755
> --- a/t/t4204-patch-id.sh
> +++ b/t/t4204-patch-id.sh
> @@ -36,11 +36,11 @@ test_expect_success 'patch-id output is well-formed' '
>  calc_patch_id () {
>  	patch_name="$1"
>  	shift
>  	git patch-id "$@" >patch-id.output &&
>  	sed "s/ .*//" patch-id.output >patch-id_"$patch_name" &&
> -	test_line_count -gt 0 patch-id_"$patch_name"
> +	test_line_count -eq 1 patch-id_"$patch_name"
>  }
>  
>  get_top_diff () {
>  	git log -p -1 "$@" -O bar-then-foo --
>  }
> @@ -200,6 +200,36 @@ EOF
>  test_expect_success 'patch-id handles no-nl-at-eof markers' '
>  	cat nonl | calc_patch_id nonl &&
>  	cat withnl | calc_patch_id withnl &&
>  	test_cmp patch-id_nonl patch-id_withnl
>  '
> +
> +cat >diffu1 <<\EOF
> +diff --git a/bar b/bar
> +index bdaf90f..31051f6 100644
> +--- a/bar
> ++++ b/bar
> +@@ -2 +2,2 @@
> + b
> ++c
> +diff --git a/car b/car
> +index 00750ed..2ae5e34 100644
> +--- a/car
> ++++ b/car
> +@@ -1 +1,2 @@
> + 3
> ++d
> +diff --git a/foo b/foo
> +index e439850..7146eb8 100644
> +--- a/foo
> ++++ b/foo
> +@@ -2 +2,2 @@
> + a
> ++e
> +EOF
> +
> +test_expect_success 'patch-id handles diffs with -U1' '
> +	cat diffu1 | calc_patch_id diffu1 &&
> +	test_config patchid.stable true &&
> +	cat diffu1 | calc_patch_id diffu1stable
> +'
>  test_done
Jerry Zhang· Jan 29, 2022, 02:47 UTC · re: Jerry Zhang · lore

[PATCH V2] patch-id: fix scan_hunk_header on diffs with 1 line of before/after

Normally diffs will contain a hunk header of the format "@@ -2,2 +2,15 @@ code". However in certain situations git can omit the second comma separated value in either before or after line counts in the case where there is only one line.

This can produce hunk headers that look like "@@ -2 +2,18 @@ code" or "@@ -2,2 +2 @@ code". As a result, scan_hunk_header mistakenly returns the line number as line count, which then results in unpredictable parsing errors with the rest of the patch, including giving multiple lines of output for a single commit.

Fix by explicitly setting line count to 1 when there is no comma, and add a test.

apply.c contains this same logic except it is correct. A worthwhile future project might be to unify these two diff parsers so they both benefit from fixes.

Signed-off-by: Jerry Zhang <jerry@skydio.com>
---
V1->V2:
- Removed references to -U1 since this isn't limited to that
- Added notes for future tasks
 builtin/patch-id.c  |  9 +++++++--
 t/t4204-patch-id.sh | 32 +++++++++++++++++++++++++++++++-
 2 files changed, 38 insertions(+), 3 deletions(-)
Show changes to 2 files +35 −3

builtin/patch-id.c, t/t4204-patch-id.sh

diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 822ffff51f..881fcf3273 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -30,26 +30,31 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)
 
 	q = p + 4;
 	n = strspn(q, digits);
 	if (q[n] == ',') {
 		q += n + 1;
+		*p_before = atoi(q);
 		n = strspn(q, digits);
+	} else {
+		*p_before = 1;
 	}
+
 	if (n == 0 || q[n] != ' ' || q[n+1] != '+')
 		return 0;
 
 	r = q + n + 2;
 	n = strspn(r, digits);
 	if (r[n] == ',') {
 		r += n + 1;
+		*p_after = atoi(r);
 		n = strspn(r, digits);
+	} else {
+		*p_after = 1;
 	}
 	if (n == 0)
 		return 0;
 
-	*p_before = atoi(q);
-	*p_after = atoi(r);
 	return 1;
 }
 
 static int get_one_patchid(struct object_id *next_oid, struct object_id *result,
 			   struct strbuf *line_buf, int stable)
diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
index 80f4a65b28..34a386aee2 100755
--- a/t/t4204-patch-id.sh
+++ b/t/t4204-patch-id.sh
@@ -36,11 +36,11 @@ test_expect_success 'patch-id output is well-formed' '
 calc_patch_id () {
 	patch_name="$1"
 	shift
 	git patch-id "$@" >patch-id.output &&
 	sed "s/ .*//" patch-id.output >patch-id_"$patch_name" &&
-	test_line_count -gt 0 patch-id_"$patch_name"
+	test_line_count -eq 1 patch-id_"$patch_name"
 }
 
 get_top_diff () {
 	git log -p -1 "$@" -O bar-then-foo --
 }
@@ -200,6 +200,36 @@ EOF
 test_expect_success 'patch-id handles no-nl-at-eof markers' '
 	cat nonl | calc_patch_id nonl &&
 	cat withnl | calc_patch_id withnl &&
 	test_cmp patch-id_nonl patch-id_withnl
 '
+
+cat >diffu1 <<\EOF
+diff --git a/bar b/bar
+index bdaf90f..31051f6 100644
+--- a/bar
++++ b/bar
+@@ -2 +2,2 @@
+ b
++c
+diff --git a/car b/car
+index 00750ed..2ae5e34 100644
+--- a/car
++++ b/car
+@@ -1 +1,2 @@
+ 3
++d
+diff --git a/foo b/foo
+index e439850..7146eb8 100644
+--- a/foo
++++ b/foo
+@@ -2 +2,2 @@
+ a
++e
+EOF
+
+test_expect_success 'patch-id handles diffs with one line of before/after'
+	cat diffu1 | calc_patch_id diffu1 &&
+	test_config patchid.stable true &&
+	cat diffu1 | calc_patch_id diffu1stable
+'
 test_done

-- 
2.32.0.1314.g6ed4fcc4cc
Junio C Hamano· Jan 31, 2022, 18:55 UTC · re: Jerry Zhang · lore

Re: [PATCH V2] patch-id: fix scan_hunk_header on diffs with 1 line of before/after

Jerry Zhang <jerry@skydio.com> writes:
> Normally diffs will contain a hunk header of the format
> "@@ -2,2 +2,15 @@ code". However in certain situations git can
> omit the second comma separated value in either before or after
> line counts in the case where there is only one line.

No need to resend, but this is specific enough (i.e. ",1" is omitted) and "in certain situations" makes it needlessly fuzzy, making readers wonder if there are any other cases. Also, this is not "git can omit". The "unified context" format output from any "diff" command share this special case.

Otherwise looking very good.
Thanks for a fix.  Will queue.
Junio C Hamano· Jan 31, 2022, 22:54 UTC · re: Jerry Zhang · lore

Re: [PATCH V2] patch-id: fix scan_hunk_header on diffs with 1 line of before/after

Jerry Zhang <jerry@skydio.com> writes:
Show 116 quoted lines
> Normally diffs will contain a hunk header of the format
> "@@ -2,2 +2,15 @@ code". However in certain situations git can
> omit the second comma separated value in either before or after
> line counts in the case where there is only one line.
>
> This can produce hunk headers that look like
> "@@ -2 +2,18 @@ code" or "@@ -2,2 +2 @@ code".
> As a result, scan_hunk_header mistakenly returns the line
> number as line count, which then results in unpredictable
> parsing errors with the rest of the patch, including giving
> multiple lines of output for a single commit.
>
> Fix by explicitly setting line count to 1 when there is
> no comma, and add a test.
>
> apply.c contains this same logic except it is correct. A
> worthwhile future project might be to unify these two diff
> parsers so they both benefit from fixes.
>
> Signed-off-by: Jerry Zhang <jerry@skydio.com>
> ---
> V1->V2:
> - Removed references to -U1 since this isn't limited to that
> - Added notes for future tasks
>
>  builtin/patch-id.c  |  9 +++++++--
>  t/t4204-patch-id.sh | 32 +++++++++++++++++++++++++++++++-
>  2 files changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/patch-id.c b/builtin/patch-id.c
> index 822ffff51f..881fcf3273 100644
> --- a/builtin/patch-id.c
> +++ b/builtin/patch-id.c
> @@ -30,26 +30,31 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)
>  
>  	q = p + 4;
>  	n = strspn(q, digits);
>  	if (q[n] == ',') {
>  		q += n + 1;
> +		*p_before = atoi(q);
>  		n = strspn(q, digits);
> +	} else {
> +		*p_before = 1;
>  	}
> +
>  	if (n == 0 || q[n] != ' ' || q[n+1] != '+')
>  		return 0;
>  
>  	r = q + n + 2;
>  	n = strspn(r, digits);
>  	if (r[n] == ',') {
>  		r += n + 1;
> +		*p_after = atoi(r);
>  		n = strspn(r, digits);
> +	} else {
> +		*p_after = 1;
>  	}
>  	if (n == 0)
>  		return 0;
>  
> -	*p_before = atoi(q);
> -	*p_after = atoi(r);
>  	return 1;
>  }
>  
>  static int get_one_patchid(struct object_id *next_oid, struct object_id *result,
>  			   struct strbuf *line_buf, int stable)
> diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
> index 80f4a65b28..34a386aee2 100755
> --- a/t/t4204-patch-id.sh
> +++ b/t/t4204-patch-id.sh
> @@ -36,11 +36,11 @@ test_expect_success 'patch-id output is well-formed' '
>  calc_patch_id () {
>  	patch_name="$1"
>  	shift
>  	git patch-id "$@" >patch-id.output &&
>  	sed "s/ .*//" patch-id.output >patch-id_"$patch_name" &&
> -	test_line_count -gt 0 patch-id_"$patch_name"
> +	test_line_count -eq 1 patch-id_"$patch_name"
>  }
>  
>  get_top_diff () {
>  	git log -p -1 "$@" -O bar-then-foo --
>  }
> @@ -200,6 +200,36 @@ EOF
>  test_expect_success 'patch-id handles no-nl-at-eof markers' '
>  	cat nonl | calc_patch_id nonl &&
>  	cat withnl | calc_patch_id withnl &&
>  	test_cmp patch-id_nonl patch-id_withnl
>  '
> +
> +cat >diffu1 <<\EOF
> +diff --git a/bar b/bar
> +index bdaf90f..31051f6 100644
> +--- a/bar
> ++++ b/bar
> +@@ -2 +2,2 @@
> + b
> ++c
> +diff --git a/car b/car
> +index 00750ed..2ae5e34 100644
> +--- a/car
> ++++ b/car
> +@@ -1 +1,2 @@
> + 3
> ++d
> +diff --git a/foo b/foo
> +index e439850..7146eb8 100644
> +--- a/foo
> ++++ b/foo
> +@@ -2 +2,2 @@
> + a
> ++e
> +EOF
> +
> +test_expect_success 'patch-id handles diffs with one line of before/after'
There is a opening quote for the body missing here.
Show 5 quoted lines
> +	cat diffu1 | calc_patch_id diffu1 &&
> +	test_config patchid.stable true &&
> +	cat diffu1 | calc_patch_id diffu1stable
> +'
>  test_done
Junio C Hamano· Jan 31, 2022, 23:05 UTC · re: Jerry Zhang · lore

Re: [PATCH V2] patch-id: fix scan_hunk_header on diffs with 1 line of before/after

Jerry Zhang <jerry@skydio.com> writes:
Show 10 quoted lines
> diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
> index 80f4a65b28..34a386aee2 100755
> --- a/t/t4204-patch-id.sh
> +++ b/t/t4204-patch-id.sh
> @@ -200,6 +200,36 @@ EOF
>  test_expect_success 'patch-id handles no-nl-at-eof markers' '
>  	cat nonl | calc_patch_id nonl &&
>  	cat withnl | calc_patch_id withnl &&
>  	test_cmp patch-id_nonl patch-id_withnl
>  '

This is not a new issue, but this old test script needs updating. And worse yet, your new test copies the existing badness and make it twice as bad.

We frown upon doing anything outside test_expect_* blocks these days, so the commands to prepare nonl and withnl files before thie precontext of this patch needs to be folded into the test body.

Also, sending a single file using "cat" into pipeline is an antipattern. Just redirect into the downstream command instead, i.e.

	test_expect_success 'patch-id handles no-nl-at-eof markers' '
		cat >nonl <<-\EOF &&
                diff --git i/a w/a
                index e69de29..2e65efe 100644
                --- i/a
                +++ w/a
                @@ -0,0 +1 @@
                +a
                \ No newline at end of file
                diff --git i/b w/b
                index e69de29..6178079 100644
                --- i/b
                +++ w/b
                @@ -0,0 +1 @@
                +b
                EOF
		cat >withnl <<-\EOF &&
		... likewise ...
		EOF
		calc_patch_id nonl <nonl &&
		calc_patch_id withnl <withnl &&
		test_cmp patch-i_ nonl patch-id-withnl
	'
	
or something like that.

I think the "handles no-nl-at-eof markers" test at the end is the first and only serious offender, so instead of leaving it after the series, we probalby would want to have a preliminary clean-up patch before this patch, and fix the test this patch adds.

Thanks.

← back to recent threads