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