Re: [PATCH 1/5] builtin/pack-objects: exclude promisor objects with "--stdin-packs"
From: Taylor Blau <hidden>
Date: 2026-01-09 23:32:14
On Mon, Jan 05, 2026 at 02:16:41PM +0100, Patrick Steinhardt wrote:
It is currently not possible to combine "--exclude-promisor-objects"
with "--stdin-packs" because both flags want to set up a revision walk
to enumerate the objects to pack. In a subsequent commit though we want
to extend geometric repacks to support promisor objects, and for that we
need to handle the combination of both flags.
There are two cases we have to think about here:
- "--stdin-packs" asks us to pack exactly the objects part of the
specified packfiles. It is somewhat questionable what to do in the
case where the user asks us to exclude promisor objects, but at the
same time explicitly passes a promisor pack to us. For now, we
simply abort the request as it is self-contradicting. As we have
also been dying before this commit there is no regression here.
I was wondering whether or not this is the case, because we don't have
an explicit `die()` here or a incompatible pair of options declared. But
it does die(), although the message is somewhat confusing:
$ git.compile pack-objects --stdin-packs --exclude-promisor-objects --stdout >/dev/null
fatal: cannot use internal rev list with --stdin-packs
;-).
- "--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?
Note that we do not support the "--exclude-promisor-objects-best-effort" flag for now as we don't need it to support geometric repacking with promisor objects.
I didn't know we had such an option in the first place, but it looks like it behaves similarly wrt. its incompatibility with `--stdin-packs`.
quoted hunk ↗ jump to hunk
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
I'll hold off on commenting on the code to give us a chance to discuss the above.
quoted hunk ↗ jump to hunk
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 &&
# ...
?
Thanks,
Taylor