On 04/15/2013 07:38 PM, Junio C Hamano wrote: > Michael Haggerty <mhag...@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 mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- 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
