On Wed, Jun 29, 2022 at 05:44:25PM -0700, Gregory Szorc wrote: > Revising my initial post, not running `loose-objects` is insufficient: > we also need to prevent `incremental-repack` from running while a `git > gc` / `git repack` is in progress. If an `incremental-repack` runs > concurrently with `git repack`, `repack` can error after bitmap > generation but before the temp packfile is renamed with "could not > find 'pack-*.pack'." I suspect this has to do with > `incremental-repack` deleting packs that were initially in place when > `git repack` started running. But I haven't looked into where exactly > repack is failing. Yeah, you would need to prevent other writers from removing packs while writing a cruft pack. I think the canonical way to do this would be to let `git maintenance` use its own locking to ensure that it wasn't trying to remove packs while simultaneously generating a cruft pack. But assuming that the cruft pack generation is running independently and concurrently with the maintenance incremental-repack task (which will delete packs in the background), then there is definitely a TOCTOU race there. The race is that `repack` will signal to `pack-objects` which packs will stay and which packs will be deleted during the repack. To generate a cruft pack, the packs that stay are formed by doing a reachability traversal and writing a pack that contains all reachable objects. That way anything that is in `all-packs \ reachable` (which form the contents of the cruft pack) are going to be just the unreachable objects. But if a pack that was going to be deleted by repack *after* generating the cruft pack disappears while `repack` is running (particularly after it starts, but before it generates the cruft pack), then the cruft pack generation can't proceed, since it has no idea what objects it needs to add. Namely, if it is told that pack P is going to be deleted, any objects in P which don't appear in a pack that *isn't* going to be deleted need to be saved. If we don't have a copy of P anymore, then there is no way to come up with what that set of objects is, meaning that it's impossible to generate the cruft pack. I should mention, BTW, that this isn't a problem unique to cruft packs. Geometric repacking, which uses `pack-objects --stdin-packs` has a similar issue where if a pack being combined is removed from underneath while `repack` is running, `pack-objects` cannot successfully generate the pack. > However, I think there is yet another bug at play: running > `incremental-repack` appears to be able to repack the cruft packfile. > In doing so, it deletes its .mtimes file and the metadata inside. That sounds like a bug to me. I'll take a look into it and see what I can find. Thanks, Taylor
