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

[Derrick Stolee <stolee@gmail.com>]

On 8/29/2026 3:00 AM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>

I'm late in reviewing this patch, so forgive me responding inline as
I discover how it works.

tl;dr: Good patch. LGTM.

> Teach find_pack_entry() to recover.  The MIDX lookup now returns a
> tri-state, distinguishing an object absent from the MIDX from one it owns
> via a pack that can no longer be opened; in the latter case, once the
> regular fallback has also missed, scan the MIDX's packs directly for a
> surviving copy.  Because the return value is no longer a boolean, rename
> fill_midx_entry() to midx_fill_entry() so callers must reckon with the
> new enum rather than silently treat MIDX_FILL_OWNER_UNAVAILABLE as a hit.

This tri-state is valuable!
 
> Do the scan only on the second read (OBJECT_INFO_SECOND_READ): by then
> the cheaper on-disk reload has run, so an object merely relocated into a
> new (uncovered) pack has already been found by the regular fallback, and
> only a genuine hidden duplicate reaches the rescan.  A QUICK caller that
> skips the second read simply accepts the false negative, as QUICK is
> designed to.
> 
> Reloading the stale MIDX would be a more complete fix but is much more
> involved (the borrowers above need proper invalidation), so leave that
> for later.

> -		if (m && fill_midx_entry(m, oid, &e, NULL)) {
> +		if (m && midx_fill_entry(m, oid, &e, NULL) == MIDX_FILL_HIT) {

One major benefit to the rename is that we can guarantee that
all callers are updated to reflect the new tri-state response.

It also has a better naming convention, overall.

(reordered header file diff up)
> +/*
> + * Result of looking an object up in a multi-pack-index.  MIDX_FILL_HIT means
> + * "e was filled in"; the two miss variants distinguish an object the midx does
> + * not know about (MIDX_FILL_MISS) from one it does know about but whose owning
> + * pack we can no longer open (MIDX_FILL_OWNER_UNAVAILABLE -- the signature of a
> + * concurrent repack having removed that pack).  A known-bad (corrupt) object
> + * reports MIDX_FILL_MISS but also sets *bad_pack, if provided, to the owning
> + * pack so the caller can tell "corrupt" apart from "absent".
> + */
> +enum midx_fill_result {
> +	MIDX_FILL_MISS = 0,
> +	MIDX_FILL_HIT,
> +	MIDX_FILL_OWNER_UNAVAILABLE,
> +};
> +
> +enum midx_fill_result midx_fill_entry(struct multi_pack_index *m,
> +				      const struct object_id *oid,
> +				      struct pack_entry *e,
> +				      struct packed_git **bad_pack);

This is good documentation that will help future uses know how to
react to the different modes.

> -int fill_midx_entry(struct multi_pack_index *m,
> -		    const struct object_id *oid,
> -		    struct pack_entry *e,
> -		    struct packed_git **bad_pack)
> +enum midx_fill_result midx_fill_entry(struct multi_pack_index *m,
> +				      const struct object_id *oid,
> +				      struct pack_entry *e,
> +				      struct packed_git **bad_pack)
>  {
>  	uint32_t pos;
>  	uint32_t pack_int_id;
>  	struct packed_git *p;
>  
>  	if (!bsearch_midx(oid, m, &pos))
> -		return 0;
> +		return MIDX_FILL_MISS;

Obviously correct: this OID isn't in the sorted list.

>  	midx_for_object(&m, pos);
>  	pack_int_id = nth_midxed_pack_int_id(m, pos);
>  
>  	if (prepare_midx_pack(m, pack_int_id))
> -		return 0;
> +		return MIDX_FILL_OWNER_UNAVAILABLE;

Obviously correct: we tried to open the pack index but failed.

>  	p = m->packs[pack_int_id - m->num_packs_in_base];
>  
>  	/*
> @@ -616,19 +616,19 @@ int fill_midx_entry(struct multi_pack_index *m,
>  	* loaded!
>  	*/
>  	if (!is_pack_valid(p))
> -		return 0;
> +		return MIDX_FILL_OWNER_UNAVAILABLE;

Same: Pack is invalid somehow, likely that the .pack disappeared.

>  	if (oidset_size(&p->bad_objects) &&
>  	    oidset_contains(&p->bad_objects, oid)) {
>  		if (bad_pack && !*bad_pack)
>  			*bad_pack = p;
> -		return 0;
> +		return MIDX_FILL_MISS;

This one is tricky, but makes sense: we have marked this as a
"bad" object so we should act like it doesn't exist. Good.

>  	}
>  
>  	e->offset = nth_midxed_offset(m, pos);
>  	e->p = p;
>  
> -	return 1;
> +	return MIDX_FILL_HIT;

finally: success!>  }


>  static int find_pack_entry(struct odb_source_packed *store,
>  			   const struct object_id *oid,
>  			   struct pack_entry *e,
> +			   enum object_info_flags flags,
>  			   struct packed_git **bad_pack)
>  {
>  	struct packfile_list_entry *l;
> +	enum midx_fill_result midx_result = MIDX_FILL_MISS;
>  
>  	odb_source_prepare(&store->base, 0);
> -	if (store->midx && fill_midx_entry(store->midx, oid, e, bad_pack))
> -		return 1;
> +	if (store->midx) {
> +		midx_result = midx_fill_entry(store->midx, oid, e, bad_pack);
> +		if (midx_result == MIDX_FILL_HIT)
> +			return 1;
> +	}

This looks good. On a hit, we return. Act like a MIDX-miss if we
don't have a midx.

Outside of the patch context is the "reprepare packfiles" to pick
up a copy from a packfile that doesn't exist within the current
(stale) midx.
> +	/*
> +	 * 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.
> +	 */

This comment does a lot of important context-setting to show
that we are in a very narrow case: the stale MIDX has multiple
packs that contain the requested object, but the "newer" one
was deleted without creating a new packfile, so we need to
look at each contained pack for the object from its pack-index.

> +	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;
> +		}
> +	}
> +

This is hopefully a very rare case, but it's good to have
this "fall back to O(num packs)" situation.

> +test_expect_success 'lookup recovers object whose midx-owning pack was removed' '
> +	test_when_finished "rm -fr repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +
> +		# "keep" ends up only in the big pack; "dup" is deliberately
> +		# placed in two packs so the midx has to choose an owner.
> +		test_commit keep &&
> +		echo duplicated-content >dup &&
> +		git add dup &&
> +		git commit -m dup &&
> +		dup_oid=$(git rev-parse HEAD:dup) &&
> +
> +		# Roll every object, including dup, into a single big pack.
> +		git repack -adq &&
> +
> +		# Build a second, "moderate" pack that also contains dup, so dup
> +		# now lives in two packs that the midx will cover.
> +		moderate=$(echo "$dup_oid" |
> +			git pack-objects --quiet $objdir/pack/pack) &&
> +
> +		# Attribute dup to the moderate pack in the midx.
> +		git multi-pack-index write \
> +			--preferred-pack="pack-$moderate.idx" &&

This use of preferred pack is a good way of getting around mtimes
that could be equal. We could also consider updating mtimes, but
this works so don't change it.

> +		# Simulate a concurrent "git repack" retiring the moderate pack:
> +		# its files disappear, but the now-stale midx still names it as
> +		# the owner of dup.  A valid copy of dup survives in the big pack.
> +		rm -f $objdir/pack/pack-$moderate.* &&
> +
> +		# The midx routes the lookup to the deleted pack, and the regular
> +		# pack fallback skips midx-covered packs, so without recovery dup
> +		# would appear missing even though it is physically present.
> +		echo blob >expect &&
> +		git cat-file -t "$dup_oid" >actual &&
> +		test_cmp expect actual
> +	)
> +'
Thanks for adding this test so we can keep this narrow case
working in perpetuity.

Thanks,
-Stolee