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

From: Jeff King <peff@peff.net>
Date: Aug 23, 2012, 08:29 UTC
Message-ID: <20120823082916.GA6963@sigill.intra.peff.net>
In-Reply-To: <7vd32i5y8w.fsf@alter.siamese.dyndns.org>

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.