Re: [BUG] redundant error message

From: Jeff King <peff@peff.net>
Date: Dec 6, 2013, 22:13 UTC
Message-ID: <20131206221356.GD25620@sigill.intra.peff.net>
In-Reply-To: <CACsJy8CKDiCX0G3jD6KLZ=cyvp6zf4MT6DCY0XodfD8rkQF=wA@mail.gmail.com>

On Fri, Dec 06, 2013 at 08:15:52AM +0700, Duy Nguyen wrote:

Show 23 quoted lines

> On Fri, Dec 6, 2013 at 4:28 AM, Jeff King <peff@peff.net> wrote:
> > BTW, the raw looping to find "--" made me wonder how we handle:
> >
> >   git log --grep -- HEAD
> >
> > I'd expect it to be equivalent to:
> >
> >   git log --grep=-- HEAD
> >
> > but it's not; we truncate the arguments and complain that --grep is
> > missing its argument. Which is probably good enough, given that the
> > alternative is doing a pass that understands all of the options. But it
> > does mean that the "--long-opt=arg" form is safer than the split form if
> > you are passing along an arbitrary "arg".
> 
> Maybe we could make setup_revisions() use parse_options() someday,
> which understands about arguments and dashdash.
> 
> $ ./git grep -e -- foo
> fatal: ambiguous argument 'foo': both revision and filename
> Use '--' to separate paths from revisions, like this:
> 'git <command> [<revision>...] -- [<file>...]'
> $ ./git grep -e -- -- foo

Yes, although we use it some in handle_revision_opt, I believe. The problem isn't inherent to parse_options or not, though. To do it correctly, we need to either:

  1. make two passes with the code that actually understands the options
     (be it parse_options or not); the first looking for "--", and the
     second to do the actual parsing. Right now our first pass does not
     understand the options at all.

  2. store the non-option arguments (including "--"), and only resolve
     and verify them after we have gone through the whole command-line
     and know whether we hit a "--" or not.

I suspect the second option would be simpler, as neither parse-options nor the current revision code is safe to run through twice (e.g., parse-options may have callbacks that add to a list, and we would need to add some kind of "dry-run" flag).

It's something that would be nice to fix, but I don't see myself working on it anytime soon. It's a lot of work for very little benefit.

-Peff