From: Justin Tobler Date: Wed, 10 Dec 2025 19:31:44 GMT Subject: Re: [PATCH 1/3] builtin/repack: fix geometric repacks with promisor remotes Message-ID: In-Reply-To: <20251205-odb-related-fixes-v1-1-ef4250abb584@pks.im> On 25/12/05 09:19AM, Patrick Steinhardt wrote: > When repacking a repository with promisor remotes git-repack(1) knows to > pass "--exclude-promisor-objects" to git-pack-objects(1). This option > ensures that the new pack will not contain any promised object that do > not yet exist locally. > > This command line option is incompatible with "--stdin-packs": the > latter option enables the rev-walk-based machinery to figure out which > objects to add to the pack, whereas the former tells git-pack-objects(1) > to merge all packs passed via stdin into one large pack. As we do not > know to filter those packs via the passed-in revisions it is clear that > at the current point in time nothing sensible comes out of combining > these two options. Is the latter/former part here backwards? I find it a bit confusing to read. As I understand it, --stdin-packs expects the packfiles provided as input to dictate the source of objects when repacking. With --exclude-promisor-objects, we walk the object graph normally, but exclude promisor objects. Thus combining these two options would create a conflict regarding which objects are included. > But there is one case where git-repack(1) decides to pass both options: > when performing a geometric repack we always pass "--stdin-packs" to > identify the packs that should be merged. So if one performs a geometric > repack in a partial clone we'll end up with both options, and that > causes the repack to fail. > > Fix this issue by never passing "--exclude-promisor-objects" when we > have a geometric split factor. We don't need the option anyway when > doing a geometric repack as we will only ever pack loose objects or > merge multiple packs. And neither of those cases can yield a promisor > object. I'm not sure I fully understand why --exclude-promisor-objects would not be needed for geometric repacks. To clarify, do geometric repacks already exclude promisor packfiles when merging? If so, then this change makes sense. > Signed-off-by: Patrick Steinhardt > --- > builtin/repack.c | 5 +++-- > t/t7703-repack-geometric.sh | 26 ++++++++++++++++++++++++++ > 2 files changed, 29 insertions(+), 2 deletions(-) > > diff --git a/builtin/repack.c b/builtin/repack.c > index d9012141f6..4621eed3e6 100644 > --- a/builtin/repack.c > +++ b/builtin/repack.c > @@ -294,9 +294,10 @@ int cmd_repack(int argc, > strvec_push(&cmd.args, "--all"); > strvec_push(&cmd.args, "--reflog"); > strvec_push(&cmd.args, "--indexed-objects"); > + > + if (repo_has_promisor_remote(repo)) > + strvec_push(&cmd.args, "--exclude-promisor-objects"); > } > - if (repo_has_promisor_remote(repo)) > - strvec_push(&cmd.args, "--exclude-promisor-objects"); Ok, now the --exclude-promisor-objects flag is only added when there is a promisor remote and geometric repacking is not used. > if (!write_midx) { > if (write_bitmaps > 0) > strvec_push(&cmd.args, "--write-bitmap-index"); > diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh > index 9fc1626fbf..6d2c712bff 100755 > --- a/t/t7703-repack-geometric.sh > +++ b/t/t7703-repack-geometric.sh > @@ -445,4 +445,30 @@ test_expect_success '--geometric -l disables writing bitmaps with non-local pack > test_path_is_file member/.git/objects/pack/multi-pack-index-*.bitmap > ' > > +test_expect_success '--geometric works with promisor packs' ' > + test_when_finished "rm -fr remote local" && > + > + git init remote && > + test_commit -C remote first file first && > + test_commit -C remote second file second && > + git -C remote config set uploadpack.allowfilter 1 && > + git -C remote config set uploadpack.allowanysha1inwant 1 && > + git -C remote repack -Ad && > + > + git clone --filter=blob:none file://"$(pwd)"/remote local && > + git -C local rev-list --objects --missing=print HEAD >missing-objects && > + test_grep "^?" missing-objects && > + > + # Assert that promisor packs are left alone and that we still manage to > + # create new geometric packs. > + ls local/.git/objects/pack/*.promisor >promisors-before && > + ls local/.git/objects/pack/*.pack >packs-before && > + test_commit -C local change && > + git -C local repack --geometric=2 && > + ls local/.git/objects/pack/*.promisor >promisors-after && > + ls local/.git/objects/pack/*.pack >packs-after && > + ! cmp packs-before packs-after && > + test_cmp promisors-before promisors-after Ok, so it does seem to be the case that promisor packfiles are ignored when performing a geometric repack. Naive question: does this mean there are scenarios where a repository could accumulate many promisor packfiles, but never repack them? -Justin