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

[Jeff King <peff@peff.net>]

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