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

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

Isn't there another?

Instead of using read_ref_full() at this callsite, can it call a
function, given a refname, returns a pointer to "struct ref_entry",
using the proper "a loose ref covers the packed ref with the same
name" semantics?

I guess that may need the same machinery for your first option to
implement it efficiently; right now read_ref_full() goes directly to
the single file that would hold the named ref without scanning and
populating sibling refs in the in-core loose ref hierarchy, and
without doing so you cannot return a struct ref_entry.  Hmm...
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to