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

[shejialuo <shejialuo@gmail.com>]

On Tue, Feb 25, 2025 at 08:39:54AM +0100, Patrick Steinhardt wrote:
> 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.
> 

Thanks for the detailed explanation.