Re: [PATCH 14/33] refs: extract a function peel_entry()
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- Apr 16, 2013, 13:07 UTC
- Message-ID
- <516D4D1F.5010207@alum.mit.edu>
- In-Reply-To
- <7v38urwv6o.fsf@alter.siamese.dyndns.org>
On 04/15/2013 07:38 PM, Junio C Hamano wrote:
Show 27 quoted lines
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> if (read_ref_full(refname, base, 1, &flag))
>> return -1;
>>
>> - if ((flag & REF_ISPACKED)) {
>> + /*
>> + * If the reference is packed, read its ref_entry from the
>> + * cache in the hope that we already know its peeled value.
>> + * We only try this optimization on packed references because
>> + * (a) forcing the filling of the loose reference cache could
>> + * be expensive and (b) loose references anyway usually do not
>> + * have REF_KNOWS_PEELED.
>> + */
>> + if (flag & REF_ISPACKED) {
>> struct ref_entry *r = get_packed_ref(refname);
>
> This code makes the reader wonder what happens when a new loose ref
> masks a stale packed ref, but the worry is unfounded because the
> read_ref_full() wouldn't have gave us REF_ISPACKED in the flag in
> such a case.
>
> But somehow the calling sequence looks like such a mistake waiting
> to happen. It would be much more clear if a function that returns a
> "struct ref_entry *" is used instead of read_ref_full() above, and
> we checked (r->flag & REF_ISPACKED) in the conditional, without a
> separate get_packed_ref(refname).As I'm sure you realize, I didn't change the code that you are referring to; I just added a comment.
But yes, I sympathize with your complaint. Additionally, the code has the drawback that get_packed_ref() is called twice: once in read_ref_full() and again in the if block here. Unfortunately, this isn't so easy to fix because read_ref_full() doesn't use the loose reference cache, so the reference that it returns might not even have a ref_entry associated with it (specifically, unless the returned flag value has REF_ISPACKED set). So there are a couple options:
* Always read loose references through the cache; that way there would always be a ref_entry in which the return value could be presented. This would not be a good idea at the moment because the loose reference cache is populated one directory at a time, and reading a whole directory of loose references could be expensive. So before implementing this, it would be advisable to change the code to populate the loose reference cache more selectively when single loose references are needed. -> This approach would be well beyond the scope of this patch series.
* Implement a function like read_ref_full() with an additional (struct ref_entry **entry) argument that is written to *in the case* that the reference that was returned has a ref_entry associated with it, and NULL otherwise. This would have to be an internal function because we don't want to expose the ref_entry structure outside of refs.c. read_ref_full() would be implemented on top of the new function.
Either way, I'd rather put this idea on my TODO list for another time.
Michael
-- Michael Haggerty mhagger@alum.mit.edu http://softwareswirl.blogspot.com/