t4204-patch-id failures

Hello,

I see 3 test failures in t4202:

expecting success:
        test_patch_id_file_order irrelevant --stable --stable

Already on 'same' cmp: cannot open patch-id_ordered-ordered-order---stable-irrelevant not ok 7 - file order is irrelevant with --stable # # test_patch_id_file_order irrelevant --stable --stable #

expecting success:
        test_patch_id_file_order relevant --unstable --unstable

Already on 'same' cmp: cannot open patch-id_ordered-ordered-order---unstable-relevant [..]

expecting success:
        test_config patchid.stable true &&
        test_patch_id irrelevant patchid.stable=true

Already on 'same' cmp: cannot open patch-id_ordered-ordered-order-patchid.stable=true-irrelevant not ok 10 - patchid.stable = true is stable # # test_config patchid.stable true && # test_patch_id irrelevant patchid.stable=true # [..]

expecting success:
        test_config patchid.stable false &&
        test_patch_id irrelevant patchid.stable=false--stable --stable

Already on 'same' cmp: cannot open patch-id_ordered-ordered-order-patchid.stable=false--stable-irrelevant not ok 13 - --stable overrides patchid.stable = false # # test_config patchid.stable false && # test_patch_id irrelevant patchid.stable=false--stable --stable #

Please notice the double "ordered"! From my point of view there is a problem in the function calc_patch_id() which changes the variable $name and causes the test to fail.

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> which prints in bash, ksh88 and ksh93 on Linux and AIX the same func2 name=foo func1 name=ordered-foo func2 again name=ordered-foo

I wonder if those 3 tests ever worked... and why? :-)

A suggested patch would be to rename $name inside calc_patch_id into something else except $name... e.g. $pname.

Armin

Armin Kunaschik <megabreit@googlemail.com> writes:

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

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

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

On Tue, May 24, 2016 at 12:23 AM, Junio C Hamano <gitster@pobox.com> wrote:

Writing portable shell scripts sometimes is quite a mess :-)

Maybe add ksh88 too, just to be "feature" complete.

Agreed.