From: Jeff King <peff@peff.net>
Date: Thu, 27 Aug 2026 06:06:22 GMT
Subject: Re: [PATCH v2 4/4] packfile: recover when a multi-pack-index names a removed pack
Message-ID: <20260827060622.GC189659@coredump.intra.peff.net>
In-Reply-To: <eacf6ba4b11e366466da18b7b668e65793c532a9.1787684429.git.gitgitgadget@gmail.com>

On Tue, Aug 25, 2026 at 07:00:29PM +0000, Elijah Newren via GitGitGadget wrote:

> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 399acd0f22..30ad7d822c 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1786,7 +1786,7 @@ static int want_object_in_pack_mtime(const struct object_id *oid,
>  		struct multi_pack_index *m = get_multi_pack_index(files->packed);
>  		struct pack_entry e;
>  
> -		if (m && fill_midx_entry(m, oid, &e, NULL)) {
> +		if (m && fill_midx_entry(m, oid, &e, NULL) == MIDX_FILL_HIT) {
>  			want = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime);
>  			if (want != -1)
>  				return want;

We've changed the return value semantics without changing the signature
(or name). So we need to make sure we adjust all callers, as here.
That's _probably_ OK in practice for such a specialized function. But we
could also rename it if we wanted to be paranoid (especially about
new callers added on parallel branches).

> +enum midx_fill_result fill_midx_entry(struct multi_pack_index *m,
> +				      const struct object_id *oid,
> +				      struct pack_entry *e,
> +				      struct packed_git **bad_pack)

OK, so this is our tri-state fix. Mostly looks as expected, though:

>  	if (prepare_midx_pack(m, pack_int_id))
> -		return 0;
> +		goto owner_unavailable;

I'd have expected just "return MIDX_FILL_OWNER_UNAVAILABLE" here. But
then, I'm not sure I buy the need for this stale_packs_detected stuff
from patch 3.

>  	p = m->packs[pack_int_id - m->num_packs_in_base];
>  
> -	/*
> -	* We are about to tell the caller where they can locate the
> -	* requested object.  We better make sure the packfile is
> -	* still here and can be accessed before supplying that
> -	* answer, as it may have been deleted since the MIDX was
> -	* loaded!
> -	*/
> +	/* Make sure the pack is still present before pointing at it. */
>  	if (!is_pack_valid(p))
> -		return 0;
> +		goto owner_unavailable;

This comment rewrite seems superfluous at best. Can we try to keep such
patch fluff to a minimum?

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

OK, and this is as-before but now gated on the SECOND_READ flag. As
expected in this revision.

-Peff