Re: [PATCH] refs/files: keep root refs out of loose ref cache

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 15, 2026, 05:58 UTC
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:

Show 15 quoted lines

> 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.

Show 28 quoted lines

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