t4204-patch-id failures

t4204-patch-id failures

[Armin Kunaschik]

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

Re: t4204-patch-id failures

[Junio C Hamano]

Armin Kunaschik <megabreit@googlemail.com> writes:

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

Re: t4204-patch-id failures

[Junio C Hamano]

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

Re: t4204-patch-id failures

[Armin Kunaschik]

On Tue, May 24, 2016 at 12:23 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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

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

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

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

Agreed.

> Reported-by: Armin Kunaschik <megabreit@googlemail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>