[PATCH 0/6] test -z/-n quoting fix + misc cleanups

From: Jeff King <peff@peff.net>
Date: May 13, 2016, 20:46 UTC
Message-ID: <20160513204654.GA10684@sigill.intra.peff.net>
In-Reply-To: <xmqqshxl9142.fsf@gitster.mtv.corp.google.com>

On Fri, May 13, 2016 at 01:03:41PM -0700, Junio C Hamano wrote:

Show 8 quoted lines

> > And sadly,
> >
> >   git grep 'test -n [^"]'
> >
> > is not empty.
> 
> Are you doing an audit?  Otherwise I'm interested in taking a brief
> look.

There was only one buggy case there (in git-stash). The rest were false positives.

I didn't audit for:

  test $foo = bar

which also has problems. You can grep for:

  git grep 'test \$'

and there are a lot of hits. Many of them are probably fine, if they are variables that are known to be non-empty and not contain whitespace (e.g., $#). But some of them are questionable, like:

  git-request-pull.sh:if test $(git cat-file -t "$head") = tag

I suspect in practice that's fine just because we're likely to see either the empty string (in which case test will barf with "unary operator expected", which matches what we want), or a single-word response (which doesn't need further quoting).

> >> 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.

Actually, it's not just older shells:

  foo='bar baz'
  test -z $foo

is "unspecified" according to POSIX, though in practice it will complain about "binary operator expected". You can get some weirdness, though, like:

  foo='!= bar'
  test -z $foo

which returns 0. Unlikely, but still clearly wrong for us not to be quoting.

Anyway. Here's a series that fixes the -n/-z cases, along with a bunch of cleanups that remove the false positives (most of which I sent out just a few minutes ago as "minor fixes to some svn tests").

  [1/6]: t/lib-git-svn: drop $remote_git_svn and $git_svn_id
  [2/6]: t9100,t3419: enclose all test code in single-quotes
  [3/6]: t9107: use "return 1" instead of "exit 1"
  [4/6]: t9107: switch inverted single/double quotes in test
  [5/6]: t9103: modernize test style
  [6/6]: always quote shell arguments to test -z/-n

You could take just 6/6 as its own series; the rest are just about removing the false positives, and fixing other issues. I put it last, though, because otherwise the "this grep is now empty" claim in it is not true. :)

-Peff