Re: [PATCH 2/4] peel_ref: do not return a null sha1

Jeff King [off-list ref] writes:

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.

Yeah, this is what I was wondering while reviewing [1/4].

We traditionally said both HEAD^0 and HEAD^0^0 peel to HEAD, but
this at least at the internal API level redefines them to "these do
not peel at all and is a failure".  In other words, peel_ref(ref,
sha1) that returns 0 is a way to see if ref points at a real tag
object, and if the function returns non-zero, the caller cannot tell
if the failure is because it was a valid commit or a corrupt object.

The check !is_null_sha1(peeled) always looked fishy to me.  Good
riddance.