From: Jeff King <peff@peff.net>
Date: Fri, 02 Oct 2026 23:13:36 GMT
Subject: Re: [PATCH v2 2/8] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'
Message-ID: <20261002231336.GB834759@coredump.intra.peff.net>
In-Reply-To: <940953e5c407046ac6789367f3341dc8ea73d07a.1790827875.git.me@ttaylorr.com>

On Wed, Sep 30, 2026 at 11:11:39PM -0500, Taylor Blau wrote:

> @@ -4151,6 +4158,34 @@ static void read_stdin_packs(struct repository *repo,
>  			     show_object_pack_hint,
>  			     &mode);
>  
> +	/*
> +	 * Trees and tags need closure even when no commit reaches them.
> +	 * Defer adding these roots to revs.pending until the first walk
> +	 * finishes. Otherwise a subtree may be visited and marked SEEN
> +	 * before its commit's root tree, using "a" instead of "sub/a"
> +	 * for a blob's namehash and delta attributes.
> +	 *
> +	 * Tags may introduce more commits in the second walk, so this
> +	 * does not *always* guarantee that trees are always visited
> +	 * with their full paths.
> +	 */

I think one of the things that confused me reading the original patch
(but is still present here) is that this comment is in read_stdin_packs,
when we're actually doing the walks. So "defer adding these roots" feels
quite late. We already did that deferring long before in
add_object_entry_from_pack() and add_loose_object(), when we called
oidset_insert() instead of add_pending().

So it would have made more sense to me to comment it there. Of course
that is hard when there are two such places.

I dunno.

> +	oidset_iter_init(&ctx.extra_roots, &iter);
> +	while ((oid = oidset_iter_next(&iter))) {
> +		struct object *obj = lookup_object(repo, oid);
> +
> +		if (!obj || !(obj->flags & SEEN))
> +			add_pending_oid(&revs, NULL, oid, 0);
> +	}
> +	if (revs.pending.nr) {
> +		if (prepare_revision_walk(&revs))
> +			die(_("revision walk setup failed"));
> +		traverse_commit_list(&revs,
> +				     show_commit_pack_hint,
> +				     show_object_pack_hint,
> +				     &mode);
> +	}
> +	oidset_clear(&ctx.extra_roots);
> +
>  	release_revisions(&revs);

BTW, is it safe to prepare_revision_walk() twice on the same rev_info? I
could believe it works, but I could also believe that there are hidden
corner cases, as I don't think it was ever really intended to work this
way.

Maybe OK for the vanilla set of options we are using here (as opposed to
taking arbitrary options from the user). The rev_info is created locally
in this function, though, so I guess if we wanted to be double-plus sure
we could release and reinit the struct.

-Peff