Re: [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
On Thu, Nov 09, 2023 at 01:48:43PM -0500, Jeff King wrote:
Show 35 quoted lines
> 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.
Show 6 quoted lines
> 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. :)Patrick Steinhardt <ps@pks.im>