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

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

quoted

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

quoted

quoted

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