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

> 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;