{"thread":{"id":"31325","subject":"[PATCH] specifying ranges: we did not mean to make \"..\" an empty set","startedAt":"2012-08-22T22:59:43Z","lastAt":"2012-08-23T22:14:27Z","messageCount":6,"participants":["Junio C Hamano","Jeff King","Thomas Rast"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"197669","messageId":"7vd32i5y8w.fsf@alter.siamese.dyndns.org","threadId":"31325","inReplyTo":null,"subject":"[PATCH] specifying ranges: we did not mean to make \"..\" an empty set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-22T22:59:43Z","receivedAt":"2012-08-22T22:59:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Date: Mon, 2 May 2011 13:39:16 -0700\n\nEither 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 <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * The other day I was looking into my old pile of leftover bits,\n   and found this ($gmane/172619).  This is rebased to more recent\n   codebase, but it is still relevant.\n\n   Comments, other than \"we may want to reuse 'log ..' as a\n   short-hand for 'log @{u}..'\", which is an utter nonsense?\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 dc0070b..69d996b 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 '..' 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\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 13495b8..47b4e7a 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -224,6 +224,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@@ -235,9 +236,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 5b81a92..457868d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1132,15 +1132,27 @@ 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 \t\tunsigned int a_flags;\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 c5cb77a..0cd2a2a 100755\n--- a/t/t1506-rev-parse-diagnosis.sh\n+++ b/t/t1506-rev-parse-diagnosis.sh\n@@ -182,4 +182,18 @@ test_expect_success '<commit>:file correctly diagnosed after a pathname' '\n \ttest_cmp expect actual\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 71be59d..45058cc 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -806,4 +806,11 @@ test_expect_success 'log --graph with diff and stats' '\n \ttest_cmp expect actual.sanitized\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.12.149.g7ae593c\n"},{"id":"197700","messageId":"20120823082916.GA6963@sigill.intra.peff.net","threadId":"31325","inReplyTo":"7vd32i5y8w.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] specifying ranges: we did not mean to make \"..\" an empty set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-23T08:29:16Z","receivedAt":"2012-08-23T08:29:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 22, 2012 at 03:59:43PM -0700, Junio C Hamano wrote:\n\n> Date: Mon, 2 May 2011 13:39:16 -0700\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 <peff@peff.net>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nHmm, for some reason I had no recollection of the original thread at\nall. And yet reading the archives, I apparently had quite a bit to say.\nReading again with fresh eyes, I still think this is sane.\n\nI don't think assigning any revision magic to \"..\" besides \"the empty\nrange\" makes sense at all for the reasons you gave in the original\nthread. And the empty range is a pointless no-op. So I don't see any\nreal argument in favor of disambiguating towards the revision.\n\nSo the only reasonable choices are to leave it as an error, or to\ndisambiguate towards the pathspec.  I suppose some script could stupidly\nbe doing \"git log $a..$b\" and would prefer to error out when both are\nblank. But that seems unlikely, and making \"..\" work is actually useful.\n\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 '..' would mean 'HEAD..HEAD' which is an\n> +empty range that is both reachable and unreachable from HEAD.\n\nThis last sentence confuses me. Now we are documenting that \"yes, ..\nreally means HEAD..HEAD, which is the empty range\". But isn't the point\nof this patch to say \"sure, it would be the empty range, but because\nthat is stupid and pointless, we do not consider it valid and treat ..\nas a pathspec\"?\n\nI think that may be what you are trying to say with the \"would\" in that\nsentence, but perhaps this would be a good point to expand and mention\nthat we special-case \"..\".\n\n> +test_expect_success 'dotdot is not an empty set' '\n> +\t( H=$(git rev-parse HEAD) && echo $H ; echo ^$H ) >expect &&\n\nIt almost certainly doesn't matter in practice, but the ';' here would\nbreak the &&-chain from rev-parse.\n"},{"id":"197711","messageId":"874nntq0sb.fsf@thomas.inf.ethz.ch","threadId":"31325","inReplyTo":"20120823082916.GA6963@sigill.intra.peff.net","subject":"Re: [PATCH] specifying ranges: we did not mean to make \"..\" an empty set","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-08-23T11:56:52Z","receivedAt":"2012-08-23T11:56:52Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Aug 22, 2012 at 03:59:43PM -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 <peff@peff.net>\n>> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n>\n> Hmm, for some reason I had no recollection of the original thread at\n> all. And yet reading the archives, I apparently had quite a bit to say.\n> Reading again with fresh eyes, I still think this is sane.\n>\n> I don't think assigning any revision magic to \"..\" besides \"the empty\n> range\" makes sense at all for the reasons you gave in the original\n> thread. And the empty range is a pointless no-op. So I don't see any\n> real argument in favor of disambiguating towards the revision.\n\nI don't think that \"..\" is really a no-op.  It is true that HEAD..HEAD\ndoes not itself result in any revisions, but it *could* be used as a\nsilly shorthand to introduce ^HEAD into the objects being walked.  This\ncan make a difference if it then excludes other objects, too.\n\nI would argue that such use is misguided, and I am in favor of the\npatch, but in theory it is possible.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"197724","messageId":"7vobm14hu9.fsf@alter.siamese.dyndns.org","threadId":"31325","inReplyTo":"874nntq0sb.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH] specifying ranges: we did not mean to make \"..\" an empty set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-23T17:51:42Z","receivedAt":"2012-08-23T17:51:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> I don't think that \"..\" is really a no-op.  It is true that HEAD..HEAD\n> does not itself result in any revisions, but it *could* be used as a\n> silly shorthand to introduce ^HEAD into the objects being walked.  This\n> can make a difference if it then excludes other objects, too.\n>\n> I would argue that such use is misguided, and I am in favor of the\n> patch, but in theory it is possible.\n\nYou could say \"log --all .. --\" when you mean \"log --all ^HEAD\",\nbecause .. === HEAD..HEAD === ^HEAD HEAD and HEAD is already\ncontained in --all.  So it is a valid point.\n\nYou however cannot say \"log --all ..\" without double-dash to\ndisambiguate, as you would get:\n\n    $ git log --all ..\n    fatal: ambiguous argument '..': both revision and filename\n\nAny existing practice that used to produce useful results couldn't\nhave been using \"..\" without an explicit \"--\", I think, and with the\ndisambiguation that favors to take \"..\" as the parent directory,\n\"log --all .. --\" still means \"log --all ^HEAD\".\n\nSo I think we are still OK.\n"},{"id":"197739","messageId":"7vhart2sos.fsf@alter.siamese.dyndns.org","threadId":"31325","inReplyTo":"20120823082916.GA6963@sigill.intra.peff.net","subject":"Re: [PATCH] specifying ranges: we did not mean to make \"..\" an empty set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-23T21:40:19Z","receivedAt":"2012-08-23T21:40:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> --- 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 '..' would mean 'HEAD..HEAD' which is an\n>> +empty range that is both reachable and unreachable from HEAD.\n>\n> This last sentence confuses me. Now we are documenting that \"yes, ..\n> really means HEAD..HEAD, which is the empty range\". But isn't the point\n> of this patch to say \"sure, it would be the empty range, but because\n> that is stupid and pointless, we do not consider it valid and treat ..\n> as a pathspec\"?\n\nNo, we still allow \"..\" as a short-hand for HEAD..HEAD when it is\nunderstood as a rev.  We also allow \"..\" as a pathspec to match the\nparent directory when it is understood as a pathspec.\n\nThe only thing the topic wanted to change was the disambiguation\nlogic.  When a string S can name both rev and path, we ask the user\nto disambiguate, but when S is \"..\", we do not have to (as one\ninterpretation is meaningless).\n\n> I think that may be what you are trying to say with the \"would\" in that\n> sentence, but perhaps this would be a good point to expand and mention\n> that we special-case \"..\".\n\nI think that documentation belongs to the section of disambiguation\nwithout \"--\".  Usually you need to use \"--\", but \"..\" is taken as\npath even without \"--\".  An interesting side effect is that\n\n\tgit log .. pu\n\nused to error out for \"..\" being both rev and path, but it will\nerror out for \"pu\" not being a path in the working tree.  This is\nbecause on a command line without \"--\" disambiguation, once you\nstart listing paths, you have to have nothing but paths after that\npoint.\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> It almost certainly doesn't matter in practice, but the ';' here would\n> break the &&-chain from rev-parse.\n\nYeah, my bad.  Thanks for spotting.\n"},{"id":"197741","messageId":"20120823221427.GA9337@sigill.intra.peff.net","threadId":"31325","inReplyTo":"7vhart2sos.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] specifying ranges: we did not mean to make \"..\" an empty set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-23T22:14:27Z","receivedAt":"2012-08-23T22:14:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 23, 2012 at 02:40:19PM -0700, Junio C Hamano wrote:\n\n> > This last sentence confuses me. Now we are documenting that \"yes, ..\n> > really means HEAD..HEAD, which is the empty range\". But isn't the point\n> > of this patch to say \"sure, it would be the empty range, but because\n> > that is stupid and pointless, we do not consider it valid and treat ..\n> > as a pathspec\"?\n> \n> No, we still allow \"..\" as a short-hand for HEAD..HEAD when it is\n> understood as a rev.  We also allow \"..\" as a pathspec to match the\n> parent directory when it is understood as a pathspec.\n> \n> The only thing the topic wanted to change was the disambiguation\n> logic.  When a string S can name both rev and path, we ask the user\n> to disambiguate, but when S is \"..\", we do not have to (as one\n> interpretation is meaningless).\n\nAh, right. OK, that makes more sense. I wasn't thinking that you could\nstill do:\n\n  git log .. --\n\nif you really wanted to. So yeah, it doesn't belong here...\n\n> I think that documentation belongs to the section of disambiguation\n> without \"--\".  Usually you need to use \"--\", but \"..\" is taken as\n> path even without \"--\".\n\n...but I agree it would be worth mentioning there. But I am not sure\nwhere \"there\" is in the current documentation.\n\n> An interesting side effect is that\n> \n> \tgit log .. pu\n> \n> used to error out for \"..\" being both rev and path, but it will\n> error out for \"pu\" not being a path in the working tree.  This is\n> because on a command line without \"--\" disambiguation, once you\n> start listing paths, you have to have nothing but paths after that\n> point.\n\nHmm. Yeah, that's a slightly surprising emergent behavior. But I think\nit is not a big deal since both cases led to errors anyway.\n\n-Peff\n"}]}