Re: t4151 missing quotes

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, May 9, 2016 at 4:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>> On Mon, May 9, 2016 at 2:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Something like this follows Documentation/SubmittingPatches [...]
>>>
>>> -- >8 --
>>> From: Armin Kunaschik <megabreit@googlemail.com>
>>> Subject: t4151: make sure argument to 'test -z' is given
>>>
>>> 88d50724 (am --skip: revert changes introduced by failed 3way merge,
>>> 2015-06-06), unlike all the other patches in the series, forgot to
>>> quote the output from "$(git ls-files -u)" when using it as the
>>> argument to "test -z", leading to a syntax error.
>>
>> To make it clear that this was not a syntax error in the typical case,
>> it might make sense to say:
>>
>>     ...potentially leading to a syntax error if some earlier tests failed.
>
> Hmph, do we have a broken &&-chain?

I don't know. Unfortunately, Armin didn't provide much information in
his initial email, saying only "skipping through some failed tests",
which doesn't necessarily indicate if those tests failed or if he
somehow manually skipped them.

> If an earlier test fails and leaves an unmerged path, "ls-files -u"
> would give some output, so "test -z" would get one or more non-empty
> strings; if we feed multiple, this would fail.  But we would not have
> even run "test -z" as long as we properly &&-chain these tests.
>
> I think the real issue is when the earlier step succeeds and does
> not leave any unmerged path.  In that case, we would run "test -z"
> without anything else on the command line, which would lead to an
> syntax error.
>
>     Side Note: /usr/bin/test and test (built into bash and dash)
>     seem not to care about the lack of string in "test -z <string>"
>     and "test -n <string>".  It appears to me that they just take
>     "-z" and "-n" without "<string>" as a special case of "test
>     <string>" that is fed "-z" or "-n" as <string>.  Apparently, the
>     platform Armin is working on doesn't.

I also tested on Mac OS X and BSD, and they happily accept bare "test
-n", as well (though, I don't doubt that there are old shells which
complain).

> Perhaps
>
>     ... leading to a syntax error on some platforms whose "test"
>     does not interpret "test -z" (no other arguments) as testing if
>     a string "-z" is the null string (which GNU test and test that
>     is built into bash and dash seem to do).
>
> would be an improvement?

Yes, that sounds good.