git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Taylor Blau <me@ttaylorr.com>, Git List <git@vger.kernel.org>,
	Elijah Newren <newren@gmail.com>,
	Junio C Hamano <gitster@pobox.com>,
	Shourya Shukla <shouryashukla.oo@gmail.com>
Subject: Re: [PATCH v2] test_cmp: diagnose incorrect arguments
Date: Wed, 12 Aug 2020 11:37:05 -0400	[thread overview]
Message-ID: <20200812153705.GC33189@coredump.intra.peff.net> (raw)
In-Reply-To: <CAPig+cSroWZEoOL78COmPS4rkvKLE-yCiqh6Part+5gUgVon+A@mail.gmail.com>

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

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

> 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

  parent reply	other threads:[~2020-08-12 15:37 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-09  6:08 [PATCH] test_cmp: diagnose incorrect arguments Eric Sunshine
2020-08-09  8:32 ` Shourya Shukla
2020-08-09  8:49   ` Eric Sunshine
2020-08-09 17:42 ` [PATCH v2] " Eric Sunshine
2020-08-09 19:12   ` Junio C Hamano
2020-08-09 19:34     ` Eric Sunshine
2020-08-10 15:22       ` Junio C Hamano
2020-08-11 18:32   ` Taylor Blau
2020-08-11 19:25     ` Eric Sunshine
2020-08-11 21:03       ` Taylor Blau
2020-08-12 15:37       ` Jeff King [this message]
2020-08-12 16:15         ` Eric Sunshine
2020-08-12 16:39           ` Eric Sunshine
2020-08-12 17:10             ` Jeff King
2020-10-16  0:17   ` Jeff King
2020-10-16  2:18     ` Eric Sunshine
2020-10-16 18:36       ` Junio C Hamano
2020-10-16 20:56         ` Junio C Hamano
2020-10-16 23:14           ` Junio C Hamano
2020-10-17  6:06             ` Eric Sunshine

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200812153705.GC33189@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=me@ttaylorr.com \
    --cc=newren@gmail.com \
    --cc=shouryashukla.oo@gmail.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).