Re: [PATCH 07/14] refs/iterator: separate lifecycle from iteration

From: Karthik Nayak <karthik.188@gmail.com>
Date: Feb 18, 2025, 17:13 UTC
Message-ID: <CAOLa=ZQzEwWci350dhhTFiG3WZB67JUB4O9L3coiXeexYGTNZw@mail.gmail.com>
In-Reply-To: <20250217-pks-update-ref-optimization-v1-7-a2b6d87a24af@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 58 quoted lines

> The ref and reflog iterators have their lifecycle attached to iteration:
> once the iterator reaches its end, it is automatically released and the
> caller doesn't have to care about that anymore. When the iterator should
> be released before it has been exhausted, callers must explicitly abort
> the iterator via `ref_iterator_abort()`.
>
> This lifecycle is somewhat unusual in the Git codebase and creates two
> problems:
>
>   - Callsites need to be very careful about when exactly they call
>     `ref_iterator_abort()`, as calling the function is only valid when
>     the iterator itself still is. This leads to somewhat awkward calling
>     patterns in some situations.
>
>   - It is impossible to reuse iterators and re-seek them to a different
>     prefix. This feature isn't supported by any iterator implementation
>     except for the reftable iterators anyway, but if it was implemented
>     it would allow us to optimize cases where we need to search for
>     specific references repeatedly by reusing internal state.
>
> Detangle the lifecycle from iteration so that we don't deallocate the
> iterator anymore once it is exhausted. Instead, callers are now expected
> to always call a newly introduce `ref_iterator_free()` function that
> deallocates the iterator and its internal state.
>
> While at it, drop the return value of `ref_iterator_abort()`, which
> wasn't really required by any of the iterator implementations anyway.
> Furthermore, stop calling `base_ref_iterator_free()` in any of the
> backends, but instead call it in `ref_iterator_free()`.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/clone.c              |  2 +
>  dir-iterator.c               | 24 +++++------
>  dir-iterator.h               | 13 ++----
>  refs.c                       |  7 +++-
>  refs/debug.c                 |  9 ++---
>  refs/files-backend.c         | 36 +++++------------
>  refs/iterator.c              | 95 ++++++++++++++------------------------------
>  refs/packed-backend.c        | 27 ++++++-------
>  refs/ref-cache.c             |  9 ++---
>  refs/refs-internal.h         | 31 +++++----------
>  refs/reftable-backend.c      | 34 ++++------------
>  t/helper/test-dir-iterator.c |  1 +
>  12 files changed, 99 insertions(+), 189 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index fd001d800c6..ac3e84b2b18 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -426,6 +426,8 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
>  		strbuf_setlen(src, src_len);
>  		die(_("failed to iterate over '%s'"), src->buf);
>  	}
> +
> +	dir_iterator_free(iter);
>  }
>

A bit puzzled to see `dir_iterator_*` change here, I'm assuming it's linked to the 'files-backend' and perhaps similar to the changes mentioned about `ref_iterator_*` in the commit message. Would be nice to call out in the commit message too.

[snip]

Show 25 quoted lines

> @@ -223,7 +223,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
>  		} else {
>  			if (level->entries_idx >= level->entries.nr) {
>  				if (pop_level(iter) == 0)
> -					return dir_iterator_abort(dir_iterator);
> +					return ITER_DONE;
>  				continue;
>  			}
>
> @@ -232,22 +232,21 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
>
>  		if (prepare_next_entry_data(iter, name)) {
>  			if (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)
> -				goto error_out;
> +				return ITER_ERROR;
>  			continue;
>  		}
>
>  		return ITER_OK;
>  	}
> -
> -error_out:
> -	dir_iterator_abort(dir_iterator);
> -	return ITER_ERROR;
>  }

Okay yeah, we're getting rid of `dir_iterator_abort` so potentially add `dir_iterator_free` below

Show 20 quoted lines

>
> -int dir_iterator_abort(struct dir_iterator *dir_iterator)
> +void dir_iterator_free(struct dir_iterator *dir_iterator)
>  {
>  	struct dir_iterator_int *iter = (struct dir_iterator_int *)dir_iterator;
>
> +	if (!iter)
> +		return;
> +
>  	for (; iter->levels_nr; iter->levels_nr--) {
>  		struct dir_iterator_level *level =
>  			&iter->levels[iter->levels_nr - 1];
> @@ -266,7 +265,6 @@ int dir_iterator_abort(struct dir_iterator *dir_iterator)
>  	free(iter->levels);
>  	strbuf_release(&iter->base.path);
>  	free(iter);
> -	return ITER_DONE;
>  }
>
>  struct dir_iterator *dir_iterator_begin(const char *path, unsigned int flags)

Okay this makes sense!

[snip]

Show 13 quoted lines

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 29f08dced40..9511b6f3448 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -919,10 +919,6 @@ static int files_ref_iterator_advance(struct ref_iterator *ref_iterator)
>  		return ITER_OK;
>  	}
>
> -	iter->iter0 = NULL;
> -	if (ref_iterator_abort(ref_iterator) != ITER_DONE)
> -		ok = ITER_ERROR;
> -
>

Since we're explicitly going to call `ref_iterator_free`, this makes sense.

Show 22 quoted lines

>  	return ok;
>  }
>
> @@ -935,23 +931,17 @@ static int files_ref_iterator_peel(struct ref_iterator *ref_iterator,
>  	return ref_iterator_peel(iter->iter0, peeled);
>  }
>
> -static int files_ref_iterator_abort(struct ref_iterator *ref_iterator)
> +static void files_ref_iterator_release(struct ref_iterator *ref_iterator)
>  {
>  	struct files_ref_iterator *iter =
>  		(struct files_ref_iterator *)ref_iterator;
> -	int ok = ITER_DONE;
> -
> -	if (iter->iter0)
> -		ok = ref_iterator_abort(iter->iter0);
> -
> -	base_ref_iterator_free(ref_iterator);
> -	return ok;
> +	ref_iterator_free(iter->iter0);
>  }
>

I like how much more cleaner it looks now.

[snip]

Show 14 quoted lines

> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index a7b6f74b6e3..38a1956d1a8 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -954,9 +954,6 @@ static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator)
>  		return ITER_OK;
>  	}
>
> -	if (ref_iterator_abort(ref_iterator) != ITER_DONE)
> -		ok = ITER_ERROR;
> -
>  	return ok;
>  }
>

The merged_iterator is used to combine the files and packed backend iterators to provide a uniform view over them. Likewise the changes here seem similar too.

Show 33 quoted lines

> @@ -976,23 +973,19 @@ static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator,
>  	}
>  }
>
> -static int packed_ref_iterator_abort(struct ref_iterator *ref_iterator)
> +static void packed_ref_iterator_release(struct ref_iterator *ref_iterator)
>  {
>  	struct packed_ref_iterator *iter =
>  		(struct packed_ref_iterator *)ref_iterator;
> -	int ok = ITER_DONE;
> -
>  	strbuf_release(&iter->refname_buf);
>  	free(iter->jump);
>  	release_snapshot(iter->snapshot);
> -	base_ref_iterator_free(ref_iterator);
> -	return ok;
>  }
>
>  static struct ref_iterator_vtable packed_ref_iterator_vtable = {
>  	.advance = packed_ref_iterator_advance,
>  	.peel = packed_ref_iterator_peel,
> -	.abort = packed_ref_iterator_abort
> +	.release = packed_ref_iterator_release,
>  };
>
>  static int jump_list_entry_cmp(const void *va, const void *vb)
> @@ -1362,8 +1355,10 @@ static int write_with_updates(struct packed_ref_store *refs,
>  	 */
>  	iter = packed_ref_iterator_begin(&refs->base, "", NULL,
>  					 DO_FOR_EACH_INCLUDE_BROKEN);
> -	if ((ok = ref_iterator_advance(iter)) != ITER_OK)
> +	if ((ok = ref_iterator_advance(iter)) != ITER_OK) {
> +		ref_iterator_free(iter);

Nit: Since we don't return early here, wouldn't the `ref_iterator_free`
at the end of the function be sufficient? I think the only early return
when `iter == NULL` is towards the end of the function, where it might
be better to add a `goto error`.

[snip]