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