Re: [PATCH v2 08/16] packfile: refactor `prepare_packed_git()` to work on packfile store

[Taylor Blau <me@ttaylorr.com>]

On Thu, Aug 21, 2025 at 09:39:06AM +0200, Patrick Steinhardt wrote:
> The `prepare_packed_git()` function and its friends are responsible for
> loading packfiles as well as the multi-pack index for a given object
> database. Refactor these functions to accept a packfile store instead of
> a repository to clarify their scope.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  packfile.c | 43 +++++++++++++++++++------------------------
>  1 file changed, 19 insertions(+), 24 deletions(-)
>
> diff --git a/packfile.c b/packfile.c
> index 90f15b0c20..2e45a3a05f 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -974,38 +974,33 @@ static int sort_pack(const struct packed_git *a, const struct packed_git *b)
>  	return -1;
>  }
>
> -static void rearrange_packed_git(struct repository *r)
> -{
> -	sort_packs(&r->objects->packfiles->packs, sort_pack);
> -}

OK, makes sense -- it looks like you inlined rearrange_packed_git() in
its sole caller packfile_store_prepare() below. I think that could have
been done as a separate step, but it's equally fine to include it here,
too.

> -static void prepare_packed_git_mru(struct repository *r)
> +static void packfile_store_prepare_mru(struct packfile_store *store)
>  {
>  	struct packed_git *p;
>
> -	INIT_LIST_HEAD(&r->objects->packfiles->mru);
> +	INIT_LIST_HEAD(&store->mru);
>
> -	for (p = r->objects->packfiles->packs; p; p = p->next)
> -		list_add_tail(&p->mru, &r->objects->packfiles->mru);
> +	for (p = store->packs; p; p = p->next)
> +		list_add_tail(&p->mru, &store->mru);

Looks all good.

> -static void prepare_packed_git(struct repository *r)
> +static void packfile_store_prepare(struct packfile_store *store)
>  {
>  	struct odb_source *source;
>
> -	if (r->objects->packfiles->initialized)
> +	if (store->initialized)
>  		return;
>
> -	odb_prepare_alternates(r->objects);
> -	for (source = r->objects->sources; source; source = source->next) {
> -		int local = (source == r->objects->sources);
> +	odb_prepare_alternates(store->odb);

Hmmm. I admit that I don't love that the packfile_store knows about the
object_store that it belongs to. This feels like a layering violation
and makes me worry that we pushed too much down into the new
packfile_store. I'm not sure I have a better idea off the top of my
head, though.

Everything down from here looks good to me.

Thanks,
Taylor