Re: [RFC PATCH] multi-pack-index: allow operating without pack files

From: Derrick Stolee <stolee@gmail.com>
Date: Aug 23, 2021, 00:34 UTC
Message-ID: <edb9c412-70c8-4fc6-04ab-417eca05ee15@gmail.com>
In-Reply-To: <20210820195558.44275-1-johannes@sipsolutions.net>

On 8/20/2021 3:55 PM, Johannes Berg wrote:

> Technically, multi-pack-index doesn't need pack files to exist,
> but add_packed_git() today checks whether it exists or not.

Having a multi-pack-index is supposed to indicate that we have these objects in the objects/pack directory within the specified pack-files.

I understand your goal to relax a condition of the multi-pack-index file, but it's triggered by a flag during write and that choice isn't persisted into the file. There is no way for a later Git process to understand that the multi-pack-index doesn't actually guarantee object existence.

And in a completely other side: one would think that including a pack-file in the multi-pack-index would allow deleting the .idx file, but there are a few reasons why we do not (including interactions with third-party tools).

So, I'm not necessarily on board with this change unless something is added to the multi-pack-index file (such as a new version 2 and an optional chunk understood by that version) that tells future Git processes that the .pack files might not exist. I'm still not sure what Git should do about that other than stop reading the multi-pack-index and ignore its contents.

> -struct packed_git *add_packed_git(const char *path, size_t path_len, int local)
> +struct packed_git *_add_packed_git(const char *path, size_t path_len, int local,
> +				   int require_pack)

The only obvious thing that I noticed in the code is that we typically use <function name>_1() as a way to create a static version that is called by the global version.

>  struct packed_git *add_packed_git(const char *path, size_t path_len, int local);
> +struct packed_git *_add_packed_git(const char *path, size_t path_len, int local,
> +				   int require_pack);

...oh, but that's not what you're doing. What you could do instead is convert the 'local' parameter into a 'flags' parameter (I think we have started to prefer 'enum's recently) and create MIDX_FLAG_LOCAL and MIDX_FLAG_PACKS_OPTIONAL flag values. That avoids multiple methods and minimizes the change to existing callers.

Thanks, -Stolee