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

[Junio C Hamano <gitster@pobox.com>]

Michael Haggerty <mhagger@alum.mit.edu> 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" ;-)