From 98926e0d015e03a3b922ba6d8ca652f1da7b4429 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 8 Oct 2021 17:46:32 -0400 Subject: [PATCH] midx.c: lookup MIDX by object directory during expire Before a new MIDX can be written, expire_midx_packs() first loads the existing MIDX, figures out which packs can be expired, and then writes a new MIDX based on that information. In order to load the existing MIDX, it uses load_multi_pack_index(), which mmaps the multi-pack-index file, but does not store the resulting `struct multi_pack_index *` in the object store. write_midx_internal() also needs to open the existing MIDX, and it does so by iterating the results of get_multi_pack_index(), so that it reuses the same pointer held by the object store. But before it can move the new MIDX into place, it close_object_store() to munmap() the multi-pack-index file to accommodate platforms like Windows which don't allow overwriting files which are memory mapped. That's where things get weird. Since expire_midx_packs has its own *separate* memory mapped copy of the MIDX, the MIDX file is still memory mapped! Interestingly, this doesn't seem to cause a problem in our tests. (I believe that this has much more to do with my own lack of familiarity with Windows than it does a lack of coverage in our tests). In any case, we can side-step the whole issue by teaching expire_midx_packs() to use the `struct multi_pack_index` pointer it found via the object store instead of maintain its own copy. That way, when write_midx_internal() calls `close_object_store()`, we know that there are no memory mapped copies of the MIDX laying around. A couple of other small notes about this patch: - As far as I can tell, passing `local == 1` to the call to load_multi_pack_index() was an error, since object_dir could be an alternate. But it doesn't matter, since even though we write `m->local = 1`, we never read that field back later on. - Setting `m = NULL` after write_midx_internal() was likely to prevent a double-free back from when that function took a `struct multi_pack_index *` that it called close_midx() on itself. We can rely on write_midx_internal() to call that for us now. Finally, this enforces the same "the value of --object-dir must be the local object store, or an alternate" rule from f57a739691 (midx: avoid opening multiple MIDXs when writing, 2021-09-01) to the `expire` sub-command, too. Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- midx.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/midx.c b/midx.c index cc2ae7e96b..053279bef3 100644 --- a/midx.c +++ b/midx.c @@ -1696,7 +1696,7 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla { uint32_t i, *count, result = 0; struct string_list packs_to_drop = STRING_LIST_INIT_DUP; - struct multi_pack_index *m = load_multi_pack_index(object_dir, 1); + struct multi_pack_index *m = lookup_multi_pack_index(r, object_dir); struct progress *progress = NULL; if (!m) @@ -1741,12 +1741,11 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla free(count); - if (packs_to_drop.nr) { + if (packs_to_drop.nr) result = write_midx_internal(object_dir, NULL, &packs_to_drop, NULL, NULL, flags); - m = NULL; - } string_list_clear(&packs_to_drop, 0); + return result; }