Re: [PATCH v2 14/16] refs/iterator: implement seeking for `packed-ref` iterators

[Patrick Steinhardt <ps@pks.im>]

On Mon, Feb 24, 2025 at 11:09:32PM +0800, shejialuo wrote:
> On Wed, Feb 19, 2025 at 02:23:41PM +0100, Patrick Steinhardt wrote:
> > Implement seeking of `packed-ref` iterators. The implementation is again
> > straight forward, except that we cannot continue to use the prefix
> > iterator as we would otherwise not be able to reseek the iterator
> > anymore in case one first asks for an empty and then for a non-empty
> > prefix. Instead, we open-code the logic to in `advance()`.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  refs/packed-backend.c | 62 +++++++++++++++++++++++++++++++++------------------
> >  1 file changed, 40 insertions(+), 22 deletions(-)
> > 
> > diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> > index 38a1956d1a8..71a38acfedc 100644
> > --- a/refs/packed-backend.c
> > +++ b/refs/packed-backend.c
> > @@ -951,12 +954,41 @@ static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator)
> >  					    &iter->oid, iter->flags))
> >  			continue;
> >  
> > +		while (prefix && *prefix) {
> > +			if (*refname < *prefix)
> > +				BUG("packed-refs backend yielded reference preceding its prefix");
> > +			else if (*refname > *prefix)
> > +				return ITER_DONE;
> > +			prefix++;
> > +			refname++;
> > +		}
> 
> Although I cannot understand the code, I want to ask a question here, we
> we need to do this in `advance`? Should we check this for
> `packed_ref_iterator_seek` or in the `next_record` function?
> 
> Before we introduce `seek`, we don't need this logic. I somehow think we
> should do this in `packed_ref_iterator_seek`.

We cannot do this in `packed_ref_iterator_seek()` because we need to do
it for every single record that we yield from the iterator. We _could_
do it in `next_record()`, but that function is rather complex already
and really only cares about yielding the next record. On the other hand,
`advance()` already knows to skip certain entries, so putting the logic
in there to also handle termination feels like a natural fit to me.

> > @@ -1130,19 +1155,12 @@ static struct ref_iterator *packed_ref_iterator_begin(
> >  
> >  	iter->snapshot = snapshot;
> >  	acquire_snapshot(snapshot);
> > -
> > -	iter->pos = start;
> > -	iter->eof = snapshot->eof;
> >  	strbuf_init(&iter->refname_buf, 0);
> > -
> >  	iter->base.oid = &iter->oid;
> > -
> >  	iter->repo = ref_store->repo;
> >  	iter->flags = flags;
> >  
> > -	if (prefix && *prefix)
> > -		/* Stop iteration after we've gone *past* prefix: */
> > -		ref_iterator = prefix_ref_iterator_begin(ref_iterator, prefix, 0);
> > +	packed_ref_iterator_seek(&iter->base, prefix);
> 
> Why don't we check the return value here? Actually, in the previous
> patch, `cache_ref_iterator_seek` will always return 0, but you still
> check. I have thought that you just want to be more defensive.

Yeah, let's have the check over here, as well.

Patrick