{"thread":{"id":"35483","subject":"[BUG] redundant error message","startedAt":"2013-12-05T10:07:31Z","lastAt":"2013-12-09T21:33:42Z","messageCount":28,"participants":["Duy Nguyen","Jeff King","Junio C Hamano","Jonathan Nieder","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"231582","messageId":"CACsJy8BtCi_QSMZXfnscQmRyjvgSV1fG6smAOoCSab_e2YATxQ@mail.gmail.com","threadId":"35483","inReplyTo":null,"subject":"[BUG] redundant error message","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-12-05T10:07:31Z","receivedAt":"2013-12-05T10:07:31Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"$ git rev-parse foobar --\nfoobar\nfatal: ambiguous argument 'foobar': unknown revision or path not in\nthe working tree.\nUse '--' to separate paths from revisions, like this:\n'git [...] -- [...]'\n\nI already put \"--\" there. So it should shut up.\n-- \nDuy\n"},{"id":"231624","messageId":"20131205191518.GB19039@sigill.intra.peff.net","threadId":"35483","inReplyTo":"CACsJy8BtCi_QSMZXfnscQmRyjvgSV1fG6smAOoCSab_e2YATxQ@mail.gmail.com","subject":"Re: [BUG] redundant error message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-05T19:15:18Z","receivedAt":"2013-12-05T19:15:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 05, 2013 at 05:07:31PM +0700, Duy Nguyen wrote:\n\n> $ git rev-parse foobar --\n> foobar\n> fatal: ambiguous argument 'foobar': unknown revision or path not in\n> the working tree.\n> Use '--' to separate paths from revisions, like this:\n> 'git [...] -- [...]'\n> \n> I already put \"--\" there. So it should shut up.\n\nI think it is more than just a bad error message; if we get that\nmessage, it means we are following the wrong code path in interpreting\n\"foobar\":\n\n $ >foobar\n $ git rev-parse foobar --\n foobar\n --\n fatal: bad flag '--' used after filename\n\nThat's not right. It looks like this isn't a new breakage, though.\nv1.5.0 produces the same result.\n\n-Peff\n"},{"id":"231630","messageId":"xmqqsiu73w6n.fsf@gitster.dls.corp.google.com","threadId":"35483","inReplyTo":"20131205191518.GB19039@sigill.intra.peff.net","subject":"Re: [BUG] redundant error message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-05T20:00:16Z","receivedAt":"2013-12-05T20:00:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> $ >foobar\n> $ git rev-parse foobar --\n> foobar\n> --\n> fatal: bad flag '--' used after filename\n>\n> That's not right.\n\nHmph, it looks like it is following the usual \"zero-or-more dashed\noptions, zero-or-more revs and then zero-or-one double-dash and then\nzero-or-more paths\" rule to parse the thing. \"foobar\" is a file and\nnot a rev, so \"--\" should not be there, no?\n\nConfused why you think it is not right...\n"},{"id":"231632","messageId":"20131205200306.GA13443@sigill.intra.peff.net","threadId":"35483","inReplyTo":"xmqqsiu73w6n.fsf@gitster.dls.corp.google.com","subject":"Re: [BUG] redundant error message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-05T20:03:06Z","receivedAt":"2013-12-05T20:03:06Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 05, 2013 at 12:00:16PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > $ >foobar\n> > $ git rev-parse foobar --\n> > foobar\n> > --\n> > fatal: bad flag '--' used after filename\n> >\n> > That's not right.\n> \n> Hmph, it looks like it is following the usual \"zero-or-more dashed\n> options, zero-or-more revs and then zero-or-one double-dash and then\n> zero-or-more paths\" rule to parse the thing. \"foobar\" is a file and\n> not a rev, so \"--\" should not be there, no?\n> \n> Confused why you think it is not right...\n\nBecause once you say \"--\", then all ambiguity goes away, no? Everything\nto the left is a rev, not a filename, and everything to the right is a\nfilename. E.g.:\n\n $ git log foobar --\n fatal: bad revision 'foobar'\n\nI think rev-parse is not following the same rules that the rest of the\nrevision-parsing programs do.\n\n-Peff\n"},{"id":"231634","messageId":"xmqqfvq73vhw.fsf@gitster.dls.corp.google.com","threadId":"35483","inReplyTo":"20131205200306.GA13443@sigill.intra.peff.net","subject":"Re: [BUG] redundant error message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-05T20:15:07Z","receivedAt":"2013-12-05T20:15:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Dec 05, 2013 at 12:00:16PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > $ >foobar\n>> > $ git rev-parse foobar --\n>> > foobar\n>> > --\n>> > fatal: bad flag '--' used after filename\n>> >\n>> > That's not right.\n>> \n>> Hmph, it looks like it is following the usual \"zero-or-more dashed\n>> options, zero-or-more revs and then zero-or-one double-dash and then\n>> zero-or-more paths\" rule to parse the thing. \"foobar\" is a file and\n>> not a rev, so \"--\" should not be there, no?\n>> \n>> Confused why you think it is not right...\n>\n> Because once you say \"--\", then all ambiguity goes away, no?\n\nBut it is tricky (not from implementation but from semantics point\nof view) to make rev-parse follow that \"-- separates revs and paths\"\nrule literally. The primary use of rev-parse is to convert revs in\nextended SHA-1 expressions into concrete object names, so that\nscripts do not have to worry about having to deal with object names\nin a format that is not 40-hexdecimal. \"git rev-parse foobar --\"\nthat gives\n\n\tfoobar\n --\n\nwithout any error, because 'foobar' cannot be made into an object\nname, would be behaving in a way unexpected by the calling script,\nno?\n"},{"id":"231637","messageId":"20131205210000.GA19617@sigill.intra.peff.net","threadId":"35483","inReplyTo":"xmqqfvq73vhw.fsf@gitster.dls.corp.google.com","subject":"Re: [BUG] redundant error message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-05T21:00:00Z","receivedAt":"2013-12-05T21:00:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 05, 2013 at 12:15:07PM -0800, Junio C Hamano wrote:\n\n> >> Hmph, it looks like it is following the usual \"zero-or-more dashed\n> >> options, zero-or-more revs and then zero-or-one double-dash and then\n> >> zero-or-more paths\" rule to parse the thing. \"foobar\" is a file and\n> >> not a rev, so \"--\" should not be there, no?\n> >> \n> >> Confused why you think it is not right...\n> >\n> > Because once you say \"--\", then all ambiguity goes away, no?\n> \n> But it is tricky (not from implementation but from semantics point\n> of view) to make rev-parse follow that \"-- separates revs and paths\"\n> rule literally. The primary use of rev-parse is to convert revs in\n> extended SHA-1 expressions into concrete object names, so that\n> scripts do not have to worry about having to deal with object names\n> in a format that is not 40-hexdecimal. \"git rev-parse foobar --\"\n> that gives\n> \n> \tfoobar\n> --\n> \n> without any error, because 'foobar' cannot be made into an object\n> name, would be behaving in a way unexpected by the calling script,\n> no?\n\nYes, I do expect an error. But it should not be \"-- after filename\". It\nshould be \"foobar is not a revision\".\n\nThinking on it more, though, the problem is purely limited to wrong\nerror messages. If \"foobar\" exists as a rev, we do parse it correctly.\nIf it does not, we are in the wrong code path, but it _must_ be an error\nat that point (either because foobar does not exist as a file, or it\ndoes and has \"--\" after it).\n\nIt would be nice to get the error messages right, though. I do not see\nany reason why it could not follow the same steps as \"git log\",\nconverting revisions (or throwing an error as appropriate) on the left\nside of the \"--\", and passing through the right side untouched.\n\n-Peff\n"},{"id":"231638","messageId":"20131205212851.GA21776@sigill.intra.peff.net","threadId":"35483","inReplyTo":"20131205210000.GA19617@sigill.intra.peff.net","subject":"Re: [BUG] redundant error message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-05T21:28:51Z","receivedAt":"2013-12-05T21:28:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 05, 2013 at 04:00:00PM -0500, Jeff King wrote:\n\n> Yes, I do expect an error. But it should not be \"-- after filename\". It\n> should be \"foobar is not a revision\".\n> [...]\n> It would be nice to get the error messages right, though. I do not see\n> any reason why it could not follow the same steps as \"git log\",\n> converting revisions (or throwing an error as appropriate) on the left\n> side of the \"--\", and passing through the right side untouched.\n\nIOW, the patch below, which is the same strategy that setup_revisions\nuses:\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex c76b89d..845eab9 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -476,6 +476,7 @@ N_(\"git rev-parse --parseopt [options] -- [...]\\n\"\n int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n {\n \tint i, as_is = 0, verify = 0, quiet = 0, revs_count = 0, type = 0;\n+\tint has_dashdash = 0;\n \tint output_prefix = 0;\n \tunsigned char sha1[20];\n \tconst char *name = NULL;\n@@ -489,6 +490,14 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \tif (argc > 1 && !strcmp(\"-h\", argv[1]))\n \t\tusage(builtin_rev_parse_usage);\n \n+\n+\tfor (i = 1; i < argc; i++) {\n+\t\tif (!strcmp(argv[i], \"--\")) {\n+\t\t\thas_dashdash = 1;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n \tprefix = setup_git_directory();\n \tgit_config(git_default_config, NULL);\n \tfor (i = 1; i < argc; i++) {\n@@ -765,6 +774,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tif (verify)\n \t\t\tdie_no_single_rev(quiet);\n+\t\tif (has_dashdash)\n+\t\t\tdie(\"bad revision '%s'\", arg);\n \t\tas_is = 1;\n \t\tif (!show_file(arg, output_prefix))\n \t\t\tcontinue;\n\n\nBTW, the raw looping to find \"--\" made me wonder how we handle:\n\n git log --grep -- HEAD\n\nI'd expect it to be equivalent to:\n\n git log --grep=-- HEAD\n\nbut it's not; we truncate the arguments and complain that --grep is\nmissing its argument. Which is probably good enough, given that the\nalternative is doing a pass that understands all of the options. But it\ndoes mean that the \"--long-opt=arg\" form is safer than the split form if\nyou are passing along an arbitrary \"arg\".\n\n-Peff\n"},{"id":"231639","messageId":"xmqq8uvz3rdf.fsf@gitster.dls.corp.google.com","threadId":"35483","inReplyTo":"20131205212851.GA21776@sigill.intra.peff.net","subject":"Re: [BUG] redundant error message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-05T21:44:12Z","receivedAt":"2013-12-05T21:44:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> BTW, the raw looping to find \"--\" made me wonder how we handle:\n>\n> git log --grep -- HEAD\n>\n> I'd expect it to be equivalent to:\n>\n> git log --grep=-- HEAD\n>\n> but it's not; we truncate the arguments and complain that --grep is\n> missing its argument. Which is probably good enough, given that the\n> alternative is doing a pass that understands all of the options. But it\n> does mean that the \"--long-opt=arg\" form is safer than the split form if\n> you are passing along an arbitrary \"arg\".\n\n;-) Good flow of thought. As to your rev-parse change, I don't\nimmediately think of a hole/flaw offhand; it looked a good\nstraight-forward change to me.\n"},{"id":"231647","messageId":"CACsJy8CKDiCX0G3jD6KLZ=cyvp6zf4MT6DCY0XodfD8rkQF=wA@mail.gmail.com","threadId":"35483","inReplyTo":"20131205212851.GA21776@sigill.intra.peff.net","subject":"Re: [BUG] redundant error message","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-12-06T01:15:52Z","receivedAt":"2013-12-06T01:15:52Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Dec 6, 2013 at 4:28 AM, Jeff King wrote:\n> BTW, the raw looping to find \"--\" made me wonder how we handle:\n>\n> git log --grep -- HEAD\n>\n> I'd expect it to be equivalent to:\n>\n> git log --grep=-- HEAD\n>\n> but it's not; we truncate the arguments and complain that --grep is\n> missing its argument. Which is probably good enough, given that the\n> alternative is doing a pass that understands all of the options. But it\n> does mean that the \"--long-opt=arg\" form is safer than the split form if\n> you are passing along an arbitrary \"arg\".\n\nMaybe we could make setup_revisions() use parse_options() someday,\nwhich understands about arguments and dashdash.\n\n$ ./git grep -e -- foo\nfatal: ambiguous argument 'foo': both revision and filename\nUse '--' to separate paths from revisions, like this:\n'git [...] -- [...]'\n$ ./git grep -e -- -- foo\n$\n-- \nDuy\n"},{"id":"231674","messageId":"20131206211222.GB20482@sigill.intra.peff.net","threadId":"35483","inReplyTo":"xmqq8uvz3rdf.fsf@gitster.dls.corp.google.com","subject":"[PATCH 0/2] rev-parse and \"--\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-06T21:12:22Z","receivedAt":"2013-12-06T21:12:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 05, 2013 at 01:44:12PM -0800, Junio C Hamano wrote:\n\n> ;-) Good flow of thought. As to your rev-parse change, I don't\n> immediately think of a hole/flaw offhand; it looked a good\n> straight-forward change to me.\n\nHere it is with tests and a commit message. While writing the tests, I\nnoticed that also do not handle true ambiguity the same (we prefer revs,\nrather than complaining). That's fixed in the second commit.\n\n [1/2]: rev-parse: correctly diagnose revision errors before \"--\"\n [2/2]: rev-parse: diagnose ambiguous revision/filename arguments\n\n-Peff\n"},{"id":"231675","messageId":"20131206211346.GA20536@sigill.intra.peff.net","threadId":"35483","inReplyTo":"20131206211222.GB20482@sigill.intra.peff.net","subject":"[PATCH 1/2] rev-parse: correctly diagnose revision errors before \"--\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-06T21:13:46Z","receivedAt":"2013-12-06T21:13:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Rev-parse understands that a \"--\" may separate revisions and\nfilenames, and that anything after the \"--\" is taken as-is.\nHowever, it does not understand that anything before the\ntoken must be a revision (which is the usual rule\nimplemented by the setup_revisions parser).\n\nSince rev-parse prefers revisions to files when parsing\nbefore the \"--\", we end up with the correct result (if such\nan argument is a revision, we parse it as one, and if it is\nnot, it is an error either way). However, we misdiagnose\nthe errors:\n\n $ git rev-parse foobar -- >/dev/null\n fatal: ambiguous argument 'foobar': unknown revision or path not in the working tree.\n Use '--' to separate paths from revisions, like this:\n 'git [...] -- [...]'\n\n $ >foobar\n $ git rev-parse foobar -- >/dev/null\n fatal: bad flag '--' used after filename\n\nIn both cases, we should know that the real error is that\n\"foobar\" is meant to be a revision, but could not be\nresolved.\n\nSigned-off-by: Jeff King \n---\n builtin/rev-parse.c | 10 ++++++++++\n t/t1506-rev-parse-diagnosis.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 34 insertions(+)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 1d9ecaf..bb694ab 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -488,6 +488,7 @@ N_(\"git rev-parse --parseopt [options] -- [...]\\n\"\n int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n {\n \tint i, as_is = 0, verify = 0, quiet = 0, revs_count = 0, type = 0;\n+\tint has_dashdash = 0;\n \tint output_prefix = 0;\n \tunsigned char sha1[20];\n \tconst char *name = NULL;\n@@ -501,6 +502,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \tif (argc > 1 && !strcmp(\"-h\", argv[1]))\n \t\tusage(builtin_rev_parse_usage);\n \n+\tfor (i = 1; i < argc; i++) {\n+\t\tif (!strcmp(argv[i], \"--\")) {\n+\t\t\thas_dashdash = 1;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n \tprefix = setup_git_directory();\n \tgit_config(git_default_config, NULL);\n \tfor (i = 1; i < argc; i++) {\n@@ -788,6 +796,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tif (verify)\n \t\t\tdie_no_single_rev(quiet);\n+\t\tif (has_dashdash)\n+\t\t\tdie(\"bad revision '%s'\", arg);\n \t\tas_is = 1;\n \t\tif (!show_file(arg, output_prefix))\n \t\t\tcontinue;\ndiff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\nindex f950c10..613d9bf 100755\n--- a/t/t1506-rev-parse-diagnosis.sh\n+++ b/t/t1506-rev-parse-diagnosis.sh\n@@ -196,4 +196,28 @@ test_expect_success 'dotdot is not an empty set' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'arg before dashdash must be a revision (missing)' '\n+\ttest_must_fail git rev-parse foobar -- 2>stderr &&\n+\ttest_i18ngrep \"bad revision\" stderr\n+'\n+\n+test_expect_success 'arg before dashdash must be a revision (file)' '\n+\t>foobar &&\n+\ttest_must_fail git rev-parse foobar -- 2>stderr &&\n+\ttest_i18ngrep \"bad revision\" stderr\n+'\n+\n+test_expect_success 'arg before dashdash must be a revision (ambiguous)' '\n+\t>foobar &&\n+\tgit update-ref refs/heads/foobar HEAD &&\n+\t{\n+\t\t# we do not want to use rev-parse here, because\n+\t\t# we are testing it\n+\t\tcat .git/refs/heads/foobar &&\n+\t\tprintf \"%s\\n\" --\n+\t} >expect &&\n+\tgit rev-parse foobar -- >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.5.524.g6743da6\n"},{"id":"231676","messageId":"20131206211509.GB20536@sigill.intra.peff.net","threadId":"35483","inReplyTo":"20131206211222.GB20482@sigill.intra.peff.net","subject":"[PATCH 2/2] rev-parse: diagnose ambiguous revision/filename arguments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-06T21:15:09Z","receivedAt":"2013-12-06T21:15:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If you have both a file and a branch named \"foo\", running:\n\n git log foo\n\nwill complain. We should do the same in rev-parse, and\ndemand that it be disambiguated with:\n\n git rev-parse foo --\n\nor\n\n git rev-parse -- foo\n\nSigned-off-by: Jeff King \n---\nHmm, looking at this again, I guess we need to give the same treatment\nto the try_difference code path above, as \"foo..bar\" can be ambiguous\nwith a filename (and \"git log\" seems to diagnose that).\n\n builtin/rev-parse.c | 5 ++++-\n t/t1506-rev-parse-diagnosis.sh | 7 +++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex bb694ab..3ff4f8a 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -790,8 +790,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\tif (!get_sha1(name, sha1)) {\n \t\t\tif (verify)\n \t\t\t\trevs_count++;\n-\t\t\telse\n+\t\t\telse {\n \t\t\t\tshow_rev(type, sha1, name);\n+\t\t\t\tif (!has_dashdash)\n+\t\t\t\t\tverify_non_filename(prefix, arg);\n+\t\t\t}\n \t\t\tcontinue;\n \t\t}\n \t\tif (verify)\ndiff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\nindex 613d9bf..7ccbcf4 100755\n--- a/t/t1506-rev-parse-diagnosis.sh\n+++ b/t/t1506-rev-parse-diagnosis.sh\n@@ -220,4 +220,11 @@ test_expect_success 'arg before dashdash must be a revision (ambiguous)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'ambiguous arg without dashdash' '\n+\t>foobar &&\n+\tgit update-ref refs/heads/foobar HEAD &&\n+\ttest_must_fail git rev-parse foobar 2>stderr &&\n+\ttest_i18ngrep ambiguous stderr\n+'\n+\n test_done\n-- \n1.8.5.524.g6743da6\n"},{"id":"231678","messageId":"20131206220520.GA30652@sigill.intra.peff.net","threadId":"35483","inReplyTo":"20131206211509.GB20536@sigill.intra.peff.net","subject":"[PATCH v2 0/3] rev-parse and \"--\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-06T22:05:20Z","receivedAt":"2013-12-06T22:05:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 06, 2013 at 04:15:09PM -0500, Jeff King wrote:\n\n> If you have both a file and a branch named \"foo\", running:\n> \n> git log foo\n> \n> will complain. We should do the same in rev-parse, and\n> demand that it be disambiguated with:\n> \n> git rev-parse foo --\n> \n> or\n> \n> git rev-parse -- foo\n> \n> Signed-off-by: Jeff King \n> ---\n> Hmm, looking at this again, I guess we need to give the same treatment\n> to the try_difference code path above, as \"foo..bar\" can be ambiguous\n> with a filename (and \"git log\" seems to diagnose that).\n\nHere's a revised series which handles that:\n\n [1/3]: rev-parse: correctly diagnose revision errors before \"--\"\n [2/3]: rev-parse: be more careful with munging arguments\n [3/3]: rev-parse: diagnose ambiguous revision/filename arguments\n\nWhile fixing this commit, I noticed another bug whose fix is a\nprerequisite. That's now patch 2 in the series. I think the first two\npatches are correct and should be applied.\n\nPatch 3 is the revised version of this patch which notices ambiguity.\nHowever, I'm having second thoughts on it. I think it's the right thing\nto do if you want to help people build something like \"git log\"\nthemselves. But it does mean that we are breaking somebody who does:\n\n echo foo >HEAD\n commit=$(git rev-parse HEAD)\n\nI'm tempted to say that people who did that are stupid and wrong (and\nugly, too). They should probably be using \"--verify\" in this case. But\nit has been that way for a long time, and there are two instances in our\ntest scripts that are broken by the patch.\n\nIs it better for \"rev-parse\" to be more careful, and to behave more like\nthe rest of git? Or is better to be historically compatible?\n\nOne thing to note is that \"git rev-parse HEAD\" is slightly broken there\nalready. Because \"git rev-parse $some_branch\" may do very different\nthings than what the caller expects if $some_branch does not exist (but\nthere is a file with the same name). So maybe we are doing a favor by\ncalling out the problem; if they want a rev, they should be using\n\"--verify\" (or \"--\").\n\n-Peff\n"},{"id":"231679","messageId":"20131206220547.GA25620@sigill.intra.peff.net","threadId":"35483","inReplyTo":"20131206220520.GA30652@sigill.intra.peff.net","subject":"[PATCH v2 1/3] rev-parse: correctly diagnose revision errors before \"--\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-06T22:05:48Z","receivedAt":"2013-12-06T22:05:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Rev-parse understands that a \"--\" may separate revisions and\nfilenames, and that anything after the \"--\" is taken as-is.\nHowever, it does not understand that anything before the\ntoken must be a revision (which is the usual rule\nimplemented by the setup_revisions parser).\n\nSince rev-parse prefers revisions to files when parsing\nbefore the \"--\", we end up with the correct result (if such\nan argument is a revision, we parse it as one, and if it is\nnot, it is an error either way). However, we misdiagnose\nthe errors:\n\n $ git rev-parse foobar -- >/dev/null\n fatal: ambiguous argument 'foobar': unknown revision or path not in the working tree.\n Use '--' to separate paths from revisions, like this:\n 'git [...] -- [...]'\n\n $ >foobar\n $ git rev-parse foobar -- >/dev/null\n fatal: bad flag '--' used after filename\n\nIn both cases, we should know that the real error is that\n\"foobar\" is meant to be a revision, but could not be\nresolved.\n\nSigned-off-by: Jeff King \n---\n builtin/rev-parse.c | 10 ++++++++++\n t/t1506-rev-parse-diagnosis.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 34 insertions(+)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 1d9ecaf..bb694ab 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -488,6 +488,7 @@ N_(\"git rev-parse --parseopt [options] -- [...]\\n\"\n int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n {\n \tint i, as_is = 0, verify = 0, quiet = 0, revs_count = 0, type = 0;\n+\tint has_dashdash = 0;\n \tint output_prefix = 0;\n \tunsigned char sha1[20];\n \tconst char *name = NULL;\n@@ -501,6 +502,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \tif (argc > 1 && !strcmp(\"-h\", argv[1]))\n \t\tusage(builtin_rev_parse_usage);\n \n+\tfor (i = 1; i < argc; i++) {\n+\t\tif (!strcmp(argv[i], \"--\")) {\n+\t\t\thas_dashdash = 1;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n \tprefix = setup_git_directory();\n \tgit_config(git_default_config, NULL);\n \tfor (i = 1; i < argc; i++) {\n@@ -788,6 +796,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tif (verify)\n \t\t\tdie_no_single_rev(quiet);\n+\t\tif (has_dashdash)\n+\t\t\tdie(\"bad revision '%s'\", arg);\n \t\tas_is = 1;\n \t\tif (!show_file(arg, output_prefix))\n \t\t\tcontinue;\ndiff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\nindex f950c10..613d9bf 100755\n--- a/t/t1506-rev-parse-diagnosis.sh\n+++ b/t/t1506-rev-parse-diagnosis.sh\n@@ -196,4 +196,28 @@ test_expect_success 'dotdot is not an empty set' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'arg before dashdash must be a revision (missing)' '\n+\ttest_must_fail git rev-parse foobar -- 2>stderr &&\n+\ttest_i18ngrep \"bad revision\" stderr\n+'\n+\n+test_expect_success 'arg before dashdash must be a revision (file)' '\n+\t>foobar &&\n+\ttest_must_fail git rev-parse foobar -- 2>stderr &&\n+\ttest_i18ngrep \"bad revision\" stderr\n+'\n+\n+test_expect_success 'arg before dashdash must be a revision (ambiguous)' '\n+\t>foobar &&\n+\tgit update-ref refs/heads/foobar HEAD &&\n+\t{\n+\t\t# we do not want to use rev-parse here, because\n+\t\t# we are testing it\n+\t\tcat .git/refs/heads/foobar &&\n+\t\tprintf \"%s\\n\" --\n+\t} >expect &&\n+\tgit rev-parse foobar -- >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.5.524.g6743da6\n"},{"id":"231680","messageId":"20131206220751.GB25620@sigill.intra.peff.net","threadId":"35483","inReplyTo":"20131206220520.GA30652@sigill.intra.peff.net","subject":"[PATCH v2 2/3] rev-parse: be more careful with munging arguments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-06T22:07:52Z","receivedAt":"2013-12-06T22:07:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When rev-parse looks at whether an argument like \"foo..bar\"\nor \"foobar^@\" is a difference or parent-shorthand, it\ninternally munges the arguments so that it can pass the\nindividual rev arguments to get_sha1. However, we do not\nconsistently un-munge the result.\n\nFor cases where we do not match (e.g., \"doesnotexist..HEAD\"),\nwe wouuld then want to try to treat the argument as a\nfilename. try_difference gets this right, and always\nunmunges in this case. However, try_parent_shorthand never\nunmunges, leading to incorrect error messages, or even\nincorrect results:\n\n $ git rev-parse foobar^@\n foobar\n fatal: ambiguous argument 'foobar': unknown revision or path not in the working tree.\n Use '--' to separate paths from revisions, like this:\n 'git [...] -- [...]'\n\n $ >foobar\n $ git rev-parse foobar^@\n foobar\n\nFor cases where we do match, neither function unmunges. This\ndoes not currently matter, since we are done with the\nargument. However, a future patch will do further\nprocessing, and this prepares for it. In addition, it's\nsimply a confusing interface for some cases to modify the\nconst argument, and others not to.\n\nSigned-off-by: Jeff King \n---\nAs an aside, this is one of those places where C's string functions do\ngross things with const. We modify a const argument to the function\nwithout any casts or warnings, because the return value from strstr\nimplicitly drops the const. Time for us all to move to C++ with\ntemplated strstr, right?\n\n builtin/rev-parse.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex bb694ab..a60fcd3 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -286,6 +286,7 @@ static int try_difference(const char *arg)\n \t\t\t\texclude = n;\n \t\t\t}\n \t\t}\n+\t\t*dotdot = '.';\n \t\treturn 1;\n \t}\n \t*dotdot = '.';\n@@ -309,8 +310,10 @@ static int try_parent_shorthands(const char *arg)\n \t\treturn 0;\n \n \t*dotdot = 0;\n-\tif (get_sha1_committish(arg, sha1))\n+\tif (get_sha1_committish(arg, sha1)) {\n+\t\t*dotdot = '^';\n \t\treturn 0;\n+\t}\n \n \tif (!parents_only)\n \t\tshow_rev(NORMAL, sha1, arg);\n@@ -319,6 +322,7 @@ static int try_parent_shorthands(const char *arg)\n \t\tshow_rev(parents_only ? NORMAL : REVERSED,\n \t\t\t\tparents->item->object.sha1, arg);\n \n+\t*dotdot = '^';\n \treturn 1;\n }\n \n-- \n1.8.5.524.g6743da6\n"},{"id":"231681","messageId":"20131206220809.GC25620@sigill.intra.peff.net","threadId":"35483","inReplyTo":"20131206220520.GA30652@sigill.intra.peff.net","subject":"[PATCH v2 3/3] rev-parse: diagnose ambiguous revision/filename arguments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-06T22:08:09Z","receivedAt":"2013-12-06T22:08:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If you have both a file and a branch named \"foo\", running:\n\n git log foo\n\nwill complain. We should do the same in rev-parse, and\ndemand that it be disambiguated with:\n\n git rev-parse foo --\n\nor\n\n git rev-parse -- foo\n\nSigned-off-by: Jeff King \n---\n builtin/rev-parse.c | 12 ++++++++----\n t/t1506-rev-parse-diagnosis.sh | 22 ++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex a60fcd3..a3a58bf 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -781,10 +781,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t}\n \n \t\t/* Not a flag argument */\n-\t\tif (try_difference(arg))\n-\t\t\tcontinue;\n-\t\tif (try_parent_shorthands(arg))\n+\t\tif (try_difference(arg) || try_parent_shorthands(arg)) {\n+\t\t\tif (!has_dashdash)\n+\t\t\t\tverify_non_filename(prefix, arg);\n \t\t\tcontinue;\n+\t\t}\n \t\tname = arg;\n \t\ttype = NORMAL;\n \t\tif (*arg == '^') {\n@@ -794,8 +795,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\tif (!get_sha1(name, sha1)) {\n \t\t\tif (verify)\n \t\t\t\trevs_count++;\n-\t\t\telse\n+\t\t\telse {\n \t\t\t\tshow_rev(type, sha1, name);\n+\t\t\t\tif (!has_dashdash)\n+\t\t\t\t\tverify_non_filename(prefix, arg);\n+\t\t\t}\n \t\t\tcontinue;\n \t\t}\n \t\tif (verify)\ndiff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\nindex 613d9bf..dc1f1dc 100755\n--- a/t/t1506-rev-parse-diagnosis.sh\n+++ b/t/t1506-rev-parse-diagnosis.sh\n@@ -220,4 +220,26 @@ test_expect_success 'arg before dashdash must be a revision (ambiguous)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'ambiguous arg without dashdash (normal)' '\n+\t>foobar &&\n+\tgit update-ref refs/heads/foobar HEAD &&\n+\ttest_must_fail git rev-parse foobar 2>stderr &&\n+\ttest_i18ngrep ambiguous stderr\n+'\n+\n+test_expect_success 'ambiguous arg without dashdash (difference)' '\n+\t>one..two &&\n+\tgit update-ref refs/heads/one HEAD &&\n+\tgit update-ref refs/heads/two HEAD &&\n+\ttest_must_fail git rev-parse one..two 2>stderr &&\n+\ttest_i18ngrep ambiguous stderr\n+'\n+\n+test_expect_success 'ambiguous arg without dashdash (parents)' '\n+\t>\"foobar^@\" &&\n+\tgit update-ref refs/heads/foobar HEAD &&\n+\ttest_must_fail git rev-parse foobar^@ 2>stderr &&\n+\ttest_i18ngrep ambiguous stderr\n+'\n+\n test_done\n-- \n1.8.5.524.g6743da6\n"},{"id":"231682","messageId":"20131206221356.GD25620@sigill.intra.peff.net","threadId":"35483","inReplyTo":"CACsJy8CKDiCX0G3jD6KLZ=cyvp6zf4MT6DCY0XodfD8rkQF=wA@mail.gmail.com","subject":"Re: [BUG] redundant error message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-06T22:13:56Z","receivedAt":"2013-12-06T22:13:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 06, 2013 at 08:15:52AM +0700, Duy Nguyen wrote:\n\n> On Fri, Dec 6, 2013 at 4:28 AM, Jeff King wrote:\n> > BTW, the raw looping to find \"--\" made me wonder how we handle:\n> >\n> > git log --grep -- HEAD\n> >\n> > I'd expect it to be equivalent to:\n> >\n> > git log --grep=-- HEAD\n> >\n> > but it's not; we truncate the arguments and complain that --grep is\n> > missing its argument. Which is probably good enough, given that the\n> > alternative is doing a pass that understands all of the options. But it\n> > does mean that the \"--long-opt=arg\" form is safer than the split form if\n> > you are passing along an arbitrary \"arg\".\n> \n> Maybe we could make setup_revisions() use parse_options() someday,\n> which understands about arguments and dashdash.\n> \n> $ ./git grep -e -- foo\n> fatal: ambiguous argument 'foo': both revision and filename\n> Use '--' to separate paths from revisions, like this:\n> 'git [...] -- [...]'\n> $ ./git grep -e -- -- foo\n\nYes, although we use it some in handle_revision_opt, I believe. The\nproblem isn't inherent to parse_options or not, though. To do it\ncorrectly, we need to either:\n\n 1. make two passes with the code that actually understands the options\n (be it parse_options or not); the first looking for \"--\", and the\n second to do the actual parsing. Right now our first pass does not\n understand the options at all.\n\n 2. store the non-option arguments (including \"--\"), and only resolve\n and verify them after we have gone through the whole command-line\n and know whether we hit a \"--\" or not.\n\nI suspect the second option would be simpler, as neither parse-options\nnor the current revision code is safe to run through twice (e.g.,\nparse-options may have callbacks that add to a list, and we would need\nto add some kind of \"dry-run\" flag).\n\nIt's something that would be nice to fix, but I don't see myself working\non it anytime soon. It's a lot of work for very little benefit.\n\n-Peff\n"},{"id":"231686","messageId":"20131206232556.GN29959@google.com","threadId":"35483","inReplyTo":"20131206220520.GA30652@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/3] rev-parse and \"--\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-06T23:25:56Z","receivedAt":"2013-12-06T23:25:56Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Patch 3 is the revised version of this patch which notices ambiguity.\n> However, I'm having second thoughts on it. I think it's the right thing\n> to do if you want to help people build something like \"git log\"\n> themselves. But it does mean that we are breaking somebody who does:\n>\n> echo foo >HEAD\n> commit=$(git rev-parse HEAD)\n>\n> I'm tempted to say that people who did that are stupid and wrong (and\n> ugly, too). They should probably be using \"--verify\" in this case. But\n> it has been that way for a long time, and there are two instances in our\n> test scripts that are broken by the patch.\n\nWouldn't the same thing happen for $(git rev-parse HEAD^..HEAD) when\nthere is a 'HEAD^..HEAD' file, too? --verify doesn't support that.\n\nI think it's a nice idea to provide this functionality for people who\nneed it, which would mean protected by an option (--check-ambiguity or\nsomething). I don't think it's a good change to do it\nunconditionally.\n\nMy two cents,\nJonathan\n"},{"id":"231687","messageId":"20131206233004.GA29524@sigill.intra.peff.net","threadId":"35483","inReplyTo":"20131206232556.GN29959@google.com","subject":"Re: [PATCH v2 0/3] rev-parse and \"--\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-06T23:30:04Z","receivedAt":"2013-12-06T23:30:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 06, 2013 at 03:25:56PM -0800, Jonathan Nieder wrote:\n\n> > commit=$(git rev-parse HEAD)\n> >\n> > I'm tempted to say that people who did that are stupid and wrong (and\n> > ugly, too). They should probably be using \"--verify\" in this case. But\n> > it has been that way for a long time, and there are two instances in our\n> > test scripts that are broken by the patch.\n> \n> Wouldn't the same thing happen for $(git rev-parse HEAD^..HEAD) when\n> there is a 'HEAD^..HEAD' file, too? --verify doesn't support that.\n\nTrue, but they can use \"rev-parse HEAD^..HEAD --\", which is probably a\nbetter alternative to --verify.\n\n> I think it's a nice idea to provide this functionality for people who\n> need it, which would mean protected by an option (--check-ambiguity or\n> something). I don't think it's a good change to do it\n> unconditionally.\n\nI don't think it's worth making it conditional. Literally nobody has\nasked for it. I was mostly trying to harmonize \"rev-parse\" and the rest\nof the revision parser. If that's not worth doing, I'd just as soon drop\nit (and people using \"--\" properly already get the right behavior).\n\n-Peff\n"},{"id":"231688","messageId":"20131206233410.GO29959@google.com","threadId":"35483","inReplyTo":"20131206220547.GA25620@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/3] rev-parse: correctly diagnose revision errors before \"--\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-06T23:34:10Z","receivedAt":"2013-12-06T23:34:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Since rev-parse prefers revisions to files when parsing\n> before the \"--\", we end up with the correct result (if such\n> an argument is a revision, we parse it as one, and if it is\n> not, it is an error either way). However, we misdiagnose\n> the errors:\n>\n> $ git rev-parse foobar -- >/dev/null\n> fatal: ambiguous argument 'foobar': unknown revision or path not in the working tree.\n> Use '--' to separate paths from revisions, like this:\n> 'git [...] -- [...]'\n>\n> $ >foobar\n> $ git rev-parse foobar -- >/dev/null\n> fatal: bad flag '--' used after filename\n>\n> In both cases, we should know that the real error is that\n> \"foobar\" is meant to be a revision, but could not be\n> resolved.\n\nNeat.\n\n[...]\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -488,6 +488,7 @@ N_(\"git rev-parse --parseopt [options] -- [...]\\n\"\n> int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> {\n> \tint i, as_is = 0, verify = 0, quiet = 0, revs_count = 0, type = 0;\n> +\tint has_dashdash = 0;\n> \tint output_prefix = 0;\n> \tunsigned char sha1[20];\n> \tconst char *name = NULL;\n> @@ -501,6 +502,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> \tif (argc > 1 && !strcmp(\"-h\", argv[1]))\n> \t\tusage(builtin_rev_parse_usage);\n> \n> +\tfor (i = 1; i < argc; i++) {\n> +\t\tif (!strcmp(argv[i], \"--\")) {\n> +\t\t\thas_dashdash = 1;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> \tprefix = setup_git_directory();\n> \tgit_config(git_default_config, NULL);\n> \tfor (i = 1; i < argc; i++) {\n> @@ -788,6 +796,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> \t\t}\n> \t\tif (verify)\n> \t\t\tdie_no_single_rev(quiet);\n> +\t\tif (has_dashdash)\n> +\t\t\tdie(\"bad revision '%s'\", arg);\n> \t\tas_is = 1;\n\nYep, this is the \"fall back to looking for a file\" part of rev-parse,\nso erroring out if there as a dashdash coming is the right thing to\ndo. And a quick code search for \"rev-parse.*\\ --\\ \" reveals that\nmost callers would simply not be affected by this.\n\nThanks for a pleasant patch.\n\nFor what it's worth,\nReviewed-by: Jonathan Nieder \n"},{"id":"231690","messageId":"20131207000437.GP29959@google.com","threadId":"35483","inReplyTo":"20131206220751.GB25620@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/3] rev-parse: be more careful with munging arguments","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-07T00:04:37Z","receivedAt":"2013-12-07T00:04:37Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> As an aside, this is one of those places where C's string functions do\n> gross things with const.\n\nYes, yuck.\n\nThe fundamental grossness is that argv is semantically char **\n(assuming this doesn't segfault) but passed around as const char **.\nI wonder why we don't use the same trick in git_extract_argv0_path and\nwhen handling STRIP_EXTENSION in git.c.\n\nAlternatively, try_difference could create a new buffer for its own\nuse. Originally (v0.99~273, 2005-06-13) this dotdot handling code was\npretty simple:\n\n\tfor (i = 1; i < argc; i++) {\n\t\tchar *arg = argv[i];\n\n\t\t...\n\n\t\tdotdot = strstr(arg, \"..\");\n\t\tif (dotdot) {\n\t\t\tunsigned char end[20];\n\t\t\tchar *n = dotdot+2;\n\t\t\t*dotdot = 0;\n\t\t\tif (!get_sha1(arg, sha1)) {\n\t\t\t\tif (!*n)\n\t\t\t\t\tn = \"HEAD\";\n\t\t\t\tif (!get_sha1(n, end)) {\n\t\t\t\t\tprintf(\"%s\\n\", sha1_to_hex(end));\n\t\t\t\t\tprintf(\"^%s\\n\", sha1_to_hex(sha1));\n\t\t\t\t\tcontinue;\n\t\t\t\t}\n\t\t\t}\n\t\t\t*dotdot = '.';\n\t\t}\n\t\tprintf(\"%s\\n\", arg);\n\t}\n\nThe 'continue' without making the argument right again was justified\nbecause we were done with the argument.\n\nNow it is still not much more complicated --- the function is\nlogically just trying to parse the argument into a \"this\" and \"next\"\npart and print them.\n\nSomething like the following (untested). Or if the allocation churn\nis too much, it could use a static strbuf.\n\ndiff --git i/builtin/rev-parse.c w/builtin/rev-parse.c\nindex c76b89d..98262b3 100644\n--- i/builtin/rev-parse.c\n+++ w/builtin/rev-parse.c\n@@ -234,37 +234,35 @@ static int try_difference(const char *arg)\n \tunsigned char sha1[20];\n \tunsigned char end[20];\n \tconst char *next;\n-\tconst char *this;\n+\tstruct strbuf this = STRBUF_INIT;\n \tint symmetric;\n-\tstatic const char head_by_default[] = \"HEAD\";\n \n \tif (!(dotdot = strstr(arg, \"..\")))\n \t\treturn 0;\n \tnext = dotdot + 2;\n-\tthis = arg;\n \tsymmetric = (*next == '.');\n-\n-\t*dotdot = 0;\n \tnext += symmetric;\n \n-\tif (!*next)\n-\t\tnext = head_by_default;\n-\tif (dotdot == arg)\n-\t\tthis = head_by_default;\n-\n-\tif (this == head_by_default && next == head_by_default &&\n-\t !symmetric) {\n+\tif (!*next && dotdot == arg && !symmetric)\n \t\t/*\n \t\t * Just \"..\"? That is not a range but the\n \t\t * pathspec for the parent directory.\n \t\t */\n-\t\t*dotdot = '.';\n \t\treturn 0;\n+\n+\tstrbuf_add(&this, arg, dotdot - arg);\n+\n+\tif (!*next)\n+\t\tnext = \"HEAD\";\n+\tif (dotdot == arg) {\n+\t\tstrbuf_reset(&this);\n+\t\tstrbuf_addstr(&this, \"HEAD\");\n \t}\n \n-\tif (!get_sha1_committish(this, sha1) && !get_sha1_committish(next, end)) {\n+\tif (!get_sha1_committish(this.buf, sha1)\n+\t && !get_sha1_committish(next, end)) {\n \t\tshow_rev(NORMAL, end, next);\n-\t\tshow_rev(symmetric ? NORMAL : REVERSED, sha1, this);\n+\t\tshow_rev(symmetric ? NORMAL : REVERSED, sha1, this.buf);\n \t\tif (symmetric) {\n \t\t\tstruct commit_list *exclude;\n \t\t\tstruct commit *a, *b;\n@@ -279,9 +277,10 @@ static int try_difference(const char *arg)\n \t\t\t\texclude = n;\n \t\t\t}\n \t\t}\n+\t\tstrbuf_release(&this);\n \t\treturn 1;\n \t}\n-\t*dotdot = '.';\n+\tstrbuf_release(&this);\n \treturn 0;\n }\n \n"},{"id":"231795","messageId":"xmqqmwk9zvyy.fsf@gitster.dls.corp.google.com","threadId":"35483","inReplyTo":"20131206220520.GA30652@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/3] rev-parse and \"--\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-09T19:05:09Z","receivedAt":"2013-12-09T19:05:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Is it better for \"rev-parse\" to be more careful, and to behave more like\n> the rest of git? Or is better to be historically compatible?\n>\n> One thing to note is that \"git rev-parse HEAD\" is slightly broken there\n> already. Because \"git rev-parse $some_branch\" may do very different\n> things than what the caller expects if $some_branch does not exist (but\n> there is a file with the same name). So maybe we are doing a favor by\n> calling out the problem; if they want a rev, they should be using\n> \"--verify\" (or \"--\").\n\nI tend to agree with the reasoning in the last sentence. Let's cook\nit for a while and see what happens.\n\nThanks.\n"},{"id":"231797","messageId":"20131209191224.GR29959@google.com","threadId":"35483","inReplyTo":"xmqqmwk9zvyy.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 0/3] rev-parse and \"--\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-09T19:12:24Z","receivedAt":"2013-12-09T19:12:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n>> So maybe we are doing a favor by\n>> calling out the problem; if they want a rev, they should be using\n>> \"--verify\" (or \"--\").\n>\n> I tend to agree with the reasoning in the last sentence. Let's cook\n> it for a while and see what happens.\n\nIsn't this essentially breaking a contract that would have been relied\non by any script that used \"git rev-parse HEAD~3..HEAD\"? Worse, it's\nbreaking that contract in a way that no one would notice until they\nare asked to manipulate a worktree with a file named 'HEAD~3..HEAD'\n--- in other words, the breakage it introduces is painfully subtle.\n\nI agree that \"git rev-parse HEAD\" is better written as \"git rev-parse\n--verify HEAD\" and hence not so much worth worrying about, but I don't\nfind it easy to believe that people should have anticipated this\nchange and added a trailing \"--\" to more complex rev-parse\nexpressions.\n\nSo to be clear, I think unless it is protected by a new option, this\nis a bad idea.\n\nHope that helps,\nJonathan\n"},{"id":"231801","messageId":"20131209192334.GT29959@google.com","threadId":"35483","inReplyTo":"20131209191224.GR29959@google.com","subject":"Re: [PATCH v2 0/3] rev-parse and \"--\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-09T19:23:34Z","receivedAt":"2013-12-09T19:23:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> Isn't this essentially breaking a contract\n\nTo clarify, if there were some strong motivation --- e.g. if this\nwere resolving some security problem --- then I would be all for\nbreaking compatibility in this way. What's confusing to me is that I\njust don't see the motivation. Why wouldn't someone that wants this\nfunctionality be able to pass a flag for it?\n\nIs the idea that without a flag it's easier to teach or something?\n\nHoping that clarifies,\nJonathan\n"},{"id":"231811","messageId":"xmqqob4pycmv.fsf@gitster.dls.corp.google.com","threadId":"35483","inReplyTo":"20131209191224.GR29959@google.com","subject":"Re: [PATCH v2 0/3] rev-parse and \"--\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-09T20:48:08Z","receivedAt":"2013-12-09T20:48:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Junio C Hamano wrote:\n>\n>>> So maybe we are doing a favor by\n>>> calling out the problem; if they want a rev, they should be using\n>>> \"--verify\" (or \"--\").\n>>\n>> I tend to agree with the reasoning in the last sentence. Let's cook\n>> it for a while and see what happens.\n>\n> Isn't this essentially breaking a contract that would have been relied\n> on by any script that used \"git rev-parse HEAD~3..HEAD\"? Worse, it's\n> breaking that contract in a way that no one would notice until they\n> are asked to manipulate a worktree with a file named 'HEAD~3..HEAD'\n> --- in other words, the breakage it introduces is painfully subtle.\n>\n> I agree that \"git rev-parse HEAD\" is better written as \"git rev-parse\n> --verify HEAD\" and hence not so much worth worrying about,\n\nI do not share the \"with --verify is better hence no problem\"\nreasoning. My \"not so much worth worrying about\" comes solely from\na hunch that nobody has \"HEAD~3..HEAD\" in their working directory,\nand if somebody has one, then they must be using \"--verify\" (or a\nclarifying \"--\"), because their \"git log\" and whatever they use \"git\nrev-parse HEAD~3..HEAD\" for would behave very differently otherwise.\n\nSo it is not merely \"--verify is better\"---in a situation where the\nbackward incompatibility matters, I doubt the existing behaves\nsensibly in the first place.\n\nBut if we cook it for a while, I suspect that we will find more and\nmore breakages of expectations in the existing scripts in and out of\nthe tree; in general, I hate _any_ change, and I do not mind\ndropping it ;-).\n"},{"id":"231815","messageId":"20131209205621.GW29959@google.com","threadId":"35483","inReplyTo":"xmqqob4pycmv.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 0/3] rev-parse and \"--\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-09T20:56:21Z","receivedAt":"2013-12-09T20:56:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> I do not share the \"with --verify is better hence no problem\"\n> reasoning. My \"not so much worth worrying about\" comes solely from\n> a hunch that nobody has \"HEAD~3..HEAD\" in their working directory,\n\nThat's what makes it a problem. This change makes it very easy to\nmake a general-purpose script that breaks in an edge case that the\nscript's author is not likely to run into. Then as soon as someone\nadds a file with such a name to the test data in their repo, their\nfavorite general-purpose repo munger just breaks.\n\nIf we wanted to forbid git from tracking files named HEAD~3..HEAD\naltogether, that would be a different story.\n\n> and if somebody has one, then they must be using \"--verify\" (or a\n> clarifying \"--\"), because their \"git log\" and whatever they use \"git\n> rev-parse HEAD~3..HEAD\" for would behave very differently otherwise.\n\nIsn't protecting against this kind of thing the reason we ask authors\nof general-purpose scripts to use \"simple, do what I say and not what\nI mean\" plumbing commands?\n\nAnother relevant detail is that using rev-parse with \"--\" is more\npainful than without, since it includes the \"--\" in its output.\nWithout this change, it seems much more likely to me that someone\nwould do\n\n\tgit rev-parse |\n\twhile read commit\n\tdo\n\t\t...\n\tdone\n\nthan\n\n\tgit rev-parse -- |\n\twhile read commit\n\tdo\n\t\tif test \"$commit\" = \"--\"\n\t\tthen\n\t\t\tcontinue\n\t\tfi\n\n\t\t...\n\tdone\n\n> So it is not merely \"--verify is better\"---in a situation where the\n> backward incompatibility matters, I doubt the existing behaves\n> sensibly in the first place.\n\nWhat in the former of the above two loops is broken?\n\n> But if we cook it for a while, I suspect that we will find more and\n> more breakages of expectations in the existing scripts in and out of\n> the tree;\n\nAlas, probably no, because nobody has \"HEAD~3..HEAD\" in their working\ndirectory. That's exactly the problem --- it creates an edge case\nthat nobody is likely to test until the once-in-a-few-years moment\nwhen they need it.\n\nJonathan\n"},{"id":"231820","messageId":"xmqqvbyxwx1q.fsf@gitster.dls.corp.google.com","threadId":"35483","inReplyTo":"20131209205621.GW29959@google.com","subject":"Re: [PATCH v2 0/3] rev-parse and \"--\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-09T21:10:09Z","receivedAt":"2013-12-09T21:10:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>> But if we cook it for a while, I suspect that we will find more and\n>> more breakages of expectations in the existing scripts in and out of\n>> the tree;\n>\n> Alas, probably no, because nobody has \"HEAD~3..HEAD\" in their working\n> directory. That's exactly the problem --- it creates an edge case\n> that nobody is likely to test...\n\nI didn't mean to say that running and encoutering the issue is the\nonly way to find \"more breakges of expectations\", by the way. I am\nstill on the fence, but leaning towards not merging it too hastily.\n"},{"id":"231828","messageId":"CAPig+cSSherOy0HXC-E0bRcW=+M370ZoDP7CbW8bKvwSs5imYA@mail.gmail.com","threadId":"35483","inReplyTo":"20131206220751.GB25620@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/3] rev-parse: be more careful with munging arguments","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-12-09T21:33:42Z","receivedAt":"2013-12-09T21:33:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 6, 2013 at 5:07 PM, Jeff King wrote:\n> When rev-parse looks at whether an argument like \"foo..bar\"\n> or \"foobar^@\" is a difference or parent-shorthand, it\n> internally munges the arguments so that it can pass the\n> individual rev arguments to get_sha1. However, we do not\n> consistently un-munge the result.\n>\n> For cases where we do not match (e.g., \"doesnotexist..HEAD\"),\n> we wouuld then want to try to treat the argument as a\n\ns/wouuld/would/\n\n> filename. try_difference gets this right, and always\n> unmunges in this case. However, try_parent_shorthand never\n> unmunges, leading to incorrect error messages, or even\n> incorrect results:\n>\n> $ git rev-parse foobar^@\n> foobar\n> fatal: ambiguous argument 'foobar': unknown revision or path not in the working tree.\n> Use '--' to separate paths from revisions, like this:\n> 'git [...] -- [...]'\n>\n> $ >foobar\n> $ git rev-parse foobar^@\n> foobar\n>\n> For cases where we do match, neither function unmunges. This\n> does not currently matter, since we are done with the\n> argument. However, a future patch will do further\n> processing, and this prepares for it. In addition, it's\n> simply a confusing interface for some cases to modify the\n> const argument, and others not to.\n>\n> Signed-off-by: Jeff King \n"}]}