On Sat, Jul 10, 2021 at 09:54:16PM +0000, brian m. carlson wrote: > > 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. > > 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
