Re: [PATCH] revision: allow pseudo options after --end-of-options

From: Jeff King <peff@peff.net>
Date: Jul 12, 2021, 17:54 UTC
Message-ID: <YOyByjmGu1oDXK4X@coredump.intra.peff.net>
In-Reply-To: <YOoXCJV2ssef/KsN@camp.crustytoothpaste.net>

On Sat, Jul 10, 2021 at 09:54:16PM +0000, brian m. carlson wrote:

Show 20 quoted lines

> > But for the original implementation, because pseudo revision options
> > (--branches, --tags, --not, ..., etc) can not be used after the
> > "--end-of-options" option, we have to put "--end-of-options" at the
> > end of revisions, such as:
> > 
> >     git log --pretty="%m %s" rev1 --not rev2 rev3 rev4 \
> >             --end-of-options -- path/file
> 
> Or you could just use the other syntax and not have the problem.  Or you
> could write this:
> 
>   git log --pretty="%m %s" refs/heads/rev1 --not --end-of-options rev2 rev3 rev4 \
>           -- path/file
> 
> Unless there's a functional problem we're trying to solve, I'd much
> rather we didn't make --end-of-options means
> --end-of-some-options-but-not-others.  That makes it hard to reason
> about, and if someone does have a need for disabling all options, then
> we have to add another option.  It's also incompatible with the previous
> behavior, so whereas "--not" used to be a revision, now it's an option.

I agree that if we can avoid making exceptions, it makes the whole thing conceptually much cleaner (both for users to understand, but also for us to avoid accidentally introducing a security problem).

I don't think fully-qualifying refs is a complete solution, though. The common use case for --end-of-options is that you're passing along names from somewhere else, and you don't know how to qualify them. E.g., in:

  git rev-list --end-of-options "$rev" --

you need to behave differently if you got "1234abcd" versus "foo" versus "refs/heads/foo".

For --not, I do think using "^" is a complete solution. It's a little more work for the caller to prepend to each argument, but there's no policy logic they have to implement.

Looking over the other pseudo-opts, I could see some where treating them as a rev is reasonable (e.g., "--all"), but many where it is not at all (e.g., "--no-walk"; why is this even in handle_revision_pseudo_opt?). Even if you're just passing along untrusted revision specifiers, they act in roughly the same way as a single specifier. The big thing we'd lose is that you could never refer to a branch named "--not" or "--all".

So my gut feeling is _not_ to support them, but I can see arguments in both directions and I don't feel that strongly about it.

Show 12 quoted lines

> > Yes, "--end-of-options" must be used if there is a revision which
> > starts with dash, such as branch "--output=yikes" in t6000. That's
> > even stranger, for we have to write  command in the middle of
> > revisions like this:
> > 
> >     git log --pretty="%m %s" rev1 --not rev2 rev3 \
> >             --end-of-options --output=yikes -- path/file
> > 
> > I know "rev1..rev2" and "rev2 ^rev1", but I prefer to use "rev1 --not
> > rev2 rev3" instead of "rev1 ^rev2 ^rev3".
> 
> I don't think a personal preference is a good reason to change this.

I do think it rises slightly above personal preference. It's potentially making things much easier for the caller if they can ferry along:

  tip=$1; shift
  git rev-list --end-of-options "$1" --not "$@"

instead of:

  tip=$1; shift
  # whoops, whitespace splitting is wrong here! Real programming
  # languages make this easier, of course.
  git rev-list --end-of-options "$1" $(for i in "$@"; do echo "^$i"; done)

Though in my experience it is usually a static "--not --all" or "--not --branches --tags" or similar in such a function. I don't think I've ever seen a case quite like the code above in practice.

-Peff