Re: [PATCH] repack: fix geometric repacking with gitalternates

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2893 bytes --]

On Mon, Apr 10, 2023 at 07:49:01PM -0400, Taylor Blau wrote:
> On Mon, Apr 10, 2023 at 11:06:49AM -0400, Derrick Stolee wrote:
[snip]
> > Perhaps this method could include a step to create a new, "local"
> > geometry containing only the packs within the local object dir. We can
> > then skip the --preferred-pack option and bitmap if this is different
> > than the original geometry (perhaps with a warning message to help
> > users who did this accidentally).
> 
> It would be nice to not have to juggle multiple pack geometry structs,
> since the logic of what gets repacked, what gets thrown away, and what
> gets kept is already fairly complicated (at least to me) and pretty
> fragile.
> 
> I think if I were sketching this out, I'd start out by doing something
> like:
> 
> --- 8< ---
> diff --git a/builtin/repack.c b/builtin/repack.c
> index df4d8e0f0b..eab5f58444 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -558,6 +558,10 @@ static void midx_included_packs(struct string_list *include,
>  		for (i = geometry->split; i < geometry->pack_nr; i++) {
>  			struct packed_git *p = geometry->pack[i];
> 
> +			/* MIDXs cannot refer to non-local packs */
> +			if (!p->pack_local)
> +				continue;
> +
>  			strbuf_addstr(&buf, pack_basename(p));
>  			strbuf_strip_suffix(&buf, ".pack");
>  			strbuf_addstr(&buf, ".idx");
> --- >8 ---
> 
> ...and I actually think that might do it, since:
> 
> 	- existing_nonkept_packs is populated by calling readdir() on the local
> 		repository's $GIT_DIR/objects/pack directory, so it will never contain
> 		any non-local packs.
> 
> 	- existing_kept_packs is also OK for the same reason
> 
> 	- names (which tracks the packs that we just wrote) will never contain a
> 		non-local pack, since we never write packs outside of our local pack
> 		directory
> 
> So that would cause you to write a MIDX containing only local packs (as
> desired) regardless of whether or not the caller passed --[no]-local or
> not.

I think this doesn't actually do anything. The reason is that the
`--stdin-packs` option in git-multi-pack-index(1) does not actually
treat the provided packs as packs that are to be included in the
multi-pack-index. Instead, it uses it as a filter for `get_all_packs()`.
And as `get_all_packs()` returns packfiles paths prefixed with the
alternate directory's name, but we pass in basenames only, we would
already be filtering out any non-local packfiles.

So ultimately, we would only ever write an MIDX containing only local
packs already. It rather feels like this is only by chance though, so I
think it is good to include your patch regardless of whether it actually
does something or not. Better be explicit here, also as documentation to
the reader.

I'll include it as part of my patch series that I'm to send out later
this week.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]