{"thread":{"id":"56077","subject":"[PATCH] revision: allow pseudo options after --end-of-options","startedAt":"2021-07-08T15:03:40Z","lastAt":"2021-07-27T06:11:09Z","messageCount":11,"participants":["Jiang Xin","brian m. carlson","Jeff King","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"429490","messageId":"20210708150316.10855-1-worldhello.net@gmail.com","threadId":"56077","inReplyTo":null,"subject":"[PATCH] revision: allow pseudo options after --end-of-options","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2021-07-08T15:03:16Z","receivedAt":"2021-07-08T15:03:40Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nOptions and revisions can be seperated by the option \"--end-of-options\"\nby introducing commit 19e8789b23 (revision: allow --end-of-options to\nend option parsing, 2019-08-06). The following command will show\nrevisions which have changes on file \"bar\" on a branch named \"--foo\":\n\n git rev-list --oneline --end-of-options --foo -- bar\n\nIf we want to see revisions between two revisions (rev1 and rev2), we\ncan use the following command:\n\n git rev-list --oneline --end-of-options rev1..rev2 --\n\nWe know that \"rev1..rev2\" is a shorthand for \"rev2 --not rev1\", but\nwe can not use the longer expression with option \"--not\" after the\n\"--end-of-options\" option. This is because the parser will not consume\nrevision pseudo options after seeing \"--end-of-option\".\n\nAllow parsing revision pseudo options after \"--end-of-options\", the\nfollowing command is valid:\n\n git rev-list --oneline --end-of-options rev2 --not rev2 --\n\nSigned-off-by: Jiang Xin \n---\n revision.c | 7 ++++---\n t/t6000-rev-list-misc.sh | 45 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 49 insertions(+), 3 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 8140561b6c..0d37b5a759 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2729,9 +2729,9 @@ 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 (!seen_end_of_options && *arg == '-') {\n-\t\t\tint opts;\n+\t\tint opts;\n \n+\t\tif (*arg == '-') {\n \t\t\topts = handle_revision_pseudo_opt(submodule,\n \t\t\t\t\t\trevs, argv + i,\n \t\t\t\t\t\t&flags);\n@@ -2739,7 +2739,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t\ti += opts - 1;\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t}\n \n+\t\tif (!seen_end_of_options && *arg == '-') {\n \t\t\tif (!strcmp(arg, \"--stdin\")) {\n \t\t\t\tif (revs->disable_stdin) {\n \t\t\t\t\targv[left++] = arg;\n@@ -2767,7 +2769,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\tcontinue;\n \t\t}\n \n-\n \t\tif (handle_revision_arg(arg, revs, flags, revarg_opt)) {\n \t\t\tint j;\n \t\t\tif (seen_dashdash || *arg == '^')\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex 12def7bcbf..3b2ca10456 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -169,4 +169,49 @@ test_expect_success 'rev-list --count --objects' '\n \ttest_line_count = $count actual\n '\n \n+test_expect_success 'merge branch \"--output=yikes\" to main' '\n+\tgit checkout main &&\n+\tgit merge -m \"Merge branch\" \\\n+\t\t--allow-unrelated-histories -- \\\n+\t\t--output=yikes &&\n+\techo three >> two/three &&\n+\tgit add two/three &&\n+\ttest_tick &&\n+\tgit commit -m \"three\" &&\n+\tcat >expect <<-EOF &&\n+\t> three\n+\t> Merge branch\n+\t> another\n+\t> that\n+\t> two\n+\t> one\n+\tEOF\n+\tgit log --pretty=\"%m %s\" --end-of-options HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'parse pseudo option \"--branches\" after \"--end-of-options\"' '\n+\tcat >expect <<-EOF &&\n+\t> three\n+\t> another\n+\t> Merge branch\n+\t> that\n+\t> two\n+\t> one\n+\tEOF\n+\tgit log --pretty=\"%m %s\" --end-of-options \\\n+\t\t--branches -- >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'parse pseudo option \"--not\" after \"--end-of-options\"' '\n+\tcat >expect <<-EOF &&\n+\t> three\n+\tEOF\n+\tgit log --pretty=\"%m %s\" --end-of-options \\\n+\t\tHEAD --not --output=yikes -- \\\n+\t\ttwo/three >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.32.0.rc0.27.g7b1e85181b\n\n"},{"id":"429500","messageId":"YOcvaFL7+6qcIOUa@camp.crustytoothpaste.net","threadId":"56077","inReplyTo":"20210708150316.10855-1-worldhello.net@gmail.com","subject":"Re: [PATCH] revision: allow pseudo options after --end-of-options","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-07-08T17:01:28Z","receivedAt":"2021-07-08T17:02:05Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-07-08 at 15:03:16, Jiang Xin wrote:\n> From: Jiang Xin \n> \n> Options and revisions can be seperated by the option \"--end-of-options\"\n> by introducing commit 19e8789b23 (revision: allow --end-of-options to\n> end option parsing, 2019-08-06). The following command will show\n> revisions which have changes on file \"bar\" on a branch named \"--foo\":\n> \n> git rev-list --oneline --end-of-options --foo -- bar\n> \n> If we want to see revisions between two revisions (rev1 and rev2), we\n> can use the following command:\n> \n> git rev-list --oneline --end-of-options rev1..rev2 --\n> \n> We know that \"rev1..rev2\" is a shorthand for \"rev2 --not rev1\", but\n> we can not use the longer expression with option \"--not\" after the\n> \"--end-of-options\" option. This is because the parser will not consume\n> revision pseudo options after seeing \"--end-of-option\".\n> \n> Allow parsing revision pseudo options after \"--end-of-options\", the\n> following command is valid:\n> \n> git rev-list --oneline --end-of-options rev2 --not rev2 --\n\nI don't think we want to do this. The goal of --end-of-options is to\nprevent parsing all future items as options, so if someone specifies a\nrevision starting with a dash, we don't end up with it being interpreted\nas an option.\n\nRemoving this constraint could end up resulting in potential security\nissues, which this option was introduced to protect against.\n\nIf you want to specify this form, you can write this:\n\n git rev-list --oneline refs/heads/rev2 --not refs/heads/rev1\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"429522","messageId":"CANYiYbGYzbMoU_2wb4duppASoYUjGLsJsr692Xe3GaVBOXUsBA@mail.gmail.com","threadId":"56077","inReplyTo":"YOcvaFL7+6qcIOUa@camp.crustytoothpaste.net","subject":"Re: [PATCH] revision: allow pseudo options after --end-of-options","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2021-07-09T01:33:23Z","receivedAt":"2021-07-09T01:33:36Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"brian m. carlson 于2021年7月9日周五 上午1:02写道：\n>\n> On 2021-07-08 at 15:03:16, Jiang Xin wrote:\n> > From: Jiang Xin \n> >\n> > Options and revisions can be seperated by the option \"--end-of-options\"\n> > by introducing commit 19e8789b23 (revision: allow --end-of-options to\n> > end option parsing, 2019-08-06). The following command will show\n> > revisions which have changes on file \"bar\" on a branch named \"--foo\":\n> >\n> > git rev-list --oneline --end-of-options --foo -- bar\n> >\n> > If we want to see revisions between two revisions (rev1 and rev2), we\n> > can use the following command:\n> >\n> > git rev-list --oneline --end-of-options rev1..rev2 --\n> >\n> > We know that \"rev1..rev2\" is a shorthand for \"rev2 --not rev1\", but\n> > we can not use the longer expression with option \"--not\" after the\n> > \"--end-of-options\" option. This is because the parser will not consume\n> > revision pseudo options after seeing \"--end-of-option\".\n> >\n> > Allow parsing revision pseudo options after \"--end-of-options\", the\n> > following command is valid:\n> >\n> > git rev-list --oneline --end-of-options rev2 --not rev2 --\n>\n> I don't think we want to do this. The goal of --end-of-options is to\n> prevent parsing all future items as options, so if someone specifies a\n> revision starting with a dash, we don't end up with it being interpreted\n> as an option.\n\nNew test case in t6000 covered this case. Branch \"--output=yikes\"\nwhich starts with a dash is used as revision after the option\n\"--end-of-options\", and it won't be interpreted as an option.\n\n test_expect_success 'parse pseudo option \"--not\" after \"--end-of-options\"' '\n cat >expect <<-EOF &&\n > three\n EOF\n git log --pretty=\"%m %s\" --end-of-options \\\n HEAD --not --output=yikes -- \\\n two/three >actual &&\n test_cmp expect actual\n '\n\nBut for the original implementation, because pseudo revision options\n(--branches, --tags, --not, ..., etc) can not be used after the\n\"--end-of-options\" option, we have to put \"--end-of-options\" at the\nend of revisions, such as:\n\n git log --pretty=\"%m %s\" rev1 --not rev2 rev3 rev4 \\\n --end-of-options -- path/file\n\nWe can see from the above command, the option \"--end-of-options\" is\nimmediately followed by a dashdash. That is very strange. DashDash is\ndesigned to separate pathspecs from args, and \"--end-of-options\" is\ndesigned to separate revisions from options. But because of the pseudo\nrevision options, \"--end-of-options\" is meaningless for commands\ncalling \"setup_revisions()\".\n\nYes, \"--end-of-options\" must be used if there is a revision which\nstarts with dash, such as branch \"--output=yikes\" in t6000. That's\neven stranger, for we have to write command in the middle of\nrevisions like this:\n\n git log --pretty=\"%m %s\" rev1 --not rev2 rev3 \\\n --end-of-options --output=yikes -- path/file\n\nI know \"rev1..rev2\" and \"rev2 ^rev1\", but I prefer to use \"rev1 --not\nrev2 rev3\" instead of \"rev1 ^rev2 ^rev3\".\n"},{"id":"429646","messageId":"YOoXCJV2ssef/KsN@camp.crustytoothpaste.net","threadId":"56077","inReplyTo":"CANYiYbGYzbMoU_2wb4duppASoYUjGLsJsr692Xe3GaVBOXUsBA@mail.gmail.com","subject":"Re: [PATCH] revision: allow pseudo options after --end-of-options","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-07-10T21:54:16Z","receivedAt":"2021-07-10T21:55:00Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-07-09 at 01:33:23, Jiang Xin wrote:\n> New test case in t6000 covered this case. Branch \"--output=yikes\"\n> which starts with a dash is used as revision after the option\n> \"--end-of-options\", and it won't be interpreted as an option.\n> \n> test_expect_success 'parse pseudo option \"--not\" after \"--end-of-options\"' '\n> cat >expect <<-EOF &&\n> > three\n> EOF\n> git log --pretty=\"%m %s\" --end-of-options \\\n> HEAD --not --output=yikes -- \\\n> two/three >actual &&\n> test_cmp expect actual\n> '\n> \n> But for the original implementation, because pseudo revision options\n> (--branches, --tags, --not, ..., etc) can not be used after the\n> \"--end-of-options\" option, we have to put \"--end-of-options\" at the\n> end of revisions, such as:\n> \n> git log --pretty=\"%m %s\" rev1 --not rev2 rev3 rev4 \\\n> --end-of-options -- path/file\n\nOr you could just use the other syntax and not have the problem. Or you\ncould write this:\n\n git log --pretty=\"%m %s\" refs/heads/rev1 --not --end-of-options rev2 rev3 rev4 \\\n -- path/file\n\nUnless there's a functional problem we're trying to solve, I'd much\nrather we didn't make --end-of-options means\n--end-of-some-options-but-not-others. That makes it hard to reason\nabout, and if someone does have a need for disabling all options, then\nwe have to add another option. It's also incompatible with the previous\nbehavior, so whereas \"--not\" used to be a revision, now it's an option.\n\nIt's unfortunate that we're not using -- here instead of\n--end-of-options because the former is the standard syntax, but that's\nwhat we have now since that's already used elsewhere.\n\n> Yes, \"--end-of-options\" must be used if there is a revision which\n> starts with dash, such as branch \"--output=yikes\" in t6000. That's\n> even stranger, for we have to write command in the middle of\n> revisions like this:\n> \n> git log --pretty=\"%m %s\" rev1 --not rev2 rev3 \\\n> --end-of-options --output=yikes -- path/file\n> \n> I know \"rev1..rev2\" and \"rev2 ^rev1\", but I prefer to use \"rev1 --not\n> rev2 rev3\" instead of \"rev1 ^rev2 ^rev3\".\n\nI don't think a personal preference is a good reason to change this.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"429787","messageId":"YOyByjmGu1oDXK4X@coredump.intra.peff.net","threadId":"56077","inReplyTo":"YOoXCJV2ssef/KsN@camp.crustytoothpaste.net","subject":"Re: [PATCH] revision: allow pseudo options after --end-of-options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-12T17:54:18Z","receivedAt":"2021-07-12T17:54:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 10, 2021 at 09:54:16PM +0000, brian m. carlson wrote:\n\n> > But for the original implementation, because pseudo revision options\n> > (--branches, --tags, --not, ..., etc) can not be used after the\n> > \"--end-of-options\" option, we have to put \"--end-of-options\" at the\n> > end of revisions, such as:\n> > \n> > git log --pretty=\"%m %s\" rev1 --not rev2 rev3 rev4 \\\n> > --end-of-options -- path/file\n> \n> Or you could just use the other syntax and not have the problem. Or you\n> could write this:\n> \n> git log --pretty=\"%m %s\" refs/heads/rev1 --not --end-of-options rev2 rev3 rev4 \\\n> -- path/file\n> \n> Unless there's a functional problem we're trying to solve, I'd much\n> rather we didn't make --end-of-options means\n> --end-of-some-options-but-not-others. That makes it hard to reason\n> about, and if someone does have a need for disabling all options, then\n> we have to add another option. It's also incompatible with the previous\n> behavior, so whereas \"--not\" used to be a revision, now it's an option.\n\nI agree that if we can avoid making exceptions, it makes the whole thing\nconceptually much cleaner (both for users to understand, but also for us\nto avoid accidentally introducing a security problem).\n\nI don't think fully-qualifying refs is a complete solution, though. The\ncommon use case for --end-of-options is that you're passing along names\nfrom somewhere else, and you don't know how to qualify them. E.g., in:\n\n git rev-list --end-of-options \"$rev\" --\n\nyou need to behave differently if you got \"1234abcd\" versus \"foo\" versus\n\"refs/heads/foo\".\n\nFor --not, I do think using \"^\" is a complete solution. It's a little\nmore work for the caller to prepend to each argument, but there's no\npolicy logic they have to implement.\n\nLooking over the other pseudo-opts, I could see some where treating them\nas a rev is reasonable (e.g., \"--all\"), but many where it is not at all\n(e.g., \"--no-walk\"; why is this even in handle_revision_pseudo_opt?).\nEven if you're just passing along untrusted revision specifiers, they\nact in roughly the same way as a single specifier. The big thing we'd\nlose is that you could never refer to a branch named \"--not\" or \"--all\".\n\nSo my gut feeling is _not_ to support them, but I can see arguments in\nboth directions and I don't feel that strongly about it.\n\n> > Yes, \"--end-of-options\" must be used if there is a revision which\n> > starts with dash, such as branch \"--output=yikes\" in t6000. That's\n> > even stranger, for we have to write command in the middle of\n> > revisions like this:\n> > \n> > git log --pretty=\"%m %s\" rev1 --not rev2 rev3 \\\n> > --end-of-options --output=yikes -- path/file\n> > \n> > I know \"rev1..rev2\" and \"rev2 ^rev1\", but I prefer to use \"rev1 --not\n> > rev2 rev3\" instead of \"rev1 ^rev2 ^rev3\".\n> \n> I don't think a personal preference is a good reason to change this.\n\nI do think it rises slightly above personal preference. It's potentially\nmaking things much easier for the caller if they can ferry along:\n\n tip=$1; shift\n git rev-list --end-of-options \"$1\" --not \"$@\"\n\ninstead of:\n\n tip=$1; shift\n # whoops, whitespace splitting is wrong here! Real programming\n # languages make this easier, of course.\n git rev-list --end-of-options \"$1\" $(for i in \"$@\"; do echo \"^$i\"; done)\n\nThough in my experience it is usually a static \"--not --all\" or \"--not\n--branches --tags\" or similar in such a function. I don't think I've\never seen a case quite like the code above in practice.\n\n-Peff\n"},{"id":"429813","messageId":"xmqqim1f5pz8.fsf@gitster.g","threadId":"56077","inReplyTo":"YOyByjmGu1oDXK4X@coredump.intra.peff.net","subject":"Re: [PATCH] revision: allow pseudo options after --end-of-options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-12T18:47:23Z","receivedAt":"2021-07-12T18:47:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I don't think fully-qualifying refs is a complete solution, though. The\n> common use case for --end-of-options is that you're passing along names\n> from somewhere else, and you don't know how to qualify them. E.g., in:\n>\n> git rev-list --end-of-options \"$rev\" --\n>\n> you need to behave differently if you got \"1234abcd\" versus \"foo\" versus\n> \"refs/heads/foo\".\n\nI suspect that you can prefix \"^^\" unconditionally, just like --not\ncan be emulated away by unconditionally prefixing \"^\".\n\n> For --not, I do think using \"^\" is a complete solution. It's a little\n> more work for the caller to prepend to each argument, but there's no\n> policy logic they have to implement.\n"},{"id":"429820","messageId":"YOycOlyqwwWZJ6rH@coredump.intra.peff.net","threadId":"56077","inReplyTo":"xmqqim1f5pz8.fsf@gitster.g","subject":"Re: [PATCH] revision: allow pseudo options after --end-of-options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-12T19:47:06Z","receivedAt":"2021-07-12T19:47:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 12, 2021 at 11:47:23AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I don't think fully-qualifying refs is a complete solution, though. The\n> > common use case for --end-of-options is that you're passing along names\n> > from somewhere else, and you don't know how to qualify them. E.g., in:\n> >\n> > git rev-list --end-of-options \"$rev\" --\n> >\n> > you need to behave differently if you got \"1234abcd\" versus \"foo\" versus\n> > \"refs/heads/foo\".\n> \n> I suspect that you can prefix \"^^\" unconditionally, just like --not\n> can be emulated away by unconditionally prefixing \"^\".\n\nThat would be clever, but I think we only parse a single \"^\":\n\n $ git rev-list ^HEAD\n [no output]\n $ git rev-list ^^HEAD\n fatal: bad revision '^^HEAD'\n\n $ git rev-parse ^HEAD\n ^d486ca60a51c9cb1fe068803c3f540724e95e83a\n $ git rev-parse ^^HEAD\n ^^HEAD\n fatal: ambiguous argument '^^HEAD': unknown revision or path not in the working tree.\n\n-Peff\n"},{"id":"429822","messageId":"xmqq4kcz47mq.fsf@gitster.g","threadId":"56077","inReplyTo":"YOycOlyqwwWZJ6rH@coredump.intra.peff.net","subject":"Re: [PATCH] revision: allow pseudo options after --end-of-options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-12T20:09:01Z","receivedAt":"2021-07-12T20:09:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jul 12, 2021 at 11:47:23AM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > I don't think fully-qualifying refs is a complete solution, though. The\n>> > common use case for --end-of-options is that you're passing along names\n>> > from somewhere else, and you don't know how to qualify them. E.g., in:\n>> >\n>> > git rev-list --end-of-options \"$rev\" --\n>> >\n>> > you need to behave differently if you got \"1234abcd\" versus \"foo\" versus\n>> > \"refs/heads/foo\".\n>> \n>> I suspect that you can prefix \"^^\" unconditionally, just like --not\n>> can be emulated away by unconditionally prefixing \"^\".\n>\n> That would be clever, but I think we only parse a single \"^\":\n>\n> $ git rev-list ^HEAD\n> [no output]\n> $ git rev-list ^^HEAD\n> fatal: bad revision '^^HEAD'\n>\n> $ git rev-parse ^HEAD\n> ^d486ca60a51c9cb1fe068803c3f540724e95e83a\n> $ git rev-parse ^^HEAD\n> ^^HEAD\n> fatal: ambiguous argument '^^HEAD': unknown revision or path not in the working tree.\n>\n> -Peff\n\n;-) Surprised.\n"},{"id":"429884","messageId":"CANYiYbF3dcOzZ5gqbYkstxv+WRxZwnoxoNP28A6px44YD98k1Q@mail.gmail.com","threadId":"56077","inReplyTo":"YOyByjmGu1oDXK4X@coredump.intra.peff.net","subject":"Re: [PATCH] revision: allow pseudo options after --end-of-options","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2021-07-13T08:57:46Z","receivedAt":"2021-07-13T08:57:59Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"Jeff King 于2021年7月13日周二 上午1:54写道：\n> On Sat, Jul 10, 2021 at 09:54:16PM +0000, brian m. carlson wrote:\n> > > I know \"rev1..rev2\" and \"rev2 ^rev1\", but I prefer to use \"rev1 --not\n> > > rev2 rev3\" instead of \"rev1 ^rev2 ^rev3\".\n> >\n> > I don't think a personal preference is a good reason to change this.\n>\n> I do think it rises slightly above personal preference. It's potentially\n> making things much easier for the caller if they can ferry along:\n>\n> tip=$1; shift\n> git rev-list --end-of-options \"$1\" --not \"$@\"\n>\n> instead of:\n>\n> tip=$1; shift\n> # whoops, whitespace splitting is wrong here! Real programming\n> # languages make this easier, of course.\n> git rev-list --end-of-options \"$1\" $(for i in \"$@\"; do echo \"^$i\"; done)\n>\n> Though in my experience it is usually a static \"--not --all\" or \"--not\n> --branches --tags\" or similar in such a function. I don't think I've\n> ever seen a case quite like the code above in practice.\n\nLast week, I made a study on how gitlab wrap and execute a git\ncommand. I saw the following code [1]:\n\n if c.supportsEndOfOptions() {\n commandArgs = append(commandArgs, \"--end-of-options\")\n }\n if len(postSepArgs) > 0 {\n commandArgs = append(commandArgs, \"--\")\n }\n\nI was surprised to see the options \"--end-of-options\" and \"--\" used\nnext to each other, and the DashDash option (\"--\") is not mandatory. I\nwant to make some changes on it, but when I try to construct a git\ncommand like this:\n\n git.SubCmd{\n Name: \"log\",\n Flags: []git.Option{\n git.Flag{\n Name: \"--stat\"\n },\n git.ValueFlag{\n Name: \"--pretty\",\n Value: \"%m %s\",\n },\n git.Flag{\n Name: \"--no-decorate\",\n },\n },\n Args: []string {\n \"topic1\",\n \"--not\",\n \"main\",\n \"release\",\n },\n PostSepArgs: []string {\n \"src/hello.c\",\n \"doc\",\n },\n }\n\nThe generated git command will be:\n\n git log --stat --pretty=\"%m %s\" --no-decorate \\\n topic1 --not main release \\\n --end-of-options \\\n -- \\\n src/hello.c doc\n\nIt works. But if I move the \"--end-of-options\" before the revisions like this:\n\n git log --stat --pretty=\"%m %s\" --no-decorate \\\n --end-of-options \\\n topic1 --not main release \\\n -- \\\n src/hello.c doc\n\nThe generated command failed to execute with error: unknown revision \"--not\".\n\nIt's reasonable for gitlab to construct git commands using mainly three fields:\n1. Flags: for options like \"--option\", or \"--key value\".\n2. Args: for args like revisions.\n3. PostSepArgs: for pathspecs.\n\nAnd if the command supports these options, it's better to add\n\"--end-of-options\" between 1 and 2, and add \"--\" between 2 and 3.\n\nIf we can handle revision pseudo opts as pseudo revisions instead of\noptions as in this patch, the only disadvantage is that we cannot\nhandle branches whose names conflict with well-known options such as\n\"--not\" and \"--all\". But users can input full branch names, such as\n\"refs/heads/--not\", \"refs/heads/--all\".\n\nMaybe I should keep this patch as a local enhancement for git.\n\n1. https://gitlab.com/gitlab-org/gitaly/-/blob/v14.0.5/internal/git/command_description.go#L320-326\n"},{"id":"429934","messageId":"YO4CBKuwCEL4Xdzg@coredump.intra.peff.net","threadId":"56077","inReplyTo":"CANYiYbF3dcOzZ5gqbYkstxv+WRxZwnoxoNP28A6px44YD98k1Q@mail.gmail.com","subject":"Re: [PATCH] revision: allow pseudo options after --end-of-options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-13T21:13:40Z","receivedAt":"2021-07-13T21:13:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 13, 2021 at 04:57:46PM +0800, Jiang Xin wrote:\n\n> > Though in my experience it is usually a static \"--not --all\" or \"--not\n> > --branches --tags\" or similar in such a function. I don't think I've\n> > ever seen a case quite like the code above in practice.\n> \n> Last week, I made a study on how gitlab wrap and execute a git\n> command. I saw the following code [1]:\n> \n> if c.supportsEndOfOptions() {\n> commandArgs = append(commandArgs, \"--end-of-options\")\n> }\n> if len(postSepArgs) > 0 {\n> commandArgs = append(commandArgs, \"--\")\n> }\n> \n> I was surprised to see the options \"--end-of-options\" and \"--\" used\n> next to each other, and the DashDash option (\"--\") is not mandatory.\n\nI think using --end-of-options there is pointless. The \"--\" will always\nsignal the end of options (_and_ revisions). So if there is nothing\nbetween the two, then the former cannot be doing anything.\n\nFor programmatic use, I do think one should always use \"--\". Even if\nthere are no paths, it makes it clear that the arguments before it are\nmeant to be revisions. This matters a little less in a bare repo, I\nthink (because we do not have a working tree to try to DWIM-match the\npaths in), but it's good practice in general.\n\nI don't think you can just blindly add \"--\" in such a function, though.\nIt depends on what the interface of the function is (i.e., are its\ncallers passing in options, revisions, and pathspecs as separate data\nstructures). It does look like you're going in that direction below,\nthough.\n\n> I\n> want to make some changes on it, but when I try to construct a git\n> command like this:\n> \n> git.SubCmd{\n> Name: \"log\",\n> Flags: []git.Option{\n> git.Flag{\n> Name: \"--stat\"\n> },\n> git.ValueFlag{\n> Name: \"--pretty\",\n> Value: \"%m %s\",\n> },\n> git.Flag{\n> Name: \"--no-decorate\",\n> },\n> },\n> Args: []string {\n> \"topic1\",\n> \"--not\",\n> \"main\",\n> \"release\",\n> },\n> PostSepArgs: []string {\n> \"src/hello.c\",\n> \"doc\",\n> },\n> }\n> \n> The generated git command will be:\n> \n> git log --stat --pretty=\"%m %s\" --no-decorate \\\n> topic1 --not main release \\\n> --end-of-options \\\n> -- \\\n> src/hello.c doc\n> \n> It works.\n\nRight, but you are not getting any protection against \"topic1\" being an\noption. The --end-of-options is doing nothing.\n\n> But if I move the \"--end-of-options\" before the revisions like this:\n> \n> git log --stat --pretty=\"%m %s\" --no-decorate \\\n> --end-of-options \\\n> topic1 --not main release \\\n> -- \\\n> src/hello.c doc\n> \n> The generated command failed to execute with error: unknown revision \"--not\".\n> \n> It's reasonable for gitlab to construct git commands using mainly three fields:\n> 1. Flags: for options like \"--option\", or \"--key value\".\n> 2. Args: for args like revisions.\n> 3. PostSepArgs: for pathspecs.\n> \n> And if the command supports these options, it's better to add\n> \"--end-of-options\" between 1 and 2, and add \"--\" between 2 and 3.\n\nYeah, so the problem there is that the definition of \"Args\" is kind of\nfuzzy. Sometimes it is useful to include stuff like \"--not\", and\nsometimes it is dangerous or unexpected. Later you say:\n\n> If we can handle revision pseudo opts as pseudo revisions instead of\n> options as in this patch, the only disadvantage is that we cannot\n> handle branches whose names conflict with well-known options such as\n> \"--not\" and \"--all\". But users can input full branch names, such as\n> \"refs/heads/--not\", \"refs/heads/--all\".\n\nbut the point of --end-of-options is that you do not necessarily trust\nthe caller (or the user) to have prefixes with \"refs/heads/\" as\nappropriate. It is about telling Git unambiguously what the various bits\non the command-line mean so that it knows how to correctly interpret\nthem.\n\nIt might be worthwhile to split the function interface into two sets:\npositive and negative traversal tips. And then the function can turn:\n\n {\n Options: { \"--stat\" }\n Tips: { \"topic1\" }\n Not-Tips: { \"main\", \"release\" }\n Pathspecs: { \"src/hello.c }\n }\n\ninto:\n\n git rev-list --stat --end-of-options topic1 ^main ^release -- src/hello.c\n\nwhich is unambiguous, even if \"--not\" appears in \"Tips\". (You can also\nkeep a single Args array, but then the caller is responsible for putting\nin the \"^\" markers).\n\n-Peff\n"},{"id":"431287","messageId":"YP+jZ/slM0MzmLF4@ncase","threadId":"56077","inReplyTo":"YO4CBKuwCEL4Xdzg@coredump.intra.peff.net","subject":"Re: [PATCH] revision: allow pseudo options after --end-of-options","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-07-27T06:10:47Z","receivedAt":"2021-07-27T06:11:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jul 13, 2021 at 05:13:40PM -0400, Jeff King wrote:\n> On Tue, Jul 13, 2021 at 04:57:46PM +0800, Jiang Xin wrote:\n> \n> > > Though in my experience it is usually a static \"--not --all\" or \"--not\n> > > --branches --tags\" or similar in such a function. I don't think I've\n> > > ever seen a case quite like the code above in practice.\n> > \n> > Last week, I made a study on how gitlab wrap and execute a git\n> > command. I saw the following code [1]:\n> > \n> > if c.supportsEndOfOptions() {\n> > commandArgs = append(commandArgs, \"--end-of-options\")\n> > }\n> > if len(postSepArgs) > 0 {\n> > commandArgs = append(commandArgs, \"--\")\n> > }\n> > \n> > I was surprised to see the options \"--end-of-options\" and \"--\" used\n> > next to each other, and the DashDash option (\"--\") is not mandatory.\n> \n> I think using --end-of-options there is pointless. The \"--\" will always\n> signal the end of options (_and_ revisions). So if there is nothing\n> between the two, then the former cannot be doing anything.\n\nIndeed it is. I somehow missed your Cc (not used to receiving Git ML\nmails on my work address), but by chance I fixed this last week because\nI realized this was broken. We've now moved the `--end-of-options` flag\nbetween positional arguments and flags like it should've been from the\nbeginning [1].\n\n[snip]\n> > But if I move the \"--end-of-options\" before the revisions like this:\n> > \n> > git log --stat --pretty=\"%m %s\" --no-decorate \\\n> > --end-of-options \\\n> > topic1 --not main release \\\n> > -- \\\n> > src/hello.c doc\n> > \n> > The generated command failed to execute with error: unknown revision \"--not\".\n> > \n> > It's reasonable for gitlab to construct git commands using mainly three fields:\n> > 1. Flags: for options like \"--option\", or \"--key value\".\n> > 2. Args: for args like revisions.\n> > 3. PostSepArgs: for pathspecs.\n> > \n> > And if the command supports these options, it's better to add\n> > \"--end-of-options\" between 1 and 2, and add \"--\" between 2 and 3.\n> \n> Yeah, so the problem there is that the definition of \"Args\" is kind of\n> fuzzy. Sometimes it is useful to include stuff like \"--not\", and\n> sometimes it is dangerous or unexpected. Later you say:\n\nYeah, this is something that has been bothering me, too. It would be\nnice to give special treatment to pseudo-revisions in git. That's why we\nhave now marked git-rev-list(1) to not support `--end-of-options` in\nGitaly, because we do pass pseudo-revisions to it in multiple places.\n\nPatrick\n\n[1]: https://gitlab.com/gitlab-org/gitaly/-/merge_requests/3676\n"}]}