Re: [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`

From: Jeff King <peff@peff.net>
Date: Nov 9, 2023, 18:48 UTC
Message-ID: <20231109184843.GC2711684@coredump.intra.peff.net>
In-Reply-To: <xmqqpm0jyx02.fsf@gitster.g>

On Thu, Nov 09, 2023 at 08:41:33PM +0900, Junio C Hamano wrote:

Show 9 quoted lines

> > -elif test -d ${GIT_DIR:-.git} -o -f .git &&
> > +elif ( test -d ${GIT_DIR:-.git} || test -f .git ) &&
> 
> I do not think this is strictly necessary.
> 
> Because the command line parser of "test" comes from left, notices
> "-d" and takes the next one to check if it is a directory.  There is
> no value in ${GIT_DIR} can make "test -d ${GIT_DIR} -o ..." fail the
> same way as the problem Peff pointed out during the discussion.

I think this is one of the ambiguous cases. If $GIT_DIR is "=", then "test" cannot tell if you meant:

  var1=-d
  var2=-o
  test "$var1" = "$var2" ...

or:

  var1="="
  test -d "$var1" -o ...

With bash, for example:

  $ test -d /tmp -o -f .git; echo $?
  0
  $ test -d = -o -f .git; echo $?
  bash: test: syntax error: `-f' unexpected
  2

Without "-o", it uses the number of arguments to disambiguate (though of course the lack of quotes around $GIT_DIR is another potential problem here).

And I think the same is true of the other cases below using "-z", "-n", and so on.

But IMHO it is worth getting rid of all -o/-a regardless. Even non-ambiguous cases make reasoning about the code harder, and we don't want to encourage people to think they're OK to use.

> I do not need a subshell for grouping, either.  Plain {} should do
> (but you may need a LF or semicolon after the statement)..

This I definitely agree with. :)

-Peff