[Annoyance] "git log .." thinks ".." is ambiguous

[Annoyance] "git log .." thinks ".." is ambiguous

[Junio C Hamano]

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 this resulted in ".." to be interpreted
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 .. --", but we shouldn't have to.

---
 * I don't have very strong feeling about this, but just got annoyed
   enough to write this patch.  Should apply cleanly all the way down to
   at least 1.6.0 if not earlier.

 revision.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index f4b8b38..26271d1 100644
--- a/revision.c
+++ b/revision.c
@@ -905,14 +905,26 @@ 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";
 
 		*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;

Re: [Annoyance] "git log .." thinks ".." is ambiguous

[Jeff King]

On Mon, May 02, 2011 at 12:15:23PM -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 this resulted in ".." to be interpreted
> 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 .. --", but we shouldn't have to.

I got slightly confused reading this, because I thought at first you
wanted ".." to be some kind of magic rev specifier. In particular, your
last line should say:

  git log -- ..

no?

I finally figured it out by reading your example carefully.

As far as the patch goes, I think it is the right thing to do. We should
probably disallow ".." as a revision (because it is nonsensical to ask
for, just as it would be to ask for a non-existent ref). And then the
only sane thing is to consider it as a path.

I didn't check, but what does your patch do with:

  $ cd "`git rev-parse --show-toplevel`"
  $ git log ..

?

-Peff

Re: [Annoyance] "git log .." thinks ".." is ambiguous

[Jeff King]

On Mon, May 02, 2011 at 03:33:21PM -0400, Jeff King wrote:

> I didn't check, but what does your patch do with:
> 
>   $ cd "`git rev-parse --show-toplevel`"
>   $ git log ..

Nevermind, it properly complains "'..' is outside repository"; of course
no new code is needed because we could already do "git log ../..".

I do notice that gitk doesn't work, though. Doing "gitk .." produces an
"ambiguous argument" error. It looks like "git rev-parse .." still
treats it as HEAD..HEAD, which may be the source of the error.

-Peff

Re: [Annoyance] "git log .." thinks ".." is ambiguous

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, May 02, 2011 at 12:15:23PM -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 this resulted in ".." to be interpreted
>> 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 .. --", but we shouldn't have to.
>
> I got slightly confused reading this, because I thought at first you
> wanted ".." to be some kind of magic rev specifier. In particular, your
> last line should say:
>
>   git log -- ..
>
> no?

Yeah, sorry about that.  Or "git log ../".

And a matching "rev-parse" update should look like this.  I wonder if we
could share some code between the two, though...

 Documentation/revisions.txt |    7 +++++++
 builtin/rev-parse.c         |   16 ++++++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index b290b61..ceed820 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 you cannot omit both ends.  '..' is not
+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 adb1cae..3c8f171 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -223,6 +223,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;
@@ -234,9 +235,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);

Re: [Annoyance] "git log .." thinks ".." is ambiguous

[Jeff King]

On Mon, May 02, 2011 at 01:36:24PM -0700, Junio C Hamano wrote:

> And a matching "rev-parse" update should look like this.  I wonder if we
> could share some code between the two, though...
> 
>  Documentation/revisions.txt |    7 +++++++
>  builtin/rev-parse.c         |   16 ++++++++++++++--
>  2 files changed, 21 insertions(+), 2 deletions(-)

The doc update looks good to me. The duplication in rev-parse is pretty
gross, but that is certainly not a problem you are introducing. It would
be nice to clean it up, but I don't know how painful that would be.  It
seems like rev-parse should just be calling handle_revision_arg, but I'm
sure there are some nasty corner cases. IOW, I am not volunteering. :)

-Peff

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

[Junio C Hamano]

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>
---

 * With docs and tests.  In an early version, I forgot to restore the
   revision string that is temporarily NUL terminated with *dotdot = '.', 
   which resulted in "git log .." from Doc/howto to show not just the log
   of Doc but show everything.  The new test in t4202 tries to catch that
   rather common mistake as well.

 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(-)

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index 3d4b79c..fd043f7 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -184,6 +184,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 you cannot omit both ends.  '..' is not
+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 a5a1c86..54ce44b 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -219,6 +219,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;
@@ -230,9 +231,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 ded8812..4572aaf 100644
--- a/revision.c
+++ b/revision.c
@@ -1004,14 +1004,26 @@ 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";
 
 		*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 0eeeb0e..4485d0a 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -75,4 +75,18 @@ test_expect_success 'invalid @{n} reference' '
 	grep "fatal: Log for [^ ]* only has [0-9][0-9]* entries." error
 '
 
+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 2043bb8..b89f9af 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -463,4 +463,11 @@ test_expect_success 'show added path under "--follow -M"' '
 	)
 '
 
+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.5.253.g2243e

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

[Jeff King]

On Mon, May 02, 2011 at 01:56:51PM -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>

Looks good to me.

Doing "..." is still allowed, but will never produce any useful results.
I don't know if it is worth disallowing it to catch errors. I am tempted
to say it should be magic for "@{u}...HEAD", but I think just "..." is
getting unreadably magical. "@{u}...HEAD" is already pretty concise and
is much more readable.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> .... "@{u}...HEAD" is already pretty concise and
> is much more readable.

Especially since you can ust say "@{u}..." without HEAD.

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

[Michael J Gruber]

Jeff King venit, vidit, dixit 02.05.2011 23:01:
> On Mon, May 02, 2011 at 01:56:51PM -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>
> 
> Looks good to me.

I'm sorry but I don't like this at all, because:

> Doing "..." is still allowed, but will never produce any useful results.
> I don't know if it is worth disallowing it to catch errors. I am tempted
> to say it should be magic for "@{u}...HEAD", but I think just "..." is
> getting unreadably magical. "@{u}...HEAD" is already pretty concise and
> is much more readable.

We need to disambiguate any pathspec with "--" which could be a revision
parameter. Therefore I find it very unnatural to disambiguate ".." to a
pathspec automatically (and have "..." error out). "../" is really
simple enough to type.

For "..", we should either catch the error or, better, have ".." default
to "@{u}..HEAD" and do the same for "...". Those are used much more
often, and this goes with "try to parse as a rev first, then pathspec".

Michael

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

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

>>> Helped-by: Jeff King <peff@peff.net>
>>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> 
>> Looks good to me.
>
> I'm sorry but I don't like this at all, because:
>
>> Doing "..." is still allowed, but will never produce any useful results.
>> I don't know if it is worth disallowing it to catch errors. I am tempted
>> to say it should be magic for "@{u}...HEAD", but I think just "..." is
>> getting unreadably magical. "@{u}...HEAD" is already pretty concise and
>> is much more readable.
>
> We need to disambiguate any pathspec with "--" which could be a revision
> parameter. Therefore I find it very unnatural to disambiguate ".." to a
> pathspec automatically (and have "..." error out). "../" is really
> simple enough to type.

If you are comfortable typing "../", why do you even care?  It would be a
different story if the patch made ".." error out and forbade to be used as
an empty range even when you disambiguated, i.e. "git log .. --", but that
is not what we are doing.

And we do not even special case "...".  Between the two potential requests
of asking for an empty revision range and asking for a pathspec "...", both
are just as unlikely.

Contrast that with ".." and realize that is very different.  It is
infinitely more likely that the user meant the immediate parent directory
than an empty revision range.

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 03.05.2011 19:38:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>>>> Helped-by: Jeff King <peff@peff.net>
>>>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>>
>>> Looks good to me.
>>
>> I'm sorry but I don't like this at all, because:
>>
>>> Doing "..." is still allowed, but will never produce any useful results.
>>> I don't know if it is worth disallowing it to catch errors. I am tempted
>>> to say it should be magic for "@{u}...HEAD", but I think just "..." is
>>> getting unreadably magical. "@{u}...HEAD" is already pretty concise and
>>> is much more readable.
>>
>> We need to disambiguate any pathspec with "--" which could be a revision
>> parameter. Therefore I find it very unnatural to disambiguate ".." to a
>> pathspec automatically (and have "..." error out). "../" is really
>> simple enough to type.
> 
> If you are comfortable typing "../", why do you even care?  It would be a

I care about:

- sane defaults
- sane arguments

> different story if the patch made ".." error out and forbade to be used as
> an empty range even when you disambiguated, i.e. "git log .. --", but that
> is not what we are doing.
> 
> And we do not even special case "...".  Between the two potential requests
> of asking for an empty revision range and asking for a pathspec "...", both
> are just as unlikely.
> 
> Contrast that with ".." and realize that is very different.  It is
> infinitely more likely that the user meant the immediate parent directory
> than an empty revision range.

and that is a straw man argument. I suggested "@{u}..HEAD" for "..",
because I consider that much more useful. "Infinitely more likely" is
obviously true and obviously pointless when you compare with something
of zero likelihood (and nonsense otherwise). I have no problem accepting
a majority vote or sane arguments, but not something like this, sorry.

Michael

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

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

>> Contrast that with ".." and realize that is very different.  It is
>> infinitely more likely that the user meant the immediate parent directory
>> than an empty revision range.
>
> and that is a straw man argument. I suggested "@{u}..HEAD" for "..",
> because I consider that much more useful.

But the thing is, 

Either of @{u}.. or ..@{u} may be an often used range depending on who you
are and what you want to find.  But reusing ".." for that purpose would be
a very steep uphill battle for obvious reasons that I can count at least
three:

 * How can a user remember ".." stands for whichever of @{u}.. or ..@{u}
   you happen to pick?

 * Can you guarantee that there will never be anything _more_ common and
   useful than "@{u}.." that deserves to use ".." as a shorthand?  I
   can't.

 * Doing anything other than an empty range for ".." is a bit _too_
   magical for my taste.  After all, if $A.. means $A..HEAD and ..$B means
   HEAD..$B, giving an empty string to $A or $B should yield nothing other
   than HEAD..HEAD, unless you want to confuse the user.

I think your argument is that I wouldn't have felt the annoyance if ".."
meant a vastly more useful range that is totally different from the
current semantics, because I would have understood that ".." could be both
a useful range and a useful pathspec, and there won't be "Stipid git!  I
know .. could be an empty range but it should be obvious to you that I
didn't mean that interpretation with no practical value. Just take it as a
parent directory pathspec!".

You could achieve that by time-travelling to a past where no git user were
present and inplement "$A..$B" to default in that way from day one, and if
I were living in such a world, I would certainly agree with you.

But that is not the world we live in.

In other words, both of us agree with the statement: "What .. means today
has no practical value".  But I do not think that the conclusion that
follows that statement should be "so let's change its semantics and make
it do something useful".  Changing it to anything magical breaks
consistency in a big way, than keeping this degerated case as such.

As I said in the beginning of the thread, this was a mere annoyance, and I
wouldn't be unhappy to drop this change.  I will have to type "../" myself
when I mean "parent directory", but that is a minor annoyance.  But it
also may turn out to be a disaster that everybody else needs to teach new
people to do so.

As to changing the semantics of "..", I am moderately against it, but I
consider that is a separate topic.  I am not opposed to giving a range
that is common and useful (be it @{u}.., ..@{u}, or anything else) a
short-hand, but that short-hand should not be "..".

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> As to changing the semantics of "..", I am moderately against it, but I
> consider that is a separate topic.  I am not opposed to giving a range
> that is common and useful (be it @{u}.., ..@{u}, or anything else) a
> short-hand, but that short-hand should not be "..".

After re-reading 1654a46 (specifying ranges: we did not mean to make ".."
an empty set, 2011-05-02), I found that I was contradicting myself in the
patch.

The last part of the documantation update in that patch is clearly wrong.
The patch does not change what ".." means.  The only thing it does is to
widen the heuristics that is used to disambiguate when the input could be
ambiguous.

 Documentation/revisions.txt |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index fd043f7..80e5a47 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -187,10 +187,10 @@ It is the set of commits that are reachable from either one of
 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 you cannot omit both ends.  '..' is not
-an empty range that is both reachable and unreachable from HEAD.
+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

Re: [Annoyance] "git log .." thinks ".." is ambiguous

[Joshua Juran]

On May 2, 2011, at 12:15 PM, Junio C Hamano wrote:

> diff --git a/revision.c b/revision.c
> index f4b8b38..26271d1 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -905,14 +905,26 @@ 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";
>
> 		*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) {

Is there a reason not to write

	const char *head_by_default = "HEAD";

or even

	const char *const head_by_default = "HEAD";

instead?  Loading a static array and checking an init flag is a  
pessimization versus just pointing into a read-only segment.

Josh

Re: [Annoyance] "git log .." thinks ".." is ambiguous

[Michael J Gruber]

Joshua Juran venit, vidit, dixit 03.05.2011 03:57:
> On May 2, 2011, at 12:15 PM, Junio C Hamano wrote:
> 
>> diff --git a/revision.c b/revision.c
>> index f4b8b38..26271d1 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -905,14 +905,26 @@ 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";
>>
>> 		*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) {
> 
> Is there a reason not to write
> 
> 	const char *head_by_default = "HEAD";
> 
> or even
> 
> 	const char *const head_by_default = "HEAD";
> 
> instead?  Loading a static array and checking an init flag is a  
> pessimization versus just pointing into a read-only segment.

Because of the comparisons later on: this == "HEAD" is not the same.

Michael

Re: [Annoyance] "git log .." thinks ".." is ambiguous

[Joshua Juran]

On May 2, 2011, at 11:33 PM, Michael J Gruber wrote:

> Joshua Juran venit, vidit, dixit 03.05.2011 03:57:
>> On May 2, 2011, at 12:15 PM, Junio C Hamano wrote:
>>
>>> diff --git a/revision.c b/revision.c
>>> index f4b8b38..26271d1 100644
>>> --- a/revision.c
>>> +++ b/revision.c
>>> @@ -905,14 +905,26 @@ 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";
>>>
>>> 		*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) {
>>
>> Is there a reason not to write
>>
>> 	const char *head_by_default = "HEAD";
>>
>> or even
>>
>> 	const char *const head_by_default = "HEAD";
>>
>> instead?  Loading a static array and checking an init flag is a
>> pessimization versus just pointing into a read-only segment.
>
> Because of the comparisons later on: this == "HEAD" is not the same.

Sorry if I was unclear.  I meant to replace only the 'static const  
char head_by_default[] = "HEAD";' line and leave the rest of the patch  
unchanged.

My recollection is that Metrowerks C will statically allocate read- 
write storage (duplicating the read-only copy of the string) and copy  
the string into it at runtime.  It only copies the string once, which  
is ensured by checking an internally generated flag (also statically  
allocated) every time the scope containing the declaration is  
executed.  This is the pessimization I speak of.

By contrast, my suggestion allocates a single pointer on the stack  
regardless of compiler optimization.

Josh

Re: [Annoyance] "git log .." thinks ".." is ambiguous

[Michael J Gruber]

Joshua Juran venit, vidit, dixit 03.05.2011 10:17:
> On May 2, 2011, at 11:33 PM, Michael J Gruber wrote:
> 
>> Joshua Juran venit, vidit, dixit 03.05.2011 03:57:
>>> On May 2, 2011, at 12:15 PM, Junio C Hamano wrote:
>>>
>>>> diff --git a/revision.c b/revision.c
>>>> index f4b8b38..26271d1 100644
>>>> --- a/revision.c
>>>> +++ b/revision.c
>>>> @@ -905,14 +905,26 @@ 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";
>>>>
>>>> 		*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) {
>>>
>>> Is there a reason not to write
>>>
>>> 	const char *head_by_default = "HEAD";
>>>
>>> or even
>>>
>>> 	const char *const head_by_default = "HEAD";
>>>
>>> instead?  Loading a static array and checking an init flag is a
>>> pessimization versus just pointing into a read-only segment.
>>
>> Because of the comparisons later on: this == "HEAD" is not the same.
> 
> Sorry if I was unclear.  I meant to replace only the 'static const  
> char head_by_default[] = "HEAD";' line and leave the rest of the patch  
> unchanged.
> 
> My recollection is that Metrowerks C will statically allocate read- 
> write storage (duplicating the read-only copy of the string) and copy  
> the string into it at runtime.  It only copies the string once, which  
> is ensured by checking an internally generated flag (also statically  
> allocated) every time the scope containing the declaration is  
> executed.  This is the pessimization I speak of.
> 
> By contrast, my suggestion allocates a single pointer on the stack  
> regardless of compiler optimization.

I see, sorry, I misread your suggestion. Junio will have to answer for
himself then ;)

Michael

Re: [Annoyance] "git log .." thinks ".." is ambiguous

[Junio C Hamano]

Joshua Juran <jjuran@gmail.com> writes:

> My recollection is that Metrowerks C will statically allocate read-
> write storage (duplicating the read-only copy of the string) and copy
> the string into it at runtime.  It only copies the string once, which
> is ensured by checking an internally generated flag (also statically
> allocated) every time the scope containing the declaration is
> executed.  This is the pessimization I speak of.

That's a stupid compiler, isn't it?  Doesn't it pay attention to the fact
that the static char string in the function scope also says "const"?

A sane compiler would store { 'H' 'E' 'A' 'D' '\0' } five bytes in a data
segment (preferrably ro), give it a label and turn its assignment to and
comparison with "next" and "this" to a reference to that label address,
and all can be resolved by the linker.  Your suggestion will give a
compiler an excuse to allocate an extra pointer variable to hold that
address in addition to that, espcially if you do not say const twice.

Of course the extra pointer could be optimized away if you follow the data
and if the compiler does so we would get the same code.

Re: [Annoyance] "git log .." thinks ".." is ambiguous

[John Szakmeister]

On Mon, May 2, 2011 at 3:15 PM, Junio C Hamano <gitster@pobox.com> 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 this resulted in ".." to be interpreted
> 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

Perhaps it's because I still don't use git quite enough yet... but I
don't find the above annoying.  *shrug*

-John