Re: [PATCH 3/4] refs: selectively set prefix in the seek functions
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Jul 3, 2025, 05:55 UTC
- Message-ID
- <aGYbUpC9OEd90J4N@pks.im>
- In-Reply-To
- <20250701-306-git-for-each-ref-pagination-v1-3-4f0ae7c0688f@gmail.com>
On Tue, Jul 01, 2025 at 05:03:29PM +0200, Karthik Nayak wrote:
Show 22 quoted lines
> 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.
Show 13 quoted lines
> 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.
Show 24 quoted lines
> 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