Re: [PATCH 2/8] packfile: refactor `prepare_packed_git_one()` to work on sources

From: Taylor Blau <me@ttaylorr.com>
Date: Jul 10, 2025, 23:14 UTC
Message-ID: <aHBJVZsFRUUM+vXu@nand.local>
In-Reply-To: <20250709-b4-pks-midx-via-odb-alternate-v1-2-f31150d21331@pks.im>

On Wed, Jul 09, 2025 at 09:54:50AM +0200, Patrick Steinhardt wrote:

> In the preceding commit we have refactored how we load multi-pack
> indices so that we take take the source as input for which we want to
> load the MIDX. As part of this refactoring we started to store a pointer
> to the MIDX in `struct odb_source` itself.

The first sentence here is a little confusing, but may read more clearly if written as:

    In the preceding commit we refactored how we load multi-pack
    indices to take a corresponding "source" as input.

> Refactor loading of packfiles in the same way: instead of passing in the
> object directory, we now pass in the source for which we want to load

s/for/from

Show 34 quoted lines

> packfiles. This allows us to simplify the code because we don't have to
> search for a corresponding MIDX anymore, but we can instead directly use
> the MIDX that we have already prepared beforehand.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  packfile.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/packfile.c b/packfile.c
> index 16efc2fdca3..b43dd2fe6cb 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -935,22 +935,17 @@ static void prepare_pack(const char *full_name, size_t full_name_len,
>  		report_garbage(PACKDIR_FILE_GARBAGE, full_name);
>  }
>
> -static void prepare_packed_git_one(struct repository *r, char *objdir, int local)
> +static void prepare_packed_git_one(struct odb_source *source, int local)
>  {
> -	struct prepare_pack_data data;
>  	struct string_list garbage = STRING_LIST_INIT_DUP;
> +	struct prepare_pack_data data = {
> +		.m = source->multi_pack_index,
> +		.r = source->odb->repo,
> +		.garbage = &garbage,
> +		.local = local,
> +	};
>
> -	data.m = r->objects->multi_pack_index;
> -
> -	/* look for the multi-pack-index for this object directory */
> -	while (data.m && strcmp(data.m->object_dir, objdir))
> -		data.m = data.m->next;

Right, since we know that the MIDX corresponding to this source belongs to the same "object_dir" path. Having an ASSERT() here may make that more clear, but this change looks correct to me.

I am still a little unclear on how sources and ODBs correspond to one another, but under my working assumption from the previous patch, I think this is right.

Thanks, Taylor