Re: [PATCH v3 4/4] packfile: recover when a multi-pack-index names a removed pack

From: Jeff King <peff@peff.net>
Date: Aug 29, 2026, 12:07 UTC
Message-ID: <20260829120721.GF40814@coredump.intra.peff.net>
In-Reply-To: <9b0966df9a060df215d8aec7816875d42651d5bb.1787986831.git.gitgitgadget@gmail.com>

On Sat, Aug 29, 2026 at 07:00:31AM +0000, Elijah Newren via GitGitGadget wrote:

Show 26 quoted lines

> +	/*
> +	 * Recovery for a concurrent-repack race: a stale MIDX may still name a
> +	 * vanished owning pack even though the object survives in another pack
> +	 * the same MIDX covers.  The regular fallback above skips MIDX-covered
> +	 * packs, and repreparing the on-disk pack set does not reload the
> +	 * borrowed, cached MIDX, so scan its packs directly for the survivor.
> +	 *
> +	 * Do this only on the second read, by which point repreparing packs has
> +	 * already had a chance to find an object merely relocated into a new,
> +	 * uncovered pack; only a genuine hidden duplicate reaches here.
> +	 */
> +	if (midx_result == MIDX_FILL_OWNER_UNAVAILABLE &&
> +	    (flags & OBJECT_INFO_SECOND_READ)) {
> +		struct multi_pack_index *m = store->midx;
> +		uint32_t i;
> +
> +		for (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {
> +			struct packed_git *p;
> +
> +			if (prepare_midx_pack(m, i))
> +				continue;
> +			p = nth_midxed_pack(m, i);
> +			if (p && packfile_fill_entry(p, oid, e, bad_pack))
> +				return 1;
> +		}
> +	}

So I think this workaround is fine to do (as long as we are not going to actually refresh the midx on SECOND_READ, which I agree is probably a bigger change).

I always get confused about m->num_packs and m->num_packs_in_base, and whether we are looking at the packs in a midx slice versus the whole thing. I _think_ what you have here is correct, because we are iterating from 0 up to the total number of packs, and prepare_midx_pack() etc will look back through the incremental slices as necessary.

But I wonder if it would be simpler to just iterate over the actual pack list in the usual way, since we already do that in this function. I _thought_ this would work:

diff --git a/odb/source-packed.c b/odb/source-packed.c
index 90d88c0a12..86e6a80d2f 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -33,40 +33,19 @@ static int find_pack_entry(struct odb_source_packed *store,
 	for (l = store->packs.head; l; l = l->next) {
 		struct packed_git *p = l->pack;
 
-		if (!p->multi_pack_index && packfile_fill_entry(p, oid, e, bad_pack)) {
+		/* ...explain tricky race case here... */
+		if (p->multi_pack_index &&
+		    (midx_result != MIDX_FILL_OWNER_UNAVAILABLE ||
+		     !(flags & OBJECT_INFO_SECOND_READ)))
+			continue;
+
+		if (packfile_fill_entry(p, oid, e, bad_pack)) {
 			if (!store->skip_mru_updates)
 				packfile_list_prepend(&store->packs, p);
 			return 1;
 		}
 	}
 
-	/*
-	 * Recovery for a concurrent-repack race: a stale MIDX may still name a
-	 * vanished owning pack even though the object survives in another pack
-	 * the same MIDX covers.  The regular fallback above skips MIDX-covered
-	 * packs, and repreparing the on-disk pack set does not reload the
-	 * borrowed, cached MIDX, so scan its packs directly for the survivor.
-	 *
-	 * Do this only on the second read, by which point repreparing packs has
-	 * already had a chance to find an object merely relocated into a new,
-	 * uncovered pack; only a genuine hidden duplicate reaches here.
-	 */
-	if (midx_result == MIDX_FILL_OWNER_UNAVAILABLE &&
-	    (flags & OBJECT_INFO_SECOND_READ)) {
-		struct multi_pack_index *m = store->midx;
-		uint32_t i;
-
-		for (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {
-			struct packed_git *p;
-
-			if (prepare_midx_pack(m, i))
-				continue;
-			p = nth_midxed_pack(m, i);
-			if (p && packfile_fill_entry(p, oid, e, bad_pack))
-				return 1;
-		}
-	}
-
 	return 0;
 }
 

but it doesn't because we don't always load the midx'd packs into the
pack list (we do it on-demand as they become useful to us). So I think
you'd essentially end up needing to do a loop like the one you have
anyway to prepare_midx_pack() on them all.

And we want to avoid doing that if we can find it outside the midx
(since that was the whole point of waiting for SECOND_READ). Which would
happen...in that loop. So we really do want to have our own
midx-specific loop like you have here.

Sorry, I know that was a lot of text to end up at "you have already
written it the best way", but it took me a while to reason through it.

The patch looks good to me. ;)

-Peff