On Mon, May 22, 2017 at 04:17:55PM +0200, Michael Haggerty wrote:
> So:
>
> * Move the responsibility for doing the prefix check directly to
> `cache_ref_iterator`. This means that `cache_ref_iterator_begin()`
> never has to wrap its return value in a `prefix_ref_iterator`.
>
> * Teach `cache_ref_iterator_begin()` (and `prime_ref_dir()`) to be
> stricter about what they iterate over and what directories they
> prime.
>
> * Teach `cache_ref_iterator` to keep track of whether the current
> `cache_ref_iterator_level` is fully within the prefix. If so, skip
> the prefix checks entirely.
As promised, I came back to this one with a more careful eye. These
changes all make sense to me, and the implementation matches.
My only question is:
> @@ -511,9 +582,12 @@ struct ref_iterator *cache_ref_iterator_begin(struct
> ref_cache *cache,
> level->index = -1;
> level->dir = dir;
>
> - if (prefix && *prefix)
> - ref_iterator = prefix_ref_iterator_begin(ref_iterator,
> - prefix, 0);
> + if (prefix && *prefix) {
> + iter->prefix = xstrdup(prefix);
> + level->prefix_state = PREFIX_WITHIN_DIR;
> + } else {
> + level->prefix_state = PREFIX_CONTAINS_DIR;
> + }
Who frees the prefix? Does this need:
diff --git a/refs/ref-cache.c b/refs/ref-cache.c
index fda3942db..a3efc5c51 100644
--- a/refs/ref-cache.c
+++ b/refs/ref-cache.c
@@ -542,6 +542,7 @@ static int cache_ref_iterator_abort(struct ref_iterator
*ref_iterator)
struct cache_ref_iterator *iter =
(struct cache_ref_iterator *)ref_iterator;
+ free(iter->prefix);
free(iter->levels);
base_ref_iterator_free(ref_iterator);
return ITER_DONE;
-Peff
