From: Taylor Blau <me@ttaylorr.com>
Date: Wed, 08 Oct 2025 20:21:23 GMT
Subject: Re: [PATCH 1/6] object-name: convert to use `packfile_store_get_all_packs()`
Message-ID: <aObHw6rxrQG22b5B@nand.local>
In-Reply-To: <20251007-pks-packfiles-convert-get-all-v1-1-428227657a89@pks.im>

On Tue, Oct 07, 2025 at 02:41:07PM +0200, Patrick Steinhardt wrote:
> When searching for abbreviated or when trying to disambiguate object IDs
> we do this in two steps:
>
>   1. We search through the multi-pack index.
>
>   2. We search through all packfiles not part of any multi-pack index.
>
> The second step uses `packfile_store_get_packs()`, which knows to skip
> loading any packfiles that are indexed by an MIDX; this is exactly what
> we want.
>
> But that function is somewhat problematic, as its behaviour is stateful
> and is influenced by `packfile_store_get_all_packs()`. This function
> basically does the same as `packfile_store_get_packs()`, but in addition
> it also loads all packfiles indexed by an MIDX. The problem here is that
> both of these functions act on the same linked list of packfiles, and
> thus depending on whether or not `get_all_packs()` was called the result
> returned by `get_packs()` will be different. Consequently, all callers
> of `get_packs()` need to be prepared to see MIDX'd packs even though
> these should in theory be excluded.
>
> This interface is confusing and thus potentially dangerous, which is why
> we're converting all callers of `get_packs()` to use `get_all_packs()`
> instead.

Well explained.

> diff --git a/object-name.c b/object-name.c
> index f6902e140d..4e62bfa330 100644
> --- a/object-name.c
> +++ b/object-name.c
> @@ -213,7 +213,7 @@ static void find_short_packed_object(struct disambiguate_state *ds)
>  			unique_in_midx(m, ds);
>  	}
>
> -	for (p = packfile_store_get_packs(ds->repo->objects->packfiles); p && !ds->ambiguous;
> +	for (p = packfile_store_get_all_packs(ds->repo->objects->packfiles); p && !ds->ambiguous;
>  	     p = p->next)
>  		unique_in_pack(p, ds);
>  }

Good. As you noted, this function already handles MIDX'd packs as a
separate case, and `unique_in_pack()` discards any packs that have
their `multi_pack_index` bit set. So this caller can be converted
without issue.

> @@ -805,7 +805,7 @@ static void find_abbrev_len_packed(struct min_abbrev_data *mad)
>  			find_abbrev_len_for_midx(m, mad);
>  	}
>
> -	for (p = packfile_store_get_packs(mad->repo->objects->packfiles); p; p = p->next)
> +	for (p = packfile_store_get_all_packs(mad->repo->objects->packfiles); p; p = p->next)
>  		find_abbrev_len_for_pack(p, mad);
>  }

And the exact same is true for this case as well. Looking good.

Thanks,
Taylor