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@xxxxxx>
> > > ---
> > > 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.
