From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 12 Jan 2026 09:37:17 GMT
Subject: Re: [PATCH 1/5] builtin/pack-objects: exclude promisor objects with "--stdin-packs"
Message-ID: <aWTAzR4H3XuAlJJn@pks.im>
In-Reply-To: <aWGP/Lp2Eo03F7vN@nand.local>

On Fri, Jan 09, 2026 at 06:32:12PM -0500, Taylor Blau wrote:
> On Mon, Jan 05, 2026 at 02:16:41PM +0100, Patrick Steinhardt wrote:
> >   - "--stdin-packs=follow" does the same as the first flag, but it also
> >     asks us to include all objects transitively reachable from any
> >     object in the packs we are about to repack. This is done by doing
> >     the revision walk mentioned further up. Luckily, fixing this case is
> >     trivial: we only need to modify the revision walk to also set the
> >     `exclude_promisor_objects` field.
> 
> Hmm. I'm not totally sure if I'm following why we handle this case
> separately. Could you elaborate?

You mean why we handle "--stdin-packs" and "--stdin-packs=follow"
separately?

The thing is that we don't really need to care about the case where we
want to exclude promisor objects with "--stdin-packs" because we don't
perform any object walk at all. We'll only merge objects part of packs
that have been passed to us via stdin. Consequently, you can say that a
request where the user asks us to exclude promisor objects while at the
same time asking us to pack a promisor pack is self-contracdicting, as
they could have just as well left out the promisor pack from the
request to achieve the same.

There's two approaches here:

  - We can simply die when seeing such a malformed request. This is
    exactly what we do with this patch, and that cannot be a regression
    because we already died beforehand. We strictly expand the set of
    supported cases where we pack objects.

  - We can honor this, but exclude promisor packs altogether. This is a
    feasible thing to do, but now we also have to care about the case
    where all passed packs are promisor packs. Also, the result would
    arguably be _more_ surprising if we exclude packing some packs that
    the user has passed to us.

In "--stdin-packs=follow" we _also_ do the same as above and die in case
we're passed a promisor pack directly. But in addition to that, we also
need to pay attention to the rev-walk we do, because "follow" asks us to
include objects reachable from any of the packs. So in case any such
object is a promisor object we need to exclude it.

In the context of `git repack --geometric=2` we won't care about the
first case: we won't ever ask git-pack-objects(1) to include a promisor
pack in the normal geometric sequence. We do care about the second case
though as git-repack(1) may end up passing "--stdin-packs=follow" when
"repack.midxMustContainCruft=false".

> > diff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh
> > index 4a8df5a389..cd949025b9 100755
> > --- a/t/t5331-pack-objects-stdin.sh
> > +++ b/t/t5331-pack-objects-stdin.sh
> > @@ -319,6 +319,45 @@ test_expect_success '--stdin-packs=follow walks into unknown packs' '
> >  	)
> >  '
> >
> > +test_expect_success '--stdin-packs with promisors' '
> > +	test_when_finished "rm -fr repo" &&
> > +	git init repo &&
> > +	(
> > +		cd repo &&
> > +		git config set maintenance.auto false &&
> > +		git remote add promisor garbage &&
> > +		git config set remote.promisor.promisor true &&
> > +
> > +		for c in A B C D
> > +		do
> > +			echo "$c" >file &&
> > +			git add file &&
> > +			git commit --message "$c" &&
> > +			git tag "$c" || return 1
> 
> Unless these changes all have to live in the same file, could this
> instead be written as:
> 
>     for c in A B C D
>     do
>         test_commit "$c" || return 1
>     done &&
>     # ...
> 
> ?

We unfortunately can't. The problem is that any object reachable from a
promisor object may be labelled as a promisor object. So if we had a
tree that makes all blobs reachable we'd treat all of them as promised
blobs.

It's quite confusing overall.

Patrick