Re: [PATCH v2 03/20] refs_ref_iterator_begin(): new function

[Michael Haggerty <mhagger@alum.mit.edu>]

On 04/07/2017 12:57 PM, Duy Nguyen wrote:
> 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.

>> +/*
>> + * 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