# [PATCH 0/4] repack: various corner cases for cruft-less MIDXs 41 messages from 2026-09-30 to 2026-10-03. Participants: Taylor Blau, Junio C Hamano, Derrick Stolee, Jeff King, Elijah Newren. Thread: https://gitlist.dev/t/66424 ## Taylor Blau, 2026-09-30 01:28 Subject: [PATCH 0/4] repack: various corner cases for cruft-less MIDXs Message-ID: ``` This patch series fixes a few bugs I spotted while investigating the cruft-less MIDX feature. The bugs addressed are found in various corner cases, and, when triggered, may result in a MIDX being written whose objects are not closed under reachability. When this happens while the caller is trying to write reachability bitmaps, bitmap generation may fail if one or more selected commits are descendants of the open portion of the MIDX. The series is structured as follows: * The first patch is a preparatory refactoring to add a context struct within pack-objects' handling of '--stdin-packs' to minimize the diff in the subsequent patch. * The second patch fixes a case where once-cruft tree and annotated tag objects may prevent reachability closure when objects reachable from them are not present in the input pack(s). * The third patch fixes a case where incremental repack operations may introduce the same bug when the pack generated by an incremental repack does not pack an additional copy of once-cruft object(s). * The fourth and final patch addresses a similar case involving .keep packs. Thanks in advance for reviewing! Taylor Blau (4): pack-objects: introduce `stdin_packs_context` struct pack-objects: ensure tree/tag closure with '--stdin-packs=follow' repack: retain cruft packs in MIDXs after incremental repacks repack: retain cruft packs in MIDXs containing kept packs Documentation/git-pack-objects.adoc | 2 + builtin/pack-objects.c | 73 ++++++++++++++++++++------ builtin/repack.c | 6 +++ repack-midx.c | 5 ++ t/t5331-pack-objects-stdin.sh | 81 +++++++++++++++++++++++++++++ t/t7704-repack-cruft.sh | 51 ++++++++++++++++++ 6 files changed, 202 insertions(+), 16 deletions(-) base-commit: a018953688f1b10bddf91bff8747068f5f4746a4 -- 2.56.0.4.gbee41d2fc68 ``` ## Taylor Blau, 2026-09-30 01:28 Subject: [PATCH 1/4] pack-objects: introduce `stdin_packs_context` struct Message-ID: <64bb13e2db2e5c22e842c188e08861d63e99dc77.1790731662.git.me@ttaylorr.com> In-Reply-To: ``` `stdin_packs_read_input()` currently receives a pointer to the `rev_info` struct and '--stdin-packs' mode separately as arguments, but the object enumeration callbacks only receive a pointer to the `rev_info` struct. Wrap the pair in a new `stdin_packs_context` struct so that a future change may reference the '--stdin-packs' mode within the various object enumeration callbacks. Signed-off-by: Taylor Blau --- builtin/pack-objects.c | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index af9390a46b9..01adf80a2bc 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -3804,11 +3804,17 @@ static int git_pack_config(const char *k, const char *v, static int stdin_packs_found_nr; static int stdin_packs_hints_nr; +struct stdin_packs_context { + struct rev_info *revs; + enum stdin_packs_mode mode; +}; + static int add_object_entry_from_pack(const struct object_id *oid, struct packed_git *p, uint32_t pos, void *_data) { + struct stdin_packs_context *ctx = _data; off_t ofs; struct object_info oi = OBJECT_INFO_INIT; enum object_type type = OBJ_NONE; @@ -3827,7 +3833,6 @@ static int add_object_entry_from_pack(const struct object_id *oid, die(_("could not get type of object %s in pack %s"), oid_to_hex(oid), p->pack_name); } else if (type == OBJ_COMMIT) { - struct rev_info *revs = _data; /* * commits in included packs are used as starting points * for the subsequent revision walk @@ -3840,7 +3845,7 @@ static int add_object_entry_from_pack(const struct object_id *oid, * However, we'll only add those objects to the packing * list after checking `want_object_in_pack()` below. */ - add_pending_oid(revs, NULL, oid, 0); + add_pending_oid(ctx->revs, NULL, oid, 0); } if (!want_object_in_pack(oid, 0, &p, &ofs)) @@ -3954,8 +3959,9 @@ static int stdin_packs_include_check(struct commit *commit, void *data) } static void stdin_packs_add_pack_entries(struct strmap *packs, - struct rev_info *revs) + struct stdin_packs_context *ctx) { + struct rev_info *revs = ctx->revs; struct string_list keys = STRING_LIST_INIT_NODUP; struct string_list_item *item; struct hashmap_iter iter; @@ -3994,15 +4000,14 @@ static void stdin_packs_add_pack_entries(struct strmap *packs, (info->kind & STDIN_PACK_EXCLUDE_OPEN)) for_each_object_in_pack(info->p, add_object_entry_from_pack, - revs, + ctx, ODB_FOR_EACH_OBJECT_PACK_ORDER); } string_list_clear(&keys, 0); } -static void stdin_packs_read_input(struct rev_info *revs, - enum stdin_packs_mode mode) +static void stdin_packs_read_input(struct stdin_packs_context *ctx) { struct strbuf buf = STRBUF_INIT; struct strmap packs = STRMAP_INIT; @@ -4017,7 +4022,7 @@ static void stdin_packs_read_input(struct rev_info *revs, continue; else if (*key == '^') kind = STDIN_PACK_EXCLUDE_CLOSED; - else if (*key == '!' && mode == STDIN_PACKS_MODE_FOLLOW) + else if (*key == '!' && ctx->mode == STDIN_PACKS_MODE_FOLLOW) kind = STDIN_PACK_EXCLUDE_OPEN; if (kind != STDIN_PACK_INCLUDE) @@ -4082,19 +4087,23 @@ static void stdin_packs_read_input(struct rev_info *revs, info->p = p; } - stdin_packs_add_pack_entries(&packs, revs); + stdin_packs_add_pack_entries(&packs, ctx); strbuf_release(&buf); strmap_clear(&packs, 1); } -static void add_unreachable_loose_objects(struct rev_info *revs); +static void add_unreachable_loose_objects(struct stdin_packs_context *ctx); static void read_stdin_packs(struct repository *repo, enum stdin_packs_mode mode, int rev_list_unpacked) { int prev_fetch_if_missing = repo->fetch_if_missing; struct rev_info revs; + struct stdin_packs_context ctx = { + .revs = &revs, + .mode = mode, + }; /* * The revision walk may hit objects that are promised, only. As the @@ -4131,9 +4140,9 @@ static void read_stdin_packs(struct repository *repo, */ ignore_packed_keep_in_core_open = 1; } - stdin_packs_read_input(&revs, mode); + stdin_packs_read_input(&ctx); if (rev_list_unpacked) - add_unreachable_loose_objects(&revs); + add_unreachable_loose_objects(&ctx); if (prepare_revision_walk(&revs)) die(_("revision walk setup failed")); @@ -4541,7 +4550,7 @@ static void add_objects_in_unpacked_packs(void) static int add_loose_object(const struct object_id *oid, const char *path, void *data) { - struct rev_info *revs = data; + struct stdin_packs_context *ctx = data; enum object_type type = odb_read_object_info(the_repository->objects, oid, NULL); if (type < 0) { @@ -4563,8 +4572,8 @@ static int add_loose_object(const struct object_id *oid, const char *path, add_object_entry(oid, type, "", 0); } - if (revs && type == OBJ_COMMIT) - add_pending_oid(revs, NULL, oid, 0); + if (ctx && type == OBJ_COMMIT) + add_pending_oid(ctx->revs, NULL, oid, 0); return 0; } @@ -4574,10 +4583,10 @@ static int add_loose_object(const struct object_id *oid, const char *path, * add_object_entry will weed out duplicates, so we just add every * loose object we find. */ -static void add_unreachable_loose_objects(struct rev_info *revs) +static void add_unreachable_loose_objects(struct stdin_packs_context *ctx) { for_each_loose_file_in_source(the_repository->objects->sources, - add_loose_object, NULL, NULL, revs); + add_loose_object, NULL, NULL, ctx); } static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid) -- 2.56.0.4.gbee41d2fc68 ``` ## Taylor Blau, 2026-09-30 01:28 Subject: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow' Message-ID: <6348667e2e3fe63aeb139888e877dd8447570253.1790731662.git.me@ttaylorr.com> In-Reply-To: ``` Since 5ee86c273bf (repack: exclude cruft pack(s) from the MIDX where possible, 2025-06-23), when the 'repack.midxMustContainCruft' configuration is set to "false", geometric repacks use '--stdin-packs=follow' to copy needed objects out of cruft packs so the MIDX can omit those packs. In cd846bacc7d (pack-objects: introduce '--stdin-packs=follow', 2025-06-23), this behavior changed such that whenever excluded-open ('!') packs are present, the walk stops at objects in excluded-closed ('^') packs. Geometric repacks use '^' for retained packs already in the MIDX, relying on the indexed object set being closed under reachability. However, the walk introduced in cd846bacc7d starts only from commit objects. A geometric repack can therefore produce a MIDX that does not maintain reachability closure for lone trees (that are not reachable from any commit otherwise in the closure). A later walk with '!' packs can stop at that tree in a retained '^' pack even if a new commit reaches it. If the cruft pack remains excluded, and the bitmap selection picks one or more commits which reach that tree, the MIDX cannot generate a bitmap for that commit. Add trees and tags from included and '!' packs (and loose ones with '--unpacked') as roots in '--stdin-packs=follow' mode. This rescues their descendants even when no input commit reaches them. Walk these roots after the existing traversal, preserving the `SEEN` bit to avoid redundant traversals. Ensure that the walk takes place *after* the existing traversal so that we don't lose the path prefix used for trees and blobs wherever possible. Objects in '^' packs remain cutoffs to avoid rewalking packs that are known to be closed under reachability. Signed-off-by: Taylor Blau --- Documentation/git-pack-objects.adoc | 2 + builtin/pack-objects.c | 32 ++++++++++++ t/t5331-pack-objects-stdin.sh | 81 +++++++++++++++++++++++++++++ t/t7704-repack-cruft.sh | 20 +++++++ 4 files changed, 135 insertions(+) diff --git a/Documentation/git-pack-objects.adoc b/Documentation/git-pack-objects.adoc index 65cd00c152f..1564d44f49d 100644 --- a/Documentation/git-pack-objects.adoc +++ b/Documentation/git-pack-objects.adoc @@ -112,6 +112,8 @@ pack may include additional objects based on the following: This mode is useful, for example, to resurrect once-unreachable objects found in cruft packs to generate packs which are closed under reachability up to the boundary set by the excluded packs. +Trees and tags in included or `!` packs are followed even when no +commit reaches them, as are loose trees and tags with `--unpacked`. + Incompatible with `--revs`, or options that imply `--revs` (such as `--all`), with the exception of `--unpacked`, which is compatible. diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 01adf80a2bc..05a94305265 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -3807,6 +3807,7 @@ static int stdin_packs_hints_nr; struct stdin_packs_context { struct rev_info *revs; enum stdin_packs_mode mode; + struct oid_array extra_roots; }; static int add_object_entry_from_pack(const struct object_id *oid, @@ -3846,6 +3847,9 @@ static int add_object_entry_from_pack(const struct object_id *oid, * list after checking `want_object_in_pack()` below. */ add_pending_oid(ctx->revs, NULL, oid, 0); + } else if (ctx->mode == STDIN_PACKS_MODE_FOLLOW && + (type == OBJ_TREE || type == OBJ_TAG)) { + oid_array_append(&ctx->extra_roots, oid); } if (!want_object_in_pack(oid, 0, &p, &ofs)) @@ -4103,6 +4107,7 @@ static void read_stdin_packs(struct repository *repo, struct stdin_packs_context ctx = { .revs = &revs, .mode = mode, + .extra_roots = OID_ARRAY_INIT, }; /* @@ -4151,6 +4156,30 @@ static void read_stdin_packs(struct repository *repo, show_object_pack_hint, &mode); + /* + * Trees and tags need closure even when no commit reaches them. + * Defer adding these roots to revs.pending until the commit walk + * finishes. Otherwise a subtree may be visited and marked SEEN + * before its commit's root tree, using "a" instead of "sub/a" for + * a blob's namehash and delta attributes. + */ + for (size_t i = 0; i < ctx.extra_roots.nr; i++) { + const struct object_id *oid = &ctx.extra_roots.oid[i]; + struct object *obj = lookup_object(repo, oid); + + if (!obj || !(obj->flags & SEEN)) + add_pending_oid(&revs, NULL, oid, 0); + } + if (revs.pending.nr) { + if (prepare_revision_walk(&revs)) + die(_("revision walk setup failed")); + traverse_commit_list(&revs, + show_commit_pack_hint, + show_object_pack_hint, + &mode); + } + oid_array_clear(&ctx.extra_roots); + release_revisions(&revs); trace2_data_intmax("pack-objects", the_repository, "stdin_packs_found", @@ -4574,6 +4603,9 @@ static int add_loose_object(const struct object_id *oid, const char *path, if (ctx && type == OBJ_COMMIT) add_pending_oid(ctx->revs, NULL, oid, 0); + else if (ctx && ctx->mode == STDIN_PACKS_MODE_FOLLOW && + (type == OBJ_TREE || type == OBJ_TAG)) + oid_array_append(&ctx->extra_roots, oid); return 0; } diff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh index c74b5861af3..aa79ecdf13c 100755 --- a/t/t5331-pack-objects-stdin.sh +++ b/t/t5331-pack-objects-stdin.sh @@ -520,4 +520,85 @@ test_expect_success '--stdin-packs with !-delimited pack without follow' ' ) ' +test_expect_success '--stdin-packs=follow traverses a tree-only input pack' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit base && + tree=$(git rev-parse HEAD^{tree}) && + P=$(echo "$tree" | git pack-objects $packdir/pack) && + echo "pack-$P.pack" >in && + + # Only --stdin-packs=follow should start a walk from the tree. + : >trace.txt && + GIT_TRACE2_EVENT="$(pwd)/trace.txt" git pack-objects \ + --stdin-packs --stdout /dev/null && + + test_trace2_data pack-objects stdin_packs_hints 0 expect.raw && + sort expect.raw >expect && + objects_in_packs $P >actual && + + test_cmp expect actual + ) +' + +test_expect_success '--stdin-packs=follow traverses an excluded-open tag' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit --annotate base && + + # Put the commit, tree, and blob in one pack, and the tag in another. + # Give only the second pack as input with a "!" prefix. The result + # must contain the commit, tree, and blob, but not the tag. + P=$(echo HEAD | git pack-objects --revs $packdir/pack) && + objects_in_packs $P >expect && + + git rev-parse base >in && + P=$(git pack-objects $packdir/pack in && + P=$(git pack-objects --stdin-packs=follow $packdir/pack actual && + + test_cmp expect actual + ) +' + +test_expect_success '--stdin-packs=follow respects delta attributes for subtree contents' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + + echo "sub/* -delta" >.gitattributes && + mkdir sub && + test-tool genrandom seed 8192 >sub/a && + cp sub/a sub/b && + echo modified >>sub/b && + git add sub && + git commit -m base && + + # If the subtree is visited first, the blobs are found as a and + # b, so the sub/* attribute does not apply. + git rev-parse HEAD HEAD:sub >in && + P=$(git pack-objects $packdir/pack in && + + git pack-objects --stdin-packs=follow $packdir/pack actual && + printf "%s\n" "$ZERO_OID" "$ZERO_OID" >expect && + test_cmp expect actual + ) +' + test_done diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh index b342e82447d..b49f22878f7 100755 --- a/t/t7704-repack-cruft.sh +++ b/t/t7704-repack-cruft.sh @@ -767,6 +767,26 @@ test_expect_success 'repack --write-midx excludes cruft where possible' ' ) ' +test_expect_success 'geometric repack rescues descendants of loose trees' ' + git init loose-tree-cruft && + ( + cd loose-tree-cruft && + git config repack.midxMustContainCruft false && + test_commit base && + blob=$(echo cruft | git hash-object -w --stdin) && + GIT_TEST_MULTI_PACK_INDEX=0 git repack --cruft -d && + + printf "100644 blob %s\tfile\n" "$blob" | git mktree && + GIT_TEST_MULTI_PACK_INDEX=0 git repack -d --geometric=2 \ + --write-midx --write-bitmap-index && + + test-tool read-midx --show-objects $objdir >midx && + cruft=$(ls $packdir/*.mtimes) && + test_grep ! "$(basename "$cruft" .mtimes).idx" midx && + test_grep "^$blob " midx + ) +' + test_expect_success 'repack --write-midx includes cruft when instructed' ' setup_cruft_exclude_tests exclude-cruft-when-instructed && ( -- 2.56.0.4.gbee41d2fc68 ``` ## Taylor Blau, 2026-09-30 01:28 Subject: [PATCH 3/4] repack: retain cruft packs in MIDXs after incremental repacks Message-ID: <1774fed77be11b37ce9eb4b7806f5f14539503fb.1790731662.git.me@ttaylorr.com> In-Reply-To: ``` An incremental repack can write a commit and tree into a new pack while leaving objects they reach in an existing cruft pack. For example, a commit can make a previously unreachable blob reachable again. Since 'repack' will invoke 'pack-objects' with '--incremental', it will not copy the blob out of its cruft pack. When the 'repack.midxMustContainCruft' configuration is set to "false", writing the first MIDX after such a repack may omit that cruft pack. The new pack bypasses the `!names.nr` fallback, and there are no previous MIDX packs for `midx_has_unknown_packs()` to check. Selecting the new commit for bitmap coverage then fails because its reachable objects are not all in the MIDX. The omission dates all the way back to 5ee86c273bf (repack: exclude cruft pack(s) from the MIDX where possible, 2025-06-23). It relies on geometric repacking to copy once-cruft objects with '--stdin-packs=follow'. However, an ordinary incremental repack makes no such guarantee. Require the MIDX to include cruft packs in that case, even when a new pack was written. Exercise this with the existing fixture that makes a cruft commit reachable again and adds a new (unpacked) commit on top, and ensure that the incremental repack is able to successfully write a reachability bitmap. Signed-off-by: Taylor Blau --- builtin/repack.c | 6 ++++++ t/t7704-repack-cruft.sh | 11 +++++++++++ 2 files changed, 17 insertions(+) diff --git a/builtin/repack.c b/builtin/repack.c index c4360382c1f..b7596d488da 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -539,6 +539,12 @@ int cmd_repack(int argc, strvec_push(&cmd.args, "--stdin-packs=follow"); strvec_push(&cmd.args, "--unpacked"); } else { + /* + * Incremental repacks do not copy already-packed objects, + * so cruft packs may be required to form a reachability + * closure for the MIDX. + */ + midx_must_contain_cruft = 1; strvec_push(&cmd.args, "--unpacked"); strvec_push(&cmd.args, "--incremental"); } diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh index b49f22878f7..f7f83e70ffe 100755 --- a/t/t7704-repack-cruft.sh +++ b/t/t7704-repack-cruft.sh @@ -787,6 +787,17 @@ test_expect_success 'geometric repack rescues descendants of loose trees' ' ) ' +test_expect_success 'incremental repack includes cruft for MIDX bitmaps' ' + setup_cruft_exclude_tests incremental-cruft && + ( + cd incremental-cruft && + + GIT_TEST_MULTI_PACK_INDEX=0 \ + git repack -d --write-midx --write-bitmap-index && + git rev-list --test-bitmap HEAD + ) +' + test_expect_success 'repack --write-midx includes cruft when instructed' ' setup_cruft_exclude_tests exclude-cruft-when-instructed && ( -- 2.56.0.4.gbee41d2fc68 ``` ## Taylor Blau, 2026-09-30 01:28 Subject: [PATCH 4/4] repack: retain cruft packs in MIDXs containing kept packs Message-ID: In-Reply-To: ``` When performing a geometric repack with 'repack.midxMustContainCruft' set to "false", Git uses '--stdin-packs=follow' to copy (once-cruft) objects needed for reachability closure out of cruft packs. .keep packs do not need to participate in that walk, though they *are* included in the resulting MIDX. A .keep pack can contain a commit that reaches an object whose only copy is in a cruft pack. When there is no previous MIDX and the repack writes a new pack, neither `midx_has_unknown_packs()` nor the `!names.nr` fallback require that cruft pack to be included. If the kept commit (or a descendant of it) is selected for bitmap coverage, the bitmap writer fails because the MIDX does not contain all of its reachable objects. Include cruft packs whenever the MIDX contains kept packs. This also retains cruft when the kept packs happen to have full closure, or when '--pack-kept-objects' lets the repack walk them. It avoids having to establish their closure before deciding which packs the MIDX needs. Add a test that packs the tip commit and its tree into a kept pack, leaving its parent in the cruft pack. The new commit's blob remains loose, making the geometric repack write a new pack and bypass the no-new-packs fallback. Verify that the repack succeeds and that we are able to successfully write a bitmap. Signed-off-by: Taylor Blau --- repack-midx.c | 5 +++++ t/t7704-repack-cruft.sh | 20 ++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/repack-midx.c b/repack-midx.c index 64c7f8d0f42..622c3c9d236 100644 --- a/repack-midx.c +++ b/repack-midx.c @@ -197,6 +197,7 @@ static void midx_included_packs(struct string_list *include, } if (opts->midx_must_contain_cruft || + existing->kept_packs.nr || midx_has_unknown_packs(include, geometry, existing)) { /* * If there are one or more unknown pack(s) present (see @@ -209,6 +210,10 @@ static void midx_included_packs(struct string_list *include, * reachability closure if the MIDX is bitmapped and one * or more of the bitmap's selected commits reaches a * once-cruft object that was later made reachable. + * + * Kept packs may also depend on cruft objects, since + * they are included above without necessarily being + * traversed by the repack. */ for_each_string_list_item(item, &existing->cruft_packs) { /* diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh index f7f83e70ffe..02db2a06d9e 100755 --- a/t/t7704-repack-cruft.sh +++ b/t/t7704-repack-cruft.sh @@ -798,6 +798,26 @@ test_expect_success 'incremental repack includes cruft for MIDX bitmaps' ' ) ' +test_expect_success 'geometric repack includes cruft for kept packs' ' + setup_cruft_exclude_tests kept-cruft && + ( + cd kept-cruft && + + # Keep HEAD and its tree outside the geometric repack. Its + # parent is reachable again, but still in the cruft pack. + git rev-parse HEAD HEAD^{tree} >objects && + pack=$(git pack-objects $packdir/pack In-Reply-To: <64bb13e2db2e5c22e842c188e08861d63e99dc77.1790731662.git.me@ttaylorr.com> ``` Taylor Blau writes: > static int add_object_entry_from_pack(const struct object_id *oid, > struct packed_git *p, > uint32_t pos, > void *_data) > { > + struct stdin_packs_context *ctx = _data; > off_t ofs; > struct object_info oi = OBJECT_INFO_INIT; > enum object_type type = OBJ_NONE; > @@ -3827,7 +3833,6 @@ static int add_object_entry_from_pack(const struct object_id *oid, > die(_("could not get type of object %s in pack %s"), > oid_to_hex(oid), p->pack_name); > } else if (type == OBJ_COMMIT) { > - struct rev_info *revs = _data; > /* > * commits in included packs are used as starting points > * for the subsequent revision walk > @@ -3840,7 +3845,7 @@ static int add_object_entry_from_pack(const struct object_id *oid, > * However, we'll only add those objects to the packing > * list after checking `want_object_in_pack()` below. > */ > - add_pending_oid(revs, NULL, oid, 0); > + add_pending_oid(ctx->revs, NULL, oid, 0); > } > > if (!want_object_in_pack(oid, 0, &p, &ofs)) > @@ -3954,8 +3959,9 @@ static int stdin_packs_include_check(struct commit *commit, void *data) > } We used to take _data that is rev_info, but no longer. We lost decl for "struct rev_info *revs" and rewrote its only use to directly reference ctx->revs. As long as the result compiles, we know there is no stray reference to "revs" left in this function, so the rewrite is complete. It is rare but I love this kind of patch whose correctness can be seen without reading beyond the context ;-) > static void stdin_packs_add_pack_entries(struct strmap *packs, > - struct rev_info *revs) > + struct stdin_packs_context *ctx) > { > + struct rev_info *revs = ctx->revs; > struct string_list keys = STRING_LIST_INIT_NODUP; > struct string_list_item *item; > struct hashmap_iter iter; > @@ -3994,15 +4000,14 @@ static void stdin_packs_add_pack_entries(struct strmap *packs, > (info->kind & STDIN_PACK_EXCLUDE_OPEN)) > for_each_object_in_pack(info->p, > add_object_entry_from_pack, > - revs, > + ctx, > ODB_FOR_EACH_OBJECT_PACK_ORDER); > } > > string_list_clear(&keys, 0); > } Ditto. > -static void stdin_packs_read_input(struct rev_info *revs, > - enum stdin_packs_mode mode) > +static void stdin_packs_read_input(struct stdin_packs_context *ctx) We used to take two separately, but now we can take them in one package. > { > struct strbuf buf = STRBUF_INIT; > struct strmap packs = STRMAP_INIT; > @@ -4017,7 +4022,7 @@ static void stdin_packs_read_input(struct rev_info *revs, > continue; > else if (*key == '^') > kind = STDIN_PACK_EXCLUDE_CLOSED; > - else if (*key == '!' && mode == STDIN_PACKS_MODE_FOLLOW) > + else if (*key == '!' && ctx->mode == STDIN_PACKS_MODE_FOLLOW) > kind = STDIN_PACK_EXCLUDE_OPEN; > > if (kind != STDIN_PACK_INCLUDE) > @@ -4082,19 +4087,23 @@ static void stdin_packs_read_input(struct rev_info *revs, > info->p = p; > } > > - stdin_packs_add_pack_entries(&packs, revs); > + stdin_packs_add_pack_entries(&packs, ctx); > > strbuf_release(&buf); > strmap_clear(&packs, 1); > } The same argument tells us that this is the right refactoring as long as the result compiles. > -static void add_unreachable_loose_objects(struct rev_info *revs); > +static void add_unreachable_loose_objects(struct stdin_packs_context *ctx); > > static void read_stdin_packs(struct repository *repo, > enum stdin_packs_mode mode, int rev_list_unpacked) > { > int prev_fetch_if_missing = repo->fetch_if_missing; > struct rev_info revs; > + struct stdin_packs_context ctx = { > + .revs = &revs, > + .mode = mode, > + }; > > /* > * The revision walk may hit objects that are promised, only. As the > @@ -4131,9 +4140,9 @@ static void read_stdin_packs(struct repository *repo, > */ > ignore_packed_keep_in_core_open = 1; > } > - stdin_packs_read_input(&revs, mode); > + stdin_packs_read_input(&ctx); > if (rev_list_unpacked) > - add_unreachable_loose_objects(&revs); > + add_unreachable_loose_objects(&ctx); > > if (prepare_revision_walk(&revs)) > die(_("revision walk setup failed")); Ditto. > @@ -4541,7 +4550,7 @@ static void add_objects_in_unpacked_packs(void) > static int add_loose_object(const struct object_id *oid, const char *path, > void *data) > { > - struct rev_info *revs = data; > + struct stdin_packs_context *ctx = data; > enum object_type type = odb_read_object_info(the_repository->objects, oid, NULL); > > if (type < 0) { > @@ -4563,8 +4572,8 @@ static int add_loose_object(const struct object_id *oid, const char *path, > add_object_entry(oid, type, "", 0); > } > > - if (revs && type == OBJ_COMMIT) > - add_pending_oid(revs, NULL, oid, 0); > + if (ctx && type == OBJ_COMMIT) > + add_pending_oid(ctx->revs, NULL, oid, 0); > > return 0; > } This one, ... > @@ -4574,10 +4583,10 @@ static int add_loose_object(const struct object_id *oid, const char *path, > * add_object_entry will weed out duplicates, so we just add every > * loose object we find. > */ > -static void add_unreachable_loose_objects(struct rev_info *revs) > +static void add_unreachable_loose_objects(struct stdin_packs_context *ctx) > { > for_each_loose_file_in_source(the_repository->objects->sources, > - add_loose_object, NULL, NULL, revs); > + add_loose_object, NULL, NULL, ctx); > } ... together with the change to add_unreachable_loose_objects() here, it is not immediately obvious if we do not have to worry about the case where (ctx && !ctx->revs). Given that 'struct stdin_packs_context' is a new structure, the fact that the instantiation on the stack in read_stdin_packs() is the only one that can give us a non-NULL 'ctx' pointer we can see in this patch means that a non-NULL 'ctx' cannot have a NULL '.revs' pointer in it. Again, as long as this patch alone compiles, we know this refactoring is correct. It is not clear to me what the implication of assuming a non-NULL 'ctx' always means a non-NULL 'ctx->revs' is for the code health in the longer term, though. Thanks. ``` ## Junio C Hamano, 2026-09-30 17:51 Subject: Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow' Message-ID: In-Reply-To: <6348667e2e3fe63aeb139888e877dd8447570253.1790731662.git.me@ttaylorr.com> ``` Taylor Blau writes: > @@ -4574,6 +4603,9 @@ static int add_loose_object(const struct object_id *oid, const char *path, > > if (ctx && type == OBJ_COMMIT) > add_pending_oid(ctx->revs, NULL, oid, 0); > + else if (ctx && ctx->mode == STDIN_PACKS_MODE_FOLLOW && > + (type == OBJ_TREE || type == OBJ_TAG)) > + oid_array_append(&ctx->extra_roots, oid); > > return 0; > } Makes me wonder if this function will always end with "if ctx is not NULL, then depending on these conditions do one more thing" like this, or if it would change later. If the former, if (!ctx) return 0; if (type == OBJ_COMMIT) do the commit thing; else if (ctx->mode == follow && type in (tree, tag)) do the tag or tree thing; return 0; might be easier to follow, perhaps? ``` ## Derrick Stolee, 2026-09-30 18:16 Subject: Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow' Message-ID: In-Reply-To: <6348667e2e3fe63aeb139888e877dd8447570253.1790731662.git.me@ttaylorr.com> ``` On 9/29/2026 9:28 PM, Taylor Blau wrote: > Add trees and tags from included and '!' packs (and loose ones with > '--unpacked') as roots in '--stdin-packs=follow' mode. This rescues > their descendants even when no input commit reaches them. Walk these > roots after the existing traversal, preserving the `SEEN` bit to avoid > redundant traversals. Ensure that the walk takes place *after* the > existing traversal so that we don't lose the path prefix used for trees > and blobs wherever possible. > @@ -3807,6 +3807,7 @@ static int stdin_packs_hints_nr; > struct stdin_packs_context { > struct rev_info *revs; > enum stdin_packs_mode mode; > + struct oid_array extra_roots; I believe this should be an oidset to avoid adding duplicate objects that appear multiple times. The order of these extra roots doesn't matter (such as in a --topo-order walk). We only care about the binary "reachable or not?" question. Thanks, -Stolee ``` ## Jeff King, 2026-09-30 20:31 Subject: Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow' Message-ID: <20260930203108.GA747209@coredump.intra.peff.net> In-Reply-To: <6348667e2e3fe63aeb139888e877dd8447570253.1790731662.git.me@ttaylorr.com> ``` On Tue, Sep 29, 2026 at 08:28:49PM -0500, Taylor Blau wrote: > In cd846bacc7d (pack-objects: introduce '--stdin-packs=follow', > 2025-06-23), this behavior changed such that whenever excluded-open > ('!') packs are present, the walk stops at objects in excluded-closed > ('^') packs. Geometric repacks use '^' for retained packs already in the > MIDX, relying on the indexed object set being closed under reachability. > > However, the walk introduced in cd846bacc7d starts only from commit > objects. A geometric repack can therefore produce a MIDX that does not > maintain reachability closure for lone trees (that are not reachable > from any commit otherwise in the closure). > > A later walk with '!' packs can stop at that tree in a retained '^' > pack even if a new commit reaches it. If the cruft pack remains > excluded, and the bitmap selection picks one or more commits which reach > that tree, the MIDX cannot generate a bitmap for that commit. OK. It took me a minute to grok this, and what I got hung up on is "a later walk". I thought you meant a later walk within the same process, but you mean "a subsequent repack / midx generation". So we fail to walk in an earlier repack, but we might not fail there because no bitmapped commit happens to require that closure. But we've set up a timebomb for that later repack, because our pack which is _supposed_ to be closed (and thus gets marked with "^") is broken. So this fixes the initial generation of that timebomb. It doesn't help us deal with existing bombs, but presumably the solution there is a full repack (and we would not want to deal with existing bombs, because the point of "^" is that we can trust it and avoid lots of extra traversal). Not really asking for a change to the commit message, but just documenting my understanding (which hopefully matches yours ;) ). > Add trees and tags from included and '!' packs (and loose ones with > '--unpacked') as roots in '--stdin-packs=follow' mode. This rescues > their descendants even when no input commit reaches them. Walk these > roots after the existing traversal, preserving the `SEEN` bit to avoid > redundant traversals. Ensure that the walk takes place *after* the > existing traversal so that we don't lose the path prefix used for trees > and blobs wherever possible. OK, that makes sense, as we should treat them the same as commits. > @@ -3846,6 +3847,9 @@ static int add_object_entry_from_pack(const struct object_id *oid, > * list after checking `want_object_in_pack()` below. > */ > add_pending_oid(ctx->revs, NULL, oid, 0); > + } else if (ctx->mode == STDIN_PACKS_MODE_FOLLOW && > + (type == OBJ_TREE || type == OBJ_TAG)) { > + oid_array_append(&ctx->extra_roots, oid); > } And this is the interesting part. What about blobs? I guess we don't care about them because they are either there or not. There is no need to walk them independently because they can't reference anything. Why do we need a separate extra_roots here, rather than just using add_pending_oid()? I'd have thought we'd add it all to the same ("--objects") walk. I guess that is explained here: > + /* > + * Trees and tags need closure even when no commit reaches them. > + * Defer adding these roots to revs.pending until the commit walk > + * finishes. Otherwise a subtree may be visited and marked SEEN > + * before its commit's root tree, using "a" instead of "sub/a" for > + * a blob's namehash and delta attributes. > + */ > + for (size_t i = 0; i < ctx.extra_roots.nr; i++) { > + const struct object_id *oid = &ctx.extra_roots.oid[i]; > + struct object *obj = lookup_object(repo, oid); > + > + if (!obj || !(obj->flags & SEEN)) > + add_pending_oid(&revs, NULL, oid, 0); > + } but I'm not sure I buy it. Don't we always visit the commits first in a walk? So a single walk with all of the proposed objects would be fine? If I understand this subtree claim, you are worried about the (single-traversal) case that we manually queue tree A, and then later visit commit C, which eventually has A as a sub-tree. So we queue A again _after_ its original, but that second visit (that we skip) would have had more interesting information (like path context). But I don't think a second walk clears you of that possibility. You are queuing tags, too, which might in turn point to commits. So you might get the same commit traversal within that second walk. I think you could fix it by putting tags into the first walk. But it will always exist to some degree (you could have a tag that points to a tree and queue that tree, but also a commit that points to it). It's not clear to me how big a problem this is in practice. We know that the "path" of a tree or blob in a traversal is subject to context. There might be multiple commits that point to it at different levels. I guess it might be more common if we are adding random trees from a pack without context. I think the more complete solution there is not two walks, but that the traversal machinery should queue context-ful trees ahead of low-context ones. I don't think we want to make the queue a stack (that would change the output considerably), so you'd probably need to keep a separate queue of low-context objects, and drain it only after the high-context ones we get from traversing the commits. I certainly think this patch is a strict improvement, and should fix the main bug. It can't make anything worse for these extra trees and tags, because we weren't even including them before. ;) But I think the subtle side-bug here is not a complete fix (though I do think it is strictly better than doing nothing). So I dunno. I'd probably be OK proceeding with this as-is, because I fear that dual-queue thing I mentioned above might turn into a rabbit hole that would derail the much more important fix. -Peff ``` ## Jeff King, 2026-09-30 20:45 Subject: Re: [PATCH 3/4] repack: retain cruft packs in MIDXs after incremental repacks Message-ID: <20260930204529.GB747209@coredump.intra.peff.net> In-Reply-To: <1774fed77be11b37ce9eb4b7806f5f14539503fb.1790731662.git.me@ttaylorr.com> ``` On Tue, Sep 29, 2026 at 08:28:53PM -0500, Taylor Blau wrote: > When the 'repack.midxMustContainCruft' configuration is set to "false", > writing the first MIDX after such a repack may omit that cruft pack. The > new pack bypasses the `!names.nr` fallback, and there are no previous > MIDX packs for `midx_has_unknown_packs()` to check. Selecting the new > commit for bitmap coverage then fails because its reachable objects are > not all in the MIDX. > > The omission dates all the way back to 5ee86c273bf (repack: exclude > cruft pack(s) from the MIDX where possible, 2025-06-23). It relies on > geometric repacking to copy once-cruft objects with > '--stdin-packs=follow'. However, an ordinary incremental repack makes no > such guarantee. Require the MIDX to include cruft packs in that case, > even when a new pack was written. OK. So this is a problem with just incremental repacks, but _not_ geometric repacks? And only when those incremental repacks write a midx? If so, that makes sense to me (and the fix seems reasonable). BTW, write_midx_incremental() does not check midx_must_contain_cruft. So I think you'd have the same problem with --write-midx=incremental. Adding that to the tests causes them to fail. I thought it might also fail with GIT_TEST_MULTI_PACK_INDEX_WRITE_INCREMENTAL=1, but doesn't seem to. That's not a new problem, but just a spot where the fix doesn't extend. Not sure how important it is to do now, or if it can wait for future work. -Peff ``` ## Jeff King, 2026-09-30 20:53 Subject: Re: [PATCH 4/4] repack: retain cruft packs in MIDXs containing kept packs Message-ID: <20260930205311.GC747209@coredump.intra.peff.net> In-Reply-To: ``` On Tue, Sep 29, 2026 at 08:28:58PM -0500, Taylor Blau wrote: > When performing a geometric repack with 'repack.midxMustContainCruft' > set to "false", Git uses '--stdin-packs=follow' to copy (once-cruft) > objects needed for reachability closure out of cruft packs. .keep packs > do not need to participate in that walk, though they *are* included in > the resulting MIDX. > > A .keep pack can contain a commit that reaches an object whose only copy > is in a cruft pack. When there is no previous MIDX and the repack writes > a new pack, neither `midx_has_unknown_packs()` nor the `!names.nr` > fallback require that cruft pack to be included. If the kept commit (or > a descendant of it) is selected for bitmap coverage, the bitmap writer > fails because the MIDX does not contain all of its reachable objects. > > Include cruft packs whenever the MIDX contains kept packs. This also > retains cruft when the kept packs happen to have full closure, or when > '--pack-kept-objects' lets the repack walk them. It avoids having to > establish their closure before deciding which packs the MIDX needs. OK. This makes sense to me, but two questions: 1. Is this going to kick in racily because of the .keep that we temporarily install during pushes? That could cause unexpected performance changes in a big repo when the midx sometimes has to randomly include cruft packs. 2. I'd have thought that the solution would be to treat .keep packs like other included follow-packs: traverse them in the usual way. But maybe there are good reasons we didn't do that in the first place. -Peff ``` ## Jeff King, 2026-09-30 20:55 Subject: Re: [PATCH 0/4] repack: various corner cases for cruft-less MIDXs Message-ID: <20260930205535.GD747209@coredump.intra.peff.net> In-Reply-To: ``` On Tue, Sep 29, 2026 at 08:28:34PM -0500, Taylor Blau wrote: > This patch series fixes a few bugs I spotted while investigating the > cruft-less MIDX feature. > > The bugs addressed are found in various corner cases, and, when > triggered, may result in a MIDX being written whose objects are not > closed under reachability. When this happens while the caller is trying > to write reachability bitmaps, bitmap generation may fail if one or more > selected commits are descendants of the open portion of the MIDX. I think all of these are making things strictly better, but I did find a few spots where the fixes might be incomplete. I'm not sure if that argues for a re-roll or for punting those to future work. ;) I agree with Stolee that an oidset is perhaps a better data structure for storing the extra roots (which are in a kind-of random order anyway, since we're pulling them in pack order from various packs). But it also probably doesn't make that big a difference in practice (we'll skip duplicates during the traversal, and you probably don't have that many duplicate objects in a repo in the first place). -Peff ``` ## Taylor Blau, 2026-10-01 03:13 Subject: Re: [PATCH 1/4] pack-objects: introduce `stdin_packs_context` struct Message-ID: In-Reply-To: ``` On Wed, Sep 30, 2026 at 10:42:10AM -0700, Junio C Hamano wrote: > We used to take _data that is rev_info, but no longer. We lost decl > for "struct rev_info *revs" and rewrote its only use to directly > reference ctx->revs. As long as the result compiles, we know there > is no stray reference to "revs" left in this function, so the > rewrite is complete. It is rare but I love this kind of patch whose > correctness can be seen without reading beyond the context ;-) ;-) > It is not clear to me what the implication of assuming a non-NULL > 'ctx' always means a non-NULL 'ctx->revs' is for the code health in > the longer term, though. That's fair. For the following round, I added a small note next to the 'revs' member in the struct's definition to indicate that it must be non-NULL. Thanks, Taylor ``` ## Taylor Blau, 2026-10-01 03:14 Subject: Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow' Message-ID: In-Reply-To: ``` On Wed, Sep 30, 2026 at 02:16:53PM -0400, Derrick Stolee wrote: > > @@ -3807,6 +3807,7 @@ static int stdin_packs_hints_nr; > > struct stdin_packs_context { > > struct rev_info *revs; > > enum stdin_packs_mode mode; > > + struct oid_array extra_roots; > > I believe this should be an oidset to avoid adding duplicate objects > that appear multiple times. The order of these extra roots doesn't > matter (such as in a --topo-order walk). We only care about the > binary "reachable or not?" question. Great suggestion! I adjusted it in the following round. Thanks, Taylor ``` ## Taylor Blau, 2026-10-01 03:18 Subject: Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow' Message-ID: In-Reply-To: <20260930203108.GA747209@coredump.intra.peff.net> ``` On Wed, Sep 30, 2026 at 04:31:08PM -0400, Jeff King wrote: > On Tue, Sep 29, 2026 at 08:28:49PM -0500, Taylor Blau wrote: > > > In cd846bacc7d (pack-objects: introduce '--stdin-packs=follow', > > 2025-06-23), this behavior changed such that whenever excluded-open > > ('!') packs are present, the walk stops at objects in excluded-closed > > ('^') packs. Geometric repacks use '^' for retained packs already in the > > MIDX, relying on the indexed object set being closed under reachability. > > > > However, the walk introduced in cd846bacc7d starts only from commit > > objects. A geometric repack can therefore produce a MIDX that does not > > maintain reachability closure for lone trees (that are not reachable > > from any commit otherwise in the closure). > > > > A later walk with '!' packs can stop at that tree in a retained '^' > > pack even if a new commit reaches it. If the cruft pack remains > > excluded, and the bitmap selection picks one or more commits which reach > > that tree, the MIDX cannot generate a bitmap for that commit. > > OK. It took me a minute to grok this, and what I got hung up on is "a > later walk". I thought you meant a later walk within the same process, > but you mean "a subsequent repack / midx generation". > > So we fail to walk in an earlier repack, but we might not fail there > because no bitmapped commit happens to require that closure. But we've > set up a timebomb for that later repack, because our pack which is > _supposed_ to be closed (and thus gets marked with "^") is broken. > > So this fixes the initial generation of that timebomb. It doesn't help > us deal with existing bombs, but presumably the solution there is a full > repack (and we would not want to deal with existing bombs, because the > point of "^" is that we can trust it and avoid lots of extra traversal). > > Not really asking for a change to the commit message, but just > documenting my understanding (which hopefully matches yours ;) ). Yup, exactly. Hopefully s/walk/repack/ clarifies things for the following round, but in the meantime your understanding matches my own. When this feature was originally introduced, the idea was "anything packed must also pack its reachability closure, less any objects in excluded packs". That was true for commit objects, but not so for trees and annotated tags, which is what this patch corrects. > > @@ -3846,6 +3847,9 @@ static int add_object_entry_from_pack(const struct object_id *oid, > > * list after checking `want_object_in_pack()` below. > > */ > > add_pending_oid(ctx->revs, NULL, oid, 0); > > + } else if (ctx->mode == STDIN_PACKS_MODE_FOLLOW && > > + (type == OBJ_TREE || type == OBJ_TAG)) { > > + oid_array_append(&ctx->extra_roots, oid); > > } > > And this is the interesting part. What about blobs? I guess we don't > care about them because they are either there or not. There is no need > to walk them independently because they can't reference anything. Exactly. > If I understand this subtree claim, you are worried about the > (single-traversal) case that we manually queue tree A, and then later > visit commit C, which eventually has A as a sub-tree. So we queue A > again _after_ its original, but that second visit (that we skip) would > have had more interesting information (like path context). > > But I don't think a second walk clears you of that possibility. You are > queuing tags, too, which might in turn point to commits. So you might > get the same commit traversal within that second walk. Yeah, that's what I was worried about when I wrote this patch, but that's a good point. Really there is no "absolute" correct path for a given tree or tree entry, since it depends on your perspective. > I think you could fix it by putting tags into the first walk. But it > will always exist to some degree (you could have a tag that points to a > tree and queue that tree, but also a commit that points to it). > > It's not clear to me how big a problem this is in practice. We know that > the "path" of a tree or blob in a traversal is subject to context. There > might be multiple commits that point to it at different levels. I guess > it might be more common if we are adding random trees from a pack > without context. ;-). > So I dunno. I'd probably be OK proceeding with this as-is, because I > fear that dual-queue thing I mentioned above might turn into a rabbit > hole that would derail the much more important fix. I tightened up the comment a bit, but I agree that rethinking the traversal machinery is best left for another day. Thanks, Taylor ``` ## Taylor Blau, 2026-10-01 03:21 Subject: Re: [PATCH 3/4] repack: retain cruft packs in MIDXs after incremental repacks Message-ID: In-Reply-To: <20260930204529.GB747209@coredump.intra.peff.net> ``` On Wed, Sep 30, 2026 at 04:45:29PM -0400, Jeff King wrote: > That's not a new problem, but just a spot where the fix doesn't extend. > Not sure how important it is to do now, or if it can wait for future > work. Yeah, good point. I have a fix for it in the subsequent round, though it did make the overall series longer to accommodate predatory patches that make the substantive ones easier to grok. I think that the result is sound, hence its inclusion in v2. I briefly considered dropping that part of this series altogether to deal with it another day. But doing so felt irresponsible as the earlier round pointed out the existence of a bug, the subsequent round should not ignore it. On the other hand, as you note, it's not a new problem relative to this series, and so perhaps leaving it out wouldn't have been so bad. But I think all thing equal, the patches exist, and I think that they are sound, so I figured that I'd send them in the following round for completeness. The maintainer should however, feel free to avoid queueing that part of the series if we want to punt on it for now. Thanks, Taylor ``` ## Taylor Blau, 2026-10-01 03:35 Subject: Re: [PATCH 4/4] repack: retain cruft packs in MIDXs containing kept packs Message-ID: In-Reply-To: <20260930205311.GC747209@coredump.intra.peff.net> ``` On Wed, Sep 30, 2026 at 04:53:11PM -0400, Jeff King wrote: > OK. This makes sense to me, but two questions: > > 1. Is this going to kick in racily because of the .keep that we > temporarily install during pushes? That could cause unexpected > performance changes in a big repo when the midx sometimes has to > randomly include cruft packs. Excellent point. If we happen to race between repacking and receiving an incoming push, it is certainly not the case that we should have to then thrust the cruft pack onto the MIDX, and defeat the whole point of this series ;-). > 2. I'd have thought that the solution would be to treat .keep packs > like other included follow-packs: traverse them in the usual way. > But maybe there are good reasons we didn't do that in the first > place. Yeah, I think that's right. The following round passes excluded kept packs into the geometric repack's traversal using '!' for packs outside the existing MIDX and '^' for those already covered by it. That lets us copy their ancestors out of cruft without copying the kept objects themselves. Non-geometric repacks still conservatively retain cruft, since kept objects may remain untraversed. There is, however, a pre-existing corner case with --pack-kept-objects and a cruft pack that is also kept. Such a pack can enter the MIDX without being traversed. This change doesn't address that case. Handling it cleanly looks more involved, so I'd leave it for a separate follow-up. Thanks, Taylor ``` ## Taylor Blau, 2026-10-01 03:37 Subject: Re: [PATCH 0/4] repack: various corner cases for cruft-less MIDXs Message-ID: In-Reply-To: <20260930205535.GD747209@coredump.intra.peff.net> ``` On Wed, Sep 30, 2026 at 04:55:35PM -0400, Jeff King wrote: > On Tue, Sep 29, 2026 at 08:28:34PM -0500, Taylor Blau wrote: > > > This patch series fixes a few bugs I spotted while investigating the > > cruft-less MIDX feature. > > > > The bugs addressed are found in various corner cases, and, when > > triggered, may result in a MIDX being written whose objects are not > > closed under reachability. When this happens while the caller is trying > > to write reachability bitmaps, bitmap generation may fail if one or more > > selected commits are descendants of the open portion of the MIDX. > > I think all of these are making things strictly better, but I did find a > few spots where the fixes might be incomplete. I'm not sure if that > argues for a re-roll or for punting those to future work. ;) Thanks for the review. Like I wrote in my response to your review, leaving it half-fixed felt dishonest, so fixes are included in the subsequent round, though they do make the series a little longer. There is a separate, pre-existing bug that I would like to fix outside of this series, but only because it (a) is independent of this series, and (b) I estimate that the fix is considerably more complex. > I agree with Stolee that an oidset is perhaps a better data structure > for storing the extra roots (which are in a kind-of random order anyway, > since we're pulling them in pack order from various packs). But it also > probably doesn't make that big a difference in practice (we'll skip > duplicates during the traversal, and you probably don't have that many > duplicate objects in a repo in the first place). Yup. Thanks, Taylor ``` ## Taylor Blau, 2026-10-01 04:11 Subject: [PATCH v2 0/8] repack: various corner cases for cruft-less MIDXs Message-ID: In-Reply-To: ``` This is a reroll of [1]. Thanks to Stolee, Junio, and Peff for reviewing. With 'repack.midxMustContainCruft=false', geometric repacks copy needed objects out of cruft packs before omitting those packs from the MIDX. The follow walk can stop at objects in retained MIDX packs, relying on the indexed set being closed under reachability. That invariant must hold for directly enumerated trees and tags, too. A subsequent repack may introduce a commit that reaches a tree already in a retained pack. The series adds those tree and tag roots, retains cruft for ordinary incremental repacks, and includes the required packs when writing either an ordinary or incremental MIDX. These omissions can otherwise make bitmap generation fail even though all reachable objects remain available in the repository. Changes since v1: - Use an oidset for the additional tree and tag roots, as Stolee suggested, avoiding duplicate entries when an object appears in several input packs. - Address Junio's comments by documenting that a non-NULL stdin_packs_context requires a non-NULL revs pointer, and by returning early from add_loose_object() after common handling when there is no context. - Following Peff's review, clarify that the tree-closure failure occurs in a subsequent repack. Describe the two walks precisely: directly enumerated commits go first, but deferred tags can introduce commits in the second walk, so path context remains best-effort. Existing MIDXs lacking closure still require a full repack. - Follow Peff's suggestion to traverse kept packs during geometric repacks, instead of always retaining cruft whenever kept packs exist. A temporary .keep installed by a push should not by itself require retaining cruft. The tests distinguish '--pack-kept-objects' from an explicit '--keep-pack'. - Fix the separate '--write-midx=incremental' issue Peff identified. Both cases include required packs outside the retained chain, including packs from a replaced tip. Existing append coverage now writes and verifies bitmaps; separate regressions cover repacking without producing a new pack and replacing a tip containing cruft. Thanks in advance for your review! Thanks, Taylor [1] https://lore.kernel.org/git/cover.1790731662.git.me@ttaylorr.com/ Taylor Blau (8): pack-objects: introduce `stdin_packs_context` struct pack-objects: ensure tree/tag closure with '--stdin-packs=follow' repack: retain cruft packs in MIDXs after incremental repacks repack: use a sorted list for explicitly kept packs repack: follow kept packs when omitting cruft from the MIDX repack: track the preferred pack explicitly in MIDX write steps repack: defer allocating the append plan's write step repack: include required packs in incremental MIDX writes Documentation/git-pack-objects.adoc | 2 + Documentation/git-repack.adoc | 5 +- builtin/pack-objects.c | 82 +++++++++++++++---- builtin/repack.c | 40 ++++++++- repack-midx.c | 123 +++++++++++++++++++--------- repack.c | 8 +- repack.h | 4 + t/t5331-pack-objects-stdin.sh | 81 ++++++++++++++++++ t/t7704-repack-cruft.sh | 74 +++++++++++++++++ t/t7705-repack-incremental-midx.sh | 63 +++++++++++--- 10 files changed, 404 insertions(+), 78 deletions(-) Range-diff against v1: 1: fcc07ede9a0 ! 1: 354c29cae73 pack-objects: introduce `stdin_packs_context` struct @@ Commit message object enumeration callbacks. Signed-off-by: Taylor Blau - Signed-off-by: Taylor Blau ## builtin/pack-objects.c ## @@ builtin/pack-objects.c: static int git_pack_config(const char *k, const char *v, @@ builtin/pack-objects.c: static int git_pack_config(const char *k, const char *v, static int stdin_packs_hints_nr; +struct stdin_packs_context { -+ struct rev_info *revs; ++ struct rev_info *revs; /* must be non-NULL */ + enum stdin_packs_mode mode; +}; + 2: 41448dca614 ! 2: 940953e5c40 pack-objects: ensure tree/tag closure with '--stdin-packs=follow' @@ Commit message maintain reachability closure for lone trees (that are not reachable from any commit otherwise in the closure). - A later walk with '!' packs can stop at that tree in a retained '^' - pack even if a new commit reaches it. If the cruft pack remains + A subsequent repack with '!' packs can stop at that tree in a retained + '^' pack even if a new commit reaches it. If the cruft pack remains excluded, and the bitmap selection picks one or more commits which reach that tree, the MIDX cannot generate a bitmap for that commit. @@ Commit message '--unpacked') as roots in '--stdin-packs=follow' mode. This rescues their descendants even when no input commit reaches them. Walk these roots after the existing traversal, preserving the `SEEN` bit to avoid - redundant traversals. Ensure that the walk takes place *after* the - existing traversal so that we don't lose the path prefix used for trees - and blobs wherever possible. + redundant traversals. This gives directly enumerated commits priority + for the path prefixes used by name hashes and delta attributes. Tags can + introduce commits in the second walk, so path selection remains + best-effort. + + Collect the extra roots in an oidset to avoid queuing duplicates. This + uses memory for each distinct root and walks its unvisited descendants. Objects in '^' packs remain cutoffs to avoid rewalking packs that are - known to be closed under reachability. + known to be closed under reachability, provided '!' packs are present. + This does not repair existing MIDXs lacking closure; those need a full + repack. Signed-off-by: Taylor Blau - Signed-off-by: Taylor Blau ## Documentation/git-pack-objects.adoc ## @@ Documentation/git-pack-objects.adoc: pack may include additional objects based on the following: @@ Documentation/git-pack-objects.adoc: pack may include additional objects based o ## builtin/pack-objects.c ## @@ builtin/pack-objects.c: static int stdin_packs_hints_nr; struct stdin_packs_context { - struct rev_info *revs; + struct rev_info *revs; /* must be non-NULL */ enum stdin_packs_mode mode; -+ struct oid_array extra_roots; ++ struct oidset extra_roots; }; static int add_object_entry_from_pack(const struct object_id *oid, @@ builtin/pack-objects.c: static int add_object_entry_from_pack(const struct objec add_pending_oid(ctx->revs, NULL, oid, 0); + } else if (ctx->mode == STDIN_PACKS_MODE_FOLLOW && + (type == OBJ_TREE || type == OBJ_TAG)) { -+ oid_array_append(&ctx->extra_roots, oid); ++ oidset_insert(&ctx->extra_roots, oid); } if (!want_object_in_pack(oid, 0, &p, &ofs)) @@ builtin/pack-objects.c: static void read_stdin_packs(struct repository *repo, struct stdin_packs_context ctx = { .revs = &revs, .mode = mode, -+ .extra_roots = OID_ARRAY_INIT, ++ .extra_roots = OIDSET_INIT, }; ++ struct oidset_iter iter; ++ const struct object_id *oid; /* + * The revision walk may hit objects that are promised, only. As the @@ builtin/pack-objects.c: static void read_stdin_packs(struct repository *repo, show_object_pack_hint, &mode); + /* + * Trees and tags need closure even when no commit reaches them. -+ * Defer adding these roots to revs.pending until the commit walk ++ * Defer adding these roots to revs.pending until the first walk + * finishes. Otherwise a subtree may be visited and marked SEEN -+ * before its commit's root tree, using "a" instead of "sub/a" for -+ * a blob's namehash and delta attributes. ++ * before its commit's root tree, using "a" instead of "sub/a" ++ * for a blob's namehash and delta attributes. ++ * ++ * Tags may introduce more commits in the second walk, so this ++ * does not *always* guarantee that trees are always visited ++ * with their full paths. + */ -+ for (size_t i = 0; i < ctx.extra_roots.nr; i++) { -+ const struct object_id *oid = &ctx.extra_roots.oid[i]; ++ oidset_iter_init(&ctx.extra_roots, &iter); ++ while ((oid = oidset_iter_next(&iter))) { + struct object *obj = lookup_object(repo, oid); + + if (!obj || !(obj->flags & SEEN)) @@ builtin/pack-objects.c: static void read_stdin_packs(struct repository *repo, + show_object_pack_hint, + &mode); + } -+ oid_array_clear(&ctx.extra_roots); ++ oidset_clear(&ctx.extra_roots); + release_revisions(&revs); trace2_data_intmax("pack-objects", the_repository, "stdin_packs_found", @@ builtin/pack-objects.c: static int add_loose_object(const struct object_id *oid, const char *path, + add_object_entry(oid, type, "", 0); + } - if (ctx && type == OBJ_COMMIT) +- if (ctx && type == OBJ_COMMIT) ++ if (!ctx) ++ return 0; ++ ++ if (type == OBJ_COMMIT) add_pending_oid(ctx->revs, NULL, oid, 0); -+ else if (ctx && ctx->mode == STDIN_PACKS_MODE_FOLLOW && ++ else if (ctx->mode == STDIN_PACKS_MODE_FOLLOW && + (type == OBJ_TREE || type == OBJ_TAG)) -+ oid_array_append(&ctx->extra_roots, oid); ++ oidset_insert(&ctx->extra_roots, oid); return 0; } 3: 72f49ff37bb ! 3: a244b26030c repack: retain cruft packs in MIDXs after incremental repacks @@ Commit message such guarantee. Require the MIDX to include cruft packs in that case, even when a new pack was written. + This fixes ordinary '--write-midx'. The separate + '--write-midx=incremental' writer does not consult this flag and needs + its own handling. + Exercise this with the existing fixture that makes a cruft commit reachable again and adds a new (unpacked) commit on top, and ensure that the incremental repack is able to successfully write a reachability bitmap. Signed-off-by: Taylor Blau - Signed-off-by: Taylor Blau ## builtin/repack.c ## @@ builtin/repack.c: int cmd_repack(int argc, -: ----------- > 4: c1ff18bf913 repack: use a sorted list for explicitly kept packs -: ----------- > 5: 51e20444dac repack: follow kept packs when omitting cruft from the MIDX -: ----------- > 6: a85dbcd04c7 repack: track the preferred pack explicitly in MIDX write steps -: ----------- > 7: 4a6504629a2 repack: defer allocating the append plan's write step 4: a10e3aa86c1 ! 8: a42f775cbe2 repack: retain cruft packs in MIDXs containing kept packs @@ Metadata Author: Taylor Blau ## Commit message ## - repack: retain cruft packs in MIDXs containing kept packs + repack: include required packs in incremental MIDX writes - When performing a geometric repack with 'repack.midxMustContainCruft' - set to "false", Git uses '--stdin-packs=follow' to copy (once-cruft) - objects needed for reachability closure out of cruft packs. .keep packs - do not need to participate in that walk, though they *are* included in - the resulting MIDX. + The append plan introduced in 06733a50eee (repack: allow + `--write-midx=incremental` without `--geometric`, 2026-05-19) adds only + newly written packs to the existing MIDX chain. The bitmap writer can + use objects from the new layer and all retained base layers, but the + plan omits preexisting packs outside the chain. Bitmap generation fails + if a selected commit reaches an object absent from the resulting chain. - A .keep pack can contain a commit that reaches an object whose only copy - is in a cruft pack. When there is no previous MIDX and the repack writes - a new pack, neither `midx_has_unknown_packs()` nor the `!names.nr` - fallback require that cruft pack to be included. If the kept commit (or - a descendant of it) is selected for bitmap coverage, the bitmap writer - fails because the MIDX does not contain all of its reachable objects. + The geometric plan from 1da62fb5c86 (repack: implement incremental MIDX + repacking, 2026-05-19) can omit kept and cruft packs, since neither + necessarily participates in the geometric repack. Such packs can also be + lost when replacing a tip layer that contains them. Neither plan + consults `midx_included_packs()`, so the rules for retaining cruft in + ordinary MIDX writes do not protect incremental writes. - Include cruft packs whenever the MIDX contains kept packs. This also - retains cruft when the kept packs happen to have full closure, or when - '--pack-kept-objects' lets the repack walk them. It avoids having to - establish their closure before deciding which packs the MIDX needs. + Use that selection logic to add missing packs to each plan's write step. + Skip packs in retained base layers, but include required packs from a + replaced tip. Count added objects when choosing which layers to compact, + without changing the preferred pack. - Add a test that packs the tip commit and its tree into a kept pack, - leaving its parent in the cruft pack. The new commit's blob remains - loose, making the geometric repack write a new pack and bypass the - no-new-packs fallback. Verify that the repack succeeds and that we are - able to successfully write a bitmap. + Write and verify bitmaps in the existing append test: its existing + checks do not detect the omitted pack containing the first commit. Cover + the no-new-pack case separately with a reachable blob in a cruft pack. Signed-off-by: Taylor Blau - Signed-off-by: Taylor Blau + + ## Documentation/git-repack.adoc ## +@@ Documentation/git-repack.adoc: linkgit:git-multi-pack-index[1]). + flat MIDX. + + + Without `--geometric`, a new MIDX layer is appended to the existing +-chain (or a new chain is started) containing whatever packs were written +-by the repack. Existing layers are preserved as-is. ++chain (or a new chain is started) containing newly written packs and any ++other required packs not already in the chain. Existing layers are ++preserved as-is. + + + When combined with `--geometric`, the incremental mode maintains a chain + of MIDX layers that is compacted over time using a geometric merging ## repack-midx.c ## +@@ + #include "odb.h" + #include "oidset.h" + #include "pack-bitmap.h" ++#include "packfile.h" + #include "path.h" + #include "refs.h" + #include "run-command.h" +@@ repack-midx.c: void midx_snapshot_refs(struct repository *repo, struct tempfile *f) + + static int midx_has_unknown_packs(struct string_list *include, + struct pack_geometry *geometry, +- struct existing_packs *existing) ++ struct existing_packs *existing, ++ struct multi_pack_index *base) + { + struct string_list_item *item; + +@@ repack-midx.c: static int midx_has_unknown_packs(struct string_list *include, + * MIDX. Note this function is called before the include + * list is populated with any cruft pack(s). + * ++ * - In a MIDX layer retained as part of the new chain's base. ++ * + * - Below the geometric split line (if using pack geometry), + * indicating that the pack won't be included in the new + * MIDX, but its contents were rolled up as part of the +@@ repack-midx.c: static int midx_has_unknown_packs(struct string_list *include, + * - In the existing non-kept packs list (if not using pack + * geometry), and marked as non-deleted. + */ +- if (string_list_has_string(include, pack_name)) { ++ if (string_list_has_string(include, pack_name) || ++ midx_contains_pack(base, pack_name)) { + continue; + } else if (geometry) { + struct strbuf buf = STRBUF_INIT; +@@ repack-midx.c: static int midx_has_unknown_packs(struct string_list *include, + } + + static void midx_included_packs(struct string_list *include, +- struct repack_write_midx_opts *opts) ++ struct repack_write_midx_opts *opts, ++ struct multi_pack_index *base) + { + struct existing_packs *existing = opts->existing; + struct pack_geometry *geometry = opts->geometry; @@ repack-midx.c: static void midx_included_packs(struct string_list *include, - } if (opts->midx_must_contain_cruft || -+ existing->kept_packs.nr || - midx_has_unknown_packs(include, geometry, existing)) { + (!geometry->split_factor && existing->kept_packs.nr) || +- midx_has_unknown_packs(include, geometry, existing)) { ++ midx_has_unknown_packs(include, geometry, existing, base)) { /* * If there are one or more unknown pack(s) present (see -@@ repack-midx.c: static void midx_included_packs(struct string_list *include, - * reachability closure if the MIDX is bitmapped and one - * or more of the bitmap's selected commits reaches a - * once-cruft object that was later made reachable. -+ * -+ * Kept packs may also depend on cruft objects, since -+ * they are included above without necessarily being -+ * traversed by the repack. - */ - for_each_string_list_item(item, &existing->cruft_packs) { - /* + * midx_has_unknown_packs() for what makes a pack +@@ repack-midx.c: static int write_midx_included_packs(struct repack_write_midx_opts *opts) + struct packed_git *preferred = pack_geometry_preferred_pack(opts->geometry); + int ret = 0; + +- midx_included_packs(&include, opts); ++ midx_included_packs(&include, opts, NULL); + if (!include.nr) + goto done; + +@@ repack-midx.c: static void midx_compaction_step_release(struct midx_compaction_step *step) + free(step->csum); + } + ++static int midx_compaction_step_include_packs(struct midx_compaction_step *step, ++ struct repack_write_midx_opts *opts, ++ struct multi_pack_index *base) ++{ ++ struct odb_source_files *files = odb_source_files_downcast(opts->existing->source); ++ struct string_list include = STRING_LIST_INIT_DUP; ++ struct string_list_item *item; ++ struct strbuf path = STRBUF_INIT; ++ int ret = 0; ++ ++ midx_included_packs(&include, opts, base); ++ string_list_sort(&step->u.write); ++ ++ for_each_string_list_item(item, &include) { ++ struct packed_git *p; ++ ++ if (string_list_has_string(&step->u.write, item->string) || ++ midx_contains_pack(base, item->string)) ++ continue; ++ ++ strbuf_reset(&path); ++ strbuf_addf(&path, "%s/%s", opts->packdir, item->string); ++ p = packfile_store_load_pack(files->packed, path.buf, 1); ++ if (!p || open_pack_index(p)) { ++ ret = error(_("cannot open index for %s"), path.buf); ++ goto out; ++ } ++ if (unsigned_add_overflows(step->objects_nr, p->num_objects)) { ++ ret = error(_("too many objects in MIDX compaction step")); ++ goto out; ++ } ++ step->objects_nr += p->num_objects; ++ string_list_insert(&step->u.write, item->string); ++ } ++ ++out: ++ strbuf_release(&path); ++ string_list_clear(&include, 0); ++ return ret; ++} ++ + /* +- * Build an append-only MIDX plan: a single WRITE step for the freshly +- * written packs, plus COPY steps for every existing layer. No ++ * Build an append-only MIDX plan: a single WRITE step for packs not ++ * already in the chain, plus COPY steps for every existing layer. No + * compaction or merging is performed. + */ + static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts, +@@ repack-midx.c: static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts, + size_t *steps_nr_p) + { + struct odb_source_files *files = odb_source_files_downcast(opts->existing->source); ++ struct string_list include = STRING_LIST_INIT_DUP; ++ struct string_list_item *item; + struct multi_pack_index *m; + struct midx_compaction_step *steps = NULL; + struct midx_compaction_step *step = NULL; +- struct strbuf buf = STRBUF_INIT; + size_t steps_nr = 0, steps_alloc = 0; +- uint32_t i; + + odb_reprepare(opts->existing->repo->objects); + m = get_multi_pack_index(files->packed); + +- for (i = 0; i < opts->names->nr; i++) { ++ midx_included_packs(&include, opts, m); ++ for_each_string_list_item(item, &include) { ++ if (midx_contains_pack(m, item->string)) ++ continue; + if (!step) { + ALLOC_GROW(steps, st_add(steps_nr, 1), steps_alloc); + step = &steps[steps_nr++]; +@@ repack-midx.c: static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts, + step->type = MIDX_COMPACTION_STEP_WRITE; + string_list_init_dup(&step->u.write); + } +- strbuf_reset(&buf); +- strbuf_addf(&buf, "pack-%s.idx", +- opts->names->items[i].string); +- string_list_append(&step->u.write, buf.buf); ++ string_list_append(&step->u.write, item->string); + } +- strbuf_release(&buf); ++ string_list_clear(&include, 0); + + for (; m; m = m->base_midx) { + ALLOC_GROW(steps, st_add(steps_nr, 1), steps_alloc); +@@ repack-midx.c: static int repack_make_midx_compaction_plan(struct repack_write_midx_opts *opts, + if (opts->geometry->midx_tip_rewritten) + m = m->base_midx; + ++ if (midx_compaction_step_include_packs(&step, opts, m) < 0) { ++ midx_compaction_step_release(&step); ++ ret = -1; ++ goto out; ++ } ++ + trace2_data_string("repack", opts->existing->repo, "midx:rewrote-tip", + opts->geometry->midx_tip_rewritten ? "true" : "false"); + - ## t/t7704-repack-cruft.sh ## -@@ t/t7704-repack-cruft.sh: test_expect_success 'incremental repack includes cruft for MIDX bitmaps' ' + ## t/t7705-repack-incremental-midx.sh ## +@@ t/t7705-repack-incremental-midx.sh: test_expect_success '--write-midx=incremental without --geometric' ' + git repack -d && + + test_commit second && +- git repack --write-midx=incremental && ++ git repack --write-midx=incremental --write-bitmap-index && + + git multi-pack-index verify && + test_line_count = 1 $midx_chain && +@@ t/t7705-repack-incremental-midx.sh: test_expect_success '--write-midx=incremental without --geometric' ' + # A second repack appends a new layer without + # disturbing the existing one. + test_commit third && +- git repack --write-midx=incremental && ++ git repack --write-midx=incremental --write-bitmap-index && + + git multi-pack-index verify && + test_line_count = 2 $midx_chain && +@@ t/t7705-repack-incremental-midx.sh: test_expect_success '--write-midx=incremental without --geometric' ' + head -n 1 $midx_chain >actual && + test_cmp expect actual && + ++ git rev-list --test-bitmap HEAD && + git fsck ) ' -+test_expect_success 'geometric repack includes cruft for kept packs' ' -+ setup_cruft_exclude_tests kept-cruft && ++test_expect_success 'incremental MIDX includes cruft without a new pack' ' ++ git init incremental-cruft && ++ ( ++ cd incremental-cruft && ++ git config repack.midxMustContainCruft false && ++ ++ test_commit base && ++ echo cruft | git hash-object -w --stdin && ++ git repack --cruft -d && ++ test_commit cruft && ++ git repack -d && ++ ++ # All objects are packed, but the new MIDX still needs cruft. ++ git repack --write-midx=incremental --write-bitmap-index && ++ git rev-list --test-bitmap HEAD ++ ) ++' ++ ++test_expect_success 'geometric incremental MIDX retains cruft when replacing its tip' ' ++ git init geometric-incremental-cruft && + ( -+ cd kept-cruft && ++ cd geometric-incremental-cruft && ++ git config repack.midxNewLayerThreshold 1 && + -+ # Keep HEAD and its tree outside the geometric repack. Its -+ # parent is reachable again, but still in the cruft pack. -+ git rev-parse HEAD HEAD^{tree} >objects && -+ pack=$(git pack-objects $packdir/pack actual && + grep "^pack-.*\.idx$" actual >actual.packs && +- test_line_count = 1 actual.packs && +- test_grep ! "$keep" actual.packs && ++ test_line_count = 2 actual.packs && + + git multi-pack-index verify && ++ git rev-list --test-bitmap HEAD && + + # All objects (from both kept and non-kept packs) + # must still be accessible. base-commit: a018953688f1b10bddf91bff8747068f5f4746a4 -- 2.56.0.8.ga42f775cbe2 ``` ## Taylor Blau, 2026-10-01 04:11 Subject: [PATCH v2 1/8] pack-objects: introduce `stdin_packs_context` struct Message-ID: <354c29cae732703e75b22fa347cb07d898304f01.1790827875.git.me@ttaylorr.com> In-Reply-To: ``` `stdin_packs_read_input()` currently receives a pointer to the `rev_info` struct and '--stdin-packs' mode separately as arguments, but the object enumeration callbacks only receive a pointer to the `rev_info` struct. Wrap the pair in a new `stdin_packs_context` struct so that a future change may reference the '--stdin-packs' mode within the various object enumeration callbacks. Signed-off-by: Taylor Blau --- builtin/pack-objects.c | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index af9390a46b9..a553064fcce 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -3804,11 +3804,17 @@ static int git_pack_config(const char *k, const char *v, static int stdin_packs_found_nr; static int stdin_packs_hints_nr; +struct stdin_packs_context { + struct rev_info *revs; /* must be non-NULL */ + enum stdin_packs_mode mode; +}; + static int add_object_entry_from_pack(const struct object_id *oid, struct packed_git *p, uint32_t pos, void *_data) { + struct stdin_packs_context *ctx = _data; off_t ofs; struct object_info oi = OBJECT_INFO_INIT; enum object_type type = OBJ_NONE; @@ -3827,7 +3833,6 @@ static int add_object_entry_from_pack(const struct object_id *oid, die(_("could not get type of object %s in pack %s"), oid_to_hex(oid), p->pack_name); } else if (type == OBJ_COMMIT) { - struct rev_info *revs = _data; /* * commits in included packs are used as starting points * for the subsequent revision walk @@ -3840,7 +3845,7 @@ static int add_object_entry_from_pack(const struct object_id *oid, * However, we'll only add those objects to the packing * list after checking `want_object_in_pack()` below. */ - add_pending_oid(revs, NULL, oid, 0); + add_pending_oid(ctx->revs, NULL, oid, 0); } if (!want_object_in_pack(oid, 0, &p, &ofs)) @@ -3954,8 +3959,9 @@ static int stdin_packs_include_check(struct commit *commit, void *data) } static void stdin_packs_add_pack_entries(struct strmap *packs, - struct rev_info *revs) + struct stdin_packs_context *ctx) { + struct rev_info *revs = ctx->revs; struct string_list keys = STRING_LIST_INIT_NODUP; struct string_list_item *item; struct hashmap_iter iter; @@ -3994,15 +4000,14 @@ static void stdin_packs_add_pack_entries(struct strmap *packs, (info->kind & STDIN_PACK_EXCLUDE_OPEN)) for_each_object_in_pack(info->p, add_object_entry_from_pack, - revs, + ctx, ODB_FOR_EACH_OBJECT_PACK_ORDER); } string_list_clear(&keys, 0); } -static void stdin_packs_read_input(struct rev_info *revs, - enum stdin_packs_mode mode) +static void stdin_packs_read_input(struct stdin_packs_context *ctx) { struct strbuf buf = STRBUF_INIT; struct strmap packs = STRMAP_INIT; @@ -4017,7 +4022,7 @@ static void stdin_packs_read_input(struct rev_info *revs, continue; else if (*key == '^') kind = STDIN_PACK_EXCLUDE_CLOSED; - else if (*key == '!' && mode == STDIN_PACKS_MODE_FOLLOW) + else if (*key == '!' && ctx->mode == STDIN_PACKS_MODE_FOLLOW) kind = STDIN_PACK_EXCLUDE_OPEN; if (kind != STDIN_PACK_INCLUDE) @@ -4082,19 +4087,23 @@ static void stdin_packs_read_input(struct rev_info *revs, info->p = p; } - stdin_packs_add_pack_entries(&packs, revs); + stdin_packs_add_pack_entries(&packs, ctx); strbuf_release(&buf); strmap_clear(&packs, 1); } -static void add_unreachable_loose_objects(struct rev_info *revs); +static void add_unreachable_loose_objects(struct stdin_packs_context *ctx); static void read_stdin_packs(struct repository *repo, enum stdin_packs_mode mode, int rev_list_unpacked) { int prev_fetch_if_missing = repo->fetch_if_missing; struct rev_info revs; + struct stdin_packs_context ctx = { + .revs = &revs, + .mode = mode, + }; /* * The revision walk may hit objects that are promised, only. As the @@ -4131,9 +4140,9 @@ static void read_stdin_packs(struct repository *repo, */ ignore_packed_keep_in_core_open = 1; } - stdin_packs_read_input(&revs, mode); + stdin_packs_read_input(&ctx); if (rev_list_unpacked) - add_unreachable_loose_objects(&revs); + add_unreachable_loose_objects(&ctx); if (prepare_revision_walk(&revs)) die(_("revision walk setup failed")); @@ -4541,7 +4550,7 @@ static void add_objects_in_unpacked_packs(void) static int add_loose_object(const struct object_id *oid, const char *path, void *data) { - struct rev_info *revs = data; + struct stdin_packs_context *ctx = data; enum object_type type = odb_read_object_info(the_repository->objects, oid, NULL); if (type < 0) { @@ -4563,8 +4572,8 @@ static int add_loose_object(const struct object_id *oid, const char *path, add_object_entry(oid, type, "", 0); } - if (revs && type == OBJ_COMMIT) - add_pending_oid(revs, NULL, oid, 0); + if (ctx && type == OBJ_COMMIT) + add_pending_oid(ctx->revs, NULL, oid, 0); return 0; } @@ -4574,10 +4583,10 @@ static int add_loose_object(const struct object_id *oid, const char *path, * add_object_entry will weed out duplicates, so we just add every * loose object we find. */ -static void add_unreachable_loose_objects(struct rev_info *revs) +static void add_unreachable_loose_objects(struct stdin_packs_context *ctx) { for_each_loose_file_in_source(the_repository->objects->sources, - add_loose_object, NULL, NULL, revs); + add_loose_object, NULL, NULL, ctx); } static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid) -- 2.56.0.8.ga42f775cbe2 ``` ## Taylor Blau, 2026-10-01 04:11 Subject: [PATCH v2 2/8] pack-objects: ensure tree/tag closure with '--stdin-packs=follow' Message-ID: <940953e5c407046ac6789367f3341dc8ea73d07a.1790827875.git.me@ttaylorr.com> In-Reply-To: ``` Since 5ee86c273bf (repack: exclude cruft pack(s) from the MIDX where possible, 2025-06-23), when the 'repack.midxMustContainCruft' configuration is set to "false", geometric repacks use '--stdin-packs=follow' to copy needed objects out of cruft packs so the MIDX can omit those packs. In cd846bacc7d (pack-objects: introduce '--stdin-packs=follow', 2025-06-23), this behavior changed such that whenever excluded-open ('!') packs are present, the walk stops at objects in excluded-closed ('^') packs. Geometric repacks use '^' for retained packs already in the MIDX, relying on the indexed object set being closed under reachability. However, the walk introduced in cd846bacc7d starts only from commit objects. A geometric repack can therefore produce a MIDX that does not maintain reachability closure for lone trees (that are not reachable from any commit otherwise in the closure). A subsequent repack with '!' packs can stop at that tree in a retained '^' pack even if a new commit reaches it. If the cruft pack remains excluded, and the bitmap selection picks one or more commits which reach that tree, the MIDX cannot generate a bitmap for that commit. Add trees and tags from included and '!' packs (and loose ones with '--unpacked') as roots in '--stdin-packs=follow' mode. This rescues their descendants even when no input commit reaches them. Walk these roots after the existing traversal, preserving the `SEEN` bit to avoid redundant traversals. This gives directly enumerated commits priority for the path prefixes used by name hashes and delta attributes. Tags can introduce commits in the second walk, so path selection remains best-effort. Collect the extra roots in an oidset to avoid queuing duplicates. This uses memory for each distinct root and walks its unvisited descendants. Objects in '^' packs remain cutoffs to avoid rewalking packs that are known to be closed under reachability, provided '!' packs are present. This does not repair existing MIDXs lacking closure; those need a full repack. Signed-off-by: Taylor Blau --- Documentation/git-pack-objects.adoc | 2 + builtin/pack-objects.c | 43 ++++++++++++++- t/t5331-pack-objects-stdin.sh | 81 +++++++++++++++++++++++++++++ t/t7704-repack-cruft.sh | 20 +++++++ 4 files changed, 145 insertions(+), 1 deletion(-) diff --git a/Documentation/git-pack-objects.adoc b/Documentation/git-pack-objects.adoc index 65cd00c152f..1564d44f49d 100644 --- a/Documentation/git-pack-objects.adoc +++ b/Documentation/git-pack-objects.adoc @@ -112,6 +112,8 @@ pack may include additional objects based on the following: This mode is useful, for example, to resurrect once-unreachable objects found in cruft packs to generate packs which are closed under reachability up to the boundary set by the excluded packs. +Trees and tags in included or `!` packs are followed even when no +commit reaches them, as are loose trees and tags with `--unpacked`. + Incompatible with `--revs`, or options that imply `--revs` (such as `--all`), with the exception of `--unpacked`, which is compatible. diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index a553064fcce..fb603059a92 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -3807,6 +3807,7 @@ static int stdin_packs_hints_nr; struct stdin_packs_context { struct rev_info *revs; /* must be non-NULL */ enum stdin_packs_mode mode; + struct oidset extra_roots; }; static int add_object_entry_from_pack(const struct object_id *oid, @@ -3846,6 +3847,9 @@ static int add_object_entry_from_pack(const struct object_id *oid, * list after checking `want_object_in_pack()` below. */ add_pending_oid(ctx->revs, NULL, oid, 0); + } else if (ctx->mode == STDIN_PACKS_MODE_FOLLOW && + (type == OBJ_TREE || type == OBJ_TAG)) { + oidset_insert(&ctx->extra_roots, oid); } if (!want_object_in_pack(oid, 0, &p, &ofs)) @@ -4103,7 +4107,10 @@ static void read_stdin_packs(struct repository *repo, struct stdin_packs_context ctx = { .revs = &revs, .mode = mode, + .extra_roots = OIDSET_INIT, }; + struct oidset_iter iter; + const struct object_id *oid; /* * The revision walk may hit objects that are promised, only. As the @@ -4151,6 +4158,34 @@ static void read_stdin_packs(struct repository *repo, show_object_pack_hint, &mode); + /* + * Trees and tags need closure even when no commit reaches them. + * Defer adding these roots to revs.pending until the first walk + * finishes. Otherwise a subtree may be visited and marked SEEN + * before its commit's root tree, using "a" instead of "sub/a" + * for a blob's namehash and delta attributes. + * + * Tags may introduce more commits in the second walk, so this + * does not *always* guarantee that trees are always visited + * with their full paths. + */ + oidset_iter_init(&ctx.extra_roots, &iter); + while ((oid = oidset_iter_next(&iter))) { + struct object *obj = lookup_object(repo, oid); + + if (!obj || !(obj->flags & SEEN)) + add_pending_oid(&revs, NULL, oid, 0); + } + if (revs.pending.nr) { + if (prepare_revision_walk(&revs)) + die(_("revision walk setup failed")); + traverse_commit_list(&revs, + show_commit_pack_hint, + show_object_pack_hint, + &mode); + } + oidset_clear(&ctx.extra_roots); + release_revisions(&revs); trace2_data_intmax("pack-objects", the_repository, "stdin_packs_found", @@ -4572,8 +4607,14 @@ static int add_loose_object(const struct object_id *oid, const char *path, add_object_entry(oid, type, "", 0); } - if (ctx && type == OBJ_COMMIT) + if (!ctx) + return 0; + + if (type == OBJ_COMMIT) add_pending_oid(ctx->revs, NULL, oid, 0); + else if (ctx->mode == STDIN_PACKS_MODE_FOLLOW && + (type == OBJ_TREE || type == OBJ_TAG)) + oidset_insert(&ctx->extra_roots, oid); return 0; } diff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh index c74b5861af3..aa79ecdf13c 100755 --- a/t/t5331-pack-objects-stdin.sh +++ b/t/t5331-pack-objects-stdin.sh @@ -520,4 +520,85 @@ test_expect_success '--stdin-packs with !-delimited pack without follow' ' ) ' +test_expect_success '--stdin-packs=follow traverses a tree-only input pack' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit base && + tree=$(git rev-parse HEAD^{tree}) && + P=$(echo "$tree" | git pack-objects $packdir/pack) && + echo "pack-$P.pack" >in && + + # Only --stdin-packs=follow should start a walk from the tree. + : >trace.txt && + GIT_TRACE2_EVENT="$(pwd)/trace.txt" git pack-objects \ + --stdin-packs --stdout /dev/null && + + test_trace2_data pack-objects stdin_packs_hints 0 expect.raw && + sort expect.raw >expect && + objects_in_packs $P >actual && + + test_cmp expect actual + ) +' + +test_expect_success '--stdin-packs=follow traverses an excluded-open tag' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit --annotate base && + + # Put the commit, tree, and blob in one pack, and the tag in another. + # Give only the second pack as input with a "!" prefix. The result + # must contain the commit, tree, and blob, but not the tag. + P=$(echo HEAD | git pack-objects --revs $packdir/pack) && + objects_in_packs $P >expect && + + git rev-parse base >in && + P=$(git pack-objects $packdir/pack in && + P=$(git pack-objects --stdin-packs=follow $packdir/pack actual && + + test_cmp expect actual + ) +' + +test_expect_success '--stdin-packs=follow respects delta attributes for subtree contents' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + + echo "sub/* -delta" >.gitattributes && + mkdir sub && + test-tool genrandom seed 8192 >sub/a && + cp sub/a sub/b && + echo modified >>sub/b && + git add sub && + git commit -m base && + + # If the subtree is visited first, the blobs are found as a and + # b, so the sub/* attribute does not apply. + git rev-parse HEAD HEAD:sub >in && + P=$(git pack-objects $packdir/pack in && + + git pack-objects --stdin-packs=follow $packdir/pack actual && + printf "%s\n" "$ZERO_OID" "$ZERO_OID" >expect && + test_cmp expect actual + ) +' + test_done diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh index b342e82447d..b49f22878f7 100755 --- a/t/t7704-repack-cruft.sh +++ b/t/t7704-repack-cruft.sh @@ -767,6 +767,26 @@ test_expect_success 'repack --write-midx excludes cruft where possible' ' ) ' +test_expect_success 'geometric repack rescues descendants of loose trees' ' + git init loose-tree-cruft && + ( + cd loose-tree-cruft && + git config repack.midxMustContainCruft false && + test_commit base && + blob=$(echo cruft | git hash-object -w --stdin) && + GIT_TEST_MULTI_PACK_INDEX=0 git repack --cruft -d && + + printf "100644 blob %s\tfile\n" "$blob" | git mktree && + GIT_TEST_MULTI_PACK_INDEX=0 git repack -d --geometric=2 \ + --write-midx --write-bitmap-index && + + test-tool read-midx --show-objects $objdir >midx && + cruft=$(ls $packdir/*.mtimes) && + test_grep ! "$(basename "$cruft" .mtimes).idx" midx && + test_grep "^$blob " midx + ) +' + test_expect_success 'repack --write-midx includes cruft when instructed' ' setup_cruft_exclude_tests exclude-cruft-when-instructed && ( -- 2.56.0.8.ga42f775cbe2 ``` ## Taylor Blau, 2026-10-01 04:11 Subject: [PATCH v2 3/8] repack: retain cruft packs in MIDXs after incremental repacks Message-ID: In-Reply-To: ``` An incremental repack can write a commit and tree into a new pack while leaving objects they reach in an existing cruft pack. For example, a commit can make a previously unreachable blob reachable again. Since 'repack' will invoke 'pack-objects' with '--incremental', it will not copy the blob out of its cruft pack. When the 'repack.midxMustContainCruft' configuration is set to "false", writing the first MIDX after such a repack may omit that cruft pack. The new pack bypasses the `!names.nr` fallback, and there are no previous MIDX packs for `midx_has_unknown_packs()` to check. Selecting the new commit for bitmap coverage then fails because its reachable objects are not all in the MIDX. The omission dates all the way back to 5ee86c273bf (repack: exclude cruft pack(s) from the MIDX where possible, 2025-06-23). It relies on geometric repacking to copy once-cruft objects with '--stdin-packs=follow'. However, an ordinary incremental repack makes no such guarantee. Require the MIDX to include cruft packs in that case, even when a new pack was written. This fixes ordinary '--write-midx'. The separate '--write-midx=incremental' writer does not consult this flag and needs its own handling. Exercise this with the existing fixture that makes a cruft commit reachable again and adds a new (unpacked) commit on top, and ensure that the incremental repack is able to successfully write a reachability bitmap. Signed-off-by: Taylor Blau --- builtin/repack.c | 6 ++++++ t/t7704-repack-cruft.sh | 11 +++++++++++ 2 files changed, 17 insertions(+) diff --git a/builtin/repack.c b/builtin/repack.c index c4360382c1f..b7596d488da 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -539,6 +539,12 @@ int cmd_repack(int argc, strvec_push(&cmd.args, "--stdin-packs=follow"); strvec_push(&cmd.args, "--unpacked"); } else { + /* + * Incremental repacks do not copy already-packed objects, + * so cruft packs may be required to form a reachability + * closure for the MIDX. + */ + midx_must_contain_cruft = 1; strvec_push(&cmd.args, "--unpacked"); strvec_push(&cmd.args, "--incremental"); } diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh index b49f22878f7..f7f83e70ffe 100755 --- a/t/t7704-repack-cruft.sh +++ b/t/t7704-repack-cruft.sh @@ -787,6 +787,17 @@ test_expect_success 'geometric repack rescues descendants of loose trees' ' ) ' +test_expect_success 'incremental repack includes cruft for MIDX bitmaps' ' + setup_cruft_exclude_tests incremental-cruft && + ( + cd incremental-cruft && + + GIT_TEST_MULTI_PACK_INDEX=0 \ + git repack -d --write-midx --write-bitmap-index && + git rev-list --test-bitmap HEAD + ) +' + test_expect_success 'repack --write-midx includes cruft when instructed' ' setup_cruft_exclude_tests exclude-cruft-when-instructed && ( -- 2.56.0.8.ga42f775cbe2 ``` ## Taylor Blau, 2026-10-01 04:11 Subject: [PATCH v2 4/8] repack: use a sorted list for explicitly kept packs Message-ID: In-Reply-To: ``` `existing_packs_collect()` performs a linear search through the '--keep-pack' arguments for each local pack. Typically the number of such arguments is small enough that the difference between a linear and binary search is just noise (especially compared with the amount of work that 'repack' is about to perform). However, an additional caller will wish to search through the same list. To prevent that caller from having to duplicate the clunky for-loop in `existing_packs_collect()`, sort the list using `fspathcmp()` and replace the existing caller's loop with `string_list_has_string()`. This does not change the overall behavior of '--keep-pack' arguments. Signed-off-by: Taylor Blau --- builtin/repack.c | 3 +++ repack.c | 8 +------- repack.h | 4 ++++ 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/builtin/repack.c b/builtin/repack.c index b7596d488da..88b05e96b5b 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -2,6 +2,7 @@ #include "builtin.h" #include "config.h" +#include "dir.h" #include "environment.h" #include "parse-options.h" #include "path.h" @@ -455,6 +456,8 @@ int cmd_repack(int argc, packtmp = mkpathdup("%s/%s", packdir, packtmp_name); existing.repo = repo; + keep_pack_list.cmp = fspathcmp; + string_list_sort(&keep_pack_list); existing_packs_collect(&existing, &keep_pack_list); if (geometry.split_factor) { diff --git a/repack.c b/repack.c index d2aa58e1348..fa748ce46ce 100644 --- a/repack.c +++ b/repack.c @@ -1,5 +1,4 @@ #include "git-compat-util.h" -#include "dir.h" #include "midx.h" #include "odb.h" #include "packfile.h" @@ -131,7 +130,6 @@ void existing_packs_collect(struct existing_packs *existing, struct strbuf buf = STRBUF_INIT; repo_for_each_pack(existing->repo, p) { - size_t i; const char *base; if (p->multi_pack_index) @@ -142,15 +140,11 @@ void existing_packs_collect(struct existing_packs *existing, base = pack_basename(p); - for (i = 0; i < extra_keep->nr; i++) - if (!fspathcmp(base, extra_keep->items[i].string)) - break; - strbuf_reset(&buf); strbuf_addstr(&buf, base); strbuf_strip_suffix(&buf, ".pack"); - if ((extra_keep->nr > 0 && i < extra_keep->nr) || p->pack_keep) + if (p->pack_keep || string_list_has_string(extra_keep, base)) string_list_append(&existing->kept_packs, buf.buf); else if (p->is_cruft) string_list_append(&existing->cruft_packs, buf.buf); diff --git a/repack.h b/repack.h index 61e554e4ed3..9f95e3a26f4 100644 --- a/repack.h +++ b/repack.h @@ -76,6 +76,10 @@ struct existing_packs { * or packs->kept based on whether each pack has a corresponding * .keep file or not. Packs without a .keep file are not to be kept * if we are going to pack everything into one file. + * + * A non-empty extra_keep must be sorted and use fspathcmp() as its + * comparator. Its entries are pack basenames, including the ".pack" + * suffix. */ void existing_packs_collect(struct existing_packs *existing, const struct string_list *extra_keep); -- 2.56.0.8.ga42f775cbe2 ``` ## Taylor Blau, 2026-10-01 04:11 Subject: [PATCH v2 5/8] repack: follow kept packs when omitting cruft from the MIDX Message-ID: <51e20444dac1223f0e0485dc5799ed6d592f7614.1790827875.git.me@ttaylorr.com> In-Reply-To: ``` When performing a geometric repack with 'repack.midxMustContainCruft' set to "false", Git uses '--stdin-packs=follow' to copy (once-cruft) objects needed for reachability closure out of cruft packs. .keep packs do not normally participate in that walk, though they *are* included in the resulting MIDX. A .keep pack can contain a commit that reaches an object whose only copy is in a cruft pack. When there is no previous MIDX and the repack writes a new pack, neither `midx_has_unknown_packs()` nor the `!names.nr` fallback require that cruft pack to be included. If the kept commit (or a descendant of it) is selected for bitmap coverage, the bitmap writer fails because the MIDX does not contain all of its reachable objects. Pass excluded kept packs to the follow walk, using '!' for packs outside the existing MIDX and '^' for packs already covered by it. This copies needed objects out of cruft without copying objects in the kept packs. It also avoids including all cruft merely because a concurrent push has installed a temporary '.keep' file. With '--pack-kept-objects', packs with '.keep' files participate in the geometric repack, and thus may have their objects copied. Packs specified as kept via '--keep-pack' still exclude their objects, even when their packs fall below the geometric split. Signed-off-by: Taylor Blau --- builtin/repack.c | 31 ++++++++++++++++++++++++++--- repack-midx.c | 5 +++++ t/t7704-repack-cruft.sh | 43 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 3 deletions(-) diff --git a/builtin/repack.c b/builtin/repack.c index 88b05e96b5b..27d6668a4ab 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -476,9 +476,11 @@ int cmd_repack(int argc, show_progress = !po_args.quiet && isatty(2); strvec_push(&cmd.args, "--keep-true-parents"); - for (i = 0; i < keep_pack_list.nr; i++) - strvec_pushf(&cmd.args, "--keep-pack=%s", - keep_pack_list.items[i].string); + /* Geometric follow walks exclude these packs through stdin instead. */ + if (!(geometry.split_factor && !midx_must_contain_cruft)) + for (i = 0; i < keep_pack_list.nr; i++) + strvec_pushf(&cmd.args, "--keep-pack=%s", + keep_pack_list.items[i].string); strvec_push(&cmd.args, "--non-empty"); if (!geometry.split_factor) { /* @@ -593,6 +595,29 @@ int cmd_repack(int argc, fprintf(in, "%c%s\n", marker, basename); } + if (!midx_must_contain_cruft) { + struct strbuf buf = STRBUF_INIT; + + for_each_string_list_item(item, &existing.kept_packs) { + char marker = '^'; + + strbuf_reset(&buf); + strbuf_addf(&buf, "%s.pack", item->string); + + if (po_args.pack_kept_objects && + !string_list_has_string(&keep_pack_list, + buf.buf)) + continue; + + /* Exclusions override any inclusion above. */ + if (!string_list_has_string(&existing.midx_packs, + buf.buf)) + marker = '!'; + + fprintf(in, "%c%s\n", marker, buf.buf); + } + strbuf_release(&buf); + } fclose(in); } diff --git a/repack-midx.c b/repack-midx.c index 64c7f8d0f42..9f7786aaac5 100644 --- a/repack-midx.c +++ b/repack-midx.c @@ -197,6 +197,7 @@ static void midx_included_packs(struct string_list *include, } if (opts->midx_must_contain_cruft || + (!geometry->split_factor && existing->kept_packs.nr) || midx_has_unknown_packs(include, geometry, existing)) { /* * If there are one or more unknown pack(s) present (see @@ -209,6 +210,10 @@ static void midx_included_packs(struct string_list *include, * reachability closure if the MIDX is bitmapped and one * or more of the bitmap's selected commits reaches a * once-cruft object that was later made reachable. + * + * Kept packs may also depend on cruft objects, since + * they are included above without necessarily being + * traversed by a non-geometric repack. */ for_each_string_list_item(item, &existing->cruft_packs) { /* diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh index f7f83e70ffe..bc6d6f588fa 100755 --- a/t/t7704-repack-cruft.sh +++ b/t/t7704-repack-cruft.sh @@ -798,6 +798,49 @@ test_expect_success 'incremental repack includes cruft for MIDX bitmaps' ' ) ' +test_expect_success 'geometric repack follows kept packs to cruft objects' ' + setup_cruft_exclude_tests kept-cruft && + ( + cd kept-cruft && + + # Put HEAD in a kept pack, while its parent is still in + # a cruft pack. + pack=$(echo "HEAD^..HEAD" | git pack-objects --revs $packdir/pack) && + git prune-packed && + GIT_TEST_MULTI_PACK_INDEX=0 \ + git repack -d --geometric=2 --write-midx --write-bitmap-index \ + --keep-pack=pack-$pack.pack && + + test-tool find-pack -c 1 HEAD && + test-tool read-midx --show-objects $objdir >midx && + cruft=$(ls $packdir/*.mtimes) && + test_grep ! "$(basename "$cruft" .mtimes).idx" midx + ) +' + +test_expect_success 'full repack retains cruft pack in MIDX for unreachable kept objects' ' + setup_cruft_exclude_tests unreachable-kept-cruft && + ( + cd unreachable-kept-cruft && + + pack=$(echo "HEAD^..HEAD" | git pack-objects --revs $packdir/pack) && + touch $packdir/pack-$pack.keep && + + # Make the kept commit unreachable so that the full + # repack leaves its parent in a cruft pack. + git reset --hard one && + git tag -d four && + git reflog expire --all --expire=all && + + GIT_TEST_MULTI_PACK_INDEX=0 \ + git repack -a --write-midx --write-bitmap-index && + + test-tool read-midx --show-objects $objdir >midx && + cruft=$(ls $packdir/*.mtimes) && + test_grep "$(basename "$cruft" .mtimes).idx" midx + ) +' + test_expect_success 'repack --write-midx includes cruft when instructed' ' setup_cruft_exclude_tests exclude-cruft-when-instructed && ( -- 2.56.0.8.ga42f775cbe2 ``` ## Taylor Blau, 2026-10-01 04:11 Subject: [PATCH v2 6/8] repack: track the preferred pack explicitly in MIDX write steps Message-ID: In-Reply-To: ``` A MIDX write step marks preferred packs in its string-list entries and chooses the last marked entry when executing the step. That makes the choice depend on list order, preventing the list from being sorted for membership checks. Record the last candidate directly in the step, borrowing its name from the write list. This preserves preferred-pack selection while allowing the list to be sorted without changing that choice. --- repack-midx.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/repack-midx.c b/repack-midx.c index 9f7786aaac5..61832114671 100644 --- a/repack-midx.c +++ b/repack-midx.c @@ -404,6 +404,7 @@ struct midx_compaction_step { uint32_t objects_nr; char *csum; + const char *preferred_pack; /* points into u.write */ enum { MIDX_COMPACTION_STEP_UNKNOWN, @@ -441,8 +442,6 @@ static int midx_compaction_step_exec_write(struct midx_compaction_step *step, { struct child_process cmd = CHILD_PROCESS_INIT; struct string_list hash = STRING_LIST_INIT_DUP; - struct string_list_item *item; - const char *preferred_pack = NULL; int ret = 0; if (!step->u.write.nr) { @@ -450,19 +449,14 @@ static int midx_compaction_step_exec_write(struct midx_compaction_step *step, goto out; } - for_each_string_list_item(item, &step->u.write) { - if (item->util) - preferred_pack = item->string; - } - repack_prepare_midx_command(&cmd, opts, "write"); strvec_pushl(&cmd.args, "--incremental", "--no-write-chain-file", NULL); strvec_pushf(&cmd.args, "--base=%s", base ? base : "none"); - if (preferred_pack) { + if (step->preferred_pack) { struct strbuf buf = STRBUF_INIT; - strbuf_addstr(&buf, preferred_pack); + strbuf_addstr(&buf, step->preferred_pack); strbuf_strip_suffix(&buf, ".idx"); strbuf_addstr(&buf, ".pack"); @@ -719,7 +713,7 @@ static int repack_make_midx_compaction_plan(struct repack_write_midx_opts *opts, item = string_list_append(&step.u.write, buf.buf); if (p->multi_pack_index || i == opts->geometry->pack_nr - 1) - item->util = (void *)1; /* mark as preferred */ + step.preferred_pack = item->string; if (unsigned_add_overflows(step.objects_nr, p->num_objects)) { ret = error(_("too many objects in MIDX compaction step")); @@ -797,7 +791,7 @@ static int repack_make_midx_compaction_plan(struct repack_write_midx_opts *opts, item = string_list_append(&step.u.write, buf.buf); if (pack_int_id == preferred_pack_idx) - item->util = (void *)1; /* mark as preferred */ + step.preferred_pack = item->string; } if (unsigned_add_overflows(step.objects_nr, m->num_objects)) { -- 2.56.0.8.ga42f775cbe2 ``` ## Taylor Blau, 2026-10-01 04:12 Subject: [PATCH v2 7/8] repack: defer allocating the append plan's write step Message-ID: <4a6504629a24cc802462f44b36f02b7118d2a9b7.1790827875.git.me@ttaylorr.com> In-Reply-To: ``` The append plan creates a write step before iterating over newly written packs. The next change will consider additional packs and skip those already covered by the MIDX chain, so a non-empty candidate list will no longer imply that a write step is needed. Allocate the step when adding its first pack instead. Continue to consider only newly written packs here, preserving the existing plan. Signed-off-by: Taylor Blau --- repack-midx.c | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/repack-midx.c b/repack-midx.c index 61832114671..06eadb9df82 100644 --- a/repack-midx.c +++ b/repack-midx.c @@ -559,33 +559,28 @@ static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts, struct odb_source_files *files = odb_source_files_downcast(opts->existing->source); struct multi_pack_index *m; struct midx_compaction_step *steps = NULL; - struct midx_compaction_step *step; + struct midx_compaction_step *step = NULL; + struct strbuf buf = STRBUF_INIT; size_t steps_nr = 0, steps_alloc = 0; + uint32_t i; odb_reprepare(opts->existing->repo->objects); m = get_multi_pack_index(files->packed); - if (opts->names->nr) { - struct strbuf buf = STRBUF_INIT; - uint32_t i; - - ALLOC_GROW(steps, st_add(steps_nr, 1), steps_alloc); - - step = &steps[steps_nr++]; - memset(step, 0, sizeof(*step)); - - step->type = MIDX_COMPACTION_STEP_WRITE; - string_list_init_dup(&step->u.write); - - for (i = 0; i < opts->names->nr; i++) { - strbuf_reset(&buf); - strbuf_addf(&buf, "pack-%s.idx", - opts->names->items[i].string); - string_list_append(&step->u.write, buf.buf); + for (i = 0; i < opts->names->nr; i++) { + if (!step) { + ALLOC_GROW(steps, st_add(steps_nr, 1), steps_alloc); + step = &steps[steps_nr++]; + memset(step, 0, sizeof(*step)); + step->type = MIDX_COMPACTION_STEP_WRITE; + string_list_init_dup(&step->u.write); } - - strbuf_release(&buf); + strbuf_reset(&buf); + strbuf_addf(&buf, "pack-%s.idx", + opts->names->items[i].string); + string_list_append(&step->u.write, buf.buf); } + strbuf_release(&buf); for (; m; m = m->base_midx) { ALLOC_GROW(steps, st_add(steps_nr, 1), steps_alloc); -- 2.56.0.8.ga42f775cbe2 ``` ## Taylor Blau, 2026-10-01 04:12 Subject: [PATCH v2 8/8] repack: include required packs in incremental MIDX writes Message-ID: In-Reply-To: ``` The append plan introduced in 06733a50eee (repack: allow `--write-midx=incremental` without `--geometric`, 2026-05-19) adds only newly written packs to the existing MIDX chain. The bitmap writer can use objects from the new layer and all retained base layers, but the plan omits preexisting packs outside the chain. Bitmap generation fails if a selected commit reaches an object absent from the resulting chain. The geometric plan from 1da62fb5c86 (repack: implement incremental MIDX repacking, 2026-05-19) can omit kept and cruft packs, since neither necessarily participates in the geometric repack. Such packs can also be lost when replacing a tip layer that contains them. Neither plan consults `midx_included_packs()`, so the rules for retaining cruft in ordinary MIDX writes do not protect incremental writes. Use that selection logic to add missing packs to each plan's write step. Skip packs in retained base layers, but include required packs from a replaced tip. Count added objects when choosing which layers to compact, without changing the preferred pack. Write and verify bitmaps in the existing append test: its existing checks do not detect the omitted pack containing the first commit. Cover the no-new-pack case separately with a reachable blob in a cruft pack. Signed-off-by: Taylor Blau --- Documentation/git-repack.adoc | 5 +- repack-midx.c | 83 ++++++++++++++++++++++++------ t/t7705-repack-incremental-midx.sh | 63 ++++++++++++++++++----- 3 files changed, 122 insertions(+), 29 deletions(-) diff --git a/Documentation/git-repack.adoc b/Documentation/git-repack.adoc index a1f9e64f668..ee59b76580a 100644 --- a/Documentation/git-repack.adoc +++ b/Documentation/git-repack.adoc @@ -303,8 +303,9 @@ linkgit:git-multi-pack-index[1]). flat MIDX. + Without `--geometric`, a new MIDX layer is appended to the existing -chain (or a new chain is started) containing whatever packs were written -by the repack. Existing layers are preserved as-is. +chain (or a new chain is started) containing newly written packs and any +other required packs not already in the chain. Existing layers are +preserved as-is. + When combined with `--geometric`, the incremental mode maintains a chain of MIDX layers that is compacted over time using a geometric merging diff --git a/repack-midx.c b/repack-midx.c index 06eadb9df82..58776c44691 100644 --- a/repack-midx.c +++ b/repack-midx.c @@ -7,6 +7,7 @@ #include "odb.h" #include "oidset.h" #include "pack-bitmap.h" +#include "packfile.h" #include "path.h" #include "refs.h" #include "run-command.h" @@ -73,7 +74,8 @@ void midx_snapshot_refs(struct repository *repo, struct tempfile *f) static int midx_has_unknown_packs(struct string_list *include, struct pack_geometry *geometry, - struct existing_packs *existing) + struct existing_packs *existing, + struct multi_pack_index *base) { struct string_list_item *item; @@ -91,6 +93,8 @@ static int midx_has_unknown_packs(struct string_list *include, * MIDX. Note this function is called before the include * list is populated with any cruft pack(s). * + * - In a MIDX layer retained as part of the new chain's base. + * * - Below the geometric split line (if using pack geometry), * indicating that the pack won't be included in the new * MIDX, but its contents were rolled up as part of the @@ -99,7 +103,8 @@ static int midx_has_unknown_packs(struct string_list *include, * - In the existing non-kept packs list (if not using pack * geometry), and marked as non-deleted. */ - if (string_list_has_string(include, pack_name)) { + if (string_list_has_string(include, pack_name) || + midx_contains_pack(base, pack_name)) { continue; } else if (geometry) { struct strbuf buf = STRBUF_INIT; @@ -141,7 +146,8 @@ static int midx_has_unknown_packs(struct string_list *include, } static void midx_included_packs(struct string_list *include, - struct repack_write_midx_opts *opts) + struct repack_write_midx_opts *opts, + struct multi_pack_index *base) { struct existing_packs *existing = opts->existing; struct pack_geometry *geometry = opts->geometry; @@ -198,7 +204,7 @@ static void midx_included_packs(struct string_list *include, if (opts->midx_must_contain_cruft || (!geometry->split_factor && existing->kept_packs.nr) || - midx_has_unknown_packs(include, geometry, existing)) { + midx_has_unknown_packs(include, geometry, existing, base)) { /* * If there are one or more unknown pack(s) present (see * midx_has_unknown_packs() for what makes a pack @@ -336,7 +342,7 @@ static int write_midx_included_packs(struct repack_write_midx_opts *opts) struct packed_git *preferred = pack_geometry_preferred_pack(opts->geometry); int ret = 0; - midx_included_packs(&include, opts); + midx_included_packs(&include, opts, NULL); if (!include.nr) goto done; @@ -547,9 +553,50 @@ static void midx_compaction_step_release(struct midx_compaction_step *step) free(step->csum); } +static int midx_compaction_step_include_packs(struct midx_compaction_step *step, + struct repack_write_midx_opts *opts, + struct multi_pack_index *base) +{ + struct odb_source_files *files = odb_source_files_downcast(opts->existing->source); + struct string_list include = STRING_LIST_INIT_DUP; + struct string_list_item *item; + struct strbuf path = STRBUF_INIT; + int ret = 0; + + midx_included_packs(&include, opts, base); + string_list_sort(&step->u.write); + + for_each_string_list_item(item, &include) { + struct packed_git *p; + + if (string_list_has_string(&step->u.write, item->string) || + midx_contains_pack(base, item->string)) + continue; + + strbuf_reset(&path); + strbuf_addf(&path, "%s/%s", opts->packdir, item->string); + p = packfile_store_load_pack(files->packed, path.buf, 1); + if (!p || open_pack_index(p)) { + ret = error(_("cannot open index for %s"), path.buf); + goto out; + } + if (unsigned_add_overflows(step->objects_nr, p->num_objects)) { + ret = error(_("too many objects in MIDX compaction step")); + goto out; + } + step->objects_nr += p->num_objects; + string_list_insert(&step->u.write, item->string); + } + +out: + strbuf_release(&path); + string_list_clear(&include, 0); + return ret; +} + /* - * Build an append-only MIDX plan: a single WRITE step for the freshly - * written packs, plus COPY steps for every existing layer. No + * Build an append-only MIDX plan: a single WRITE step for packs not + * already in the chain, plus COPY steps for every existing layer. No * compaction or merging is performed. */ static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts, @@ -557,17 +604,20 @@ static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts, size_t *steps_nr_p) { struct odb_source_files *files = odb_source_files_downcast(opts->existing->source); + struct string_list include = STRING_LIST_INIT_DUP; + struct string_list_item *item; struct multi_pack_index *m; struct midx_compaction_step *steps = NULL; struct midx_compaction_step *step = NULL; - struct strbuf buf = STRBUF_INIT; size_t steps_nr = 0, steps_alloc = 0; - uint32_t i; odb_reprepare(opts->existing->repo->objects); m = get_multi_pack_index(files->packed); - for (i = 0; i < opts->names->nr; i++) { + midx_included_packs(&include, opts, m); + for_each_string_list_item(item, &include) { + if (midx_contains_pack(m, item->string)) + continue; if (!step) { ALLOC_GROW(steps, st_add(steps_nr, 1), steps_alloc); step = &steps[steps_nr++]; @@ -575,12 +625,9 @@ static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts, step->type = MIDX_COMPACTION_STEP_WRITE; string_list_init_dup(&step->u.write); } - strbuf_reset(&buf); - strbuf_addf(&buf, "pack-%s.idx", - opts->names->items[i].string); - string_list_append(&step->u.write, buf.buf); + string_list_append(&step->u.write, item->string); } - strbuf_release(&buf); + string_list_clear(&include, 0); for (; m; m = m->base_midx) { ALLOC_GROW(steps, st_add(steps_nr, 1), steps_alloc); @@ -729,6 +776,12 @@ static int repack_make_midx_compaction_plan(struct repack_write_midx_opts *opts, if (opts->geometry->midx_tip_rewritten) m = m->base_midx; + if (midx_compaction_step_include_packs(&step, opts, m) < 0) { + midx_compaction_step_release(&step); + ret = -1; + goto out; + } + trace2_data_string("repack", opts->existing->repo, "midx:rewrote-tip", opts->geometry->midx_tip_rewritten ? "true" : "false"); diff --git a/t/t7705-repack-incremental-midx.sh b/t/t7705-repack-incremental-midx.sh index 25a8c40e8ee..4760c920a50 100755 --- a/t/t7705-repack-incremental-midx.sh +++ b/t/t7705-repack-incremental-midx.sh @@ -74,7 +74,7 @@ test_expect_success '--write-midx=incremental without --geometric' ' git repack -d && test_commit second && - git repack --write-midx=incremental && + git repack --write-midx=incremental --write-bitmap-index && git multi-pack-index verify && test_line_count = 1 $midx_chain && @@ -83,7 +83,7 @@ test_expect_success '--write-midx=incremental without --geometric' ' # A second repack appends a new layer without # disturbing the existing one. test_commit third && - git repack --write-midx=incremental && + git repack --write-midx=incremental --write-bitmap-index && git multi-pack-index verify && test_line_count = 2 $midx_chain && @@ -91,10 +91,50 @@ test_expect_success '--write-midx=incremental without --geometric' ' head -n 1 $midx_chain >actual && test_cmp expect actual && + git rev-list --test-bitmap HEAD && git fsck ) ' +test_expect_success 'incremental MIDX includes cruft without a new pack' ' + git init incremental-cruft && + ( + cd incremental-cruft && + git config repack.midxMustContainCruft false && + + test_commit base && + echo cruft | git hash-object -w --stdin && + git repack --cruft -d && + test_commit cruft && + git repack -d && + + # All objects are packed, but the new MIDX still needs cruft. + git repack --write-midx=incremental --write-bitmap-index && + git rev-list --test-bitmap HEAD + ) +' + +test_expect_success 'geometric incremental MIDX retains cruft when replacing its tip' ' + git init geometric-incremental-cruft && + ( + cd geometric-incremental-cruft && + git config repack.midxNewLayerThreshold 1 && + + test_commit base && + echo cruft | git hash-object -w --stdin && + git repack --cruft -d && + git multi-pack-index write --incremental --bitmap && + test_commit cruft && + + # Pack the new commit and tree, leaving the blob in cruft. + git repack -d && + git repack --geometric=2 --write-midx=incremental \ + --write-bitmap-index && + test_line_count = 1 $midx_chain && + git rev-list --test-bitmap HEAD + ) +' + test_expect_success 'below layer threshold, tip packs excluded' ' git init below-layer-threshold-tip-packs-excluded && ( @@ -338,7 +378,7 @@ test_expect_success 'geometric rollup with surviving tip packs' ' ) ' -test_expect_success 'kept packs are excluded from repack' ' +test_expect_success 'kept packs are excluded from repack but included in MIDX' ' git init kept-packs-excluded-from-repack && ( cd kept-packs-excluded-from-repack && @@ -353,21 +393,20 @@ test_expect_success 'kept packs are excluded from repack' ' test_commit "$i" && git repack -d || return 1 done && - keep=$(ls $packdir/pack-*.idx | head -n 1) && - touch "${keep%.idx}.keep" && + keep=$(test-tool find-pack A) && + touch "${keep%.pack}.keep" && - # The kept pack is excluded as a repacking candidate - # entirely, so no rollup occurs as there is only one - # non-kept pack. A new MIDX layer is written containing - # that pack. - git repack --geometric=2 -d --write-midx=incremental && + # Neither pack is repacked, but both are needed for the + # bitmap of B, which reaches objects in the kept pack. + git repack --geometric=2 -d --write-midx=incremental \ + --write-bitmap-index && test-tool read-midx $objdir >actual && grep "^pack-.*\.idx$" actual >actual.packs && - test_line_count = 1 actual.packs && - test_grep ! "$keep" actual.packs && + test_line_count = 2 actual.packs && git multi-pack-index verify && + git rev-list --test-bitmap HEAD && # All objects (from both kept and non-kept packs) # must still be accessible. -- 2.56.0.8.ga42f775cbe2 ``` ## Elijah Newren, 2026-10-01 23:22 Subject: Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow' Message-ID: In-Reply-To: ``` On Wed, Sep 30, 2026 at 3:23 PM Derrick Stolee wrote: > > On 9/29/2026 9:28 PM, Taylor Blau wrote: > > > Add trees and tags from included and '!' packs (and loose ones with > > '--unpacked') as roots in '--stdin-packs=follow' mode. This rescues > > their descendants even when no input commit reaches them. Walk these > > roots after the existing traversal, preserving the `SEEN` bit to avoid > > redundant traversals. Ensure that the walk takes place *after* the > > existing traversal so that we don't lose the path prefix used for trees > > and blobs wherever possible. > > > @@ -3807,6 +3807,7 @@ static int stdin_packs_hints_nr; > > struct stdin_packs_context { > > struct rev_info *revs; > > enum stdin_packs_mode mode; > > + struct oid_array extra_roots; > > I believe this should be an oidset to avoid adding duplicate objects > that appear multiple times. The order of these extra roots doesn't > matter (such as in a --topo-order walk). We only care about the > binary "reachable or not?" question. Is that true? After iterating with copilot for a while, it says: The walk supplies paths to pack-objects, and those paths affect more than reachability. In particular, they affect both namehash-based delta selection and path-based attributes. First, it is possible to demonstrate a pack-quality regression with a vanilla repository configuration: test_expect_success '--stdin-packs=follow preserves namehash ordering' ' test_when_finished "rm -rf repo" && git init repo && ( cd repo && mkdir sub && test-tool genrandom similar 8192 >sub/a && cp sub/a sub/c && printf x >>sub/c && for name in \ b yzz9 zzz9 aazz9 abzz9 aczz9 \ adyz9 adzz9 aeyz9 aezz9 afyz9 do test-tool genrandom "unrelated-9-$name" 8192 >"$name" || return 1 done && git add . && git commit -m base && git rev-parse HEAD^{tree} HEAD:sub >in && P=$(git pack-objects --window=0 $packdir/pack in && git pack-objects --stdin-packs=follow \ --no-reuse-delta $packdir/pack actual && printf "%s\n" "$(git rev-parse HEAD:sub/c)" \ "$ZERO_OID" >expect && test_cmp expect actual ) ' The funny-looking names make the test deterministic: their namehashes fall between the hashes for a and c, but not between those for sub/a and sub/c. There are enough of them to fill the normal default delta window. --window=0 on the input pack and --no-reuse-delta on the output merely ensure that the test observes the new delta search; the output pack uses the normal default window. With the submitted oid_array, the parent-first input-pack order is preserved. sub/a and sub/c remain adjacent in the namehash sort, and sub/a is written as a six-byte delta against sub/c. With the straightforward oidset conversion, hash iteration visits the subtree first. The blobs are named a and c, the unrelated blobs separate them in the namehash sort, and both are written in full. Both packs are valid, so this is a pack-quality regression rather than repository corruption. There is also a shorter, but admittedly more contrived, example using path-based attributes: test_expect_success '--stdin-packs=follow preserves paths for attributes' ' test_when_finished "rm -rf repo" && git init repo && ( cd repo && echo "sub/* -delta" >.gitattributes && mkdir sub && test-tool genrandom seed-2 8192 >sub/a && cp sub/a sub/b && echo modified >>sub/b && git add . && git commit -m base && git rev-parse HEAD^{tree} HEAD:sub >in && P=$(git pack-objects $packdir/pack in && git pack-objects --stdin-packs=follow $packdir/pack actual && printf "%s\n" "$ZERO_OID" "$ZERO_OID" >expect && test_cmp expect actual ) ' With the oid_array and parent-first pack order, the blobs are visited as sub/a and sub/b, so sub/* -delta applies. With the oidset, the subtree is visited first and the blobs are seen as a and b, so one is delta-compressed. When the root is processed later, the subtree is already marked SEEN and is not revisited with the sub/ prefix. The oid_array does not manufacture parent-before-child ordering if the input pack itself has the subtree first; this path information is explicitly best-effort. But it preserves a useful order when one exists, whereas an oidset discards it. ``` ## Taylor Blau, 2026-10-02 00:51 Subject: Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow' Message-ID: In-Reply-To: ``` On Thu, Oct 01, 2026 at 04:22:11PM -0700, Elijah Newren wrote: > With the oid_array and parent-first pack order, the blobs are visited > as sub/a and sub/b, so sub/* -delta applies. With the oidset, the > subtree is visited first and the blobs are seen as a and b, so one is > delta-compressed. When the root is processed later, the subtree is > already marked SEEN and is not revisited with the sub/ prefix. > > The oid_array does not manufacture parent-before-child ordering if the > input pack itself has the subtree first; this path information is > explicitly best-effort. But it preserves a useful order when one > exists, whereas an oidset discards it. Sure, though as Peff and I discussed elsewhere in the thread, there are also situations where you can produce a sub-optimal pack even with oid_array. That's because the namehash you get for a given tree object depends on the path you took to get there. So you can certainly come up with examples where the ordering of tree objects in an array of extra roots produces a lesser-quality delta selection than the same objects permuted into some different order. The other thing to keep in mind is that, while there are clearly trade-offs as we have discussed here, the oidset ensures that we don't allocate memory wastefully when the same object is listed multiple times as an extra root. The other other thing to keep in mind is that the size of this set is almost always going to be puny compared to the size of the overall pack. These objects are merely meant to pull in the (likely) few objects that need refreshed out of the cruft pack in order to ensure reachability closure. So I think it's clear that this is a trade-off, and neither decision (oidset vs oid_array) is absolutely perfect for all cases. But on balance I think that the trade-offs push us towards oidset much more than they do towards oid_array. Thanks, Taylor ``` ## Jeff King, 2026-10-02 23:02 Subject: Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow' Message-ID: <20261002230225.GA834759@coredump.intra.peff.net> In-Reply-To: ``` On Thu, Oct 01, 2026 at 07:51:48PM -0500, Taylor Blau wrote: > On Thu, Oct 01, 2026 at 04:22:11PM -0700, Elijah Newren wrote: > > With the oid_array and parent-first pack order, the blobs are visited > > as sub/a and sub/b, so sub/* -delta applies. With the oidset, the > > subtree is visited first and the blobs are seen as a and b, so one is > > delta-compressed. When the root is processed later, the subtree is > > already marked SEEN and is not revisited with the sub/ prefix. > > > > The oid_array does not manufacture parent-before-child ordering if the > > input pack itself has the subtree first; this path information is > > explicitly best-effort. But it preserves a useful order when one > > exists, whereas an oidset discards it. > > Sure, though as Peff and I discussed elsewhere in the thread, there are > also situations where you can produce a sub-optimal pack even with > oid_array. That's because the namehash you get for a given tree object > depends on the path you took to get there. > > So you can certainly come up with examples where the ordering of tree > objects in an array of extra roots produces a lesser-quality delta > selection than the same objects permuted into some different order. Hmm. Yeah, it is not a 100% solved issue, for sure, but I think Elijah has a point. Even though yes, we may see trees in weird orders between packs, or when visited separate from another commit, the ordering in a single pack _is_ useful, because it puts root trees before subtrees. So even though these are a few objects we're rescuing out of a cruft pack, we'd expect them to be correlated. E.g., an update to "a/b/c/file" is going to have four trees: the root, a, a/b, and a/b/c. And we'd like to visit them in that order. Which is the order in which we'd typically write them in a pack. One thing I'm not 100% on is if that "typically" qualifier applies to cruft packs. We might be throwing objects in there with a little less thought, because the point is that they're _not_ reachable, and we didn't get there from a traversal. So I dunno. > The other thing to keep in mind is that, while there are clearly > trade-offs as we have discussed here, the oidset ensures that we don't > allocate memory wastefully when the same object is listed multiple times > as an extra root. Yeah, that was my thinking when endorsing the oidset earlier; it is better bounded. It can have worse memory use in practice, though, because it's a hash table rather than a vanilla array. So if we don't expect a lot of duplicates, then the simpler array may be more efficient. -Peff ``` ## Jeff King, 2026-10-02 23:13 Subject: Re: [PATCH v2 2/8] pack-objects: ensure tree/tag closure with '--stdin-packs=follow' Message-ID: <20261002231336.GB834759@coredump.intra.peff.net> In-Reply-To: <940953e5c407046ac6789367f3341dc8ea73d07a.1790827875.git.me@ttaylorr.com> ``` On Wed, Sep 30, 2026 at 11:11:39PM -0500, Taylor Blau wrote: > @@ -4151,6 +4158,34 @@ static void read_stdin_packs(struct repository *repo, > show_object_pack_hint, > &mode); > > + /* > + * Trees and tags need closure even when no commit reaches them. > + * Defer adding these roots to revs.pending until the first walk > + * finishes. Otherwise a subtree may be visited and marked SEEN > + * before its commit's root tree, using "a" instead of "sub/a" > + * for a blob's namehash and delta attributes. > + * > + * Tags may introduce more commits in the second walk, so this > + * does not *always* guarantee that trees are always visited > + * with their full paths. > + */ I think one of the things that confused me reading the original patch (but is still present here) is that this comment is in read_stdin_packs, when we're actually doing the walks. So "defer adding these roots" feels quite late. We already did that deferring long before in add_object_entry_from_pack() and add_loose_object(), when we called oidset_insert() instead of add_pending(). So it would have made more sense to me to comment it there. Of course that is hard when there are two such places. I dunno. > + oidset_iter_init(&ctx.extra_roots, &iter); > + while ((oid = oidset_iter_next(&iter))) { > + struct object *obj = lookup_object(repo, oid); > + > + if (!obj || !(obj->flags & SEEN)) > + add_pending_oid(&revs, NULL, oid, 0); > + } > + if (revs.pending.nr) { > + if (prepare_revision_walk(&revs)) > + die(_("revision walk setup failed")); > + traverse_commit_list(&revs, > + show_commit_pack_hint, > + show_object_pack_hint, > + &mode); > + } > + oidset_clear(&ctx.extra_roots); > + > release_revisions(&revs); BTW, is it safe to prepare_revision_walk() twice on the same rev_info? I could believe it works, but I could also believe that there are hidden corner cases, as I don't think it was ever really intended to work this way. Maybe OK for the vanilla set of options we are using here (as opposed to taking arbitrary options from the user). The rev_info is created locally in this function, though, so I guess if we wanted to be double-plus sure we could release and reinit the struct. -Peff ``` ## Jeff King, 2026-10-02 23:16 Subject: Re: [PATCH v2 4/8] repack: use a sorted list for explicitly kept packs Message-ID: <20261002231601.GC834759@coredump.intra.peff.net> In-Reply-To: ``` On Wed, Sep 30, 2026 at 11:11:47PM -0500, Taylor Blau wrote: > `existing_packs_collect()` performs a linear search through the > '--keep-pack' arguments for each local pack. Typically the number of > such arguments is small enough that the difference between a linear and > binary search is just noise (especially compared with the amount of work > that 'repack' is about to perform). > > However, an additional caller will wish to search through the same list. > To prevent that caller from having to duplicate the clunky for-loop in > `existing_packs_collect()`, sort the list using `fspathcmp()` and > replace the existing caller's loop with `string_list_has_string()`. > > This does not change the overall behavior of '--keep-pack' arguments. OK, makes sense, and the patch looks correct. -Peff ``` ## Jeff King, 2026-10-02 23:25 Subject: Re: [PATCH v2 5/8] repack: follow kept packs when omitting cruft from the MIDX Message-ID: <20261002232529.GD834759@coredump.intra.peff.net> In-Reply-To: <51e20444dac1223f0e0485dc5799ed6d592f7614.1790827875.git.me@ttaylorr.com> ``` On Wed, Sep 30, 2026 at 11:11:51PM -0500, Taylor Blau wrote: > diff --git a/builtin/repack.c b/builtin/repack.c > index 88b05e96b5b..27d6668a4ab 100644 > --- a/builtin/repack.c > +++ b/builtin/repack.c > @@ -476,9 +476,11 @@ int cmd_repack(int argc, > show_progress = !po_args.quiet && isatty(2); > > strvec_push(&cmd.args, "--keep-true-parents"); > - for (i = 0; i < keep_pack_list.nr; i++) > - strvec_pushf(&cmd.args, "--keep-pack=%s", > - keep_pack_list.items[i].string); > + /* Geometric follow walks exclude these packs through stdin instead. */ > + if (!(geometry.split_factor && !midx_must_contain_cruft)) > + for (i = 0; i < keep_pack_list.nr; i++) > + strvec_pushf(&cmd.args, "--keep-pack=%s", > + keep_pack_list.items[i].string); This conditional makes my head hurt because of the double-negation. By De Morgan's it is just: if (!geometry.split_factor || midx_must_contain_cruft) which at least untangles it. The comment makes sense to say "we do not need to do this in geometric" mode, which matches the first half. But why does midx_must_contain_cruft trigger it? I guess it is "we do not need to bother doing the "^"-exclusion later in that mode", but I wonder if there is any advantage to suppressing it. I don't remember enough of the details here about why we were treating keep packs specially in the first place. > @@ -593,6 +595,29 @@ int cmd_repack(int argc, > > fprintf(in, "%c%s\n", marker, basename); > } > + if (!midx_must_contain_cruft) { OK, and this is the flip side of the earlier conditional. We are in geometric mode if we get here, and we kick in only in non-midx-cruft mode. IMHO the De Morgan untangling above makes it more clear, but you could probably even further with: /* explanatory comment here */ int handle_keep_packs_via_follow = geometry.split_factor && !midx_must_contain_cruft; And then use that in both spots. That might be overkill, though (and the name I proposed certainly sucks). -Peff ``` ## Jeff King, 2026-10-02 23:28 Subject: Re: [PATCH v2 6/8] repack: track the preferred pack explicitly in MIDX write steps Message-ID: <20261002232834.GE834759@coredump.intra.peff.net> In-Reply-To: ``` On Wed, Sep 30, 2026 at 11:11:58PM -0500, Taylor Blau wrote: > A MIDX write step marks preferred packs in its string-list entries and > chooses the last marked entry when executing the step. That makes the > choice depend on list order, preventing the list from being sorted for > membership checks. > > Record the last candidate directly in the step, borrowing its name from > the write list. This preserves preferred-pack selection while allowing > the list to be sorted without changing that choice. This is certainly cleaner, though it looks like the existing code works by marking item->util and then doing a linear search for it. So wouldn't that work even after sorting? > @@ -719,7 +713,7 @@ static int repack_make_midx_compaction_plan(struct repack_write_midx_opts *opts, > > item = string_list_append(&step.u.write, buf.buf); > if (p->multi_pack_index || i == opts->geometry->pack_nr - 1) > - item->util = (void *)1; /* mark as preferred */ > + step.preferred_pack = item->string; I am certainly happy to see these gross casts go away, though. -Peff ``` ## Jeff King, 2026-10-02 23:41 Subject: Re: [PATCH v2 8/8] repack: include required packs in incremental MIDX writes Message-ID: <20261002234157.GF834759@coredump.intra.peff.net> In-Reply-To: ``` On Wed, Sep 30, 2026 at 11:12:05PM -0500, Taylor Blau wrote: > The geometric plan from 1da62fb5c86 (repack: implement incremental MIDX > repacking, 2026-05-19) can omit kept and cruft packs, since neither > necessarily participates in the geometric repack. Such packs can also be > lost when replacing a tip layer that contains them. Neither plan > consults `midx_included_packs()`, so the rules for retaining cruft in > ordinary MIDX writes do not protect incremental writes. > > Use that selection logic to add missing packs to each plan's write step. > Skip packs in retained base layers, but include required packs from a > replaced tip. Count added objects when choosing which layers to compact, > without changing the preferred pack. I admit I had a hard time following this patch. I think the point is that we're going to include some packs in the midx that were not covered previously. But it was hard to see where that happens. I think the magic bit is this: > @@ -557,17 +604,20 @@ static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts, > size_t *steps_nr_p) > [..] > - for (i = 0; i < opts->names->nr; i++) { > + midx_included_packs(&include, opts, m); where we rely on midx_included_packs() to do that selection. So I _think_ this is doing the right thing, but my confidence in my review is kind of low. To some degree I'd just rely on the functional tests here. -Peff ``` ## Taylor Blau, 2026-10-03 00:50 Subject: Re: [PATCH v2 5/8] repack: follow kept packs when omitting cruft from the MIDX Message-ID: In-Reply-To: <20261002232529.GD834759@coredump.intra.peff.net> ``` On Fri, Oct 02, 2026 at 07:25:29PM -0400, Jeff King wrote: > On Wed, Sep 30, 2026 at 11:11:51PM -0500, Taylor Blau wrote: > > > diff --git a/builtin/repack.c b/builtin/repack.c > > index 88b05e96b5b..27d6668a4ab 100644 > > --- a/builtin/repack.c > > +++ b/builtin/repack.c > > @@ -476,9 +476,11 @@ int cmd_repack(int argc, > > show_progress = !po_args.quiet && isatty(2); > > > > strvec_push(&cmd.args, "--keep-true-parents"); > > - for (i = 0; i < keep_pack_list.nr; i++) > > - strvec_pushf(&cmd.args, "--keep-pack=%s", > > - keep_pack_list.items[i].string); > > + /* Geometric follow walks exclude these packs through stdin instead. */ > > + if (!(geometry.split_factor && !midx_must_contain_cruft)) > > + for (i = 0; i < keep_pack_list.nr; i++) > > + strvec_pushf(&cmd.args, "--keep-pack=%s", > > + keep_pack_list.items[i].string); > > This conditional makes my head hurt because of the double-negation. By > De Morgan's it is just: > > if (!geometry.split_factor || midx_must_contain_cruft) Yeah, I struggled a bit when writing it TBH and flip-flopped between the two. I read the conditional (as proposed in my patch) as: "If we aren't doing a geometric repack where the MIDX is allowed to omit cruft objects". But I think the original sin here is midx_must_contain_cruft, which probably should have been midx_may_exclude_cruft, which defaults to false as opposed to the former which defaults to true. It's not quite a double negation, but I agree that it's a little awkward. TBH I find the rewritten version just as confusing if not more so. Thanks, Taylor ``` ## Taylor Blau, 2026-10-03 00:55 Subject: Re: [PATCH v2 2/8] pack-objects: ensure tree/tag closure with '--stdin-packs=follow' Message-ID: In-Reply-To: <20261002231336.GB834759@coredump.intra.peff.net> ``` On Fri, Oct 02, 2026 at 07:13:36PM -0400, Jeff King wrote: > So it would have made more sense to me to comment it there. Of course > that is hard when there are two such places. > > I dunno. Yeah, me either. I'm happy to change things around if you feel strongly. > > + oidset_iter_init(&ctx.extra_roots, &iter); > > + while ((oid = oidset_iter_next(&iter))) { > > + struct object *obj = lookup_object(repo, oid); > > + > > + if (!obj || !(obj->flags & SEEN)) > > + add_pending_oid(&revs, NULL, oid, 0); > > + } > > + if (revs.pending.nr) { > > + if (prepare_revision_walk(&revs)) > > + die(_("revision walk setup failed")); > > + traverse_commit_list(&revs, > > + show_commit_pack_hint, > > + show_object_pack_hint, > > + &mode); > > + } > > + oidset_clear(&ctx.extra_roots); > > + > > release_revisions(&revs); > > BTW, is it safe to prepare_revision_walk() twice on the same rev_info? I > could believe it works, but I could also believe that there are hidden > corner cases, as I don't think it was ever really intended to work this > way. > > Maybe OK for the vanilla set of options we are using here (as opposed to > taking arbitrary options from the user). The rev_info is created locally > in this function, though, so I guess if we wanted to be double-plus sure > we could release and reinit the struct. It seems to work in practice. From reading through and thinking about it I couldn't find any obvious issues. Just as well, there are a couple of spots that I was able to find that already call `prepare_revision_walk()` more than once: * In builtin/pack-objects.c::get_object_list() (with the exception of '--path-walk') we call `prepare_revision_walk()` twice when exploding unreachable objects as loose. * In reachable.c::mark_reachable_objects(), we also call the `prepare_revision_walk()` function twice when given a timestamp via `mark_recent`. This all works since `revs.pending` is emptied by the first revwalk. But it is under-documented, so callers relying on this behavior may be surprised if/when it changes. Probably good #leftoverbits. Thanks, Taylor ``` ## Taylor Blau, 2026-10-03 01:00 Subject: Re: [PATCH v2 6/8] repack: track the preferred pack explicitly in MIDX write steps Message-ID: In-Reply-To: <20261002232834.GE834759@coredump.intra.peff.net> ``` On Fri, Oct 02, 2026 at 07:28:34PM -0400, Jeff King wrote: > On Wed, Sep 30, 2026 at 11:11:58PM -0500, Taylor Blau wrote: > > > A MIDX write step marks preferred packs in its string-list entries and > > chooses the last marked entry when executing the step. That makes the > > choice depend on list order, preventing the list from being sorted for > > membership checks. > > > > Record the last candidate directly in the step, borrowing its name from > > the write list. This preserves preferred-pack selection while allowing > > the list to be sorted without changing that choice. > > This is certainly cleaner, though it looks like the existing code works > by marking item->util and then doing a linear search for it. So wouldn't > that work even after sorting? It would if only one entry were marked, but we can mark several. For example, when `repack_make_midx_compaction_plan()` folds multiple MIDX layers into one via a WRITE step, it marks each layer's preferred pack without clearing the earlier marks. The scan doesn't stop at the first such mark, and the last marked entry wins. So sorting would of course preserve the marks, but may change which one comes last. > > @@ -719,7 +713,7 @@ static int repack_make_midx_compaction_plan(struct repack_write_midx_opts *opts, > > > > item = string_list_append(&step.u.write, buf.buf); > > if (p->multi_pack_index || i == opts->geometry->pack_nr - 1) > > - item->util = (void *)1; /* mark as preferred */ > > + step.preferred_pack = item->string; > > I am certainly happy to see these gross casts go away, though. Me too ;-). Thanks, Taylor ``` ## Taylor Blau, 2026-10-03 01:01 Subject: Re: [PATCH v2 8/8] repack: include required packs in incremental MIDX writes Message-ID: In-Reply-To: <20261002234157.GF834759@coredump.intra.peff.net> ``` On Fri, Oct 02, 2026 at 07:41:57PM -0400, Jeff King wrote: > On Wed, Sep 30, 2026 at 11:12:05PM -0500, Taylor Blau wrote: > > > The geometric plan from 1da62fb5c86 (repack: implement incremental MIDX > > repacking, 2026-05-19) can omit kept and cruft packs, since neither > > necessarily participates in the geometric repack. Such packs can also be > > lost when replacing a tip layer that contains them. Neither plan > > consults `midx_included_packs()`, so the rules for retaining cruft in > > ordinary MIDX writes do not protect incremental writes. > > > > Use that selection logic to add missing packs to each plan's write step. > > Skip packs in retained base layers, but include required packs from a > > replaced tip. Count added objects when choosing which layers to compact, > > without changing the preferred pack. > > I admit I had a hard time following this patch. I think the point is > that we're going to include some packs in the midx that were not covered > previously. But it was hard to see where that happens. I think the magic > bit is this: > > > @@ -557,17 +604,20 @@ static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts, > > size_t *steps_nr_p) > > [..] > > - for (i = 0; i < opts->names->nr; i++) { > > + midx_included_packs(&include, opts, m); > > where we rely on midx_included_packs() to do that selection. Yeah, that's right. I wrote this code in the first place, and it wasn't even *that* long ago and I had to spend a not-insignificant amount of time (re)acquainting myself with this area before writing this patch. Thanks, Taylor ``` ## Jeff King, 2026-10-03 01:06 Subject: Re: [PATCH v2 2/8] pack-objects: ensure tree/tag closure with '--stdin-packs=follow' Message-ID: <20261003010613.GA839051@coredump.intra.peff.net> In-Reply-To: ``` On Fri, Oct 02, 2026 at 07:55:32PM -0500, Taylor Blau wrote: > On Fri, Oct 02, 2026 at 07:13:36PM -0400, Jeff King wrote: > > So it would have made more sense to me to comment it there. Of course > > that is hard when there are two such places. > > > > I dunno. > > Yeah, me either. I'm happy to change things around if you feel strongly. I don't. If there were an easy solution I probably would. ;) > > BTW, is it safe to prepare_revision_walk() twice on the same rev_info? I > > could believe it works, but I could also believe that there are hidden > > corner cases, as I don't think it was ever really intended to work this > > way. > [...] > Just as well, there are a couple of spots that I was able to find that > already call `prepare_revision_walk()` more than once: OK. That makes me feel like we're in good company, at least. If some combination turns out to be a problem, we can deal with it later. > * In builtin/pack-objects.c::get_object_list() (with the exception of > '--path-walk') we call `prepare_revision_walk()` twice when > exploding unreachable objects as loose. > > * In reachable.c::mark_reachable_objects(), we also call the > `prepare_revision_walk()` function twice when given a timestamp via > `mark_recent`. I have a feeling that least one of those is my fault, too. ;) -Peff ``` ## Jeff King, 2026-10-03 01:07 Subject: Re: [PATCH v2 6/8] repack: track the preferred pack explicitly in MIDX write steps Message-ID: <20261003010720.GB839051@coredump.intra.peff.net> In-Reply-To: ``` On Fri, Oct 02, 2026 at 08:00:52PM -0500, Taylor Blau wrote: > On Fri, Oct 02, 2026 at 07:28:34PM -0400, Jeff King wrote: > > On Wed, Sep 30, 2026 at 11:11:58PM -0500, Taylor Blau wrote: > > > > > A MIDX write step marks preferred packs in its string-list entries and > > > chooses the last marked entry when executing the step. That makes the > > > choice depend on list order, preventing the list from being sorted for > > > membership checks. > > > > > > Record the last candidate directly in the step, borrowing its name from > > > the write list. This preserves preferred-pack selection while allowing > > > the list to be sorted without changing that choice. > > > > This is certainly cleaner, though it looks like the existing code works > > by marking item->util and then doing a linear search for it. So wouldn't > > that work even after sorting? > > It would if only one entry were marked, but we can mark several. > > For example, when `repack_make_midx_compaction_plan()` folds multiple > MIDX layers into one via a WRITE step, it marks each layer's preferred pack > without clearing the earlier marks. The scan doesn't stop at the first > such mark, and the last marked entry wins. > > So sorting would of course preserve the marks, but may change which one > comes last. OK, that does make more sense. Re-reading your commit message again, I see it even says that, but somehow it didn't quite sink in the first time for me. -Peff ```