{"thread":{"id":"51595","subject":"[PATCH 0/3] --end-of-options marker","startedAt":"2019-08-06T14:38:32Z","lastAt":"2019-08-08T10:28:59Z","messageCount":16,"participants":["Jeff King","Junio C Hamano","Randall S. Becker","SZEDER Gábor","brian m. carlson","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"380000","messageId":"20190806143829.GA515@sigill.intra.peff.net","threadId":"51595","inReplyTo":null,"subject":"[PATCH 0/3] --end-of-options marker","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-06T14:38:30Z","receivedAt":"2019-08-06T14:38:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's hard for scripted uses of rev-list, etc, to avoid option injection\nfrom untrusted arguments, because revision arguments must come before\nany \"--\" separator. I.e.:\n\n git rev-list \"$revision\" -- \"$path\"\n\nmight mistake \"$revision\" for an option (with rev-list, that would make\nit an error, but something like git-log would default to HEAD).\n\nThis series provides an alternative to \"--\" to stop option parsing\nwithout indicating that further arguments are pathspecs.\n\n [1/3]: revision: allow --end-of-options to end option parsing\n [2/3]: parse-options: allow --end-of-options as a synonym for \"--\"\n [3/3]: gitcli: document --end-of-options\n\n Documentation/gitcli.txt | 6 ++++++\n parse-options.c | 3 ++-\n revision.c | 8 +++++++-\n t/t0040-parse-options.sh | 7 +++++++\n t/t4202-log.sh | 7 +++++++\n t/t6000-rev-list-misc.sh | 8 ++++++++\n 6 files changed, 37 insertions(+), 2 deletions(-)\n\n"},{"id":"380001","messageId":"20190806143957.GA2161@sigill.intra.peff.net","threadId":"51595","inReplyTo":"20190806143829.GA515@sigill.intra.peff.net","subject":"[PATCH 1/3] revision: allow --end-of-options to end option parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-06T14:39:58Z","receivedAt":"2019-08-06T14:40:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's currently no robust way to tell Git that a particular option is\nmeant to be a revision, and not an option. So if you have a branch\n\"refs/heads/--foo\", you cannot just say:\n\n git rev-list --foo\n\nYou can say:\n\n git rev-list refs/heads/--foo\n\nBut that breaks down if you don't know the refname, and in particular if\nyou're a script passing along a value from elsewhere. In most programs,\nyou can use \"--\" to end option parsing, like this:\n\n some-prog -- \"$revision\"\n\nBut that doesn't work for the revision parser, because \"--\" is already\nmeaningful there: it separates revisions from pathspecs. So we need some\nother marker to separate options from revisions.\n\nThis patch introduces \"--end-of-options\", which serves that purpose:\n\n git rev-list --oneline --end-of-options \"$revision\"\n\nwill work regardless of what's in \"$revision\" (well, if you say \"--\" it\nmay fail, but it won't do something dangerous, like triggering an\nunexpected option).\n\nThe name is verbose, but that's probably a good thing; this is meant to\nbe used for scripted invocations where readability is more important\nthan terseness.\n\nOne alternative would be to introduce an explicit option to mark a\nrevision, like:\n\n git rev-list --oneline --revision=\"$revision\"\n\nThat's slightly _more_ informative than this commit (because it makes\neven something silly like \"--\" unambiguous). But the pattern of using a\nseparator like \"--\" is well established in git and in other commands,\nand it makes some scripting tasks simpler like:\n\n git rev-list --end-of-options \"$@\"\n\nThere's no documentation in this patch, because it will make sense to\ndescribe the feature once it is available everywhere (and support will\nbe added in further patches).\n\nSigned-off-by: Jeff King \n---\n revision.c | 8 +++++++-\n t/t6000-rev-list-misc.sh | 8 ++++++++\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex 07412297f0..51690e480d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2523,6 +2523,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \tint i, flags, left, seen_dashdash, got_rev_arg = 0, revarg_opt;\n \tstruct argv_array prune_data = ARGV_ARRAY_INIT;\n \tconst char *submodule = NULL;\n+\tint seen_end_of_options = 0;\n \n \tif (opt)\n \t\tsubmodule = opt->submodule;\n@@ -2552,7 +2553,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\trevarg_opt |= REVARG_CANNOT_BE_FILENAME;\n \tfor (left = i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n-\t\tif (*arg == '-') {\n+\t\tif (!seen_end_of_options && *arg == '-') {\n \t\t\tint opts;\n \n \t\t\topts = handle_revision_pseudo_opt(submodule,\n@@ -2574,6 +2575,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t\tcontinue;\n \t\t\t}\n \n+\t\t\tif (!strcmp(arg, \"--end-of-options\")) {\n+\t\t\t\tseen_end_of_options = 1;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\n \t\t\topts = handle_revision_opt(revs, argc - i, argv + i,\n \t\t\t\t\t\t &left, argv, opt);\n \t\t\tif (opts > 0) {\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex 52a9e38d66..b8cf82349b 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -140,4 +140,12 @@ test_expect_success '--header shows a NUL after each commit' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'rev-list --end-of-options' '\n+\tgit update-ref refs/heads/--output=yikes HEAD &&\n+\tgit rev-list --end-of-options --output=yikes >actual &&\n+\ttest_path_is_missing yikes &&\n+\tgit rev-list HEAD >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.23.0.rc1.436.g24d2e81391\n\n"},{"id":"380002","messageId":"20190806144015.GB2161@sigill.intra.peff.net","threadId":"51595","inReplyTo":"20190806143829.GA515@sigill.intra.peff.net","subject":"[PATCH 2/3] parse-options: allow --end-of-options as a synonym for \"--\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-06T14:40:16Z","receivedAt":"2019-08-06T14:40:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The revision option parser recently learned about --end-of-options, but\nthat's not quite enough for all callers. Some of them, like git-log,\npick out some options using parse_options(), and then feed the remainder\nto setup_revisions(). For those cases we need to stop parse_options()\nfrom finding more options when it sees --end-of-options, and to retain\nthat option in argv so that setup_revisions() can see it as well.\n\nLet's handle this the same as we do \"--\". We can even piggy-back on the\nhandling of PARSE_OPT_KEEP_DASHDASH, because any caller that wants to\nretain one will want to retain the other.\n\nI've included two tests here. The \"log\" test covers \"--source\", which is\none of the options it handles with parse_options(), and would fail\nbefore this patch. There's also a test that uses the parse-options\nhelper directly. That confirms that the option is handled correctly even\nin cases without KEEP_DASHDASH or setup_revisions(). I.e., it is safe to\nuse --end-of-options in place of \"--\" in other programs.\n\nSigned-off-by: Jeff King \n---\n parse-options.c | 3 ++-\n t/t0040-parse-options.sh | 7 +++++++\n t/t4202-log.sh | 7 +++++++\n 3 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 87b26a1d92..b42f54d48b 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -780,7 +780,8 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (!arg[2]) { /* \"--\" */\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\t\tctx->argc--;\n \t\t\t\tctx->argv++;\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex cebc77fab0..705a136ed9 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -399,4 +399,11 @@ test_expect_success 'GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS works' '\n \t\ttest-tool parse-options --ye\n '\n \n+test_expect_success '--end-of-options treats remainder as args' '\n+\ttest-tool parse-options \\\n+\t --expect=\"verbose: -1\" \\\n+\t --expect=\"arg 00: --verbose\" \\\n+\t --end-of-options --verbose\n+'\n+\n test_done\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex c20209324c..e88ccb04a9 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1707,4 +1707,11 @@ test_expect_success '--exclude-promisor-objects does not BUG-crash' '\n \ttest_must_fail git log --exclude-promisor-objects source-a\n '\n \n+test_expect_success 'log --end-of-options' '\n+ git update-ref refs/heads/--source HEAD &&\n+ git log --end-of-options --source >actual &&\n+ git log >expect &&\n+ test_cmp expect actual\n+'\n+\n test_done\n-- \n2.23.0.rc1.436.g24d2e81391\n\n"},{"id":"380003","messageId":"20190806144030.GC2161@sigill.intra.peff.net","threadId":"51595","inReplyTo":"20190806143829.GA515@sigill.intra.peff.net","subject":"[PATCH 3/3] gitcli: document --end-of-options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-06T14:40:30Z","receivedAt":"2019-08-06T14:40:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Now that --end-of-options is available for any users of\nsetup_revisions() or parse_options(), which should be effectively\neverywhere, we can guide people to use it for all their disambiguating\nneeds.\n\nSigned-off-by: Jeff King \n---\n Documentation/gitcli.txt | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt\nindex 1ed3ca33b7..4b32876b6e 100644\n--- a/Documentation/gitcli.txt\n+++ b/Documentation/gitcli.txt\n@@ -37,6 +37,12 @@ arguments. Here are the rules:\n file called HEAD in your work tree, `git diff HEAD` is ambiguous, and\n you have to say either `git diff HEAD --` or `git diff -- HEAD` to\n disambiguate.\n+\n+ * Because `--` disambiguates revisions and paths in some commands, it\n+ cannot be used for those commands to separate options and revisions.\n+ You can use `--end-of-options` for this (it also works for commands\n+ that do not distinguish between revisions in paths, in which case it\n+ is simply an alias for `--`).\n +\n When writing a script that is expected to handle random user-input, it is\n a good practice to make it explicit which arguments are which by placing\n-- \n2.23.0.rc1.436.g24d2e81391\n"},{"id":"380008","messageId":"xmqqa7cml0s9.fsf@gitster-ct.c.googlers.com","threadId":"51595","inReplyTo":"20190806143829.GA515@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] --end-of-options marker","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-06T16:24:38Z","receivedAt":"2019-08-06T16:24:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It's hard for scripted uses of rev-list, etc, to avoid option injection\n> from untrusted arguments, because revision arguments must come before\n> any \"--\" separator. I.e.:\n>\n> git rev-list \"$revision\" -- \"$path\"\n>\n> might mistake \"$revision\" for an option (with rev-list, that would make\n> it an error, but something like git-log would default to HEAD).\n\nJust to make sure I understand what I just read, let me paraphrase.\nWe would want to accept\n\n\tgit rev-list --max-parents=4 \\\n\t\t--end-of-options \\\n\t\t--count -- docs/\n\nso that '--count' would go thru the usual \"as we have -- later, it\nmust be a rev and we do not even disambiguate. What does get_sha1()\nsay it is?\" and \"docs/\" would be taken as a pathspec.\n\n\"git rev-list --max-parents=4 --count -- docs/\" would have treated\n\"--count\" as an option and would error out due to lack of any\nstarting revision.\n\nOn the other hand, \"git log --count -- docs/\" would take \"--count\"\nas an option, but does not complain about lack of any revs. It just\nstarts digging from HEAD and ends up ignoring the \"--count\" branch\n(or is this feature meant to support tags? As far as I recall, we\ndo not allow branch names that begin with a dash).\n\n> This series provides an alternative to \"--\" to stop option parsing\n> without indicating that further arguments are pathspecs.\n>\n> [1/3]: revision: allow --end-of-options to end option parsing\n> [2/3]: parse-options: allow --end-of-options as a synonym for \"--\"\n> [3/3]: gitcli: document --end-of-options\n>\n> Documentation/gitcli.txt | 6 ++++++\n> parse-options.c | 3 ++-\n> revision.c | 8 +++++++-\n> t/t0040-parse-options.sh | 7 +++++++\n> t/t4202-log.sh | 7 +++++++\n> t/t6000-rev-list-misc.sh | 8 ++++++++\n> 6 files changed, 37 insertions(+), 2 deletions(-)\n"},{"id":"380010","messageId":"000f01d54c75$1a8fe460$4fafad20$@nexbridge.com","threadId":"51595","inReplyTo":"xmqqa7cml0s9.fsf@gitster-ct.c.googlers.com","subject":"RE: [PATCH 0/3] --end-of-options marker","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2019-08-06T16:36:26Z","receivedAt":"2019-08-06T16:36:47Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On August 6, 2019 12:25 PM, Junio wrote:\n> Jeff King writes:\n> \n> > It's hard for scripted uses of rev-list, etc, to avoid option\n> > injection from untrusted arguments, because revision arguments must\n> > come before any \"--\" separator. I.e.:\n> >\n> > git rev-list \"$revision\" -- \"$path\"\n> >\n> > might mistake \"$revision\" for an option (with rev-list, that would\n> > make it an error, but something like git-log would default to HEAD).\n> \n> Just to make sure I understand what I just read, let me paraphrase.\n> We would want to accept\n> \n> \tgit rev-list --max-parents=4 \\\n> \t\t--end-of-options \\\n> \t\t--count -- docs/\n> \n> so that '--count' would go thru the usual \"as we have -- later, it must be\na rev\n> and we do not even disambiguate. What does get_sha1() say it is?\" and\n> \"docs/\" would be taken as a pathspec.\n> \"git rev-list --max-parents=4 --count -- docs/\" would have treated\n\"--count\"\n> as an option and would error out due to lack of any starting revision.\n> \n> On the other hand, \"git log --count -- docs/\" would take \"--count\"\n> as an option, but does not complain about lack of any revs. It just\nstarts\n> digging from HEAD and ends up ignoring the \"--count\" branch (or is this\n> feature meant to support tags? As far as I recall, we do not allow branch\n> names that begin with a dash).\n> \n> > This series provides an alternative to \"--\" to stop option parsing\n> > without indicating that further arguments are pathspecs.\n\nWould this offer the opportunity to, in the long term, supply options to\nexternal diff engines, for example?\n\nSomething like git diff --end-of-options --diff-opt1 --diff-opt2 -- a b\n\nI'm just noodling here, wondering why otherwise\n\ngit rev-list --max-parents=4 -- --count docs/\n\ndoes not work. I thought -- was pretty specific in terms of turning off\ninterpretation. So is it not a defect that --count is being interpreted?\n\nI have a fear for all my sub-teams who script with the assumption that --\nhas a specific meaning of stopping interpretation.\n\nSlightly confused,\nRandall\n\n> >\n> > [1/3]: revision: allow --end-of-options to end option parsing\n> > [2/3]: parse-options: allow --end-of-options as a synonym for \"--\"\n> > [3/3]: gitcli: document --end-of-options\n> >\n> > Documentation/gitcli.txt | 6 ++++++\n> > parse-options.c | 3 ++-\n> > revision.c | 8 +++++++-\n> > t/t0040-parse-options.sh | 7 +++++++\n> > t/t4202-log.sh | 7 +++++++\n> > t/t6000-rev-list-misc.sh | 8 ++++++++\n> > 6 files changed, 37 insertions(+), 2 deletions(-)\n\n"},{"id":"380021","messageId":"20190806173349.GA4961@sigill.intra.peff.net","threadId":"51595","inReplyTo":"xmqqa7cml0s9.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/3] --end-of-options marker","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-06T17:33:50Z","receivedAt":"2019-08-06T17:33:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 06, 2019 at 09:24:38AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > It's hard for scripted uses of rev-list, etc, to avoid option injection\n> > from untrusted arguments, because revision arguments must come before\n> > any \"--\" separator. I.e.:\n> >\n> > git rev-list \"$revision\" -- \"$path\"\n> >\n> > might mistake \"$revision\" for an option (with rev-list, that would make\n> > it an error, but something like git-log would default to HEAD).\n> \n> Just to make sure I understand what I just read, let me paraphrase.\n> We would want to accept\n> \n> \tgit rev-list --max-parents=4 \\\n> \t\t--end-of-options \\\n> \t\t--count -- docs/\n> \n> so that '--count' would go thru the usual \"as we have -- later, it\n> must be a rev and we do not even disambiguate. What does get_sha1()\n> say it is?\" and \"docs/\" would be taken as a pathspec.\n\nYes, that's how I'd expect that to be parsed.\n\n> \"git rev-list --max-parents=4 --count -- docs/\" would have treated\n> \"--count\" as an option and would error out due to lack of any\n> starting revision.\n\nRight. Though some options may have an impact even before rev-list would\ncomplain. For instance --output=foo (which is actually a diff option,\nbut revision.c handles both) opens and truncates \"foo\" before rev-list\nrealizes it doesn't have enough options.\n\n> On the other hand, \"git log --count -- docs/\" would take \"--count\"\n> as an option, but does not complain about lack of any revs. It just\n> starts digging from HEAD and ends up ignoring the \"--count\" branch\n\nCorrect.\n\n> (or is this feature meant to support tags? As far as I recall, we\n> do not allow branch names that begin with a dash).\n\nWe do forbid them via \"git branch\", but they are not forbidden by\ncheck-ref-format. So:\n\n git update-ref refs/heads/-foo $oid\n\nworks fine, and receive-pack is happy to accept it as a push. I think\nit might be reasonable to forbid that, but we'd have to accept potential\nfallouts.\n\nThat said, for the purposes of option injection, it actually doesn't\nmatter whether the ref exists or not (or if it's even an allowed name).\nThe problem is that the caller is feeding what it _thinks_ will be\ninterpreted as a rev name, but it isn't. So yes, most of the time\ntreating \"--count\" as a rev is nonsense and will fail. But the important\nthing is not so much that we do the right thing when you have a branch\n(or tag) with a funny name, but that we _don't_ do something dangerous\nor unexpected.\n\n-Peff\n"},{"id":"380022","messageId":"20190806173817.GB4961@sigill.intra.peff.net","threadId":"51595","inReplyTo":"000f01d54c75$1a8fe460$4fafad20$@nexbridge.com","subject":"Re: [PATCH 0/3] --end-of-options marker","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-06T17:38:17Z","receivedAt":"2019-08-06T17:38:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 06, 2019 at 12:36:26PM -0400, Randall S. Becker wrote:\n\n> > > This series provides an alternative to \"--\" to stop option parsing\n> > > without indicating that further arguments are pathspecs.\n> \n> Would this offer the opportunity to, in the long term, supply options to\n> external diff engines, for example?\n> \n> Something like git diff --end-of-options --diff-opt1 --diff-opt2 -- a b\n\nI'd expect that to interpret \"--diff-opt1\" and \"--diff-opt2\" as\nnon-option arguments, which in the context of git-diff means endpoints\nof the diff.\n\nSo no, I don't think you can use it like you're asking here.\n\n> I'm just noodling here, wondering why otherwise\n> \n> git rev-list --max-parents=4 -- --count docs/\n> \n> does not work. I thought -- was pretty specific in terms of turning off\n> interpretation. So is it not a defect that --count is being interpreted?\n\nThe command-line above means that \"--count\" is interpreted\n(unambiguously) as a path. The problem is that if you want it to be\ninterpreted as a starting point for traversal, then it must come\n_before_ the \"--\".\n\n> I have a fear for all my sub-teams who script with the assumption that --\n> has a specific meaning of stopping interpretation.\n\nNothing about \"--\" is changed by my series; it will still stop option\ninterpretation in rev-list and in other commands. But as before,\nrev-list (and other Git commands that use the revision.c parser) use it\nto separate revisions and pathspecs. That's unlike how most other\nprograms use \"--\", but that ship sailed for Git in 2005.\n\n-Peff\n"},{"id":"380032","messageId":"001401d54c80$991cbd20$cb563760$@nexbridge.com","threadId":"51595","inReplyTo":"20190806173817.GB4961@sigill.intra.peff.net","subject":"RE: [PATCH 0/3] --end-of-options marker","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2019-08-06T17:58:43Z","receivedAt":"2019-08-06T17:58:56Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On August 6, 2019 1:38 PM, Jeff King wrote:\n> To: Randall S. Becker \n> Cc: 'Junio C Hamano' ; git@vger.kernel.org\n> Subject: Re: [PATCH 0/3] --end-of-options marker\n> \n> On Tue, Aug 06, 2019 at 12:36:26PM -0400, Randall S. Becker wrote:\n> \n> > > > This series provides an alternative to \"--\" to stop option parsing\n> > > > without indicating that further arguments are pathspecs.\n> >\n> > Would this offer the opportunity to, in the long term, supply options\n> > to external diff engines, for example?\n> >\n> > Something like git diff --end-of-options --diff-opt1 --diff-opt2 -- a\n> > b\n> \n> I'd expect that to interpret \"--diff-opt1\" and \"--diff-opt2\" as non-option\n> arguments, which in the context of git-diff means endpoints of the diff.\n> \n> So no, I don't think you can use it like you're asking here.\n> \n> > I'm just noodling here, wondering why otherwise\n> >\n> > git rev-list --max-parents=4 -- --count docs/\n> >\n> > does not work. I thought -- was pretty specific in terms of turning\n> > off interpretation. So is it not a defect that --count is being interpreted?\n> \n> The command-line above means that \"--count\" is interpreted\n> (unambiguously) as a path. The problem is that if you want it to be\n> interpreted as a starting point for traversal, then it must come _before_ the\n> \"--\".\n> \n> > I have a fear for all my sub-teams who script with the assumption that\n> > -- has a specific meaning of stopping interpretation.\n> \n> Nothing about \"--\" is changed by my series; it will still stop option\n> interpretation in rev-list and in other commands. But as before, rev-list (and\n> other Git commands that use the revision.c parser) use it to separate\n> revisions and pathspecs. That's unlike how most other programs use \"--\", but\n> that ship sailed for Git in 2005.\n\nThanks for the explanation.\n\nRandall\n\n"},{"id":"380035","messageId":"20190806181459.GG20404@szeder.dev","threadId":"51595","inReplyTo":"20190806173817.GB4961@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] --end-of-options marker","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-06T18:14:59Z","receivedAt":"2019-08-06T18:15:05Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Aug 06, 2019 at 01:38:17PM -0400, Jeff King wrote:\n> Nothing about \"--\" is changed by my series; it will still stop option\n> interpretation in rev-list and in other commands. But as before,\n> rev-list (and other Git commands that use the revision.c parser) use it\n> to separate revisions and pathspecs. That's unlike how most other\n> programs use \"--\", but that ship sailed for Git in 2005.\n\nI'd like to draw attention to the oddball 'git filter-branch' command,\nwhich uses '--' as a separator between 'filter-branch' and 'rev-list'\noptions. Will it still work with this new option? I think it will,\nbut not sure.\n\n"},{"id":"380049","messageId":"20190806225853.GH118825@genre.crustytoothpaste.net","threadId":"51595","inReplyTo":"20190806143829.GA515@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] --end-of-options marker","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-06T22:58:53Z","receivedAt":"2019-08-06T22:59:01Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2019-08-06 at 14:38:30, Jeff King wrote:\n> It's hard for scripted uses of rev-list, etc, to avoid option injection\n> from untrusted arguments, because revision arguments must come before\n> any \"--\" separator. I.e.:\n> \n> git rev-list \"$revision\" -- \"$path\"\n> \n> might mistake \"$revision\" for an option (with rev-list, that would make\n> it an error, but something like git-log would default to HEAD).\n> \n> This series provides an alternative to \"--\" to stop option parsing\n> without indicating that further arguments are pathspecs.\n\nSorry, I hadn't had a chance to look at this series in depth, but I was\nwondering: could we not just accept two separate \"--\" arguments, and if\nthere are two of them, interpret the first with the traditional meaning\nand the second with the Git-specific meaning? That would be much more\nintuitive for folks, although I suspect it would take a little more work\nin the options parser.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"380053","messageId":"20190806234319.GA19178@sigill.intra.peff.net","threadId":"51595","inReplyTo":"20190806225853.GH118825@genre.crustytoothpaste.net","subject":"Re: [PATCH 0/3] --end-of-options marker","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-06T23:43:20Z","receivedAt":"2019-08-06T23:43:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 06, 2019 at 10:58:53PM +0000, brian m. carlson wrote:\n\n> On 2019-08-06 at 14:38:30, Jeff King wrote:\n> > It's hard for scripted uses of rev-list, etc, to avoid option injection\n> > from untrusted arguments, because revision arguments must come before\n> > any \"--\" separator. I.e.:\n> > \n> > git rev-list \"$revision\" -- \"$path\"\n> > \n> > might mistake \"$revision\" for an option (with rev-list, that would make\n> > it an error, but something like git-log would default to HEAD).\n> > \n> > This series provides an alternative to \"--\" to stop option parsing\n> > without indicating that further arguments are pathspecs.\n> \n> Sorry, I hadn't had a chance to look at this series in depth, but I was\n> wondering: could we not just accept two separate \"--\" arguments, and if\n> there are two of them, interpret the first with the traditional meaning\n> and the second with the Git-specific meaning? That would be much more\n> intuitive for folks, although I suspect it would take a little more work\n> in the options parser.\n\nThat also crossed my mind, but I think it opens up some complicated\ncorner cases. For instance, if I'm parsing left-to-right and see \"--\",\nhow do I know which separator it is meant to be? I think the only rule\nthat makes sense is that you must have two \"--\", like:\n\n git rev-list [options] -- [revs] -- [paths]\n\nbut that means parsing the whole thing before we can interpret any of\nit. What kinds of tricks can an attacker play by putting \"--\" in the\nrevs or paths areas? E.g., what does this mean:\n\n # expanded from \"git rev-list -- $revs -- $paths\"\n git rev-list -- --foo -- -- --bar --\n\nI think if we at least choose the left-most \"--\" as the official\nend-of-options then they can't inject an option (they can only inject a\nrev as a path). I guess that's the same as with --end-of-options. But it\nsomehow feels less clear to me than a separate marker.\n\nIt also doesn't allow this:\n\n # allow paths and revs, with optional separator, but no more options\n git rev-list --end-of-options \"$@\"\n\nthough I'm not sure whether anybody cares.\n\n-Peff\n"},{"id":"380055","messageId":"20190807041749.GI118825@genre.crustytoothpaste.net","threadId":"51595","inReplyTo":"20190806234319.GA19178@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] --end-of-options marker","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-07T04:17:49Z","receivedAt":"2019-08-07T04:17:58Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2019-08-06 at 23:43:20, Jeff King wrote:\n> On Tue, Aug 06, 2019 at 10:58:53PM +0000, brian m. carlson wrote:\n> > Sorry, I hadn't had a chance to look at this series in depth, but I was\n> > wondering: could we not just accept two separate \"--\" arguments, and if\n> > there are two of them, interpret the first with the traditional meaning\n> > and the second with the Git-specific meaning? That would be much more\n> > intuitive for folks, although I suspect it would take a little more work\n> > in the options parser.\n> \n> That also crossed my mind, but I think it opens up some complicated\n> corner cases. For instance, if I'm parsing left-to-right and see \"--\",\n> how do I know which separator it is meant to be? I think the only rule\n> that makes sense is that you must have two \"--\", like:\n> \n> git rev-list [options] -- [revs] -- [paths]\n\nI was assuming that we wouldn't have a huge number of command-line\narguments and we'd check ahead, although that could of course cause some\npain when used with xargs, I suppose, especially on Linux with its huge\nARG_MAX.\n\n> but that means parsing the whole thing before we can interpret any of\n> it. What kinds of tricks can an attacker play by putting \"--\" in the\n> revs or paths areas? E.g., what does this mean:\n> \n> # expanded from \"git rev-list -- $revs -- $paths\"\n> git rev-list -- --foo -- -- --bar --\n> \n> I think if we at least choose the left-most \"--\" as the official\n> end-of-options then they can't inject an option (they can only inject a\n> rev as a path). I guess that's the same as with --end-of-options. But it\n> somehow feels less clear to me than a separate marker.\n\nI suppose if there's more than two, then interpret the first one as the\nend-of-options marker, the second one in the traditional way, and any\nsubsequent ones as pathspecs matching the file \"--\". Writing such a\ncommand line would be silly, but we'd fail secure.\n\n> It also doesn't allow this:\n> \n> # allow paths and revs, with optional separator, but no more options\n> git rev-list --end-of-options \"$@\"\n> \n> though I'm not sure whether anybody cares.\n\nThat's a good point. I don't have a strong view either way, but I\nthought I'd ask about alternatives.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"380076","messageId":"20190807165431.GA60876@syl.local","threadId":"51595","inReplyTo":"20190807041749.GI118825@genre.crustytoothpaste.net","subject":"Re: [PATCH 0/3] --end-of-options marker","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-08-07T16:54:31Z","receivedAt":"2019-08-07T16:54:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Aug 07, 2019 at 04:17:49AM +0000, brian m. carlson wrote:\n> On 2019-08-06 at 23:43:20, Jeff King wrote:\n> > On Tue, Aug 06, 2019 at 10:58:53PM +0000, brian m. carlson wrote:\n> > > Sorry, I hadn't had a chance to look at this series in depth, but I was\n> > > wondering: could we not just accept two separate \"--\" arguments, and if\n> > > there are two of them, interpret the first with the traditional meaning\n> > > and the second with the Git-specific meaning? That would be much more\n> > > intuitive for folks, although I suspect it would take a little more work\n> > > in the options parser.\n> >\n> > That also crossed my mind, but I think it opens up some complicated\n> > corner cases. For instance, if I'm parsing left-to-right and see \"--\",\n> > how do I know which separator it is meant to be? I think the only rule\n> > that makes sense is that you must have two \"--\", like:\n> >\n> > git rev-list [options] -- [revs] -- [paths]\n>\n> I was assuming that we wouldn't have a huge number of command-line\n> arguments and we'd check ahead, although that could of course cause some\n> pain when used with xargs, I suppose, especially on Linux with its huge\n> ARG_MAX.\n>\n> > but that means parsing the whole thing before we can interpret any of\n> > it. What kinds of tricks can an attacker play by putting \"--\" in the\n> > revs or paths areas? E.g., what does this mean:\n> >\n> > # expanded from \"git rev-list -- $revs -- $paths\"\n> > git rev-list -- --foo -- -- --bar --\n> >\n> > I think if we at least choose the left-most \"--\" as the official\n> > end-of-options then they can't inject an option (they can only inject a\n> > rev as a path). I guess that's the same as with --end-of-options. But it\n> > somehow feels less clear to me than a separate marker.\n\nThis is definitely the secure option among the two, but I'm not sure\nthat it makes me feel better about this alternative direction. I dislike\nthe ambiguity in having two '--'s, and I don't think that this is\nsomething we ought to concern callers with.\n\n'--end-of-options' is on the one hand, cumbersome to write, but it is\nclear. I think that this is an acceptable trade-off, because we don't\nexpect users at the command line to ever type this. So, some extra\nclarity in favor of a drop in convenience for a supposedly smaller\nnumber of use cases seems like a favorable trade-off to me.\n\n> I suppose if there's more than two, then interpret the first one as the\n> end-of-options marker, the second one in the traditional way, and any\n> subsequent ones as pathspecs matching the file \"--\". Writing such a\n> command line would be silly, but we'd fail secure.\n>\n> > It also doesn't allow this:\n> >\n> > # allow paths and revs, with optional separator, but no more options\n> > git rev-list --end-of-options \"$@\"\n> >\n> > though I'm not sure whether anybody cares.\n>\n> That's a good point. I don't have a strong view either way, but I\n> thought I'd ask about alternatives.\n> --\n> brian m. carlson: Houston, Texas, US\n> OpenPGP: https://keybase.io/bk2204\n\nThanks,\nTaylor\n"},{"id":"380113","messageId":"20190808100318.GC12257@sigill.intra.peff.net","threadId":"51595","inReplyTo":"20190806181459.GG20404@szeder.dev","subject":"Re: [PATCH 0/3] --end-of-options marker","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-08T10:03:18Z","receivedAt":"2019-08-08T10:03:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 06, 2019 at 08:14:59PM +0200, SZEDER Gábor wrote:\n\n> On Tue, Aug 06, 2019 at 01:38:17PM -0400, Jeff King wrote:\n> > Nothing about \"--\" is changed by my series; it will still stop option\n> > interpretation in rev-list and in other commands. But as before,\n> > rev-list (and other Git commands that use the revision.c parser) use it\n> > to separate revisions and pathspecs. That's unlike how most other\n> > programs use \"--\", but that ship sailed for Git in 2005.\n> \n> I'd like to draw attention to the oddball 'git filter-branch' command,\n> which uses '--' as a separator between 'filter-branch' and 'rev-list'\n> options. Will it still work with this new option? I think it will,\n> but not sure.\n\nGood question.\n\nCertainly \"--\" will work as it did before, since the code here only\nchanges behavior when it sees --end-of-options.\n\nfilter-branch doesn't use any of our parseopt infrastructure itself, so\nit won't understand the new option itself. I.e., this won't work[1]:\n\n git filter-branch --end-of-options -this-is-a-branch-name\n\nBut since it passes rev-list options as-is, this does successfully pass\nthe name to rev-list:\n\n git filter-branch -- --end-of-options -this-is-a-branch-name\n\nHowever, filter-branch itself seems to do some magic of its own with the\nrev-list options, and will try to filter HEAD in that case. So I think\nit needs further work to cover all cases correctly.\n\n-Peff\n\n[1] I think the first one there would work if filter-branch actually\n used \"rev-parse --parseopt\". E.g.:\n\n git rev-parse --parseopt -- --foo --end-of-options --bar <<-\\EOF\n cmd\n --\n foo an option\n bar another option\n EOF\n\n yields:\n\n set -- --foo -- '--bar'\n"},{"id":"380114","messageId":"20190808102857.GD12257@sigill.intra.peff.net","threadId":"51595","inReplyTo":"20190807041749.GI118825@genre.crustytoothpaste.net","subject":"Re: [PATCH 0/3] --end-of-options marker","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-08T10:28:57Z","receivedAt":"2019-08-08T10:28:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 07, 2019 at 04:17:49AM +0000, brian m. carlson wrote:\n\n> > I think if we at least choose the left-most \"--\" as the official\n> > end-of-options then they can't inject an option (they can only inject a\n> > rev as a path). I guess that's the same as with --end-of-options. But it\n> > somehow feels less clear to me than a separate marker.\n> \n> I suppose if there's more than two, then interpret the first one as the\n> end-of-options marker, the second one in the traditional way, and any\n> subsequent ones as pathspecs matching the file \"--\". Writing such a\n> command line would be silly, but we'd fail secure.\n\nYeah, I think that could work. I'd be a little concerned that the\nimplementation would end up complicated and confusing, just because\nthere are other parts of the code that treat \"--\" specially. That's not\na necessarily a reason to avoid it if there's a compelling reason, but I\nthink I favor a unique marker anyway (or at least am otherwise\nambivalent).\n\n> That's a good point. I don't have a strong view either way, but I\n> thought I'd ask about alternatives.\n\nDiscussion of alternatives is very welcome.\n\nI think the most compelling alternative is the one I pointed out in one\nof the commit messages:\n\n git rev-list --revision=\n\nwhich lets normal left-to-right parsing work without any complex\nreasoning. It is harder to use with \"$@\", though.\n\nRelated, my proposal doesn't do anything for rev-parse. I think that:\n\n git rev-parse --end-of-options -xyz\n\nshould probably return:\n\n --end-of-options\n \n\nbut I mostly consider that kind of use of rev-parse (pretending to be an\noptions parser for rev-list) to be vestigial. The main use of rev-parse\n(in my experience) is \"rev-parse --verify\" to resolve a single name.\n\nThere are still some gaps there. For instance:\n\n git rev-parse --verify --foo\n\nwill treat \"--foo\" as an option (and then complain that there was no rev\nargument). I don't think you can do anything too mischievous from this,\nbut it might be nice to tighten it up. I'm tempted to say that\n\"--verify\" should complain if there isn't exactly one argument, but\ntechnically things like this do work:\n\n git rev-parse --verify --sq \"$rev\"\n\n git rev-parse --verify --symbolic-full-name \"$rev\"\n\nI don't know if anybody cares or not. We could perhaps work around it by\nhaving --verify treat the final argument as a non-option, even if it\nstarts with \"-\". That would allow those cases, but:\n\n git rev-parse --verify --symbolic-full-name\n\nwould treat the latter as an argument (and currently that's always an\nerror anyway). Looking at rev-parse, there are other weird bits to\n--verify, too. E.g., this:\n\n git rev-parse --verify a...b c\n\nshows a...b, ignoring that --verify was given, and then eventually \"c\".\n\n-Peff\n"}]}