Re: [PATCH v1] stash show: fix breakage in 1.7.3

From: Jon Seymour <jon.seymour@gmail.com>
Date: Sep 25, 2010, 06:19 UTC
Message-ID: <AANLkTimK4nTr9bS0L9xc9mGo5F900pUAOQoDnZ9HMdEz@mail.gmail.com>
In-Reply-To: <AF7816CF-7515-4386-9736-417005434433@gernhardtsoftware.com>

On Sat, Sep 25, 2010 at 2:45 PM, Brian Gernhardt <brian@gernhardtsoftware.com> wrote:

Show 7 quoted lines

>
> On Sep 24, 2010, at 11:32 PM, Jon Seymour wrote:
>
>> due to a faulty assumption that:
>>   git rev-parse --no-revs -- stash@{0}
>
> This assumption is faulty, it should be "git rev-parse --no-revs --flags stash@{0}", which works properly for all revision arguments and flags _except_ -q and --quiet.

Agreed. If I recall the evolution of the code, I originally had --no-revs --flags, but then found -q was being eaten. The only way to prevent it being eaten was to remove --flags and add --, which "fixed" the -q problem but created the git stash show xxx problem. Of course, had my unit tests been more thorough, I would have picked this, but alas they were not.

Show 7 quoted lines

>
>> This revision further simplifies the parsing code
>> by removing use of git rev-parse for FLAGS parsing
>> altogether.
>
> That is simpler, and does fix this specific issue.  However, I would strongly argue that "git rev-parse --no-revs --flags" is broken.  I really don't have the time tonight or probably this weekend to work on it, but git-rev-parse should only take "-q" and "--quiet" for itself if "--verify" was passed.  (Since that is the only mode in which rev-parse uses quiet, AFAIK.)
>

This seems like a reasonable compromise to me.

> Possibly rev-parse should also (or instead) separate "arguments for rev-parse" and "arguments rev-parse is parsing" using the standard "--".  I don't know if this will affect any current users.
>

I suspect this could be problematic.

Current behaviour:

   $ git rev-parse --flags -X -- Y -Z
   -X

Here -- is being used to say, don't interpret anything after -- as being flag-like even if it could be flag-like.

So, if a command foo uses git rev-parse to parse its own arguments:

    foo -X -- Y -Z

then I suspect, it would only want to treat -X as being flag-like, from the user of foo's point of view, both Y and -Z are not meant to be interpreted by foo.

It might make sense for --flags to be an instruction to git rev-parse not to interpret any subsequent arguments as git rev-parse options so that:

  $ git rev-parse --flags -q --no-flags -- --revs-only

would output:

  -q --no-flags

That is, prevent -q being eaten by git rev-parse, but preserve existing interpretation of --.

> ~~ Brian
>
>