Re: [PATCH v3 09/25] midx: avoid opening multiple MIDXs when writing

On Tue, Jul 27, 2021 at 05:19:46PM -0400, Taylor Blau wrote:

Opening multiple instance of the same MIDX can lead to problems like two
separate packed_git structures which represent the same pack being added
to the repository's object store.
[...]

Thanks, I think this approach fixes all of the potential problems from
our earlier discussion. You already noted the "!ctx->m" thing in a
follow-up. But also...

Likewise, replace the call to `close_midx()` with
`close_object_store()`, since we're about to replace the MIDX with a new
one and should invalidate the object store's memory of any MIDX that
might have existed beforehand.

Yes, I agree we need to do this, but I don't see the change in the
patch. Did something get lost in the rebasing/squashing process?

I think we'd need something like this:

diff --git a/midx.c b/midx.c
index 6dfafe7a8c..bfb6afea2e 100644
--- a/midx.c
+++ b/midx.c

@@ -1123,8 +1123,7 @@ static int write_midx_internal(const char *object_dir,
 	hold_lock_file_for_update(&lk, midx_name, LOCK_DIE_ON_ERROR);
 	f = hashfd(get_lock_file_fd(&lk), get_lock_file_path(&lk));
 
-	if (ctx.m)
-		close_midx(ctx.m);
+	close_object_store(the_repository->objects);
 
 	if (ctx.nr - dropped_packs == 0) {
 		error(_("no pack files to index."));

though I'm not sure:

 - if this should be unconditional or dependent on ctx.m (I think the
   latter, because if we are renaming over any open midx, we would have
   filled in ctx.m earlier).

 - if this should go below the "no pack files to index" check (i.e., is
   there any point in closing if we know we will not write?). In fact,
   its purpose might be more obvious right before finalize_hashfile(),
   but I am OK either way on that.

-Peff