Re: [PATCH 1/5] builtin/pack-objects: exclude promisor objects with "--stdin-packs"

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:

quoted

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

quoted

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