{"thread":{"id":"38757","subject":"[PATCH] t5528: do not fail with FreeBSD shell","startedAt":"2015-03-08T15:37:50Z","lastAt":"2015-03-09T06:04:04Z","messageCount":4,"participants":["Kyle J. McKay","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"257328","messageId":"e3bfc53363b14826d828e1adffbbeea@74d39fa044aa309eaea14b9f57fe79c","threadId":"38757","inReplyTo":null,"subject":"[PATCH] t5528: do not fail with FreeBSD shell","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-03-08T15:37:50Z","receivedAt":"2015-03-08T15:37:50Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"The FreeBSD shell converts this expression:\n\n  git ${1:+-c push.default=\"$1\"} push\n\nto this when \"$1\" is not empty:\n\n  git \"-c push.default=$1\" push\n\nwhich causes git to fail.  To avoid this we simply break up the\nexpansion into two parts so that the whitespace which creates\ntwo arguments instead of one is outside the ${...} like so:\n\n  git ${1:+-c} ${1:+push.default=\"$1\"} push\n\nThis has the desired effect on all platforms allowing the test\nto pass on FreeBSD.\n\nSigned-off-by: Kyle J. McKay <mackyle@gmail.com>\n---\n t/t5528-push-default.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh\nindex cc745190..73f4bb63 100755\n--- a/t/t5528-push-default.sh\n+++ b/t/t5528-push-default.sh\n@@ -26,7 +26,7 @@ check_pushed_commit () {\n # $2 = expected target branch for the push\n # $3 = [optional] repo to check for actual output (repo1 by default)\n test_push_success () {\n-\tgit ${1:+-c push.default=\"$1\"} push &&\n+\tgit ${1:+-c} ${1:+push.default=\"$1\"} push &&\n \tcheck_pushed_commit HEAD \"$2\" \"$3\"\n }\n \n@@ -34,7 +34,7 @@ test_push_success () {\n # check that push fails and does not modify any remote branch\n test_push_failure () {\n \tgit --git-dir=repo1 log --no-walk --format='%h %s' --all >expect &&\n-\ttest_must_fail git ${1:+-c push.default=\"$1\"} push &&\n+\ttest_must_fail git ${1:+-c} ${1:+push.default=\"$1\"} push &&\n \tgit --git-dir=repo1 log --no-walk --format='%h %s' --all >actual &&\n \ttest_cmp expect actual\n }\n---\n"},{"id":"257333","messageId":"20150308175624.GA30399@peff.net","threadId":"38757","inReplyTo":"e3bfc53363b14826d828e1adffbbeea@74d39fa044aa309eaea14b9f57fe79c","subject":"Re: [PATCH] t5528: do not fail with FreeBSD shell","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-08T17:56:25Z","receivedAt":"2015-03-08T17:56:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 08, 2015 at 08:37:50AM -0700, Kyle J. McKay wrote:\n\n> The FreeBSD shell converts this expression:\n> \n>   git ${1:+-c push.default=\"$1\"} push\n> \n> to this when \"$1\" is not empty:\n> \n>   git \"-c push.default=$1\" push\n> \n> which causes git to fail.\n\nHmph, just when I thought I knew about all of the weird shell quirks. :)\n\nI am not convinced this isn't a violation of POSIX (which specifies that\nfield splitting is done on the results of parameter expansions outside\nof double-quotes). But whether it is or not, we have to live with it.\n\nFor my own curiosity, what does:\n\n  foo='with space'\n  printf \"%s\\n\" ${foo:+first \"$foo\"}\n\nprint? That is, are the double-quotes even doing anything on such a\nshell? On bash and dash, it prints:\n\n  first\n  with space\n\nwhich is what I would expect. So does \"ash\" (0.5.7, packaged for\nDebian), which is what I _thought_ FreeBSD's shell was based on. But\nclearly there is some divergence.\n\nI guess they are getting eaten by your shell, otherwise we would pass\nthem along to git in the test script, which would complain.\n\n> ---\n>  t/t5528-push-default.sh | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n\nPatch itself looks obviously correct.\n\n-Peff\n"},{"id":"257359","messageId":"211E8F7E-5588-45B1-ACF6-BB7DFB798ABB@gmail.com","threadId":"38757","inReplyTo":"20150308175624.GA30399@peff.net","subject":"Re: [PATCH] t5528: do not fail with FreeBSD shell","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-03-09T05:19:20Z","receivedAt":"2015-03-09T05:19:20Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Mar 8, 2015, at 10:56, Jeff King wrote:\n> On Sun, Mar 08, 2015 at 08:37:50AM -0700, Kyle J. McKay wrote:\n>\n>> The FreeBSD shell converts this expression:\n>>\n>>  git ${1:+-c push.default=\"$1\"} push\n>>\n>> to this when \"$1\" is not empty:\n>>\n>>  git \"-c push.default=$1\" push\n>>\n>> which causes git to fail.\n>\n> Hmph, just when I thought I knew about all of the weird shell  \n> quirks. :)\n>\n> I am not convinced this isn't a violation of POSIX (which specifies  \n> that\n> field splitting is done on the results of parameter expansions outside\n> of double-quotes). But whether it is or not, we have to live with it.\n\nThat's not the only problem the shell has, t5560 had an issue, rebase  \nhad issues.  They've have been worked around.  It probably also  \naffects related BSDs' shells as well (at least older versions that  \ndidn't change the shell).\n\n> For my own curiosity, what does:\n>\n>  foo='with space'\n>  printf \"%s\\n\" ${foo:+first \"$foo\"}\n>\n> print? That is, are the double-quotes even doing anything on such a\n> shell? On bash and dash, it prints:\n>\n>  first\n>  with space\n>\n> which is what I would expect.\n\n\n$ foo='with space'\n$ printf \"%s\\n\" ${foo:+first \"$foo\"}\nfirst with space\n\nI also happen to have a handy-dandy test program called \"showargs\".\n\n$ foo='with space'\n$ showargs ${foo:+first \"$foo\"}\nuid=1001 euid=1001\ngid=1001 egid=1001\numask(octal)=022\nstdin=/dev/pts/12 stdout=/dev/pts/12 stderr=/dev/pts/12\npid=5261\n$0=showargs\n$1=first with space\n\nSo no quotes are being passed on.  Of course bash works just fine.\n\n> So does \"ash\" (0.5.7, packaged for\n> Debian), which is what I _thought_ FreeBSD's shell was based on. But\n> clearly there is some divergence.\n\nI like to test on FreeBSD 8, which is slightly older, once in a while  \nto make sure I catch stuff like this.  :)\n\nRunning \"ident /bin/sh\" shows a bunch of source file names which  \nmatches up pretty well with the dash distribution so I'm pretty sure  \nit's just a much older ancestor of dash/ash.\n\nIf I run dash 0.5.6 (installed via FreeBSD ports), it works properly  \ntoo.\n\n> I guess they are getting eaten by your shell, otherwise we would pass\n> them along to git in the test script, which would complain.\n\nWhen I run t5528 with -v -x -d -i this is where it dies (without the  \nfix):\n\n+ git '-c push.default=upstream' push\nUnknown option: -c push.default=upstream\n\nSo yeah, the quotes are gone, but no word-splitting occurred.\n"},{"id":"257361","messageId":"20150309060403.GA27128@peff.net","threadId":"38757","inReplyTo":"211E8F7E-5588-45B1-ACF6-BB7DFB798ABB@gmail.com","subject":"Re: [PATCH] t5528: do not fail with FreeBSD shell","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-09T06:04:04Z","receivedAt":"2015-03-09T06:04:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 08, 2015 at 10:19:20PM -0700, Kyle J. McKay wrote:\n\n> >I am not convinced this isn't a violation of POSIX (which specifies that\n> >field splitting is done on the results of parameter expansions outside\n> >of double-quotes). But whether it is or not, we have to live with it.\n> \n> That's not the only problem the shell has, t5560 had an issue, rebase had\n> issues.  They've have been worked around.  It probably also affects related\n> BSDs' shells as well (at least older versions that didn't change the shell).\n\nYeah, I hope that didn't come across as \"bleh, this shell is not worth\nsupporting\". It was \"whether I think it is a bug or not, it is a real\nproblem and we must work around it\".\n\n> >For my own curiosity, what does:\n> [...]\n\nThanks. Weird behavior, certainly, but I think the solution in your\npatch is the right thing to do.\n\n-Peff\n"}]}