Re: [PATCH v3 3/4] refs: selectively set prefix in the seek functions

> Patrick Steinhardt <ps@pks.im> writes:
>
>>> + * When the REF_ITERATOR_SEEK_SET_PREFIX flag is set, the iterator's prefix is
>>> + * updated to match the seek string, affecting all subsequent iterations. If
>>> + * not, the iterator seeks to the specified reference and clears any previously
>>> + * set prefix.
>>>   *
>>>   * Returns 0 on success, a negative error code otherwise.
>>>   */
>>> -int ref_iterator_seek(struct ref_iterator *ref_iterator,
>>> -		      const char *prefix);
>>> +int ref_iterator_seek(struct ref_iterator *ref_iterator, const char *seek,
>>> +		      unsigned int flags);
>>
>> Another tiny nit: instead of calling the variable `seek` we can just
>> call it `refname`. That might give a bit more of a hint what you're
>> actually seeking for.
>>
>> But other than that I'm happy with the new behaviour, where we are now
>> consistently either setting or resetting the prefix depending on whether
>> or not the caller set the flag.
>
> I am not sure.  The way the "prefix" is used, if I understand correctly, is
>
>  - it is set by iterator-begin, typically to the area to iterate
>    over (e.g. "refs/heads/" for iterating over branches) in the
>    for_each_ref_*() family of helpers, and internally we seek to
>    that area (skipping anything that come strictly before
>    "refs/heads/" for example).
>
>  - iterator-advance looks at it and decides we are done when the
>    iterator points beyond that prefix
>

> So if you are iterating inside "refs/heads/" hierarchy and seek to
> "refs/heads/m", don't you still want to stop when you step outside
> "refs/heads/" by keeping the original prefix, instead of unsetting
> the prefix to empty?  A postimage of this patch for packed backend
> (picked at random) reads like this:
>
>         static int packed_ref_iterator_seek(struct ref_iterator *ref_iterator,
>                                             const char *seek, unsigned int flags)
>         {
>                 struct packed_ref_iterator *iter =
>                         (struct packed_ref_iterator *)ref_iterator;
>                 const char *start;
>
>                 if (seek && *seek)
>                         start = find_reference_location(iter->snapshot, seek, 0);
>                 else
>                         start = iter->snapshot->start;
>
>                 /* Unset any previously set prefix */
>                 FREE_AND_NULL(iter->prefix);
>
>                 if (flags & REF_ITERATOR_SEEK_SET_PREFIX)
>                         iter->prefix = xstrdup_or_null(seek);
>
> so after (true) seeking that does not have the SET_PREFIX flag on,
> wouldn't our iterator-advance run through the end since it no longer
> is aware of where to stop?
>