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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to