From: Jiang Xin <redacted>
Options and revisions can be seperated by the option "--end-of-options"
by introducing commit 19e8789b23 (revision: allow --end-of-options to
end option parsing, 2019-08-06). The following command will show
revisions which have changes on file "bar" on a branch named "--foo":
git rev-list --oneline --end-of-options --foo -- bar
If we want to see revisions between two revisions (rev1 and rev2), we
can use the following command:
git rev-list --oneline --end-of-options rev1..rev2 --
We know that "rev1..rev2" is a shorthand for "rev2 --not rev1", but
we can not use the longer expression with option "--not" after the
"--end-of-options" option. This is because the parser will not consume
revision pseudo options after seeing "--end-of-option".
Allow parsing revision pseudo options after "--end-of-options", the
following command is valid:
git rev-list --oneline --end-of-options rev2 --not rev2 --
Signed-off-by: Jiang Xin <redacted>
---
revision.c | 7 ++++---
t/t6000-rev-list-misc.sh | 45 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 49 insertions(+), 3 deletions(-)
From: brian m. carlson <hidden> Date: 2021-07-08 17:02:05
On 2021-07-08 at 15:03:16, Jiang Xin wrote:
From: Jiang Xin <redacted>
Options and revisions can be seperated by the option "--end-of-options"
by introducing commit 19e8789b23 (revision: allow --end-of-options to
end option parsing, 2019-08-06). The following command will show
revisions which have changes on file "bar" on a branch named "--foo":
git rev-list --oneline --end-of-options --foo -- bar
If we want to see revisions between two revisions (rev1 and rev2), we
can use the following command:
git rev-list --oneline --end-of-options rev1..rev2 --
We know that "rev1..rev2" is a shorthand for "rev2 --not rev1", but
we can not use the longer expression with option "--not" after the
"--end-of-options" option. This is because the parser will not consume
revision pseudo options after seeing "--end-of-option".
Allow parsing revision pseudo options after "--end-of-options", the
following command is valid:
git rev-list --oneline --end-of-options rev2 --not rev2 --
I don't think we want to do this. The goal of --end-of-options is to
prevent parsing all future items as options, so if someone specifies a
revision starting with a dash, we don't end up with it being interpreted
as an option.
Removing this constraint could end up resulting in potential security
issues, which this option was introduced to protect against.
If you want to specify this form, you can write this:
git rev-list --oneline refs/heads/rev2 --not refs/heads/rev1
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
brian m. carlson [off-list ref] 于2021年7月9日周五 上午1:02写道：
On 2021-07-08 at 15:03:16, Jiang Xin wrote:
quoted
From: Jiang Xin <redacted>
Options and revisions can be seperated by the option "--end-of-options"
by introducing commit 19e8789b23 (revision: allow --end-of-options to
end option parsing, 2019-08-06). The following command will show
revisions which have changes on file "bar" on a branch named "--foo":
git rev-list --oneline --end-of-options --foo -- bar
If we want to see revisions between two revisions (rev1 and rev2), we
can use the following command:
git rev-list --oneline --end-of-options rev1..rev2 --
We know that "rev1..rev2" is a shorthand for "rev2 --not rev1", but
we can not use the longer expression with option "--not" after the
"--end-of-options" option. This is because the parser will not consume
revision pseudo options after seeing "--end-of-option".
Allow parsing revision pseudo options after "--end-of-options", the
following command is valid:
git rev-list --oneline --end-of-options rev2 --not rev2 --
I don't think we want to do this. The goal of --end-of-options is to
prevent parsing all future items as options, so if someone specifies a
revision starting with a dash, we don't end up with it being interpreted
as an option.
New test case in t6000 covered this case. Branch "--output=yikes"
which starts with a dash is used as revision after the option
"--end-of-options", and it won't be interpreted as an option.
test_expect_success 'parse pseudo option "--not" after "--end-of-options"' '
cat >expect <<-EOF &&
> three
EOF
git log --pretty="%m %s" --end-of-options \
HEAD --not --output=yikes -- \
two/three >actual &&
test_cmp expect actual
'
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
We can see from the above command, the option "--end-of-options" is
immediately followed by a dashdash. That is very strange. DashDash is
designed to separate pathspecs from args, and "--end-of-options" is
designed to separate revisions from options. But because of the pseudo
revision options, "--end-of-options" is meaningless for commands
calling "setup_revisions()".
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".
From: brian m. carlson <hidden> Date: 2021-07-10 21:55:00
On 2021-07-09 at 01:33:23, Jiang Xin wrote:
New test case in t6000 covered this case. Branch "--output=yikes"
which starts with a dash is used as revision after the option
"--end-of-options", and it won't be interpreted as an option.
test_expect_success 'parse pseudo option "--not" after "--end-of-options"' '
cat >expect <<-EOF &&
> three
EOF
git log --pretty="%m %s" --end-of-options \
HEAD --not --output=yikes -- \
two/three >actual &&
test_cmp expect actual
'
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.
It's unfortunate that we're not using -- here instead of
--end-of-options because the former is the standard syntax, but that's
what we have now since that's already used elsewhere.
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.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
From: Jeff King <hidden> Date: 2021-07-12 17:54:22
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
On Sat, Jul 10, 2021 at 09:54:16PM +0000, brian m. carlson wrote:
quoted
quoted
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.
Last week, I made a study on how gitlab wrap and execute a git
command. I saw the following code [1]:
if c.supportsEndOfOptions() {
commandArgs = append(commandArgs, "--end-of-options")
}
if len(postSepArgs) > 0 {
commandArgs = append(commandArgs, "--")
}
I was surprised to see the options "--end-of-options" and "--" used
next to each other, and the DashDash option ("--") is not mandatory. I
want to make some changes on it, but when I try to construct a git
command like this:
git.SubCmd{
Name: "log",
Flags: []git.Option{
git.Flag{
Name: "--stat"
},
git.ValueFlag{
Name: "--pretty",
Value: "%m %s",
},
git.Flag{
Name: "--no-decorate",
},
},
Args: []string {
"topic1",
"--not",
"main",
"release",
},
PostSepArgs: []string {
"src/hello.c",
"doc",
},
}
The generated git command will be:
git log --stat --pretty="%m %s" --no-decorate \
topic1 --not main release \
--end-of-options \
-- \
src/hello.c doc
It works. But if I move the "--end-of-options" before the revisions like this:
git log --stat --pretty="%m %s" --no-decorate \
--end-of-options \
topic1 --not main release \
-- \
src/hello.c doc
The generated command failed to execute with error: unknown revision "--not".
It's reasonable for gitlab to construct git commands using mainly three fields:
1. Flags: for options like "--option", or "--key value".
2. Args: for args like revisions.
3. PostSepArgs: for pathspecs.
And if the command supports these options, it's better to add
"--end-of-options" between 1 and 2, and add "--" between 2 and 3.
If we can handle revision pseudo opts as pseudo revisions instead of
options as in this patch, the only disadvantage is that we cannot
handle branches whose names conflict with well-known options such as
"--not" and "--all". But users can input full branch names, such as
"refs/heads/--not", "refs/heads/--all".
Maybe I should keep this patch as a local enhancement for git.
1. https://gitlab.com/gitlab-org/gitaly/-/blob/v14.0.5/internal/git/command_description.go#L320-326
From: Jeff King <hidden> Date: 2021-07-13 21:13:42
On Tue, Jul 13, 2021 at 04:57:46PM +0800, Jiang Xin wrote:
quoted
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.
Last week, I made a study on how gitlab wrap and execute a git
command. I saw the following code [1]:
if c.supportsEndOfOptions() {
commandArgs = append(commandArgs, "--end-of-options")
}
if len(postSepArgs) > 0 {
commandArgs = append(commandArgs, "--")
}
I was surprised to see the options "--end-of-options" and "--" used
next to each other, and the DashDash option ("--") is not mandatory.
I think using --end-of-options there is pointless. The "--" will always
signal the end of options (_and_ revisions). So if there is nothing
between the two, then the former cannot be doing anything.
For programmatic use, I do think one should always use "--". Even if
there are no paths, it makes it clear that the arguments before it are
meant to be revisions. This matters a little less in a bare repo, I
think (because we do not have a working tree to try to DWIM-match the
paths in), but it's good practice in general.
I don't think you can just blindly add "--" in such a function, though.
It depends on what the interface of the function is (i.e., are its
callers passing in options, revisions, and pathspecs as separate data
structures). It does look like you're going in that direction below,
though.
I
want to make some changes on it, but when I try to construct a git
command like this:
git.SubCmd{
Name: "log",
Flags: []git.Option{
git.Flag{
Name: "--stat"
},
git.ValueFlag{
Name: "--pretty",
Value: "%m %s",
},
git.Flag{
Name: "--no-decorate",
},
},
Args: []string {
"topic1",
"--not",
"main",
"release",
},
PostSepArgs: []string {
"src/hello.c",
"doc",
},
}
The generated git command will be:
git log --stat --pretty="%m %s" --no-decorate \
topic1 --not main release \
--end-of-options \
-- \
src/hello.c doc
It works.
Right, but you are not getting any protection against "topic1" being an
option. The --end-of-options is doing nothing.
But if I move the "--end-of-options" before the revisions like this:
git log --stat --pretty="%m %s" --no-decorate \
--end-of-options \
topic1 --not main release \
-- \
src/hello.c doc
The generated command failed to execute with error: unknown revision "--not".
It's reasonable for gitlab to construct git commands using mainly three fields:
1. Flags: for options like "--option", or "--key value".
2. Args: for args like revisions.
3. PostSepArgs: for pathspecs.
And if the command supports these options, it's better to add
"--end-of-options" between 1 and 2, and add "--" between 2 and 3.
Yeah, so the problem there is that the definition of "Args" is kind of
fuzzy. Sometimes it is useful to include stuff like "--not", and
sometimes it is dangerous or unexpected. Later you say:
If we can handle revision pseudo opts as pseudo revisions instead of
options as in this patch, the only disadvantage is that we cannot
handle branches whose names conflict with well-known options such as
"--not" and "--all". But users can input full branch names, such as
"refs/heads/--not", "refs/heads/--all".
but the point of --end-of-options is that you do not necessarily trust
the caller (or the user) to have prefixes with "refs/heads/" as
appropriate. It is about telling Git unambiguously what the various bits
on the command-line mean so that it knows how to correctly interpret
them.
It might be worthwhile to split the function interface into two sets:
positive and negative traversal tips. And then the function can turn:
{
Options: { "--stat" }
Tips: { "topic1" }
Not-Tips: { "main", "release" }
Pathspecs: { "src/hello.c }
}
into:
git rev-list --stat --end-of-options topic1 ^main ^release -- src/hello.c
which is unambiguous, even if "--not" appears in "Tips". (You can also
keep a single Args array, but then the caller is responsible for putting
in the "^" markers).
-Peff
From: Patrick Steinhardt <hidden> Date: 2021-07-27 06:11:09
On Tue, Jul 13, 2021 at 05:13:40PM -0400, Jeff King wrote:
On Tue, Jul 13, 2021 at 04:57:46PM +0800, Jiang Xin wrote:
quoted
quoted
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.
Last week, I made a study on how gitlab wrap and execute a git
command. I saw the following code [1]:
if c.supportsEndOfOptions() {
commandArgs = append(commandArgs, "--end-of-options")
}
if len(postSepArgs) > 0 {
commandArgs = append(commandArgs, "--")
}
I was surprised to see the options "--end-of-options" and "--" used
next to each other, and the DashDash option ("--") is not mandatory.
I think using --end-of-options there is pointless. The "--" will always
signal the end of options (_and_ revisions). So if there is nothing
between the two, then the former cannot be doing anything.
Indeed it is. I somehow missed your Cc (not used to receiving Git ML
mails on my work address), but by chance I fixed this last week because
I realized this was broken. We've now moved the `--end-of-options` flag
between positional arguments and flags like it should've been from the
beginning [1].
[snip]
quoted
But if I move the "--end-of-options" before the revisions like this:
git log --stat --pretty="%m %s" --no-decorate \
--end-of-options \
topic1 --not main release \
-- \
src/hello.c doc
The generated command failed to execute with error: unknown revision "--not".
It's reasonable for gitlab to construct git commands using mainly three fields:
1. Flags: for options like "--option", or "--key value".
2. Args: for args like revisions.
3. PostSepArgs: for pathspecs.
And if the command supports these options, it's better to add
"--end-of-options" between 1 and 2, and add "--" between 2 and 3.
Yeah, so the problem there is that the definition of "Args" is kind of
fuzzy. Sometimes it is useful to include stuff like "--not", and
sometimes it is dangerous or unexpected. Later you say:
Yeah, this is something that has been bothering me, too. It would be
nice to give special treatment to pseudo-revisions in git. That's why we
have now marked git-rev-list(1) to not support `--end-of-options` in
Gitaly, because we do pass pseudo-revisions to it in multiple places.
Patrick
[1]: https://gitlab.com/gitlab-org/gitaly/-/merge_requests/3676