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

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(+)

Richard Hansen <rhansen@bbn.com> writes:

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

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

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

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.

Right, so all of those would barf.

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

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

Richard Hansen <rhansen@bbn.com> writes:

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.

Richard Hansen wrote:

Interesting.

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

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.