On Wed, Feb 19, 2025 at 02:23:38PM +0100, Patrick Steinhardt wrote:
> 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@xxxxxx>
> ---
> 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]
> +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?
> +
> + 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.
> }
>
> 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
