Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'

On Wed, Sep 30, 2026 at 04:31:08PM -0400, Jeff King wrote:

On Tue, Sep 29, 2026 at 08:28:49PM -0500, Taylor Blau wrote:

quoted

In cd846bacc7d (pack-objects: introduce '--stdin-packs=follow',
2025-06-23), this behavior changed such that whenever excluded-open
('!') packs are present, the walk stops at objects in excluded-closed
('^') packs. Geometric repacks use '^' for retained packs already in the
MIDX, relying on the indexed object set being closed under reachability.

However, the walk introduced in cd846bacc7d starts only from commit
objects. A geometric repack can therefore produce a MIDX that does not
maintain reachability closure for lone trees (that are not reachable
from any commit otherwise in the closure).

A later walk with '!' packs can stop at that tree in a retained '^'
pack even if a new commit reaches it. If the cruft pack remains
excluded, and the bitmap selection picks one or more commits which reach
that tree, the MIDX cannot generate a bitmap for that commit.

OK. It took me a minute to grok this, and what I got hung up on is "a
later walk". I thought you meant a later walk within the same process,
but you mean "a subsequent repack / midx generation".

So we fail to walk in an earlier repack, but we might not fail there
because no bitmapped commit happens to require that closure. But we've
set up a timebomb for that later repack, because our pack which is
_supposed_ to be closed (and thus gets marked with "^") is broken.

So this fixes the initial generation of that timebomb. It doesn't help
us deal with existing bombs, but presumably the solution there is a full
repack (and we would not want to deal with existing bombs, because the
point of "^" is that we can trust it and avoid lots of extra traversal).

Not really asking for a change to the commit message, but just
documenting my understanding (which hopefully matches yours ;) ).

Yup, exactly. Hopefully s/walk/repack/ clarifies things for the
following round, but in the meantime your understanding matches my own.

When this feature was originally introduced, the idea was "anything
packed must also pack its reachability closure, less any objects in
excluded packs". That was true for commit objects, but not so for trees
and annotated tags, which is what this patch corrects.

quoted

@@ -3846,6 +3847,9 @@ static int add_object_entry_from_pack(const struct object_id *oid,
 		 * list after checking `want_object_in_pack()` below.
 		 */
 		add_pending_oid(ctx->revs, NULL, oid, 0);
+	} else if (ctx->mode == STDIN_PACKS_MODE_FOLLOW &&
+		   (type == OBJ_TREE || type == OBJ_TAG)) {
+		oid_array_append(&ctx->extra_roots, oid);
 	}

And this is the interesting part. What about blobs? I guess we don't
care about them because they are either there or not. There is no need
to walk them independently because they can't reference anything.

Exactly.

If I understand this subtree claim, you are worried about the
(single-traversal) case that we manually queue tree A, and then later
visit commit C, which eventually has A as a sub-tree. So we queue A
again _after_ its original, but that second visit (that we skip) would
have had more interesting information (like path context).

But I don't think a second walk clears you of that possibility. You are
queuing tags, too, which might in turn point to commits. So you might
get the same commit traversal within that second walk.

Yeah, that's what I was worried about when I wrote this patch, but
that's a good point. Really there is no "absolute" correct path for a
given tree or tree entry, since it depends on your perspective.

I think you could fix it by putting tags into the first walk. But it
will always exist to some degree (you could have a tag that points to a
tree and queue that tree, but also a commit that points to it).

It's not clear to me how big a problem this is in practice. We know that
the "path" of a tree or blob in a traversal is subject to context. There
might be multiple commits that point to it at different levels. I guess
it might be more common if we are adding random trees from a pack
without context.

;-).

So I dunno. I'd probably be OK proceeding with this as-is, because I
fear that dual-queue thing I mentioned above might turn into a rabbit
hole that would derail the much more important fix.

I tightened up the comment a bit, but I agree that rethinking the
traversal machinery is best left for another day.

Thanks,
Taylor