Re: t4204-patch-id failures

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

Junio C Hamano <gitster@pobox.com> writes:

> 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 --

Having said all that, this illustrates the root cause of different
behaviours better, but it is harder to reason about than simply
changing the variable name used in this shell function.  POSIX reads
a bit fuzzy to me around here:

    ... each command of a multi-command pipeline is in a subshell
    environment; as an extension, however, any or all commands in a
    pipeline may be executed in the current environment. All other
    commands shall be executed in the current shell environment.

That essentially says nothing useful; it does not guarantee that
each command on a pipeline runs in its own subshell environment, and
a portable script must be prepared to see some of them run in the
current shell environment.

So let's do this instead:

-- >8 --
Subject: t4204: do not let $name variable clobbered

test_patch_id_file_order shell function uses $name variable to hold
one filename, and calls another shell function calc_patch_id as a
downstream of one pipeline.  The called function, however, also uses
the same $name variable.  With a shell implementation that runs the
callee in the current shell environment, the caller's $name would
be clobbered by the callee's use of the same variable.

This hasn't been an issue with dash and bash.  ksh93 reveals the
breakage in the test script.

Fix it by using a distinct variable name in the callee.

Reported-by: Armin Kunaschik <megabreit@googlemail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4204-patch-id.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
index baa9d3c..84a8096 100755
--- a/t/t4204-patch-id.sh
+++ b/t/t4204-patch-id.sh
@@ -30,11 +30,11 @@ test_expect_success 'patch-id output is well-formed' '
 
 #calculate patch id. Make sure output is not empty.
 calc_patch_id () {
-	name="$1"
+	patch_name="$1"
 	shift
 	git patch-id "$@" |
-	sed "s/ .*//" >patch-id_"$name" &&
-	test_line_count -gt 0 patch-id_"$name"
+	sed "s/ .*//" >patch-id_"$patch_name" &&
+	test_line_count -gt 0 patch-id_"$patch_name"
 }
 
 get_top_diff () {