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

[Karthik Nayak <karthik.188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3717 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Tue, Jul 01, 2025 at 05:03:29PM +0200, Karthik Nayak wrote:
>> The ref iterator exposes a `ref_iterator_seek()` function. The name
>> suggests that this would seek the iterator to a specific reference in
>> some ways similar to how `fseek()` works for the filesystem.
>>
>> However, the function actually sets the prefix for refs iteration. So
>> further iteration would only yield references which match the particular
>> prefix. This is a bit confusing.
>>
>> Let's add a 'set_prefix' field to the function, which when set, will set
>> the prefix for the iteration in-line with the existing behavior. But
>> when the 'set_prefix' field is not set, the reference backends will
>> simply seek to the specified reference without setting prefix. This
>> allows users to start iteration from a specific reference.
>>
>> In the packed and reftable backend, since references are available in a
>> sorted list, the changes are simply setting the prefix if needed. The
>> changes on the files-backend are a little more involved, since the files
>> backend uses the 'ref-cache' mechanism. We move out the existing logic
>> within `cache_ref_iterator_seek()` to `cache_ref_iterator_set_prefix()`
>> which is called when `set_prefix` is set. We then parse the provided
>> seek string and set the required levels and their indexes to ensure that
>> seeking is possible.
>
> That solution makes sense.
>
>> diff --git a/refs.c b/refs.c
>> index dce5c49ca2..a4220d3537 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2669,7 +2669,7 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs
>>  			if (!iter) {
>>  				iter = refs_ref_iterator_begin(refs, dirname.buf, NULL, 0,
>>  							       DO_FOR_EACH_INCLUDE_BROKEN);
>> -			} else if (ref_iterator_seek(iter, dirname.buf) < 0) {
>> +			} else if (ref_iterator_seek(iter, dirname.buf, 1) < 0) {
>>  				goto cleanup;
>>  			}
>>
>
> This is quite unreadable, as you have no idea what `1` could mean. Let's
> make this a `unsigned flags` variable instead so that we can provide
> meaningful names.
>

Yeah, that would make a lot more sense. Will amend.

>> diff --git a/refs.h b/refs.h
>> index c05be6d0ac..c5e08db0ff 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -1300,20 +1300,25 @@ struct ref_iterator *refs_ref_iterator_begin(
>>  int ref_iterator_advance(struct ref_iterator *ref_iterator);
>>
>>  /*
>> - * Seek the iterator to the first reference with the given prefix.
>> - * The prefix is matched as a literal string, without regard for path
>> + * 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 prefix 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.
>> + * When set_prefix is true, this function behaves as if a new ref iterator
>> + * with the same prefix had been created, setting the prefix for subsequent
>> + * iteration. When set_prefix is false, the iterator simply seeks to the
>> + * specified reference without changing the existing prefix, allowing
>> + * iteration to start from that specific reference.
>
> I think we should detangle this paragraph a bit.
>
>     This function is expected to behave as if a new ref iterator has
>     been created, but allows reuse of it

Sure, let me add this in. Thanks!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]