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

From: Taylor Blau <me@ttaylorr.com>
Date: Aug 26, 2025, 01:58 UTC
Message-ID: <aK0UrslvCQsglMiA@nand.local>
In-Reply-To: <20250821-b4-pks-packfiles-store-v2-8-d10623355e9f@pks.im>

On Thu, Aug 21, 2025 at 09:39:06AM +0200, Patrick Steinhardt wrote:

Show 22 quoted lines

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

Show 12 quoted lines

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

Show 13 quoted lines

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