Re: [PATCH 3/4] refs: selectively set prefix in the seek functions
From: Patrick Steinhardt <hidden>
Date: 2025-07-03 05:55:50
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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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