Re: [PATCH] pack-bitmap.c: ensure pack validity for all reuse packs

[Taylor Blau <me@ttaylorr.com>]

On Wed, Dec 18, 2024 at 11:41:50AM -0800, Junio C Hamano wrote:
> Both are from you, and I am guessing that you have tried all of your
> topics in flight together, if not the other topics.

Oops, sorry for the trouble. I believe the correct resolution is the
following:

--- 8< ---
diff --cc pack-bitmap.c
index ff09b15eb7,83696d834f..0000000000
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@@ -388,9 -427,17 +427,16 @@@ static int open_midx_bitmap_1(struct bi
  {
  	struct stat st;
  	char *bitmap_name = midx_bitmap_filename(midx);
- 	int fd = git_open(bitmap_name);
+ 	int fd;
 -	uint32_t i, preferred_pack;
 -	struct packed_git *preferred;
 +	uint32_t i;

+ 	fd = git_open(bitmap_name);
+ 	if (fd < 0 && errno == ENOENT) {
+ 		FREE_AND_NULL(bitmap_name);
+ 		bitmap_name = midx_bitmap_filename(midx);
+ 		fd = git_open(bitmap_name);
+ 	}
+
  	if (fd < 0) {
  		if (errno != ENOENT)
  			warning_errno("cannot open '%s'", bitmap_name);
@@@ -444,6 -491,25 +490,13 @@@
  		}
  	}

 -	if (midx_preferred_pack(bitmap_git->midx, &preferred_pack) < 0) {
 -		warning(_("could not determine MIDX preferred pack"));
 -		goto cleanup;
 -	}
 -
 -	preferred = nth_midxed_pack(bitmap_git->midx, preferred_pack);
 -	if (!is_pack_valid(preferred)) {
 -		warning(_("preferred pack (%s) is invalid"),
 -			preferred->pack_name);
 -		goto cleanup;
 -	}
 -
+ 	if (midx->base_midx) {
+ 		bitmap_git->base = prepare_midx_bitmap_git(midx->base_midx);
+ 		bitmap_git->base_nr = bitmap_git->base->base_nr + 1;
+ 	} else {
+ 		bitmap_git->base_nr = 1;
+ 	}
+
  	return 0;

  cleanup:
--- >8 ---

IOW, we no longer need to check the validity of the preferred pack in
either case. But in an incremental MIDX bitmaps world, we do need to
keep calling prepare_midx_bitmap_git() along the MIDX's ->base pointer,
if non-NULL.

> I wonder what we can do better to make sure the work a contributor has
> already done (in this case, resolve interaction between two topics) is
> not wasted and recreated (possibly incorrectly) by the maintainer.

I am not sure. During the interim maintainer period, Patrick sent a
couple of rounds of ps/build with a final patch to the effect of
"unbreak everything in seen", which could be dropped.

But I think an easier thing to do would have been for myself to indicate
that you'd run into a non-trivial conflict here and provide the
resolution proactively.

Thanks,
Taylor