Re: [PATCH 1/3] builtin/repack: fix geometric repacks with promisor remotes
From: Justin Tobler <hidden>
Date: 2025-12-10 19:31:54
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.
quoted hunk ↗ jump to hunk
Signed-off-by: Patrick Steinhardt <redacted> --- 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.
quoted hunk ↗ jump to hunk
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