From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 15 Oct 2026 05:58:01 GMT
Subject: Re: [PATCH] refs/files: keep root refs out of loose ref cache
Message-ID: <atBraYoY1SVwnezN@pks.im>
In-Reply-To: <20261014153208.2194727-1-andy.koppe@gmail.com>

On Wed, Oct 14, 2026 at 04:32:08PM +0100, Andy Koppe wrote:
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 71628550f2..74409878b6 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -481,8 +481,21 @@ static void add_root_refs(struct files_ref_store *refs,
>  	for_each_root_ref(refs, fill_root_ref, &data);
>  }
>  
> -static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs,
> -					     unsigned int flags)
> +/*
> + * Create a separate ref cache containing only the root refs. These are kept
> + * out of the loose ref cache, which may already exist and is shared by
> + * iterations that should not include them.
> + */

This raises the question whether an alternative fix could have been to
teach the cache itself to skip over root refs in case the caller is not
interested in them. The answer may very well be a "no", but I would be
curious to learn why you settled on the proposed fix instead.

> +static struct ref_cache *create_root_ref_cache(struct files_ref_store *refs)
> +{
> +	struct ref_cache *cache = create_ref_cache(&refs->base, NULL);
> +
> +	cache->root->flag &= ~REF_INCOMPLETE;
> +	add_root_refs(refs, get_ref_dir(cache->root));
> +	return cache;
> +}
> +
> +static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
>  {
>  	if (!refs->loose) {
>  		struct ref_dir *dir;
> @@ -1094,9 +1107,17 @@ static struct ref_iterator *files_ref_iterator_begin(
>  	 * disk, and re-reads it if not.
>  	 */
>  
> -	loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, flags),
> +	loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs),
>  					      prefix, ref_store->repo, 1);
>  
> +	if (flags & REFS_FOR_EACH_INCLUDE_ROOT_REFS) {
> +		root_refs = create_root_ref_cache(refs);
> +		loose_iter = overlay_ref_iterator_begin(
> +				cache_ref_iterator_begin(root_refs, prefix,
> +							 ref_store->repo, 0),
> +				loose_iter);
> +	}

It's a bit weird that we recreate the root ref "cache" for every single
ref iterator we create that asks for root refs. Is there a reason why
you have decided to not actually cache the root refs the same way as we
cache the loose refs?

Thanks!

Patrick