{"thread":{"id":"27240","subject":"[Annoyance] \"git log ..\" thinks \"..\" is ambiguous","startedAt":"2011-05-02T19:15:23Z","lastAt":"2011-05-04T18:00:35Z","messageCount":19,"participants":["Junio C Hamano","Jeff King","Joshua Juran","Michael J Gruber","John Szakmeister"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"166896","messageId":"7vr58glxro.fsf@alter.siamese.dyndns.org","threadId":"27240","inReplyTo":null,"subject":"[Annoyance] \"git log ..\" thinks \"..\" is ambiguous","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-02T19:15:23Z","receivedAt":"2011-05-02T19:15:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Either end of revision range operator can be omitted to default to\nHEAD, as in \"origin..\" (what did I do since I forked) or \"..origin\" (what\ndid they do since I forked). But this resulted in \"..\" to be interpreted\nas an empty range \"HEAD..HEAD\", and worse yet, because \"..\" does exist on\nthe filesystem, we get this annoying output:\n\n $ cd Documentation/howto\n $ git log .. ;# give me recent commits that touch Documentation/ area.\n fatal: ambiguous argument '..': both revision and filename\n Use '--' to separate filenames from revisions\n\nSurely we could say \"git log .. --\", but we shouldn't have to.\n\n---\n * I don't have very strong feeling about this, but just got annoyed\n enough to write this patch. Should apply cleanly all the way down to\n at least 1.6.0 if not earlier.\n\n revision.c | 16 ++++++++++++++--\n 1 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex f4b8b38..26271d1 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -905,14 +905,26 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,\n \t\tconst char *this = arg;\n \t\tint symmetric = *next == '.';\n \t\tunsigned int flags_exclude = flags ^ UNINTERESTING;\n+\t\tstatic const char head_by_default[] = \"HEAD\";\n \n \t\t*dotdot = 0;\n \t\tnext += symmetric;\n \n \t\tif (!*next)\n-\t\t\tnext = \"HEAD\";\n+\t\t\tnext = head_by_default;\n \t\tif (dotdot == arg)\n-\t\t\tthis = \"HEAD\";\n+\t\t\tthis = head_by_default;\n+\t\tif (this == head_by_default && next == head_by_default &&\n+\t\t !symmetric) {\n+\t\t\t/*\n+\t\t\t * Just \"..\"? That is not a range but the\n+\t\t\t * pathspec for the parent directory.\n+\t\t\t */\n+\t\t\tif (!cant_be_filename) {\n+\t\t\t\t*dotdot = '.';\n+\t\t\t\treturn -1;\n+\t\t\t}\n+\t\t}\n \t\tif (!get_sha1(this, from_sha1) &&\n \t\t !get_sha1(next, sha1)) {\n \t\t\tstruct commit *a, *b;\n"},{"id":"166900","messageId":"20110502193321.GB10487@sigill.intra.peff.net","threadId":"27240","inReplyTo":"7vr58glxro.fsf@alter.siamese.dyndns.org","subject":"Re: [Annoyance] \"git log ..\" thinks \"..\" is ambiguous","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-02T19:33:21Z","receivedAt":"2011-05-02T19:33:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 02, 2011 at 12:15:23PM -0700, Junio C Hamano wrote:\n\n> Either end of revision range operator can be omitted to default to\n> HEAD, as in \"origin..\" (what did I do since I forked) or \"..origin\" (what\n> did they do since I forked). But this resulted in \"..\" to be interpreted\n> as an empty range \"HEAD..HEAD\", and worse yet, because \"..\" does exist on\n> the filesystem, we get this annoying output:\n> \n> $ cd Documentation/howto\n> $ git log .. ;# give me recent commits that touch Documentation/ area.\n> fatal: ambiguous argument '..': both revision and filename\n> Use '--' to separate filenames from revisions\n> \n> Surely we could say \"git log .. --\", but we shouldn't have to.\n\nI got slightly confused reading this, because I thought at first you\nwanted \"..\" to be some kind of magic rev specifier. In particular, your\nlast line should say:\n\n git log -- ..\n\nno?\n\nI finally figured it out by reading your example carefully.\n\nAs far as the patch goes, I think it is the right thing to do. We should\nprobably disallow \"..\" as a revision (because it is nonsensical to ask\nfor, just as it would be to ask for a non-existent ref). And then the\nonly sane thing is to consider it as a path.\n\nI didn't check, but what does your patch do with:\n\n $ cd \"`git rev-parse --show-toplevel`\"\n $ git log ..\n\n?\n\n-Peff\n"},{"id":"166902","messageId":"20110502193651.GC10487@sigill.intra.peff.net","threadId":"27240","inReplyTo":"20110502193321.GB10487@sigill.intra.peff.net","subject":"Re: [Annoyance] \"git log ..\" thinks \"..\" is ambiguous","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-02T19:36:51Z","receivedAt":"2011-05-02T19:36:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 02, 2011 at 03:33:21PM -0400, Jeff King wrote:\n\n> I didn't check, but what does your patch do with:\n> \n> $ cd \"`git rev-parse --show-toplevel`\"\n> $ git log ..\n\nNevermind, it properly complains \"'..' is outside repository\"; of course\nno new code is needed because we could already do \"git log ../..\".\n\nI do notice that gitk doesn't work, though. Doing \"gitk ..\" produces an\n\"ambiguous argument\" error. It looks like \"git rev-parse ..\" still\ntreats it as HEAD..HEAD, which may be the source of the error.\n\n-Peff\n"},{"id":"166905","messageId":"7vhb9clu0n.fsf@alter.siamese.dyndns.org","threadId":"27240","inReplyTo":"20110502193321.GB10487@sigill.intra.peff.net","subject":"Re: [Annoyance] \"git log ..\" thinks \"..\" is ambiguous","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-02T20:36:24Z","receivedAt":"2011-05-02T20:36:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, May 02, 2011 at 12:15:23PM -0700, Junio C Hamano wrote:\n>\n>> Either end of revision range operator can be omitted to default to\n>> HEAD, as in \"origin..\" (what did I do since I forked) or \"..origin\" (what\n>> did they do since I forked). But this resulted in \"..\" to be interpreted\n>> as an empty range \"HEAD..HEAD\", and worse yet, because \"..\" does exist on\n>> the filesystem, we get this annoying output:\n>> \n>> $ cd Documentation/howto\n>> $ git log .. ;# give me recent commits that touch Documentation/ area.\n>> fatal: ambiguous argument '..': both revision and filename\n>> Use '--' to separate filenames from revisions\n>> \n>> Surely we could say \"git log .. --\", but we shouldn't have to.\n>\n> I got slightly confused reading this, because I thought at first you\n> wanted \"..\" to be some kind of magic rev specifier. In particular, your\n> last line should say:\n>\n> git log -- ..\n>\n> no?\n\nYeah, sorry about that. Or \"git log ../\".\n\nAnd a matching \"rev-parse\" update should look like this. I wonder if we\ncould share some code between the two, though...\n\n Documentation/revisions.txt | 7 +++++++\n builtin/rev-parse.c | 16 ++++++++++++++--\n 2 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/revisions.txt b/Documentation/revisions.txt\nindex b290b61..ceed820 100644\n--- a/Documentation/revisions.txt\n+++ b/Documentation/revisions.txt\n@@ -213,6 +213,13 @@ of 'r1' and 'r2' and is defined as\n It is the set of commits that are reachable from either one of\n 'r1' or 'r2' but not from both.\n \n+In these two shorthands, you can omit one end and let it default to HEAD.\n+For example, 'origin..' is a shorthand for 'origin..HEAD' and asks \"What\n+did I do since I forked from the origin branch?\" Similarly, '..origin'\n+is a shorthand for 'HEAD..origin' and asks \"What did the origin do since\n+I forked from them?\" Note that you cannot omit both ends. '..' is not\n+an empty range that is both reachable and unreachable from HEAD.\n+\n Two other shorthands for naming a set that is formed by a commit\n and its parent commits exist. The 'r1{caret}@' notation means all\n parents of 'r1'. 'r1{caret}!' includes commit 'r1' but excludes\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex adb1cae..3c8f171 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -223,6 +223,7 @@ static int try_difference(const char *arg)\n \tconst char *next;\n \tconst char *this;\n \tint symmetric;\n+\tstatic const char head_by_default[] = \"HEAD\";\n \n \tif (!(dotdot = strstr(arg, \"..\")))\n \t\treturn 0;\n@@ -234,9 +235,20 @@ static int try_difference(const char *arg)\n \tnext += symmetric;\n \n \tif (!*next)\n-\t\tnext = \"HEAD\";\n+\t\tnext = head_by_default;\n \tif (dotdot == arg)\n-\t\tthis = \"HEAD\";\n+\t\tthis = head_by_default;\n+\n+\tif (this == head_by_default && next == head_by_default &&\n+\t !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+\t}\n+\n \tif (!get_sha1(this, sha1) && !get_sha1(next, end)) {\n \t\tshow_rev(NORMAL, end, next);\n \t\tshow_rev(symmetric ? NORMAL : REVERSED, sha1, this);\n"},{"id":"166906","messageId":"20110502204235.GA13584@sigill.intra.peff.net","threadId":"27240","inReplyTo":"7vhb9clu0n.fsf@alter.siamese.dyndns.org","subject":"Re: [Annoyance] \"git log ..\" thinks \"..\" is ambiguous","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-02T20:42:35Z","receivedAt":"2011-05-02T20:42:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 02, 2011 at 01:36:24PM -0700, Junio C Hamano wrote:\n\n> And a matching \"rev-parse\" update should look like this. I wonder if we\n> could share some code between the two, though...\n> \n> Documentation/revisions.txt | 7 +++++++\n> builtin/rev-parse.c | 16 ++++++++++++++--\n> 2 files changed, 21 insertions(+), 2 deletions(-)\n\nThe doc update looks good to me. The duplication in rev-parse is pretty\ngross, but that is certainly not a problem you are introducing. It would\nbe nice to clean it up, but I don't know how painful that would be. It\nseems like rev-parse should just be calling handle_revision_arg, but I'm\nsure there are some nasty corner cases. IOW, I am not volunteering. :)\n\n-Peff\n"},{"id":"166907","messageId":"7v62pslt2k.fsf_-_@alter.siamese.dyndns.org","threadId":"27240","inReplyTo":"7vhb9clu0n.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] specifying ranges: we did not mean to make \"..\" an empty set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-02T20:56:51Z","receivedAt":"2011-05-02T20:56:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Either end of revision range operator can be omitted to default to HEAD,\nas in \"origin..\" (what did I do since I forked) or \"..origin\" (what did\nthey do since I forked). But the current parser interprets \"..\" as an\nempty range \"HEAD..HEAD\", and worse yet, because \"..\" does exist on the\nfilesystem, we get this annoying output:\n\n $ cd Documentation/howto\n $ git log .. ;# give me recent commits that touch Documentation/ area.\n fatal: ambiguous argument '..': both revision and filename\n Use '--' to separate filenames from revisions\n\nSurely we could say \"git log ../\" or even \"git log -- ..\" to disambiguate,\nbut we shouldn't have to.\n\nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n\n * With docs and tests. In an early version, I forgot to restore the\n revision string that is temporarily NUL terminated with *dotdot = '.', \n which resulted in \"git log ..\" from Doc/howto to show not just the log\n of Doc but show everything. The new test in t4202 tries to catch that\n rather common mistake as well.\n\n Documentation/revisions.txt | 7 +++++++\n builtin/rev-parse.c | 16 ++++++++++++++--\n revision.c | 16 ++++++++++++++--\n t/t1506-rev-parse-diagnosis.sh | 14 ++++++++++++++\n t/t4202-log.sh | 7 +++++++\n 5 files changed, 56 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/revisions.txt b/Documentation/revisions.txt\nindex 3d4b79c..fd043f7 100644\n--- a/Documentation/revisions.txt\n+++ b/Documentation/revisions.txt\n@@ -184,6 +184,13 @@ of `r1` and `r2` and is defined as\n It is the set of commits that are reachable from either one of\n `r1` or `r2` but not from both.\n \n+In these two shorthands, you can omit one end and let it default to HEAD.\n+For example, 'origin..' is a shorthand for 'origin..HEAD' and asks \"What\n+did I do since I forked from the origin branch?\" Similarly, '..origin'\n+is a shorthand for 'HEAD..origin' and asks \"What did the origin do since\n+I forked from them?\" Note that you cannot omit both ends. '..' is not\n+an empty range that is both reachable and unreachable from HEAD.\n+\n Two other shorthands for naming a set that is formed by a commit\n and its parent commits exist. The `r1{caret}@` notation means all\n parents of `r1`. `r1{caret}!` includes commit `r1` but excludes\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex a5a1c86..54ce44b 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -219,6 +219,7 @@ static int try_difference(const char *arg)\n \tconst char *next;\n \tconst char *this;\n \tint symmetric;\n+\tstatic const char head_by_default[] = \"HEAD\";\n \n \tif (!(dotdot = strstr(arg, \"..\")))\n \t\treturn 0;\n@@ -230,9 +231,20 @@ static int try_difference(const char *arg)\n \tnext += symmetric;\n \n \tif (!*next)\n-\t\tnext = \"HEAD\";\n+\t\tnext = head_by_default;\n \tif (dotdot == arg)\n-\t\tthis = \"HEAD\";\n+\t\tthis = head_by_default;\n+\n+\tif (this == head_by_default && next == head_by_default &&\n+\t !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+\t}\n+\n \tif (!get_sha1(this, sha1) && !get_sha1(next, end)) {\n \t\tshow_rev(NORMAL, end, next);\n \t\tshow_rev(symmetric ? NORMAL : REVERSED, sha1, this);\ndiff --git a/revision.c b/revision.c\nindex ded8812..4572aaf 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1004,14 +1004,26 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,\n \t\tconst char *this = arg;\n \t\tint symmetric = *next == '.';\n \t\tunsigned int flags_exclude = flags ^ UNINTERESTING;\n+\t\tstatic const char head_by_default[] = \"HEAD\";\n \n \t\t*dotdot = 0;\n \t\tnext += symmetric;\n \n \t\tif (!*next)\n-\t\t\tnext = \"HEAD\";\n+\t\t\tnext = head_by_default;\n \t\tif (dotdot == arg)\n-\t\t\tthis = \"HEAD\";\n+\t\t\tthis = head_by_default;\n+\t\tif (this == head_by_default && next == head_by_default &&\n+\t\t !symmetric) {\n+\t\t\t/*\n+\t\t\t * Just \"..\"? That is not a range but the\n+\t\t\t * pathspec for the parent directory.\n+\t\t\t */\n+\t\t\tif (!cant_be_filename) {\n+\t\t\t\t*dotdot = '.';\n+\t\t\t\treturn -1;\n+\t\t\t}\n+\t\t}\n \t\tif (!get_sha1(this, from_sha1) &&\n \t\t !get_sha1(next, sha1)) {\n \t\t\tstruct commit *a, *b;\ndiff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\nindex 0eeeb0e..4485d0a 100755\n--- a/t/t1506-rev-parse-diagnosis.sh\n+++ b/t/t1506-rev-parse-diagnosis.sh\n@@ -75,4 +75,18 @@ test_expect_success 'invalid @{n} reference' '\n \tgrep \"fatal: Log for [^ ]* only has [0-9][0-9]* entries.\" error\n '\n \n+test_expect_success 'dotdot is not an empty set' '\n+\t( H=$(git rev-parse HEAD) && echo $H ; echo ^$H ) >expect &&\n+\n+\tgit rev-parse HEAD.. >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit rev-parse ..HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\n+\techo .. >expect &&\n+\tgit rev-parse .. >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 2043bb8..b89f9af 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -463,4 +463,11 @@ test_expect_success 'show added path under \"--follow -M\"' '\n \t)\n '\n \n+test_expect_success 'dotdot is a parent directory' '\n+\tmkdir -p a/b &&\n+\t( echo sixth && echo fifth ) >expect &&\n+\t( cd a/b && git log --format=%s .. ) >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.5.253.g2243e\n"},{"id":"166908","messageId":"20110502210141.GA15753@sigill.intra.peff.net","threadId":"27240","inReplyTo":"7v62pslt2k.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] specifying ranges: we did not mean to make \"..\" an empty set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-02T21:01:41Z","receivedAt":"2011-05-02T21:01:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 02, 2011 at 01:56:51PM -0700, Junio C Hamano wrote:\n\n> Either end of revision range operator can be omitted to default to HEAD,\n> as in \"origin..\" (what did I do since I forked) or \"..origin\" (what did\n> they do since I forked). But the current parser interprets \"..\" as an\n> empty range \"HEAD..HEAD\", and worse yet, because \"..\" does exist on the\n> filesystem, we get this annoying output:\n> \n> $ cd Documentation/howto\n> $ git log .. ;# give me recent commits that touch Documentation/ area.\n> fatal: ambiguous argument '..': both revision and filename\n> Use '--' to separate filenames from revisions\n> \n> Surely we could say \"git log ../\" or even \"git log -- ..\" to disambiguate,\n> but we shouldn't have to.\n> \n> Helped-by: Jeff King \n> Signed-off-by: Junio C Hamano \n\nLooks good to me.\n\nDoing \"...\" is still allowed, but will never produce any useful results.\nI don't know if it is worth disallowing it to catch errors. I am tempted\nto say it should be magic for \"@{u}...HEAD\", but I think just \"...\" is\ngetting unreadably magical. \"@{u}...HEAD\" is already pretty concise and\nis much more readable.\n\n-Peff\n"},{"id":"166910","messageId":"7v1v0glqoh.fsf@alter.siamese.dyndns.org","threadId":"27240","inReplyTo":"20110502210141.GA15753@sigill.intra.peff.net","subject":"Re: [PATCH v2] specifying ranges: we did not mean to make \"..\" an empty set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-02T21:48:30Z","receivedAt":"2011-05-02T21:48:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> .... \"@{u}...HEAD\" is already pretty concise and\n> is much more readable.\n\nEspecially since you can ust say \"@{u}...\" without HEAD.\n"},{"id":"166916","messageId":"FB837238-9953-4FD8-BC52-5C0AB061C291@gmail.com","threadId":"27240","inReplyTo":"7vr58glxro.fsf@alter.siamese.dyndns.org","subject":"Re: [Annoyance] \"git log ..\" thinks \"..\" is ambiguous","fromName":"Joshua Juran","fromEmail":"jjuran@gmail.com","sentAt":"2011-05-03T01:57:42Z","receivedAt":"2011-05-03T01:57:42Z","isPatch":false,"sender":{"key":"jjuran@gmail.com","avatar":null},"body":"On May 2, 2011, at 12:15 PM, Junio C Hamano wrote:\n\n> diff --git a/revision.c b/revision.c\n> index f4b8b38..26271d1 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -905,14 +905,26 @@ int handle_revision_arg(const char *arg, \n> struct rev_info *revs,\n> \t\tconst char *this = arg;\n> \t\tint symmetric = *next == '.';\n> \t\tunsigned int flags_exclude = flags ^ UNINTERESTING;\n> +\t\tstatic const char head_by_default[] = \"HEAD\";\n>\n> \t\t*dotdot = 0;\n> \t\tnext += symmetric;\n>\n> \t\tif (!*next)\n> -\t\t\tnext = \"HEAD\";\n> +\t\t\tnext = head_by_default;\n> \t\tif (dotdot == arg)\n> -\t\t\tthis = \"HEAD\";\n> +\t\t\tthis = head_by_default;\n> +\t\tif (this == head_by_default && next == head_by_default &&\n> +\t\t !symmetric) {\n\nIs there a reason not to write\n\n\tconst char *head_by_default = \"HEAD\";\n\nor even\n\n\tconst char *const head_by_default = \"HEAD\";\n\ninstead? Loading a static array and checking an init flag is a \npessimization versus just pointing into a read-only segment.\n\nJosh\n"},{"id":"166921","messageId":"4DBFA1A2.6050409@drmicha.warpmail.net","threadId":"27240","inReplyTo":"FB837238-9953-4FD8-BC52-5C0AB061C291@gmail.com","subject":"Re: [Annoyance] \"git log ..\" thinks \"..\" is ambiguous","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-05-03T06:33:06Z","receivedAt":"2011-05-03T06:33:06Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Joshua Juran venit, vidit, dixit 03.05.2011 03:57:\n> On May 2, 2011, at 12:15 PM, Junio C Hamano wrote:\n> \n>> diff --git a/revision.c b/revision.c\n>> index f4b8b38..26271d1 100644\n>> --- a/revision.c\n>> +++ b/revision.c\n>> @@ -905,14 +905,26 @@ int handle_revision_arg(const char *arg, \n>> struct rev_info *revs,\n>> \t\tconst char *this = arg;\n>> \t\tint symmetric = *next == '.';\n>> \t\tunsigned int flags_exclude = flags ^ UNINTERESTING;\n>> +\t\tstatic const char head_by_default[] = \"HEAD\";\n>>\n>> \t\t*dotdot = 0;\n>> \t\tnext += symmetric;\n>>\n>> \t\tif (!*next)\n>> -\t\t\tnext = \"HEAD\";\n>> +\t\t\tnext = head_by_default;\n>> \t\tif (dotdot == arg)\n>> -\t\t\tthis = \"HEAD\";\n>> +\t\t\tthis = head_by_default;\n>> +\t\tif (this == head_by_default && next == head_by_default &&\n>> +\t\t !symmetric) {\n> \n> Is there a reason not to write\n> \n> \tconst char *head_by_default = \"HEAD\";\n> \n> or even\n> \n> \tconst char *const head_by_default = \"HEAD\";\n> \n> instead? Loading a static array and checking an init flag is a \n> pessimization versus just pointing into a read-only segment.\n\nBecause of the comparisons later on: this == \"HEAD\" is not the same.\n\nMichael\n"},{"id":"166922","messageId":"4DBFA31E.40207@drmicha.warpmail.net","threadId":"27240","inReplyTo":"20110502210141.GA15753@sigill.intra.peff.net","subject":"Re: [PATCH v2] specifying ranges: we did not mean to make \"..\" an empty set","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-05-03T06:39:26Z","receivedAt":"2011-05-03T06:39:26Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 02.05.2011 23:01:\n> On Mon, May 02, 2011 at 01:56:51PM -0700, Junio C Hamano wrote:\n> \n>> Either end of revision range operator can be omitted to default to HEAD,\n>> as in \"origin..\" (what did I do since I forked) or \"..origin\" (what did\n>> they do since I forked). But the current parser interprets \"..\" as an\n>> empty range \"HEAD..HEAD\", and worse yet, because \"..\" does exist on the\n>> filesystem, we get this annoying output:\n>>\n>> $ cd Documentation/howto\n>> $ git log .. ;# give me recent commits that touch Documentation/ area.\n>> fatal: ambiguous argument '..': both revision and filename\n>> Use '--' to separate filenames from revisions\n>>\n>> Surely we could say \"git log ../\" or even \"git log -- ..\" to disambiguate,\n>> but we shouldn't have to.\n>>\n>> Helped-by: Jeff King \n>> Signed-off-by: Junio C Hamano \n> \n> Looks good to me.\n\nI'm sorry but I don't like this at all, because:\n\n> Doing \"...\" is still allowed, but will never produce any useful results.\n> I don't know if it is worth disallowing it to catch errors. I am tempted\n> to say it should be magic for \"@{u}...HEAD\", but I think just \"...\" is\n> getting unreadably magical. \"@{u}...HEAD\" is already pretty concise and\n> is much more readable.\n\nWe need to disambiguate any pathspec with \"--\" which could be a revision\nparameter. Therefore I find it very unnatural to disambiguate \"..\" to a\npathspec automatically (and have \"...\" error out). \"../\" is really\nsimple enough to type.\n\nFor \"..\", we should either catch the error or, better, have \"..\" default\nto \"@{u}..HEAD\" and do the same for \"...\". Those are used much more\noften, and this goes with \"try to parse as a rev first, then pathspec\".\n\nMichael\n"},{"id":"166927","messageId":"86404CD9-6040-4083-912F-23F4C11F4ED2@gmail.com","threadId":"27240","inReplyTo":"4DBFA1A2.6050409@drmicha.warpmail.net","subject":"Re: [Annoyance] \"git log ..\" thinks \"..\" is ambiguous","fromName":"Joshua Juran","fromEmail":"jjuran@gmail.com","sentAt":"2011-05-03T08:17:17Z","receivedAt":"2011-05-03T08:17:17Z","isPatch":false,"sender":{"key":"jjuran@gmail.com","avatar":null},"body":"On May 2, 2011, at 11:33 PM, Michael J Gruber wrote:\n\n> Joshua Juran venit, vidit, dixit 03.05.2011 03:57:\n>> On May 2, 2011, at 12:15 PM, Junio C Hamano wrote:\n>>\n>>> diff --git a/revision.c b/revision.c\n>>> index f4b8b38..26271d1 100644\n>>> --- a/revision.c\n>>> +++ b/revision.c\n>>> @@ -905,14 +905,26 @@ int handle_revision_arg(const char *arg,\n>>> struct rev_info *revs,\n>>> \t\tconst char *this = arg;\n>>> \t\tint symmetric = *next == '.';\n>>> \t\tunsigned int flags_exclude = flags ^ UNINTERESTING;\n>>> +\t\tstatic const char head_by_default[] = \"HEAD\";\n>>>\n>>> \t\t*dotdot = 0;\n>>> \t\tnext += symmetric;\n>>>\n>>> \t\tif (!*next)\n>>> -\t\t\tnext = \"HEAD\";\n>>> +\t\t\tnext = head_by_default;\n>>> \t\tif (dotdot == arg)\n>>> -\t\t\tthis = \"HEAD\";\n>>> +\t\t\tthis = head_by_default;\n>>> +\t\tif (this == head_by_default && next == head_by_default &&\n>>> +\t\t !symmetric) {\n>>\n>> Is there a reason not to write\n>>\n>> \tconst char *head_by_default = \"HEAD\";\n>>\n>> or even\n>>\n>> \tconst char *const head_by_default = \"HEAD\";\n>>\n>> instead? Loading a static array and checking an init flag is a\n>> pessimization versus just pointing into a read-only segment.\n>\n> Because of the comparisons later on: this == \"HEAD\" is not the same.\n\nSorry if I was unclear. I meant to replace only the 'static const \nchar head_by_default[] = \"HEAD\";' line and leave the rest of the patch \nunchanged.\n\nMy recollection is that Metrowerks C will statically allocate read- \nwrite storage (duplicating the read-only copy of the string) and copy \nthe string into it at runtime. It only copies the string once, which \nis ensured by checking an internally generated flag (also statically \nallocated) every time the scope containing the declaration is \nexecuted. This is the pessimization I speak of.\n\nBy contrast, my suggestion allocates a single pointer on the stack \nregardless of compiler optimization.\n\nJosh\n"},{"id":"166928","messageId":"BANLkTimZTV5bf2fNdeu4nEJkkJAdUSwupg@mail.gmail.com","threadId":"27240","inReplyTo":"7vr58glxro.fsf@alter.siamese.dyndns.org","subject":"Re: [Annoyance] \"git log ..\" thinks \"..\" is ambiguous","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2011-05-03T08:38:52Z","receivedAt":"2011-05-03T08:38:52Z","isPatch":false,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"On Mon, May 2, 2011 at 3:15 PM, Junio C Hamano wrote:\n> Either end of revision range operator can be omitted to default to\n> HEAD, as in \"origin..\" (what did I do since I forked) or \"..origin\" (what\n> did they do since I forked).  But this resulted in \"..\" to be interpreted\n> as an empty range \"HEAD..HEAD\", and worse yet, because \"..\" does exist on\n> the filesystem, we get this annoying output:\n>\n>  $ cd Documentation/howto\n>  $ git log .. ;# give me recent commits that touch Documentation/ area.\n>  fatal: ambiguous argument '..': both revision and filename\n>  Use '--' to separate filenames from revisions\n\nPerhaps it's because I still don't use git quite enough yet... but I\ndon't find the above annoying. *shrug*\n\n-John\n"},{"id":"166929","messageId":"4DBFC0B6.3080001@drmicha.warpmail.net","threadId":"27240","inReplyTo":"86404CD9-6040-4083-912F-23F4C11F4ED2@gmail.com","subject":"Re: [Annoyance] \"git log ..\" thinks \"..\" is ambiguous","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-05-03T08:45:42Z","receivedAt":"2011-05-03T08:45:42Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Joshua Juran venit, vidit, dixit 03.05.2011 10:17:\n> On May 2, 2011, at 11:33 PM, Michael J Gruber wrote:\n> \n>> Joshua Juran venit, vidit, dixit 03.05.2011 03:57:\n>>> On May 2, 2011, at 12:15 PM, Junio C Hamano wrote:\n>>>\n>>>> diff --git a/revision.c b/revision.c\n>>>> index f4b8b38..26271d1 100644\n>>>> --- a/revision.c\n>>>> +++ b/revision.c\n>>>> @@ -905,14 +905,26 @@ int handle_revision_arg(const char *arg,\n>>>> struct rev_info *revs,\n>>>> \t\tconst char *this = arg;\n>>>> \t\tint symmetric = *next == '.';\n>>>> \t\tunsigned int flags_exclude = flags ^ UNINTERESTING;\n>>>> +\t\tstatic const char head_by_default[] = \"HEAD\";\n>>>>\n>>>> \t\t*dotdot = 0;\n>>>> \t\tnext += symmetric;\n>>>>\n>>>> \t\tif (!*next)\n>>>> -\t\t\tnext = \"HEAD\";\n>>>> +\t\t\tnext = head_by_default;\n>>>> \t\tif (dotdot == arg)\n>>>> -\t\t\tthis = \"HEAD\";\n>>>> +\t\t\tthis = head_by_default;\n>>>> +\t\tif (this == head_by_default && next == head_by_default &&\n>>>> +\t\t !symmetric) {\n>>>\n>>> Is there a reason not to write\n>>>\n>>> \tconst char *head_by_default = \"HEAD\";\n>>>\n>>> or even\n>>>\n>>> \tconst char *const head_by_default = \"HEAD\";\n>>>\n>>> instead? Loading a static array and checking an init flag is a\n>>> pessimization versus just pointing into a read-only segment.\n>>\n>> Because of the comparisons later on: this == \"HEAD\" is not the same.\n> \n> Sorry if I was unclear. I meant to replace only the 'static const \n> char head_by_default[] = \"HEAD\";' line and leave the rest of the patch \n> unchanged.\n> \n> My recollection is that Metrowerks C will statically allocate read- \n> write storage (duplicating the read-only copy of the string) and copy \n> the string into it at runtime. It only copies the string once, which \n> is ensured by checking an internally generated flag (also statically \n> allocated) every time the scope containing the declaration is \n> executed. This is the pessimization I speak of.\n> \n> By contrast, my suggestion allocates a single pointer on the stack \n> regardless of compiler optimization.\n\nI see, sorry, I misread your suggestion. Junio will have to answer for\nhimself then ;)\n\nMichael\n"},{"id":"166951","messageId":"7vvcxrit07.fsf@alter.siamese.dyndns.org","threadId":"27240","inReplyTo":"4DBFA31E.40207@drmicha.warpmail.net","subject":"Re: [PATCH v2] specifying ranges: we did not mean to make \"..\" an empty set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-03T17:38:48Z","receivedAt":"2011-05-03T17:38:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n>>> Helped-by: Jeff King \n>>> Signed-off-by: Junio C Hamano \n>> \n>> Looks good to me.\n>\n> I'm sorry but I don't like this at all, because:\n>\n>> Doing \"...\" is still allowed, but will never produce any useful results.\n>> I don't know if it is worth disallowing it to catch errors. I am tempted\n>> to say it should be magic for \"@{u}...HEAD\", but I think just \"...\" is\n>> getting unreadably magical. \"@{u}...HEAD\" is already pretty concise and\n>> is much more readable.\n>\n> We need to disambiguate any pathspec with \"--\" which could be a revision\n> parameter. Therefore I find it very unnatural to disambiguate \"..\" to a\n> pathspec automatically (and have \"...\" error out). \"../\" is really\n> simple enough to type.\n\nIf you are comfortable typing \"../\", why do you even care? It would be a\ndifferent story if the patch made \"..\" error out and forbade to be used as\nan empty range even when you disambiguated, i.e. \"git log .. --\", but that\nis not what we are doing.\n\nAnd we do not even special case \"...\". Between the two potential requests\nof asking for an empty revision range and asking for a pathspec \"...\", both\nare just as unlikely.\n\nContrast that with \"..\" and realize that is very different. It is\ninfinitely more likely that the user meant the immediate parent directory\nthan an empty revision range.\n"},{"id":"166960","messageId":"7v7ha7ip81.fsf@alter.siamese.dyndns.org","threadId":"27240","inReplyTo":"86404CD9-6040-4083-912F-23F4C11F4ED2@gmail.com","subject":"Re: [Annoyance] \"git log ..\" thinks \"..\" is ambiguous","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-03T19:00:30Z","receivedAt":"2011-05-03T19:00:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joshua Juran writes:\n\n> My recollection is that Metrowerks C will statically allocate read-\n> write storage (duplicating the read-only copy of the string) and copy\n> the string into it at runtime. It only copies the string once, which\n> is ensured by checking an internally generated flag (also statically\n> allocated) every time the scope containing the declaration is\n> executed. This is the pessimization I speak of.\n\nThat's a stupid compiler, isn't it? Doesn't it pay attention to the fact\nthat the static char string in the function scope also says \"const\"?\n\nA sane compiler would store { 'H' 'E' 'A' 'D' '\\0' } five bytes in a data\nsegment (preferrably ro), give it a label and turn its assignment to and\ncomparison with \"next\" and \"this\" to a reference to that label address,\nand all can be resolved by the linker. Your suggestion will give a\ncompiler an excuse to allocate an extra pointer variable to hold that\naddress in addition to that, espcially if you do not say const twice.\n\nOf course the extra pointer could be optimized away if you follow the data\nand if the compiler does so we would get the same code.\n"},{"id":"166996","messageId":"4DC0F845.2080903@drmicha.warpmail.net","threadId":"27240","inReplyTo":"7vvcxrit07.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] specifying ranges: we did not mean to make \"..\" an empty set","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-05-04T06:55:01Z","receivedAt":"2011-05-04T06:55:01Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 03.05.2011 19:38:\n> Michael J Gruber writes:\n> \n>>>> Helped-by: Jeff King \n>>>> Signed-off-by: Junio C Hamano \n>>>\n>>> Looks good to me.\n>>\n>> I'm sorry but I don't like this at all, because:\n>>\n>>> Doing \"...\" is still allowed, but will never produce any useful results.\n>>> I don't know if it is worth disallowing it to catch errors. I am tempted\n>>> to say it should be magic for \"@{u}...HEAD\", but I think just \"...\" is\n>>> getting unreadably magical. \"@{u}...HEAD\" is already pretty concise and\n>>> is much more readable.\n>>\n>> We need to disambiguate any pathspec with \"--\" which could be a revision\n>> parameter. Therefore I find it very unnatural to disambiguate \"..\" to a\n>> pathspec automatically (and have \"...\" error out). \"../\" is really\n>> simple enough to type.\n> \n> If you are comfortable typing \"../\", why do you even care? It would be a\n\nI care about:\n\n- sane defaults\n- sane arguments\n\n> different story if the patch made \"..\" error out and forbade to be used as\n> an empty range even when you disambiguated, i.e. \"git log .. --\", but that\n> is not what we are doing.\n> \n> And we do not even special case \"...\". Between the two potential requests\n> of asking for an empty revision range and asking for a pathspec \"...\", both\n> are just as unlikely.\n> \n> Contrast that with \"..\" and realize that is very different. It is\n> infinitely more likely that the user meant the immediate parent directory\n> than an empty revision range.\n\nand that is a straw man argument. I suggested \"@{u}..HEAD\" for \"..\",\nbecause I consider that much more useful. \"Infinitely more likely\" is\nobviously true and obviously pointless when you compare with something\nof zero likelihood (and nonsense otherwise). I have no problem accepting\na majority vote or sane arguments, but not something like this, sorry.\n\nMichael\n"},{"id":"167023","messageId":"7vpqnyfpey.fsf@alter.siamese.dyndns.org","threadId":"27240","inReplyTo":"4DC0F845.2080903@drmicha.warpmail.net","subject":"Re: [PATCH v2] specifying ranges: we did not mean to make \"..\" an empty set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-04T15:36:53Z","receivedAt":"2011-05-04T15:36:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n>> Contrast that with \"..\" and realize that is very different. It is\n>> infinitely more likely that the user meant the immediate parent directory\n>> than an empty revision range.\n>\n> and that is a straw man argument. I suggested \"@{u}..HEAD\" for \"..\",\n> because I consider that much more useful.\n\nBut the thing is, \n\nEither of @{u}.. or ..@{u} may be an often used range depending on who you\nare and what you want to find. But reusing \"..\" for that purpose would be\na very steep uphill battle for obvious reasons that I can count at least\nthree:\n\n * How can a user remember \"..\" stands for whichever of @{u}.. or ..@{u}\n you happen to pick?\n\n * Can you guarantee that there will never be anything _more_ common and\n useful than \"@{u}..\" that deserves to use \"..\" as a shorthand? I\n can't.\n\n * Doing anything other than an empty range for \"..\" is a bit _too_\n magical for my taste. After all, if $A.. means $A..HEAD and ..$B means\n HEAD..$B, giving an empty string to $A or $B should yield nothing other\n than HEAD..HEAD, unless you want to confuse the user.\n\nI think your argument is that I wouldn't have felt the annoyance if \"..\"\nmeant a vastly more useful range that is totally different from the\ncurrent semantics, because I would have understood that \"..\" could be both\na useful range and a useful pathspec, and there won't be \"Stipid git! I\nknow .. could be an empty range but it should be obvious to you that I\ndidn't mean that interpretation with no practical value. Just take it as a\nparent directory pathspec!\".\n\nYou could achieve that by time-travelling to a past where no git user were\npresent and inplement \"$A..$B\" to default in that way from day one, and if\nI were living in such a world, I would certainly agree with you.\n\nBut that is not the world we live in.\n\nIn other words, both of us agree with the statement: \"What .. means today\nhas no practical value\". But I do not think that the conclusion that\nfollows that statement should be \"so let's change its semantics and make\nit do something useful\". Changing it to anything magical breaks\nconsistency in a big way, than keeping this degerated case as such.\n\nAs I said in the beginning of the thread, this was a mere annoyance, and I\nwouldn't be unhappy to drop this change. I will have to type \"../\" myself\nwhen I mean \"parent directory\", but that is a minor annoyance. But it\nalso may turn out to be a disaster that everybody else needs to teach new\npeople to do so.\n\nAs to changing the semantics of \"..\", I am moderately against it, but I\nconsider that is a separate topic. I am not opposed to giving a range\nthat is common and useful (be it @{u}.., ..@{u}, or anything else) a\nshort-hand, but that short-hand should not be \"..\".\n"},{"id":"167028","messageId":"7v8vumfirg.fsf@alter.siamese.dyndns.org","threadId":"27240","inReplyTo":"7vpqnyfpey.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] specifying ranges: we did not mean to make \"..\" an empty set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-04T18:00:35Z","receivedAt":"2011-05-04T18:00:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> As to changing the semantics of \"..\", I am moderately against it, but I\n> consider that is a separate topic. I am not opposed to giving a range\n> that is common and useful (be it @{u}.., ..@{u}, or anything else) a\n> short-hand, but that short-hand should not be \"..\".\n\nAfter re-reading 1654a46 (specifying ranges: we did not mean to make \"..\"\nan empty set, 2011-05-02), I found that I was contradicting myself in the\npatch.\n\nThe last part of the documantation update in that patch is clearly wrong.\nThe patch does not change what \"..\" means. The only thing it does is to\nwiden the heuristics that is used to disambiguate when the input could be\nambiguous.\n\n Documentation/revisions.txt | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/revisions.txt b/Documentation/revisions.txt\nindex fd043f7..80e5a47 100644\n--- a/Documentation/revisions.txt\n+++ b/Documentation/revisions.txt\n@@ -187,10 +187,10 @@ It is the set of commits that are reachable from either one of\n In these two shorthands, you can omit one end and let it default to HEAD.\n For example, 'origin..' is a shorthand for 'origin..HEAD' and asks \"What\n did I do since I forked from the origin branch?\" Similarly, '..origin'\n is a shorthand for 'HEAD..origin' and asks \"What did the origin do since\n-I forked from them?\" Note that you cannot omit both ends. '..' is not\n-an empty range that is both reachable and unreachable from HEAD.\n+I forked from them?\" Note that '..' would mean 'HEAD..HEAD' which is an\n+empty range that is both reachable and unreachable from HEAD.\n \n Two other shorthands for naming a set that is formed by a commit\n and its parent commits exist. The `r1{caret}@` notation means all\n parents of `r1`. `r1{caret}!` includes commit `r1` but excludes\n"}]}