Junio C Hamano <gitster@xxxxxxxxx> writes:
> Patrick Steinhardt <ps@xxxxxx> 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
>
That's right.
> 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?
>
That's also right and that is indeed the intention. We're trying to make
the actions more intentional.
So if a user sets a 'prefix' for the iterator, all previous state of the
iterator is reset. So, the same function for seeking an iterator should
also have the same side-effect of resetting the previous state.
There could be a usecase where we add support for keeping the prefix,
while also seeking the iterator. That would be an explicit change
(perhaps with a corresponding flag?) that we'd have to build, add tests
for and call out. Until then, we explicitly reset the state whenever a
user calls 'ref_iterator_seek()', they can be sure that any previous
state is reset.
> Thanks.
Attachment:
signature.asc
Description: PGP signature
