On 05/23/2017 09:45 PM, Jeff King wrote:
> 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;
Yes, you are right. Thanks for catching this.
Note: it has to be
free((char *)iter->prefix);
because `prefix` is const.
Junio, if a reroll is not needed for other reasons, would you mind
squashing this into the last patch of my series?
Michael
