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