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

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 5, 2023, 07:08 UTC
Message-ID: <ZC0eY8q6ushpfkrQ@ncase>
In-Reply-To: <ZCxytq1esQWvjIz/@nand.local>

On Tue, Apr 04, 2023 at 02:55:50PM -0400, Taylor Blau wrote:

Show 57 quoted lines

> 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.
> 
> For example, if you do something like:
> 
>     rm -fr shared member
> 
>     git init shared
>     git -C shared commit --allow-empty -m "base" && git -C shared repack -d
> 
>     git clone --shared shared member
>     git -C member repack -ad
> 
>     for dir in shared member
>     do
>       echo "==> $dir"
>       find $dir/.git/objects -type f
>     done
> 
> Then you'll end up with the output:
> 
>     ==> shared
>     shared/.git/objects/pack/pack-a2f0c663b287c3eeb0207397f8cafb9ae49f8277.idx
>     shared/.git/objects/pack/pack-a2f0c663b287c3eeb0207397f8cafb9ae49f8277.pack
>     shared/.git/objects/info/packs
>     ==> member
>     member/.git/objects/pack/pack-a2f0c663b287c3eeb0207397f8cafb9ae49f8277.idx
>     member/.git/objects/pack/pack-a2f0c663b287c3eeb0207397f8cafb9ae49f8277.pack
>     member/.git/objects/info/alternates
>     member/.git/objects/info/packs
> 
> In other words, we end up creating the pack necessary in the member
> repository necessary to complete the repack. Since we're using `-a`
> here, that means creating an identical pack as we have in the shared
> repository.
> 
> If we instead did something like:
> 
>     git -C member repack -adl # <- `-l` is new here
> 
> , our output changes to instead contain the following (empty) pack
> directory in the member repository:
> 
>     ==> member
>     member/.git/objects/info/alternates
>     member/.git/objects/info/packs

Yup, that's fair.

Show 15 quoted lines

> > Skip over packfiles that aren't local. This will cause geometric repacks
> > to never include packfiles of its alternates.
> 
> ...So I wonder if this is the right thing to do in all cases. IOW,
> should we be creating packs in the "member" repository which may be
> based off of packs from the shared repository when `-l` is not
> specified?
> 
> I think this gets tricky. For one, the geometric repack code creates at
> most one new pack. So if some of the packs that were above the split
> line (IOW, the ones that don't need to be squashed together) were in the
> shared repository, I'm not sure how you'd write a MIDX that covers both
> without using the MIDX-over-alternates feature. I have no idea how that
> works with MIDX bitmaps (IIUC, the MIDX/alternates feature is very
> niche).

I agree, things would become tricky in case geometric repacks are expected to work across alternates.

Show 7 quoted lines

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

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:

Show 6 quoted lines

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

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

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?

Thanks for your feedback and this interesting discussion!

Patrick