Re: t3404 static check of bad SHA-1 failure

From: Junio C Hamano <gitster@pobox.com>
Date: May 13, 2016, 19:52 UTC
Message-ID: <xmqqwpmx91mb.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160513182325.GB30700@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 42 quoted lines

> On Fri, May 13, 2016 at 06:09:35PM +0200, Armin Kunaschik wrote:
>
>> in t3404 test 91 - static check of bad SHA-1 fails (with ksh) with a
>> syntax error in git-rebase.
>> git-rebase[6]: test: argument expected
>
> Here's a fix that covers these and another I found:
>
> -- >8 --
> Subject: [PATCH] always quote arguments to "test -z" in shell
>
> Modern shells are pretty forgiving about us doing:
>
>   test -z $foo
>
> If $foo is indeed empty, the test command will see only:
>
>   test -z
>
> and treat the missing argument as "yes, this is empty". But
> some older shells, reportedly ksh88, complain about the
> missing argument. We can be more portable by spelling this
> as:
>
>   test -z "$foo"
>
> so that "test" sees an empty argument, not a missing one.
>
> This covers all cases detected by:
>
>   git grep 'test -z [^"]'
>
> (though note that has a few false positives for tests which
> need an extra layer of quoting to do '\"').
>
> Reported-by: Armin Kunaschik <megabreit@googlemail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Actually, this misses the case in t4151 which already has a fix queued
> on pu. Arguably these should all just be squashed together (and I am
> happy, Junio, if you want to do so and leave Armin as the author of the
> new commit).

I _think_ "test -z" should succeed according to POSIX, because

 (1) it is not "test -z string" because it lacks string,

 (2) it is not any of the other "test -<option> thing" because -z,
    and

 (3) the only thing it matches in the supported form of "test" is
     "test <string>" that tests if the <string> is not the null
     string, and "-z" indeed is not the null string.

For the same reason, "test -n" succeeds.

But working around older/broken shells is easy and the resulting script it more readable, so let's take this. It makes the resulting code easier to understand even when we know we run it under POSIX shell.

Thanks.

Show 36 quoted lines

>  git-rebase--interactive.sh | 4 ++--
>  git-stash.sh               | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 9ea3075..470413b 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -866,12 +866,12 @@ add_exec_commands () {
>  # $3: the input filename
>  check_commit_sha () {
>  	badsha=0
> -	if test -z $1
> +	if test -z "$1"
>  	then
>  		badsha=1
>  	else
>  		sha1_verif="$(git rev-parse --verify --quiet $1^{commit})"
> -		if test -z $sha1_verif
> +		if test -z "$sha1_verif"
>  		then
>  			badsha=1
>  		fi
> diff --git a/git-stash.sh b/git-stash.sh
> index c7c65e2..57f9dc1 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -185,7 +185,7 @@ store_stash () {
>  
>  	git update-ref --create-reflog -m "$stash_msg" $ref_stash $w_commit
>  	ret=$?
> -	test $ret != 0 && test -z $quiet &&
> +	test $ret != 0 && test -z "$quiet" &&
>  	die "$(eval_gettext "Cannot update \$ref_stash with \$w_commit")"
>  	return $ret
>  }