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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1760 bytes --]

On Thu, Nov 09, 2023 at 01:48:43PM -0500, Jeff King wrote:
> On Thu, Nov 09, 2023 at 08:41:33PM +0900, Junio C Hamano wrote:
> 
> > > -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).

Right, let me fix the missing quoting while at it.

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

Agreed. I'll amend the commit message to say so.

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

Will fix.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]