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

[Taylor Blau <ttaylorr@openai.com>]

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:
>
> > 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.

> > @@ -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