{"thread":{"id":"32563","subject":"[PATCH] t1402: work around shell quoting issue on NetBSD","startedAt":"2013-01-08T20:23:01Z","lastAt":"2013-01-10T21:24:52Z","messageCount":11,"participants":["René Scharfe","Junio C Hamano","Greg Troxel","Adam Spiers"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"206330","messageId":"50EC8025.8000707@lsrfire.ath.cx","threadId":"32563","inReplyTo":null,"subject":"[PATCH] t1402: work around shell quoting issue on NetBSD","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-01-08T20:23:01Z","receivedAt":"2013-01-08T20:23:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The test fails for me on NetBSD 6.0.1 and reports:\n\n\tok 1 - ref name '' is invalid\n\tok 2 - ref name '/' is invalid\n\tok 3 - ref name '/' is invalid with options --allow-onelevel\n\tok 4 - ref name '/' is invalid with options --normalize\n\terror: bug in the test script: not 2 or 3 parameters to test-expect-success\n\nThe alleged bug is in this line:\n\n\tinvalid_ref NOT_MINGW '/' '--allow-onelevel --normalize'\n\ninvalid_ref() constructs a test case description using its last argument,\nbut the shell seems to split it up into two pieces if it contains a\nspace. Minimal test case:\n\n\t# on NetBSD with /bin/sh\n\t$ a() { echo $#-$1-$2; }\n\t$ t=\"x\"; a \"${t:+$t}\"\n\t1-x-\n\t$ t=\"x y\"; a \"${t:+$t}\"\n\t2-x-y\n\t$ t=\"x y\"; a \"${t:+x y}\"\n\t1-x y-\n\n\t# and with bash\n\t$ t=\"x y\"; a \"${t:+$t}\"\n\t1-x y-\n\t$ t=\"x y\"; a \"${t:+x y}\"\n\t1-x y-\n\nThis may be a bug in the shell, but here's a simple workaround: Construct\nthe description string first and store it in a variable, and then use\nthat to call test_expect_success().\n\nSigned-off-by: Rene Scharfe \n---\n t/t1402-check-ref-format.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh\nindex 1ae4d87..1a5a5f3 100755\n--- a/t/t1402-check-ref-format.sh\n+++ b/t/t1402-check-ref-format.sh\n@@ -11,7 +11,8 @@ valid_ref() {\n \t\tprereq=$1\n \t\tshift\n \tesac\n-\ttest_expect_success $prereq \"ref name '$1' is valid${2:+ with options $2}\" \"\n+\tdesc=\"ref name '$1' is valid${2:+ with options $2}\"\n+\ttest_expect_success $prereq \"$desc\" \"\n \t\tgit check-ref-format $2 '$1'\n \t\"\n }\n@@ -22,7 +23,8 @@ invalid_ref() {\n \t\tprereq=$1\n \t\tshift\n \tesac\n-\ttest_expect_success $prereq \"ref name '$1' is invalid${2:+ with options $2}\" \"\n+\tdesc=\"ref name '$1' is invalid${2:+ with options $2}\"\n+\ttest_expect_success $prereq \"$desc\" \"\n \t\ttest_must_fail git check-ref-format $2 '$1'\n \t\"\n }\n-- \n1.7.12\n"},{"id":"206331","messageId":"7vr4lvcstt.fsf@alter.siamese.dyndns.org","threadId":"32563","inReplyTo":"50EC8025.8000707@lsrfire.ath.cx","subject":"Re: [PATCH] t1402: work around shell quoting issue on NetBSD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-08T20:39:42Z","receivedAt":"2013-01-08T20:39:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> The test fails for me on NetBSD 6.0.1 and reports:\n>\n> \tok 1 - ref name '' is invalid\n> \tok 2 - ref name '/' is invalid\n> \tok 3 - ref name '/' is invalid with options --allow-onelevel\n> \tok 4 - ref name '/' is invalid with options --normalize\n> \terror: bug in the test script: not 2 or 3 parameters to test-expect-success\n>\n> The alleged bug is in this line:\n>\n> \tinvalid_ref NOT_MINGW '/' '--allow-onelevel --normalize'\n>\n> invalid_ref() constructs a test case description using its last argument,\n> but the shell seems to split it up into two pieces if it contains a\n> space. Minimal test case:\n>\n> \t# on NetBSD with /bin/sh\n> \t$ a() { echo $#-$1-$2; }\n> \t$ t=\"x\"; a \"${t:+$t}\"\n> \t1-x-\n> \t$ t=\"x y\"; a \"${t:+$t}\"\n> \t2-x-y\n> \t$ t=\"x y\"; a \"${t:+x y}\"\n> \t1-x y-\n>\n> \t# and with bash\n> \t$ t=\"x y\"; a \"${t:+$t}\"\n> \t1-x y-\n> \t$ t=\"x y\"; a \"${t:+x y}\"\n> \t1-x y-\n>\n> This may be a bug in the shell, but here's a simple workaround: Construct\n> the description string first and store it in a variable, and then use\n> that to call test_expect_success().\n\nInteresting. I notice that t0008 added recently to 'pu' has the\nsame construct.\n\n>\n> Signed-off-by: Rene Scharfe \n> ---\n> t/t1402-check-ref-format.sh | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh\n> index 1ae4d87..1a5a5f3 100755\n> --- a/t/t1402-check-ref-format.sh\n> +++ b/t/t1402-check-ref-format.sh\n> @@ -11,7 +11,8 @@ valid_ref() {\n> \t\tprereq=$1\n> \t\tshift\n> \tesac\n> -\ttest_expect_success $prereq \"ref name '$1' is valid${2:+ with options $2}\" \"\n> +\tdesc=\"ref name '$1' is valid${2:+ with options $2}\"\n> +\ttest_expect_success $prereq \"$desc\" \"\n> \t\tgit check-ref-format $2 '$1'\n> \t\"\n> }\n> @@ -22,7 +23,8 @@ invalid_ref() {\n> \t\tprereq=$1\n> \t\tshift\n> \tesac\n> -\ttest_expect_success $prereq \"ref name '$1' is invalid${2:+ with options $2}\" \"\n> +\tdesc=\"ref name '$1' is invalid${2:+ with options $2}\"\n> +\ttest_expect_success $prereq \"$desc\" \"\n> \t\ttest_must_fail git check-ref-format $2 '$1'\n> \t\"\n> }\n"},{"id":"206332","messageId":"50EC8BE7.2010508@lsrfire.ath.cx","threadId":"32563","inReplyTo":"7vr4lvcstt.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t1402: work around shell quoting issue on NetBSD","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-01-08T21:13:11Z","receivedAt":"2013-01-08T21:13:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.01.2013 21:39, schrieb Junio C Hamano:\n> René Scharfe writes:\n>> \t# on NetBSD with /bin/sh\n>> \t$ a() { echo $#-$1-$2; }\n>> \t$ t=\"x\"; a \"${t:+$t}\"\n>> \t1-x-\n>> \t$ t=\"x y\"; a \"${t:+$t}\"\n>> \t2-x-y\n>> \t$ t=\"x y\"; a \"${t:+x y}\"\n>> \t1-x y-\n>>\n>> \t# and with bash\n>> \t$ t=\"x y\"; a \"${t:+$t}\"\n>> \t1-x y-\n>> \t$ t=\"x y\"; a \"${t:+x y}\"\n>> \t1-x y-\n>>\n>> This may be a bug in the shell, but here's a simple workaround: Construct\n>> the description string first and store it in a variable, and then use\n>> that to call test_expect_success().\n>\n> Interesting. I notice that t0008 added recently to 'pu' has the\n> same construct.\n\nA quick check shows that subtests 64-68 and 89-93 of t0008 fail for me \non Debian (10 in total) and subtests 64 and 89 fail on NetBSD (2 in \ntotal). Unlike t1402 they don't report \"bug in the test script\".\n\nt0008 only uses ${:+} substitution on variables that don't contain \nspaces. With the test changed to store the description in a variable \nfirst I still get the same 2 failures.\n\nThere must be something else going on here. The different results are \ninteresting, especially the higher number of failures on Debian.\n\nRené\n"},{"id":"206334","messageId":"7vboczcq5a.fsf@alter.siamese.dyndns.org","threadId":"32563","inReplyTo":"50EC8BE7.2010508@lsrfire.ath.cx","subject":"Re: [PATCH] t1402: work around shell quoting issue on NetBSD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-08T21:37:37Z","receivedAt":"2013-01-08T21:37:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> A quick check shows that subtests 64-68 and 89-93 of t0008 fail for me\n> on Debian (10 in total) and subtests 64 and 89 fail on NetBSD (2 in\n> total). Unlike t1402 they don't report \"bug in the test script\".\n>\n> t0008 only uses ${:+} substitution on variables that don't contain\n> spaces. With the test changed to store the description in a variable\n> first I still get the same 2 failures.\n>\n> There must be something else going on here. The different results are\n> interesting, especially the higher number of failures on Debian.\n\nI forgot to mention that some of them seem to be broken under dash\nbut pass under bash.\n"},{"id":"206348","messageId":"rmihamrjgcj.fsf@fnord.ir.bbn.com","threadId":"32563","inReplyTo":"50EC8025.8000707@lsrfire.ath.cx","subject":"Re: [PATCH] t1402: work around shell quoting issue on NetBSD","fromName":"Greg Troxel","fromEmail":"gdt@ir.bbn.com","sentAt":"2013-01-09T01:27:24Z","receivedAt":"2013-01-09T01:27:24Z","isPatch":true,"sender":{"key":"gdt@ir.bbn.com","avatar":null},"body":"\nRené Scharfe writes:\n\n> invalid_ref() constructs a test case description using its last argument,\n> but the shell seems to split it up into two pieces if it contains a\n> space. Minimal test case:\n\nThis is indeed a bug in NetBSD's shell, which I reported after finding\nthis test case problem, and I think someone is working on a fix. But\nbecause git does not intend to be a shell torture test, if it's possible\nto avoid bugs in a reasonable way, I think it's nice to do so.\n"},{"id":"206349","messageId":"rmitxqrhzwl.fsf@fnord.ir.bbn.com","threadId":"32563","inReplyTo":"50EC8025.8000707@lsrfire.ath.cx","subject":"Re: [PATCH] t1402: work around shell quoting issue on NetBSD","fromName":"Greg Troxel","fromEmail":"gdt@ir.bbn.com","sentAt":"2013-01-09T02:07:54Z","receivedAt":"2013-01-09T02:07:54Z","isPatch":true,"sender":{"key":"gdt@ir.bbn.com","avatar":null},"body":"\nRené Scharfe writes:\n\n> The test fails for me on NetBSD 6.0.1 and reports:\n>\n> \tok 1 - ref name '' is invalid\n> \tok 2 - ref name '/' is invalid\n> \tok 3 - ref name '/' is invalid with options --allow-onelevel\n> \tok 4 - ref name '/' is invalid with options --normalize\n> \terror: bug in the test script: not 2 or 3 parameters to test-expect-success\n>\n> The alleged bug is in this line:\n>\n> \tinvalid_ref NOT_MINGW '/' '--allow-onelevel --normalize'\n\nThe bug in NetBSD's sh has been fixed in -current:\n\n http://gnats.netbsd.org/47361\n\nand the change will almost certainly make it to the -6 and -5 release\nbranches.\n\nWith the fixed sh:\n 414c78c (with the workaround): t1402 passes\n 69637e5 (without the workaround): t1402 passes\n\nWith the buggy sh,\n 414c78c (with the workaround): t1402 passes\n 69637e5 (without workaround): t1402 fails\n\nso I can confirm that the workaround is successful on NetBSD 5.\n\nThanks for addressing this, and sorry I didn't mention it on this list.\n\nGreg\n\n\n"},{"id":"206433","messageId":"50EE01F8.1070109@lsrfire.ath.cx","threadId":"32563","inReplyTo":"7vboczcq5a.fsf@alter.siamese.dyndns.org","subject":"[PATCH] t0008: avoid brace expansion","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-01-09T23:49:12Z","receivedAt":"2013-01-09T23:49:12Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Brace expansion is not required by POSIX and not supported by dash nor\nNetBSD's sh. Explicitly list all combinations instead.\n\nSigned-off-by: Rene Scharfe \n---\n t/t0008-ignores.sh | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\nindex 9b0fcd6..0273680 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -129,8 +129,9 @@ test_expect_success 'setup' '\n \t\tone\n \t\tignored-*\n \tEOF\n-\ttouch {,a/}{not-ignored,ignored-{and-untracked,but-in-index}} &&\n-\tgit add -f {,a/}ignored-but-in-index\n+\ttouch not-ignored ignored-and-untracked ignored-but-in-index &&\n+\ttouch a/not-ignored a/ignored-and-untracked a/ignored-but-in-index &&\n+\tgit add -f ignored-but-in-index a/ignored-but-in-index &&\n \tcat <<-\\EOF >a/.gitignore &&\n \t\ttwo*\n \t\t*three\n-- \n1.8.0\n"},{"id":"206434","messageId":"CAOkDyE_EuuV04KxkkLuHMV+VbDWsDMN1q3YShLtKaimaXH40Sg@mail.gmail.com","threadId":"32563","inReplyTo":"50EE01F8.1070109@lsrfire.ath.cx","subject":"Re: [PATCH] t0008: avoid brace expansion","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2013-01-09T23:56:27Z","receivedAt":"2013-01-09T23:56:27Z","isPatch":true,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"On Wed, Jan 9, 2013 at 11:49 PM, René Scharfe\n wrote:\n> Brace expansion is not required by POSIX and not supported by dash nor\n> NetBSD's sh. Explicitly list all combinations instead.\n\nGood catch, thanks!\n"},{"id":"206436","messageId":"7vsj693n6o.fsf@alter.siamese.dyndns.org","threadId":"32563","inReplyTo":"CAOkDyE_EuuV04KxkkLuHMV+VbDWsDMN1q3YShLtKaimaXH40Sg@mail.gmail.com","subject":"Re: [PATCH] t0008: avoid brace expansion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-10T00:18:39Z","receivedAt":"2013-01-10T00:18:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Spiers writes:\n\n> On Wed, Jan 9, 2013 at 11:49 PM, René Scharfe\n> wrote:\n>> Brace expansion is not required by POSIX and not supported by dash nor\n>> NetBSD's sh. Explicitly list all combinations instead.\n>\n> Good catch, thanks!\n\nYeah; thanks.\n\nIt would also be nice to avoid touch while we are at it, by the way.\n"},{"id":"206438","messageId":"CAOkDyE_gSn488N9q1_PD40s+B9sRY32qMXddoC3fNUFD1jtbhQ@mail.gmail.com","threadId":"32563","inReplyTo":"7vsj693n6o.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t0008: avoid brace expansion","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2013-01-10T00:22:58Z","receivedAt":"2013-01-10T00:22:58Z","isPatch":true,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"On Thu, Jan 10, 2013 at 12:18 AM, Junio C Hamano wrote:\n> Adam Spiers writes:\n>\n>> On Wed, Jan 9, 2013 at 11:49 PM, René Scharfe\n>> wrote:\n>>> Brace expansion is not required by POSIX and not supported by dash nor\n>>> NetBSD's sh. Explicitly list all combinations instead.\n>>\n>> Good catch, thanks!\n>\n> Yeah; thanks.\n>\n> It would also be nice to avoid touch while we are at it, by the way.\n\nNoted.\n"},{"id":"206468","messageId":"50EF31A4.3000205@lsrfire.ath.cx","threadId":"32563","inReplyTo":"7vsj693n6o.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t0008: avoid brace expansion","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-01-10T21:24:52Z","receivedAt":"2013-01-10T21:24:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.01.2013 01:18, schrieb Junio C Hamano:\n> Adam Spiers writes:\n> \n>> On Wed, Jan 9, 2013 at 11:49 PM, René Scharfe\n>> wrote:\n>>> Brace expansion is not required by POSIX and not supported by dash nor\n>>> NetBSD's sh. Explicitly list all combinations instead.\n>>\n>> Good catch, thanks!\n> \n> Yeah; thanks.\n> \n> It would also be nice to avoid touch while we are at it, by the way.\n\nGood idea! Replacement patch:\n\n--- >8 ---\nBrace expansion is a shell feature that's not required by POSIX and not\nsupported by dash nor NetBSD's sh. Explicitly list all combinations\ninstead. Also avoid calling touch by creating the test files with a\nredirection instead, as suggested by Junio.\n\nSigned-off-by: Rene Scharfe \n---\n t/t0008-ignores.sh | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\nindex 9b0fcd6..d7df719 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -129,8 +129,13 @@ test_expect_success 'setup' '\n \t\tone\n \t\tignored-*\n \tEOF\n-\ttouch {,a/}{not-ignored,ignored-{and-untracked,but-in-index}} &&\n-\tgit add -f {,a/}ignored-but-in-index\n+\tfor dir in . a\n+\tdo\n+\t\t: >$dir/not-ignored &&\n+\t\t: >$dir/ignored-and-untracked &&\n+\t\t: >$dir/ignored-but-in-index\n+\tdone &&\n+\tgit add -f ignored-but-in-index a/ignored-but-in-index &&\n \tcat <<-\\EOF >a/.gitignore &&\n \t\ttwo*\n \t\t*three\n-- \n1.8.0\n"}]}