threads / patch / 31325

patchspecifying ranges: we did not mean to make ".." an empty set

Subject: [PATCH] specifying ranges: we did not mean to make ".." an empty set

## tl;dr

6 messages between Aug 22, 2012 and Aug 23, 2012. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Junio C Hamano· Aug 22, 2012, 22:59 UTC · lore
Date: Mon, 2 May 2011 13:39:16 -0700

Either end of revision range operator can be omitted to default to HEAD, as in "origin.." (what did I do since I forked) or "..origin" (what did they do since I forked). But the current parser interprets ".." as an empty range "HEAD..HEAD", and worse yet, because ".." does exist on the filesystem, we get this annoying output:

  $ cd Documentation/howto
  $ git log .. ;# give me recent commits that touch Documentation/ area.
  fatal: ambiguous argument '..': both revision and filename
  Use '--' to separate filenames from revisions

Surely we could say "git log ../" or even "git log -- .." to disambiguate, but we shouldn't have to.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * The other day I was looking into my old pile of leftover bits,
   and found this ($gmane/172619).  This is rebased to more recent
   codebase, but it is still relevant.
   Comments, other than "we may want to reuse 'log ..' as a
   short-hand for 'log @{u}..'", which is an utter nonsense?
 Documentation/revisions.txt    |  7 +++++++
 builtin/rev-parse.c            | 16 ++++++++++++++--
 revision.c                     | 16 ++++++++++++++--
 t/t1506-rev-parse-diagnosis.sh | 14 ++++++++++++++
 t/t4202-log.sh                 |  7 +++++++
 5 files changed, 56 insertions(+), 4 deletions(-)
Show changes to 5 files +56 −4

Documentation/revisions.txt, builtin/rev-parse.c, revision.c, t/t1506-rev-parse-diagnosis.sh, t/t4202-log.sh

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index dc0070b..69d996b 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -213,6 +213,13 @@ of 'r1' and 'r2' and is defined as
 It is the set of commits that are reachable from either one of
 'r1' or 'r2' but not from both.
 
+In these two shorthands, you can omit one end and let it default to HEAD.
+For example, 'origin..' is a shorthand for 'origin..HEAD' and asks "What
+did I do since I forked from the origin branch?"  Similarly, '..origin'
+is a shorthand for 'HEAD..origin' and asks "What did the origin do since
+I forked from them?"  Note that '..' would mean 'HEAD..HEAD' which is an
+empty range that is both reachable and unreachable from HEAD.
+
 Two other shorthands for naming a set that is formed by a commit
 and its parent commits exist.  The 'r1{caret}@' notation means all
 parents of 'r1'.  'r1{caret}!' includes commit 'r1' but excludes
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 13495b8..47b4e7a 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -224,6 +224,7 @@ static int try_difference(const char *arg)
 	const char *next;
 	const char *this;
 	int symmetric;
+	static const char head_by_default[] = "HEAD";
 
 	if (!(dotdot = strstr(arg, "..")))
 		return 0;
@@ -235,9 +236,20 @@ static int try_difference(const char *arg)
 	next += symmetric;
 
 	if (!*next)
-		next = "HEAD";
+		next = head_by_default;
 	if (dotdot == arg)
-		this = "HEAD";
+		this = head_by_default;
+
+	if (this == head_by_default && next == head_by_default &&
+	    !symmetric) {
+		/*
+		 * Just ".."?  That is not a range but the
+		 * pathspec for the parent directory.
+		 */
+		*dotdot = '.';
+		return 0;
+	}
+
 	if (!get_sha1(this, sha1) && !get_sha1(next, end)) {
 		show_rev(NORMAL, end, next);
 		show_rev(symmetric ? NORMAL : REVERSED, sha1, this);
diff --git a/revision.c b/revision.c
index 5b81a92..457868d 100644
--- a/revision.c
+++ b/revision.c
@@ -1132,15 +1132,27 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs,
 		const char *this = arg;
 		int symmetric = *next == '.';
 		unsigned int flags_exclude = flags ^ UNINTERESTING;
+		static const char head_by_default[] = "HEAD";
 		unsigned int a_flags;
 
 		*dotdot = 0;
 		next += symmetric;
 
 		if (!*next)
-			next = "HEAD";
+			next = head_by_default;
 		if (dotdot == arg)
-			this = "HEAD";
+			this = head_by_default;
+		if (this == head_by_default && next == head_by_default &&
+		    !symmetric) {
+			/*
+			 * Just ".."?  That is not a range but the
+			 * pathspec for the parent directory.
+			 */
+			if (!cant_be_filename) {
+				*dotdot = '.';
+				return -1;
+			}
+		}
 		if (!get_sha1(this, from_sha1) &&
 		    !get_sha1(next, sha1)) {
 			struct commit *a, *b;
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index c5cb77a..0cd2a2a 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -182,4 +182,18 @@ test_expect_success '<commit>:file correctly diagnosed after a pathname' '
 	test_cmp expect actual
 '
 
+test_expect_success 'dotdot is not an empty set' '
+	( H=$(git rev-parse HEAD) && echo $H ; echo ^$H ) >expect &&
+
+	git rev-parse HEAD.. >actual &&
+	test_cmp expect actual &&
+
+	git rev-parse ..HEAD >actual &&
+	test_cmp expect actual &&
+
+	echo .. >expect &&
+	git rev-parse .. >actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 71be59d..45058cc 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -806,4 +806,11 @@ test_expect_success 'log --graph with diff and stats' '
 	test_cmp expect actual.sanitized
 '
 
+test_expect_success 'dotdot is a parent directory' '
+	mkdir -p a/b &&
+	( echo sixth && echo fifth ) >expect &&
+	( cd a/b && git log --format=%s .. ) >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.12.149.g7ae593c
Jeff King· Aug 23, 2012, 08:29 UTC · re: Junio C Hamano · lore

Re: [PATCH] specifying ranges: we did not mean to make ".." an empty set

On Wed, Aug 22, 2012 at 03:59:43PM -0700, Junio C Hamano wrote:
Show 18 quoted lines
> Date: Mon, 2 May 2011 13:39:16 -0700
> 
> Either end of revision range operator can be omitted to default to HEAD,
> as in "origin.." (what did I do since I forked) or "..origin" (what did
> they do since I forked).  But the current parser interprets ".."  as an
> empty range "HEAD..HEAD", and worse yet, because ".." does exist on the
> filesystem, we get this annoying output:
> 
>   $ cd Documentation/howto
>   $ git log .. ;# give me recent commits that touch Documentation/ area.
>   fatal: ambiguous argument '..': both revision and filename
>   Use '--' to separate filenames from revisions
> 
> Surely we could say "git log ../" or even "git log -- .." to disambiguate,
> but we shouldn't have to.
> 
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Hmm, for some reason I had no recollection of the original thread at all. And yet reading the archives, I apparently had quite a bit to say. Reading again with fresh eyes, I still think this is sane.

I don't think assigning any revision magic to ".." besides "the empty range" makes sense at all for the reasons you gave in the original thread. And the empty range is a pointless no-op. So I don't see any real argument in favor of disambiguating towards the revision.

So the only reasonable choices are to leave it as an error, or to disambiguate towards the pathspec. I suppose some script could stupidly be doing "git log $a..$b" and would prefer to error out when both are blank. But that seems unlikely, and making ".." work is actually useful.

Show 12 quoted lines
> --- a/Documentation/revisions.txt
> +++ b/Documentation/revisions.txt
> @@ -213,6 +213,13 @@ of 'r1' and 'r2' and is defined as
>  It is the set of commits that are reachable from either one of
>  'r1' or 'r2' but not from both.
>  
> +In these two shorthands, you can omit one end and let it default to HEAD.
> +For example, 'origin..' is a shorthand for 'origin..HEAD' and asks "What
> +did I do since I forked from the origin branch?"  Similarly, '..origin'
> +is a shorthand for 'HEAD..origin' and asks "What did the origin do since
> +I forked from them?"  Note that '..' would mean 'HEAD..HEAD' which is an
> +empty range that is both reachable and unreachable from HEAD.

This last sentence confuses me. Now we are documenting that "yes, .. really means HEAD..HEAD, which is the empty range". But isn't the point of this patch to say "sure, it would be the empty range, but because that is stupid and pointless, we do not consider it valid and treat .. as a pathspec"?

I think that may be what you are trying to say with the "would" in that sentence, but perhaps this would be a good point to expand and mention that we special-case "..".

> +test_expect_success 'dotdot is not an empty set' '
> +	( H=$(git rev-parse HEAD) && echo $H ; echo ^$H ) >expect &&

It almost certainly doesn't matter in practice, but the ';' here would break the &&-chain from rev-parse.

Thomas Rast· Aug 23, 2012, 11:56 UTC · re: Jeff King · lore

Re: [PATCH] specifying ranges: we did not mean to make ".." an empty set

Jeff King <peff@peff.net> writes:
Show 27 quoted lines
> On Wed, Aug 22, 2012 at 03:59:43PM -0700, Junio C Hamano wrote:
>
>> Either end of revision range operator can be omitted to default to HEAD,
>> as in "origin.." (what did I do since I forked) or "..origin" (what did
>> they do since I forked).  But the current parser interprets ".."  as an
>> empty range "HEAD..HEAD", and worse yet, because ".." does exist on the
>> filesystem, we get this annoying output:
>> 
>>   $ cd Documentation/howto
>>   $ git log .. ;# give me recent commits that touch Documentation/ area.
>>   fatal: ambiguous argument '..': both revision and filename
>>   Use '--' to separate filenames from revisions
>> 
>> Surely we could say "git log ../" or even "git log -- .." to disambiguate,
>> but we shouldn't have to.
>> 
>> Helped-by: Jeff King <peff@peff.net>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> Hmm, for some reason I had no recollection of the original thread at
> all. And yet reading the archives, I apparently had quite a bit to say.
> Reading again with fresh eyes, I still think this is sane.
>
> I don't think assigning any revision magic to ".." besides "the empty
> range" makes sense at all for the reasons you gave in the original
> thread. And the empty range is a pointless no-op. So I don't see any
> real argument in favor of disambiguating towards the revision.

I don't think that ".." is really a no-op. It is true that HEAD..HEAD does not itself result in any revisions, but it *could* be used as a silly shorthand to introduce ^HEAD into the objects being walked. This can make a difference if it then excludes other objects, too.

I would argue that such use is misguided, and I am in favor of the patch, but in theory it is possible.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Junio C Hamano· Aug 23, 2012, 17:51 UTC · re: Thomas Rast · lore

Re: [PATCH] specifying ranges: we did not mean to make ".." an empty set

Thomas Rast <trast@student.ethz.ch> writes:
Show 7 quoted lines
> I don't think that ".." is really a no-op.  It is true that HEAD..HEAD
> does not itself result in any revisions, but it *could* be used as a
> silly shorthand to introduce ^HEAD into the objects being walked.  This
> can make a difference if it then excludes other objects, too.
>
> I would argue that such use is misguided, and I am in favor of the
> patch, but in theory it is possible.

You could say "log --all .. --" when you mean "log --all ^HEAD", because .. === HEAD..HEAD === ^HEAD HEAD and HEAD is already contained in --all. So it is a valid point.

You however cannot say "log --all .." without double-dash to disambiguate, as you would get:

    $ git log --all ..
    fatal: ambiguous argument '..': both revision and filename

Any existing practice that used to produce useful results couldn't have been using ".." without an explicit "--", I think, and with the disambiguation that favors to take ".." as the parent directory, "log --all .. --" still means "log --all ^HEAD".

So I think we are still OK.
Junio C Hamano· Aug 23, 2012, 21:40 UTC · re: Jeff King · lore

Re: [PATCH] specifying ranges: we did not mean to make ".." an empty set

Jeff King <peff@peff.net> writes:
Show 18 quoted lines
>> --- a/Documentation/revisions.txt
>> +++ b/Documentation/revisions.txt
>> @@ -213,6 +213,13 @@ of 'r1' and 'r2' and is defined as
>>  It is the set of commits that are reachable from either one of
>>  'r1' or 'r2' but not from both.
>>  
>> +In these two shorthands, you can omit one end and let it default to HEAD.
>> +For example, 'origin..' is a shorthand for 'origin..HEAD' and asks "What
>> +did I do since I forked from the origin branch?"  Similarly, '..origin'
>> +is a shorthand for 'HEAD..origin' and asks "What did the origin do since
>> +I forked from them?"  Note that '..' would mean 'HEAD..HEAD' which is an
>> +empty range that is both reachable and unreachable from HEAD.
>
> This last sentence confuses me. Now we are documenting that "yes, ..
> really means HEAD..HEAD, which is the empty range". But isn't the point
> of this patch to say "sure, it would be the empty range, but because
> that is stupid and pointless, we do not consider it valid and treat ..
> as a pathspec"?

No, we still allow ".." as a short-hand for HEAD..HEAD when it is understood as a rev. We also allow ".." as a pathspec to match the parent directory when it is understood as a pathspec.

The only thing the topic wanted to change was the disambiguation logic. When a string S can name both rev and path, we ask the user to disambiguate, but when S is "..", we do not have to (as one interpretation is meaningless).

> I think that may be what you are trying to say with the "would" in that
> sentence, but perhaps this would be a good point to expand and mention
> that we special-case "..".

I think that documentation belongs to the section of disambiguation without "--". Usually you need to use "--", but ".." is taken as path even without "--". An interesting side effect is that

	git log .. pu

used to error out for ".." being both rev and path, but it will error out for "pu" not being a path in the working tree. This is because on a command line without "--" disambiguation, once you start listing paths, you have to have nothing but paths after that point.

Show 5 quoted lines
>> +test_expect_success 'dotdot is not an empty set' '
>> +	( H=$(git rev-parse HEAD) && echo $H ; echo ^$H ) >expect &&
>
> It almost certainly doesn't matter in practice, but the ';' here would
> break the &&-chain from rev-parse.
Yeah, my bad.  Thanks for spotting.
Jeff King· Aug 23, 2012, 22:14 UTC · re: Junio C Hamano · lore

Re: [PATCH] specifying ranges: we did not mean to make ".." an empty set

On Thu, Aug 23, 2012 at 02:40:19PM -0700, Junio C Hamano wrote:
Show 14 quoted lines
> > This last sentence confuses me. Now we are documenting that "yes, ..
> > really means HEAD..HEAD, which is the empty range". But isn't the point
> > of this patch to say "sure, it would be the empty range, but because
> > that is stupid and pointless, we do not consider it valid and treat ..
> > as a pathspec"?
> 
> No, we still allow ".." as a short-hand for HEAD..HEAD when it is
> understood as a rev.  We also allow ".." as a pathspec to match the
> parent directory when it is understood as a pathspec.
> 
> The only thing the topic wanted to change was the disambiguation
> logic.  When a string S can name both rev and path, we ask the user
> to disambiguate, but when S is "..", we do not have to (as one
> interpretation is meaningless).

Ah, right. OK, that makes more sense. I wasn't thinking that you could still do:

  git log .. --
if you really wanted to. So yeah, it doesn't belong here...
> I think that documentation belongs to the section of disambiguation
> without "--".  Usually you need to use "--", but ".." is taken as
> path even without "--".

...but I agree it would be worth mentioning there. But I am not sure where "there" is in the current documentation.

Show 9 quoted lines
> An interesting side effect is that
> 
> 	git log .. pu
> 
> used to error out for ".." being both rev and path, but it will
> error out for "pu" not being a path in the working tree.  This is
> because on a command line without "--" disambiguation, once you
> start listing paths, you have to have nothing but paths after that
> point.

Hmm. Yeah, that's a slightly surprising emergent behavior. But I think it is not a big deal since both cases led to errors anyway.

-Peff

← back to recent threads