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

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

[Richard Hansen]

gitrevisions(7) implies that <rev>^{tag} should work, but before now
it did not:

    $ git rev-parse --verify v1.8.3.1^{tag}
    fatal: Needed a single revision

This commit fixes the omission:

    $ git rev-parse --verify v1.8.3.1^{tag}
    4bc068950abec778a02ebf3ee73cf0735affabb4

Other object type names work as expected:

    $ git rev-parse --verify v1.8.3.1^{commit}
    362de916c06521205276acb7f51c99f47db94727
    $ git rev-parse --verify v1.8.3.1^{tree}
    c1fef48675edd74e9af19405339e8bdaebd56b0c
    $ git rev-parse --verify v1.8.3.1^{blob}
    error: v1.8.3.1^{blob}: expected blob type, but the object dereferences to tree type
    fatal: Needed a single revision

Note that <rev>^{tag} is not the same as <rev>^{object} when <rev> is
not a tag:

    $ git rev-parse --verify v1.8.3.1^{}^{object}
    362de916c06521205276acb7f51c99f47db94727
    $ git rev-parse --verify v1.8.3.1^{}^{tag}
    error: v1.8.3.1^{}^{tag}: expected tag type, but the object dereferences to tree type
    fatal: Needed a single revision

Signed-off-by: Richard Hansen <rhansen@bbn.com>
---
 sha1_name.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sha1_name.c b/sha1_name.c
index 90419ef..68fd0e4 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -677,6 +677,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] == '}')
-- 
1.8.3.1

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

[Junio C Hamano]

Richard Hansen <rhansen@bbn.com> writes:

> gitrevisions(7) implies that <rev>^{tag} should work,...

Does it?  Is it possible that that should be fixed?

What does it even _mean_ to peel something to a TAG?

A commit, a tree or a blob cannot be peeled to a tag---none of them
can contain a tag.

When you have a tag that points at something else, what you have is
already a tag, so <that-tag>^{tag} would be <that-tag> itself.

Even more confusingly, when you have a tag that points at another
tag, what does <that-outer-tag>^{tag} mean?  The outer tag itself,
or do you need to peel at least once to reveal the inner-tag?  What
if that inner-tag points at yet another tag?

The patch does not touch peel_to_type(), so your answer to the above
question seems to be "if T is already a tag, T^{tag} is T itself",
but then that operation does not look all that useful.

Confused...

> diff --git a/sha1_name.c b/sha1_name.c
> index 90419ef..68fd0e4 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -677,6 +677,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] == '}')

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

[Richard Hansen]

On 2013-06-19 14:38, Junio C Hamano wrote:
> Richard Hansen <rhansen@bbn.com> writes:
> 
>> gitrevisions(7) implies that <rev>^{tag} should work,...
> 
> Does it?  Is it possible that that should be fixed?

Depends on whether you think ^{tag} is a useful feature or not; see below.

> 
> What does it even _mean_ to peel something to a TAG?

It's the same as peeling something to any other object type:  If the
object is that type, done.  Otherwise dereference and try again.  If it
can't be dereferenced, barf.

> 
> A commit, a tree or a blob cannot be peeled to a tag---none of them
> can contain a tag.

Right, so all of those would barf.

> 
> When you have a tag that points at something else, what you have is
> already a tag, so <that-tag>^{tag} would be <that-tag> itself.

Exactly, just like <object>^{object} is <object> itself.

> 
> Even more confusingly, when you have a tag that points at another
> tag, what does <that-outer-tag>^{tag} mean?  The outer tag itself,
> or do you need to peel at least once to reveal the inner-tag?  What
> if that inner-tag points at yet another tag?
> 
> The patch does not touch peel_to_type(), so your answer to the above
> question seems to be "if T is already a tag, T^{tag} is T itself",
> but then that operation does not look all that useful.

Barfing on non-tags is the feature this adds.  It's otherwise useless,
just like <object>^{object} is useless except to barf when <object>
doesn't exist.

It's a sometimes-convenient way to assert that an object specifier
refers to a tag object and not something else.  For example, instead of:

   fatal() { printf %s\\n "ERROR: $*" >&2; exit 1; }

   type=$(git cat-file -t "$1") || fatal "$1 is not a valid object"
   [ "${type}" = tag ] || fatal "$1 is not a tag object"
   use "$1" here

you can do:

   use "$1"^{tag} here

-Richard


> 
> Confused...
> 
>> diff --git a/sha1_name.c b/sha1_name.c
>> index 90419ef..68fd0e4 100644
>> --- a/sha1_name.c
>> +++ b/sha1_name.c
>> @@ -677,6 +677,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] == '}')
> 

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

[Junio C Hamano]

Richard Hansen <rhansen@bbn.com> writes:

> Barfing on non-tags is the feature this adds.  It's otherwise useless,
> just like <object>^{object} is useless except to barf when <object>
> doesn't exist.

Thanks.

I could buy that.  And after re-reading the proposed log message,
you do not quite have anything to say that.  Instead, you have this:

    Note that <rev>^{tag} is not the same as <rev>^{object} when <rev> is
    not a tag:

        $ git rev-parse --verify v1.8.3.1^{}^{object}
        362de916c06521205276acb7f51c99f47db94727
        $ git rev-parse --verify v1.8.3.1^{}^{tag}
        error: v1.8.3.1^{}^{tag}: expected tag type, but the object deref...
        fatal: Needed a single revision

The latter peels v1.8.3.1 to a non-tag (i.e. a commit) and then asks
to peel that commit to a tag, which will of course fail, but that is
not a good example.  

Perhaps something like this instead.

    Note that <rev>^{tag} can be used to make sure <rev> names a tag:

        $ git rev-parse --verify v1.8.3.1^{tag}
        $ git rev-parse --verify master^{tag}

    The former succeeds, while the latter fails.

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

[Ramkumar Ramachandra]

Richard Hansen wrote:
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -677,6 +677,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;

Interesting.

> gitrevisions(7) implies that <rev>^{tag} should work, but before now
> it did not:

The wording (especially of <rev>^{}) special-cases tags as "objects to
dereference".

>     $ git rev-parse --verify v1.8.3.1^{}^{object}
>     362de916c06521205276acb7f51c99f47db94727
>     $ git rev-parse --verify v1.8.3.1^{}^{tag}
>     error: v1.8.3.1^{}^{tag}: expected tag type, but the object dereferences to tree type
>     fatal: Needed a single revision

And the points out the problem: while ^{object} means "expect
OBJ_ANY", ^{} means "expect OBJ_NONE".  What does that even mean?  See
sha1_name.c:704 where this is handled sneakily: it just calls
deref_tag(), bypassing peel_to_type() altogether.  If anything, ^{} is
already a poor-man's version of your ^{tag}; the reason it's a
poor-man's version is precisely because it doesn't error out when
<rev> isn't a tag, while your ^{tag} does.  I would argue that ^{} is
very poorly done and must be deprecated in favor of your ^{tag}.  What
is the point of using ^{} if you can't even be sure that what you get
is a deref?  peel_to_type() already does the right thing by not using
deref_tag(), and explicitly checking.

Your commit message needs some tweaking, but I'm happy with your patch
otherwise.

Thanks.