ls-remote: inconsistency from the order of args and opts

It has a different results by executing "git ls-remote -heads origin" and "git ls-remote origin --heads". A test for reproduction is as follows:

@@ -0,0 +1,17 @@
+#!/bin/sh
+
+test_description='test ls-tree for reproduction'
+
+. ./test-lib.sh
+
+
+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 &&
+    test_cmp result.1 result.2

+'
+
+test_done
--
2.34.1.391.g9ef3d6f133

"git -C "test.git" ls-remote ./. --heads" returns nothing here because "ls-remote" supports to treat args as patterns to filter the refs list , and "--heads" here as the pattern does not match any.

If a repo exists a ref which is named as "refs/heads/--heads", execute mgit ls-remote origin --heads will return ("mgit" is a alias means the git binary that is built by "master" in my env):

       0319b78d7c31e5203d79157fe82960db88cc4a54        refs/heads/--heads

If we execute again in terms of this patch by git ls-remote origin --heads:

      0319b78d7c31e5203d79157fe82960db88cc4a54        refs/heads/--heads
      0319b78d7c31e5203d79157fe82960db88cc4a54        refs/heads/master

And if we hope to let "--heads" to be treated as a "pattern", we can execute git ls-remote origin --heads -- "--heads":

      0319b78d7c31e5203d79157fe82960db88cc4a54        refs/heads/--heads

Personally, I think the second option is probably a little better, but there are also possible backward compatibility issues, such as users ending branch names with "-heads" and so on.

So I sent this patch to try to figure out if this was a requirement of the original design, or if it was something that could really be improved, or if we can find a better way to deal this issue with compatibility.

Thanks.

Teng Long (1):
ls-remote: Make the output independent of the order of opts and
<remote>

 builtin/ls-remote.c  |  3 +--
 t/t5512-ls-remote.sh | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 2 deletions(-)

Range-diff against v0:
-: ---------- > 1: 9ef3d6f133 ls-remote: Make the output independent of the order of opts and <remote>

2.34.1.391.g9ef3d6f133

This commit is used to solve the problem of inconsistent execution results caused by switching the order of parameters and options in some scenarios. For example, if we execute the following commands:

$/opt/git/master/bin/git ls-remote .git

   $/opt/git/master/bin/git ls-remote --heads .git
   >3ea29ad061137e321853d64fb38b6c6e907546a0        refs/heads/master

   $/opt/git/master/bin/git ls-remote .git --heads
   >

Please omit the ">" because they are just to make the output more intuitive. The tests are based on 2ae0a9cb82 (The fifth batch, 2021-12-22). in master.

Similarily "--tags", "--refs" and "--symref" also produces the same result (no output print in this case).

The issue is caused by ls-remote command supports args as "patterns", which will filter the refs list from <remote> through the trailing matching rule. In these usages, Options can be differentiated and processed more accurately with "patterns", otherwise the result might lead to confusion if user has a loss of the context.

This commit sets "parse_opt_flags" value in "parse_options()" to 0, this will let the commands produce the same result in this scenario. At the same time, this commit does not break the current test cases in t5512.

---
 builtin/ls-remote.c  |  3 +--
 t/t5512-ls-remote.sh | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 2 deletions(-)

index 44448fa61d..45eeecdb71 100644
@@ -80,8 +80,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 
 	memset(&ref_array, 0, sizeof(ref_array));
 
-	argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
-			     PARSE_OPT_STOP_AT_NON_OPTION);
+	argc = parse_options(argc, argv, prefix, options, ls_remote_usage, 0);
 	dest = argv[0];
 
 	packet_trace_identity("ls-remote");

index f53f58895a..7c51cc23d1 100755
@@ -349,4 +349,28 @@ test_expect_success 'ls-remote prefixes work with all protocol versions' '
 	test_cmp expect actual.v2
 '
 
+test_expect_success 'ls-remote same result if exchange the order of --heads and <remote>' '
+	git ls-remote --heads self > expect.heads &&
+	git ls-remote self --heads > actual.heads &&
+	test_cmp expect.heads actual.heads
+'
+
+test_expect_success 'ls-remote same result if exchange the order of --tags and <remote>' '
+	git ls-remote --tags self > expect.tags &&
+	git ls-remote self --tags > actual.tags &&
+	test_cmp expect.tags actual.tags
+'
+
+test_expect_success 'ls-remote same result if exchange the order of --refs and <remote>' '
+	git ls-remote --refs self > expect.refs &&
+	git ls-remote self --refs > actual.refs &&
+	test_cmp expect.refs actual.refs
+'
+
+test_expect_success 'ls-remote same result if exchange the order of --symref and <remote>' '
+	git ls-remote --symref self > expect.symref &&
+	git ls-remote self --symref > actual.symref &&
+	test_cmp expect.symref actual.symref
+'
+
 test_done

2.34.1.391.g9ef3d6f133

I failed to say one important thing (I was again fooled by the "it is too obvious to say" that led "gitcli" not to mention this, too).

"dashed-options first and then args" rule means that generally we scan the command line from left to right one by one, and as long as the one we are looking at begins with "-", we try to interpret it as a dashed option, possibly eat the next one as an argument given to the option (e.g. "--abbrev 7"), and keep repeating, until the one we are at right now does not begin with "-". And then everything after that, we do not interpret it as an option. That is how "--heads" on the second example above, since we have seen . and took it as an argument (not a dashed option) is considered as a pattern.

It's much clearer now, thanks for the detailed answer.

Another question, if I want to follow your advice and add a short paragraph in git CLI document, should this patch continue in the current RFC patchset or launch a new patchset?

Thanks.

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.

Yes, but I consider that a bug that we cannot fix due to backward compatibility issues.

That is why my preference is to encourage users to stick to the POSIX way in gltcli, just like we recommend "stuck" form of options its parameter.

Yes, among the harm GNU has done on mankind, this is one of the biggest ones. We shouldn't waste our engineering time to support more of them in our tools.

As long as users stick to the recommended "dashed options first and then args, among which revs come first and then pathspecs", they will be fine.

This is only an approximate list, but:

    $ git grep -C3 'parse_options' -- 'builtin/*.c'|grep -c PARSE_OPT_STOP_AT_NON_OPTION
    16
    $ git grep -C3 'parse_options' -- 'builtin/*.c'|grep -c -F ', 0);'
    101

The GNU-like behavior is far more common in our codebase, and I think it's less surprising if commands work the same way for consistency.

I manually looked through the PARSE_OPT_STOP_AT_NON_OPTION cases, and I think this is the only one that's using it for no good reason. The others (e.g. "git config") would become ambiguous or error out as a result.

I find it quite useful. E.g. if you typo a command or forget/want to remove an option:

    git push origin HEAD --dry-run

You can just (under readline) do C-p M-DEL, instead of the equivalent navigating back a few words, or having to use more advanced readline features like ^--dry-run^^ or whatever.

Anecdotally, I've been surprised by the amount of regular terminal users whose readline skills pretty much and at using the arrow keys to make command corrections. I think this GNU UX decision has probably saved several accumulated man-lifetimes by now :)

Yes, POSIX specifies this is how it should work because it avoids ambiguity. According to POSIX, -v is a file, and that's a valid name on Unix. If GNU rm fails to delete that file or provide a diagnostic about why it didn't, that's a bug.

In some cases, we do allow the GNU behavior of providing options anywhere on the command line, but we don't when it causes ambiguity, like in this case. I think we should document the current behavior, but I also think it's a given when working on Unix because many tools don't work that way. For example, test and find don't permit arbitrary location of options and arguments and they are found on all Unix systems. You can't write "test foo -f".

And to prove that this is ambiguous, I provide you the following example:

$ git update-ref refs/heads/--symref HEAD
$ git ls-remote . --symref
1ffcbaa1a5f10c9f706314d77f88de20a4a498c2        refs/heads/--symref

That prints something very different if I write "git ls-remote --symref .". And it is actually the case that people write this kind of syntax in scripts relying on the current behavior and then those scripts get used in a variety of situations with arbitrary ref names, so this should continue to work this way. I believe a former employer may have these kinds of scripts, for example.

I'm not opposed to us building new tools which support the GNU behavior, but I don't think we should change tools where we have the existing behavior because it does lead to breakage in some scripting situations.

brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

Don't waste your time on this, and instead spend on something more useful. What I gave wrt gitcli.txt in an earlier message is final.

If I were you, I would retract the ls-remote topic and create a brand new topic that is about clarifying/enhancing the gitcli.txt file, which has nothing to do with ls-remote, because the command line option/argument convention is not specific to a single command.

It's not a bug that its default behavior isn't to slavishly follow POSIX, but if you'd like you can turn it on:

    $ touch -- file -v; rm -v file -v
    removed 'file'
    $ touch -- file -v; POSIXLY_CORRECT=1 rm -v file -v
    removed 'file'
    removed '-v'

There's a good reason for this departure in behavior: If you have a single file called '-v' POSIX doesn't provide any way to remove it other than something like:

    rm ./-v

Which can be painful when scripting things. I.e. you'll need to munge the name itself, v.s. consistently supporting "--":

    rm -- -v

Yes, *nix systems are all over the place with this. And then e.g. "dd" and the like accept arguments whose syntax pre-dates "-"-prefixed arguments.

I'm only talking about how our command collection should behave, and how we should explain its behavior, or what behavior we might prefer.

Yes, my claim that it's "not ambiguous" in <[email protected]> isn't correct, as you point out.

I suspect those cases are probably too obscure to worry about in practice, but yes, we might not want to convert any existing command due to those.

But we should really be recommending use of -- or --end-of-options whenever possible, as it can be hard to know in Git's CLI whether options after args are accepted or not.

Whatever we do with git-ls-remote (which I don't really care all that much about) gitcli should really be documenting how our tooling behaves.

Which is what I was mainly pointing out upthread, that your summary of options before other types of args omitted that many utilities support the reverse. Or perhaps you were only describing an asthetic choice (which I don't think is worth debating). I'm just talking about what the ground truth is.

What do you think about something like this to clear this up?:

index 92e4ba6a2fa..b1387c4fe68 100644
@@ -19,6 +19,13 @@ Many commands take revisions (most often "commits", but sometimes
 "tree-ish", depending on the context and command) and paths as their
 arguments.  Here are the rules:
 
+ * Options are (almost) universally accpted before other types of
+   arguments, e.g. `git cat-file -t HEAD` or `git push --dry-run
+   origin`, but in the case of those commands a GNU-style `git
+   cat-file HEAD -t` and `git push origin --dry-run` would work just
+   as well. The reverse is often not true, many commands do not accept
+   options after non-option arguments.
+
  * Revisions come first and then paths.
    E.g. in `git diff v1.0 v2.0 arch/x86 include/asm-x86`,
    `v1.0` and `v2.0` are revisions and `arch/x86` and `include/asm-x86`

Way before doing so, we should recommend the "options and then args" order. Users do not have to know which subcommands accepts options after args.

The ground truth is that it is unlikely that we can fix some of our commands so that they stop taking options that are given after args, because of inertia. But we can teach users especially the new ones to always use the canonical order to sidestep the whole "some subcommands imitate misguided GNUism to make it ambiguous, some don't" problem.

As we should aspire to fix the misguided "options can come still after we saw args" eventually (don't talk back on this point to waste any more of my time on a release day), I do not think it is a good idea to say "reverse is often not true" and stopping there.

It will mislead people to think these "not true" commands should somehow be updated to the GNUism in the future. It's the other way around.

If you want to script git to do e.g.:

touch -- foo -r
git add foo -r
git rm foo -r
git status foo -r
git log foo -r

You do need to know that those won't work, if that "foo -r" is from e.g. scripted arguments that those commands will accept or interpret that -r as an argument.

Hence suggesting that the user just add "--" to resolve the ambiguity, as gitcli already discusses.

Sadly, requiring "--end-of-options" is a solution for a problem that we didn't have to create. If we didn't take options written after arg,

git rm --end-of-options foo -r
git rm foo ./-r

to force "-r" to be interpreted as a filename wouldn't have been necessary. The presence of "foo" before "-r" would have been sufficient.

I agree with you that, unfortunately, we'd need to teach a way (i.e. "--" or "--end-of-options") to defeat this misguided GNUism in some commands. Even if users stick to "options and then args", sadly, they need to know it.

But we do already explain "--" and "--end-of-options" in gitcli.txt so we should be OK.

I really appreciate your comments and explanations.

Now I've cleared that up, personally. It's not a bad thing that there are some conventions for use, the point is whether the rules are clear to others. So, after that, I will try to modify a little in "git-cli.txt" with a new patchset.

Thanks.