Re: [PATCH v2 25/25] cache_ref_iterator_begin(): avoid priming unneeded directories
- From
Jeff King <peff@peff.net>
- Date
- May 23, 2017, 19:45 UTC
- Message-ID
- <20170523194519.luhuej3rerzb57t7@sigill.intra.peff.net>
- In-Reply-To
- <0d6a608a62026352c1496239bab02122f123f2e1.1495460199.git.mhagger@alum.mit.edu>
On Mon, May 22, 2017 at 04:17:55PM +0200, Michael Haggerty wrote:
Show 13 quoted lines
> 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:
Show 13 quoted lines
> @@ -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