Re: [PATCH v3 3/3] patch-id-test: test --stable and --unstable flags

[Junio C Hamano <gitster@pobox.com>]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> Verify that patch ID is now stable against diff split and reordering.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> Changes from v2:
> 	added test to verify patch ID is stable against diff splitting
>
>  t/t4204-patch-id.sh | 117 ++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 109 insertions(+), 8 deletions(-)
>
> diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
> index d2c930d..1679714 100755
> --- a/t/t4204-patch-id.sh
> +++ b/t/t4204-patch-id.sh
> @@ -5,12 +5,46 @@ test_description='git patch-id'
>  . ./test-lib.sh
>  
>  test_expect_success 'setup' '
> -	test_commit initial foo a &&
> -	test_commit first foo b &&
> -	git checkout -b same HEAD^ &&
> -	test_commit same-msg foo b &&
> -	git checkout -b notsame HEAD^ &&
> -	test_commit notsame-msg foo c
> +	cat > a <<-\EOF &&

Please do not add an extra space between the redirection operator
(e.g. ">", "2>", etc.) and its operand.  The same style violations
everywhere in this patch.

> +		a
> +		a
> +		a
> +		a
> +		a
> +		a
> +		a
> +		a
> +		EOF

Please align EOF with the cat that receives it, and pretend that the
column the head of the cat is at is the leftmost column for the
payload, when writing <<- here-document, e.g.

        cat >a <<-\EOF &&
        a
        EOF

> +	(cat a; echo b) > ab &&
> +	(echo d; cat a; echo b) > dab &&
> +	cp a foo &&
> +	cp a bar &&

Probably a helper function would make it more apparent what is going
on?

	as="a a a a a a a a" && # eight a
	test_write_lines $as b >ab &&
	test_write_lines d $as b >dab &&
        test_write_lines $as >foo &&
        test_write_lines $as >bar &&

Or even better, use test_write_lines in places you do use the result
to overwrite foo/bar and get rid of ab and dab?

That helper can also be used to prepare bar-then-foo.

>  test_expect_success 'patch-id output is well-formed' '
> @@ -23,11 +57,33 @@ calc_patch_id () {
>  		sed "s# .*##" > patch-id_"$1"
>  }
>  
> +calc_patch_id_unstable () {
> +	git patch-id --unstable |
> +		sed "s# .*##" > patch-id_"$1"

Not a new problem, but can you align "git patch-id" and "sed" to the
same column?  Also, not using "/" when there is no slash involved in
the pattern makes readers waste their time wondering why the script
avoids it.

> +}
> +
> +calc_patch_id_stable () {
> +	git patch-id --stable |
> +		sed "s# .*##" > patch-id_"$1"
> +}
> +
> +

Extra blank line.

>  get_patch_id () {
> -	git log -p -1 "$1" | git patch-id |
> +	git log -p -1 "$1" -O bar-then-foo -- | git patch-id |
>  		sed "s# .*##" > patch-id_"$1"
>  }
>  
> +get_patch_id_stable () {
> +	git log -p -1 "$1" -O bar-then-foo | git patch-id --stable |
> +		sed "s# .*##" > patch-id_"$1"

Why doesn't it use calc_patch_id_stable?

> +}
> +
> +get_patch_id_unstable () {
> +	git log -p -1 "$1" -O bar-then-foo | git patch-id --unstable |
> +		sed "s# .*##" > patch-id_"$1"

Ditto.

> +}
> +
> +

Extra blank line.

>  test_expect_success 'patch-id detects equality' '
>  	get_patch_id master &&
>  	get_patch_id same &&
> @@ -52,10 +108,55 @@ test_expect_success 'patch-id supports git-format-patch output' '
>  test_expect_success 'whitespace is irrelevant in footer' '
>  	get_patch_id master &&
>  	git checkout same &&
> -	git format-patch -1 --stdout | sed "s/ \$//" | calc_patch_id same &&
> +	git format-patch -1 --stdout | sed "s/ \$//" |
> +		calc_patch_id same &&

What is this change about?