[PATCH 0/3] midx: honor custom bases for incremental writes

[PATCH 0/3] midx: honor custom bases for incremental writes

[Taylor Blau]

SZEDER noticed[1] that t5334 was trying to call `nth_line()`, despite
that helper living only in t5335.

Fixing that should have made the test exercise `git multi-pack-index
write --incremental --base=...`. Instead, it uncovered another wrinkle,
which is that the normal MIDX write path parsed "--bases" without
actually passing it down to the MIDX writer.

This short series fixes both issues. It is structured as follows:

 * The first patch moves `nth_line()` to lib-midx.sh so that t5334 and
   t5335 use the same helper.

 * The second patch threads the parsed `--base` value through
   `write_midx_file()`, and consequently marks two t5334 cases as known
   breakages.

 * The final patch fixes the pack inclusion check and marks the tests
   successful again.

The result is that `--base=none` and `--base=<hash>` now correctly
produce detached incremental layers that include any packs above the
selected base, preserving reachability closure for bitmaps.

Thanks in advance for your review!

[1]: https://lore.kernel.org/git/aiuaf3fKJ6kIITrf@szeder.dev/

Taylor Blau (3):
  t5334: expose shared `nth_line()` helper
  midx: pass custom '--base' through incremental writes
  midx-write: include packs above custom incremental base

 builtin/multi-pack-index.c              |  3 ++-
 builtin/repack.c                        |  2 +-
 midx-write.c                            | 18 +++++++++++++-----
 midx.h                                  |  2 +-
 t/lib-midx.sh                           |  6 ++++++
 t/t5334-incremental-multi-pack-index.sh | 20 +++++++++++++++++---
 t/t5335-compact-multi-pack-index.sh     |  7 +------
 7 files changed, 41 insertions(+), 17 deletions(-)


base-commit: 3e65291872de10c3f0bf05ea8c24187e7a71ebf0
-- 
2.55.0.rc0.3.g7bf7c87b605

[PATCH 1/3] t5334: expose shared `nth_line()` helper

[Taylor Blau]

Since commit 0cd2255e64b (midx: support custom `--base` for incremental
MIDX writes, 2026-05-19), t5334 has referred to a non-existent helper
function 'nth_line', which is defined in t5335, but not here.

Move the helper to lib-midx.sh so that both tests can use the same
implementation. Ensure likewise that `nth_line()` remains visible from
within t5335 by sourcing lib-midx.sh there appropriately.

Curiously, t5334 passes both before and after this change. Before this
change, the failed command substitution leaves '--base' with an empty
value, and after this change, the custom base value is still ignored by
the normal incremental write path. The following commits will explain
and address that behavior.

Noticed-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/lib-midx.sh                       | 6 ++++++
 t/t5335-compact-multi-pack-index.sh | 7 +------
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/t/lib-midx.sh b/t/lib-midx.sh
index e38c609604c..b522dbdb0f4 100644
--- a/t/lib-midx.sh
+++ b/t/lib-midx.sh
@@ -34,3 +34,9 @@ compare_results_with_midx () {
 		midx_git_two_modes "cat-file --batch-all-objects --batch-check --unordered" sorted
 	'
 }
+
+nth_line() {
+	local n="$1"
+	shift
+	awk "NR==$n" "$@"
+}
diff --git a/t/t5335-compact-multi-pack-index.sh b/t/t5335-compact-multi-pack-index.sh
index ec1dafe89fc..6a4b799b9c9 100755
--- a/t/t5335-compact-multi-pack-index.sh
+++ b/t/t5335-compact-multi-pack-index.sh
@@ -3,6 +3,7 @@
 test_description='multi-pack-index compaction'
 
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-midx.sh
 
 GIT_TEST_MULTI_PACK_INDEX=0
 GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0
@@ -13,12 +14,6 @@ packdir=$objdir/pack
 midxdir=$packdir/multi-pack-index.d
 midx_chain=$midxdir/multi-pack-index-chain
 
-nth_line() {
-	local n="$1"
-	shift
-	awk "NR==$n" "$@"
-}
-
 write_packs () {
 	for c in "$@"
 	do
-- 
2.55.0.rc0.3.g7bf7c87b605

[PATCH 2/3] midx: pass custom '--base' through incremental writes

[Taylor Blau]

The 'multi-pack-index' builtin parses '--base' for incremental writes,
but the normal write path does not pass that value through to
`write_midx_file()`.

As a result, something like:

    $ git multi-pack-index write --incremental --base=<base>

behaves as if no custom base had been given (unless the caller used the
'--stdin-packs' path).

Thread the parsed base through `write_midx_file()`, and update the
repack caller to pass NULL for the new argument where no custom base
selection is needed.

This exposes a pre-existing problem in incremental writes with custom
bases: the writer skips packs from the full existing MIDX chain, even
when the caller selected an older base or no base at all.

The affected t5334 cases fail while trying to write MIDX bitmaps. The
detached layer omits packs above the selected base, and thus the
resulting MIDX does not have a reachability closure, making it
impossible to generate reachability bitmaps.

Mark those tests as expected failures accordingly. The following commit
will fix the broken behavior and restore these tests.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/multi-pack-index.c              |  3 ++-
 builtin/repack.c                        |  2 +-
 midx-write.c                            |  2 ++
 midx.h                                  |  2 +-
 t/t5334-incremental-multi-pack-index.sh | 24 +++++++++++++++++++-----
 5 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index 00ffb36394d..949bfa796b2 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c
@@ -224,7 +224,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv,
 	}
 
 	ret = write_midx_file(source, opts.preferred_pack,
-			      opts.refs_snapshot, opts.flags);
+			      opts.refs_snapshot, opts.incremental_base,
+			      opts.flags);
 
 	free(opts.refs_snapshot);
 	return ret;
diff --git a/builtin/repack.c b/builtin/repack.c
index 1524a9c13ad..0092a72a996 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -629,7 +629,7 @@ int cmd_repack(int argc,
 		unsigned flags = 0;
 		if (git_env_bool(GIT_TEST_MULTI_PACK_INDEX_WRITE_INCREMENTAL, 0))
 			flags |= MIDX_WRITE_INCREMENTAL;
-		write_midx_file(existing.source, NULL, NULL, flags);
+		write_midx_file(existing.source, NULL, NULL, NULL, flags);
 	}
 
 cleanup:
diff --git a/midx-write.c b/midx-write.c
index 561e9eedc0e..aa438775ebd 100644
--- a/midx-write.c
+++ b/midx-write.c
@@ -1850,12 +1850,14 @@ static int write_midx_internal(struct write_midx_opts *opts)
 int write_midx_file(struct odb_source *source,
 		    const char *preferred_pack_name,
 		    const char *refs_snapshot,
+		    const char *incremental_base,
 		    unsigned flags)
 {
 	struct write_midx_opts opts = {
 		.source = source,
 		.preferred_pack_name = preferred_pack_name,
 		.refs_snapshot = refs_snapshot,
+		.incremental_base = incremental_base,
 		.flags = flags,
 	};
 
diff --git a/midx.h b/midx.h
index 63853a03a47..92ed29d913d 100644
--- a/midx.h
+++ b/midx.h
@@ -131,7 +131,7 @@ int prepare_multi_pack_index_one(struct odb_source *source);
  */
 int write_midx_file(struct odb_source *source,
 		    const char *preferred_pack_name, const char *refs_snapshot,
-		    unsigned flags);
+		    const char *incremental_base, unsigned flags);
 int write_midx_file_only(struct odb_source *source,
 			 struct string_list *packs_to_include,
 			 const char *preferred_pack_name,
diff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh
index 68a103d13d2..69e96bf8d93 100755
--- a/t/t5334-incremental-multi-pack-index.sh
+++ b/t/t5334-incremental-multi-pack-index.sh
@@ -119,7 +119,7 @@ test_expect_success 'write MIDX layer with --base without --no-write-chain-file'
 	test_grep "cannot use --base without --no-write-chain-file" err
 '
 
-test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file' '
+test_expect_failure 'write MIDX layer with --base=none and --no-write-chain-file' '
 	test_commit base-none &&
 	git repack -d &&
 
@@ -128,19 +128,33 @@ test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file
 		--no-write-chain-file --base=none)" &&
 
 	test_cmp "$midx_chain.bak" "$midx_chain" &&
-	test_path_is_file "$midxdir/multi-pack-index-$layer.midx"
+	test_path_is_file "$midxdir/multi-pack-index-$layer.midx" &&
+
+	echo "$layer" >"$midx_chain" &&
+	test-tool read-midx --show-objects "$objdir" "$layer" >midx.objects &&
+	test_grep "^$(git rev-parse 2.2) " midx.objects &&
+	cp "$midx_chain.bak" "$midx_chain"
 '
 
-test_expect_success 'write MIDX layer with --base=<hash> and --no-write-chain-file' '
+test_expect_failure 'write MIDX layer with --base=<hash> and --no-write-chain-file' '
 	test_commit base-hash &&
 	git repack -d &&
 
 	cp "$midx_chain" "$midx_chain.bak" &&
+	base="$(nth_line 1 "$midx_chain")" &&
 	layer="$(git multi-pack-index write --bitmap --incremental \
-		--no-write-chain-file --base="$(nth_line 1 "$midx_chain")")" &&
+		--no-write-chain-file --base="$base")" &&
 
 	test_cmp "$midx_chain.bak" "$midx_chain" &&
-	test_path_is_file "$midxdir/multi-pack-index-$layer.midx"
+	test_path_is_file "$midxdir/multi-pack-index-$layer.midx" &&
+
+	{
+		echo "$base" &&
+		echo "$layer"
+	} >"$midx_chain" &&
+	test-tool read-midx --show-objects "$objdir" "$layer" >midx.objects &&
+	test_grep "^$(git rev-parse 2.2) " midx.objects &&
+	cp "$midx_chain.bak" "$midx_chain"
 '
 
 for reuse in false single multi
-- 
2.55.0.rc0.3.g7bf7c87b605

[PATCH 3/3] midx-write: include packs above custom incremental base

[Taylor Blau]

The previous commit made '--base' take effect on the normal incremental
write path, which exposed an existing assumption in our helper function
`should_include_pack()`, which is that any pack already present in
`ctx->m` was skipped.

That is only correct for non-incremental writes. For incremental writes,
`ctx->base_midx` is the boundary that should be excluded from the new
layer. If the caller selects an older base, or no base at all, then
packs from layers above that base have to be included in the detached
layer so that its bitmap has reachability closure.

Teach `should_include_pack()` to choose the MIDX used for pack exclusion
based on whether or not we are performing an incremental write. When
doing so, use `ctx->base_midx`, and use `ctx->m` otherwise.

The t5334 cases from the previous commit can now be marked as
successful.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 midx-write.c                            | 16 +++++++++++-----
 t/t5334-incremental-multi-pack-index.sh |  4 ++--
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/midx-write.c b/midx-write.c
index aa438775ebd..c50fdb5c6d1 100644
--- a/midx-write.c
+++ b/midx-write.c
@@ -133,8 +133,17 @@ static uint32_t midx_pack_perm(struct write_midx_context *ctx,
 static int should_include_pack(const struct write_midx_context *ctx,
 			       const char *file_name)
 {
+	struct multi_pack_index *m = ctx->m;
 	/*
-	 * Note that at most one of ctx->m and ctx->to_include are set,
+	 * When writing incrementally, ctx->m may contain layers above
+	 * the selected base MIDX, which must be included in the new
+	 * layer.
+	 */
+	if (ctx->incremental)
+		m = ctx->base_midx;
+
+	/*
+	 * Note that at most one of m and ctx->to_include are set,
 	 * so we are testing midx_contains_pack() and
 	 * string_list_has_string() independently (guarded by the
 	 * appropriate NULL checks).
@@ -148,10 +157,7 @@ static int should_include_pack(const struct write_midx_context *ctx,
 	 * should be performed independently (likely checking
 	 * to_include before the existing MIDX).
 	 */
-	if (ctx->m && midx_contains_pack(ctx->m, file_name))
-		return 0;
-	else if (ctx->base_midx && midx_contains_pack(ctx->base_midx,
-						      file_name))
+	if (m && midx_contains_pack(m, file_name))
 		return 0;
 	else if (ctx->to_include &&
 		 !string_list_has_string(ctx->to_include, file_name))
diff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh
index 69e96bf8d93..84ff6120978 100755
--- a/t/t5334-incremental-multi-pack-index.sh
+++ b/t/t5334-incremental-multi-pack-index.sh
@@ -119,7 +119,7 @@ test_expect_success 'write MIDX layer with --base without --no-write-chain-file'
 	test_grep "cannot use --base without --no-write-chain-file" err
 '
 
-test_expect_failure 'write MIDX layer with --base=none and --no-write-chain-file' '
+test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file' '
 	test_commit base-none &&
 	git repack -d &&
 
@@ -136,7 +136,7 @@ test_expect_failure 'write MIDX layer with --base=none and --no-write-chain-file
 	cp "$midx_chain.bak" "$midx_chain"
 '
 
-test_expect_failure 'write MIDX layer with --base=<hash> and --no-write-chain-file' '
+test_expect_success 'write MIDX layer with --base=<hash> and --no-write-chain-file' '
 	test_commit base-hash &&
 	git repack -d &&
 
-- 
2.55.0.rc0.3.g7bf7c87b605

Re: [PATCH 1/3] t5334: expose shared `nth_line()` helper

[Patrick Steinhardt]

On Fri, Jun 12, 2026 at 04:07:08PM -0400, Taylor Blau wrote:
> diff --git a/t/lib-midx.sh b/t/lib-midx.sh
> index e38c609604c..b522dbdb0f4 100644
> --- a/t/lib-midx.sh
> +++ b/t/lib-midx.sh
> @@ -34,3 +34,9 @@ compare_results_with_midx () {
>  		midx_git_two_modes "cat-file --batch-all-objects --batch-check --unordered" sorted
>  	'
>  }
> +
> +nth_line() {
> +	local n="$1"
> +	shift
> +	awk "NR==$n" "$@"
> +}

It feels a bit weird to have such a general function in "lib-midx.sh",
but so be it.

Patrick

Re: [PATCH 2/3] midx: pass custom '--base' through incremental writes

[Patrick Steinhardt]

On Fri, Jun 12, 2026 at 04:07:11PM -0400, Taylor Blau wrote:
> The 'multi-pack-index' builtin parses '--base' for incremental writes,
> but the normal write path does not pass that value through to
> `write_midx_file()`.
> 
> As a result, something like:
> 
>     $ git multi-pack-index write --incremental --base=<base>
> 
> behaves as if no custom base had been given (unless the caller used the
> '--stdin-packs' path).

I'm a bit confused. Is the "normal" write path the one that generates a
completely new, full MIDX? I assume not, and that you use "normal" to
discern between whether or not we pass "--stdin-packs"? I think that
could be made a bit more explicit.

*goes looking into the code* Yeah, seems like the distinction indeed is
whether "--stdin-packs" was passed in the first place.

> Thread the parsed base through `write_midx_file()`, and update the
> repack caller to pass NULL for the new argument where no custom base
> selection is needed.
> 
> This exposes a pre-existing problem in incremental writes with custom
> bases: the writer skips packs from the full existing MIDX chain, even
> when the caller selected an older base or no base at all.

So as the "normal" write path didn't honor this option at all, I assume
this bug here then refers to "--stdin-packs" being broken?

> The affected t5334 cases fail while trying to write MIDX bitmaps. The
> detached layer omits packs above the selected base, and thus the
> resulting MIDX does not have a reachability closure, making it
> impossible to generate reachability bitmaps.
> 
> Mark those tests as expected failures accordingly. The following commit
> will fix the broken behavior and restore these tests.

Okay.

> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
> index 00ffb36394d..949bfa796b2 100644
> --- a/builtin/multi-pack-index.c
> +++ b/builtin/multi-pack-index.c
> @@ -224,7 +224,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv,
>  	}
>  
>  	ret = write_midx_file(source, opts.preferred_pack,
> -			      opts.refs_snapshot, opts.flags);
> +			      opts.refs_snapshot, opts.incremental_base,
> +			      opts.flags);
>  
>  	free(opts.refs_snapshot);
>  	return ret;

Previously we only passed the base to `write_midx_file_only()`, which is
what we use with "--stdin-packs". Here we now update the normal write
path to use the incremental base, too.

> diff --git a/midx-write.c b/midx-write.c
> index 561e9eedc0e..aa438775ebd 100644
> --- a/midx-write.c
> +++ b/midx-write.c
> @@ -1850,12 +1850,14 @@ static int write_midx_internal(struct write_midx_opts *opts)
>  int write_midx_file(struct odb_source *source,
>  		    const char *preferred_pack_name,
>  		    const char *refs_snapshot,
> +		    const char *incremental_base,
>  		    unsigned flags)
>  {
>  	struct write_midx_opts opts = {
>  		.source = source,
>  		.preferred_pack_name = preferred_pack_name,
>  		.refs_snapshot = refs_snapshot,
> +		.incremental_base = incremental_base,
>  		.flags = flags,
>  	};

I was wondering whether there needs to be error checking somewhere so
that we only accept an incremental base in case MIDX_WRITE_INCREMENTAL
is set. I couldn't find any.

> diff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh
> index 68a103d13d2..69e96bf8d93 100755
> --- a/t/t5334-incremental-multi-pack-index.sh
> +++ b/t/t5334-incremental-multi-pack-index.sh
> @@ -119,7 +119,7 @@ test_expect_success 'write MIDX layer with --base without --no-write-chain-file'
>  	test_grep "cannot use --base without --no-write-chain-file" err
>  '
>  
> -test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file' '
> +test_expect_failure 'write MIDX layer with --base=none and --no-write-chain-file' '
>  	test_commit base-none &&
>  	git repack -d &&
>  

Okay. If I understand correctly, the expectation here would be that we
generate a complete MIDX as we don't select any base at all. But we
don't, and instead we base our incremental MIDX on top of the newest
layer by accident.

> @@ -128,19 +128,33 @@ test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file
>  		--no-write-chain-file --base=none)" &&
>  
>  	test_cmp "$midx_chain.bak" "$midx_chain" &&
> -	test_path_is_file "$midxdir/multi-pack-index-$layer.midx"
> +	test_path_is_file "$midxdir/multi-pack-index-$layer.midx" &&
> +
> +	echo "$layer" >"$midx_chain" &&
> +	test-tool read-midx --show-objects "$objdir" "$layer" >midx.objects &&
> +	test_grep "^$(git rev-parse 2.2) " midx.objects &&
> +	cp "$midx_chain.bak" "$midx_chain"
>  '

Would it make sense to also test for an object from the first MIDX layer
to be included? Otherwise we don't really assert that all layers are
included in the new MIDX.

Patrick

Re: [PATCH 3/3] midx-write: include packs above custom incremental base

[Patrick Steinhardt]

On Fri, Jun 12, 2026 at 04:07:14PM -0400, Taylor Blau wrote:
> diff --git a/midx-write.c b/midx-write.c
> index aa438775ebd..c50fdb5c6d1 100644
> --- a/midx-write.c
> +++ b/midx-write.c
> @@ -133,8 +133,17 @@ static uint32_t midx_pack_perm(struct write_midx_context *ctx,
>  static int should_include_pack(const struct write_midx_context *ctx,
>  			       const char *file_name)
>  {
> +	struct multi_pack_index *m = ctx->m;
>  	/*
> -	 * Note that at most one of ctx->m and ctx->to_include are set,
> +	 * When writing incrementally, ctx->m may contain layers above
> +	 * the selected base MIDX, which must be included in the new
> +	 * layer.
> +	 */
> +	if (ctx->incremental)
> +		m = ctx->base_midx;
> +	/*
> +	 * Note that at most one of m and ctx->to_include are set,

Is that true? With "--stdin-packs --incremental --base=<foo>" I'd expect
that we have both set now.

> @@ -148,10 +157,7 @@ static int should_include_pack(const struct write_midx_context *ctx,
>  	 * should be performed independently (likely checking
>  	 * to_include before the existing MIDX).
>  	 */
> -	if (ctx->m && midx_contains_pack(ctx->m, file_name))
> -		return 0;
> -	else if (ctx->base_midx && midx_contains_pack(ctx->base_midx,
> -						      file_name))
> +	if (m && midx_contains_pack(m, file_name))
>  		return 0;

Okay, previously we were always checking against `ctx->m`, so we
would exclude packs that are contained in the current MIDX. And that
includes the case where parts of the current MIDX are supposed to be
thrown away because we want to write a new layer that excludes all
layers starting at the base.

This is fixed by instead always comparing against the base MIDX in case
"--incremental" was passed. When the user passes "--base=none" we don't
have any base, and consequently we'd include all packs. Otherwise, we'll
exclude all packs that are already covered by our base, but include all
the other ones.

That feels sensible to me.

>  	else if (ctx->to_include &&
>  		 !string_list_has_string(ctx->to_include, file_name))
> diff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh
> index 69e96bf8d93..84ff6120978 100755
> --- a/t/t5334-incremental-multi-pack-index.sh
> +++ b/t/t5334-incremental-multi-pack-index.sh
> @@ -119,7 +119,7 @@ test_expect_success 'write MIDX layer with --base without --no-write-chain-file'
>  	test_grep "cannot use --base without --no-write-chain-file" err
>  '
>  
> -test_expect_failure 'write MIDX layer with --base=none and --no-write-chain-file' '
> +test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file' '
>  	test_commit base-none &&
>  	git repack -d &&
>  
> @@ -136,7 +136,7 @@ test_expect_failure 'write MIDX layer with --base=none and --no-write-chain-file
>  	cp "$midx_chain.bak" "$midx_chain"
>  '
>  
> -test_expect_failure 'write MIDX layer with --base=<hash> and --no-write-chain-file' '
> +test_expect_success 'write MIDX layer with --base=<hash> and --no-write-chain-file' '
>  	test_commit base-hash &&
>  	git repack -d &&

And those two tests pass now.

Thanks!

Patrick