Re: [PATCH v2 8/8] repack: disable writing bitmaps when doing a local geometric repack

[Patrick Steinhardt <ps@pks.im>]

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

On Thu, Apr 13, 2023 at 11:54:55AM +0200, Patrick Steinhardt wrote:
> On Wed, Apr 12, 2023 at 06:01:06PM -0400, Taylor Blau wrote:
> > On Wed, Apr 12, 2023 at 12:23:01PM +0200, Patrick Steinhardt wrote:
[snip]
> > One thing that I don't quite follow with this logic is why we need to
> > have geometric_factor set. You could (somewhat unreasonably) write a
> > MIDX containing a single pack (git repack -[A|a] --write-midx
> > --write-bitmap-index), or a MIDX containing just the new pack along with
> > all of the existing (local) packs, (git repack --write-midx
> > --write-bitmap-index).
> > 
> > So I think we'd want to drop the geometric_factor from the above
> > conditional. (And in the future, I think we typically refer to whether
> > or not the "geometry" pointer is NULL or not to indicate whether or not
> > we are doing a geometric repack, though the diff context doesn't give me
> > enough to know whether we have even attempted to set up that instance
> > yet, so this is fine, too).
> 
> Mh. Yeah, I think you're right. I'll change it.

Actually, do we even have to care about the `write_midx` case? Right now
we have:

```
if (write_bitmaps && !(pack_everything & ALL_INTO_ONE) && !write_midx)
    die(_(incremental_bitmap_conflict_error));
```

The intent is to die when not repacking all objects into a single pack
(potentially with a cruft pack), but to allow this when writing a
multi-pack-index because they can have a bitmap that spans across
multiple packs.

Now, whether or not we write a multi-pack-index, as soon as the user
passes `-l` we cannot guarantee that we have all objects available
locally either in a single packfile nor in multiple packfiles when the
repository is connected to an alternate object directory. So in the
spirit of the preexisting check, couldn't we do the following:

```
if (write_bitmaps && po_args.local && has_alternates(repo))
    die(_("Repacking local objects is incompatible with bitmap indexes.");
```

So in words, we die when the user asks us to write a bitmap index
for a repack that is supposed to only include local objects when there
are objects that could have been inherited from an alternate object
directory.

I'm not sure whether we are okay with retroactively tightening checks
though. I'd argue it's likely fine, because it wouldn't have worked
before this check either. And I'd rather fail with explicit reasons that
are user-actionable rather than implicitly somewhere deep down in the
callstack.

Patrick

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