Re: [PATCH v2 03/20] refs_ref_iterator_begin(): new function
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- Apr 16, 2017, 04:39 UTC
- Message-ID
- <3b508517-80fc-d759-d27f-a582a36286ba@alum.mit.edu>
- In-Reply-To
- <CACsJy8CECcC1EDpr8u39Pzq75E5KXrYwm_Bz+UTXobWvgV5weA@mail.gmail.com>
On 04/07/2017 12:57 PM, Duy Nguyen wrote:
Show 33 quoted lines
> On Fri, Mar 31, 2017 at 9:11 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> Extract a new function from `do_for_each_ref()`. It will be useful
>> elsewhere.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>> refs.c | 15 +++++++++++++--
>> refs/refs-internal.h | 11 +++++++++++
>> 2 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index 0ed6c3c7a4..aeb704ab92 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1230,6 +1230,18 @@ int head_ref(each_ref_fn fn, void *cb_data)
>> return head_ref_submodule(NULL, fn, cb_data);
>> }
>>
>> +struct ref_iterator *refs_ref_iterator_begin(
>> + struct ref_store *refs,
>> + const char *prefix, int trim, int flags)
>> +{
>> + struct ref_iterator *iter;
>> +
>> + iter = refs->be->iterator_begin(refs, prefix, flags);
>> + iter = prefix_ref_iterator_begin(iter, prefix, trim);
>
> Off topic. This code made me wonder if we really need the prefix
> iterator if prefix is NULL. And in fact we don't since
> prefix_ref_iterator_begin() will return the old iter in that case. But
> it's probably better to move that optimization outside. I think it's
> easier to understand that way, calling prefix_ref_ will always give
> you a new iterator. Don't call it unless you want to have it.I like this aspect of the design because it is more powerful. Consider for example that some reference backend might (e.g., a future `packed_ref_store`) need to use a `prefix_ref_iterator` to implement `iterator_begin()`, while another (e.g., one based on `ref_cache`) might not. It would be easy to make `prefix_ref_iterator_begin()` check whether its argument is already a `prefix_ref_iterator`, and if so, swap it out with a new one with different arguments (to avoid having to stack them up). It would be inappropriate for the caller to make such an optimization, because it (a) shouldn't have to care what type of reference iterator it is dealing with, and (b) shouldn't have to know enough about `prefix_ref_iterator`s to know that the optimization makes sense.
Show 10 quoted lines
>> +/* >> + * Return an iterator that goes over each reference in `refs` for >> + * which the refname begins with prefix. If trim is non-zero, then >> + * trim that many characters off the beginning of each refname. flags >> + * can be DO_FOR_EACH_INCLUDE_BROKEN to include broken references in >> + * the iteration. >> + */ > > Do we need a separate docstring here? I think we document more or less > the same for ref_iterator_begin_fn (except the include-broken flag).
There is a subtle difference: currently, `ref_iterator_begin_fn()` doesn't use its full `prefix` argument as prefix, but rather `find_containing_dir(prefix)`. (This is basically an implementation detail of `ref_cache` leaking out into the virtual function interface.)
`refs_ref_iterator_begin()`, on the other hand, treats the prefix literally, using `starts_with()`.
I don't like this design and plan to improve it sometime, but for now that's an important difference. The fix, incidentally, would motivate the `prefix_ref_iterator_begin()` optimization mentioned above.
Michael