Re: [PATCH v3 3/4] mktree: do not use OBJECT_INFO_QUICK when checking objects

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

mktree_line() checks each referenced object's type with
odb_read_object_info_extended() under OBJECT_INFO_QUICK.  QUICK skips the
reprepare-and-retry that reloads the on-disk pack set, so a resident
"git mktree --batch" reader reports an object that a concurrent repack
just relocated into a new pack as missing, and rejects the entry.

QUICK entered this lookup in 817b0f602710 (mktree: do not check type of
remote objects, 2022-06-21) only to avoid lazily fetching promisor
objects; OBJECT_INFO_SKIP_FETCH_OBJECT already provides that.  Drop
OBJECT_INFO_QUICK and keep OBJECT_INFO_SKIP_FETCH_OBJECT, so mktree still
avoids a promisor fetch but recovers an object that was merely repacked.

I think this line of reasoning is fine.

We probably _could_ use QUICK when the caller specified --missing, which
would optimize out the SECOND_READ effort if the caller told us they
expect (or at least allow) some items to be missing. But:

  1. It's not clear how people use --missing. If you are just trying to
     be gentle with an occasional missing entry, then the optimization
     is not that interesting. If you run mktree all the time to make
     synthetic trees full of objects you don't have, then maybe you do
     care about the optimization. But if you are doing that then you
     probably are better off with an option that avoids the lookup
     entirely (i.e., we should just trust the type found in the input).

     So there's maybe room for a --yolo argument to mktree, though I
     guess in practice you could just use "hash-object" for that. But
     either way that is way out of scope for this patch.

  2. Prior to 817b0f602710 we were not QUICK either! And that commit was
     only trying to trigger SKIP_FETCH_OBJECT. So whether there is an
     argument for linking --missing and QUICK or not, it should be made
     separately. This patch is just fixing the extra flag that probably
     should not have been added by 817b0f602710.

+test_expect_success PIPE 'mktree --batch survives a concurrent repack retiring a pack' '

OK. I was hoping we could test this without all of the PIPE complexity,
but I don't think we can. We really need a case where the first lookup
fails but SECOND_READ succeeds, which is inherently a race. Feeding one
entry at a time lets us implement that in a deterministic way, and I
think is the simplest we can get.

So the patch looks good to me overall.

-Peff