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

[Derrick Stolee <derrickstolee@github.com>]

On 4/5/2023 3:08 AM, Patrick Steinhardt wrote:
> On Tue, Apr 04, 2023 at 02:55:50PM -0400, Taylor Blau wrote:
>> On Tue, Apr 04, 2023 at 01:08:33PM +0200, Patrick Steinhardt wrote:
>>> Both issues have the same underlying root cause, which is that geometric
>>> repacks don't honor whether packfiles are local or not. As a result,
>>> they will try to include packs part of the alternate object directory
>>> and then at a later point fail to locate them as they do not exist in
>>> the object directory of the repository we're about to repack.
>>
>> Interesting. This fix does make sense, but I wonder if it is doing the
>> right thing.
>>
>> When we have an alternated repository and do 'git repack -ad' in the
>> "member" repository, we'll end up creating a new pack containing all
>> objects in that repository, including ones from the alternate.
...
>> I think we reasonably could do something like ignoring non-local packs
>> in `init_pack_geometry()` only when `-l` is given. That still runs into
>> problems when trying to write a MIDX or MIDX bitmaps, so we should
>> likely declare the combination "-l --write-midx --write-bitmap-index" as
>> unsupported. For backwards compatibility, I think it would make sense to
>> have "--no-local" be the default when `--geometric` is given (which is
>> already the case, since po_args is zero-initialized).
> 
> Okay, I agree that it's not all that sensible to allow writing bitmaps
> in a geometric repack that spans across multiple repositories. These
> bitmaps would immediately break once the shared repository performs a
> repack that removes a subset of packfiles that the bitmap depends on,
> which would make it non-obvious for how to even do repacks in such a
> shared repository at all.

We have mechanisms for avoiding writing bitmaps for packs that are not
closed under reachability. We should have some protection against writing
them for multi-pack-indexes that are not closed under reachability, if
only as a check during bitmap_writer_build().

> But I'm not yet sure whether I understand why `-l --write-midx` should
> be prohibited like you summarized in the follow-up mail:
> 
> On Tue, Apr 04, 2023 at 02:55:50PM -0400, Taylor Blau wrote:
>> TL;DR: I think that this is a (much) more complicated problem than
>> originally anticipated, but the easiest thing to do is to assume that
>> git repack --geometric=<d> means git repack --geometric=<d> --no-local
>> unless otherwise specified, and declare --geometric=<d> --local
>> unsupported when used in conjunction with --write-midx or
>> --write-bitmap-index.
> 
> The newly written MIDX would of course only span over the local
> packfiles, but is that even a problem? Ideally, Git would know to handle
> multiple MIDX files, and in that case it would make sense both for the
> shared and for the member repository to have one.

Yes, each odb is allowed a multi-pack-index, and they chain the same way
pack-files do. I agree that this restriction isn't necessary.

>> I suspect in practice that nobody cares about what happens when you run
>> "git repack --geometric=<d> --local", but in case they do, I think the
>> above is probably the most reasonable behavior that I can quickly come
>> up with.
> 
> Well, I do as we use alternates to implement fork networks at GitLab and
> we're in the process of adopting geometric repacking. So what I want to
> have is that I can perform geometric repacks both in the shared and in
> the member repository that includes only the local packfiles.
> 
> And yes, I agree that the above is the most reasonable behaviour, with
> the exception of disallowing MIDXs when doing a local geometric repack.

I think the recommended

	if (po_args.local && !p->local)
		continue;

approach is a nice _performance improvement_ for the --local case, since
it avoids adding a list of objects to be packed (and then thrown away,
because those objects exist in an alternate). Of course, we are currently
blocked on that part working because of the non-local packs being a
problem.

> But that raises the question: what do we do about the currently-broken
> behaviour when executing `git repack --geometric=<d> --no-local` in a
> alternated repository?

Much like "git repack -a --no-local" in an alternated repository, I
don't think this is something good for users to do, but I agree that it
not working is a problem we should fix.

Your original message includes error messages like "could not find pack"
and "unknown preferred pack" which makes me think the _real_ problem is
that we are not respecting the full path name of the pack-file and are
somehow localizing packs to the local object dir.

The basic reason is that write_midx_included_packs() takes all of the
pack-files from the geometry, but does not strip out the pack-files
that are not in the same object directory.

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

> I'd personally be fine to start honoring the `po_args.local` flag so
> that we skip over any non-local packfiles there while ignoring the
> larger problem of non-local geometric repacks breaking in certain
> scenarios. It would at least improve the status quo as users now have a
> way out in case they ever happen to hit that error. And it allows us to
> use geometric repacks in alternated repositories. But are we okay with
> punting on the larger issue for the time being?

I think the real bug is isolated in write_midx_included_packs() in how
it may specify packs that the multi-pack-index cannot track. It should
be worth the time exploring if there is an easy fix there, and then the
po_args.local version can be used as a backup/performance tweak on top
of that.

Thanks,
-Stolee