Re: Tag peeling peculiarities

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 15, 2013, 16:28 UTC
Message-ID: <7vzjy4vd84.fsf@alter.siamese.dyndns.org>
In-Reply-To: <5142ADAA.6050503@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 5 quoted lines

> What is stored in ref_value.peeled?  Is it the peeled version of
> ref_value.sha1, or is it the peeled version of the associated refname?
> Because they are not necessarily the same thing: an entry in the packed
> ref_cache *might* be overridden by a loose reference with the same
> refname but a different SHA1 which peels to a different value.

The way it should work is that we look for loose ones first and then only if we do not find a loose one we use packed one, when asked for a ref by name. The .sha1 and .peeled fields for a single ref_entry struct must be consistent with each other, even though you might have got a ref_value by reading the packed-refs file and another ref_value by reading loose one and have both in core. When you have both packed and loose, the former should not be used at all, and it certainly should not be pointed by current_ref.

Stepping back a bit, I suspect that it may be worth evaluating to see if it still makes sense to keep the current_ref optimization that was introduced in 0ae91be0e1fa (Optimize peel_ref for the current ref of a for_each_ref callback, 2008-02-24). Back then, it was fairly expensive to find a cached_ref entry by name, because it was implemented as two linked lists (one for loose, one for packed) and we would have had to traverse them to answer the question.

Since then, e9c4c11165e4 (refs: Use binary search to lookup refs faster, 2011-09-29) introduced ref_array to make it more efficient to find a ref_entry by name, and your more recent series that includes bc5fd6d3c29f (refs.c: reorder definitions more logically, 2012-04-10) further touched that codepath for enumeration in a subtree.

I am not sure if that last change helped or hurt the performance of a single ref lookup, but we can certainly say that the performance characteristics of read_ref_full() is very different in today's code and the current_ref optimization may no longer be of much value.