{"thread":{"id":"54613","subject":"check_refname_format allows refs with components that begin with -, even though `git tag` does not","startedAt":"2020-11-10T19:32:16Z","lastAt":"2020-11-11T02:22:41Z","messageCount":10,"participants":["Demi M. Obenour","Junio C Hamano","Ævar Arnfjörð Bjarmason","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"409508","messageId":"c926193b-a328-7562-6d4b-1ab2765c8cca@invisiblethingslab.com","threadId":"54613","inReplyTo":null,"subject":"check_refname_format allows refs with components that begin with -, even though `git tag` does not","fromName":"Demi M. Obenour","fromEmail":"athena@invisiblethingslab.com","sentAt":"2020-11-10T19:32:04Z","receivedAt":"2020-11-10T19:32:16Z","isPatch":false,"sender":{"key":"athena@invisiblethingslab.com","avatar":null},"body":"If I try to create a Git tag with a name beginning with `-`,\nGit complains.  However, Git does not check that a repository does\nnot have tags containing `-`.  This almost led to a vulnerability\nin the QubesOS `verify-git-tag` script.  Fortunately, this was not\nexploitable, as neither `git tag -v`, `git verify-tag --raw`, nor\n`git describe` have options that are useful to an attacker.\n\nSince this could cause vulnerabilities in other programs, I initially\nreported it as an embargoed security bug, but was told to post it\npublicly.\n\nThe best idea I had for a fix is to print names beginning with `-`\nusing the fully-qualified form, such as \"refs/tags/-a\".  Also, `--`\nis used as a delimiter in many commands, and can’t be escaped,\nso disallowing it might be a good idea.\n\nIn the long run, I hope to see leading dashes banned entirely, but\nbackwards compatibility might prevent that.\n\nSincerely,\n\nDemi\n"},{"id":"409516","messageId":"xmqqd00l6kj0.fsf@gitster.c.googlers.com","threadId":"54613","inReplyTo":"c926193b-a328-7562-6d4b-1ab2765c8cca@invisiblethingslab.com","subject":"Re: check_refname_format allows refs with components that begin with -, even though `git tag` does not","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-10T20:09:55Z","receivedAt":"2020-11-10T20:10:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Demi M. Obenour\" <athena@invisiblethingslab.com> writes:\n\n> If I try to create a Git tag with a name beginning with `-`,\n> Git complains.  However, Git does not check that a repository does\n> not have tags containing `-`.\n\nThis is quite deliberate.  The command line parser of \"git checkout\"\nand friends long lacked way to say \"switch to THAT BRANCH whose name\nbegins with a hyphen\" etc., and preventing tags and branches whose\nname begins with a hyphen from created at the Porcelain level was a\nway to stop users from hurting themselves.  \n\nThese funny names are supported at the plumbing level primarily\nbecause we have historically allowed them and suddenly forbidding\ntheir use would break existing repository.  A secondary reason is to\nhave a way to learn the current value of and then remove them, so\npeople with these funnily named branches and tags can \"rename\" them.\n\n> This almost led to a vulnerability in the QubesOS `verify-git-tag`\n> script.\n\nScripts need to be careful about their inputs, period.\n\n> The best idea I had for a fix is to print names beginning with `-`\n> using the fully-qualified form, such as \"refs/tags/-a\".  Also, `--`\n> is used as a delimiter in many commands, and can’t be escaped,\n> so disallowing it might be a good idea.\n\nI do not think there is anything to fix.\n\nCommand line parsers of some commands may have to learn how to\ndisambiguate such a strangely named tags and branches, though.  Some\ncommands do not know --end-of-options convention, for example.\n\nThanks.\n\n\n[Further reading]\n\nhttps://lore.kernel.org/git/7v62pjo4km.fsf@alter.siamese.dyndns.org/\nhttps://lore.kernel.org/git/7vsk262vla.fsf@alter.siamese.dyndns.org/\n"},{"id":"409549","messageId":"87imachp6x.fsf@evledraar.gmail.com","threadId":"54613","inReplyTo":"c926193b-a328-7562-6d4b-1ab2765c8cca@invisiblethingslab.com","subject":"Re: check_refname_format allows refs with components that begin with -, even though `git tag` does not","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-11-10T21:33:42Z","receivedAt":"2020-11-10T21:33:48Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 10 2020, Demi M. Obenour wrote:\n\n> If I try to create a Git tag with a name beginning with `-`,\n> Git complains.  However, Git does not check that a repository does\n> not have tags containing `-`.  This almost led to a vulnerability\n> in the QubesOS `verify-git-tag` script.  Fortunately, this was not\n> exploitable, as neither `git tag -v`, `git verify-tag --raw`, nor\n> `git describe` have options that are useful to an attacker.\n>\n> Since this could cause vulnerabilities in other programs, I initially\n> reported it as an embargoed security bug, but was told to post it\n> publicly.\n>\n> The best idea I had for a fix is to print names beginning with `-`\n> using the fully-qualified form, such as \"refs/tags/-a\".  Also, `--`\n> is used as a delimiter in many commands, and can’t be escaped,\n> so disallowing it might be a good idea.\n>\n> In the long run, I hope to see leading dashes banned entirely, but\n> backwards compatibility might prevent that.\n\n[Re-posted from the very brief discussion on the security list]\n\nI'm inclined to say that we might as well remove the porcelain\nlimitation of these \"-\" tags and branches. In reality we support them,\nfsck doesn't even warn about them, and git-check-ref-format(1) doesn't\ndocument them as forbidden.\n\nFrom reading 63486240108 (\"disallow branch names that start with a\nhyphen\", 2010-09-14) and 4f0accd638b (\"tag: disallow '-' as tag name\",\n2011-05-10) I don't see why we'd need to forbid these. A WIP patch of\nremoving the checks from strbuf_check_branch_ref() and\nstrbuf_check_tag_ref() fails just one test.\n\nThe reason we forbade these seems to have been mainly to avoid the UI\nconfusion of \"git checkout -\" or \"git checkout -foo\" and \"git checkout\n-- -foo\" not working. We can just make the \"-\" a special-case, and since\nthen we've had \"git switch -- -foo\" which works, and you can do \"git\ncheckout refs/heads/-foo\".\n\nI'm probably missing some porcelain bits but with my trivial patch \"git\n[opts] [tag|branch] -- -foo\" works.\n\nSo I think we should just allow these, since at best it just leads to a\nfalse sense of security.\n\nFWIW this is the very basic WIP patch I was playing with. Fails just one\ntest that explicitly tests for a branch named \"-naster\" with\ncheck-ref-format, should probably still disallow refs named just \"-\" (or\nmaybe not...).\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex ecf011776dc..86088838bb4 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -369,9 +369,6 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n \n static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)\n {\n-\tif (name[0] == '-')\n-\t\treturn -1;\n-\n \tstrbuf_reset(sb);\n \tstrbuf_addf(sb, \"refs/tags/%s\", name);\n \ndiff --git a/sha1-name.c b/sha1-name.c\nindex 0b23b86ceb4..9a01138336c 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -1592,7 +1592,7 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n \t */\n \tstrbuf_splice(sb, 0, 0, \"refs/heads/\", 11);\n \n-\tif (*name == '-' ||\n+\tif (!strcmp(name, \"-\") ||\n \t    !strcmp(sb->buf, \"refs/heads/HEAD\"))\n \t\treturn -1;\n \n\n"},{"id":"409550","messageId":"20201110213544.GA3263091@coredump.intra.peff.net","threadId":"54613","inReplyTo":"xmqqd00l6kj0.fsf@gitster.c.googlers.com","subject":"Re: check_refname_format allows refs with components that begin with -, even though `git tag` does not","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-10T21:35:44Z","receivedAt":"2020-11-10T21:35:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 10, 2020 at 12:09:55PM -0800, Junio C Hamano wrote:\n\n> > The best idea I had for a fix is to print names beginning with `-`\n> > using the fully-qualified form, such as \"refs/tags/-a\".  Also, `--`\n> > is used as a delimiter in many commands, and can’t be escaped,\n> > so disallowing it might be a good idea.\n> \n> I do not think there is anything to fix.\n> \n> Command line parsers of some commands may have to learn how to\n> disambiguate such a strangely named tags and branches, though.  Some\n> commands do not know --end-of-options convention, for example.\n\nI think there is at least one thing to fix: rev-parse never learned\nabout --end-of-options. It also seems to have some weirdness around\n\"--\".\n\nHere's a series which fixes it.\n\n  [1/3]: rev-parse: don't accept options after dashdash\n  [2/3]: rev-parse: put all options under the \"-\" check\n  [3/3]: rev-parse: handle --end-of-options\n\n Documentation/git-rev-parse.txt |   8 ++-\n builtin/rev-parse.c             | 100 ++++++++++++++++++--------------\n t/t1503-rev-parse-verify.sh     |  13 +++++\n t/t1506-rev-parse-diagnosis.sh  |  25 ++++++++\n 4 files changed, 99 insertions(+), 47 deletions(-)\n\n-Peff\n"},{"id":"409551","messageId":"20201110213803.GB788740@coredump.intra.peff.net","threadId":"54613","inReplyTo":"20201110213544.GA3263091@coredump.intra.peff.net","subject":"[PATCH 2/3] rev-parse: put all options under the \"-\" check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-10T21:38:03Z","receivedAt":"2020-11-10T21:39:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The option-parsing loop of rev-parse checks whether the first character\nof an arg is \"-\". If so, then it enters a series of conditionals\nchecking for individual options. But some options are inexplicably\noutside of that outer conditional.\n\nThis doesn't produce the wrong behavior; the conditional is actually\nredundant with the individual option checks, and it's really only its\nfallback \"continue\" that we care about. But we should at least be\nconsistent.\n\nOne obvious alternative is that we could get rid of the conditional\nentirely. But we'll be using the extra block it provides in the next\npatch.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/rev-parse.c | 47 ++++++++++++++++++++++-----------------------\n 1 file changed, 23 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 293428fa0d..79689286d8 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -652,30 +652,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\tdid_repo_setup = 1;\n \t\t}\n \n-\t\tif (!strcmp(arg, \"--git-path\")) {\n-\t\t\tif (!argv[i + 1])\n-\t\t\t\tdie(\"--git-path requires an argument\");\n-\t\t\tstrbuf_reset(&buf);\n-\t\t\tputs(relative_path(git_path(\"%s\", argv[i + 1]),\n-\t\t\t\t\t   prefix, &buf));\n-\t\t\ti++;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg,\"-n\")) {\n-\t\t\tif (++i >= argc)\n-\t\t\t\tdie(\"-n requires an argument\");\n-\t\t\tif ((filter & DO_FLAGS) && (filter & DO_REVS)) {\n-\t\t\t\tshow(arg);\n-\t\t\t\tshow(argv[i]);\n-\t\t\t}\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (starts_with(arg, \"-n\")) {\n-\t\t\tif ((filter & DO_FLAGS) && (filter & DO_REVS))\n-\t\t\t\tshow(arg);\n-\t\t\tcontinue;\n-\t\t}\n-\n \t\tif (*arg == '-') {\n \t\t\tif (!strcmp(arg, \"--\")) {\n \t\t\t\tas_is = 2;\n@@ -684,6 +660,29 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tshow_file(arg, 0);\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (!strcmp(arg, \"--git-path\")) {\n+\t\t\t\tif (!argv[i + 1])\n+\t\t\t\t\tdie(\"--git-path requires an argument\");\n+\t\t\t\tstrbuf_reset(&buf);\n+\t\t\t\tputs(relative_path(git_path(\"%s\", argv[i + 1]),\n+\t\t\t\t\t\t   prefix, &buf));\n+\t\t\t\ti++;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tif (!strcmp(arg,\"-n\")) {\n+\t\t\t\tif (++i >= argc)\n+\t\t\t\t\tdie(\"-n requires an argument\");\n+\t\t\t\tif ((filter & DO_FLAGS) && (filter & DO_REVS)) {\n+\t\t\t\t\tshow(arg);\n+\t\t\t\t\tshow(argv[i]);\n+\t\t\t\t}\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tif (starts_with(arg, \"-n\")) {\n+\t\t\t\tif ((filter & DO_FLAGS) && (filter & DO_REVS))\n+\t\t\t\t\tshow(arg);\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (!strcmp(arg, \"--default\")) {\n \t\t\t\tdef = argv[++i];\n \t\t\t\tif (!def)\n-- \n2.29.2.640.g9e24689a4c\n\n"},{"id":"409552","messageId":"20201110213727.GA788740@coredump.intra.peff.net","threadId":"54613","inReplyTo":"20201110213544.GA3263091@coredump.intra.peff.net","subject":"[PATCH 1/3] rev-parse: don't accept options after dashdash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-10T21:37:27Z","receivedAt":"2020-11-10T21:39:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Because of the order in which we check options in rev-parse, there are a\nfew options we accept even after a \"--\". This is wrong, because the\nwhole point of \"--\" is to say \"everything after here is a path\". Let's\nmove the \"did we see a dashdash\" check (it's called \"as_is\" in the code)\nto the top of the parsing loop.\n\nNote there is one subtlety here. The options are ordered so that some\nare checked before we even see if we're in a repository (they continue\nthe loop, and if we get past a certain point, then we do the repository\nsetup). By moving the as_is check higher, it's also in that \"before\nsetup\" section, even though it might look at the repository via\nverify_filename(). However, this works out: we'd never set as_is until\nwe parse \"--\", and we don't parse that until after doing the setup.\n\nAn alternative here to avoid the subtlety is to put the as_is check at\nthe top of the post-setup options. But then every pre-setup option would\nhave to remember to check \"if (!as_is && !strcmp(...))\". So while this\nis a bit magical, it's harder for future code to get wrong.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI guess it could also shove them all into an:\n\n  if (!as_is) {\n\tif (!strcmp(\"--local-env-vars\"))\n  }\n\nconditional. I do end up having to do that later for end-of-options\nanyway.\n\n builtin/rev-parse.c            | 11 ++++++-----\n t/t1506-rev-parse-diagnosis.sh |  9 +++++++++\n 2 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex ed200c8af1..293428fa0d 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -622,6 +622,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \tfor (i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \n+\t\tif (as_is) {\n+\t\t\tif (show_file(arg, output_prefix) && as_is < 2)\n+\t\t\t\tverify_filename(prefix, arg, 0);\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\tif (!strcmp(arg, \"--local-env-vars\")) {\n \t\t\tint i;\n \t\t\tfor (i = 0; local_repo_env[i]; i++)\n@@ -655,11 +661,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\ti++;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (as_is) {\n-\t\t\tif (show_file(arg, output_prefix) && as_is < 2)\n-\t\t\t\tverify_filename(prefix, arg, 0);\n-\t\t\tcontinue;\n-\t\t}\n \t\tif (!strcmp(arg,\"-n\")) {\n \t\t\tif (++i >= argc)\n \t\t\t\tdie(\"-n requires an argument\");\ndiff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\nindex 3e657e693b..2ed5d50059 100755\n--- a/t/t1506-rev-parse-diagnosis.sh\n+++ b/t/t1506-rev-parse-diagnosis.sh\n@@ -254,4 +254,13 @@ test_expect_success 'escaped char does not trigger wildcard rule' '\n \ttest_must_fail git rev-parse \"foo\\\\*bar\"\n '\n \n+test_expect_success 'arg after dashdash not interpreted as option' '\n+\tcat >expect <<-\\EOF &&\n+\t--\n+\t--local-env-vars\n+\tEOF\n+\tgit rev-parse -- --local-env-vars >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.29.2.640.g9e24689a4c\n\n"},{"id":"409554","messageId":"20201110214019.GC788740@coredump.intra.peff.net","threadId":"54613","inReplyTo":"20201110213544.GA3263091@coredump.intra.peff.net","subject":"[PATCH 3/3] rev-parse: handle --end-of-options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-10T21:40:19Z","receivedAt":"2020-11-10T21:40:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We taught rev-list a new way to separate options from revisions in\n19e8789b23 (revision: allow --end-of-options to end option parsing,\n2019-08-06), but rev-parse uses its own parser. It should know about\n--end-of-options not only for consistency, but because it may be\npresented with similarly ambiguous cases. E.g., if a caller does:\n\n  git rev-parse \"$rev\" -- \"$path\"\n\nto parse an untrusted input, then it will get confused if $rev contains\nan option-like string like \"--local-env-vars\". Or even \"--not-real\",\nwhich we'd keep as an option to pass along to rev-list.\n\nOr even more importantly:\n\n  git rev-parse --verify \"$rev\"\n\ncan be confused by options, even though its purpose is safely parsing\nuntrusted input. On the plus side, it will always fail the --verify\npart, as it will not have parsed a revision, so the caller will\ngenerally \"fail closed\" rather than continue to use the untrusted\nstring. But it will still trigger whatever option was in \"$rev\"; this\nshould be mostly harmless, since rev-parse options are all read-only,\nbut I didn't carefully audit all paths.\n\nThis patch lets callers write:\n\n  git rev-parse --end-of-options \"$rev\" -- \"$path\"\n\nand:\n\n  git rev-parse --verify --end-of-options \"$rev\"\n\nwhich will both treat \"$rev\" always as a revision parameter. The latter\nis a bit clunky. It would be nicer if we had defined \"--verify\" to\nrequire that its next argument be the revision. But we have not\nhistorically done so, and:\n\n  git rev-parse --verify -q \"$rev\"\n\ndoes currently work. I added a test here to confirm that we didn't break\nthat.\n\nA few implementation notes:\n\n - We don't document --end-of-options explicitly in commands, but rather\n   in gitcli(7). So I didn't give it its own section in git-rev-parse(1).\n   But I did call it out specifically in the --verify section, and\n   include it in the examples, which should show best practices.\n\n - We don't have to re-indent the main option-parsing block, because we\n   can combine our \"did we see end of options\" check with \"does it start\n   with a dash\". The exception is the pre-setup options, which need\n   their own block.\n\n - We do however have to pull the \"--\" parsing out of the \"does it start\n   with dash\" block, because we want to parse it even if we've seen\n   --end-of-options.\n\n - We'll leave \"--end-of-options\" in the output. This is probably not\n   technically necessary, as a careful caller will do:\n\n     git rev-parse --end-of-options $revs -- $paths\n\n   and anything in $revs will be resolved to an object id. However, it\n   does help a slightly less careful caller like:\n\n     git rev-parse --end-of-options $revs_or_paths\n\n   where a path \"--foo\" will remain in the output as long as it also\n   exists on disk. In that case, it's helpful to retain --end-of-options\n   to get passed along to rev-list, s it would otherwise see just\n   \"--foo\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/git-rev-parse.txt |  8 +++--\n builtin/rev-parse.c             | 56 +++++++++++++++++++--------------\n t/t1503-rev-parse-verify.sh     | 13 ++++++++\n t/t1506-rev-parse-diagnosis.sh  | 16 ++++++++++\n 4 files changed, 68 insertions(+), 25 deletions(-)\n\ndiff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt\nindex 19b12b6d43..5013daa6ef 100644\n--- a/Documentation/git-rev-parse.txt\n+++ b/Documentation/git-rev-parse.txt\n@@ -109,6 +109,10 @@ names an existing object that is a commit-ish (i.e. a commit, or an\n annotated tag that points at a commit).  To make sure that `$VAR`\n names an existing object of any type, `git rev-parse \"$VAR^{object}\"`\n can be used.\n++\n+Note that if you are verifying a name from an untrusted source, it is\n+wise to use `--end-of-options` so that the name argument is not mistaken\n+for another option.\n \n -q::\n --quiet::\n@@ -446,15 +450,15 @@ $ git rev-parse --verify HEAD\n * Print the commit object name from the revision in the $REV shell variable:\n +\n ------------\n-$ git rev-parse --verify $REV^{commit}\n+$ git rev-parse --verify --end-of-options $REV^{commit}\n ------------\n +\n This will error out if $REV is empty or not a valid revision.\n \n * Similar to above:\n +\n ------------\n-$ git rev-parse --default master --verify $REV\n+$ git rev-parse --default master --verify --end-of-options $REV\n ------------\n +\n but if $REV is empty, the commit object name from master will be printed.\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 79689286d8..69ba7326cf 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -595,6 +595,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \tstruct object_context unused;\n \tstruct strbuf buf = STRBUF_INIT;\n \tconst int hexsz = the_hash_algo->hexsz;\n+\tint seen_end_of_options = 0;\n \n \tif (argc > 1 && !strcmp(\"--parseopt\", argv[1]))\n \t\treturn cmd_parseopt(argc - 1, argv + 1, prefix);\n@@ -628,21 +629,23 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (!strcmp(arg, \"--local-env-vars\")) {\n-\t\t\tint i;\n-\t\t\tfor (i = 0; local_repo_env[i]; i++)\n-\t\t\t\tprintf(\"%s\\n\", local_repo_env[i]);\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--resolve-git-dir\")) {\n-\t\t\tconst char *gitdir = argv[++i];\n-\t\t\tif (!gitdir)\n-\t\t\t\tdie(\"--resolve-git-dir requires an argument\");\n-\t\t\tgitdir = resolve_gitdir(gitdir);\n-\t\t\tif (!gitdir)\n-\t\t\t\tdie(\"not a gitdir '%s'\", argv[i]);\n-\t\t\tputs(gitdir);\n-\t\t\tcontinue;\n+\t\tif (!seen_end_of_options) {\n+\t\t\tif (!strcmp(arg, \"--local-env-vars\")) {\n+\t\t\t\tint i;\n+\t\t\t\tfor (i = 0; local_repo_env[i]; i++)\n+\t\t\t\t\tprintf(\"%s\\n\", local_repo_env[i]);\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tif (!strcmp(arg, \"--resolve-git-dir\")) {\n+\t\t\t\tconst char *gitdir = argv[++i];\n+\t\t\t\tif (!gitdir)\n+\t\t\t\t\tdie(\"--resolve-git-dir requires an argument\");\n+\t\t\t\tgitdir = resolve_gitdir(gitdir);\n+\t\t\t\tif (!gitdir)\n+\t\t\t\t\tdie(\"not a gitdir '%s'\", argv[i]);\n+\t\t\t\tputs(gitdir);\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t}\n \n \t\t/* The rest of the options require a git repository. */\n@@ -652,14 +655,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\tdid_repo_setup = 1;\n \t\t}\n \n-\t\tif (*arg == '-') {\n-\t\t\tif (!strcmp(arg, \"--\")) {\n-\t\t\t\tas_is = 2;\n-\t\t\t\t/* Pass on the \"--\" if we show anything but files.. */\n-\t\t\t\tif (filter & (DO_FLAGS | DO_REVS))\n-\t\t\t\t\tshow_file(arg, 0);\n-\t\t\t\tcontinue;\n-\t\t\t}\n+\t\tif (!strcmp(arg, \"--\")) {\n+\t\t\tas_is = 2;\n+\t\t\t/* Pass on the \"--\" if we show anything but files.. */\n+\t\t\tif (filter & (DO_FLAGS | DO_REVS))\n+\t\t\t\tshow_file(arg, 0);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (!seen_end_of_options && *arg == '-') {\n \t\t\tif (!strcmp(arg, \"--git-path\")) {\n \t\t\t\tif (!argv[i + 1])\n \t\t\t\t\tdie(\"--git-path requires an argument\");\n@@ -937,6 +941,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tputs(the_hash_algo->name);\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (!strcmp(arg, \"--end-of-options\")) {\n+\t\t\t\tseen_end_of_options = 1;\n+\t\t\t\tif (filter & (DO_FLAGS | DO_REVS))\n+\t\t\t\t\tshow_file(arg, 0);\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (show_flag(arg) && verify)\n \t\t\t\tdie_no_single_rev(quiet);\n \t\t\tcontinue;\ndiff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh\nindex 492edffa9c..dc9fe3cbf1 100755\n--- a/t/t1503-rev-parse-verify.sh\n+++ b/t/t1503-rev-parse-verify.sh\n@@ -144,4 +144,17 @@ test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '\n \ttest_must_fail git rev-parse --verify broken\n '\n \n+test_expect_success 'options can appear after --verify' '\n+\tgit rev-parse --verify HEAD >expect &&\n+\tgit rev-parse --verify -q HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'verify respects --end-of-options' '\n+\tgit update-ref refs/heads/-tricky HEAD &&\n+\tgit rev-parse --verify HEAD >expect &&\n+\tgit rev-parse --verify --end-of-options -tricky >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\nindex 2ed5d50059..e2ae15a2cf 100755\n--- a/t/t1506-rev-parse-diagnosis.sh\n+++ b/t/t1506-rev-parse-diagnosis.sh\n@@ -263,4 +263,20 @@ test_expect_success 'arg after dashdash not interpreted as option' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'arg after end-of-options not interpreted as option' '\n+\ttest_must_fail git rev-parse --end-of-options --not-real -- 2>err &&\n+\ttest_i18ngrep bad.revision.*--not-real err\n+'\n+\n+test_expect_success 'end-of-options still allows --' '\n+\tcat >expect <<-EOF &&\n+\t--end-of-options\n+\t$(git rev-parse --verify HEAD)\n+\t--\n+\tpath\n+\tEOF\n+\tgit rev-parse --end-of-options HEAD -- path >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.29.2.640.g9e24689a4c\n"},{"id":"409557","messageId":"xmqqr1p06ec9.fsf@gitster.c.googlers.com","threadId":"54613","inReplyTo":"20201110214019.GC788740@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] rev-parse: handle --end-of-options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-10T22:23:34Z","receivedAt":"2020-11-10T22:23:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This patch lets callers write:\n>\n>   git rev-parse --end-of-options \"$rev\" -- \"$path\"\n>\n> and:\n>\n>   git rev-parse --verify --end-of-options \"$rev\"\n>\n> which will both treat \"$rev\" always as a revision parameter.\n\nNice.  The only iffy case I can think of is that we can never have\n\"--\" to specify a rev, because with \"git cmd -- -- path\" we don't\nknow which double-dash among the two is the disambiguator that makes\nthe other double-dash to be either rev or path, but that is not a\nnew problem with this change.\n\n> +test_expect_success 'verify respects --end-of-options' '\n> +\tgit update-ref refs/heads/-tricky HEAD &&\n> +\tgit rev-parse --verify HEAD >expect &&\n> +\tgit rev-parse --verify --end-of-options -tricky >actual &&\n> +\ttest_cmp expect actual\n> +'\n\n;-)  Or refs/heads/--tricky?\n\nThe whole thing looked good.  Thanks.\n"},{"id":"409558","messageId":"f6736e4b-679b-437a-8768-2352efbb4274@gmail.com","threadId":"54613","inReplyTo":"xmqqr1p06ec9.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/3] rev-parse: handle --end-of-options","fromName":"Demi M. Obenour","fromEmail":"demiobenour@gmail.com","sentAt":"2020-11-10T22:28:13Z","receivedAt":"2020-11-10T22:28:46Z","isPatch":true,"sender":{"key":"demiobenour@gmail.com","avatar":null},"body":"On 11/10/20 5:23 PM, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n>> This patch lets callers write:\n>>\n>>   git rev-parse --end-of-options \"$rev\" -- \"$path\"\n>>\n>> and:\n>>\n>>   git rev-parse --verify --end-of-options \"$rev\"\n>>\n>> which will both treat \"$rev\" always as a revision parameter.\n> \n> Nice.  The only iffy case I can think of is that we can never have\n> \"--\" to specify a rev, because with \"git cmd -- -- path\" we don't\n> know which double-dash among the two is the disambiguator that makes\n> the other double-dash to be either rev or path, but that is not a\n> new problem with this change.\n\nI think Git should at least reject refs named \"--\", since they cause\nproblems in so many places.\n\nSincerely,\n\nDemi\n"},{"id":"409579","messageId":"20201111022238.GA806755@coredump.intra.peff.net","threadId":"54613","inReplyTo":"xmqqr1p06ec9.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/3] rev-parse: handle --end-of-options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-11T02:22:38Z","receivedAt":"2020-11-11T02:22:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 10, 2020 at 02:23:34PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > This patch lets callers write:\n> >\n> >   git rev-parse --end-of-options \"$rev\" -- \"$path\"\n> >\n> > and:\n> >\n> >   git rev-parse --verify --end-of-options \"$rev\"\n> >\n> > which will both treat \"$rev\" always as a revision parameter.\n> \n> Nice.  The only iffy case I can think of is that we can never have\n> \"--\" to specify a rev, because with \"git cmd -- -- path\" we don't\n> know which double-dash among the two is the disambiguator that makes\n> the other double-dash to be either rev or path, but that is not a\n> new problem with this change.\n\nYeah. It is sufficient to me that \"rev-parse\" (or any command) does not\ndo a bad thing when somebody malicious asks about \"--\", even if the repo\ncontents themselves are sane and do not have \"--\". If somebody has a\nnot-sane repo with refs/heads/-- and the worst case is that they have\ntrouble accessing it without a fully-qualified name, then I find it hard\nto sympathize. ;)\n\n> > +test_expect_success 'verify respects --end-of-options' '\n> > +\tgit update-ref refs/heads/-tricky HEAD &&\n> > +\tgit rev-parse --verify HEAD >expect &&\n> > +\tgit rev-parse --verify --end-of-options -tricky >actual &&\n> > +\ttest_cmp expect actual\n> > +'\n> \n> ;-)  Or refs/heads/--tricky?\n\nYeah, arguably that is a better name. It could even be a real option\nname, though once the bug is fixed none of it matters. :)\n\n-Peff\n"}]}