{"thread":{"id":"60563","subject":"--end-of-options inconsistently available?!","startedAt":"2023-11-27T11:29:50Z","lastAt":"2023-12-06T22:21:46Z","messageCount":5,"participants":["Sven Strickroth","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"485166","messageId":"4d944fe3-d31d-4859-8ed2-6c1da64410fe@cs-ware.de","threadId":"60563","inReplyTo":null,"subject":"--end-of-options inconsistently available?!","fromName":"Sven Strickroth","fromEmail":"sven@cs-ware.de","sentAt":"2023-11-27T11:22:44Z","receivedAt":"2023-11-27T11:29:50Z","isPatch":false,"sender":{"key":"sven@cs-ware.de","avatar":null},"body":"Hi,\n\ngitcli(7) states:\n> 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 --).\n\nHowever, when I use this for certain commands it fails:\n\n$ git reset --end-of-options HEAD --\nfatal: option '--end-of-options' must come before non-option arguments\n\n$ git rev-parse --symbolic-full-name --end-of-options master\n--end-of-options\nrefs/heads/master\n\nHere, the output also contains \"--end-of-options\" as if it is a \nreference (same for \"--\")\n\n$ git checkout -f --end-of-options HEAD~1 -- afile.txt\nfatal: only one reference expected, 2 given.\n\nBest,\n  Sven\n"},{"id":"485179","messageId":"20231127212254.GA87495@coredump.intra.peff.net","threadId":"60563","inReplyTo":"4d944fe3-d31d-4859-8ed2-6c1da64410fe@cs-ware.de","subject":"Re: --end-of-options inconsistently available?!","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-27T21:22:54Z","receivedAt":"2023-11-27T21:22:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 27, 2023 at 12:22:44PM +0100, Sven Strickroth wrote:\n\n> Hi,\n> \n> gitcli(7) states:\n> > 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 --).\n> \n> However, when I use this for certain commands it fails:\n> \n> $ git reset --end-of-options HEAD --\n> fatal: option '--end-of-options' must come before non-option arguments\n\nThis one seems like a bug. Handling of --end-of-options usually happens\nvia the parse_options() API. But in this case, cmd_reset() calls it with\nPARSE_OPT_KEEP_DASHDASH, which retains the --end-of-options marker. But\nthen the caller is not ready to deal with that string being left in\nargv[0] (it is OK with \"--\", but not anything else).\n\nSo at first glance, it feels like parse-options should avoid leaving it\nin place, like:\n\ndiff --git a/parse-options.c b/parse-options.c\nindex e0c94b0546..5c07ad47ec 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -931,7 +931,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n \n \t\tif (!arg[2] /* \"--\" */ ||\n \t\t    !strcmp(arg + 2, \"end-of-options\")) {\n-\t\t\tif (!(ctx->flags & PARSE_OPT_KEEP_DASHDASH)) {\n+\t\t\tif (arg[2] || !(ctx->flags & PARSE_OPT_KEEP_DASHDASH)) {\n \t\t\t\tctx->argc--;\n \t\t\t\tctx->argv++;\n \t\t\t}\n\nBut I think that confuses other callers. For example, t4202 fails\nbecause we try (with a ref called refs/heads/--source) to run:\n\n  git log --end-of-options --source\n\nexpecting it it to be resolved as a ref. With the patch above, it gets\nconfused. So I think we may need to teach KEEP_DASHDASH callers to\nhandle end-of-options themselves. In the case of git-log, it is done by\nthe revision machinery, but reset doesn't use that.\n\nSo something like this works:\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 4b018d20e3..a0d801179a 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -259,6 +259,9 @@ static void parse_args(struct pathspec *pathspec,\n \t * At this point, argv points immediately after [-opts].\n \t */\n \n+\tif (argv[0] && !strcmp(argv[0], \"--end-of-options\"))\n+\t\targv++;\n+\n \tif (argv[0]) {\n \t\tif (!strcmp(argv[0], \"--\")) {\n \t\t\targv++; /* reset to HEAD, possibly with paths */\n\nbut it feels like a maintenance problem that we'd have to audit every\ncaller that uses KEEP_DASHDASH.\n\nOn the other hand, I do think the callers need to be a bit aware of the\nissue to make things work seamlessly. In particular, this now does what\nyou'd expect:\n\n  git reset --end-of-options foo -- bar\n\nBut if we do this:\n\n  git reset --end-of-options --foo\n\nit works if \"--foo\" can be resolved, but otherwise complains \"option\n'--foo' must come before non-option arguments\", even if it exists as a\nfile! IOW, the do-what-I-mean handling of \"--\" is too picky; in\nverify_filename() it complains about things that look like options, not\nrealizing we already made sure to avoid those.\n\nOTOH that is also true of \"git log --end-of-options --foo\". And maybe\nnot that big a deal in practice. If you are truly being careful you'd\nalways do:\n\n  git log --end-of-options --foo -- --bar\n\nanyway, which is unambiguous.\n\nSo I dunno. I'm not sure there's a central fix, and we may have to just\nfix this spot and look for others.\n\n> $ git rev-parse --symbolic-full-name --end-of-options master\n> --end-of-options\n> refs/heads/master\n> \n> Here, the output also contains \"--end-of-options\" as if it is a reference\n> (same for \"--\")\n\nThis one is intentional. rev-parse in its default mode is not just\nspitting out revisions, but also options that are meant to be passed\nalong to the revision machinery via other commands (like rev-list). So\nfor example:\n\n  $ git rev-parse --foo HEAD\n  --foo\n  564d0252ca632e0264ed670534a51d18a689ef5d\n\nAnd it does understand end-of-options explicitly, so:\n\n  $ git rev-parse --end-of-options --foo --\n  --end-of-options\n  fatal: bad revision '--foo'\n\nIf you just want to parse a name robustly, use --verify.\n\n> $ git checkout -f --end-of-options HEAD~1 -- afile.txt\n> fatal: only one reference expected, 2 given.\n\nI think this is the same KEEP_DASHDASH problem as with git-reset.\n\n-Peff\n"},{"id":"485190","messageId":"ab14260c-d515-425e-8ef6-5739d3d6ca4e@cs-ware.de","threadId":"60563","inReplyTo":"20231127212254.GA87495@coredump.intra.peff.net","subject":"Re: --end-of-options inconsistently available?!","fromName":"Sven Strickroth","fromEmail":"sven@cs-ware.de","sentAt":"2023-11-28T08:40:08Z","receivedAt":"2023-11-28T08:40:13Z","isPatch":false,"sender":{"key":"sven@cs-ware.de","avatar":null},"body":"Am 27.11.2023 um 22:22 schrieb Jeff King:\n>> $ git rev-parse --symbolic-full-name --end-of-options master\n>> --end-of-options\n>> refs/heads/master\n>>\n>> Here, the output also contains \"--end-of-options\" as if it is a reference\n>> (same for \"--\")\n> \n> This one is intentional. rev-parse in its default mode is not just\n> spitting out revisions, but also options that are meant to be passed\n> along to the revision machinery via other commands (like rev-list). So\n> for example:\n> \n>    $ git rev-parse --foo HEAD\n>    --foo\n>    564d0252ca632e0264ed670534a51d18a689ef5d\n> \n> And it does understand end-of-options explicitly, so:\n> \n>    $ git rev-parse --end-of-options --foo --\n>    --end-of-options\n>    fatal: bad revision '--foo'\n> \n> If you just want to parse a name robustly, use --verify.\n\nI would expect that -- and --end-of-options are handled in a special way \nhere so that rev-parse can also be used in scripts. I need to check \nwhether --verify works for me (from the manual I thought I need to \nspecify full reference names).\n\n>> $ git checkout -f --end-of-options HEAD~1 -- afile.txt\n>> fatal: only one reference expected, 2 given.\n> \n> I think this is the same KEEP_DASHDASH problem as with git-reset.\n\nI also found another problem:\n$ git format-patch --end-of-options -1\nfatal: option '-1' must come before non-option arguments\n\nWhere -1 is the number of commits here...\n\nBest,\n  Sven\n\n"},{"id":"485399","messageId":"20231206211639.GB106480@coredump.intra.peff.net","threadId":"60563","inReplyTo":"ab14260c-d515-425e-8ef6-5739d3d6ca4e@cs-ware.de","subject":"Re: --end-of-options inconsistently available?!","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-06T21:16:39Z","receivedAt":"2023-12-06T21:16:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 28, 2023 at 09:40:08AM +0100, Sven Strickroth wrote:\n\n> > This one is intentional. rev-parse in its default mode is not just\n> > spitting out revisions, but also options that are meant to be passed\n> > along to the revision machinery via other commands (like rev-list). So\n> > for example:\n> > \n> >    $ git rev-parse --foo HEAD\n> >    --foo\n> >    564d0252ca632e0264ed670534a51d18a689ef5d\n> > \n> > And it does understand end-of-options explicitly, so:\n> > \n> >    $ git rev-parse --end-of-options --foo --\n> >    --end-of-options\n> >    fatal: bad revision '--foo'\n> > \n> > If you just want to parse a name robustly, use --verify.\n> \n> I would expect that -- and --end-of-options are handled in a special way\n> here so that rev-parse can also be used in scripts. I need to check whether\n> --verify works for me (from the manual I thought I need to specify full\n> reference names).\n\nThey _are_ handled specially, and for the purpose of using rev-parse in\nscripts. It's just that in its default mode it does not do what you\nwant, because it has another purpose.\n\n> > > $ git checkout -f --end-of-options HEAD~1 -- afile.txt\n> > > fatal: only one reference expected, 2 given.\n> > \n> > I think this is the same KEEP_DASHDASH problem as with git-reset.\n> \n> I also found another problem:\n> $ git format-patch --end-of-options -1\n> fatal: option '-1' must come before non-option arguments\n> \n> Where -1 is the number of commits here...\n\nThis is the same as the \"log --end-of-options --foo\" example I showed\nearlier. That \"-1\" cannot mean \"use 1 commit\", since you used it after\n--end-of-options. It will correctly resolve refs/heads/-1 if you have\nsuch a ref. But if you don't, then the DWIM logic for distinguishing\nrevisions and pathspecs produces a confusing message.\n\nIt would be possible to fix that (by telling verify_filename() that we\nsaw --end-of-options, and not to treat dashes specially). But in\npractice if you are bothering to use --end-of-options, you really ought\nto be using \"--\" as well, like:\n\n  git format-patch --end-of-options $revs -- $paths\n\nin which case it will know that \"-1\" _must_ be a revision, and complain:\n\n  $ git format-patch --end-of-options -1 --\n  fatal: bad revision '-1'\n\n-Peff\n"},{"id":"485400","messageId":"20231206222145.GA136253@coredump.intra.peff.net","threadId":"60563","inReplyTo":"20231127212254.GA87495@coredump.intra.peff.net","subject":"[PATCH] parse-options: decouple \"--end-of-options\" and \"--\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-06T22:21:45Z","receivedAt":"2023-12-06T22:21:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 27, 2023 at 04:22:54PM -0500, Jeff King wrote:\n\n> So something like this works:\n> \n> diff --git a/builtin/reset.c b/builtin/reset.c\n> index 4b018d20e3..a0d801179a 100644\n> --- a/builtin/reset.c\n> +++ b/builtin/reset.c\n> @@ -259,6 +259,9 @@ static void parse_args(struct pathspec *pathspec,\n>  \t * At this point, argv points immediately after [-opts].\n>  \t */\n>  \n> +\tif (argv[0] && !strcmp(argv[0], \"--end-of-options\"))\n> +\t\targv++;\n> +\n>  \tif (argv[0]) {\n>  \t\tif (!strcmp(argv[0], \"--\")) {\n>  \t\t\targv++; /* reset to HEAD, possibly with paths */\n> \n> but it feels like a maintenance problem that we'd have to audit every\n> caller that uses KEEP_DASHDASH.\n\nSo here's my attempt at a central fix. There is a downside (see the\ndiscussion below), but I think in practice it should be a strict\nimprovement for most commands. I won't be too surprised if we find a\ncounter-example, but even if we do, my gut feeling is that we should fix\nthat command on top of this, rather than give up and require every\ncommand to be aware of --end-of-options.\n\n-- >8 --\nSubject: [PATCH] parse-options: decouple \"--end-of-options\" and \"--\"\n\nWhen we added generic end-of-options support in 51b4594b40\n(parse-options: allow --end-of-options as a synonym for \"--\",\n2019-08-06), we made them true synonyms. They both stop option parsing,\nand they are both returned in the resulting argv if the KEEP_DASHDASH\nflag is used.\n\nThe hope was that this would work for all callers:\n\n  - most generic callers would not pass KEEP_DASHDASH, and so would just\n    do the right thing (stop parsing there) without needing to know\n    anything more.\n\n  - callers with KEEP_DASHDASH were generally going to rely on\n    setup_revisions(), which knew to handle --end-of-options specially\n\nBut that turned out miss quite a few cases that pass KEEP_DASHDASH but\ndo their own manual parsing. For example, \"git reset\", \"git checkout\",\nand so on want pass KEEP_DASHDASH so they can support:\n\n  git reset $revs -- $paths\n\nbut of course aren't going to actually do a traversal, so they don't\ncall setup_revisions(). And those cases currently get confused by\n--end-of-options being left in place, like:\n\n   $ git reset --end-of-options HEAD\n   fatal: option '--end-of-options' must come before non-option arguments\n\nWe could teach each of these callers to handle the leftover option\nexplicitly. But let's try to be a bit more clever and see if we can\nsolve it centrally in parse-options.c.\n\nThe bogus assumption here is that KEEP_DASHDASH tells us the caller\nwants to see --end-of-options in the result. But really, the callers\nwhich need to know that --end-of-options was reached are those that may\npotentially parse more options from argv. In other words, those that\npass the KEEP_UNKNOWN_OPT flag.\n\nIf such a caller is aware of --end-of-options (e.g., because they call\nsetup_revisions() with the result), then this will continue to do the\nright thing, treating anything after --end-of-options as a non-option.\n\nAnd if the caller is not aware of --end-of-options, they are better off\nkeeping it intact, because either:\n\n  1. They are just passing the options along to somebody else anyway, in\n     which case that somebody would need to know about the\n     --end-of-options marker.\n\n  2. They are going to parse the remainder themselves, at which point\n     choking on --end-of-options is much better than having it silently\n     removed. The point is to avoid option injection from untrusted\n     command line arguments, and bailing is better than quietly treating\n     the untrusted argument as an option.\n\nThis fixes bugs with --end-of-options across several commands, but I've\nfocused on two in particular here:\n\n  - t7102 confirms that \"git reset --end-of-options --foo\" now works.\n    This checks two things. One, that we no longer barf on\n    \"--end-of-options\" itself (which previously we did, even if the rev\n    was something vanilla like \"HEAD\" instead of \"--foo\"). And two, that\n    we correctly treat \"--foo\" as a revision rather than an option.\n\n    This fix applies to any other cases which pass KEEP_DASHDASH but not\n    KEEP_UNKNOWN_OPT, like \"git checkout\", \"git check-attr\", \"git grep\",\n    etc, which would previously choke on \"--end-of-options\".\n\n  - t9350 shows the opposite case: fast-export passed KEEP_UNKNOWN_OPT\n    but not KEEP_DASHDASH, but then passed the result on to\n    setup_revisions(). So it never saw --end-of-options, and would\n    erroneously parse \"fast-export --end-of-options --foo\" as having a\n    \"--foo\" option. This is now fixed.\n\nNote that this does shut the door for callers which want to know if we\nhit end-of-options, but don't otherwise need to keep unknown opts. The\nobvious thing here is feeding it to the DWIM verify_filename()\nmachinery. And indeed, this is a problem even for commands which do\nunderstand --end-of-options already. For example, without this patch,\nyou get:\n\n  $ git log --end-of-options --foo\n  fatal: option '--foo' must come before non-option arguments\n\nbecause we refuse to accept \"--foo\" as a filename (because it starts\nwith a dash) even though we could know that we saw end-of-options. The\nverify_filename() function simply doesn't accept this extra information.\n\nSo that is the status quo, and this patch doubles down further on that.\nCommands like \"git reset\" have the same problem, but they won't even\nknow that parse-options saw --end-of-options! So even if we fixed\nverify_filename(), they wouldn't have anything to pass to it.\n\nBut in practice I don't think this is a big deal. If you are being\ncareful enough to use --end-of-options, then you should also be using\n\"--\" to disambiguate and avoid the DWIM behavior in the first place. In\nother words, doing:\n\n  git log --end-of-options --this-is-a-rev -- --this-is-a-path\n\nworks correctly, and will continue to do so. And likewise, with this\npatch now:\n\n  git reset --end-of-options --this-is-a-rev -- --this-is-a-path\n\nwill work, as well.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n parse-options.c        |  9 +++++++--\n t/t7102-reset.sh       |  8 ++++++++\n t/t9350-fast-export.sh | 10 ++++++++++\n 3 files changed, 25 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex e0c94b0546..d50962062e 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -929,13 +929,18 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (!arg[2] /* \"--\" */ ||\n-\t\t    !strcmp(arg + 2, \"end-of-options\")) {\n+\t\tif (!arg[2] /* \"--\" */) {\n \t\t\tif (!(ctx->flags & PARSE_OPT_KEEP_DASHDASH)) {\n \t\t\t\tctx->argc--;\n \t\t\t\tctx->argv++;\n \t\t\t}\n \t\t\tbreak;\n+\t\t} else if (!strcmp(arg + 2, \"end-of-options\")) {\n+\t\t\tif (!(ctx->flags & PARSE_OPT_KEEP_UNKNOWN_OPT)) {\n+\t\t\t\tctx->argc--;\n+\t\t\t\tctx->argv++;\n+\t\t\t}\n+\t\t\tbreak;\n \t\t}\n \n \t\tif (internal_help && !strcmp(arg + 2, \"help-all\"))\ndiff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\nindex 4287863ae6..62d9f846ce 100755\n--- a/t/t7102-reset.sh\n+++ b/t/t7102-reset.sh\n@@ -616,4 +616,12 @@ test_expect_success 'reset --mixed sets up work tree' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'reset handles --end-of-options' '\n+\tgit update-ref refs/heads/--foo HEAD^ &&\n+\tgit log -1 --format=%s refs/heads/--foo >expect &&\n+\tgit reset --hard --end-of-options --foo &&\n+\tgit log -1 --format=%s HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex 26c25c0eb2..e9a12c18bb 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -791,4 +791,14 @@ test_expect_success 'fast-export --first-parent outputs all revisions output by\n \t)\n '\n \n+test_expect_success 'fast-export handles --end-of-options' '\n+\tgit update-ref refs/heads/nodash HEAD &&\n+\tgit update-ref refs/heads/--dashes HEAD &&\n+\tgit fast-export --end-of-options nodash >expect &&\n+\tgit fast-export --end-of-options --dashes >actual.raw &&\n+\t# fix up lines which mention the ref for comparison\n+\tsed s/--dashes/nodash/ <actual.raw >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.43.0.664.ga12c899002\n\n"}]}