Re: [PATCH v4 05/25] midx: clear auxiliary .rev after replacing the MIDX

Junio C Hamano [off-list ref] writes:

FWIW, here is what I have somewhere in 'seen' where two topics meet.

Oops, one change missed.

diff --cc midx.c
index c0209751b5,4574e6d411..0000000000

--- i/midx.c
+++ w/midx.c

@@@ -947,11 -1136,29 +1136,29 @@@ static int write_midx_internal(const ch
  	for_each_file_in_pack_dir(object_dir, add_pack_to_midx, &ctx);
  	stop_progress(&ctx.progress);
  
- 	if (ctx.m && ctx.nr == ctx.m->num_packs && !packs_to_drop)
- 		goto cleanup;
+ 	if (ctx.m && ctx.nr == ctx.m->num_packs && !packs_to_drop) {
+ 		struct bitmap_index *bitmap_git;
+ 		int bitmap_exists;
+ 		int want_bitmap = flags & MIDX_WRITE_BITMAP;
+ 
+ 		bitmap_git = prepare_midx_bitmap_git(the_repository, ctx.m);
+ 		bitmap_exists = bitmap_git && bitmap_is_midx(bitmap_git);
+ 		free_bitmap_index(bitmap_git);
+ 
+ 		if (bitmap_exists || !want_bitmap) {
+ 			/*
+ 			 * The correct MIDX already exists, and so does a
+ 			 * corresponding bitmap (or one wasn't requested).
+ 			 */
+ 			if (!want_bitmap)
 -				clear_midx_files_ext(the_repository, ".bitmap",
++				clear_midx_files_ext(object_dir, ".bitmap",
+ 						     NULL);
+ 			goto cleanup;
+ 		}
+ 	}
  
- 	ctx.preferred_pack_idx = -1;
  	if (preferred_pack_name) {
+ 		int found = 0;
  		for (i = 0; i < ctx.nr; i++) {
  			if (!cmp_idx_or_pack_name(preferred_pack_name,
  						  ctx.info[i].pack_name)) {
@@@ -1090,6 -1351,9 +1351,9 @@@
  
  	commit_lock_file(&lk);
  
 -	clear_midx_files_ext(the_repository, ".bitmap", midx_hash);
 -	clear_midx_files_ext(the_repository, ".rev", midx_hash);
++	clear_midx_files_ext(object_dir, ".bitmap", midx_hash);
++	clear_midx_files_ext(object_dir, ".rev", midx_hash);
+ 
  cleanup:
  	for (i = 0; i < ctx.nr; i++) {
  		if (ctx.info[i].p) {
@@@ -1165,7 -1429,8 +1429,8 @@@ void clear_midx_file(struct repository 
  	if (remove_path(midx))
  		die(_("failed to clear multi-pack-index at %s"), midx);
  
 -	clear_midx_files_ext(r, ".bitmap", NULL);
 -	clear_midx_files_ext(r, ".rev", NULL);
++	clear_midx_files_ext(r->objects->odb->path, ".bitmap", NULL);
 +	clear_midx_files_ext(r->objects->odb->path, ".rev", NULL);
  
  	free(midx);
  }

On Tue, Aug 24, 2021 at 03:06:55PM -0700, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

FWIW, here is what I have somewhere in 'seen' where two topics meet.

Oops, one change missed.

Thanks; that matches my own resolution. I noticed that it does fail the
new test in t5319, since writing a MIDX wants to make sure that we are
only touching an alternate's object directory (which will fail if we are
running `git multi-pack-index` from outside of a repository).

My opinion is that we should require being inside of a repository to run
the MIDX builtin. Otherwise we're allowing that command to modify any
old MIDX, which doesn't make sense.

I think we probably need a single unifying topic, so I'm happy if you
want to discard one of our two topics from seen in the meantime.

Thanks,
Taylor