peel_ref: do not return a null sha1

The idea of the peel_ref function is to dereference tag
objects recursively until we hit a non-tag, and return the
sha1. Conceptually, it should return 0 if it is successful
(and fill in the sha1), or -1 if there was nothing to peel.

However, the current behavior is much more confusing. For a
regular loose ref, the behavior is as described above. But
there is an optimization to reuse the peeled-ref value for a
ref that came from a packed-refs file. If we have such a
ref, we return its peeled value, even if that peeled value
is null (indicating that we know the ref definitely does
_not_ peel).

It might seem like such information is useful to the caller,
who would then know not to bother loading and trying to peel
the object. Except that they should not bother loading and
trying to peel the object _anyway_, because that fallback is
already handled by peel_ref. In other words, the whole point
of calling this function is that it handles those details
internally, and you either get a sha1, or you know that it
is not peel-able.

This patch catches the null sha1 case internally and
converts it into a -1 return value (i.e., there is nothing
to peel). This simplifies callers, which do not need to
bother checking themselves.

Two callers are worth noting:

- in pack-objects, a comment indicates that there is a
difference between non-peelable tags and unannotated
tags. But that is not the case (before or after this
patch). Whether you get a null sha1 has to do with
internal details of how peel_ref operated.

- in show-ref, if peel_ref returns a failure, the caller
tries to decide whether to try peeling manually based on
whether the REF_ISPACKED flag is set. But this doesn't
make any sense. If the flag is set, that does not
necessarily mean the ref came from a packed-refs file
with the "peeled" extension. But it doesn't matter,
because even if it didn't, there's no point in trying to
peel it ourselves, as peel_ref would already have done
so. In other words, the fallback peeling is guaranteed
to fail.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/describe.c b/builtin/describe.c
index 9f63067f50a6f49d61d40474608535905bec905b..94b06069a0f79e2300c6e56239d61eeebde992d5 100644 (file)
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -144,7 +144,7 @@ static int get_name(const char *path, const unsigned char *sha1, int flag, void
        if (!all && !might_be_tag)
                return 0;
 
        if (!all && !might_be_tag)
                return 0;
 

-       if (!peel_ref(path, peeled) && !is_null_sha1(peeled)) {
+       if (!peel_ref(path, peeled)) {

                is_tag = !!hashcmp(sha1, peeled);
        } else {
                hashcpy(peeled, sha1);
                is_tag = !!hashcmp(sha1, peeled);
        } else {
                hashcpy(peeled, sha1);

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 782e7d0c38aa939a7db03266c3e6ff4993e4d2bd..035ed3b2d0e21a725c002779aad54987e317cee1 100644 (file)
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2033,7 +2033,6 @@ static int add_ref_tag(const char *path, const unsigned char *sha1, int flag, vo
 
        if (!prefixcmp(path, "refs/tags/") && /* is a tag? */
            !peel_ref(path, peeled)        && /* peelable? */
 
        if (!prefixcmp(path, "refs/tags/") && /* is a tag? */
            !peel_ref(path, peeled)        && /* peelable? */

-           !is_null_sha1(peeled)          && /* annotated tag? */

            locate_object_entry(peeled))      /* object packed? */
                add_object_entry(sha1, OBJ_TAG, NULL, 0);
        return 0;
            locate_object_entry(peeled))      /* object packed? */
                add_object_entry(sha1, OBJ_TAG, NULL, 0);
        return 0;

diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 3911661900f79562133b4ebd6cbb513a06232b5b..aaac2b22f942e0ea69404a0af4c198a8287154b0 100644 (file)
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -28,7 +28,6 @@ static void show_one(const char *refname, const unsigned char *sha1)
 
 static int show_ref(const char *refname, const unsigned char *sha1, int flag, void *cbdata)
 {
 
 static int show_ref(const char *refname, const unsigned char *sha1, int flag, void *cbdata)
 {

-       struct object *obj;

        const char *hex;
        unsigned char peeled[20];
 
        const char *hex;
        unsigned char peeled[20];
 


@@ -79,25 +78,9 @@ static int show_ref(const char *refname, const unsigned char *sha1, int flag, vo
        if (!deref_tags)
                return 0;
 
        if (!deref_tags)
                return 0;
 

-       if ((flag & REF_ISPACKED) && !peel_ref(refname, peeled)) {
-               if (!is_null_sha1(peeled)) {
-                       hex = find_unique_abbrev(peeled, abbrev);
-                       printf("%s %s^{}\n", hex, refname);
-               }
-       }
-       else {
-               obj = parse_object(sha1);
-               if (!obj)
-                       die("git show-ref: bad ref %s (%s)", refname,
-                           sha1_to_hex(sha1));
-               if (obj->type == OBJ_TAG) {
-                       obj = deref_tag(obj, refname, 0);
-                       if (!obj)
-                               die("git show-ref: bad tag at ref %s (%s)", refname,
-                                   sha1_to_hex(sha1));
-                       hex = find_unique_abbrev(obj->sha1, abbrev);
-                       printf("%s %s^{}\n", hex, refname);
-               }
+       if (!peel_ref(refname, peeled)) {
+               hex = find_unique_abbrev(peeled, abbrev);
+               printf("%s %s^{}\n", hex, refname);

        }
        return 0;
 }
        }
        return 0;
 }

diff --git a/refs.c b/refs.c
index 0a916a0c2aafea1c87333dee2da595d58db3f9e6..f672ad93a63bbab69ab2b4e8a5d0a9b6ee62ad77 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -1202,6 +1202,8 @@ int peel_ref(const char *refname, unsigned char *sha1)
        if (current_ref && (current_ref->name == refname
                || !strcmp(current_ref->name, refname))) {
                if (current_ref->flag & REF_KNOWS_PEELED) {
        if (current_ref && (current_ref->name == refname
                || !strcmp(current_ref->name, refname))) {
                if (current_ref->flag & REF_KNOWS_PEELED) {

+                       if (is_null_sha1(current_ref->u.value.peeled))
+                           return -1;

                        hashcpy(sha1, current_ref->u.value.peeled);
                        return 0;
                }
                        hashcpy(sha1, current_ref->u.value.peeled);
                        return 0;
                }