On Mon, Jul 26, 2021 at 06:14:09PM -0400, Taylor Blau wrote: > > Ah, this jogged my memory: this is a relic from when we generated MIDX > > bitmaps in-process with the rest of the `repack` code. And when we did > > that, we did have to call `reprepare_packed_git()` after writing the new > > packs but before moving them into place. > > Actually, I take that back. You were right from the start: the way the > code is written we *can* end up calling both: > > - load_multi_pack_index, from write_midx_internal(), which sets up a > MIDX, but does not update r->objects->multi_pack_index to point at > it. > > - ...and prepare_multi_pack_index_one (via prepare_bitmap_git -> > open_bitmap -> open_midx_bitmap -> get_multi_pack_index -> > prepare_packed_git) which *also* creates a new MIDX, *and* > updates the_repository->objects->multi_pack_index to point at it. > > (The latter codepath is from the check in write_midx_internal() to see > if we already have a MIDX bitmap when the MIDX we are trying to write > already exists on disk.) > > So in this scenario, we have two copies of the same MIDX open, and the > repository's single pack is opened in one of the MIDXs, but not both. > One copy of the pack is pointed at via r->objects->packed_git. Then when > we fall back to open_pack_bitmap(), we call get_all_packs(), which calls > prepare_midx_pack(), which installs the second MIDX's copy of the same > pack into the r->objects->packed_git, and we have a cycle. Right, I understand how that ends up with duplicate structs for each pack. But how do we get a cycle out of that? > I think there are a few ways to fix this bug. The most obvious is to > make install_packed_git() check for the existence of the pack in the > hashmap of installed packs before (re-)installing it. But that could be > quadratic if the hashmap has too many collisions (and the lookup tends > towards being linear in the number of keys rather than constant). I think it may be worth doing that anyway. You can assume the hashmap will behave reasonably. But it would mean that the "multi_pack_index" flag in packed_git does not specify _which_ midx is pointing to it. At the very least, it would need to become a ref-count (so when one midx goes away, it does not lose its "I am part of a midx" flag). And possibly it would need to actually know the complete list of midx structs it's associated with (I haven't looked at all of the uses of that flag). That makes things sufficiently tricky that I would prefer not to untangle it as part of this series. > But I think that a more straightforward way would be to open the MIDX we > use when generating the MIDX with prepare_multi_pack_index_one() instead > of load_multi_pack_index() so that the resulting MIDX is pointed at by > r->objects->multi_pack_index. That would prevent the latter call from > deep within the callstack of prepare_bitmap_git() from opening another > copy and then (mistakenly) re-installing the same pack twice. But now the internal midx writing code can never call close_midx() on that, because it does not own it to close. Can we simply drop the close_midx() call there? This would all make much more sense to me if write_midx_internal() simply took a conceptually read-only midx as a parameter, and the caller passed in the appropriate one (probably even using prepare_multi_pack_index_one() to get it). -Peff
