Re: [PATCH v2 3/4] packfile: recover object lookups racing a concurrent repack

[Jeff King <peff@peff.net>]

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

>   1. open_pack_index() fails, so we print
> 
>         error: packfile <path> index unavailable
> 
>      and report the pack as unusable, even though the object still lives
>      in the replacement pack.
> 
>   2. A normal lookup recovers: odb_read_object_info_extended() issues a
>      second read that reloads the on-disk pack state and finds the object
>      in its new home, making the message above mere noise.  But an
>      OBJECT_INFO_QUICK lookup deliberately skips that second read to stay
>      fast on a genuine miss, so it does *not* recover: it reports the
>      object as absent even though it still lives in the replacement pack.
>      A resident reader that resolves objects with a QUICK lookup -- such
>      as the `git mktree --batch` process the tests below drive -- then
>      produces wrong results.  Even where a spurious miss is not fatal it
>      is not harmless: `git upload-pack` checks a client's "have" lines
>      with a QUICK lookup, and a dropped "have" removes a common object
>      from the negotiation, so the client is sent more than it needs.

Maybe I am still being dense, but this description does not make any
sense to me at all.

The _point_ of QUICK is to accept those false negatives. It is the right
thing for upload-pack to do, to avoid re-scans for objects which we
simply don't have (and don't necessarily expect to have).

It sounds like mktree is wrong to be using QUICK at all. It comes from
817b0f6027 (mktree: do not check type of remote objects, 2022-06-21)
which rewrote a call to vanilla oid_object_info(). From the description
there it probably should be using SKIP_FETCH_OBJECT but not QUICK. Or
possibly it should use neither unless --missing is given.

So I don't see QUICK itself here violating any contract (even if it
_could_ find the object in some cases with just a little more work, as
in the case that we were discussing for v1).

The much more interesting case is the non-QUICK one that Patrick
outlined earlier in the thread. Where we say "nope, we don't have that
object" even though we could find it with a little more work. But that
doesn't seem to be described here either. But I think that is not even
what this patch is about; that's in patch 4.

If the "error:" message is scary and gross (especially because we may
retry and correct it anyway) and happens due to routine races, we might
consider suppressing it.

> +	/*
> +	 * Set when a lookup finds that a pack we already know about has
> +	 * vanished -- its ".idx" or ".pack" removed out from under us, the
> +	 * signature of a concurrent "git repack".  It tells
> +	 * odb_read_object_info_extended() to reprepare and retry even for an
> +	 * OBJECT_INFO_QUICK lookup, which normally skips that rescan to stay
> +	 * fast on a genuine miss.  Reset when the packfiles are reprepared
> +	 * (see odb_source_packed_prepare()).
> +	 */
> +	unsigned stale_packs_detected : 1;

So this is a way of hackily triggering SECOND_READ for QUICK queries,
even though the point of QUICK is to suppress that second read! Again,
maybe I'm just being dense, but I don't get it.

> @@ -535,8 +550,20 @@ static int open_packed_git_1(struct packed_git *p)
>  	ssize_t read_result;
>  	const unsigned hashsz = p->repo->hash_algo->rawsz;
>  
> -	if (open_pack_index(p))
> +	if (open_pack_index(p)) {
> +		/*
> +		 * A concurrent repack may have removed this pack, deleting its
> +		 * ".idx" before its ".pack" (see unlink_pack_path()).  If the
> +		 * index simply vanished, note the stale pack set and stay
> +		 * quiet; the pack is still reported unusable.  Only a
> +		 * still-present but unreadable index is worth an error.
> +		 */
> +		if (pack_index_is_missing(p)) {
> +			p->repo->objects->stale_packs_detected = 1;
> +			return -1;
> +		}
>  		return error("packfile %s index unavailable", p->pack_name);
> +	}

And this seems racy. We might catch the .idx but miss the .pack file.
That would cause a failed read, but not trigger sale_packs_detected.

-Peff