[PATCH 0/4] repack: various corner cases for cruft-less MIDXs

[PATCH 0/4] repack: various corner cases for cruft-less MIDXs

[Taylor Blau]

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

[PATCH 1/4] pack-objects: introduce `stdin_packs_context` struct

[Taylor Blau]

`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 <ttaylorr@openai.com>
---
 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

Re: [PATCH 1/4] pack-objects: introduce `stdin_packs_context` struct

[Junio C Hamano]

Taylor Blau <ttaylorr@openai.com> 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.

Re: [PATCH 1/4] pack-objects: introduce `stdin_packs_context` struct

[Taylor Blau]

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

[PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'

[Taylor Blau]

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 <ttaylorr@openai.com>
---
 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 <in >/dev/null &&
+
+		test_trace2_data pack-objects stdin_packs_hints 0 <trace.txt &&
+
+		P=$(git pack-objects --stdin-packs=follow $packdir/pack <in) &&
+		git rev-parse "$tree" "$tree:base.t" >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) &&
+		git prune-packed &&
+
+		echo "!pack-$P.pack" >in &&
+		P=$(git pack-objects --stdin-packs=follow $packdir/pack <in) &&
+		objects_in_packs $P >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) &&
+		echo "pack-$P.pack" >in &&
+
+		git pack-objects --stdin-packs=follow $packdir/pack <in &&
+		git prune-packed &&
+
+		printf "%s\n" HEAD:sub/a HEAD:sub/b |
+			git cat-file --batch-check="%(deltabase)" >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

Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'

[Junio C Hamano]

Taylor Blau <ttaylorr@openai.com> 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?

Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'

[Derrick Stolee]

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

Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'

[Taylor Blau]

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

Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'

[Elijah Newren]

On Wed, Sep 30, 2026 at 3:23 PM Derrick Stolee <stolee@gmail.com> 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) &&
        echo "pack-$P.pack" >in &&

        git pack-objects --stdin-packs=follow \
            --no-reuse-delta $packdir/pack <in &&
        rm "$packdir/pack-$P.pack" "$packdir/pack-$P.idx" &&
        git prune-packed &&

        printf "%s\n" HEAD:sub/a HEAD:sub/c |
            git cat-file --batch-check="%(deltabase)" >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) &&
        echo "pack-$P.pack" >in &&

        git pack-objects --stdin-packs=follow $packdir/pack <in &&
        git prune-packed &&

        printf "%s\n" HEAD:sub/a HEAD:sub/b |
            git cat-file --batch-check="%(deltabase)" >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.

Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'

[Taylor Blau]

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

Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'

[Jeff King]

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

Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'

[Jeff King]

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

Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'

[Taylor Blau]

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

[PATCH 3/4] repack: retain cruft packs in MIDXs after incremental repacks

[Taylor Blau]

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 <ttaylorr@openai.com>
---
 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

Re: [PATCH 3/4] repack: retain cruft packs in MIDXs after incremental repacks

[Jeff King]

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

Re: [PATCH 3/4] repack: retain cruft packs in MIDXs after incremental repacks

[Taylor Blau]

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

[PATCH 4/4] repack: retain cruft packs in MIDXs containing kept packs

[Taylor Blau]

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 <ttaylorr@openai.com>
---
 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 <objects) &&
+		touch $packdir/pack-$pack.keep &&
+		git prune-packed &&
+
+		# The new blob is still loose, so this writes a pack instead
+		# of taking the no-new-packs fallback.
+		GIT_TEST_MULTI_PACK_INDEX=0 \
+		git repack -d --geometric=2 --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

Re: [PATCH 4/4] repack: retain cruft packs in MIDXs containing kept packs

[Jeff King]

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

Re: [PATCH 4/4] repack: retain cruft packs in MIDXs containing kept packs

[Taylor Blau]

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

Re: [PATCH 0/4] repack: various corner cases for cruft-less MIDXs

[Jeff King]

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

Re: [PATCH 0/4] repack: various corner cases for cruft-less MIDXs

[Taylor Blau]

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

[PATCH v2 0/8] repack: various corner cases for cruft-less MIDXs

[Taylor Blau]

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 <ttaylorr@openai.com>
    -    Signed-off-by: Taylor Blau <me@ttaylorr.com>
     
      ## 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 <ttaylorr@openai.com>
    -    Signed-off-by: Taylor Blau <me@ttaylorr.com>
     
      ## 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 <ttaylorr@openai.com>
    -    Signed-off-by: Taylor Blau <me@ttaylorr.com>
     
      ## 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 <me@ttaylorr.com>
     
      ## 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 <ttaylorr@openai.com>
    -    Signed-off-by: Taylor Blau <me@ttaylorr.com>
    +
    + ## 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 <objects) &&
    -+		touch $packdir/pack-$pack.keep &&
    -+		git prune-packed &&
    ++		test_commit base &&
    ++		echo cruft | git hash-object -w --stdin &&
    ++		git repack --cruft -d &&
    ++		git multi-pack-index write --incremental --bitmap &&
    ++		test_commit cruft &&
     +
    -+		# The new blob is still loose, so this writes a pack instead
    -+		# of taking the no-new-packs fallback.
    -+		GIT_TEST_MULTI_PACK_INDEX=0 \
    -+		git repack -d --geometric=2 --write-midx --write-bitmap-index &&
    ++		# 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 'repack --write-midx includes cruft when instructed' '
    - 	setup_cruft_exclude_tests exclude-cruft-when-instructed &&
    + test_expect_success 'below layer threshold, tip packs excluded' '
    + 	git init below-layer-threshold-tip-packs-excluded &&
    + 	(
    +@@ t/t7705-repack-incremental-midx.sh: 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 &&
    +@@ t/t7705-repack-incremental-midx.sh: 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.

base-commit: a018953688f1b10bddf91bff8747068f5f4746a4
-- 
2.56.0.8.ga42f775cbe2

[PATCH v2 1/8] pack-objects: introduce `stdin_packs_context` struct

[Taylor Blau]

`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 <ttaylorr@openai.com>
---
 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

[PATCH v2 2/8] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'

[Taylor Blau]

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 <ttaylorr@openai.com>
---
 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 <in >/dev/null &&
+
+		test_trace2_data pack-objects stdin_packs_hints 0 <trace.txt &&
+
+		P=$(git pack-objects --stdin-packs=follow $packdir/pack <in) &&
+		git rev-parse "$tree" "$tree:base.t" >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) &&
+		git prune-packed &&
+
+		echo "!pack-$P.pack" >in &&
+		P=$(git pack-objects --stdin-packs=follow $packdir/pack <in) &&
+		objects_in_packs $P >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) &&
+		echo "pack-$P.pack" >in &&
+
+		git pack-objects --stdin-packs=follow $packdir/pack <in &&
+		git prune-packed &&
+
+		printf "%s\n" HEAD:sub/a HEAD:sub/b |
+			git cat-file --batch-check="%(deltabase)" >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

Re: [PATCH v2 2/8] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'

[Jeff King]

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

Re: [PATCH v2 2/8] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'

[Taylor Blau]

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

Re: [PATCH v2 2/8] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'

[Jeff King]

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

[PATCH v2 3/8] repack: retain cruft packs in MIDXs after incremental repacks

[Taylor Blau]

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 <ttaylorr@openai.com>
---
 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

[PATCH v2 4/8] repack: use a sorted list for explicitly kept packs

[Taylor Blau]

`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 <ttaylorr@openai.com>
---
 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

Re: [PATCH v2 4/8] repack: use a sorted list for explicitly kept packs

[Jeff King]

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

[PATCH v2 5/8] repack: follow kept packs when omitting cruft from the MIDX

[Taylor Blau]

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 <ttaylorr@openai.com>
---
 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

Re: [PATCH v2 5/8] repack: follow kept packs when omitting cruft from the MIDX

[Jeff King]

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

Re: [PATCH v2 5/8] repack: follow kept packs when omitting cruft from the MIDX

[Taylor Blau]

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

[PATCH v2 6/8] repack: track the preferred pack explicitly in MIDX write steps

[Taylor Blau]

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

Re: [PATCH v2 6/8] repack: track the preferred pack explicitly in MIDX write steps

[Jeff King]

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

Re: [PATCH v2 6/8] repack: track the preferred pack explicitly in MIDX write steps

[Taylor Blau]

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

Re: [PATCH v2 6/8] repack: track the preferred pack explicitly in MIDX write steps

[Jeff King]

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

[PATCH v2 7/8] repack: defer allocating the append plan's write step

[Taylor Blau]

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 <ttaylorr@openai.com>
---
 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

[PATCH v2 8/8] repack: include required packs in incremental MIDX writes

[Taylor Blau]

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 <ttaylorr@openai.com>
---
 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

Re: [PATCH v2 8/8] repack: include required packs in incremental MIDX writes

[Jeff King]

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

Re: [PATCH v2 8/8] repack: include required packs in incremental MIDX writes

[Taylor Blau]

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