[PATCH] t5528: do not fail with FreeBSD shell

[PATCH] t5528: do not fail with FreeBSD shell

[Kyle J. McKay]

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 <mackyle@gmail.com>
---
 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
 }
---

Re: [PATCH] t5528: do not fail with FreeBSD shell

[Jeff King]

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

Re: [PATCH] t5528: do not fail with FreeBSD shell

[Kyle J. McKay]

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.

Re: [PATCH] t5528: do not fail with FreeBSD shell

[Jeff King]

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