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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> 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.