threads / patch / 38757

patcht5528: do not fail with FreeBSD shell

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

## tl;dr

4 messages between Mar 8, 2015 and Mar 9, 2015. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Kyle J. McKay· Mar 8, 2015, 15:37 UTC · lore
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(-)
Show changes to t/t5528-push-default.sh +2 −2
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· Mar 8, 2015, 17:56 UTC · re: Kyle J. McKay · lore

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

On Sun, Mar 08, 2015 at 08:37:50AM -0700, Kyle J. McKay wrote:
Show 9 quoted lines
> 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· Mar 9, 2015, 05:19 UTC · re: Jeff King · lore

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

On Mar 8, 2015, at 10:56, Jeff King wrote:
Show 19 quoted lines
> 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).

Show 12 quoted lines
> 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· Mar 9, 2015, 06:04 UTC · re: Kyle J. McKay · lore

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

On Sun, Mar 08, 2015 at 10:19:20PM -0700, Kyle J. McKay wrote:
Show 7 quoted lines
> >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

← back to recent threads