Re: [PATCH 15/33] refs: change the internal reference-iteration API
Michael Haggerty writes: >>> ... 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. Yeah, I misread the message when I said "Hopefully". I somehow thought it was saying "we will fix it in the future; otherwise things can break in a confusing way", which is not the case. -- 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
Re: [PATCH 15/33] refs: change the internal reference-iteration API
On 04/15/2013 07:38 PM, Junio C Hamano wrote: > Michael Haggerty 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
Re: [PATCH 15/33] refs: change the internal reference-iteration API
Michael Haggerty writes: > Establish an internal API for iterating over references, which gives > the callback functions direct access to the ref_entry structure > describing the reference. (Do not change the iteration API that is > exposed outside of the module.) > > Define a new internal callback signature > >int each_ref_entry_fn(struct ref_entry *entry, void *cb_data) > > Change do_for_each_ref_in_dir() and do_for_each_ref_in_dirs() to > accept each_ref_entry_fn callbacks, and rename them to > do_for_each_entry_in_dir() and do_for_each_entry_in_dirs(), > respectively. Adapt their callers accordingly. > > Add a new function do_for_each_entry() analogous to do_for_each_ref() > but using the new callback style. Nicely done. > > Change do_one_ref() into an each_ref_entry_fn that does some > bookkeeping and then calls a wrapped each_ref_fn. > > Reimplement do_for_each_ref() in terms of do_for_each_entry(), using > do_one_ref() as an adapter. > > Please note that the responsibility for setting current_ref remains in > do_one_ref(), which means that current_ref is *not* set when iterating > over references via the new internal API. This is not a disadvantage, > because current_ref is not needed by callers of the internal API (they > receive a pointer to the current ref_entry anyway). 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" ;-) -- 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
