From acfdb54eb359c9912fb74ec260f00d73e0b2d785 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 12 Jun 2026 16:07:08 -0400 Subject: [PATCH 1/3] t5334: expose shared `nth_line()` helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- 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 e38c609604..b522dbdb0f 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 ec1dafe89f..6a4b799b9c 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 From 8e519b87565a8689d509693a049bc6dad632dbb3 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 12 Jun 2026 16:07:11 -0400 Subject: [PATCH 2/3] midx: pass custom '--base' through incremental writes 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= 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 Signed-off-by: Junio C Hamano --- 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 00ffb36394..949bfa796b 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 1524a9c13a..0092a72a99 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 561e9eedc0..aa438775eb 100644 --- a/midx-write.c +++ b/midx-write.c @@ -1850,12 +1850,14 @@ cleanup: 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 63853a03a4..92ed29d913 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 68a103d13d..69e96bf8d9 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= and --no-write-chain-file' ' +test_expect_failure 'write MIDX layer with --base= 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 From 6afc679fa62362ddaab955778d973c85a6cbf004 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 12 Jun 2026 16:07:14 -0400 Subject: [PATCH 3/3] midx-write: include packs above custom incremental base 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 Signed-off-by: Junio C Hamano --- 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 aa438775eb..c50fdb5c6d 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 69e96bf8d9..84ff612097 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= and --no-write-chain-file' ' +test_expect_success 'write MIDX layer with --base= and --no-write-chain-file' ' test_commit base-hash && git repack -d &&