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

[Patrick Steinhardt <ps@pks.im>]

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.

> 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