Re: [PATCH v3 3/4] refs: selectively set prefix in the seek functions
Patrick Steinhardt <ps@pks.im> writes:
Show 24 quoted lines
> On Tue, Jul 08, 2025 at 03:47:48PM +0200, Karthik Nayak wrote:
>> diff --git a/refs.h b/refs.h
>> index 7c21aaef3d..7852ad36f3 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -1299,21 +1299,32 @@ struct ref_iterator *refs_ref_iterator_begin(
>> */
>> int ref_iterator_advance(struct ref_iterator *ref_iterator);
>>
>> +enum ref_iterator_seek_flag {
>> + /*
>> + * Also set the seek pattern as a prefix for iteration. This ensures
>> + * that only references which match the prefix are yielded.
>> + */
>> + REF_ITERATOR_SEEK_SET_PREFIX = (1 << 0),
>> +};
>> +
>
> Nit: I think it's a tiny bit confusing that the documentation of this
> enum is split up across here and the doc of `ref_iterator_seek()`. I
> think it would be sensible to move the last paragraph of the function
> over here so that the whole behaviour of the enum is explained in a
> single place.
>Yeah I think that makes sense.
Show 32 quoted lines
>> /*
>> - * Seek the iterator to the first reference with the given prefix.
>> - * The prefix is matched as a literal string, without regard for path
>> - * separators. If prefix is NULL or the empty string, seek the iterator to the
>> + * Seek the iterator to the first reference matching the given seek string.
>> + * The seek string is matched as a literal string, without regard for path
>> + * separators. If seek is NULL or the empty string, seek the iterator to the
>> * first reference again.
>> *
>> - * This function is expected to behave as if a new ref iterator with the same
>> - * prefix had been created, but allows reuse of iterators and thus may allow
>> - * the backend to optimize. Parameters other than the prefix that have been
>> - * passed when creating the iterator will remain unchanged.
>> + * This function is expected to behave as if a new ref iterator has been
>> + * created, but allows reuse of existing iterators for optimization.
>> + *
>> + * 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.
>
Fair enough, let me change that.
> 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.
>
Show 22 quoted lines
>> diff --git a/refs/iterator.c b/refs/iterator.c
>> index 766d96e795..f2364bd6e7 100644
>> --- a/refs/iterator.c
>> +++ b/refs/iterator.c
>> @@ -407,13 +408,16 @@ static int prefix_ref_iterator_advance(struct ref_iterator *ref_iterator)
>> }
>>
>> static int prefix_ref_iterator_seek(struct ref_iterator *ref_iterator,
>> - const char *prefix)
>> + const char *seek, unsigned int flags)
>> {
>> struct prefix_ref_iterator *iter =
>> (struct prefix_ref_iterator *)ref_iterator;
>> - free(iter->prefix);
>> - iter->prefix = xstrdup_or_null(prefix);
>> - return ref_iterator_seek(iter->iter0, prefix);
>> +
>> + if (flags & REF_ITERATOR_SEEK_SET_PREFIX) {
>> + free(iter->prefix);
>> + iter->prefix = xstrdup_or_null(seek);
>> + }
>> + return ref_iterator_seKarthik Nayak <karthik.188@gmail.com>