--end-of-options inconsistently available?!

--end-of-options inconsistently available?!

[Sven Strickroth]

Hi,

gitcli(7) states:
> Because -- disambiguates revisions and paths in some commands, it cannot be used for those commands to separate options and revisions. You can use --end-of-options for this (it also works for commands that do not distinguish between revisions in paths, in which case it is simply an alias for --).

However, when I use this for certain commands it fails:

$ git reset --end-of-options HEAD --
fatal: option '--end-of-options' must come before non-option arguments

$ git rev-parse --symbolic-full-name --end-of-options master
--end-of-options
refs/heads/master

Here, the output also contains "--end-of-options" as if it is a 
reference (same for "--")

$ git checkout -f --end-of-options HEAD~1 -- afile.txt
fatal: only one reference expected, 2 given.

Best,
  Sven

Re: --end-of-options inconsistently available?!

[Jeff King]

On Mon, Nov 27, 2023 at 12:22:44PM +0100, Sven Strickroth wrote:

> Hi,
> 
> gitcli(7) states:
> > Because -- disambiguates revisions and paths in some commands, it cannot be used for those commands to separate options and revisions. You can use --end-of-options for this (it also works for commands that do not distinguish between revisions in paths, in which case it is simply an alias for --).
> 
> However, when I use this for certain commands it fails:
> 
> $ git reset --end-of-options HEAD --
> fatal: option '--end-of-options' must come before non-option arguments

This one seems like a bug. Handling of --end-of-options usually happens
via the parse_options() API. But in this case, cmd_reset() calls it with
PARSE_OPT_KEEP_DASHDASH, which retains the --end-of-options marker. But
then the caller is not ready to deal with that string being left in
argv[0] (it is OK with "--", but not anything else).

So at first glance, it feels like parse-options should avoid leaving it
in place, like:

diff --git a/parse-options.c b/parse-options.c
index e0c94b0546..5c07ad47ec 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -931,7 +931,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
 
 		if (!arg[2] /* "--" */ ||
 		    !strcmp(arg + 2, "end-of-options")) {
-			if (!(ctx->flags & PARSE_OPT_KEEP_DASHDASH)) {
+			if (arg[2] || !(ctx->flags & PARSE_OPT_KEEP_DASHDASH)) {
 				ctx->argc--;
 				ctx->argv++;
 			}

But I think that confuses other callers. For example, t4202 fails
because we try (with a ref called refs/heads/--source) to run:

  git log --end-of-options --source

expecting it it to be resolved as a ref. With the patch above, it gets
confused. So I think we may need to teach KEEP_DASHDASH callers to
handle end-of-options themselves. In the case of git-log, it is done by
the revision machinery, but reset doesn't use that.

So something like this works:

diff --git a/builtin/reset.c b/builtin/reset.c
index 4b018d20e3..a0d801179a 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -259,6 +259,9 @@ static void parse_args(struct pathspec *pathspec,
 	 * At this point, argv points immediately after [-opts].
 	 */
 
+	if (argv[0] && !strcmp(argv[0], "--end-of-options"))
+		argv++;
+
 	if (argv[0]) {
 		if (!strcmp(argv[0], "--")) {
 			argv++; /* reset to HEAD, possibly with paths */

but it feels like a maintenance problem that we'd have to audit every
caller that uses KEEP_DASHDASH.

On the other hand, I do think the callers need to be a bit aware of the
issue to make things work seamlessly. In particular, this now does what
you'd expect:

  git reset --end-of-options foo -- bar

But if we do this:

  git reset --end-of-options --foo

it works if "--foo" can be resolved, but otherwise complains "option
'--foo' must come before non-option arguments", even if it exists as a
file! IOW, the do-what-I-mean handling of "--" is too picky; in
verify_filename() it complains about things that look like options, not
realizing we already made sure to avoid those.

OTOH that is also true of "git log --end-of-options --foo". And maybe
not that big a deal in practice. If you are truly being careful you'd
always do:

  git log --end-of-options --foo -- --bar

anyway, which is unambiguous.

So I dunno. I'm not sure there's a central fix, and we may have to just
fix this spot and look for others.

> $ git rev-parse --symbolic-full-name --end-of-options master
> --end-of-options
> refs/heads/master
> 
> Here, the output also contains "--end-of-options" as if it is a reference
> (same for "--")

This one is intentional. rev-parse in its default mode is not just
spitting out revisions, but also options that are meant to be passed
along to the revision machinery via other commands (like rev-list). So
for example:

  $ git rev-parse --foo HEAD
  --foo
  564d0252ca632e0264ed670534a51d18a689ef5d

And it does understand end-of-options explicitly, so:

  $ git rev-parse --end-of-options --foo --
  --end-of-options
  fatal: bad revision '--foo'

If you just want to parse a name robustly, use --verify.

> $ git checkout -f --end-of-options HEAD~1 -- afile.txt
> fatal: only one reference expected, 2 given.

I think this is the same KEEP_DASHDASH problem as with git-reset.

-Peff

Re: --end-of-options inconsistently available?!

[Sven Strickroth]

Am 27.11.2023 um 22:22 schrieb Jeff King:
>> $ git rev-parse --symbolic-full-name --end-of-options master
>> --end-of-options
>> refs/heads/master
>>
>> Here, the output also contains "--end-of-options" as if it is a reference
>> (same for "--")
> 
> This one is intentional. rev-parse in its default mode is not just
> spitting out revisions, but also options that are meant to be passed
> along to the revision machinery via other commands (like rev-list). So
> for example:
> 
>    $ git rev-parse --foo HEAD
>    --foo
>    564d0252ca632e0264ed670534a51d18a689ef5d
> 
> And it does understand end-of-options explicitly, so:
> 
>    $ git rev-parse --end-of-options --foo --
>    --end-of-options
>    fatal: bad revision '--foo'
> 
> If you just want to parse a name robustly, use --verify.

I would expect that -- and --end-of-options are handled in a special way 
here so that rev-parse can also be used in scripts. I need to check 
whether --verify works for me (from the manual I thought I need to 
specify full reference names).

>> $ git checkout -f --end-of-options HEAD~1 -- afile.txt
>> fatal: only one reference expected, 2 given.
> 
> I think this is the same KEEP_DASHDASH problem as with git-reset.

I also found another problem:
$ git format-patch --end-of-options -1
fatal: option '-1' must come before non-option arguments

Where -1 is the number of commits here...

Best,
  Sven

Re: --end-of-options inconsistently available?!

[Jeff King]

On Tue, Nov 28, 2023 at 09:40:08AM +0100, Sven Strickroth wrote:

> > This one is intentional. rev-parse in its default mode is not just
> > spitting out revisions, but also options that are meant to be passed
> > along to the revision machinery via other commands (like rev-list). So
> > for example:
> > 
> >    $ git rev-parse --foo HEAD
> >    --foo
> >    564d0252ca632e0264ed670534a51d18a689ef5d
> > 
> > And it does understand end-of-options explicitly, so:
> > 
> >    $ git rev-parse --end-of-options --foo --
> >    --end-of-options
> >    fatal: bad revision '--foo'
> > 
> > If you just want to parse a name robustly, use --verify.
> 
> I would expect that -- and --end-of-options are handled in a special way
> here so that rev-parse can also be used in scripts. I need to check whether
> --verify works for me (from the manual I thought I need to specify full
> reference names).

They _are_ handled specially, and for the purpose of using rev-parse in
scripts. It's just that in its default mode it does not do what you
want, because it has another purpose.

> > > $ git checkout -f --end-of-options HEAD~1 -- afile.txt
> > > fatal: only one reference expected, 2 given.
> > 
> > I think this is the same KEEP_DASHDASH problem as with git-reset.
> 
> I also found another problem:
> $ git format-patch --end-of-options -1
> fatal: option '-1' must come before non-option arguments
> 
> Where -1 is the number of commits here...

This is the same as the "log --end-of-options --foo" example I showed
earlier. That "-1" cannot mean "use 1 commit", since you used it after
--end-of-options. It will correctly resolve refs/heads/-1 if you have
such a ref. But if you don't, then the DWIM logic for distinguishing
revisions and pathspecs produces a confusing message.

It would be possible to fix that (by telling verify_filename() that we
saw --end-of-options, and not to treat dashes specially). But in
practice if you are bothering to use --end-of-options, you really ought
to be using "--" as well, like:

  git format-patch --end-of-options $revs -- $paths

in which case it will know that "-1" _must_ be a revision, and complain:

  $ git format-patch --end-of-options -1 --
  fatal: bad revision '-1'

-Peff

[PATCH] parse-options: decouple "--end-of-options" and "--"

[Jeff King]

On Mon, Nov 27, 2023 at 04:22:54PM -0500, Jeff King wrote:

> So something like this works:
> 
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 4b018d20e3..a0d801179a 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -259,6 +259,9 @@ static void parse_args(struct pathspec *pathspec,
>  	 * At this point, argv points immediately after [-opts].
>  	 */
>  
> +	if (argv[0] && !strcmp(argv[0], "--end-of-options"))
> +		argv++;
> +
>  	if (argv[0]) {
>  		if (!strcmp(argv[0], "--")) {
>  			argv++; /* reset to HEAD, possibly with paths */
> 
> but it feels like a maintenance problem that we'd have to audit every
> caller that uses KEEP_DASHDASH.

So here's my attempt at a central fix. There is a downside (see the
discussion below), but I think in practice it should be a strict
improvement for most commands. I won't be too surprised if we find a
counter-example, but even if we do, my gut feeling is that we should fix
that command on top of this, rather than give up and require every
command to be aware of --end-of-options.

-- >8 --
Subject: [PATCH] parse-options: decouple "--end-of-options" and "--"

When we added generic end-of-options support in 51b4594b40
(parse-options: allow --end-of-options as a synonym for "--",
2019-08-06), we made them true synonyms. They both stop option parsing,
and they are both returned in the resulting argv if the KEEP_DASHDASH
flag is used.

The hope was that this would work for all callers:

  - most generic callers would not pass KEEP_DASHDASH, and so would just
    do the right thing (stop parsing there) without needing to know
    anything more.

  - callers with KEEP_DASHDASH were generally going to rely on
    setup_revisions(), which knew to handle --end-of-options specially

But that turned out miss quite a few cases that pass KEEP_DASHDASH but
do their own manual parsing. For example, "git reset", "git checkout",
and so on want pass KEEP_DASHDASH so they can support:

  git reset $revs -- $paths

but of course aren't going to actually do a traversal, so they don't
call setup_revisions(). And those cases currently get confused by
--end-of-options being left in place, like:

   $ git reset --end-of-options HEAD
   fatal: option '--end-of-options' must come before non-option arguments

We could teach each of these callers to handle the leftover option
explicitly. But let's try to be a bit more clever and see if we can
solve it centrally in parse-options.c.

The bogus assumption here is that KEEP_DASHDASH tells us the caller
wants to see --end-of-options in the result. But really, the callers
which need to know that --end-of-options was reached are those that may
potentially parse more options from argv. In other words, those that
pass the KEEP_UNKNOWN_OPT flag.

If such a caller is aware of --end-of-options (e.g., because they call
setup_revisions() with the result), then this will continue to do the
right thing, treating anything after --end-of-options as a non-option.

And if the caller is not aware of --end-of-options, they are better off
keeping it intact, because either:

  1. They are just passing the options along to somebody else anyway, in
     which case that somebody would need to know about the
     --end-of-options marker.

  2. They are going to parse the remainder themselves, at which point
     choking on --end-of-options is much better than having it silently
     removed. The point is to avoid option injection from untrusted
     command line arguments, and bailing is better than quietly treating
     the untrusted argument as an option.

This fixes bugs with --end-of-options across several commands, but I've
focused on two in particular here:

  - t7102 confirms that "git reset --end-of-options --foo" now works.
    This checks two things. One, that we no longer barf on
    "--end-of-options" itself (which previously we did, even if the rev
    was something vanilla like "HEAD" instead of "--foo"). And two, that
    we correctly treat "--foo" as a revision rather than an option.

    This fix applies to any other cases which pass KEEP_DASHDASH but not
    KEEP_UNKNOWN_OPT, like "git checkout", "git check-attr", "git grep",
    etc, which would previously choke on "--end-of-options".

  - t9350 shows the opposite case: fast-export passed KEEP_UNKNOWN_OPT
    but not KEEP_DASHDASH, but then passed the result on to
    setup_revisions(). So it never saw --end-of-options, and would
    erroneously parse "fast-export --end-of-options --foo" as having a
    "--foo" option. This is now fixed.

Note that this does shut the door for callers which want to know if we
hit end-of-options, but don't otherwise need to keep unknown opts. The
obvious thing here is feeding it to the DWIM verify_filename()
machinery. And indeed, this is a problem even for commands which do
understand --end-of-options already. For example, without this patch,
you get:

  $ git log --end-of-options --foo
  fatal: option '--foo' must come before non-option arguments

because we refuse to accept "--foo" as a filename (because it starts
with a dash) even though we could know that we saw end-of-options. The
verify_filename() function simply doesn't accept this extra information.

So that is the status quo, and this patch doubles down further on that.
Commands like "git reset" have the same problem, but they won't even
know that parse-options saw --end-of-options! So even if we fixed
verify_filename(), they wouldn't have anything to pass to it.

But in practice I don't think this is a big deal. If you are being
careful enough to use --end-of-options, then you should also be using
"--" to disambiguate and avoid the DWIM behavior in the first place. In
other words, doing:

  git log --end-of-options --this-is-a-rev -- --this-is-a-path

works correctly, and will continue to do so. And likewise, with this
patch now:

  git reset --end-of-options --this-is-a-rev -- --this-is-a-path

will work, as well.

Signed-off-by: Jeff King <peff@peff.net>
---
 parse-options.c        |  9 +++++++--
 t/t7102-reset.sh       |  8 ++++++++
 t/t9350-fast-export.sh | 10 ++++++++++
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index e0c94b0546..d50962062e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -929,13 +929,18 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
 			continue;
 		}
 
-		if (!arg[2] /* "--" */ ||
-		    !strcmp(arg + 2, "end-of-options")) {
+		if (!arg[2] /* "--" */) {
 			if (!(ctx->flags & PARSE_OPT_KEEP_DASHDASH)) {
 				ctx->argc--;
 				ctx->argv++;
 			}
 			break;
+		} else if (!strcmp(arg + 2, "end-of-options")) {
+			if (!(ctx->flags & PARSE_OPT_KEEP_UNKNOWN_OPT)) {
+				ctx->argc--;
+				ctx->argv++;
+			}
+			break;
 		}
 
 		if (internal_help && !strcmp(arg + 2, "help-all"))
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 4287863ae6..62d9f846ce 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -616,4 +616,12 @@ test_expect_success 'reset --mixed sets up work tree' '
 	test_must_be_empty actual
 '
 
+test_expect_success 'reset handles --end-of-options' '
+	git update-ref refs/heads/--foo HEAD^ &&
+	git log -1 --format=%s refs/heads/--foo >expect &&
+	git reset --hard --end-of-options --foo &&
+	git log -1 --format=%s HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 26c25c0eb2..e9a12c18bb 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -791,4 +791,14 @@ test_expect_success 'fast-export --first-parent outputs all revisions output by
 	)
 '
 
+test_expect_success 'fast-export handles --end-of-options' '
+	git update-ref refs/heads/nodash HEAD &&
+	git update-ref refs/heads/--dashes HEAD &&
+	git fast-export --end-of-options nodash >expect &&
+	git fast-export --end-of-options --dashes >actual.raw &&
+	# fix up lines which mention the ref for comparison
+	sed s/--dashes/nodash/ <actual.raw >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.43.0.664.ga12c899002