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

On 04/15/2013 07:38 PM, Junio C Hamano wrote:

Michael Haggerty [off-list ref] writes:

quoted

[...]  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/