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

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

quoted

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.

quoted

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