Re: [RFC PATCH v1 0/1] ls-remote: inconsistency from the order of args and opts

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jan 14, 2022, 19:57 UTC
Message-ID: <220114.867db2rs0n.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqfspqeun5.fsf@gitster.g>

On Thu, Jan 13 2022, Junio C Hamano wrote:

Show 27 quoted lines

> Teng Long <dyroneteng@gmail.com> writes:
>
>> +test_must_fail 'Exchange the order of "--heads" and <remote>' '
>> +    git --version &&
>> +    git init "test.git" &&
>> +    test_commit -C "test.git" one &&
>> +    git -C "test.git" ls-remote --heads ./. > result.1 &&
>> +    git -C "test.git" ls-remote ./. --heads > result.2 &&
>
> I would say that this is working exactly as designed.  As with the
> unix tradition, after the command name, first come options
> (e.g. "--heads", "-v", etc. that begin with a dash or two dashes),
> then arguments like "origin", "master", "." that are not dashed
> options/flags.
>
> Then among the arguments, we generally take revs first and then
> pathspecs.  "git help cli" explicitly mentions this, because it is
> specific to "git" command suite, but it does not mention "dashed
> options/flags first and then args", primarily because, at least back
> when the documentation was written, this was taken as granted, iow,
> those who wrote the "gitcli" documentation thought it was a common
> knowledge among users that did not need to be spelled out.
>
> Apparently, it is not a common knowledge at least for you (and
> probably others).  Perhaps we should add a paragraph to the cli help
> and explicitly mention "options first and then args", before we go
> on to say "among args, revs first and then pathspecs".

I don't think this summary is accurate.

We have multiple commands that are in GNU-fashion loose about whether you provide options first before no-option args, or after. E.g. we accept both of:

    git push --dry-run <remote> <ref>

And:

    git push <remote> <ref> --dry-run

The "tradition" you're referring to accurately summarizes how POSIX specifies that things should work.

But when GNU came around its option parser was generally happy to accept options and args in either order. E.g. these both work with GNU coreutils, but the latter will fail on FreeBSD and various other capital-U UNIX-es:

    touch foo; rm -v foo
    touch foo; rm foo -v

The relevant documentation being https://www.gnu.org/prep/standards/standards.html#Standards-for-Command-Line-Interfaces; specifically:

    "Note that the GNU version of getopt will normally permit options
    anywhere among the arguments unless the special argument ‘--’ is
    used. This is not what POSIX specifies; it is a GNU extension."

Now, as to git's behavior we mostly follow the GNU style. The default behavior of parse_options() is to accept the looser GNU style. In this case we're explicitly passing the PARSE_OPT_STOP_AT_NON_OPTION flag.

But why is that?

Most things that do that are doing so because we're parsing things in two passes, i.e. for commands that have sub-commands, e.g.:

    git commit-graph [opts] [subcommand] [subcommand-opts]

In those cases we use PARSE_OPT_STOP_AT_NON_OPTION on our first pass, so that we'll stop at "subcommand", then we'll generally continue the parsing as of "[subcommand]" without that flag.

The general exception to that rule, as you note, being things that take pathspecs, although for some of those we'll apply other custom disambiguation.

In this case a look at ba5f28bf79e (ls-remote: use parse-options api, 2016-01-19) will show that it was faithfully converted from an ad-hoc custom option parser using the "BSD-like" option parsing discussed above.

I really don't see why we wouldn't take this patch given that the command has one parse_options() invocation (so no subcommand complexity), and doesn't accept pathspecs etc.

We should aim to make the command line UX consistent when possible, not slavishly bug-for-bug follow whatever the behavior of the pre-parse_options() code was.

We can't do that in some cases, but this case seems like a pretty clear case where simply removing the PARSE_OPT_STOP_AT_NON_OPTION flag would be an improvement. It's not ambiguous, and makes invocations that currently (and inconsistently v.s. our usual behavior) error out work.