Re: [PATCH v2 25/25] cache_ref_iterator_begin(): avoid priming unneeded directories

[Jeff King <peff@peff.net>]

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