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

> 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 ;) ).

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

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

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