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

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

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

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 <zhiyou.jx@alibaba-inc.com>
---
 revision.c               |  7 ++++---
 t/t6000-rev-list-misc.sh | 45 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/revision.c b/revision.c
index 8140561b6c..0d37b5a759 100644
--- a/revision.c
+++ b/revision.c
@@ -2729,9 +2729,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		revarg_opt |= REVARG_CANNOT_BE_FILENAME;
 	for (left = i = 1; i < argc; i++) {
 		const char *arg = argv[i];
-		if (!seen_end_of_options && *arg == '-') {
-			int opts;
+		int opts;
 
+		if (*arg == '-') {
 			opts = handle_revision_pseudo_opt(submodule,
 						revs, argv + i,
 						&flags);
@@ -2739,7 +2739,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 				i += opts - 1;
 				continue;
 			}
+		}
 
+		if (!seen_end_of_options && *arg == '-') {
 			if (!strcmp(arg, "--stdin")) {
 				if (revs->disable_stdin) {
 					argv[left++] = arg;
@@ -2767,7 +2769,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			continue;
 		}
 
-
 		if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
 			int j;
 			if (seen_dashdash || *arg == '^')
diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 12def7bcbf..3b2ca10456 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -169,4 +169,49 @@ test_expect_success 'rev-list --count --objects' '
 	test_line_count = $count actual
 '
 
+test_expect_success 'merge branch "--output=yikes" to main' '
+	git checkout main &&
+	git merge -m "Merge branch" \
+		--allow-unrelated-histories -- \
+		--output=yikes &&
+	echo three >> two/three &&
+	git add two/three &&
+	test_tick &&
+	git commit -m "three" &&
+	cat >expect <<-EOF &&
+	> three
+	> Merge branch
+	> another
+	> that
+	> two
+	> one
+	EOF
+	git log --pretty="%m %s" --end-of-options HEAD >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'parse pseudo option "--branches" after "--end-of-options"' '
+	cat >expect <<-EOF &&
+	> three
+	> another
+	> Merge branch
+	> that
+	> two
+	> one
+	EOF
+	git log --pretty="%m %s" --end-of-options \
+		--branches -- >actual &&
+	test_cmp expect actual
+'
+
+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
+'
+
 test_done
-- 
2.32.0.rc0.27.g7b1e85181b

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1640 bytes --]

On 2021-07-08 at 15:03:16, Jiang Xin wrote:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> 
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Jiang Xin]

brian m. carlson <sandals@crustytoothpaste.net> 于2021年7月9日周五 上午1:02写道：
>
> On 2021-07-08 at 15:03:16, Jiang Xin wrote:
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >
> > 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".

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2347 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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".

I suspect that you can prefix "^^" unconditionally, just like --not
can be emulated away by unconditionally prefixing "^".

> 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.

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

[Jeff King]

On Mon, Jul 12, 2021 at 11:47:23AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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".
> 
> I suspect that you can prefix "^^" unconditionally, just like --not
> can be emulated away by unconditionally prefixing "^".

That would be clever, but I think we only parse a single "^":

  $ git rev-list ^HEAD
  [no output]
  $ git rev-list ^^HEAD
  fatal: bad revision '^^HEAD'

  $ git rev-parse ^HEAD
  ^d486ca60a51c9cb1fe068803c3f540724e95e83a
  $ git rev-parse ^^HEAD
  ^^HEAD
  fatal: ambiguous argument '^^HEAD': unknown revision or path not in the working tree.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jul 12, 2021 at 11:47:23AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > 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".
>> 
>> I suspect that you can prefix "^^" unconditionally, just like --not
>> can be emulated away by unconditionally prefixing "^".
>
> That would be clever, but I think we only parse a single "^":
>
>   $ git rev-list ^HEAD
>   [no output]
>   $ git rev-list ^^HEAD
>   fatal: bad revision '^^HEAD'
>
>   $ git rev-parse ^HEAD
>   ^d486ca60a51c9cb1fe068803c3f540724e95e83a
>   $ git rev-parse ^^HEAD
>   ^^HEAD
>   fatal: ambiguous argument '^^HEAD': unknown revision or path not in the working tree.
>
> -Peff

;-)  Surprised.

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

[Jiang Xin]

Jeff King <peff@peff.net> 于2021年7月13日周二 上午1:54写道：
> On Sat, Jul 10, 2021 at 09:54:16PM +0000, brian m. carlson wrote:
> > > 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

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

[Jeff King]

On Tue, Jul 13, 2021 at 04:57:46PM +0800, Jiang Xin wrote:

> > 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

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2651 bytes --]

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:
> 
> > > 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]
> > 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]