Re: [PATCH 15/33] refs: change the internal reference-iteration API

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

On 04/15/2013 07:38 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> [...]  But more
>> importantly, this change prevents peel_ref() from returning invalid
>> results in the following scenario:
>>
>> When iterating via the external API, the iteration always includes
>> both packed and loose references, and in particular never presents a
>> packed ref if there is a loose ref with the same name.  The internal
>> API, on the other hand, gives the option to iterate over only the
>> packed references.  During such an iteration, there is no check
>> whether the packed ref might be hidden by a loose ref of the same
>> name.  But until now the packed ref was recorded in current_ref during
>> the iteration.  So if peel_ref() were called with the reference name
>> corresponding to current ref, it would return the peeled version of
>> the packed ref even though there might be a loose ref that peels to a
>> different value.  This scenario doesn't currently occur in the code,
>> but fix it to prevent things from breaking in a very confusing way in
>> the future.
> 
> Hopefully that means "in later patches in this series" ;-)

I don't think that the rest of the series would have triggered this
problem either.  In fact, if I had written repack_without_ref()'s
peeling functionality using peel_ref(), then it would have *depended* on
this bug for its proper operation...otherwise it would have written the
peeled version of the loose ref to the packed-ref file.  Of course, it's
all pretty academic because the peeled version of a packed ref should
never be used when it is overridden by a loose ref, so the incorrect
peeled values in the packed-ref file shouldn't have any observable effects.

The real problem is that calling the old peel_ref() function on a packed
reference was illegitimate because the function only knew how to peel a
ref that was still active.  Plus it's kindof silly tucking away the
current reference in a global variable then looking it up again instead
of passing the ref_entry around.

Callers outside of refs.c could also not have triggered this bug because
they have no way to access overridden packed refs.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/