threads / patch / 34844

v4peel_onion(): add support for <rev>^{tag}

Subject: [PATCH v4] peel_onion(): add support for <rev>^{tag}

## tl;dr

5 messages between Sep 3, 2013 and Sep 3, 2013. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Richard Hansen· Sep 3, 2013, 19:50 UTC · lore

Complete the <rev>^{<type>} family of object descriptors by having <rev>^{tag} dereference <rev> until a tag object is found (or fail if unable).

At first glance this may not seem very useful, as commits, trees, and blobs cannot be peeled to a tag, and a tag would just peel to itself. However, this can be used to ensure that <rev> names a tag object:

    $ git rev-parse --verify v1.8.4^{tag}
    04f013dc38d7512eadb915eba22efc414f18b869
    $ git rev-parse --verify master^{tag}
    error: master^{tag}: expected tag type, but the object dereferences to tree type
    fatal: Needed a single revision
Users can already ensure that <rev> is a tag object by checking the
output of 'git cat-file -t <rev>', but:
  * users may expect <rev>^{tag} to exist given that <rev>^{commit},
    <rev>^{tree}, and <rev>^{blob} all exist
  * this syntax is more convenient/natural in some circumstances
Signed-off-by: Richard Hansen <rhansen@bbn.com>
---
Changes from v3 (2013-09-03, see
<http://thread.gmane.org/gmane.comp.version-control.git/233752>):
  * Use Peff's simpler test case.
 Documentation/revisions.txt | 3 +++
 sha1_name.c                 | 2 ++
 t/t1511-rev-parse-caret.sh  | 7 +++++++
 3 files changed, 12 insertions(+)
Show changes to 3 files +12 −0

Documentation/revisions.txt, sha1_name.c, t/t1511-rev-parse-caret.sh

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index d477b3f..b3322ad 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -121,6 +121,9 @@ some output processing may assume ref names in UTF-8.
 object that exists, without requiring 'rev' to be a tag, and
 without dereferencing 'rev'; because a tag is already an object,
 it does not have to be dereferenced even once to get to an object.
++
+'rev{caret}\{tag\}' can be used to ensure that 'rev' identifies an
+existing tag object.
 
 '<rev>{caret}\{\}', e.g. 'v0.99.8{caret}\{\}'::
   A suffix '{caret}' followed by an empty brace pair
diff --git a/sha1_name.c b/sha1_name.c
index 65ad066..6dc496d 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -679,6 +679,8 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
 	sp++; /* beginning of type name, or closing brace for empty */
 	if (!strncmp(commit_type, sp, 6) && sp[6] == '}')
 		expected_type = OBJ_COMMIT;
+	else if (!strncmp(tag_type, sp, 3) && sp[3] == '}')
+		expected_type = OBJ_TAG;
 	else if (!strncmp(tree_type, sp, 4) && sp[4] == '}')
 		expected_type = OBJ_TREE;
 	else if (!strncmp(blob_type, sp, 4) && sp[4] == '}')
diff --git a/t/t1511-rev-parse-caret.sh b/t/t1511-rev-parse-caret.sh
index eaefc77..15973f2 100755
--- a/t/t1511-rev-parse-caret.sh
+++ b/t/t1511-rev-parse-caret.sh
@@ -54,6 +54,13 @@ test_expect_success 'ref^{tree}' '
 	test_must_fail git rev-parse blob-tag^{tree}
 '
 
+test_expect_success 'ref^{tag}' '
+	test_must_fail git rev-parse HEAD^{tag} &&
+	git rev-parse commit-tag >expected &&
+	git rev-parse commit-tag^{tag} >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'ref^{/.}' '
 	git rev-parse master >expected &&
 	git rev-parse master^{/.} >actual &&
-- 
1.8.4
Junio C Hamano· Sep 3, 2013, 20:10 UTC · re: Richard Hansen · lore

Re: [PATCH v4] peel_onion(): add support for <rev>^{tag}

Richard Hansen <rhansen@bbn.com> writes:
Show 15 quoted lines
> diff --git a/t/t1511-rev-parse-caret.sh b/t/t1511-rev-parse-caret.sh
> index eaefc77..15973f2 100755
> --- a/t/t1511-rev-parse-caret.sh
> +++ b/t/t1511-rev-parse-caret.sh
> @@ -54,6 +54,13 @@ test_expect_success 'ref^{tree}' '
>  	test_must_fail git rev-parse blob-tag^{tree}
>  '
>  
> +test_expect_success 'ref^{tag}' '
> +	test_must_fail git rev-parse HEAD^{tag} &&
> +	git rev-parse commit-tag >expected &&
> +	git rev-parse commit-tag^{tag} >actual &&
> +	test_cmp expected actual
> +'
> +
Looks good to me.  Testing both failure case and success case.
Thanks; will queue.
Jeff King· Sep 3, 2013, 20:20 UTC · re: Richard Hansen · lore

[PATCH 2/1] peel_onion: do not assume length of x_type globals

When we are parsing "rev^{foo}", we check "foo" against the various global type strings, like "commit_type", "tree_type", etc. This is nicely abstracted, but then we destroy the abstraction completely by using magic numbers that must match the length of the type strings.

We can avoid these magic numbers by using skip_prefix. In an ideal world, the compiler would compute the magic number without even calling strlen() at runtime. However, it cannot do so in this case because we hide the definition of commit_type and friends behind an extern declaration.

This shouldn't matter, though, as this is not a performance critical code path. A few short strlens inside get_sha1() will not even be measurable. And if we care, we can adjust the declaration of commit_type and friends later on.

Signed-off-by: Jeff King <peff@peff.net>
---
This is the potential cleanup we discussed. Having written that commit
message, I wonder if it is really even worth using commit_type at all.
It is not like that variable can ever change without the git data
structure changing, and even if it did, we would almost certainly want
to revisit how that change affects the user-visible "rev^{}" syntax
anyway.
So maybe simply:
  if (!prefixcmp("commit}"))

would be fine, and it is way more readable. That's what we do already for "rev^{object}".

 sha1_name.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
Show changes to sha1_name.c +5 −5
diff --git a/sha1_name.c b/sha1_name.c
index 6dc496d..23d0d71 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -652,7 +652,7 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
 static int peel_onion(const char *name, int len, unsigned char *sha1)
 {
 	unsigned char outer[20];
-	const char *sp;
+	const char *sp, *x;
 	unsigned int expected_type = 0;
 	unsigned lookup_flags = 0;
 	struct object *o;
@@ -677,13 +677,13 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
 		return -1;
 
 	sp++; /* beginning of type name, or closing brace for empty */
-	if (!strncmp(commit_type, sp, 6) && sp[6] == '}')
+	if ((x = skip_prefix(sp, commit_type)) && *x == '}')
 		expected_type = OBJ_COMMIT;
-	else if (!strncmp(tag_type, sp, 3) && sp[3] == '}')
+	else if ((x = skip_prefix(sp, tag_type)) && *x == '}')
 		expected_type = OBJ_TAG;
-	else if (!strncmp(tree_type, sp, 4) && sp[4] == '}')
+	else if ((x = skip_prefix(sp, tree_type)) && *x == '}')
 		expected_type = OBJ_TREE;
-	else if (!strncmp(blob_type, sp, 4) && sp[4] == '}')
+	else if ((x = skip_prefix(sp, blob_type)) && *x == '}')
 		expected_type = OBJ_BLOB;
 	else if (!prefixcmp(sp, "object}"))
 		expected_type = OBJ_ANY;
-- 
1.8.4.2.g87d4a77
Jeff King· Sep 3, 2013, 20:27 UTC · re: Jeff King · lore

[PATCH 2/1 alt] peel_onion: do not assume length of x_type globals

When we are parsing "rev^{foo}", we check "foo" against the various global type strings, like "commit_type", "tree_type", etc. This is nicely abstracted, but then we destroy the abstraction completely by using magic numbers that must match the length of the type strings.

We could avoid these magic numbers by using skip_prefix. But taking a step back, we can realize that using the "commit_type" global is not really buying us anything. It is not ever going to change from being "commit" without causing severe breakage to existing uses. And even if it did change for some crazy reason, we would want to evaluate its effects on the "rev^{}" syntax, anyway.

Let's just switch these to using a custom string literal, as we do for "rev^{object}". The resulting code is more robust to changes in the type strings, and is more readable.

Signed-off-by: Jeff King <peff@peff.net>
---
If you _really_ wanted to abstract it, you could make commit_type a
macro and use string concatenation along with prefixcmp. But that is
going in the direction of less readable, I think. :)

This has probably consumed enough brain cycles for such a small and probably unimportant cleanup. I'll let Junio pick from the 2 options (or choose to do nothing at all) as he sees fit.

 sha1_name.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to sha1_name.c +4 −4
diff --git a/sha1_name.c b/sha1_name.c
index 6dc496d..2f6e5ab 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -677,13 +677,13 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
 		return -1;
 
 	sp++; /* beginning of type name, or closing brace for empty */
-	if (!strncmp(commit_type, sp, 6) && sp[6] == '}')
+	if (!prefixcmp(sp, "commit}"))
 		expected_type = OBJ_COMMIT;
-	else if (!strncmp(tag_type, sp, 3) && sp[3] == '}')
+	else if (!prefixcmp(sp, "tag}"))
 		expected_type = OBJ_TAG;
-	else if (!strncmp(tree_type, sp, 4) && sp[4] == '}')
+	else if (!prefixcmp(sp, "tree}"))
 		expected_type = OBJ_TREE;
-	else if (!strncmp(blob_type, sp, 4) && sp[4] == '}')
+	else if (!prefixcmp(sp, "blob}"))
 		expected_type = OBJ_BLOB;
 	else if (!prefixcmp(sp, "object}"))
 		expected_type = OBJ_ANY;
-- 
1.8.4.2.g87d4a77
Junio C Hamano· Sep 3, 2013, 20:46 UTC · re: Jeff King · lore

Re: [PATCH 2/1 alt] peel_onion: do not assume length of x_type globals

Jeff King <peff@peff.net> writes:
> If you _really_ wanted to abstract it, you could make commit_type a
> macro and use string concatenation along with prefixcmp. But that is
> going in the direction of less readable, I think. :)
;-) we are on the same wave-length.
Thanks, will apply.
Show 30 quoted lines
>
> This has probably consumed enough brain cycles for such a small and
> probably unimportant cleanup. I'll let Junio pick from the 2 options (or
> choose to do nothing at all) as he sees fit.
>
>  sha1_name.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 6dc496d..2f6e5ab 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -677,13 +677,13 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
>  		return -1;
>  
>  	sp++; /* beginning of type name, or closing brace for empty */
> -	if (!strncmp(commit_type, sp, 6) && sp[6] == '}')
> +	if (!prefixcmp(sp, "commit}"))
>  		expected_type = OBJ_COMMIT;
> -	else if (!strncmp(tag_type, sp, 3) && sp[3] == '}')
> +	else if (!prefixcmp(sp, "tag}"))
>  		expected_type = OBJ_TAG;
> -	else if (!strncmp(tree_type, sp, 4) && sp[4] == '}')
> +	else if (!prefixcmp(sp, "tree}"))
>  		expected_type = OBJ_TREE;
> -	else if (!strncmp(blob_type, sp, 4) && sp[4] == '}')
> +	else if (!prefixcmp(sp, "blob}"))
>  		expected_type = OBJ_BLOB;
>  	else if (!prefixcmp(sp, "object}"))
>  		expected_type = OBJ_ANY;

← back to recent threads