{"thread":{"id":"42420","subject":"t4204-patch-id failures","startedAt":"2016-05-23T16:30:38Z","lastAt":"2016-05-23T22:56:18Z","messageCount":4,"participants":["Armin Kunaschik","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"287257","messageId":"CALR6jEgf_FiGWs=45+n8uzKEiXU7yKDsP+CjOUAWu1CnUXZbPw@mail.gmail.com","threadId":"42420","inReplyTo":null,"subject":"t4204-patch-id failures","fromName":"Armin Kunaschik","fromEmail":"megabreit@googlemail.com","sentAt":"2016-05-23T16:30:38Z","receivedAt":"2016-05-23T16:30:38Z","isPatch":false,"sender":{"key":"megabreit@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/10429218?v=4"},"body":"Hello,\n\nI see 3 test failures in t4202:\n\nexpecting success:\n        test_patch_id_file_order irrelevant --stable --stable\n\nAlready on 'same'\ncmp: cannot open patch-id_ordered-ordered-order---stable-irrelevant\nnot ok 7 - file order is irrelevant with --stable\n#\n#               test_patch_id_file_order irrelevant --stable --stable\n#\n\nexpecting success:\n        test_patch_id_file_order relevant --unstable --unstable\n\nAlready on 'same'\ncmp: cannot open patch-id_ordered-ordered-order---unstable-relevant\n[..]\n\nexpecting success:\n        test_config patchid.stable true &&\n        test_patch_id irrelevant patchid.stable=true\n\nAlready on 'same'\ncmp: cannot open patch-id_ordered-ordered-order-patchid.stable=true-irrelevant\nnot ok 10 - patchid.stable = true is stable\n#\n#               test_config patchid.stable true &&\n#               test_patch_id irrelevant patchid.stable=true\n#\n[..]\n\nexpecting success:\n        test_config patchid.stable false &&\n        test_patch_id irrelevant patchid.stable=false--stable --stable\n\nAlready on 'same'\ncmp: cannot open\npatch-id_ordered-ordered-order-patchid.stable=false--stable-irrelevant\nnot ok 13 - --stable overrides patchid.stable = false\n#\n#               test_config patchid.stable false &&\n#               test_patch_id irrelevant patchid.stable=false--stable --stable\n#\n\n\nPlease notice the double \"ordered\"!\nFrom my point of view there is a problem in the function\ncalc_patch_id() which changes\nthe variable $name and causes the test to fail.\n\nEssentially it's working like this:\n<snip>\n#!/bin/bash\n\nfunc1() {\n        name=${1}\n        echo \"func1 name=$name\"\n}\n\nfunc2() {\n        name=${1}\n        echo \"func2 name=$name\"\n        func1 \"ordered-$name\"\n        echo \"func2 again name=$name\"\n}\n\nfunc2 foo\n<snip>\nwhich prints in bash, ksh88 and ksh93 on Linux and AIX the same\nfunc2 name=foo\nfunc1 name=ordered-foo\nfunc2 again name=ordered-foo\n\nI wonder if those 3 tests ever worked... and why? :-)\n\nA suggested patch would be to rename $name inside calc_patch_id into\nsomething else\nexcept $name... e.g. $pname.\n\nArmin\n"},{"id":"287311","messageId":"xmqqiny48ps8.fsf@gitster.mtv.corp.google.com","threadId":"42420","inReplyTo":"CALR6jEgf_FiGWs=45+n8uzKEiXU7yKDsP+CjOUAWu1CnUXZbPw@mail.gmail.com","subject":"Re: t4204-patch-id failures","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-23T20:47:51Z","receivedAt":"2016-05-23T20:47:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Armin Kunaschik <megabreit@googlemail.com> writes:\n\n> Essentially it's working like this:\n> <snip>\n> #!/bin/bash\n>\n> func1() {\n>         name=${1}\n>         echo \"func1 name=$name\"\n> }\n>\n> func2() {\n>         name=${1}\n>         echo \"func2 name=$name\"\n>         func1 \"ordered-$name\"\n>         echo \"func2 again name=$name\"\n> }\n>\n> func2 foo\n> <snip>\n\n\nI think what you have are these two functions interacting in an\nunexpected way:\n\ntest_patch_id_file_order () {\n\trelevant=\"$1\"\n\tshift\n\tname=\"order-${1}-$relevant\"\n\tshift\n\tget_top_diff \"master\" | calc_patch_id \"$name\" \"$@\" &&\n\tgit checkout same &&\n\tgit format-patch -1 --stdout -O foo-then-bar |\n\t\tcalc_patch_id \"ordered-$name\" \"$@\" &&\n\tcmp_patch_id $relevant \"$name\" \"ordered-$name\"\n\n}\n\ncalc_patch_id () {\n\tname=\"$1\"\n\tshift\n\tgit patch-id \"$@\" |\n\tsed \"s/ .*//\" >patch-id_\"$name\" &&\n\ttest_line_count -gt 0 patch-id_\"$name\"\n}\n\n\nThe first call to calc_patch_id passes $name and the called helper\nreceives it as $name, but the second call to it passes \"ordered-$name\"\nwhich is assigned by the function to $name.\n\nYour example over-simplified these two and lacks the pipeline to\nmake the invocation of func1 as downstream of a pipe in func2; that\nis why your simplified example makes you wonder why these two work\non other people's shells; i.e. you need to replace your func2 to\nthis in order to mimick what is really going on:\n\nfunc2 () {\n        name=${1}\n        echo \"func2 name=$name\"\n        echo foo | func1 \"ordered-$name\"\n        echo \"func2 again name=$name\"\n}\n\nBoth bash and dash seem to run the func1 in the downstream of the\npipe in a separate process, and $name used in \"func2 again\" is not\naffected.  But it seems that ksh93 behaves differently (I do not\nhave access to ksh88).\n\nAn obvious workaround is to replace your func1 to\n\nfunc1 () (\n\tname=$1\n        echo \"func1 name=$name\"\n)\n\nto force it to be run in its own process without disrupting $name.\n\nPerhaps like this?\n\n t/t4204-patch-id.sh | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex baa9d3c..b8bd467 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -28,14 +28,18 @@ test_expect_success 'patch-id output is well-formed' '\n \tgrep \"^[a-f0-9]\\{40\\} $(git rev-parse HEAD)$\" output\n '\n \n-#calculate patch id. Make sure output is not empty.\n-calc_patch_id () {\n+# calculate patch id. Make sure output is not empty.\n+# Because ksh lets this helper run as a downstream of a pipe in\n+# test_patch_id_file_order and ends up clobbering $name, make\n+# sure it is run as a separate process by using (body) not {body}\n+\n+calc_patch_id () (\n \tname=\"$1\"\n \tshift\n \tgit patch-id \"$@\" |\n \tsed \"s/ .*//\" >patch-id_\"$name\" &&\n \ttest_line_count -gt 0 patch-id_\"$name\"\n-}\n+)\n \n get_top_diff () {\n \tgit log -p -1 \"$@\" -O bar-then-foo --\n"},{"id":"287325","messageId":"xmqqbn3w76rn.fsf@gitster.mtv.corp.google.com","threadId":"42420","inReplyTo":"xmqqiny48ps8.fsf@gitster.mtv.corp.google.com","subject":"Re: t4204-patch-id failures","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-23T22:23:56Z","receivedAt":"2016-05-23T22:23:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Both bash and dash seem to run the func1 in the downstream of the\n> pipe in a separate process, and $name used in \"func2 again\" is not\n> affected.  But it seems that ksh93 behaves differently (I do not\n> have access to ksh88).\n>\n> An obvious workaround is to replace your func1 to\n>\n> func1 () (\n> \tname=$1\n>         echo \"func1 name=$name\"\n> )\n>\n> to force it to be run in its own process without disrupting $name.\n>\n> Perhaps like this?\n> ...\n>  t/t4204-patch-id.sh | 10 +++++++---\n>  1 file changed, 7 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\n> index baa9d3c..b8bd467 100755\n> --- a/t/t4204-patch-id.sh\n> +++ b/t/t4204-patch-id.sh\n> @@ -28,14 +28,18 @@ test_expect_success 'patch-id output is well-formed' '\n>  \tgrep \"^[a-f0-9]\\{40\\} $(git rev-parse HEAD)$\" output\n>  '\n>  \n> -#calculate patch id. Make sure output is not empty.\n> -calc_patch_id () {\n> +# calculate patch id. Make sure output is not empty.\n> +# Because ksh lets this helper run as a downstream of a pipe in\n> +# test_patch_id_file_order and ends up clobbering $name, make\n> +# sure it is run as a separate process by using (body) not {body}\n> +\n> +calc_patch_id () (\n>  \tname=\"$1\"\n>  \tshift\n>  \tgit patch-id \"$@\" |\n>  \tsed \"s/ .*//\" >patch-id_\"$name\" &&\n>  \ttest_line_count -gt 0 patch-id_\"$name\"\n> -}\n> +)\n>  \n>  get_top_diff () {\n>  \tgit log -p -1 \"$@\" -O bar-then-foo --\n\nHaving said all that, this illustrates the root cause of different\nbehaviours better, but it is harder to reason about than simply\nchanging the variable name used in this shell function.  POSIX reads\na bit fuzzy to me around here:\n\n    ... each command of a multi-command pipeline is in a subshell\n    environment; as an extension, however, any or all commands in a\n    pipeline may be executed in the current environment. All other\n    commands shall be executed in the current shell environment.\n\nThat essentially says nothing useful; it does not guarantee that\neach command on a pipeline runs in its own subshell environment, and\na portable script must be prepared to see some of them run in the\ncurrent shell environment.\n\nSo let's do this instead:\n\n-- >8 --\nSubject: t4204: do not let $name variable clobbered\n\ntest_patch_id_file_order shell function uses $name variable to hold\none filename, and calls another shell function calc_patch_id as a\ndownstream of one pipeline.  The called function, however, also uses\nthe same $name variable.  With a shell implementation that runs the\ncallee in the current shell environment, the caller's $name would\nbe clobbered by the callee's use of the same variable.\n\nThis hasn't been an issue with dash and bash.  ksh93 reveals the\nbreakage in the test script.\n\nFix it by using a distinct variable name in the callee.\n\nReported-by: Armin Kunaschik <megabreit@googlemail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t4204-patch-id.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex baa9d3c..84a8096 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -30,11 +30,11 @@ test_expect_success 'patch-id output is well-formed' '\n \n #calculate patch id. Make sure output is not empty.\n calc_patch_id () {\n-\tname=\"$1\"\n+\tpatch_name=\"$1\"\n \tshift\n \tgit patch-id \"$@\" |\n-\tsed \"s/ .*//\" >patch-id_\"$name\" &&\n-\ttest_line_count -gt 0 patch-id_\"$name\"\n+\tsed \"s/ .*//\" >patch-id_\"$patch_name\" &&\n+\ttest_line_count -gt 0 patch-id_\"$patch_name\"\n }\n \n get_top_diff () {\n"},{"id":"287327","messageId":"CALR6jEgjcAGjLBbHuB+sG24Xwp9AH1i8SbtSdQoLyFcKmwAAcA@mail.gmail.com","threadId":"42420","inReplyTo":"xmqqbn3w76rn.fsf@gitster.mtv.corp.google.com","subject":"Re: t4204-patch-id failures","fromName":"Armin Kunaschik","fromEmail":"megabreit@googlemail.com","sentAt":"2016-05-23T22:56:18Z","receivedAt":"2016-05-23T22:56:18Z","isPatch":false,"sender":{"key":"megabreit@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/10429218?v=4"},"body":"On Tue, May 24, 2016 at 12:23 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n\n> Having said all that, this illustrates the root cause of different\n> behaviours better, but it is harder to reason about than simply\n> changing the variable name used in this shell function.  POSIX reads\n> a bit fuzzy to me around here:\n>\n>     ... each command of a multi-command pipeline is in a subshell\n>     environment; as an extension, however, any or all commands in a\n>     pipeline may be executed in the current environment. All other\n>     commands shall be executed in the current shell environment.\n>\n> That essentially says nothing useful; it does not guarantee that\n> each command on a pipeline runs in its own subshell environment, and\n> a portable script must be prepared to see some of them run in the\n> current shell environment.\n\nWriting portable shell scripts sometimes is quite a mess :-)\n\n> So let's do this instead:\n>\n> -- >8 --\n> Subject: t4204: do not let $name variable clobbered\n>\n> test_patch_id_file_order shell function uses $name variable to hold\n> one filename, and calls another shell function calc_patch_id as a\n> downstream of one pipeline.  The called function, however, also uses\n> the same $name variable.  With a shell implementation that runs the\n> callee in the current shell environment, the caller's $name would\n> be clobbered by the callee's use of the same variable.\n>\n> This hasn't been an issue with dash and bash.  ksh93 reveals the\n> breakage in the test script.\n\nMaybe add ksh88 too, just to be \"feature\" complete.\n\n> Fix it by using a distinct variable name in the callee.\n\nAgreed.\n\n> Reported-by: Armin Kunaschik <megabreit@googlemail.com>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n"}]}