Re: [PATCH 2/4] midx.c: lookup MIDX by object directory during expire

[Derrick Stolee <stolee@gmail.com>]

On 10/8/2021 5:46 PM, Taylor Blau wrote:
> 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).

You are fixing a bug in two ways:

1. This change ensures we use the 'struct multi_pack_index' that exists
   in the object store, ensuring it is closed before committing the lockfile.

2. Without this change, the change in patch 4 would cause the 'expire' tests
   to start failing on Windows, because the commands would die().

If our tests also verified that the .git/objects/pack/multi-pack-index.lock
file was missing at the end of the command, then we would have caught this
bug on Windows. I don't think that's a reasonable test, but instead we(I)
should have used the API correctly from the start.

The tests _do_ verify that the expired packs are deleted, but the new MIDX
probably refers to the old packs still. Since those packs are not actually
used (the necessary condition for expiring them), later Git commands do not
attempt to load them and hence do not fail. That is, until we try to expire
again and likely get warnings about missing pack-files.

Thanks,
-Stolee