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

From: shejialuo <shejialuo@gmail.com>
Date: Feb 24, 2025, 13:37 UTC
Message-ID: <Z7x2IEdRP4fzdXMo@ArchLinux>
In-Reply-To: <20250219-pks-update-ref-optimization-v2-11-e696e7220b22@pks.im>

On Wed, Feb 19, 2025 at 02:23:38PM +0100, Patrick Steinhardt wrote:

Show 20 quoted lines

> Implement seeking on merged iterators. The implementation is rather
> straight forward, with the only exception that we must not deallocate
> the underlying iterators once they have been exhausted.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs/iterator.c | 38 +++++++++++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 9 deletions(-)
> 
> 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)
>  struct merge_ref_iterator {
>  	struct ref_iterator base;
>  
> -	struct ref_iterator *iter0, *iter1;
> +	struct ref_iterator *iter0, *iter0_owned;
> +	struct ref_iterator *iter1, *iter1_owned;

We would always free `iter0_owned` and `iter1_owned`. That's the reason why in the below code, we could drop `ref_iterator_free`. Make sense.

[snip]

Show 18 quoted lines

> +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?

Show 15 quoted lines

> +
> +	return 0;
> +}
> +
>  static int merge_ref_iterator_peel(struct ref_iterator *ref_iterator,
>  				   struct object_id *peeled)
>  {
> @@ -242,12 +261,13 @@ static void merge_ref_iterator_release(struct ref_iterator *ref_iterator)
>  {
>  	struct merge_ref_iterator *iter =
>  		(struct merge_ref_iterator *)ref_iterator;
> -	ref_iterator_free(iter->iter0);
> -	ref_iterator_free(iter->iter1);
> +	ref_iterator_free(iter->iter0_owned);
> +	ref_iterator_free(iter->iter1_owned);

We free the internal pointer but not the pointer exposed to the caller. Make sense.

Show 16 quoted lines

>  }
>  
>  static struct ref_iterator_vtable merge_ref_iterator_vtable = {
>  	.advance = merge_ref_iterator_advance,
> +	.seek = merge_ref_iterator_seek,
>  	.peel = merge_ref_iterator_peel,
>  	.release = merge_ref_iterator_release,
>  };
> @@ -268,8 +288,8 @@ struct ref_iterator *merge_ref_iterator_begin(
>  	 */
>  
>  	base_ref_iterator_init(ref_iterator, &merge_ref_iterator_vtable);
> -	iter->iter0 = iter0;
> -	iter->iter1 = iter1;
> +	iter->iter0 = iter->iter0_owned = iter0;
> +	iter->iter1 = iter->iter1_owned = iter1;

OK, we would assign `iter0` to `iter0_owned` and `iter1` to `iter1_owned`.

Thanks, Jialuo