Re: [PATCH v2 11/16] refs/iterator: implement seeking for merged iterators

[Patrick Steinhardt <ps@pks.im>]

On Mon, Feb 24, 2025 at 09:37:36PM +0800, shejialuo wrote:
> On Wed, Feb 19, 2025 at 02:23:38PM +0100, Patrick Steinhardt wrote:
> > diff --git a/refs/iterator.c b/refs/iterator.c
> > index 757b105261a..63608ef9907 100644
> > --- a/refs/iterator.c
> > +++ b/refs/iterator.c
> > @@ -96,7 +96,8 @@ int is_empty_ref_iterator(struct ref_iterator *ref_iterator)
> > +static int merge_ref_iterator_seek(struct ref_iterator *ref_iterator,
> > +				   const char *prefix)
> > +{
> > +	struct merge_ref_iterator *iter =
> > +		(struct merge_ref_iterator *)ref_iterator;
> > +	int ret;
> > +
> > +	iter->current = NULL;
> > +	iter->iter0 = iter->iter0_owned;
> > +	iter->iter1 = iter->iter1_owned;
> > +
> > +	ret = ref_iterator_seek(iter->iter0, prefix);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = ref_iterator_seek(iter->iter1, prefix);
> > +	if (ret < 0)
> > +		return ret;
> 
> We could simply use a single `if` statement to handle this. Is the
> reason why we design this is that we want to return the exact error code
> for each case?

Yup, I don't want to loose the error code. We could write this as:

    if ((ret = ref_iterator_seek(iter->iter0, prefix)) < 0 ||
        (ret = ref_iterator_seek(iter->iter0, prefix)) < 0)
            return ret;

But assigning to variables in conditions is not something we typically
do in the Git codebase.

Patrick