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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 31, 2022, 23:05 UTC
Message-ID: <xmqqmtjbh5fu.fsf@gitster.g>
In-Reply-To: <20220129024757.24763-1-jerry@skydio.com>

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.