Re: t4204-patch-id failures

From: Junio C Hamano <gitster@pobox.com>
Date: May 23, 2016, 20:47 UTC
Message-ID: <xmqqiny48ps8.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CALR6jEgf_FiGWs=45+n8uzKEiXU7yKDsP+CjOUAWu1CnUXZbPw@mail.gmail.com>

Armin Kunaschik <megabreit@googlemail.com> writes:

Show 18 quoted lines

> Essentially it's working like this:
> <snip>
> #!/bin/bash
>
> func1() {
>         name=${1}
>         echo "func1 name=$name"
> }
>
> func2() {
>         name=${1}
>         echo "func2 name=$name"
>         func1 "ordered-$name"
>         echo "func2 again name=$name"
> }
>
> func2 foo
> <snip>

I think what you have are these two functions interacting in an unexpected way:

test_patch_id_file_order () {
	relevant="$1"
	shift
	name="order-${1}-$relevant"
	shift
	get_top_diff "master" | calc_patch_id "$name" "$@" &&
	git checkout same &&
	git format-patch -1 --stdout -O foo-then-bar |
		calc_patch_id "ordered-$name" "$@" &&
	cmp_patch_id $relevant "$name" "ordered-$name"

calc_patch_id () {
	name="$1"
	shift
	git patch-id "$@" |
	sed "s/ .*//" >patch-id_"$name" &&
	test_line_count -gt 0 patch-id_"$name"
}

The first call to calc_patch_id passes $name and the called helper receives it as $name, but the second call to it passes "ordered-$name" which is assigned by the function to $name.

Your example over-simplified these two and lacks the pipeline to make the invocation of func1 as downstream of a pipe in func2; that is why your simplified example makes you wonder why these two work on other people's shells; i.e. you need to replace your func2 to this in order to mimick what is really going on:

func2 () {
        name=${1}
        echo "func2 name=$name"
        echo foo | func1 "ordered-$name"
        echo "func2 again name=$name"
}

Both bash and dash seem to run the func1 in the downstream of the pipe in a separate process, and $name used in "func2 again" is not affected. But it seems that ksh93 behaves differently (I do not have access to ksh88).

An obvious workaround is to replace your func1 to

func1 () (
	name=$1
        echo "func1 name=$name"
)

to force it to be run in its own process without disrupting $name.

Perhaps like this?

 t/t4204-patch-id.sh | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
index baa9d3c..b8bd467 100755
--- a/t/t4204-patch-id.sh
+++ b/t/t4204-patch-id.sh
@@ -28,14 +28,18 @@ test_expect_success 'patch-id output is well-formed' '
 	grep "^[a-f0-9]\{40\} $(git rev-parse HEAD)$" output
 '
 
-#calculate patch id. Make sure output is not empty.
-calc_patch_id () {
+# calculate patch id. Make sure output is not empty.
+# Because ksh lets this helper run as a downstream of a pipe in
+# test_patch_id_file_order and ends up clobbering $name, make
+# sure it is run as a separate process by using (body) not {body}
+
+calc_patch_id () (
 	name="$1"
 	shift
 	git patch-id "$@" |
 	sed "s/ .*//" >patch-id_"$name" &&
 	test_line_count -gt 0 patch-id_"$name"
-}
+)
 
 get_top_diff () {
 	git log -p -1 "$@" -O bar-then-foo --