threads / patch / 12696

patchgit-rev-parse.txt: clarify meaning of rev~ and rev~0.

Subject: [PATCH] git-rev-parse.txt: clarify meaning of rev~ and rev~0.

## tl;dr

4 messages between Mar 14, 2008 and Mar 14, 2008. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Sergei Organov· Mar 14, 2008, 17:20 UTC · lore

Signed-off-by: Sergei Organov <osv@javad.com>
---
 Documentation/git-rev-parse.txt |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

Show changes to Documentation/git-rev-parse.txt +3 −2

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 6513c2e..0234f89 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -200,8 +200,9 @@ blobs contained in a commit.
   object that is the <n>th generation grand-parent of the named
   commit object, following only the first parent.  I.e. rev~3 is
   equivalent to rev{caret}{caret}{caret} which is equivalent to
-  rev{caret}1{caret}1{caret}1.  See below for a illustration of
-  the usage of this form.
+  rev{caret}1{caret}1{caret}1.  See below for an illustration of
+  the usage of this form.  'rev{tilde}' is equivalent to 'rev{tilde}0'
+  which in turn is equivalent to 'rev'.
 
 * A suffix '{caret}' followed by an object type name enclosed in
   brace pair (e.g. `v0.99.8{caret}\{commit\}`) means the object

-- 
1.5.4.4.551.g1658c

Linus Torvalds· Mar 14, 2008, 18:49 UTC · re: Sergei Organov · lore

Re: [PATCH] git-rev-parse.txt: clarify meaning of rev~ and rev~0.

On Fri, 14 Mar 2008, Sergei Organov wrote:

>
> +                      ...  'rev{tilde}' is equivalent to 'rev{tilde}0'
> +  which in turn is equivalent to 'rev'.

I'd actually prefer to just fix that.

I think it would make more sense to have the same guarantees that rev^ has, namely to always return a commit. I would also suggest that not giving a number would have the same effect of defaulting to 1, not 0.

Right now it's a bit illogical, but at least it's an _undocumented_ illogical behaviour. If we document it, we should fix it and document the logical behaviour instead, no?

Here's a patch to make '^' and '~' act the same for the default count (ie both default to 1), and also have the same behaviour for a count of zero.

Before (no discernible pattern):

	[torvalds@woody git]$ git rev-parse v1.5.1 v1.5.1^0 v1.5.1~0 v1.5.1^ v1.5.1~
	45354a57ee7e3e42c7137db6c94fa968c6babe8d
	89815cab95268e8f0f58142b848ac4cd5e9cbdcb
	45354a57ee7e3e42c7137db6c94fa968c6babe8d
	045f5759c97746589a067461e50fad16f60711ac
	45354a57ee7e3e42c7137db6c94fa968c6babe8d

After (fairly logical):

	[torvalds@woody git]$ ./git rev-parse v1.5.1 v1.5.1^0 v1.5.1~0 v1.5.1^ v1.5.1~
	45354a57ee7e3e42c7137db6c94fa968c6babe8d
	89815cab95268e8f0f58142b848ac4cd5e9cbdcb
	89815cab95268e8f0f58142b848ac4cd5e9cbdcb
	045f5759c97746589a067461e50fad16f60711ac
	045f5759c97746589a067461e50fad16f60711ac

Hmm?

(That parent-finding loop is also now much tighter, not that it matters one whit ;)

		Linus

---
 sha1_name.c |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)

Show changes to sha1_name.c +13 −10

diff --git a/sha1_name.c b/sha1_name.c
index 8b6c76f..6afc0e8 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -407,18 +407,22 @@ static int get_nth_ancestor(const char *name, int len,
 			    unsigned char *result, int generation)
 {
 	unsigned char sha1[20];
-	int ret = get_sha1_1(name, len, sha1);
+	struct commit *commit;
+	int ret;
+
+	ret = get_sha1_1(name, len, sha1);
 	if (ret)
 		return ret;
+	commit = lookup_commit_reference(sha1);
+	if (!commit)
+		return -1;
 
 	while (generation--) {
-		struct commit *commit = lookup_commit_reference(sha1);
-
-		if (!commit || parse_commit(commit) || !commit->parents)
+		if (parse_commit(commit) || !commit->parents)
 			return -1;
-		hashcpy(sha1, commit->parents->item->object.sha1);
+		commit = commit->parents->item;
 	}
-	hashcpy(result, sha1);
+	hashcpy(result, commit->object.sha1);
 	return 0;
 }
 
@@ -564,11 +568,10 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1)
 		cp++;
 		while (cp < name + len)
 			num = num * 10 + *cp++ - '0';
-		if (has_suffix == '^') {
-			if (!num && len1 == len - 1)
-				num = 1;
+		if (!num && len1 == len - 1)
+			num = 1;
+		if (has_suffix == '^')
 			return get_parent(name, len1, sha1, num);
-		}
 		/* else if (has_suffix == '~') -- goes without saying */
 		return get_nth_ancestor(name, len1, sha1, num);
 	}

Sergei Organov· Mar 14, 2008, 19:11 UTC · re: Linus Torvalds · lore

Re: [PATCH] git-rev-parse.txt: clarify meaning of rev~ and rev~0.

Linus Torvalds <torvalds@linux-foundation.org> writes:

Show 15 quoted lines

> On Fri, 14 Mar 2008, Sergei Organov wrote:
>>
>> +                      ...  'rev{tilde}' is equivalent to 'rev{tilde}0'
>> +  which in turn is equivalent to 'rev'.
>
> I'd actually prefer to just fix that. 
>
> I think it would make more sense to have the same guarantees that rev^ 
> has, namely to always return a commit. I would also suggest that not 
> giving a number would have the same effect of defaulting to 1, not 0.
>
> Right now it's a bit illogical, but at least it's an _undocumented_ 
> illogical behaviour. If we document it, we should fix it and document the 
> logical behaviour instead, no?
>

I entirely agree. I just took (maybe erroneously) Junio's answer to my original question as "this behavior is by design", assuming some rationale behind it, so submitted the above patch to documentation.

-- Sergei.

Junio C Hamano· Mar 14, 2008, 20:13 UTC · re: Linus Torvalds · lore

Re: [PATCH] git-rev-parse.txt: clarify meaning of rev~ and rev~0.

Linus Torvalds <torvalds@linux-foundation.org> writes:

Show 14 quoted lines

> On Fri, 14 Mar 2008, Sergei Organov wrote:
>>
>> +                      ...  'rev{tilde}' is equivalent to 'rev{tilde}0'
>> +  which in turn is equivalent to 'rev'.
>
> I'd actually prefer to just fix that. 
>
> I think it would make more sense to have the same guarantees that rev^ 
> has, namely to always return a commit. I would also suggest that not 
> giving a number would have the same effect of defaulting to 1, not 0.
>
> Right now it's a bit illogical, but at least it's an _undocumented_ 
> illogical behaviour. If we document it, we should fix it and document the 
> logical behaviour instead, no?

Yeah, I like it. Not that I looked at your patch yet (which needs to wait til evening), but I agree with the intent.