{"thread":{"id":"64723","subject":"[PATCH 0/5] builtin/repack: make geometric repacking compatible with promisors","startedAt":"2026-01-05T13:16:51Z","lastAt":"2026-01-14T15:26:53Z","messageCount":11,"participants":["Patrick Steinhardt","Taylor Blau","Toon Claes"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"533036","messageId":"20260105-pks-geometric-repack-with-promisors-v1-0-c4660573437e@pks.im","threadId":"64723","inReplyTo":null,"subject":"[PATCH 0/5] builtin/repack: make geometric repacking compatible with promisors","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-05T13:16:40Z","receivedAt":"2026-01-05T13:16:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nI recently noticed that geometric repacking is incompatible with\npromisor remotes. This is because we invoke git-pack-objects(1) with\nboth \"--stdin-packs\" and \"--exclude-promisor-objects\", and those flags\nare mutually exclusive. Next to us dying though, we also don't have any\nlogic to mark merged packs as promisors in case any of the source packs\nwas a promisor.\n\nThis patch series fixes this by making these flags work with one another\nand by introducing special handling for promisor packs during geometric\nrepacks.\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (5):\n      builtin/pack-objects: exclude promisor objects with \"--stdin-packs\"\n      repack-geometry: extract function to compute repacking split\n      repack-promisor: extract function to finalize repacking\n      repack-promisor: extract function to remove redundant packs\n      builtin/repack: handle promisor packs with geometric repacking\n\n builtin/pack-objects.c        | 14 +++++--\n builtin/repack.c              |  3 ++\n repack-geometry.c             | 89 ++++++++++++++++++++++++++-------------\n repack-promisor.c             | 97 ++++++++++++++++++++++++++++++-------------\n repack.h                      | 10 +++++\n t/t5331-pack-objects-stdin.sh | 39 +++++++++++++++++\n t/t7703-repack-geometric.sh   | 61 +++++++++++++++++++++++++++\n 7 files changed, 250 insertions(+), 63 deletions(-)\n\n\n---\nbase-commit: 68cb7f9e92a5d8e9824f5b52ac3d0a9d8f653dbe\nchange-id: 20260105-pks-geometric-repack-with-promisors-2f948d6db774\n\n"},{"id":"533037","messageId":"20260105-pks-geometric-repack-with-promisors-v1-1-c4660573437e@pks.im","threadId":"64723","inReplyTo":"20260105-pks-geometric-repack-with-promisors-v1-0-c4660573437e@pks.im","subject":"[PATCH 1/5] builtin/pack-objects: exclude promisor objects with \"--stdin-packs\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-05T13:16:41Z","receivedAt":"2026-01-05T13:16:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It is currently not possible to combine \"--exclude-promisor-objects\"\nwith \"--stdin-packs\" because both flags want to set up a revision walk\nto enumerate the objects to pack. In a subsequent commit though we want\nto extend geometric repacks to support promisor objects, and for that we\nneed to handle the combination of both flags.\n\nThere are two cases we have to think about here:\n\n  - \"--stdin-packs\" asks us to pack exactly the objects part of the\n    specified packfiles. It is somewhat questionable what to do in the\n    case where the user asks us to exclude promisor objects, but at the\n    same time explicitly passes a promisor pack to us. For now, we\n    simply abort the request as it is self-contradicting. As we have\n    also been dying before this commit there is no regression here.\n\n  - \"--stdin-packs=follow\" does the same as the first flag, but it also\n    asks us to include all objects transitively reachable from any\n    object in the packs we are about to repack. This is done by doing\n    the revision walk mentioned further up. Luckily, fixing this case is\n    trivial: we only need to modify the revision walk to also set the\n    `exclude_promisor_objects` field.\n\nNote that we do not support the \"--exclude-promisor-objects-best-effort\"\nflag for now as we don't need it to support geometric repacking with\npromisor objects.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/pack-objects.c        | 14 +++++++++++---\n t/t5331-pack-objects-stdin.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 50 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 1ce8d6ee21..560b3228aa 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3857,8 +3857,11 @@ static void read_packs_list_from_stdin(struct rev_info *revs)\n \trepo_for_each_pack(the_repository, p) {\n \t\tconst char *pack_name = pack_basename(p);\n \n-\t\tif ((item = string_list_lookup(&include_packs, pack_name)))\n+\t\tif ((item = string_list_lookup(&include_packs, pack_name))) {\n+\t\t\tif (exclude_promisor_objects && p->pack_promisor)\n+\t\t\t\tdie(_(\"packfile %s is a promisor but --exclude-promisor-objects was given\"), p->pack_name);\n \t\t\titem->util = p;\n+\t\t}\n \t\tif ((item = string_list_lookup(&exclude_packs, pack_name)))\n \t\t\titem->util = p;\n \t}\n@@ -3936,6 +3939,7 @@ static void read_stdin_packs(enum stdin_packs_mode mode, int rev_list_unpacked)\n \trevs.tree_objects = 1;\n \trevs.tag_objects = 1;\n \trevs.ignore_missing_links = 1;\n+\trevs.exclude_promisor_objects = exclude_promisor_objects;\n \n \t/* avoids adding objects in excluded packs */\n \tignore_packed_keep_in_core = 1;\n@@ -5092,9 +5096,13 @@ int cmd_pack_objects(int argc,\n \t\t\t\t  exclude_promisor_objects_best_effort,\n \t\t\t\t  \"--exclude-promisor-objects-best-effort\");\n \tif (exclude_promisor_objects) {\n-\t\tuse_internal_rev_list = 1;\n \t\tfetch_if_missing = 0;\n-\t\tstrvec_push(&rp, \"--exclude-promisor-objects\");\n+\n+\t\t/* --stdin-packs handles promisor objects separately. */\n+\t\tif (!stdin_packs) {\n+\t\t\tuse_internal_rev_list = 1;\n+\t\t\tstrvec_push(&rp, \"--exclude-promisor-objects\");\n+\t\t}\n \t} else if (exclude_promisor_objects_best_effort) {\n \t\tuse_internal_rev_list = 1;\n \t\tfetch_if_missing = 0;\ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nindex 4a8df5a389..cd949025b9 100755\n--- a/t/t5331-pack-objects-stdin.sh\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -319,6 +319,45 @@ test_expect_success '--stdin-packs=follow walks into unknown packs' '\n \t)\n '\n \n+test_expect_success '--stdin-packs with promisors' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit config set maintenance.auto false &&\n+\t\tgit remote add promisor garbage &&\n+\t\tgit config set remote.promisor.promisor true &&\n+\n+\t\tfor c in A B C D\n+\t\tdo\n+\t\t\techo \"$c\" >file &&\n+\t\t\tgit add file &&\n+\t\t\tgit commit --message \"$c\" &&\n+\t\t\tgit tag \"$c\" || return 1\n+\t\tdone &&\n+\n+\t\tA=\"$(echo A | git pack-objects --revs $packdir/pack)\" &&\n+\t\tB=\"$(echo A..B | git pack-objects --revs $packdir/pack --filter=blob:none)\" &&\n+\t\tC=\"$(echo B..C | git pack-objects --revs $packdir/pack)\" &&\n+\t\tD=\"$(echo C..D | git pack-objects --revs $packdir/pack)\" &&\n+\t\ttouch $packdir/pack-$B.promisor &&\n+\n+\t\ttest_must_fail git pack-objects --stdin-packs --exclude-promisor-objects pack- 2>err <<-EOF &&\n+\t\t\tpack-$B.pack\n+\t\tEOF\n+\t\ttest_grep \"is a promisor but --exclude-promisor-objects was given\" err &&\n+\n+\t\tPACK=$(git pack-objects --stdin-packs=follow --exclude-promisor-objects $packdir/pack <<-EOF\n+\t\t\tpack-$D.pack\n+\t\t\tEOF\n+\t\t) &&\n+\t\tobjects_in_packs $C $D >expect &&\n+\t\tobjects_in_packs $PACK >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\trm -f $packdir/pack-$PACK.*\n+\t)\n+'\n+\n stdin_packs__follow_with_only () {\n \trm -fr stdin_packs__follow_with_only &&\n \tgit init stdin_packs__follow_with_only &&\n\n-- \n2.52.0.508.g883dcfc63e.dirty\n\n"},{"id":"533038","messageId":"20260105-pks-geometric-repack-with-promisors-v1-2-c4660573437e@pks.im","threadId":"64723","inReplyTo":"20260105-pks-geometric-repack-with-promisors-v1-0-c4660573437e@pks.im","subject":"[PATCH 2/5] repack-geometry: extract function to compute repacking split","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-05T13:16:42Z","receivedAt":"2026-01-05T13:16:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're about to add a second caller that wants to compute the repacking\nsplit for a set of packfiles. Split out the function that computes this\nsplit to prepare for that.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n repack-geometry.c | 39 +++++++++++++++++++++------------------\n 1 file changed, 21 insertions(+), 18 deletions(-)\n\ndiff --git a/repack-geometry.c b/repack-geometry.c\nindex b3e32cd07e..17e6652a91 100644\n--- a/repack-geometry.c\n+++ b/repack-geometry.c\n@@ -78,33 +78,32 @@ void pack_geometry_init(struct pack_geometry *geometry,\n \tstrbuf_release(&buf);\n }\n \n-void pack_geometry_split(struct pack_geometry *geometry)\n+static uint32_t compute_pack_geometry_split(struct packed_git **pack, size_t pack_nr,\n+\t\t\t\t\t    int split_factor)\n {\n \tuint32_t i;\n \tuint32_t split;\n \toff_t total_size = 0;\n \n-\tif (!geometry->pack_nr) {\n-\t\tgeometry->split = geometry->pack_nr;\n-\t\treturn;\n-\t}\n+\tif (!pack_nr)\n+\t\treturn 0;\n \n \t/*\n \t * First, count the number of packs (in descending order of size) which\n \t * already form a geometric progression.\n \t */\n-\tfor (i = geometry->pack_nr - 1; i > 0; i--) {\n-\t\tstruct packed_git *ours = geometry->pack[i];\n-\t\tstruct packed_git *prev = geometry->pack[i - 1];\n+\tfor (i = pack_nr - 1; i > 0; i--) {\n+\t\tstruct packed_git *ours = pack[i];\n+\t\tstruct packed_git *prev = pack[i - 1];\n \n-\t\tif (unsigned_mult_overflows(geometry->split_factor,\n+\t\tif (unsigned_mult_overflows(split_factor,\n \t\t\t\t\t    pack_geometry_weight(prev)))\n \t\t\tdie(_(\"pack %s too large to consider in geometric \"\n \t\t\t      \"progression\"),\n \t\t\t    prev->pack_name);\n \n \t\tif (pack_geometry_weight(ours) <\n-\t\t    geometry->split_factor * pack_geometry_weight(prev))\n+\t\t    split_factor * pack_geometry_weight(prev))\n \t\t\tbreak;\n \t}\n \n@@ -130,21 +129,19 @@ void pack_geometry_split(struct pack_geometry *geometry)\n \t * the geometric progression.\n \t */\n \tfor (i = 0; i < split; i++) {\n-\t\tstruct packed_git *p = geometry->pack[i];\n+\t\tstruct packed_git *p = pack[i];\n \n \t\tif (unsigned_add_overflows(total_size, pack_geometry_weight(p)))\n \t\t\tdie(_(\"pack %s too large to roll up\"), p->pack_name);\n \t\ttotal_size += pack_geometry_weight(p);\n \t}\n-\tfor (i = split; i < geometry->pack_nr; i++) {\n-\t\tstruct packed_git *ours = geometry->pack[i];\n+\tfor (i = split; i < pack_nr; i++) {\n+\t\tstruct packed_git *ours = pack[i];\n \n-\t\tif (unsigned_mult_overflows(geometry->split_factor,\n-\t\t\t\t\t    total_size))\n+\t\tif (unsigned_mult_overflows(split_factor, total_size))\n \t\t\tdie(_(\"pack %s too large to roll up\"), ours->pack_name);\n \n-\t\tif (pack_geometry_weight(ours) <\n-\t\t    geometry->split_factor * total_size) {\n+\t\tif (pack_geometry_weight(ours) < split_factor * total_size) {\n \t\t\tif (unsigned_add_overflows(total_size,\n \t\t\t\t\t\t   pack_geometry_weight(ours)))\n \t\t\t\tdie(_(\"pack %s too large to roll up\"),\n@@ -156,7 +153,13 @@ void pack_geometry_split(struct pack_geometry *geometry)\n \t\t\tbreak;\n \t}\n \n-\tgeometry->split = split;\n+\treturn split;\n+}\n+\n+void pack_geometry_split(struct pack_geometry *geometry)\n+{\n+\tgeometry->split = compute_pack_geometry_split(geometry->pack, geometry->pack_nr,\n+\t\t\t\t\t\t      geometry->split_factor);\n }\n \n struct packed_git *pack_geometry_preferred_pack(struct pack_geometry *geometry)\n\n-- \n2.52.0.508.g883dcfc63e.dirty\n\n"},{"id":"533040","messageId":"20260105-pks-geometric-repack-with-promisors-v1-3-c4660573437e@pks.im","threadId":"64723","inReplyTo":"20260105-pks-geometric-repack-with-promisors-v1-0-c4660573437e@pks.im","subject":"[PATCH 3/5] repack-promisor: extract function to finalize repacking","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-05T13:16:43Z","receivedAt":"2026-01-05T13:16:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're about to add a second caller that wants to finalize repacking of\npromisor objects. Split out the function which does this to prepare for\nthat.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n repack-promisor.c | 69 +++++++++++++++++++++++++++++++------------------------\n 1 file changed, 39 insertions(+), 30 deletions(-)\n\ndiff --git a/repack-promisor.c b/repack-promisor.c\nindex ee6e0669f6..125038d92e 100644\n--- a/repack-promisor.c\n+++ b/repack-promisor.c\n@@ -34,39 +34,17 @@ static int write_oid(const struct object_id *oid,\n \treturn 0;\n }\n \n-void repack_promisor_objects(struct repository *repo,\n-\t\t\t     const struct pack_objects_args *args,\n-\t\t\t     struct string_list *names, const char *packtmp)\n+static void finish_repacking_promisor_objects(struct repository *repo,\n+\t\t\t\t\t      struct child_process *cmd,\n+\t\t\t\t\t      struct string_list *names,\n+\t\t\t\t\t      const char *packtmp)\n {\n-\tstruct write_oid_context ctx;\n-\tstruct child_process cmd = CHILD_PROCESS_INIT;\n-\tFILE *out;\n \tstruct strbuf line = STRBUF_INIT;\n+\tFILE *out;\n \n-\tprepare_pack_objects(&cmd, args, packtmp);\n-\tcmd.in = -1;\n-\n-\t/*\n-\t * NEEDSWORK: Giving pack-objects only the OIDs without any ordering\n-\t * hints may result in suboptimal deltas in the resulting pack. See if\n-\t * the OIDs can be sent with fake paths such that pack-objects can use a\n-\t * {type -> existing pack order} ordering when computing deltas instead\n-\t * of a {type -> size} ordering, which may produce better deltas.\n-\t */\n-\tctx.cmd = &cmd;\n-\tctx.algop = repo->hash_algo;\n-\tfor_each_packed_object(repo, write_oid, &ctx,\n-\t\t\t       FOR_EACH_OBJECT_PROMISOR_ONLY);\n-\n-\tif (cmd.in == -1) {\n-\t\t/* No packed objects; cmd was never started */\n-\t\tchild_process_clear(&cmd);\n-\t\treturn;\n-\t}\n-\n-\tclose(cmd.in);\n+\tclose(cmd->in);\n \n-\tout = xfdopen(cmd.out, \"r\");\n+\tout = xfdopen(cmd->out, \"r\");\n \twhile (strbuf_getline_lf(&line, out) != EOF) {\n \t\tstruct string_list_item *item;\n \t\tchar *promisor_name;\n@@ -96,7 +74,38 @@ void repack_promisor_objects(struct repository *repo,\n \t}\n \n \tfclose(out);\n-\tif (finish_command(&cmd))\n+\tif (finish_command(cmd))\n \t\tdie(_(\"could not finish pack-objects to repack promisor objects\"));\n \tstrbuf_release(&line);\n }\n+\n+void repack_promisor_objects(struct repository *repo,\n+\t\t\t     const struct pack_objects_args *args,\n+\t\t\t     struct string_list *names, const char *packtmp)\n+{\n+\tstruct write_oid_context ctx;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\tprepare_pack_objects(&cmd, args, packtmp);\n+\tcmd.in = -1;\n+\n+\t/*\n+\t * NEEDSWORK: Giving pack-objects only the OIDs without any ordering\n+\t * hints may result in suboptimal deltas in the resulting pack. See if\n+\t * the OIDs can be sent with fake paths such that pack-objects can use a\n+\t * {type -> existing pack order} ordering when computing deltas instead\n+\t * of a {type -> size} ordering, which may produce better deltas.\n+\t */\n+\tctx.cmd = &cmd;\n+\tctx.algop = repo->hash_algo;\n+\tfor_each_packed_object(repo, write_oid, &ctx,\n+\t\t\t       FOR_EACH_OBJECT_PROMISOR_ONLY);\n+\n+\tif (cmd.in == -1) {\n+\t\t/* No packed objects; cmd was never started */\n+\t\tchild_process_clear(&cmd);\n+\t\treturn;\n+\t}\n+\n+\tfinish_repacking_promisor_objects(repo, &cmd, names, packtmp);\n+}\n\n-- \n2.52.0.508.g883dcfc63e.dirty\n\n"},{"id":"533041","messageId":"20260105-pks-geometric-repack-with-promisors-v1-4-c4660573437e@pks.im","threadId":"64723","inReplyTo":"20260105-pks-geometric-repack-with-promisors-v1-0-c4660573437e@pks.im","subject":"[PATCH 4/5] repack-promisor: extract function to remove redundant packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-05T13:16:44Z","receivedAt":"2026-01-05T13:17:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're about to add a second caller that wants to remove redundant packs\nafter a geometric repack. Split out the function which does this to\nprepare for that.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n repack-geometry.c | 22 ++++++++++++++++------\n 1 file changed, 16 insertions(+), 6 deletions(-)\n\ndiff --git a/repack-geometry.c b/repack-geometry.c\nindex 17e6652a91..0daf545a81 100644\n--- a/repack-geometry.c\n+++ b/repack-geometry.c\n@@ -197,17 +197,18 @@ struct packed_git *pack_geometry_preferred_pack(struct pack_geometry *geometry)\n \treturn NULL;\n }\n \n-void pack_geometry_remove_redundant(struct pack_geometry *geometry,\n-\t\t\t\t    struct string_list *names,\n-\t\t\t\t    struct existing_packs *existing,\n-\t\t\t\t    const char *packdir)\n+static void remove_redundant_packs(struct packed_git **pack,\n+\t\t\t\t   uint32_t pack_nr,\n+\t\t\t\t   struct string_list *names,\n+\t\t\t\t   struct existing_packs *existing,\n+\t\t\t\t   const char *packdir)\n {\n \tconst struct git_hash_algo *algop = existing->repo->hash_algo;\n \tstruct strbuf buf = STRBUF_INIT;\n \tuint32_t i;\n \n-\tfor (i = 0; i < geometry->split; i++) {\n-\t\tstruct packed_git *p = geometry->pack[i];\n+\tfor (i = 0; i < pack_nr; i++) {\n+\t\tstruct packed_git *p = pack[i];\n \t\tif (string_list_has_string(names, hash_to_hex_algop(p->hash,\n \t\t\t\t\t\t\t\t    algop)))\n \t\t\tcontinue;\n@@ -226,6 +227,15 @@ void pack_geometry_remove_redundant(struct pack_geometry *geometry,\n \tstrbuf_release(&buf);\n }\n \n+void pack_geometry_remove_redundant(struct pack_geometry *geometry,\n+\t\t\t\t    struct string_list *names,\n+\t\t\t\t    struct existing_packs *existing,\n+\t\t\t\t    const char *packdir)\n+{\n+\tremove_redundant_packs(geometry->pack, geometry->split,\n+\t\t\t       names, existing, packdir);\n+}\n+\n void pack_geometry_release(struct pack_geometry *geometry)\n {\n \tif (!geometry)\n\n-- \n2.52.0.508.g883dcfc63e.dirty\n\n"},{"id":"533042","messageId":"20260105-pks-geometric-repack-with-promisors-v1-5-c4660573437e@pks.im","threadId":"64723","inReplyTo":"20260105-pks-geometric-repack-with-promisors-v1-0-c4660573437e@pks.im","subject":"[PATCH 5/5] builtin/repack: handle promisor packs with geometric repacking","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-05T13:16:45Z","receivedAt":"2026-01-05T13:17:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When performing a fetch with an object filter, we mark the resulting\npackfile as a promisor pack. An object part of such a pack may miss any\nof its referenced objects, and Git knows to handle this case by fetching\nany such missing objects from the promisor remote.\n\nThe \"promisor\" property needs to be retained going forward. So every\ntime we pack a promisor object, the resulting pack must be marked as a\npromisor pack. git-repack(1) does this already: when a repository has a\npromisor remote, it knows to pass \"--exclude-promisor-objects\" to the\ngit-pack-objects(1) child process. Promisor packs are written separately\nwhen doing an all-into-one repack via `repack_promisor_objects()`.\n\nBut we don't support promisor objects when doing a geometric repack yet.\nPromisor packs do not get any special treatment there, as we simply\nmerge promisor and non-promisor packs. The resulting pack is not even\nmarked as a promisor pack, which essentially corrupts the repository.\n\nThis corruption couldn't happen in the real world though: we pass both\n\"--exclude-promisor-objects\" and \"--stdin-packs\" to git-pack-objects(1)\nif a repository has a promisor remote, but as those options are mutually\nexclusive we always end up dying. And while we made those flags\ncompatible with one another in a preceding commit, we still end up dying\nin case git-pack-objects(1) is asked to repack a promisor pack.\n\nThere's multiple ways to fix this:\n\n  - We can exclude promisor packs from the geometric progression\n    altogether. This would have the consequence that we never repack\n    promisor packs at all. But in a partial clone it is quite likely\n    that the user generates a bunch of promisor packs over time, as\n    every backfill fetch would create another one. So this doesn't\n    really feel like a sensible option.\n\n  - We can adapt git-pack-objects(1) to support repacking promisor packs\n    and include them in the normal geometric progression. But this would\n    mean that the set of promisor objects expands over time as the packs\n    are merged with normal packs.\n\n  - We can use a separate geometric progression to repack promisor\n    packs.\n\nThe first two options both have significant downsides, so they aren't\nreally feasible. But the third option fixes both of these downsides: we\nmake sure that promisor packs get merged, and at the same time we never\nexpand the set of promisor objects beyond the set of objects that are\nalready marked as promisor objects.\n\nImplement this strategy so that geometric repacking works in partial\nclones.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/repack.c            |  3 +++\n repack-geometry.c           | 28 ++++++++++++++++-----\n repack-promisor.c           | 28 +++++++++++++++++++++\n repack.h                    | 10 ++++++++\n t/t7703-repack-geometric.sh | 61 +++++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 124 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex d9012141f6..f6bb04bef7 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -332,6 +332,9 @@ int cmd_repack(int argc,\n \t\t    !(pack_everything & PACK_CRUFT))\n \t\t\tstrvec_push(&cmd.args, \"--pack-loose-unreachable\");\n \t} else if (geometry.split_factor) {\n+\t\tpack_geometry_repack_promisors(repo, &po_args, &geometry,\n+\t\t\t\t\t       &names, packtmp);\n+\n \t\tif (midx_must_contain_cruft)\n \t\t\tstrvec_push(&cmd.args, \"--stdin-packs\");\n \t\telse\ndiff --git a/repack-geometry.c b/repack-geometry.c\nindex 0daf545a81..7cebd0cb45 100644\n--- a/repack-geometry.c\n+++ b/repack-geometry.c\n@@ -66,15 +66,25 @@ void pack_geometry_init(struct pack_geometry *geometry,\n \t\tif (p->is_cruft)\n \t\t\tcontinue;\n \n-\t\tALLOC_GROW(geometry->pack,\n-\t\t\t   geometry->pack_nr + 1,\n-\t\t\t   geometry->pack_alloc);\n-\n-\t\tgeometry->pack[geometry->pack_nr] = p;\n-\t\tgeometry->pack_nr++;\n+\t\tif (p->pack_promisor) {\n+\t\t\tALLOC_GROW(geometry->promisor_pack,\n+\t\t\t\t   geometry->promisor_pack_nr + 1,\n+\t\t\t\t   geometry->promisor_pack_alloc);\n+\n+\t\t\tgeometry->promisor_pack[geometry->promisor_pack_nr] = p;\n+\t\t\tgeometry->promisor_pack_nr++;\n+\t\t} else {\n+\t\t\tALLOC_GROW(geometry->pack,\n+\t\t\t\t   geometry->pack_nr + 1,\n+\t\t\t\t   geometry->pack_alloc);\n+\n+\t\t\tgeometry->pack[geometry->pack_nr] = p;\n+\t\t\tgeometry->pack_nr++;\n+\t\t}\n \t}\n \n \tQSORT(geometry->pack, geometry->pack_nr, pack_geometry_cmp);\n+\tQSORT(geometry->promisor_pack, geometry->promisor_pack_nr, pack_geometry_cmp);\n \tstrbuf_release(&buf);\n }\n \n@@ -160,6 +170,9 @@ void pack_geometry_split(struct pack_geometry *geometry)\n {\n \tgeometry->split = compute_pack_geometry_split(geometry->pack, geometry->pack_nr,\n \t\t\t\t\t\t      geometry->split_factor);\n+\tgeometry->promisor_split = compute_pack_geometry_split(geometry->promisor_pack,\n+\t\t\t\t\t\t\t       geometry->promisor_pack_nr,\n+\t\t\t\t\t\t\t       geometry->split_factor);\n }\n \n struct packed_git *pack_geometry_preferred_pack(struct pack_geometry *geometry)\n@@ -234,6 +247,8 @@ void pack_geometry_remove_redundant(struct pack_geometry *geometry,\n {\n \tremove_redundant_packs(geometry->pack, geometry->split,\n \t\t\t       names, existing, packdir);\n+\tremove_redundant_packs(geometry->promisor_pack, geometry->promisor_split,\n+\t\t\t       names, existing, packdir);\n }\n \n void pack_geometry_release(struct pack_geometry *geometry)\n@@ -242,4 +257,5 @@ void pack_geometry_release(struct pack_geometry *geometry)\n \t\treturn;\n \n \tfree(geometry->pack);\n+\tfree(geometry->promisor_pack);\n }\ndiff --git a/repack-promisor.c b/repack-promisor.c\nindex 125038d92e..73af57bce3 100644\n--- a/repack-promisor.c\n+++ b/repack-promisor.c\n@@ -109,3 +109,31 @@ void repack_promisor_objects(struct repository *repo,\n \n \tfinish_repacking_promisor_objects(repo, &cmd, names, packtmp);\n }\n+\n+void pack_geometry_repack_promisors(struct repository *repo,\n+\t\t\t\t    const struct pack_objects_args *args,\n+\t\t\t\t    const struct pack_geometry *geometry,\n+\t\t\t\t    struct string_list *names,\n+\t\t\t\t    const char *packtmp)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tFILE *in;\n+\n+\tif (!geometry->promisor_split)\n+\t\treturn;\n+\n+\tprepare_pack_objects(&cmd, args, packtmp);\n+\tstrvec_push(&cmd.args, \"--stdin-packs\");\n+\tcmd.in = -1;\n+\tif (start_command(&cmd))\n+\t\tdie(_(\"could not start pack-objects to repack promisor packs\"));\n+\n+\tin = xfdopen(cmd.in, \"w\");\n+\tfor (size_t i = 0; i < geometry->promisor_split; i++)\n+\t\tfprintf(in, \"%s\\n\", pack_basename(geometry->promisor_pack[i]));\n+\tfor (size_t i = geometry->promisor_split; i < geometry->promisor_pack_nr; i++)\n+\t\tfprintf(in, \"^%s\\n\", pack_basename(geometry->promisor_pack[i]));\n+\tfclose(in);\n+\n+\tfinish_repacking_promisor_objects(repo, &cmd, names, packtmp);\n+}\ndiff --git a/repack.h b/repack.h\nindex 3a688a12ee..bc9f2e1a5d 100644\n--- a/repack.h\n+++ b/repack.h\n@@ -103,9 +103,19 @@ struct pack_geometry {\n \tuint32_t pack_nr, pack_alloc;\n \tuint32_t split;\n \n+\tstruct packed_git **promisor_pack;\n+\tuint32_t promisor_pack_nr, promisor_pack_alloc;\n+\tuint32_t promisor_split;\n+\n \tint split_factor;\n };\n \n+void pack_geometry_repack_promisors(struct repository *repo,\n+\t\t\t\t    const struct pack_objects_args *args,\n+\t\t\t\t    const struct pack_geometry *geometry,\n+\t\t\t\t    struct string_list *names,\n+\t\t\t\t    const char *packtmp);\n+\n void pack_geometry_init(struct pack_geometry *geometry,\n \t\t\tstruct existing_packs *existing,\n \t\t\tconst struct pack_objects_args *args);\ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 98806cdb6f..04d5d8fc33 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -480,4 +480,65 @@ test_expect_success '--geometric -l disables writing bitmaps with non-local pack\n \ttest_path_is_file member/.git/objects/pack/multi-pack-index-*.bitmap\n '\n \n+write_packfile () {\n+\tNR=\"$1\"\n+\tPREFIX=\"$2\"\n+\n+\tprintf \"blob\\ndata <<EOB\\n$PREFIX %s\\nEOB\\n\" $(test_seq $NR) |\n+\t\tgit fast-import &&\n+\tgit pack-objects --pack-loose-unreachable .git/objects/pack/pack &&\n+\tgit prune-packed\n+}\n+\n+write_promisor_packfile () {\n+\tPACKFILE=$(write_packfile \"$@\") &&\n+\ttouch .git/objects/pack/pack-$PACKFILE.promisor &&\n+\techo \"$PACKFILE\"\n+}\n+\n+test_expect_success 'geometric repack works with promisor packs' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit config set maintenance.auto false &&\n+\t\tgit remote add promisor garbage &&\n+\t\tgit config set remote.promisor.promisor true &&\n+\n+\t\t# Packs A and B need to be merged.\n+\t\tNORMAL_A=$(write_packfile 2 normal-a) &&\n+\t\tNORMAL_B=$(write_packfile 2 normal-b) &&\n+\t\tNORMAL_C=$(write_packfile 14 normal-c) &&\n+\n+\t\t# Packs A, B and C need to be merged.\n+\t\tPROMISOR_A=$(write_promisor_packfile 1 promisor-a) &&\n+\t\tPROMISOR_B=$(write_promisor_packfile 3 promisor-b) &&\n+\t\tPROMISOR_C=$(write_promisor_packfile 3 promisor-c) &&\n+\t\tPROMISOR_D=$(write_promisor_packfile 20 promisor-d) &&\n+\t\tPROMISOR_E=$(write_promisor_packfile 40 promisor-e) &&\n+\n+\t\tgit cat-file --batch-all-objects --batch-check=\"%(objectname)\" >objects-expect &&\n+\n+\t\tls .git/objects/pack/*.pack >packs-before &&\n+\t\ttest_line_count = 8 packs-before &&\n+\t\tgit repack --geometric=2 -d &&\n+\t\tls .git/objects/pack/*.pack >packs-after &&\n+\t\ttest_line_count = 5 packs-after &&\n+\t\ttest_grep ! \"$NORMAL_A\" packs-after &&\n+\t\ttest_grep ! \"$NORMAL_B\" packs-after &&\n+\t\ttest_grep \"$NORMAL_C\" packs-after &&\n+\t\ttest_grep ! \"$PROMISOR_A\" packs-after &&\n+\t\ttest_grep ! \"$PROMISOR_B\" packs-after &&\n+\t\ttest_grep ! \"$PROMISOR_C\" packs-after &&\n+\t\ttest_grep \"$PROMISOR_D\" packs-after &&\n+\t\ttest_grep \"$PROMISOR_E\" packs-after &&\n+\n+\t\tls .git/objects/pack/*.promisor >promisors &&\n+\t\ttest_line_count = 3 promisors &&\n+\n+\t\tgit cat-file --batch-all-objects --batch-check=\"%(objectname)\" >objects-actual &&\n+\t\ttest_cmp objects-expect objects-actual\n+\t)\n+'\n+\n test_done\n\n-- \n2.52.0.508.g883dcfc63e.dirty\n\n"},{"id":"533454","messageId":"aWGP/Lp2Eo03F7vN@nand.local","threadId":"64723","inReplyTo":"20260105-pks-geometric-repack-with-promisors-v1-1-c4660573437e@pks.im","subject":"Re: [PATCH 1/5] builtin/pack-objects: exclude promisor objects with \"--stdin-packs\"","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-01-09T23:32:12Z","receivedAt":"2026-01-09T23:32:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Jan 05, 2026 at 02:16:41PM +0100, Patrick Steinhardt wrote:\n> It is currently not possible to combine \"--exclude-promisor-objects\"\n> with \"--stdin-packs\" because both flags want to set up a revision walk\n> to enumerate the objects to pack. In a subsequent commit though we want\n> to extend geometric repacks to support promisor objects, and for that we\n> need to handle the combination of both flags.\n>\n> There are two cases we have to think about here:\n>\n>   - \"--stdin-packs\" asks us to pack exactly the objects part of the\n>     specified packfiles. It is somewhat questionable what to do in the\n>     case where the user asks us to exclude promisor objects, but at the\n>     same time explicitly passes a promisor pack to us. For now, we\n>     simply abort the request as it is self-contradicting. As we have\n>     also been dying before this commit there is no regression here.\n\nI was wondering whether or not this is the case, because we don't have\nan explicit `die()` here or a incompatible pair of options declared. But\nit does die(), although the message is somewhat confusing:\n\n    $ git.compile pack-objects --stdin-packs --exclude-promisor-objects --stdout >/dev/null\n    fatal: cannot use internal rev list with --stdin-packs\n\n;-).\n\n>   - \"--stdin-packs=follow\" does the same as the first flag, but it also\n>     asks us to include all objects transitively reachable from any\n>     object in the packs we are about to repack. This is done by doing\n>     the revision walk mentioned further up. Luckily, fixing this case is\n>     trivial: we only need to modify the revision walk to also set the\n>     `exclude_promisor_objects` field.\n\nHmm. I'm not totally sure if I'm following why we handle this case\nseparately. Could you elaborate?\n\n> Note that we do not support the \"--exclude-promisor-objects-best-effort\"\n> flag for now as we don't need it to support geometric repacking with\n> promisor objects.\n\nI didn't know we had such an option in the first place, but it looks\nlike it behaves similarly wrt. its incompatibility with `--stdin-packs`.\n\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n\nI'll hold off on commenting on the code to give us a chance to discuss\nthe above.\n\n> diff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\n> index 4a8df5a389..cd949025b9 100755\n> --- a/t/t5331-pack-objects-stdin.sh\n> +++ b/t/t5331-pack-objects-stdin.sh\n> @@ -319,6 +319,45 @@ test_expect_success '--stdin-packs=follow walks into unknown packs' '\n>  \t)\n>  '\n>\n> +test_expect_success '--stdin-packs with promisors' '\n> +\ttest_when_finished \"rm -fr repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\tgit config set maintenance.auto false &&\n> +\t\tgit remote add promisor garbage &&\n> +\t\tgit config set remote.promisor.promisor true &&\n> +\n> +\t\tfor c in A B C D\n> +\t\tdo\n> +\t\t\techo \"$c\" >file &&\n> +\t\t\tgit add file &&\n> +\t\t\tgit commit --message \"$c\" &&\n> +\t\t\tgit tag \"$c\" || return 1\n\nUnless these changes all have to live in the same file, could this\ninstead be written as:\n\n    for c in A B C D\n    do\n        test_commit \"$c\" || return 1\n    done &&\n    # ...\n\n?\n\nThanks,\nTaylor\n"},{"id":"533455","messageId":"aWGR1r5PlLL3rWWd@nand.local","threadId":"64723","inReplyTo":"20260105-pks-geometric-repack-with-promisors-v1-4-c4660573437e@pks.im","subject":"Re: [PATCH 4/5] repack-promisor: extract function to remove redundant packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-01-09T23:40:06Z","receivedAt":"2026-01-09T23:40:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Jan 05, 2026 at 02:16:44PM +0100, Patrick Steinhardt wrote:\n> @@ -226,6 +227,15 @@ void pack_geometry_remove_redundant(struct pack_geometry *geometry,\n>  \tstrbuf_release(&buf);\n>  }\n>\n> +void pack_geometry_remove_redundant(struct pack_geometry *geometry,\n> +\t\t\t\t    struct string_list *names,\n> +\t\t\t\t    struct existing_packs *existing,\n> +\t\t\t\t    const char *packdir)\n> +{\n> +\tremove_redundant_packs(geometry->pack, geometry->split,\n> +\t\t\t       names, existing, packdir);\n> +}\n> +\n\nThe refactoring up to this point looks all good to me.\n\nAs a side-note, I would love to get rid of the\npack_geometry_remove_redundant() function altogether. It is kind of a\nhack that we handle determining which packs are made redundant by a\nrepacking operation in two different ways depending on whether or not we\nare doing a geometric repack.\n\nI have some patches to do this in a series that implements the\n\"reachability-guided\" geometric repacking technique that I have talked\nabove[^1] previously. I think (having skimmed the next patch but not yet\nfully reviewed it) that this should still all be doable with your\npatches. We just have to take two passes (once through the existing\nnon-promisor packs and then another pass through the promisor\nones).\n\nSo I think that this all looks good to me and shouldn't interfere with\nthat effort, though I'll make a note to rebase those patches on top of\nthese to make it easier for the maintainer to queue both of them.\n\nThanks,\nTaylor\n\n[^1]: Well, I was pretty sure that I had mentioned it on the list, but\n  can't seem to find anything corresponding to it in my \"sent\" folder.\n  In case I haven't talked above it before, the gist is a special mode\n  of --stdin-packs that only packs objects from the included set of\n  packs which are reachable. If repack generates a cruft pack after the\n  fact, that allows us to \"incrementally\" build up the cruft pack over\n  time.\n"},{"id":"533599","messageId":"aWTAzR4H3XuAlJJn@pks.im","threadId":"64723","inReplyTo":"aWGP/Lp2Eo03F7vN@nand.local","subject":"Re: [PATCH 1/5] builtin/pack-objects: exclude promisor objects with \"--stdin-packs\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-12T09:37:17Z","receivedAt":"2026-01-12T09:37:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 09, 2026 at 06:32:12PM -0500, Taylor Blau wrote:\n> On Mon, Jan 05, 2026 at 02:16:41PM +0100, Patrick Steinhardt wrote:\n> >   - \"--stdin-packs=follow\" does the same as the first flag, but it also\n> >     asks us to include all objects transitively reachable from any\n> >     object in the packs we are about to repack. This is done by doing\n> >     the revision walk mentioned further up. Luckily, fixing this case is\n> >     trivial: we only need to modify the revision walk to also set the\n> >     `exclude_promisor_objects` field.\n> \n> Hmm. I'm not totally sure if I'm following why we handle this case\n> separately. Could you elaborate?\n\nYou mean why we handle \"--stdin-packs\" and \"--stdin-packs=follow\"\nseparately?\n\nThe thing is that we don't really need to care about the case where we\nwant to exclude promisor objects with \"--stdin-packs\" because we don't\nperform any object walk at all. We'll only merge objects part of packs\nthat have been passed to us via stdin. Consequently, you can say that a\nrequest where the user asks us to exclude promisor objects while at the\nsame time asking us to pack a promisor pack is self-contracdicting, as\nthey could have just as well left out the promisor pack from the\nrequest to achieve the same.\n\nThere's two approaches here:\n\n  - We can simply die when seeing such a malformed request. This is\n    exactly what we do with this patch, and that cannot be a regression\n    because we already died beforehand. We strictly expand the set of\n    supported cases where we pack objects.\n\n  - We can honor this, but exclude promisor packs altogether. This is a\n    feasible thing to do, but now we also have to care about the case\n    where all passed packs are promisor packs. Also, the result would\n    arguably be _more_ surprising if we exclude packing some packs that\n    the user has passed to us.\n\nIn \"--stdin-packs=follow\" we _also_ do the same as above and die in case\nwe're passed a promisor pack directly. But in addition to that, we also\nneed to pay attention to the rev-walk we do, because \"follow\" asks us to\ninclude objects reachable from any of the packs. So in case any such\nobject is a promisor object we need to exclude it.\n\nIn the context of `git repack --geometric=2` we won't care about the\nfirst case: we won't ever ask git-pack-objects(1) to include a promisor\npack in the normal geometric sequence. We do care about the second case\nthough as git-repack(1) may end up passing \"--stdin-packs=follow\" when\n\"repack.midxMustContainCruft=false\".\n\n> > diff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\n> > index 4a8df5a389..cd949025b9 100755\n> > --- a/t/t5331-pack-objects-stdin.sh\n> > +++ b/t/t5331-pack-objects-stdin.sh\n> > @@ -319,6 +319,45 @@ test_expect_success '--stdin-packs=follow walks into unknown packs' '\n> >  \t)\n> >  '\n> >\n> > +test_expect_success '--stdin-packs with promisors' '\n> > +\ttest_when_finished \"rm -fr repo\" &&\n> > +\tgit init repo &&\n> > +\t(\n> > +\t\tcd repo &&\n> > +\t\tgit config set maintenance.auto false &&\n> > +\t\tgit remote add promisor garbage &&\n> > +\t\tgit config set remote.promisor.promisor true &&\n> > +\n> > +\t\tfor c in A B C D\n> > +\t\tdo\n> > +\t\t\techo \"$c\" >file &&\n> > +\t\t\tgit add file &&\n> > +\t\t\tgit commit --message \"$c\" &&\n> > +\t\t\tgit tag \"$c\" || return 1\n> \n> Unless these changes all have to live in the same file, could this\n> instead be written as:\n> \n>     for c in A B C D\n>     do\n>         test_commit \"$c\" || return 1\n>     done &&\n>     # ...\n> \n> ?\n\nWe unfortunately can't. The problem is that any object reachable from a\npromisor object may be labelled as a promisor object. So if we had a\ntree that makes all blobs reachable we'd treat all of them as promised\nblobs.\n\nIt's quite confusing overall.\n\nPatrick\n"},{"id":"533820","messageId":"87qzrsjsa2.fsf@iotcl.com","threadId":"64723","inReplyTo":"20260105-pks-geometric-repack-with-promisors-v1-2-c4660573437e@pks.im","subject":"Re: [PATCH 2/5] repack-geometry: extract function to compute repacking split","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2026-01-14T12:24:53Z","receivedAt":"2026-01-14T12:25:05Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> We're about to add a second caller that wants to compute the repacking\n> split for a set of packfiles. Split out the function that computes this\n> split to prepare for that.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  repack-geometry.c | 39 +++++++++++++++++++++------------------\n>  1 file changed, 21 insertions(+), 18 deletions(-)\n>\n> diff --git a/repack-geometry.c b/repack-geometry.c\n> index b3e32cd07e..17e6652a91 100644\n> --- a/repack-geometry.c\n> +++ b/repack-geometry.c\n> @@ -78,33 +78,32 @@ void pack_geometry_init(struct pack_geometry *geometry,\n>  \tstrbuf_release(&buf);\n>  }\n>  \n> -void pack_geometry_split(struct pack_geometry *geometry)\n> +static uint32_t compute_pack_geometry_split(struct packed_git **pack, size_t pack_nr,\n> +\t\t\t\t\t    int split_factor)\n>  {\n>  \tuint32_t i;\n>  \tuint32_t split;\n>  \toff_t total_size = 0;\n>  \n> -\tif (!geometry->pack_nr) {\n> -\t\tgeometry->split = geometry->pack_nr;\n> -\t\treturn;\n> -\t}\n> +\tif (!pack_nr)\n> +\t\treturn 0;\n\nThanks for making this easier to read now. Took me a while to realize\nthey behave identical.\n\n\n-- \nCheers,\nToon\n"},{"id":"533830","messageId":"87ms2gjjv1.fsf@iotcl.com","threadId":"64723","inReplyTo":"20260105-pks-geometric-repack-with-promisors-v1-5-c4660573437e@pks.im","subject":"Re: [PATCH 5/5] builtin/repack: handle promisor packs with geometric repacking","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2026-01-14T15:26:42Z","receivedAt":"2026-01-14T15:26:53Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> When performing a fetch with an object filter, we mark the resulting\n> packfile as a promisor pack. An object part of such a pack may miss any\n> of its referenced objects, and Git knows to handle this case by fetching\n> any such missing objects from the promisor remote.\n>\n> The \"promisor\" property needs to be retained going forward. So every\n> time we pack a promisor object, the resulting pack must be marked as a\n> promisor pack. git-repack(1) does this already: when a repository has a\n> promisor remote, it knows to pass \"--exclude-promisor-objects\" to the\n> git-pack-objects(1) child process. Promisor packs are written separately\n> when doing an all-into-one repack via `repack_promisor_objects()`.\n>\n> But we don't support promisor objects when doing a geometric repack yet.\n> Promisor packs do not get any special treatment there, as we simply\n> merge promisor and non-promisor packs. The resulting pack is not even\n> marked as a promisor pack, which essentially corrupts the repository.\n>\n> This corruption couldn't happen in the real world though: we pass both\n> \"--exclude-promisor-objects\" and \"--stdin-packs\" to git-pack-objects(1)\n> if a repository has a promisor remote, but as those options are mutually\n> exclusive we always end up dying. And while we made those flags\n> compatible with one another in a preceding commit, we still end up dying\n> in case git-pack-objects(1) is asked to repack a promisor pack.\n>\n> There's multiple ways to fix this:\n>\n>   - We can exclude promisor packs from the geometric progression\n>     altogether. This would have the consequence that we never repack\n>     promisor packs at all. But in a partial clone it is quite likely\n>     that the user generates a bunch of promisor packs over time, as\n>     every backfill fetch would create another one. So this doesn't\n>     really feel like a sensible option.\n>\n>   - We can adapt git-pack-objects(1) to support repacking promisor packs\n>     and include them in the normal geometric progression. But this would\n>     mean that the set of promisor objects expands over time as the packs\n>     are merged with normal packs.\n>\n>   - We can use a separate geometric progression to repack promisor\n>     packs.\n>\n> The first two options both have significant downsides, so they aren't\n> really feasible. But the third option fixes both of these downsides: we\n> make sure that promisor packs get merged, and at the same time we never\n> expand the set of promisor objects beyond the set of objects that are\n> already marked as promisor objects.\n>\n> Implement this strategy so that geometric repacking works in partial\n> clones.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  builtin/repack.c            |  3 +++\n>  repack-geometry.c           | 28 ++++++++++++++++-----\n>  repack-promisor.c           | 28 +++++++++++++++++++++\n>  repack.h                    | 10 ++++++++\n>  t/t7703-repack-geometric.sh | 61 +++++++++++++++++++++++++++++++++++++++++++++\n>  5 files changed, 124 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index d9012141f6..f6bb04bef7 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -332,6 +332,9 @@ int cmd_repack(int argc,\n>  \t\t    !(pack_everything & PACK_CRUFT))\n>  \t\t\tstrvec_push(&cmd.args, \"--pack-loose-unreachable\");\n>  \t} else if (geometry.split_factor) {\n> +\t\tpack_geometry_repack_promisors(repo, &po_args, &geometry,\n> +\t\t\t\t\t       &names, packtmp);\n> +\n>  \t\tif (midx_must_contain_cruft)\n>  \t\t\tstrvec_push(&cmd.args, \"--stdin-packs\");\n>  \t\telse\n> diff --git a/repack-geometry.c b/repack-geometry.c\n> index 0daf545a81..7cebd0cb45 100644\n> --- a/repack-geometry.c\n> +++ b/repack-geometry.c\n> @@ -66,15 +66,25 @@ void pack_geometry_init(struct pack_geometry *geometry,\n>  \t\tif (p->is_cruft)\n>  \t\t\tcontinue;\n>  \n> -\t\tALLOC_GROW(geometry->pack,\n> -\t\t\t   geometry->pack_nr + 1,\n> -\t\t\t   geometry->pack_alloc);\n> -\n> -\t\tgeometry->pack[geometry->pack_nr] = p;\n> -\t\tgeometry->pack_nr++;\n> +\t\tif (p->pack_promisor) {\n> +\t\t\tALLOC_GROW(geometry->promisor_pack,\n> +\t\t\t\t   geometry->promisor_pack_nr + 1,\n> +\t\t\t\t   geometry->promisor_pack_alloc);\n> +\n> +\t\t\tgeometry->promisor_pack[geometry->promisor_pack_nr] = p;\n> +\t\t\tgeometry->promisor_pack_nr++;\n> +\t\t} else {\n> +\t\t\tALLOC_GROW(geometry->pack,\n> +\t\t\t\t   geometry->pack_nr + 1,\n> +\t\t\t\t   geometry->pack_alloc);\n> +\n> +\t\t\tgeometry->pack[geometry->pack_nr] = p;\n> +\t\t\tgeometry->pack_nr++;\n> +\t\t}\n>  \t}\n>  \n>  \tQSORT(geometry->pack, geometry->pack_nr, pack_geometry_cmp);\n> +\tQSORT(geometry->promisor_pack, geometry->promisor_pack_nr, pack_geometry_cmp);\n>  \tstrbuf_release(&buf);\n>  }\n>  \n> @@ -160,6 +170,9 @@ void pack_geometry_split(struct pack_geometry *geometry)\n>  {\n>  \tgeometry->split = compute_pack_geometry_split(geometry->pack, geometry->pack_nr,\n>  \t\t\t\t\t\t      geometry->split_factor);\n> +\tgeometry->promisor_split = compute_pack_geometry_split(geometry->promisor_pack,\n> +\t\t\t\t\t\t\t       geometry->promisor_pack_nr,\n> +\t\t\t\t\t\t\t       geometry->split_factor);\n>  }\n>  \n>  struct packed_git *pack_geometry_preferred_pack(struct pack_geometry *geometry)\n> @@ -234,6 +247,8 @@ void pack_geometry_remove_redundant(struct pack_geometry *geometry,\n>  {\n>  \tremove_redundant_packs(geometry->pack, geometry->split,\n>  \t\t\t       names, existing, packdir);\n> +\tremove_redundant_packs(geometry->promisor_pack, geometry->promisor_split,\n> +\t\t\t       names, existing, packdir);\n>  }\n>  \n>  void pack_geometry_release(struct pack_geometry *geometry)\n> @@ -242,4 +257,5 @@ void pack_geometry_release(struct pack_geometry *geometry)\n>  \t\treturn;\n>  \n>  \tfree(geometry->pack);\n> +\tfree(geometry->promisor_pack);\n>  }\n> diff --git a/repack-promisor.c b/repack-promisor.c\n> index 125038d92e..73af57bce3 100644\n> --- a/repack-promisor.c\n> +++ b/repack-promisor.c\n> @@ -109,3 +109,31 @@ void repack_promisor_objects(struct repository *repo,\n>  \n>  \tfinish_repacking_promisor_objects(repo, &cmd, names, packtmp);\n>  }\n> +\n> +void pack_geometry_repack_promisors(struct repository *repo,\n> +\t\t\t\t    const struct pack_objects_args *args,\n> +\t\t\t\t    const struct pack_geometry *geometry,\n> +\t\t\t\t    struct string_list *names,\n> +\t\t\t\t    const char *packtmp)\n> +{\n> +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> +\tFILE *in;\n> +\n> +\tif (!geometry->promisor_split)\n> +\t\treturn;\n> +\n> +\tprepare_pack_objects(&cmd, args, packtmp);\n> +\tstrvec_push(&cmd.args, \"--stdin-packs\");\n> +\tcmd.in = -1;\n> +\tif (start_command(&cmd))\n> +\t\tdie(_(\"could not start pack-objects to repack promisor packs\"));\n> +\n> +\tin = xfdopen(cmd.in, \"w\");\n> +\tfor (size_t i = 0; i < geometry->promisor_split; i++)\n> +\t\tfprintf(in, \"%s\\n\", pack_basename(geometry->promisor_pack[i]));\n> +\tfor (size_t i = geometry->promisor_split; i < geometry->promisor_pack_nr; i++)\n> +\t\tfprintf(in, \"^%s\\n\", pack_basename(geometry->promisor_pack[i]));\n \nOkay, so in the situation of the added integration test: PROMISOR_A,\nPROMISOR_B, and PROMISOR_C are below the split, and PROMISOR_D and\nPROMISOR_E above. So using a caret, the latter two marked to be excluded\nby git-pack-objects. Makes sense.\n\n> +\tfclose(in);\n> +\n> +\tfinish_repacking_promisor_objects(repo, &cmd, names, packtmp);\n> +}\n> diff --git a/repack.h b/repack.h\n> index 3a688a12ee..bc9f2e1a5d 100644\n> --- a/repack.h\n> +++ b/repack.h\n> @@ -103,9 +103,19 @@ struct pack_geometry {\n>  \tuint32_t pack_nr, pack_alloc;\n>  \tuint32_t split;\n>  \n> +\tstruct packed_git **promisor_pack;\n> +\tuint32_t promisor_pack_nr, promisor_pack_alloc;\n> +\tuint32_t promisor_split;\n> +\n>  \tint split_factor;\n>  };\n>  \n> +void pack_geometry_repack_promisors(struct repository *repo,\n> +\t\t\t\t    const struct pack_objects_args *args,\n> +\t\t\t\t    const struct pack_geometry *geometry,\n> +\t\t\t\t    struct string_list *names,\n> +\t\t\t\t    const char *packtmp);\n> +\n>  void pack_geometry_init(struct pack_geometry *geometry,\n>  \t\t\tstruct existing_packs *existing,\n>  \t\t\tconst struct pack_objects_args *args);\n> diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\n> index 98806cdb6f..04d5d8fc33 100755\n> --- a/t/t7703-repack-geometric.sh\n> +++ b/t/t7703-repack-geometric.sh\n> @@ -480,4 +480,65 @@ test_expect_success '--geometric -l disables writing bitmaps with non-local pack\n>  \ttest_path_is_file member/.git/objects/pack/multi-pack-index-*.bitmap\n>  '\n>  \n> +write_packfile () {\n> +\tNR=\"$1\"\n> +\tPREFIX=\"$2\"\n> +\n> +\tprintf \"blob\\ndata <<EOB\\n$PREFIX %s\\nEOB\\n\" $(test_seq $NR) |\n\nThis test_seq() is fancy trickery if you ask me, but it seems to work\nvery well. In case anyone is wondering when 'NR=3' and\n'PREFIX=normal-a', you'll get:\n\n    blob\n    data <<EOB\n    normal-a 1\n    EOB\n    blob\n    data <<EOB\n    normal-a 2\n    EOB\n    blob\n    data <<EOB\n    normal-a 3\n    EOB\n\nI didn't realize doing `printf %s` will print the format string for each\nline passed in.\n\n> +\t\tgit fast-import &&\n> +\tgit pack-objects --pack-loose-unreachable .git/objects/pack/pack &&\n> +\tgit prune-packed\n> +}\n> +\n> +write_promisor_packfile () {\n> +\tPACKFILE=$(write_packfile \"$@\") &&\n> +\ttouch .git/objects/pack/pack-$PACKFILE.promisor &&\n\nOkay, understood. As mentioned in Documentation/git-repack.adoc, packs\nhaving a .promisor file, will force them to be packed separately.\n\n> +\techo \"$PACKFILE\"\n> +}\n> +\n> +test_expect_success 'geometric repack works with promisor packs' '\n> +\ttest_when_finished \"rm -fr repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\tgit config set maintenance.auto false &&\n> +\t\tgit remote add promisor garbage &&\n> +\t\tgit config set remote.promisor.promisor true &&\n> +\n> +\t\t# Packs A and B need to be merged.\n> +\t\tNORMAL_A=$(write_packfile 2 normal-a) &&\n> +\t\tNORMAL_B=$(write_packfile 2 normal-b) &&\n> +\t\tNORMAL_C=$(write_packfile 14 normal-c) &&\n\nOkay, so pack C is more than twice as large as A and B combined, so\nthat's why that should be merged.\n\n> +\n> +\t\t# Packs A, B and C need to be merged.\n> +\t\tPROMISOR_A=$(write_promisor_packfile 1 promisor-a) &&\n> +\t\tPROMISOR_B=$(write_promisor_packfile 3 promisor-b) &&\n> +\t\tPROMISOR_C=$(write_promisor_packfile 3 promisor-c) &&\n> +\t\tPROMISOR_D=$(write_promisor_packfile 20 promisor-d) &&\n> +\t\tPROMISOR_E=$(write_promisor_packfile 40 promisor-e) &&\n\nSimilar here, D and E are a geometric sequence, but all the previous\nshould be merged.\n\n> +\n> +\t\tgit cat-file --batch-all-objects --batch-check=\"%(objectname)\" >objects-expect &&\n> +\n> +\t\tls .git/objects/pack/*.pack >packs-before &&\n> +\t\ttest_line_count = 8 packs-before &&\n> +\t\tgit repack --geometric=2 -d &&\n> +\t\tls .git/objects/pack/*.pack >packs-after &&\n> +\t\ttest_line_count = 5 packs-after &&\n> +\t\ttest_grep ! \"$NORMAL_A\" packs-after &&\n> +\t\ttest_grep ! \"$NORMAL_B\" packs-after &&\n> +\t\ttest_grep \"$NORMAL_C\" packs-after &&\n> +\t\ttest_grep ! \"$PROMISOR_A\" packs-after &&\n> +\t\ttest_grep ! \"$PROMISOR_B\" packs-after &&\n> +\t\ttest_grep ! \"$PROMISOR_C\" packs-after &&\n> +\t\ttest_grep \"$PROMISOR_D\" packs-after &&\n> +\t\ttest_grep \"$PROMISOR_E\" packs-after &&\n> +\n> +\t\tls .git/objects/pack/*.promisor >promisors &&\n> +\t\ttest_line_count = 3 promisors &&\n> +\n> +\t\tgit cat-file --batch-all-objects --batch-check=\"%(objectname)\" >objects-actual &&\n> +\t\ttest_cmp objects-expect objects-actual\n> +\t)\n> +'\n> +\n>  test_done\n>\n> -- \n> 2.52.0.508.g883dcfc63e.dirty\n>\n>\n\nI like the solution you have chosen. It makes sense, and so does the\ncode, or at least to the best of my knowledge. Included test coverage\nhelps understanding the solution and demonstrate it works.\n\n-- \nCheers,\nToon\n"}]}