Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary

[Derrick Stolee <stolee@gmail.com>]

On 8/25/2020 12:47 PM, Jeff King wrote:
> It may be worth thinking a bit about atomicity here, though. Rather than
> separate delete and write steps, would somebody want a sequence like:

...
> If we do care, though, that implies some cooperation between the
> deletion process and the midx code. Perhaps it even argues that repack
> should refuse to delete such a single pack at all, since it _isn't_
> redundant. It's part of a midx, and the caller should rewrite the midx
> first itself, and _then_ look for redundant packs.

It is worth noting that we _do_ have a way to integrate the delete and
write code using 'git multi-pack-index expire'. One way to resolve this
atomicity would be to do the following inside the repack command:

 1. Create and index the new pack.
 2. git multi-pack-index write
 3. git multi-pack-index expire

While this _mostly_ works, we still have issues around a concurrent
process opening a multi-pack-index before step (2) and trying to
read from the pack-file deleted in step (3). For this reason, the
'incremental-repack' task in the maintenance builtin series [1] runs
'expire' then 'repack' (and not the opposite order). To be completely
safe, we'd want to make sure that no Git command that started before
the 'repack' command is still running when we run 'expire'. In practice,
it suffices to run the 'incremental-repack' step at most once a day.

[1] https://lore.kernel.org/git/a8d956dad6b3a81d0f1b63cbd48f36a5e1195ae8.1597760730.git.gitgitgadget@gmail.com/

This discussion points out a big reason why the multi-pack-index
wasn't fully integrated with 'git repack': it can be tricky. Using
a repacking strategy that is focused on the multi-pack-index is
less tricky.

I still think that this patch is moving us in a better direction.

I'm making note of these possible improvements:

1. Have 'git repack' integrate with 'git multi-pack-index expire'
   when deleting pack-files after creating a new one (if a
   multi-pack-index exists).

2. Handle "missing packs" referenced by the multi-pack-index more
   gracefully, likely by disabling the multi-pack-index and
   calling reprepare_packed_git().

Thanks,
-Stolee