Re: [PATCH v2 6/9] midx: load multi-pack indices via their source
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Aug 7, 2025, 22:25 UTC
- Message-ID
- <aJUn0qeliNQ/nnWr@nand.local>
- In-Reply-To
- <20250807-b4-pks-midx-deduplicate-source-info-v2-6-bcffb8fc119c@pks.im>
On Thu, Aug 07, 2025 at 10:09:56AM +0200, Patrick Steinhardt wrote:
Show 56 quoted lines
> To load a multi-pack index the caller is expected to pass both the
> repository and the object directory where the multi-pack index is
> located. While this works, this layout has a couple of downsides:
>
> - We need to pass in information reduntant with the owning source,
> namely its object directory and whether the source is local or not.
>
> - We don't have access to the source when loading the multi-pack
> index. If we had that access, we could store a pointer to the owning
> source in the MIDX and thus deduplicate some information.
>
> - Multi-pack indices are inherently specific to the object source and
> its format. With the goal of pluggable object backends in mind we
> will eventually want the backends to own the logic of reading and
> writing multi-pack indices. Making the logic work on top of object
> sources is a step into that direction.
>
> Refactor loading of multi-pack indices accordingly.
>
> This surfaces one small problem though: git-multi-pack-index(1) and our
> MIDX test helper both know to read and write multi-pack-indices located
> in a different object directory. This issue is addressed by adding the
> user-provided object directory as an in-memory alternate.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> builtin/multi-pack-index.c | 18 ++++++++++++--
> midx.c | 57 ++++++++++++++++++++-------------------------
> midx.h | 6 ++---
> t/helper/test-read-midx.c | 25 ++++++++++++--------
> t/t5319-multi-pack-index.sh | 8 +++----
> 5 files changed, 62 insertions(+), 52 deletions(-)
>
> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
> index aa25b06f9d..e4a9305af3 100644
> --- a/builtin/multi-pack-index.c
> +++ b/builtin/multi-pack-index.c
> @@ -64,12 +64,20 @@ static int parse_object_dir(const struct option *opt, const char *arg,
> char **value = opt->value;
> free(*value);
> if (unset)
> - *value = xstrdup(repo_get_object_directory(the_repository));
> + *value = xstrdup(the_repository->objects->sources->path);
> else
> *value = real_pathdup(arg, 1);
> return 0;
> }
>
> +static struct odb_source *handle_object_dir_option(struct repository *repo)
> +{
> + struct odb_source *source = odb_find_source(repo->objects, opts.object_dir);
> + if (!source)
> + source = odb_add_to_alternates_memory(repo->objects, opts.object_dir);
> + return source;
> +}
> +Hmm... I admit that I am pretty unfamiliar with the --object-dir option here and only have a vague recollection of why it was added in the first place.
I am not sure one way or another if adding the user-provided directory as an in-memory alternate is the right thing to do here.
Stolee (CC'd) would you mind sharing your thoughts on this?
Thanks, Taylor