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

[Michael Haggerty]

On 04/07/2017 12:57 PM, Duy Nguyen wrote:
> On Fri, Mar 31, 2017 at 9:11 PM, Michael Haggerty  
> wrote:
>> Extract a new function from `do_for_each_ref()`. It will be useful
>> elsewhere.
>>
>> Signed-off-by: Michael Haggerty 
>> ---
>>  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

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

[Duy Nguyen]

On Fri, Mar 31, 2017 at 9:11 PM, Michael Haggerty  wrote:
> Extract a new function from `do_for_each_ref()`. It will be useful
> elsewhere.
>
> Signed-off-by: Michael Haggerty 
> ---
>  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.

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

> +struct ref_iterator *refs_ref_iterator_begin(
> +   struct ref_store *refs,
> +   const char *prefix, int trim, int flags);
> +
-- 
Duy

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

[Michael Haggerty]

Extract a new function from `do_for_each_ref()`. It will be useful
elsewhere.

Signed-off-by: Michael Haggerty 
---
 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);
+
+   return iter;
+}
+
 /*
  * Call fn for each reference in the specified submodule for which the
  * refname begins with prefix. If trim is non-zero, then trim that
@@ -1247,8 +1259,7 @@ static int do_for_each_ref(struct ref_store *refs, const 
char *prefix,
if (!refs)
return 0;
 
-   iter = refs->be->iterator_begin(refs, prefix, flags);
-   iter = prefix_ref_iterator_begin(iter, prefix, trim);
+   iter = refs_ref_iterator_begin(refs, prefix, trim, flags);
 
return do_for_each_ref_iterator(iter, fn, cb_data);
 }
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 6ee9f20dbc..545989ae7f 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -335,6 +335,17 @@ struct ref_iterator *empty_ref_iterator_begin(void);
  */
 int is_empty_ref_iterator(struct ref_iterator *ref_iterator);
 
+/*
+ * 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.
+ */
+struct ref_iterator *refs_ref_iterator_begin(
+   struct ref_store *refs,
+   const char *prefix, int trim, int flags);
+
 /*
  * A callback function used to instruct merge_ref_iterator how to
  * interleave the entries from iter0 and iter1. The function should
-- 
2.11.0