Re: [PATCH v2 10/16] refs/iterator: provide infrastructure to re-seek iterators

> On Wed, Feb 19, 2025 at 02:23:37PM +0100, Patrick Steinhardt wrote:
> > Reftable iterators need to be scrapped after they have either been
> > exhausted or aren't useful to the caller anymore, and it is explicitly
> > not possible to reuse them for iterations. But enabling for reuse of
> > iterators may allow us to tune them by reusing internal state of an
> > iterator. The reftable iterators for example can already be reused
> > internally, but we're not able to expose this to any users outside of
> > the reftable backend.
> > 
> 
> Out of curiosity, is there any benefits for reusing iterators for files
> backend?

> > Introduce a new `.seek` function in the ref iterator vtable that allows
> > callers to re-seek an iterator. It is expected to be functionally the
> 
> It's a bit strange that we use "re-seek". I think we just want to see an
> iterator. Isn't it? Don't worth a reroll.

> > @@ -368,6 +381,16 @@ static int prefix_ref_iterator_advance(struct ref_iterator *ref_iterator)
> >  	return ok;
> >  }
> >  
> > +static int prefix_ref_iterator_seek(struct ref_iterator *ref_iterator,
> > +				    const char *prefix)
> > +{
> > +	struct prefix_ref_iterator *iter =
> > +		(struct prefix_ref_iterator *)ref_iterator;
> > +	free(iter->prefix);
> 
> Here, we need to free the "iter->prefix". We don't know whether the
> caller would call `prefix_ref_iterator_seek` many times for the same ref
> iterator. So, we need to restore the state/context.
> 
> I want to ask a question here, why don't we care about "trim" parameter
> which is declared in the "prefix_ref_iterator_begin"? From my
> understanding, we may want to reuse "trim" state in the original state.
> So, when we want to reuse iterator, we only consider about the "prefix"
> but leave the other stats the same. Is my understanding correct?

> > +	iter->prefix = xstrdup_or_null(prefix);
> > +	return ref_iterator_seek(iter->iter0, prefix);
> > +}
> > +
> 
> > diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> > index 74e2c03cef1..3f6d43110b7 100644
> > --- a/refs/refs-internal.h
> > +++ b/refs/refs-internal.h
> > @@ -327,6 +327,21 @@ struct ref_iterator {
> >   */
> >  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
> > + * 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.
> 
> I somehow think we may emphasis that we want to reuse some internal
> states of the ref iterator except the prefix. However, I am not sure.
> Just think about this.

> > @@ -445,6 +460,13 @@ void base_ref_iterator_init(struct ref_iterator *iter,
> >   */
> >  typedef int ref_iterator_advance_fn(struct ref_iterator *ref_iterator);
> >  
> > +/*
> > + * Seek the iterator to the first reference matching the given prefix. Should
> 
> Maybe which should?
> 
> > + * behave the same as if a new iterator was created with the same prefix.
> > + */
> 
> This statement makes me a little confused. I think there are something
> difference between `seek` and `begin`? For prefix ref iterator, we will
> pass "trim" when calling `begin`, but for seeking, we don't care about
> "trim". Although the prefix maybe the same, but the internal state may
> be different.