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

[Junio C Hamano <gitster@pobox.com>]

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?

Thanks.