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

From: Richard Hansen <rhansen@bbn.com>
Date: Jun 19, 2013, 20:08 UTC
Message-ID: <51C20FD1.4090203@bbn.com>
In-Reply-To: <7vvc5aymhd.fsf@alter.siamese.dyndns.org>

On 2013-06-19 14:38, Junio C Hamano wrote:

Show 5 quoted lines

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

Show 9 quoted lines

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

Show 17 quoted lines

> 
> 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] == '}')
>