Re: [PATCH v2 2/8] builtin/multi-pack-index.c: support `--stdin-packs` mode

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 23, 2021, 02:06 UTC
Message-ID: <YUvhPpRzyfBua0wB@nand.local>
In-Reply-To: <20210922231137.2610100-1-jonathantanmy@google.com>

On Wed, Sep 22, 2021 at 04:11:37PM -0700, Jonathan Tan wrote:

Show 13 quoted lines

> > An alternative approach
> > would have been to call that function from the `git repack` builtin
> > directly, but this introduces awkward problems around closing and
> > reopening the object store, so the MIDX will be written out-of-process.
>
> I'm not sure if the implementation direction started by this patch
> (eventually, running "git multi-pack-index write --stdin-packs" to
> replace the midx of a repository while "git repack" is running) would
> work on platforms in which mmap-ing a file means that other processes
> can't delete it, but if it works on a Windows CI, this should be fine.
> (I don't have a Windows CI handy to test it on, though.)
>
> Assuming it works on platforms like Windows, this patch looks fine.

It definitely passes CI :-). But special care is taken to handle this case, and it works for a couple of reasons. Notably that we only call `write_midx_included_packs()` (which in turn invokes the multi-pack-index builtin as a sub-process) *after* repack has called close_object_store(). That means that `repack` won't be holding the MIDX open while the sub-process is trying to write a new one in its place.

The other reason is that the multi-pack-index process also makes sure to close the old MIDX before writing the new one, so we can be certain that neither of these spots are mapping the file or have it opened when trying to write over it.

Thanks, Taylor