Shell quote

[chriscool]

Fix shellquote() implementation and add tests as discussed in PR #1757.
Thanks @ion1!

[chriscool]

Yeah \012 caused problems. And still it looks like I messed up something as the tests don't pass.
I will also try to use "%s" instead of '%s' as you are right it should work and be simpler.

[chriscool]

I think I should drop the first patch as instead of fixing things it makes them fail.

[chriscool]

It looks like shellquote() doesn't work the same way on MacOS and on Linux :-(

[chriscool]

The problem comes from sed adding a newline character on Mac OS.

Let's try with:

printf '%s' "$(printf '%s' "$_arg" | sed -e "s/'/'\\\\''/g; s/^/'/; s/\$/'/;")"

as encapsulating everything inside a printf should remove the extra newline.

[chriscool]

The Travis test failure is not related, so it looks like encapsulating in a printf is working.

[chriscool]

@ion1 By the way the Git implementation quotes both ' and ! specially:

https://github.com/git/git/blob/master/quote.c#L23

so maybe we will have to do something for ! too.

[ion1]

Huh, interesting. I have never seen a shell which needs special handling for ! within ''s. I tested this with some shells some time ago (link below). The only bytes the shells I tested had problems with were 0 (excluded from the test), newline, 128 and 255 (decimal). I wish they documented their reason for the ! thing.

https://gist.github.com/ion1/7405148

Testing whether you can safely sh-quote any byte string (consisting of no null bytes) by substituting ' with ''' and adding ' on either side. Works in bash, dash, mksh, posh, zsh. Fails in csh, ksh, lksh, pdksh. I was informed OpenBSD ksh works correctly.

[chriscool]

It is because of csh-based shells according to the following commit:

git/git@77d604c

[ion1]

This should substitute ! with '\!' and ' with '\''.

sed -e 's/\([!'\'']\)/'\''\\\1'\''/g'

[ion1]

Since you’re working around the newline issue with a wrapping printf, I wonder if you could just do printf "'%s'" in the wrapper and get the s/^/'/; s/$/'/ out of the sed expression? I understand we have given up on handling newlines given that some shells have trouble with them, but even trying to split a single parameter into two at newlines (which the s///s result in) is still wrong.

[chriscool]

Yeah, it is a good idea I think to do printf "'%s'" in the wrapper and get the s/^/'/; s/$/'/ out of the sed expression. The last commit I sent does that and also puts the space in the wrapper printf().

[chriscool]

The CI test failure is not related.

[jbenet]

thanks @chriscool