{"thread":{"id":"41977","subject":"Hardcoded #!/bin/sh in t5532 causes problems on Solaris","startedAt":"2016-04-09T20:27:37Z","lastAt":"2016-04-12T17:23:33Z","messageCount":14,"participants":["Tom G. Christensen","Jeff King","Junio C Hamano","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"283048","messageId":"570965B9.9040207@jupiterrise.com","threadId":"41977","inReplyTo":null,"subject":"Hardcoded #!/bin/sh in t5532 causes problems on Solaris","fromName":"Tom G. Christensen","fromEmail":"tgc@jupiterrise.com","sentAt":"2016-04-09T20:27:37Z","receivedAt":"2016-04-09T20:27:37Z","isPatch":false,"sender":{"key":"tgc@jupiterrise.com","avatar":"https://avatars.githubusercontent.com/u/912180?v=4"},"body":"Hello,\n\nLooking at the testsuite results on Solaris I see a failure in t5532.3.\n\nRunning the testsuite with -v -i revealed a shell syntax error:\n\nproxying for example.com 9418\n./proxy: syntax error at line 3: `cmd=$' unexpected\nnot ok 3 - fetch through proxy works\n#\n#               git fetch fake &&\n#               echo one >expect &&\n#               git log -1 --format=%s FETCH_HEAD >actual &&\n#               test_cmp expect actual\n#\n\n\nLooking a t5532-fetch-proxy.sh the problem is obvious, it writes out a \nhelper script which explicitly uses #!/bin/sh but fails to take into \naccount that systems like Solaris has an ancient /bin/sh that knows \nnothing about POSIX things like $().\nReplacing $() with `` was enough to make the test pass.\n\n-tgc\n"},{"id":"283057","messageId":"20160409210429.GB18989@sigill.intra.peff.net","threadId":"41977","inReplyTo":"570965B9.9040207@jupiterrise.com","subject":"Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-09T21:04:30Z","receivedAt":"2016-04-09T21:04:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 09, 2016 at 10:27:37PM +0200, Tom G. Christensen wrote:\n\n> Looking at the testsuite results on Solaris I see a failure in t5532.3.\n> \n> Running the testsuite with -v -i revealed a shell syntax error:\n> \n> proxying for example.com 9418\n> ./proxy: syntax error at line 3: `cmd=$' unexpected\n> not ok 3 - fetch through proxy works\n> #\n> #               git fetch fake &&\n> #               echo one >expect &&\n> #               git log -1 --format=%s FETCH_HEAD >actual &&\n> #               test_cmp expect actual\n> #\n> \n> \n> Looking a t5532-fetch-proxy.sh the problem is obvious, it writes out a\n> helper script which explicitly uses #!/bin/sh but fails to take into account\n> that systems like Solaris has an ancient /bin/sh that knows nothing about\n> POSIX things like $().\n> Replacing $() with `` was enough to make the test pass.\n\nRight, this is a recent regression from Elia's $() series. Rather than\njust revert, I think the cleanup below is the best fix.\n\nI did some quick grepping around, and I suspect you may run\ninto the same thing in other places (e.g., t3404.40 looks\nlike a similar case). We left a lot of \"#!/bin/sh\" cases\nunconverted to write_script because we knew what they were\ndoing was trivial enough not to matter. But the $()\nconversion made them non-trivial.\n\n-- >8 --\nSubject: [PATCH] t5532: use write_script\n\nThe recent cleanup in b7cbbff switched t5532's use of\nbackticks to $(). This matches our normal shell style, which\nis good. But it also breaks the test on Solaris, where\n/bin/sh does not understand $().\n\nOur normal shell style assumes a modern-ish shell which\nknows about $(). However, some tests create small helper\nscripts and just write \"#!/bin/sh\" into them. These scripts\neither need to go back to using backticks, or they need to\nrespect $SHELL_PATH. The easiest way to do the latter is to\nuse write_script.\n\nWhile we're at it, let's also stick the script creation\ninside a test_expect block (our usual style), and split the\nperl snippet into its own script (to prevent quoting\nmadness).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5532-fetch-proxy.sh | 21 ++++++++++++---------\n 1 file changed, 12 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t5532-fetch-proxy.sh b/t/t5532-fetch-proxy.sh\nindex d75ef0e..51c9669 100755\n--- a/t/t5532-fetch-proxy.sh\n+++ b/t/t5532-fetch-proxy.sh\n@@ -12,10 +12,8 @@ test_expect_success 'setup remote repo' '\n \t)\n '\n \n-cat >proxy <<'EOF'\n-#!/bin/sh\n-echo >&2 \"proxying for $*\"\n-cmd=$(\"$PERL_PATH\" -e '\n+test_expect_success 'setup proxy script' '\n+\twrite_script proxy-get-cmd \"$PERL_PATH\" <<-\\EOF &&\n \tread(STDIN, $buf, 4);\n \tmy $n = hex($buf) - 4;\n \tread(STDIN, $buf, $n);\n@@ -23,11 +21,16 @@ cmd=$(\"$PERL_PATH\" -e '\n \t# drop absolute-path on repo name\n \t$cmd =~ s{ /}{ };\n \tprint $cmd;\n-')\n-echo >&2 \"Running '$cmd'\"\n-exec $cmd\n-EOF\n-chmod +x proxy\n+\tEOF\n+\n+\twrite_script proxy <<-\\EOF\n+\techo >&2 \"proxying for $*\"\n+\tcmd=$(./proxy-get-cmd)\n+\techo >&2 \"Running $cmd\"\n+\texec $cmd\n+\tEOF\n+'\n+\n test_expect_success 'setup local repo' '\n \tgit remote add fake git://example.com/remote &&\n \tgit config core.gitproxy ./proxy\n-- \n2.8.1.245.g18e0f5c\n"},{"id":"283058","messageId":"57098259.1060608@jupiterrise.com","threadId":"41977","inReplyTo":"20160409210429.GB18989@sigill.intra.peff.net","subject":"Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris","fromName":"Tom G. Christensen","fromEmail":"tgc@jupiterrise.com","sentAt":"2016-04-09T22:29:45Z","receivedAt":"2016-04-09T22:29:45Z","isPatch":false,"sender":{"key":"tgc@jupiterrise.com","avatar":"https://avatars.githubusercontent.com/u/912180?v=4"},"body":"On 09/04/16 23:04, Jeff King wrote:\n> I did some quick grepping around, and I suspect you may run\n> into the same thing in other places (e.g., t3404.40 looks\n> like a similar case).\n\nThere are only a few tests that fail and just t5532.3 seems affected by \nthis issue.\n\n> Subject: [PATCH] t5532: use write_script\n>\n> The recent cleanup in b7cbbff switched t5532's use of\n> backticks to $(). This matches our normal shell style, which\n> is good. But it also breaks the test on Solaris, where\n> /bin/sh does not understand $().\n>\n> Our normal shell style assumes a modern-ish shell which\n> knows about $(). However, some tests create small helper\n> scripts and just write \"#!/bin/sh\" into them. These scripts\n> either need to go back to using backticks, or they need to\n> respect $SHELL_PATH. The easiest way to do the latter is to\n> use write_script.\n>\n> While we're at it, let's also stick the script creation\n> inside a test_expect block (our usual style), and split the\n> perl snippet into its own script (to prevent quoting\n> madness).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>   t/t5532-fetch-proxy.sh | 21 ++++++++++++---------\n>   1 file changed, 12 insertions(+), 9 deletions(-)\n>\n\nI applied this to 2.8.1 and as expected the test now passes on Solaris.\n\n-tgc\n"},{"id":"283059","messageId":"20160409223738.GA1738@sigill.intra.peff.net","threadId":"41977","inReplyTo":"57098259.1060608@jupiterrise.com","subject":"Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-09T22:37:39Z","receivedAt":"2016-04-09T22:37:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 10, 2016 at 12:29:45AM +0200, Tom G. Christensen wrote:\n\n> On 09/04/16 23:04, Jeff King wrote:\n> >I did some quick grepping around, and I suspect you may run\n> >into the same thing in other places (e.g., t3404.40 looks\n> >like a similar case).\n> \n> There are only a few tests that fail and just t5532.3 seems affected by this\n> issue.\n\nHmm. t3404.40 does this:\n\n        echo \"#!/bin/sh\" > $PRE_COMMIT &&\n\techo \"test -z \\\"\\$(git diff --cached --check)\\\"\" >>$PRE_COMMIT &&\n\tchmod a+x $PRE_COMMIT &&\n\nSo I'm pretty sure that $PRE_COMMIT script should be barfing each time\nit is called on Solaris. I think the test itself doesn't notice because\n\"/bin/sh barfed\" and \"the pre-commit check said no\" look the same from\ngit's perspective (both non-zero exits), and we test only cases where we\nexpect the hook to fail.\n\nI think that particular test could simplify its pre-commit hook to just\n\"exit 1\".\n\nI didn't dig into any other cases, so that might be the only one. If\nyou're not seeing problems, I'm not inclined to explore each one\nmanually.\n\n> I applied this to 2.8.1 and as expected the test now passes on Solaris.\n\nThanks.\n\n-Peff\n"},{"id":"283060","messageId":"xmqqmvp2ti20.fsf@gitster.mtv.corp.google.com","threadId":"41977","inReplyTo":"20160409223738.GA1738@sigill.intra.peff.net","subject":"Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-10T00:37:43Z","receivedAt":"2016-04-10T00:37:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Hmm. t3404.40 does this:\n>\n>         echo \"#!/bin/sh\" > $PRE_COMMIT &&\n> \techo \"test -z \\\"\\$(git diff --cached --check)\\\"\" >>$PRE_COMMIT &&\n> \tchmod a+x $PRE_COMMIT &&\n>\n> So I'm pretty sure that $PRE_COMMIT script should be barfing each time\n> it is called on Solaris. I think the test itself doesn't notice because\n> \"/bin/sh barfed\" and \"the pre-commit check said no\" look the same from\n> git's perspective (both non-zero exits), and we test only cases where we\n> expect the hook to fail.\n\nI looked at\n\n    $ git grep -c '#! */bin/sh' t | grep -v ':1$'\n\nand did a few just for fun.  Doing it fully may be a good\nmicroproject for next year ;-)\n\n t/t1020-subdirectory.sh       |  6 +++---\n t/t2050-git-dir-relative.sh   | 11 ++++++-----\n t/t3404-rebase-interactive.sh |  7 +++----\n 3 files changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh\nindex 8e22b03..6dedb1c 100755\n--- a/t/t1020-subdirectory.sh\n+++ b/t/t1020-subdirectory.sh\n@@ -142,9 +142,9 @@ test_expect_success 'GIT_PREFIX for built-ins' '\n \t# Use GIT_EXTERNAL_DIFF to test that the \"diff\" built-in\n \t# receives the GIT_PREFIX variable.\n \tprintf \"dir/\" >expect &&\n-\tprintf \"#!/bin/sh\\n\" >diff &&\n-\tprintf \"printf \\\"\\$GIT_PREFIX\\\"\" >>diff &&\n-\tchmod +x diff &&\n+\twrite_script diff <<-\\EOF &&\n+\tprintf \"%s\" \"$GIT_PREFIX\"\n+\tEOF\n \t(\n \t\tcd dir &&\n \t\tprintf \"change\" >two &&\ndiff --git a/t/t2050-git-dir-relative.sh b/t/t2050-git-dir-relative.sh\nindex 21f4659..7a05b20 100755\n--- a/t/t2050-git-dir-relative.sh\n+++ b/t/t2050-git-dir-relative.sh\n@@ -18,11 +18,12 @@ COMMIT_FILE=\"$(pwd)/output\"\n export COMMIT_FILE\n \n test_expect_success 'Setting up post-commit hook' '\n-mkdir -p .git/hooks &&\n-echo >.git/hooks/post-commit \"#!/bin/sh\n-touch \\\"\\${COMMIT_FILE}\\\"\n-echo Post commit hook was called.\" &&\n-chmod +x .git/hooks/post-commit'\n+\tmkdir -p .git/hooks &&\n+\twrite_script .git/hooks/post-commit <<-\\EOF\n+\t>\"${COMMIT_FILE}\"\n+\techo Post commit hook was called.\n+\tEOF\n+'\n \n test_expect_success 'post-commit hook used ordinarily' '\n echo initial >top &&\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex b79f442..d96d0e4 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -555,10 +555,9 @@ test_expect_success 'rebase a detached HEAD' '\n test_expect_success 'rebase a commit violating pre-commit' '\n \n \tmkdir -p .git/hooks &&\n-\tPRE_COMMIT=.git/hooks/pre-commit &&\n-\techo \"#!/bin/sh\" > $PRE_COMMIT &&\n-\techo \"test -z \\\"\\$(git diff --cached --check)\\\"\" >> $PRE_COMMIT &&\n-\tchmod a+x $PRE_COMMIT &&\n+\twrite_script .git/hooks/pre-commit <<-\\EOF &&\n+\ttest -z \"$(git diff --cached --check)\"\n+\tEOF\n \techo \"monde! \" >> file1 &&\n \ttest_tick &&\n \ttest_must_fail git commit -m doesnt-verify file1 &&\n"},{"id":"283109","messageId":"xmqq37qtthit.fsf@gitster.mtv.corp.google.com","threadId":"41977","inReplyTo":"xmqqmvp2ti20.fsf@gitster.mtv.corp.google.com","subject":"Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-10T19:01:30Z","receivedAt":"2016-04-10T19:01:30Z","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> I looked at\n>\n>     $ git grep -c '#! */bin/sh' t | grep -v ':1$'\n>\n> and did a few just for fun.  Doing it fully may be a good\n> microproject for next year ;-)\n>\n>  t/t1020-subdirectory.sh       |  6 +++---\n>  t/t2050-git-dir-relative.sh   | 11 ++++++-----\n>  t/t3404-rebase-interactive.sh |  7 +++----\n>  3 files changed, 12 insertions(+), 12 deletions(-)\n>\n> diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh\n> index 8e22b03..6dedb1c 100755\n> --- a/t/t1020-subdirectory.sh\n> +++ b/t/t1020-subdirectory.sh\n> @@ -142,9 +142,9 @@ test_expect_success 'GIT_PREFIX for built-ins' '\n>  \t# Use GIT_EXTERNAL_DIFF to test that the \"diff\" built-in\n>  \t# receives the GIT_PREFIX variable.\n>  \tprintf \"dir/\" >expect &&\n> -\tprintf \"#!/bin/sh\\n\" >diff &&\n> -\tprintf \"printf \\\"\\$GIT_PREFIX\\\"\" >>diff &&\n> -\tchmod +x diff &&\n> +\twrite_script diff <<-\\EOF &&\n> +\tprintf \"%s\" \"$GIT_PREFIX\"\n> +\tEOF\n>  \t(\n>  \t\tcd dir &&\n>  \t\tprintf \"change\" >two &&\n\nRegarding this one, I notice that \"expect\" and \"actual\" (produced\nlater in this script by executing \"diff\" script) are eventually\ncompared by test_cmp, which runs \"diff\" to show the actual\ndifferences.  If we are doing this modernization to use write_script\nmore, we probably should make \"expect\" and \"actual\" text files that\nend with a complete line.\n\nI.e.\n\n-- >8 --\nSubject: t1020: do not overuse printf and use write_script\n\nThe test prepares a sample file \"dir/two\" with a single incomplete\nline in it with \"printf\", and also prepares a small helper script\n\"diff\" to create a file with a single incomplete line in it, again\nwith \"printf\".  The output from the latter is compared with an\nexpected output, again prepared with \"printf\" hance lacking the\nfinal LF.  There is no reason for this test to be using files with\nan incomplete line at the end, and these look more like a mistake\nof not using\n\n\tprintf \"%s\\n\" \"string to be written\"\n\nand using\n\n\tprintf \"string to be written\"\n\nDepending on what would be in $GIT_PREFIX, using the latter form\ncould be a bug waiting to happen.  Correct them.\n\nAlso, the test uses hardcoded #!/bin/sh to create a small helper\nscript.  For a small task like what the generated script does, it\ndoes not matter too much in that what appears as /bin/sh would not\nbe _so_ broken, but while we are at it, use write_script instead,\nwhich happens to make the result easier to read by reducing need\nof one level of quoting.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t1020-subdirectory.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh\nindex 8e22b03..df3183e 100755\n--- a/t/t1020-subdirectory.sh\n+++ b/t/t1020-subdirectory.sh\n@@ -141,13 +141,13 @@ test_expect_success 'GIT_PREFIX for !alias' '\n test_expect_success 'GIT_PREFIX for built-ins' '\n \t# Use GIT_EXTERNAL_DIFF to test that the \"diff\" built-in\n \t# receives the GIT_PREFIX variable.\n-\tprintf \"dir/\" >expect &&\n-\tprintf \"#!/bin/sh\\n\" >diff &&\n-\tprintf \"printf \\\"\\$GIT_PREFIX\\\"\" >>diff &&\n-\tchmod +x diff &&\n+\techo \"dir/\" >expect &&\n+\twrite_script diff <<-\\EOF &&\n+\tprintf \"%s\\n\" \"$GIT_PREFIX\"\n+\tEOF\n \t(\n \t\tcd dir &&\n-\t\tprintf \"change\" >two &&\n+\t\techo \"change\" >two &&\n \t\tGIT_EXTERNAL_DIFF=./diff git diff >../actual\n \t\tgit checkout -- two\n \t) &&\n"},{"id":"283112","messageId":"CAPig+cQzGmohkyshwi+yhQHPT_VVb2fr52OK_1Axw4Q2vLxRHw@mail.gmail.com","threadId":"41977","inReplyTo":"xmqq37qtthit.fsf@gitster.mtv.corp.google.com","subject":"Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-10T21:51:06Z","receivedAt":"2016-04-10T21:51:06Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Apr 10, 2016 at 3:01 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Subject: t1020: do not overuse printf and use write_script\n>\n> The test prepares a sample file \"dir/two\" with a single incomplete\n> line in it with \"printf\", and also prepares a small helper script\n> \"diff\" to create a file with a single incomplete line in it, again\n> with \"printf\".  The output from the latter is compared with an\n> expected output, again prepared with \"printf\" hance lacking the\n\ns/hance/hence/\n\n> final LF.  There is no reason for this test to be using files with\n> an incomplete line at the end, and these look more like a mistake\n> of not using\n>\n>         printf \"%s\\n\" \"string to be written\"\n>\n> and using\n>\n>         printf \"string to be written\"\n>\n> Depending on what would be in $GIT_PREFIX, using the latter form\n> could be a bug waiting to happen.  Correct them.\n>\n> Also, the test uses hardcoded #!/bin/sh to create a small helper\n> script.  For a small task like what the generated script does, it\n> does not matter too much in that what appears as /bin/sh would not\n> be _so_ broken, but while we are at it, use write_script instead,\n> which happens to make the result easier to read by reducing need\n> of one level of quoting.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n"},{"id":"283144","messageId":"xmqqpotwrtb9.fsf@gitster.mtv.corp.google.com","threadId":"41977","inReplyTo":"CAPig+cQzGmohkyshwi+yhQHPT_VVb2fr52OK_1Axw4Q2vLxRHw@mail.gmail.com","subject":"Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-11T16:42:02Z","receivedAt":"2016-04-11T16:42:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n>> with \"printf\".  The output from the latter is compared with an\n>> expected output, again prepared with \"printf\" hance lacking the\n>\n> s/hance/hence/\n\nThanks\n"},{"id":"283147","messageId":"20160411172741.GD4011@sigill.intra.peff.net","threadId":"41977","inReplyTo":"xmqq37qtthit.fsf@gitster.mtv.corp.google.com","subject":"Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-11T17:27:41Z","receivedAt":"2016-04-11T17:27:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 10, 2016 at 12:01:30PM -0700, Junio C Hamano wrote:\n\n> > diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh\n> > index 8e22b03..6dedb1c 100755\n> > --- a/t/t1020-subdirectory.sh\n> > +++ b/t/t1020-subdirectory.sh\n> > @@ -142,9 +142,9 @@ test_expect_success 'GIT_PREFIX for built-ins' '\n> >  \t# Use GIT_EXTERNAL_DIFF to test that the \"diff\" built-in\n> >  \t# receives the GIT_PREFIX variable.\n> >  \tprintf \"dir/\" >expect &&\n> > -\tprintf \"#!/bin/sh\\n\" >diff &&\n> > -\tprintf \"printf \\\"\\$GIT_PREFIX\\\"\" >>diff &&\n> > -\tchmod +x diff &&\n> > +\twrite_script diff <<-\\EOF &&\n> > +\tprintf \"%s\" \"$GIT_PREFIX\"\n> > +\tEOF\n> >  \t(\n> >  \t\tcd dir &&\n> >  \t\tprintf \"change\" >two &&\n> \n> Regarding this one, I notice that \"expect\" and \"actual\" (produced\n> later in this script by executing \"diff\" script) are eventually\n> compared by test_cmp, which runs \"diff\" to show the actual\n> differences.  If we are doing this modernization to use write_script\n> more, we probably should make \"expect\" and \"actual\" text files that\n> end with a complete line.\n\nYeah I wondered about that. And also the fact that the shell script\nitself doesn't end in newline. But I think that is just an accident, and\nno shell happened to complain (not that I would expect them to, but we\ncome across enough weirdness around final newlines with tools like sed\nand tr, I wouldn't have been surprised).\n\n> -- >8 --\n> Subject: t1020: do not overuse printf and use write_script\n> \n> The test prepares a sample file \"dir/two\" with a single incomplete\n> line in it with \"printf\", and also prepares a small helper script\n> \"diff\" to create a file with a single incomplete line in it, again\n> with \"printf\".  The output from the latter is compared with an\n> expected output, again prepared with \"printf\" hance lacking the\n> final LF.  There is no reason for this test to be using files with\n> an incomplete line at the end, and these look more like a mistake\n> of not using\n> \n> \tprintf \"%s\\n\" \"string to be written\"\n> \n> and using\n> \n> \tprintf \"string to be written\"\n> \n> Depending on what would be in $GIT_PREFIX, using the latter form\n> could be a bug waiting to happen.  Correct them.\n> \n> Also, the test uses hardcoded #!/bin/sh to create a small helper\n> script.  For a small task like what the generated script does, it\n> does not matter too much in that what appears as /bin/sh would not\n> be _so_ broken, but while we are at it, use write_script instead,\n> which happens to make the result easier to read by reducing need\n> of one level of quoting.\n\nLooks good to me. I suspect you could actually just use:\n\n  echo \"$GIT_PREFIX\"\n\nin the helper script. That is also not completely safe against arbitrary\nbytes in $GIT_PREFIX (due to unportable backslash escapes), though I\nsuspect it would be fine for the purposes of the test script. Using a\nproper printf isn't that many more bytes, though.\n\n-Peff\n"},{"id":"283148","messageId":"20160411173224.GE4011@sigill.intra.peff.net","threadId":"41977","inReplyTo":"xmqqmvp2ti20.fsf@gitster.mtv.corp.google.com","subject":"Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-11T17:32:25Z","receivedAt":"2016-04-11T17:32:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 09, 2016 at 05:37:43PM -0700, Junio C Hamano wrote:\n\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index b79f442..d96d0e4 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -555,10 +555,9 @@ test_expect_success 'rebase a detached HEAD' '\n>  test_expect_success 'rebase a commit violating pre-commit' '\n>  \n>  \tmkdir -p .git/hooks &&\n> -\tPRE_COMMIT=.git/hooks/pre-commit &&\n> -\techo \"#!/bin/sh\" > $PRE_COMMIT &&\n> -\techo \"test -z \\\"\\$(git diff --cached --check)\\\"\" >> $PRE_COMMIT &&\n> -\tchmod a+x $PRE_COMMIT &&\n> +\twrite_script .git/hooks/pre-commit <<-\\EOF &&\n> +\ttest -z \"$(git diff --cached --check)\"\n> +\tEOF\n\nLooks good and is the minimal change. I kind of wonder if the example\nwould be more clear, though, as just:\n\n  write_script .git/hooks/pre-commit <<-\\EOF &&\n  exit 1\n  EOF\n  echo whatever >file1 &&\n  ...\n\nI don't think we ever actually need the pre-commit check to pass, as we\nsimply override it with --no-verify. But I dunno. Maybe people find it\neasier to read with a pseudo-realistic example (it took me a minute to\nrealize the trailing whitespace in the content was important).\n\nIt could also stand to clean up its hook with test_when_finished. The\nnext test resorts to \"rm -rf\" on the hooks directory at the beginning.\nYuck.\n\n-Peff\n"},{"id":"283203","messageId":"xmqqvb3mrcgj.fsf@gitster.mtv.corp.google.com","threadId":"41977","inReplyTo":"20160411173224.GE4011@sigill.intra.peff.net","subject":"Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-12T16:58:20Z","receivedAt":"2016-04-12T16:58:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sat, Apr 09, 2016 at 05:37:43PM -0700, Junio C Hamano wrote:\n>\n>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n>> index b79f442..d96d0e4 100755\n>> --- a/t/t3404-rebase-interactive.sh\n>> +++ b/t/t3404-rebase-interactive.sh\n>> @@ -555,10 +555,9 @@ test_expect_success 'rebase a detached HEAD' '\n>>  test_expect_success 'rebase a commit violating pre-commit' '\n>>  \n>>  \tmkdir -p .git/hooks &&\n>> -\tPRE_COMMIT=.git/hooks/pre-commit &&\n>> -\techo \"#!/bin/sh\" > $PRE_COMMIT &&\n>> -\techo \"test -z \\\"\\$(git diff --cached --check)\\\"\" >> $PRE_COMMIT &&\n>> -\tchmod a+x $PRE_COMMIT &&\n>> +\twrite_script .git/hooks/pre-commit <<-\\EOF &&\n>> +\ttest -z \"$(git diff --cached --check)\"\n>> +\tEOF\n>\n> Looks good and is the minimal change. I kind of wonder if the example\n> would be more clear, though, as just:\n>\n>   write_script .git/hooks/pre-commit <<-\\EOF &&\n>   exit 1\n>   EOF\n>   echo whatever >file1 &&\n>   ...\n>\n> I don't think we ever actually need the pre-commit check to pass, as we\n> simply override it with --no-verify. But I dunno. Maybe people find it\n> easier to read with a pseudo-realistic example (it took me a minute to\n> realize the trailing whitespace in the content was important).\n\nI was mostly worried about closing the door for future enhancement\nwhere there are multiple commits to be replayed, some of which fail\nand others pass the test.  Unconditional \"exit 1\" would have to be\nreverted when it happens.\n\n> It could also stand to clean up its hook with test_when_finished. The\n> next test resorts to \"rm -rf\" on the hooks directory at the beginning.\n> Yuck.\n\nYeah, that may be an accident waiting to happen.\n"},{"id":"283204","messageId":"xmqqoa9erbso.fsf@gitster.mtv.corp.google.com","threadId":"41977","inReplyTo":"xmqqvb3mrcgj.fsf@gitster.mtv.corp.google.com","subject":"Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-12T17:12:39Z","receivedAt":"2016-04-12T17:12:39Z","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> Jeff King <peff@peff.net> writes:\n> ...\n>> Looks good and is the minimal change. I kind of wonder if the example\n>> would be more clear, though, as just:\n>>\n>>   write_script .git/hooks/pre-commit <<-\\EOF &&\n>>   exit 1\n>>   EOF\n>>   echo whatever >file1 &&\n>>   ...\n>>\n>> I don't think we ever actually need the pre-commit check to pass, as we\n>> simply override it with --no-verify. But I dunno. Maybe people find it\n>> easier to read with a pseudo-realistic example (it took me a minute to\n>> realize the trailing whitespace in the content was important).\n>\n> I was mostly worried about closing the door for future enhancement\n> where there are multiple commits to be replayed, some of which fail\n> and others pass the test.  Unconditional \"exit 1\" would have to be\n> reverted when it happens.\n>\n>> It could also stand to clean up its hook with test_when_finished. The\n>> next test resorts to \"rm -rf\" on the hooks directory at the beginning.\n>> Yuck.\n>\n> Yeah, that may be an accident waiting to happen.\n\nIn any case, lest we forget...\n\n-- >8 --\nSubject: [PATCH] t3404: use write_script\n\nThe test uses hardcoded #!/bin/sh to create a pre-commit hook\nscript.  Because the generated script uses $(command substitution),\nwhich is not supported by /bin/sh on some platforms (e.g. Solaris),\nthe resulting pre-commit always fails.\n\nWhich is not noticeable as the test that uses the hook is about\nchecking the behaviour of the command when the hook fails ;-), but\nnevertheless it is not testing what we wanted to test.\n\nUse write_script so that the resulting script is run under the same\nshell our scripted Porcelain commands are run, which must support\nthe necessary $(construct).\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t3404-rebase-interactive.sh | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 544f9ad..d6d65a3 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -555,10 +555,9 @@ test_expect_success 'rebase a detached HEAD' '\n test_expect_success 'rebase a commit violating pre-commit' '\n \n \tmkdir -p .git/hooks &&\n-\tPRE_COMMIT=.git/hooks/pre-commit &&\n-\techo \"#!/bin/sh\" > $PRE_COMMIT &&\n-\techo \"test -z \\\"\\$(git diff --cached --check)\\\"\" >> $PRE_COMMIT &&\n-\tchmod a+x $PRE_COMMIT &&\n+\twrite_script .git/hooks/pre-commit <<-\\EOF &&\n+\ttest -z \"$(git diff --cached --check)\"\n+\tEOF\n \techo \"monde! \" >> file1 &&\n \ttest_tick &&\n \ttest_must_fail git commit -m doesnt-verify file1 &&\n-- \n2.8.1-339-gc925d85\n"},{"id":"283205","messageId":"20160412172247.GA2856@sigill.intra.peff.net","threadId":"41977","inReplyTo":"xmqqvb3mrcgj.fsf@gitster.mtv.corp.google.com","subject":"Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-12T17:22:47Z","receivedAt":"2016-04-12T17:22:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 12, 2016 at 09:58:20AM -0700, Junio C Hamano wrote:\n\n> > Looks good and is the minimal change. I kind of wonder if the example\n> > would be more clear, though, as just:\n> >\n> >   write_script .git/hooks/pre-commit <<-\\EOF &&\n> >   exit 1\n> >   EOF\n> >   echo whatever >file1 &&\n> >   ...\n> >\n> > I don't think we ever actually need the pre-commit check to pass, as we\n> > simply override it with --no-verify. But I dunno. Maybe people find it\n> > easier to read with a pseudo-realistic example (it took me a minute to\n> > realize the trailing whitespace in the content was important).\n> \n> I was mostly worried about closing the door for future enhancement\n> where there are multiple commits to be replayed, some of which fail\n> and others pass the test.  Unconditional \"exit 1\" would have to be\n> reverted when it happens.\n\nYeah, that's fair. It is at least trying to re-create a real-world\nsituation.\n\n-Peff\n"},{"id":"283206","messageId":"20160412172333.GB2856@sigill.intra.peff.net","threadId":"41977","inReplyTo":"xmqqoa9erbso.fsf@gitster.mtv.corp.google.com","subject":"Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-12T17:23:33Z","receivedAt":"2016-04-12T17:23:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 12, 2016 at 10:12:39AM -0700, Junio C Hamano wrote:\n\n> In any case, lest we forget...\n> \n> -- >8 --\n> Subject: [PATCH] t3404: use write_script\n\nYep, looks good. Thanks.\n\n-Peff\n"}]}