Re: [PATCH 14/33] refs: extract a function peel_entry()

> On 04/15/2013 07:38 PM, Junio C Hamano wrote:
>> 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.