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

[Michael Haggerty <mhagger@alum.mit.edu>]

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.

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/