From: Taylor Blau <ttaylorr@openai.com>
Date: Sat, 03 Oct 2026 00:55:32 GMT
Subject: Re: [PATCH v2 2/8] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'
Message-ID: <asBShFkQRWJX4RQU@com-79390>
In-Reply-To: <20261002231336.GB834759@coredump.intra.peff.net>

On Fri, Oct 02, 2026 at 07:13:36PM -0400, Jeff King wrote:
> 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.

Yeah, me either. I'm happy to change things around if you feel strongly.

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

It seems to work in practice. From reading through and thinking about it
I couldn't find any obvious issues.

Just as well, there are a couple of spots that I was able to find that
already call `prepare_revision_walk()` more than once:

  * In builtin/pack-objects.c::get_object_list() (with the exception of
    '--path-walk') we call `prepare_revision_walk()` twice when
    exploding unreachable objects as loose.

  * In reachable.c::mark_reachable_objects(), we also call the
    `prepare_revision_walk()` function twice when given a timestamp via
    `mark_recent`.

This all works since `revs.pending` is emptied by the first revwalk. But
it is under-documented, so callers relying on this behavior may be
surprised if/when it changes. Probably good #leftoverbits.

Thanks,
Taylor