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

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Jun 20, 2013, 17:14 UTC
Message-ID: <CALkWK0n5NMBFC_eE8V_Avy57RrBOgDuUs+V_VMvNY6VkBBZyBQ@mail.gmail.com>
In-Reply-To: <1371605946-32565-1-git-send-email-rhansen@bbn.com>

Richard Hansen wrote:

Show 8 quoted lines

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

Show 5 quoted lines

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