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

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH 0/3] midx: honor custom bases for incremental writes
@ 2026-06-12 20:07 Taylor Blau
  2026-06-12 20:07 ` [PATCH 1/3] t5334: expose shared `nth_line()` helper Taylor Blau
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Taylor Blau @ 2026-06-12 20:07 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Elijah Newren, Patrick Steinhardt,
	SZEDER Gábor

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 1/3] t5334: expose shared `nth_line()` helper
  2026-06-12 20:07 [PATCH 0/3] midx: honor custom bases for incremental writes Taylor Blau
@ 2026-06-12 20:07 ` Taylor Blau
  2026-06-12 20:07 ` [PATCH 2/3] midx: pass custom '--base' through incremental writes Taylor Blau
  2026-06-12 20:07 ` [PATCH 3/3] midx-write: include packs above custom incremental base Taylor Blau
  2 siblings, 0 replies; 4+ messages in thread
From: Taylor Blau @ 2026-06-12 20:07 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Elijah Newren, Patrick Steinhardt,
	SZEDER Gábor

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


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH 2/3] midx: pass custom '--base' through incremental writes
  2026-06-12 20:07 [PATCH 0/3] midx: honor custom bases for incremental writes Taylor Blau
  2026-06-12 20:07 ` [PATCH 1/3] t5334: expose shared `nth_line()` helper Taylor Blau
@ 2026-06-12 20:07 ` Taylor Blau
  2026-06-12 20:07 ` [PATCH 3/3] midx-write: include packs above custom incremental base Taylor Blau
  2 siblings, 0 replies; 4+ messages in thread
From: Taylor Blau @ 2026-06-12 20:07 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Elijah Newren, Patrick Steinhardt,
	SZEDER Gábor

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


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH 3/3] midx-write: include packs above custom incremental base
  2026-06-12 20:07 [PATCH 0/3] midx: honor custom bases for incremental writes Taylor Blau
  2026-06-12 20:07 ` [PATCH 1/3] t5334: expose shared `nth_line()` helper Taylor Blau
  2026-06-12 20:07 ` [PATCH 2/3] midx: pass custom '--base' through incremental writes Taylor Blau
@ 2026-06-12 20:07 ` Taylor Blau
  2 siblings, 0 replies; 4+ messages in thread
From: Taylor Blau @ 2026-06-12 20:07 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Elijah Newren, Patrick Steinhardt,
	SZEDER Gábor

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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-06-12 20:07 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-12 20:07 [PATCH 0/3] midx: honor custom bases for incremental writes Taylor Blau
2026-06-12 20:07 ` [PATCH 1/3] t5334: expose shared `nth_line()` helper Taylor Blau
2026-06-12 20:07 ` [PATCH 2/3] midx: pass custom '--base' through incremental writes Taylor Blau
2026-06-12 20:07 ` [PATCH 3/3] midx-write: include packs above custom incremental base Taylor Blau

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.