Re: [PATCH v2] test_cmp: diagnose incorrect arguments

From: Jeff King <peff@peff.net>
Date: Aug 12, 2020, 15:37 UTC
Message-ID: <20200812153705.GC33189@coredump.intra.peff.net>
In-Reply-To: <CAPig+cSroWZEoOL78COmPS4rkvKLE-yCiqh6Part+5gUgVon+A@mail.gmail.com>

On Tue, Aug 11, 2020 at 03:25:03PM -0400, Eric Sunshine wrote:

Show 12 quoted lines

> 'test' can accept both switches (i.e. "-e") and non-switch arguments.
> Keep in mind, too, that all the quoting is stripped by the shell
> _before_ 'test' ever sees its arguments. Let's say that the caller has
> a filename whose name actually is "-e" and passes that in as $1. So,
> what does 'test' see?
> 
>     test -e = -
> 
> Rather than comparing literal string "-e" to literal string "-", it's
> instead (almost) asking if the file named "=" exists; I say "almost"
> because it's actually an error since switch -e only accepts one
> argument, but it's being given two arguments, "=" and "-".

I don't think this is an error. The program can tell which you meant by the number of arguments. POSIX lays out some rules here (from "man 1posix test" on my system, but I'm sure you can find it online):

  3 arguments:
    *  If $2 is a binary primary, perform the binary test of $1 and $3.

    *  If $1 is '!', negate the two-argument test of $2 and $3.

    *  If $1 is '(' and $3 is ')', perform the unary test of $2.  On
       systems that do not support the XSI option, the results are
       unspecified if $1 is '(' and $3 is ')'.

    *  Otherwise, produce unspecified results.

So we'd see that "=" is a binary primary (the complete set is defined earlier). Likewise "! -e -" would hit the second rule. We wouldn't get fooled by trying to compare the string "!" because it knows that "=" is a binary operator and "-e" is a unary operator.

It gets weird with "-a" joining expressions. There's some discussion in the Rationale section of the posix page, and it even warns explicitly against "-a" (in favor of "test expr1 && test expr1").

Show 6 quoted lines

> which may or may not be an error in a particular implementation of
> 'test'. Some implementations may understand that "-" is not a valid
> switch, thus infer that you're actually asking for an equality
> comparison between arguments, but other implementations may complain
> either that there is no switch named "-" or that those arguments
> simply make no sense.

Yeah, historically I think there were shells that were not quite so clever. I have no idea which ones, or whether any are still around. I don't think we've generally been that concerned with this case in Git, and nobody has complained. I'd be totally unsurprised to hear that SunOS /bin/sh doesn't get this right, but we've already written it off as unusable (it doesn't even support $() expansion).

-Peff