# [PATCH] t5528: do not fail with FreeBSD shell 4 messages from 2015-03-08 to 2015-03-09. Participants: Kyle J. McKay, Jeff King. Thread: https://gitlist.dev/t/38757 ## Kyle J. McKay, 2015-03-08 15:37 Subject: [PATCH] t5528: do not fail with FreeBSD shell Message-ID: URL: https://gitlist.dev/e/e3bfc53363b14826d828e1adffbbeea%4074d39fa044aa309eaea14b9f57fe79c ``` The FreeBSD shell converts this expression: git ${1:+-c push.default="$1"} push to this when "$1" is not empty: git "-c push.default=$1" push which causes git to fail. To avoid this we simply break up the expansion into two parts so that the whitespace which creates two arguments instead of one is outside the ${...} like so: git ${1:+-c} ${1:+push.default="$1"} push This has the desired effect on all platforms allowing the test to pass on FreeBSD. Signed-off-by: Kyle J. McKay --- t/t5528-push-default.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh index cc745190..73f4bb63 100755 --- a/t/t5528-push-default.sh +++ b/t/t5528-push-default.sh @@ -26,7 +26,7 @@ check_pushed_commit () { # $2 = expected target branch for the push # $3 = [optional] repo to check for actual output (repo1 by default) test_push_success () { - git ${1:+-c push.default="$1"} push && + git ${1:+-c} ${1:+push.default="$1"} push && check_pushed_commit HEAD "$2" "$3" } @@ -34,7 +34,7 @@ test_push_success () { # check that push fails and does not modify any remote branch test_push_failure () { git --git-dir=repo1 log --no-walk --format='%h %s' --all >expect && - test_must_fail git ${1:+-c push.default="$1"} push && + test_must_fail git ${1:+-c} ${1:+push.default="$1"} push && git --git-dir=repo1 log --no-walk --format='%h %s' --all >actual && test_cmp expect actual } --- ``` ## Jeff King, 2015-03-08 17:56 Subject: Re: [PATCH] t5528: do not fail with FreeBSD shell Message-ID: <20150308175624.GA30399@peff.net> URL: https://gitlist.dev/e/20150308175624.GA30399%40peff.net In-Reply-To: ``` On Sun, Mar 08, 2015 at 08:37:50AM -0700, Kyle J. McKay wrote: > The FreeBSD shell converts this expression: > > git ${1:+-c push.default="$1"} push > > to this when "$1" is not empty: > > git "-c push.default=$1" push > > which causes git to fail. Hmph, just when I thought I knew about all of the weird shell quirks. :) I am not convinced this isn't a violation of POSIX (which specifies that field splitting is done on the results of parameter expansions outside of double-quotes). But whether it is or not, we have to live with it. For my own curiosity, what does: foo='with space' printf "%s\n" ${foo:+first "$foo"} print? That is, are the double-quotes even doing anything on such a shell? On bash and dash, it prints: first with space which is what I would expect. So does "ash" (0.5.7, packaged for Debian), which is what I _thought_ FreeBSD's shell was based on. But clearly there is some divergence. I guess they are getting eaten by your shell, otherwise we would pass them along to git in the test script, which would complain. > --- > t/t5528-push-default.sh | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Patch itself looks obviously correct. -Peff ``` ## Kyle J. McKay, 2015-03-09 05:19 Subject: Re: [PATCH] t5528: do not fail with FreeBSD shell Message-ID: <211E8F7E-5588-45B1-ACF6-BB7DFB798ABB@gmail.com> URL: https://gitlist.dev/e/211E8F7E-5588-45B1-ACF6-BB7DFB798ABB%40gmail.com In-Reply-To: <20150308175624.GA30399@peff.net> ``` On Mar 8, 2015, at 10:56, Jeff King wrote: > On Sun, Mar 08, 2015 at 08:37:50AM -0700, Kyle J. McKay wrote: > >> The FreeBSD shell converts this expression: >> >> git ${1:+-c push.default="$1"} push >> >> to this when "$1" is not empty: >> >> git "-c push.default=$1" push >> >> which causes git to fail. > > Hmph, just when I thought I knew about all of the weird shell > quirks. :) > > I am not convinced this isn't a violation of POSIX (which specifies > that > field splitting is done on the results of parameter expansions outside > of double-quotes). But whether it is or not, we have to live with it. That's not the only problem the shell has, t5560 had an issue, rebase had issues. They've have been worked around. It probably also affects related BSDs' shells as well (at least older versions that didn't change the shell). > For my own curiosity, what does: > > foo='with space' > printf "%s\n" ${foo:+first "$foo"} > > print? That is, are the double-quotes even doing anything on such a > shell? On bash and dash, it prints: > > first > with space > > which is what I would expect. $ foo='with space' $ printf "%s\n" ${foo:+first "$foo"} first with space I also happen to have a handy-dandy test program called "showargs". $ foo='with space' $ showargs ${foo:+first "$foo"} uid=1001 euid=1001 gid=1001 egid=1001 umask(octal)=022 stdin=/dev/pts/12 stdout=/dev/pts/12 stderr=/dev/pts/12 pid=5261 $0=showargs $1=first with space So no quotes are being passed on. Of course bash works just fine. > So does "ash" (0.5.7, packaged for > Debian), which is what I _thought_ FreeBSD's shell was based on. But > clearly there is some divergence. I like to test on FreeBSD 8, which is slightly older, once in a while to make sure I catch stuff like this. :) Running "ident /bin/sh" shows a bunch of source file names which matches up pretty well with the dash distribution so I'm pretty sure it's just a much older ancestor of dash/ash. If I run dash 0.5.6 (installed via FreeBSD ports), it works properly too. > I guess they are getting eaten by your shell, otherwise we would pass > them along to git in the test script, which would complain. When I run t5528 with -v -x -d -i this is where it dies (without the fix): + git '-c push.default=upstream' push Unknown option: -c push.default=upstream So yeah, the quotes are gone, but no word-splitting occurred. ``` ## Jeff King, 2015-03-09 06:04 Subject: Re: [PATCH] t5528: do not fail with FreeBSD shell Message-ID: <20150309060403.GA27128@peff.net> URL: https://gitlist.dev/e/20150309060403.GA27128%40peff.net In-Reply-To: <211E8F7E-5588-45B1-ACF6-BB7DFB798ABB@gmail.com> ``` On Sun, Mar 08, 2015 at 10:19:20PM -0700, Kyle J. McKay wrote: > >I am not convinced this isn't a violation of POSIX (which specifies that > >field splitting is done on the results of parameter expansions outside > >of double-quotes). But whether it is or not, we have to live with it. > > That's not the only problem the shell has, t5560 had an issue, rebase had > issues. They've have been worked around. It probably also affects related > BSDs' shells as well (at least older versions that didn't change the shell). Yeah, I hope that didn't come across as "bleh, this shell is not worth supporting". It was "whether I think it is a bug or not, it is a real problem and we must work around it". > >For my own curiosity, what does: > [...] Thanks. Weird behavior, certainly, but I think the solution in your patch is the right thing to do. -Peff ```