{"thread":{"id":"65799","subject":"[PATCH 0/3] midx: honor custom bases for incremental writes","startedAt":"2026-06-12T20:07:07Z","lastAt":"2026-08-31T06:12:29Z","messageCount":14,"participants":["Taylor Blau","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"545414","messageId":"cover.1781294771.git.me@ttaylorr.com","threadId":"65799","inReplyTo":null,"subject":"[PATCH 0/3] midx: honor custom bases for incremental writes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-06-12T20:07:05Z","receivedAt":"2026-06-12T20:07:07Z","isPatch":true,"body":"SZEDER noticed[1] that t5334 was trying to call `nth_line()`, despite\nthat helper living only in t5335.\n\nFixing that should have made the test exercise `git multi-pack-index\nwrite --incremental --base=...`. Instead, it uncovered another wrinkle,\nwhich is that the normal MIDX write path parsed \"--bases\" without\nactually passing it down to the MIDX writer.\n\nThis short series fixes both issues. It is structured as follows:\n\n * The first patch moves `nth_line()` to lib-midx.sh so that t5334 and\n   t5335 use the same helper.\n\n * The second patch threads the parsed `--base` value through\n   `write_midx_file()`, and consequently marks two t5334 cases as known\n   breakages.\n\n * The final patch fixes the pack inclusion check and marks the tests\n   successful again.\n\nThe result is that `--base=none` and `--base=<hash>` now correctly\nproduce detached incremental layers that include any packs above the\nselected base, preserving reachability closure for bitmaps.\n\nThanks in advance for your review!\n\n[1]: https://lore.kernel.org/git/aiuaf3fKJ6kIITrf@szeder.dev/\n\nTaylor Blau (3):\n  t5334: expose shared `nth_line()` helper\n  midx: pass custom '--base' through incremental writes\n  midx-write: include packs above custom incremental base\n\n builtin/multi-pack-index.c              |  3 ++-\n builtin/repack.c                        |  2 +-\n midx-write.c                            | 18 +++++++++++++-----\n midx.h                                  |  2 +-\n t/lib-midx.sh                           |  6 ++++++\n t/t5334-incremental-multi-pack-index.sh | 20 +++++++++++++++++---\n t/t5335-compact-multi-pack-index.sh     |  7 +------\n 7 files changed, 41 insertions(+), 17 deletions(-)\n\n\nbase-commit: 3e65291872de10c3f0bf05ea8c24187e7a71ebf0\n-- \n2.55.0.rc0.3.g7bf7c87b605\n"},{"id":"545415","messageId":"a3a51a1ebbf1ba67592a1c884ae7ace526c6aae1.1781294771.git.me@ttaylorr.com","threadId":"65799","inReplyTo":"cover.1781294771.git.me@ttaylorr.com","subject":"[PATCH 1/3] t5334: expose shared `nth_line()` helper","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-06-12T20:07:08Z","receivedAt":"2026-06-12T20:07:10Z","isPatch":true,"body":"Since commit 0cd2255e64b (midx: support custom `--base` for incremental\nMIDX writes, 2026-05-19), t5334 has referred to a non-existent helper\nfunction 'nth_line', which is defined in t5335, but not here.\n\nMove the helper to lib-midx.sh so that both tests can use the same\nimplementation. Ensure likewise that `nth_line()` remains visible from\nwithin t5335 by sourcing lib-midx.sh there appropriately.\n\nCuriously, t5334 passes both before and after this change. Before this\nchange, the failed command substitution leaves '--base' with an empty\nvalue, and after this change, the custom base value is still ignored by\nthe normal incremental write path. The following commits will explain\nand address that behavior.\n\nNoticed-by: SZEDER Gábor <szeder.dev@gmail.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/lib-midx.sh                       | 6 ++++++\n t/t5335-compact-multi-pack-index.sh | 7 +------\n 2 files changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/t/lib-midx.sh b/t/lib-midx.sh\nindex e38c609604c..b522dbdb0f4 100644\n--- a/t/lib-midx.sh\n+++ b/t/lib-midx.sh\n@@ -34,3 +34,9 @@ compare_results_with_midx () {\n \t\tmidx_git_two_modes \"cat-file --batch-all-objects --batch-check --unordered\" sorted\n \t'\n }\n+\n+nth_line() {\n+\tlocal n=\"$1\"\n+\tshift\n+\tawk \"NR==$n\" \"$@\"\n+}\ndiff --git a/t/t5335-compact-multi-pack-index.sh b/t/t5335-compact-multi-pack-index.sh\nindex ec1dafe89fc..6a4b799b9c9 100755\n--- a/t/t5335-compact-multi-pack-index.sh\n+++ b/t/t5335-compact-multi-pack-index.sh\n@@ -3,6 +3,7 @@\n test_description='multi-pack-index compaction'\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-midx.sh\n \n GIT_TEST_MULTI_PACK_INDEX=0\n GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0\n@@ -13,12 +14,6 @@ packdir=$objdir/pack\n midxdir=$packdir/multi-pack-index.d\n midx_chain=$midxdir/multi-pack-index-chain\n \n-nth_line() {\n-\tlocal n=\"$1\"\n-\tshift\n-\tawk \"NR==$n\" \"$@\"\n-}\n-\n write_packs () {\n \tfor c in \"$@\"\n \tdo\n-- \n2.55.0.rc0.3.g7bf7c87b605\n\n"},{"id":"545416","messageId":"4115ee0a9a09351e47d557a1283fc6ec4d633304.1781294771.git.me@ttaylorr.com","threadId":"65799","inReplyTo":"cover.1781294771.git.me@ttaylorr.com","subject":"[PATCH 2/3] midx: pass custom '--base' through incremental writes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-06-12T20:07:11Z","receivedAt":"2026-06-12T20:07:13Z","isPatch":true,"body":"The 'multi-pack-index' builtin parses '--base' for incremental writes,\nbut the normal write path does not pass that value through to\n`write_midx_file()`.\n\nAs a result, something like:\n\n    $ git multi-pack-index write --incremental --base=<base>\n\nbehaves as if no custom base had been given (unless the caller used the\n'--stdin-packs' path).\n\nThread the parsed base through `write_midx_file()`, and update the\nrepack caller to pass NULL for the new argument where no custom base\nselection is needed.\n\nThis exposes a pre-existing problem in incremental writes with custom\nbases: the writer skips packs from the full existing MIDX chain, even\nwhen the caller selected an older base or no base at all.\n\nThe affected t5334 cases fail while trying to write MIDX bitmaps. The\ndetached layer omits packs above the selected base, and thus the\nresulting MIDX does not have a reachability closure, making it\nimpossible to generate reachability bitmaps.\n\nMark those tests as expected failures accordingly. The following commit\nwill fix the broken behavior and restore these tests.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/multi-pack-index.c              |  3 ++-\n builtin/repack.c                        |  2 +-\n midx-write.c                            |  2 ++\n midx.h                                  |  2 +-\n t/t5334-incremental-multi-pack-index.sh | 24 +++++++++++++++++++-----\n 5 files changed, 25 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\nindex 00ffb36394d..949bfa796b2 100644\n--- a/builtin/multi-pack-index.c\n+++ b/builtin/multi-pack-index.c\n@@ -224,7 +224,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv,\n \t}\n \n \tret = write_midx_file(source, opts.preferred_pack,\n-\t\t\t      opts.refs_snapshot, opts.flags);\n+\t\t\t      opts.refs_snapshot, opts.incremental_base,\n+\t\t\t      opts.flags);\n \n \tfree(opts.refs_snapshot);\n \treturn ret;\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 1524a9c13ad..0092a72a996 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -629,7 +629,7 @@ int cmd_repack(int argc,\n \t\tunsigned flags = 0;\n \t\tif (git_env_bool(GIT_TEST_MULTI_PACK_INDEX_WRITE_INCREMENTAL, 0))\n \t\t\tflags |= MIDX_WRITE_INCREMENTAL;\n-\t\twrite_midx_file(existing.source, NULL, NULL, flags);\n+\t\twrite_midx_file(existing.source, NULL, NULL, NULL, flags);\n \t}\n \n cleanup:\ndiff --git a/midx-write.c b/midx-write.c\nindex 561e9eedc0e..aa438775ebd 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1850,12 +1850,14 @@ static int write_midx_internal(struct write_midx_opts *opts)\n int write_midx_file(struct odb_source *source,\n \t\t    const char *preferred_pack_name,\n \t\t    const char *refs_snapshot,\n+\t\t    const char *incremental_base,\n \t\t    unsigned flags)\n {\n \tstruct write_midx_opts opts = {\n \t\t.source = source,\n \t\t.preferred_pack_name = preferred_pack_name,\n \t\t.refs_snapshot = refs_snapshot,\n+\t\t.incremental_base = incremental_base,\n \t\t.flags = flags,\n \t};\n \ndiff --git a/midx.h b/midx.h\nindex 63853a03a47..92ed29d913d 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -131,7 +131,7 @@ int prepare_multi_pack_index_one(struct odb_source *source);\n  */\n int write_midx_file(struct odb_source *source,\n \t\t    const char *preferred_pack_name, const char *refs_snapshot,\n-\t\t    unsigned flags);\n+\t\t    const char *incremental_base, unsigned flags);\n int write_midx_file_only(struct odb_source *source,\n \t\t\t struct string_list *packs_to_include,\n \t\t\t const char *preferred_pack_name,\ndiff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh\nindex 68a103d13d2..69e96bf8d93 100755\n--- a/t/t5334-incremental-multi-pack-index.sh\n+++ b/t/t5334-incremental-multi-pack-index.sh\n@@ -119,7 +119,7 @@ test_expect_success 'write MIDX layer with --base without --no-write-chain-file'\n \ttest_grep \"cannot use --base without --no-write-chain-file\" err\n '\n \n-test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file' '\n+test_expect_failure 'write MIDX layer with --base=none and --no-write-chain-file' '\n \ttest_commit base-none &&\n \tgit repack -d &&\n \n@@ -128,19 +128,33 @@ test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file\n \t\t--no-write-chain-file --base=none)\" &&\n \n \ttest_cmp \"$midx_chain.bak\" \"$midx_chain\" &&\n-\ttest_path_is_file \"$midxdir/multi-pack-index-$layer.midx\"\n+\ttest_path_is_file \"$midxdir/multi-pack-index-$layer.midx\" &&\n+\n+\techo \"$layer\" >\"$midx_chain\" &&\n+\ttest-tool read-midx --show-objects \"$objdir\" \"$layer\" >midx.objects &&\n+\ttest_grep \"^$(git rev-parse 2.2) \" midx.objects &&\n+\tcp \"$midx_chain.bak\" \"$midx_chain\"\n '\n \n-test_expect_success 'write MIDX layer with --base=<hash> and --no-write-chain-file' '\n+test_expect_failure 'write MIDX layer with --base=<hash> and --no-write-chain-file' '\n \ttest_commit base-hash &&\n \tgit repack -d &&\n \n \tcp \"$midx_chain\" \"$midx_chain.bak\" &&\n+\tbase=\"$(nth_line 1 \"$midx_chain\")\" &&\n \tlayer=\"$(git multi-pack-index write --bitmap --incremental \\\n-\t\t--no-write-chain-file --base=\"$(nth_line 1 \"$midx_chain\")\")\" &&\n+\t\t--no-write-chain-file --base=\"$base\")\" &&\n \n \ttest_cmp \"$midx_chain.bak\" \"$midx_chain\" &&\n-\ttest_path_is_file \"$midxdir/multi-pack-index-$layer.midx\"\n+\ttest_path_is_file \"$midxdir/multi-pack-index-$layer.midx\" &&\n+\n+\t{\n+\t\techo \"$base\" &&\n+\t\techo \"$layer\"\n+\t} >\"$midx_chain\" &&\n+\ttest-tool read-midx --show-objects \"$objdir\" \"$layer\" >midx.objects &&\n+\ttest_grep \"^$(git rev-parse 2.2) \" midx.objects &&\n+\tcp \"$midx_chain.bak\" \"$midx_chain\"\n '\n \n for reuse in false single multi\n-- \n2.55.0.rc0.3.g7bf7c87b605\n\n"},{"id":"545417","messageId":"7bf7c87b60532a90c04c4a2404449a9d8ea21214.1781294771.git.me@ttaylorr.com","threadId":"65799","inReplyTo":"cover.1781294771.git.me@ttaylorr.com","subject":"[PATCH 3/3] midx-write: include packs above custom incremental base","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-06-12T20:07:14Z","receivedAt":"2026-06-12T20:07:16Z","isPatch":true,"body":"The previous commit made '--base' take effect on the normal incremental\nwrite path, which exposed an existing assumption in our helper function\n`should_include_pack()`, which is that any pack already present in\n`ctx->m` was skipped.\n\nThat is only correct for non-incremental writes. For incremental writes,\n`ctx->base_midx` is the boundary that should be excluded from the new\nlayer. If the caller selects an older base, or no base at all, then\npacks from layers above that base have to be included in the detached\nlayer so that its bitmap has reachability closure.\n\nTeach `should_include_pack()` to choose the MIDX used for pack exclusion\nbased on whether or not we are performing an incremental write. When\ndoing so, use `ctx->base_midx`, and use `ctx->m` otherwise.\n\nThe t5334 cases from the previous commit can now be marked as\nsuccessful.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx-write.c                            | 16 +++++++++++-----\n t/t5334-incremental-multi-pack-index.sh |  4 ++--\n 2 files changed, 13 insertions(+), 7 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex aa438775ebd..c50fdb5c6d1 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -133,8 +133,17 @@ static uint32_t midx_pack_perm(struct write_midx_context *ctx,\n static int should_include_pack(const struct write_midx_context *ctx,\n \t\t\t       const char *file_name)\n {\n+\tstruct multi_pack_index *m = ctx->m;\n \t/*\n-\t * Note that at most one of ctx->m and ctx->to_include are set,\n+\t * When writing incrementally, ctx->m may contain layers above\n+\t * the selected base MIDX, which must be included in the new\n+\t * layer.\n+\t */\n+\tif (ctx->incremental)\n+\t\tm = ctx->base_midx;\n+\n+\t/*\n+\t * Note that at most one of m and ctx->to_include are set,\n \t * so we are testing midx_contains_pack() and\n \t * string_list_has_string() independently (guarded by the\n \t * appropriate NULL checks).\n@@ -148,10 +157,7 @@ static int should_include_pack(const struct write_midx_context *ctx,\n \t * should be performed independently (likely checking\n \t * to_include before the existing MIDX).\n \t */\n-\tif (ctx->m && midx_contains_pack(ctx->m, file_name))\n-\t\treturn 0;\n-\telse if (ctx->base_midx && midx_contains_pack(ctx->base_midx,\n-\t\t\t\t\t\t      file_name))\n+\tif (m && midx_contains_pack(m, file_name))\n \t\treturn 0;\n \telse if (ctx->to_include &&\n \t\t !string_list_has_string(ctx->to_include, file_name))\ndiff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh\nindex 69e96bf8d93..84ff6120978 100755\n--- a/t/t5334-incremental-multi-pack-index.sh\n+++ b/t/t5334-incremental-multi-pack-index.sh\n@@ -119,7 +119,7 @@ test_expect_success 'write MIDX layer with --base without --no-write-chain-file'\n \ttest_grep \"cannot use --base without --no-write-chain-file\" err\n '\n \n-test_expect_failure 'write MIDX layer with --base=none and --no-write-chain-file' '\n+test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file' '\n \ttest_commit base-none &&\n \tgit repack -d &&\n \n@@ -136,7 +136,7 @@ test_expect_failure 'write MIDX layer with --base=none and --no-write-chain-file\n \tcp \"$midx_chain.bak\" \"$midx_chain\"\n '\n \n-test_expect_failure 'write MIDX layer with --base=<hash> and --no-write-chain-file' '\n+test_expect_success 'write MIDX layer with --base=<hash> and --no-write-chain-file' '\n \ttest_commit base-hash &&\n \tgit repack -d &&\n \n-- \n2.55.0.rc0.3.g7bf7c87b605\n"},{"id":"550482","messageId":"an2E6OV1Fr7wKFhn@pks.im","threadId":"65799","inReplyTo":"a3a51a1ebbf1ba67592a1c884ae7ace526c6aae1.1781294771.git.me@ttaylorr.com","subject":"Re: [PATCH 1/3] t5334: expose shared `nth_line()` helper","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-13T08:48:49Z","receivedAt":"2026-08-13T08:49:02Z","isPatch":true,"body":"On Fri, Jun 12, 2026 at 04:07:08PM -0400, Taylor Blau wrote:\n> diff --git a/t/lib-midx.sh b/t/lib-midx.sh\n> index e38c609604c..b522dbdb0f4 100644\n> --- a/t/lib-midx.sh\n> +++ b/t/lib-midx.sh\n> @@ -34,3 +34,9 @@ compare_results_with_midx () {\n>  \t\tmidx_git_two_modes \"cat-file --batch-all-objects --batch-check --unordered\" sorted\n>  \t'\n>  }\n> +\n> +nth_line() {\n> +\tlocal n=\"$1\"\n> +\tshift\n> +\tawk \"NR==$n\" \"$@\"\n> +}\n\nIt feels a bit weird to have such a general function in \"lib-midx.sh\",\nbut so be it.\n\nPatrick\n"},{"id":"550483","messageId":"an2E_F_1DC4cPKG3@pks.im","threadId":"65799","inReplyTo":"4115ee0a9a09351e47d557a1283fc6ec4d633304.1781294771.git.me@ttaylorr.com","subject":"Re: [PATCH 2/3] midx: pass custom '--base' through incremental writes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-13T08:49:00Z","receivedAt":"2026-08-13T08:49:06Z","isPatch":true,"body":"On Fri, Jun 12, 2026 at 04:07:11PM -0400, Taylor Blau wrote:\n> The 'multi-pack-index' builtin parses '--base' for incremental writes,\n> but the normal write path does not pass that value through to\n> `write_midx_file()`.\n> \n> As a result, something like:\n> \n>     $ git multi-pack-index write --incremental --base=<base>\n> \n> behaves as if no custom base had been given (unless the caller used the\n> '--stdin-packs' path).\n\nI'm a bit confused. Is the \"normal\" write path the one that generates a\ncompletely new, full MIDX? I assume not, and that you use \"normal\" to\ndiscern between whether or not we pass \"--stdin-packs\"? I think that\ncould be made a bit more explicit.\n\n*goes looking into the code* Yeah, seems like the distinction indeed is\nwhether \"--stdin-packs\" was passed in the first place.\n\n> Thread the parsed base through `write_midx_file()`, and update the\n> repack caller to pass NULL for the new argument where no custom base\n> selection is needed.\n> \n> This exposes a pre-existing problem in incremental writes with custom\n> bases: the writer skips packs from the full existing MIDX chain, even\n> when the caller selected an older base or no base at all.\n\nSo as the \"normal\" write path didn't honor this option at all, I assume\nthis bug here then refers to \"--stdin-packs\" being broken?\n\n> The affected t5334 cases fail while trying to write MIDX bitmaps. The\n> detached layer omits packs above the selected base, and thus the\n> resulting MIDX does not have a reachability closure, making it\n> impossible to generate reachability bitmaps.\n> \n> Mark those tests as expected failures accordingly. The following commit\n> will fix the broken behavior and restore these tests.\n\nOkay.\n\n> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\n> index 00ffb36394d..949bfa796b2 100644\n> --- a/builtin/multi-pack-index.c\n> +++ b/builtin/multi-pack-index.c\n> @@ -224,7 +224,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv,\n>  \t}\n>  \n>  \tret = write_midx_file(source, opts.preferred_pack,\n> -\t\t\t      opts.refs_snapshot, opts.flags);\n> +\t\t\t      opts.refs_snapshot, opts.incremental_base,\n> +\t\t\t      opts.flags);\n>  \n>  \tfree(opts.refs_snapshot);\n>  \treturn ret;\n\nPreviously we only passed the base to `write_midx_file_only()`, which is\nwhat we use with \"--stdin-packs\". Here we now update the normal write\npath to use the incremental base, too.\n\n> diff --git a/midx-write.c b/midx-write.c\n> index 561e9eedc0e..aa438775ebd 100644\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@ -1850,12 +1850,14 @@ static int write_midx_internal(struct write_midx_opts *opts)\n>  int write_midx_file(struct odb_source *source,\n>  \t\t    const char *preferred_pack_name,\n>  \t\t    const char *refs_snapshot,\n> +\t\t    const char *incremental_base,\n>  \t\t    unsigned flags)\n>  {\n>  \tstruct write_midx_opts opts = {\n>  \t\t.source = source,\n>  \t\t.preferred_pack_name = preferred_pack_name,\n>  \t\t.refs_snapshot = refs_snapshot,\n> +\t\t.incremental_base = incremental_base,\n>  \t\t.flags = flags,\n>  \t};\n\nI was wondering whether there needs to be error checking somewhere so\nthat we only accept an incremental base in case MIDX_WRITE_INCREMENTAL\nis set. I couldn't find any.\n\n> diff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh\n> index 68a103d13d2..69e96bf8d93 100755\n> --- a/t/t5334-incremental-multi-pack-index.sh\n> +++ b/t/t5334-incremental-multi-pack-index.sh\n> @@ -119,7 +119,7 @@ test_expect_success 'write MIDX layer with --base without --no-write-chain-file'\n>  \ttest_grep \"cannot use --base without --no-write-chain-file\" err\n>  '\n>  \n> -test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file' '\n> +test_expect_failure 'write MIDX layer with --base=none and --no-write-chain-file' '\n>  \ttest_commit base-none &&\n>  \tgit repack -d &&\n>  \n\nOkay. If I understand correctly, the expectation here would be that we\ngenerate a complete MIDX as we don't select any base at all. But we\ndon't, and instead we base our incremental MIDX on top of the newest\nlayer by accident.\n\n> @@ -128,19 +128,33 @@ test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file\n>  \t\t--no-write-chain-file --base=none)\" &&\n>  \n>  \ttest_cmp \"$midx_chain.bak\" \"$midx_chain\" &&\n> -\ttest_path_is_file \"$midxdir/multi-pack-index-$layer.midx\"\n> +\ttest_path_is_file \"$midxdir/multi-pack-index-$layer.midx\" &&\n> +\n> +\techo \"$layer\" >\"$midx_chain\" &&\n> +\ttest-tool read-midx --show-objects \"$objdir\" \"$layer\" >midx.objects &&\n> +\ttest_grep \"^$(git rev-parse 2.2) \" midx.objects &&\n> +\tcp \"$midx_chain.bak\" \"$midx_chain\"\n>  '\n\nWould it make sense to also test for an object from the first MIDX layer\nto be included? Otherwise we don't really assert that all layers are\nincluded in the new MIDX.\n\nPatrick\n"},{"id":"550484","messageId":"an2FAWvyfX2LuGsG@pks.im","threadId":"65799","inReplyTo":"7bf7c87b60532a90c04c4a2404449a9d8ea21214.1781294771.git.me@ttaylorr.com","subject":"Re: [PATCH 3/3] midx-write: include packs above custom incremental base","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-13T08:49:05Z","receivedAt":"2026-08-13T08:49:11Z","isPatch":true,"body":"On Fri, Jun 12, 2026 at 04:07:14PM -0400, Taylor Blau wrote:\n> diff --git a/midx-write.c b/midx-write.c\n> index aa438775ebd..c50fdb5c6d1 100644\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@ -133,8 +133,17 @@ static uint32_t midx_pack_perm(struct write_midx_context *ctx,\n>  static int should_include_pack(const struct write_midx_context *ctx,\n>  \t\t\t       const char *file_name)\n>  {\n> +\tstruct multi_pack_index *m = ctx->m;\n>  \t/*\n> -\t * Note that at most one of ctx->m and ctx->to_include are set,\n> +\t * When writing incrementally, ctx->m may contain layers above\n> +\t * the selected base MIDX, which must be included in the new\n> +\t * layer.\n> +\t */\n> +\tif (ctx->incremental)\n> +\t\tm = ctx->base_midx;\n> +\t/*\n> +\t * Note that at most one of m and ctx->to_include are set,\n\nIs that true? With \"--stdin-packs --incremental --base=<foo>\" I'd expect\nthat we have both set now.\n\n> @@ -148,10 +157,7 @@ static int should_include_pack(const struct write_midx_context *ctx,\n>  \t * should be performed independently (likely checking\n>  \t * to_include before the existing MIDX).\n>  \t */\n> -\tif (ctx->m && midx_contains_pack(ctx->m, file_name))\n> -\t\treturn 0;\n> -\telse if (ctx->base_midx && midx_contains_pack(ctx->base_midx,\n> -\t\t\t\t\t\t      file_name))\n> +\tif (m && midx_contains_pack(m, file_name))\n>  \t\treturn 0;\n\nOkay, previously we were always checking against `ctx->m`, so we\nwould exclude packs that are contained in the current MIDX. And that\nincludes the case where parts of the current MIDX are supposed to be\nthrown away because we want to write a new layer that excludes all\nlayers starting at the base.\n\nThis is fixed by instead always comparing against the base MIDX in case\n\"--incremental\" was passed. When the user passes \"--base=none\" we don't\nhave any base, and consequently we'd include all packs. Otherwise, we'll\nexclude all packs that are already covered by our base, but include all\nthe other ones.\n\nThat feels sensible to me.\n\n>  \telse if (ctx->to_include &&\n>  \t\t !string_list_has_string(ctx->to_include, file_name))\n> diff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh\n> index 69e96bf8d93..84ff6120978 100755\n> --- a/t/t5334-incremental-multi-pack-index.sh\n> +++ b/t/t5334-incremental-multi-pack-index.sh\n> @@ -119,7 +119,7 @@ test_expect_success 'write MIDX layer with --base without --no-write-chain-file'\n>  \ttest_grep \"cannot use --base without --no-write-chain-file\" err\n>  '\n>  \n> -test_expect_failure 'write MIDX layer with --base=none and --no-write-chain-file' '\n> +test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file' '\n>  \ttest_commit base-none &&\n>  \tgit repack -d &&\n>  \n> @@ -136,7 +136,7 @@ test_expect_failure 'write MIDX layer with --base=none and --no-write-chain-file\n>  \tcp \"$midx_chain.bak\" \"$midx_chain\"\n>  '\n>  \n> -test_expect_failure 'write MIDX layer with --base=<hash> and --no-write-chain-file' '\n> +test_expect_success 'write MIDX layer with --base=<hash> and --no-write-chain-file' '\n>  \ttest_commit base-hash &&\n>  \tgit repack -d &&\n\nAnd those two tests pass now.\n\nThanks!\n\nPatrick\n"},{"id":"550565","messageId":"an4a-t0auyMFzbgS@com-79390","threadId":"65799","inReplyTo":"an2E6OV1Fr7wKFhn@pks.im","subject":"Re: [PATCH 1/3] t5334: expose shared `nth_line()` helper","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-08-13T19:28:58Z","receivedAt":"2026-08-13T19:29:11Z","isPatch":true,"body":"On Thu, Aug 13, 2026 at 10:48:49AM +0200, Patrick Steinhardt wrote:\n> On Fri, Jun 12, 2026 at 04:07:08PM -0400, Taylor Blau wrote:\n> > diff --git a/t/lib-midx.sh b/t/lib-midx.sh\n> > index e38c609604c..b522dbdb0f4 100644\n> > --- a/t/lib-midx.sh\n> > +++ b/t/lib-midx.sh\n> > @@ -34,3 +34,9 @@ compare_results_with_midx () {\n> >  \t\tmidx_git_two_modes \"cat-file --batch-all-objects --batch-check --unordered\" sorted\n> >  \t'\n> >  }\n> > +\n> > +nth_line() {\n> > +\tlocal n=\"$1\"\n> > +\tshift\n> > +\tawk \"NR==$n\" \"$@\"\n> > +}\n>\n> It feels a bit weird to have such a general function in \"lib-midx.sh\",\n> but so be it.\n\nYeah, I agree. It mostly just felt weird to write '| awk \"NR==$n\"' a\nbunch of times.\n\nThanks,\nTaylor\n"},{"id":"550575","messageId":"an4pffUrCY4xhTH2@com-79390","threadId":"65799","inReplyTo":"an2E_F_1DC4cPKG3@pks.im","subject":"Re: [PATCH 2/3] midx: pass custom '--base' through incremental writes","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-08-13T20:30:53Z","receivedAt":"2026-08-13T20:31:00Z","isPatch":true,"body":"On Thu, Aug 13, 2026 at 10:49:00AM +0200, Patrick Steinhardt wrote:\n> > Thread the parsed base through `write_midx_file()`, and update the\n> > repack caller to pass NULL for the new argument where no custom base\n> > selection is needed.\n> >\n> > This exposes a pre-existing problem in incremental writes with custom\n> > bases: the writer skips packs from the full existing MIDX chain, even\n> > when the caller selected an older base or no base at all.\n>\n> So as the \"normal\" write path didn't honor this option at all, I assume\n> this bug here then refers to \"--stdin-packs\" being broken?\n\nYeah, that's right.\n\n> > @@ -128,19 +128,33 @@ test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file\n> >  \t\t--no-write-chain-file --base=none)\" &&\n> >\n> >  \ttest_cmp \"$midx_chain.bak\" \"$midx_chain\" &&\n> > -\ttest_path_is_file \"$midxdir/multi-pack-index-$layer.midx\"\n> > +\ttest_path_is_file \"$midxdir/multi-pack-index-$layer.midx\" &&\n> > +\n> > +\techo \"$layer\" >\"$midx_chain\" &&\n> > +\ttest-tool read-midx --show-objects \"$objdir\" \"$layer\" >midx.objects &&\n> > +\ttest_grep \"^$(git rev-parse 2.2) \" midx.objects &&\n> > +\tcp \"$midx_chain.bak\" \"$midx_chain\"\n> >  '\n>\n> Would it make sense to also test for an object from the first MIDX layer\n> to be included? Otherwise we don't really assert that all layers are\n> included in the new MIDX.\n\nI don't think that is necessary in this case, but let me know if I am\nmissing something below.\n\nThe new layer is written with '--bitmap', and '--base=none' means that\nthere is no base layer from which the bitmap can inherit objects. Since\n1.2 is an ancestor of 2.2, writing a bitmap for the new layer already\nrequires that it contain 1.2 and the rest of its reachable history.\nOtherwise bitmap generation would fail with the missing-closure error\nbefore we reached the assertion.\n\nChecking 2.2 confirms that an object from the old tip was pulled into\nthe new layer; the successful bitmap write already establishes that its\nobjects from the earlier layer were pulled in, too.\n\nThanks,\nTaylor\n"},{"id":"550579","messageId":"an4uIQA09rDCwwBp@com-79390","threadId":"65799","inReplyTo":"an2FAWvyfX2LuGsG@pks.im","subject":"Re: [PATCH 3/3] midx-write: include packs above custom incremental base","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-08-13T20:50:41Z","receivedAt":"2026-08-13T20:50:45Z","isPatch":true,"body":"On Thu, Aug 13, 2026 at 10:49:05AM +0200, Patrick Steinhardt wrote:\n> On Fri, Jun 12, 2026 at 04:07:14PM -0400, Taylor Blau wrote:\n> > diff --git a/midx-write.c b/midx-write.c\n> > index aa438775ebd..c50fdb5c6d1 100644\n> > --- a/midx-write.c\n> > +++ b/midx-write.c\n> > @@ -133,8 +133,17 @@ static uint32_t midx_pack_perm(struct write_midx_context *ctx,\n> >  static int should_include_pack(const struct write_midx_context *ctx,\n> >  \t\t\t       const char *file_name)\n> >  {\n> > +\tstruct multi_pack_index *m = ctx->m;\n> >  \t/*\n> > -\t * Note that at most one of ctx->m and ctx->to_include are set,\n> > +\t * When writing incrementally, ctx->m may contain layers above\n> > +\t * the selected base MIDX, which must be included in the new\n> > +\t * layer.\n> > +\t */\n> > +\tif (ctx->incremental)\n> > +\t\tm = ctx->base_midx;\n> > +\t/*\n> > +\t * Note that at most one of m and ctx->to_include are set,\n>\n> Is that true? With \"--stdin-packs --incremental --base=<foo>\" I'd expect\n> that we have both set now.\n\nThat invariant holds for `ctx->m`j, but not for the local m after the\nassignment above. With '--stdin-packs', `write_midx_internal()` leaves\n`ctx->m` unset, but can still set `ctx->base_midx` for an incremental\nwrite.  Once we assign `m = ctx->base_midx`, both `m` and\n`ctx->to_include` can indeed be non-NULL.\n\nThe filtering still does the right thing: packs covered by the selected\nbase are excluded, and the remaining packs are checked against the stdin\nlist. But the comment is wrong, so I'll fix it.\n\n> Okay, previously we were always checking against `ctx->m`, so we\n> would exclude packs that are contained in the current MIDX. And that\n> includes the case where parts of the current MIDX are supposed to be\n> thrown away because we want to write a new layer that excludes all\n> layers starting at the base.\n\nOn the non- '--stdin-packs' path, yes. With '--stdin-packs', `ctx->m` is\n`NULL` and the old code already checks `ctx->base_midx`. The problem\nappears when the previous patch starts honoring '--base' on the ordinary\nwrite path. Since `ctx->m` still refers to the entire existing chain, it\nexcludes packs from layers above the selected base.\n\n> This is fixed by instead always comparing against the base MIDX in case\n> \"--incremental\" was passed. When the user passes \"--base=none\" we don't\n> have any base, and consequently we'd include all packs. Otherwise, we'll\n> exclude all packs that are already covered by our base, but include all\n> the other ones.\n>\n> That feels sensible to me.\n\nExactly.\n\nThanks,\nTaylor\n"},{"id":"550594","messageId":"an7DcGwWwjbq-C5a@pks.im","threadId":"65799","inReplyTo":"an4pffUrCY4xhTH2@com-79390","subject":"Re: [PATCH 2/3] midx: pass custom '--base' through incremental writes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-14T07:27:44Z","receivedAt":"2026-08-14T07:41:42Z","isPatch":true,"body":"On Thu, Aug 13, 2026 at 03:30:53PM -0500, Taylor Blau wrote:\n> On Thu, Aug 13, 2026 at 10:49:00AM +0200, Patrick Steinhardt wrote:\n> > > @@ -128,19 +128,33 @@ test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file\n> > >  \t\t--no-write-chain-file --base=none)\" &&\n> > >\n> > >  \ttest_cmp \"$midx_chain.bak\" \"$midx_chain\" &&\n> > > -\ttest_path_is_file \"$midxdir/multi-pack-index-$layer.midx\"\n> > > +\ttest_path_is_file \"$midxdir/multi-pack-index-$layer.midx\" &&\n> > > +\n> > > +\techo \"$layer\" >\"$midx_chain\" &&\n> > > +\ttest-tool read-midx --show-objects \"$objdir\" \"$layer\" >midx.objects &&\n> > > +\ttest_grep \"^$(git rev-parse 2.2) \" midx.objects &&\n> > > +\tcp \"$midx_chain.bak\" \"$midx_chain\"\n> > >  '\n> >\n> > Would it make sense to also test for an object from the first MIDX layer\n> > to be included? Otherwise we don't really assert that all layers are\n> > included in the new MIDX.\n> \n> I don't think that is necessary in this case, but let me know if I am\n> missing something below.\n> \n> The new layer is written with '--bitmap', and '--base=none' means that\n> there is no base layer from which the bitmap can inherit objects. Since\n> 1.2 is an ancestor of 2.2, writing a bitmap for the new layer already\n> requires that it contain 1.2 and the rest of its reachable history.\n> Otherwise bitmap generation would fail with the missing-closure error\n> before we reached the assertion.\n> \n> Checking 2.2 confirms that an object from the old tip was pulled into\n> the new layer; the successful bitmap write already establishes that its\n> objects from the earlier layer were pulled in, too.\n\nI think that's a bit roundabout, as it simply tells us that the bitmap\nwas generated correctly, but not that the MIDX contains the objects. It\nof course should if the bitmap was generated properly, but I would have\npreferred if we verified the property directly.\n\nAnyway, this is not a huge concern, more of a nitpick. Thanks!\n\nPatrick\n"},{"id":"551318","messageId":"xmqqtsogd0mk.fsf@gitster.g","threadId":"65799","inReplyTo":"an4uIQA09rDCwwBp@com-79390","subject":"Re: [PATCH 3/3] midx-write: include packs above custom incremental base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-26T21:37:23Z","receivedAt":"2026-08-26T21:37:26Z","isPatch":true,"body":"Taylor Blau <ttaylorr@openai.com> writes:\n\n> `ctx->to_include` can indeed be non-NULL.\n> ...\n> The filtering still does the right thing: packs covered by the selected\n> base are excluded, and the remaining packs are checked against the stdin\n> list. But the comment is wrong, so I'll fix it.\n\nHas anything happened since we saw this comment on Aug 13th?\n\nThanks.\n"},{"id":"551326","messageId":"ao945zRwDt9ThFTG@com-79390","threadId":"65799","inReplyTo":"xmqqtsogd0mk.fsf@gitster.g","subject":"Re: [PATCH 3/3] midx-write: include packs above custom incremental base","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-08-26T23:38:15Z","receivedAt":"2026-08-26T23:38:27Z","isPatch":true,"body":"On Wed, Aug 26, 2026 at 02:37:23PM -0700, Junio C Hamano wrote:\n> Taylor Blau <ttaylorr@openai.com> writes:\n>\n> > `ctx->to_include` can indeed be non-NULL.\n> > ...\n> > The filtering still does the right thing: packs covered by the selected\n> > base are excluded, and the remaining packs are checked against the stdin\n> > list. But the comment is wrong, so I'll fix it.\n>\n> Has anything happened since we saw this comment on Aug 13th?\n\nNot until you sent this message ;-).\n\nI had a small reroll prepped that I had meant to send a couple of weeks\nago but never got around to doing so. When I looked at it just now, I\nfound that I wasn't quite satisfied with the range-diff in that the\nresulting block comment was somewhat confusing.\n\nInstead of sending a new round immediately, let me instead share the\ncomment that I wrote instead. Patrick (or others): does this comment\nseem clear, or do you think there are ways to tighten it up further?\n\n--- 8< ---\ndiff --git a/midx-write.c b/midx-write.c\nindex 66da608370..ff94076104 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -143,15 +143,31 @@ static int should_include_pack(const struct write_midx_context *ctx,\n \t\tm = ctx->base_midx;\n\n \t/*\n-\t * Note that m and ctx->to_include may both be set,\n-\t * so we are testing midx_contains_pack() and\n-\t * string_list_has_string() independently (guarded by the\n-\t * appropriate NULL checks).\n-\t *\n-\t * We could support passing to_include while reusing an existing\n-\t * MIDX, but don't currently since the reuse process drags\n-\t * forward all packs from an existing MIDX (without checking\n-\t * whether or not they appear in the to_include list).\n+\t * Note that it is OK for both ctx->base_midx and\n+\t * ctx->to_include may both be non-NULL, but at most one of\n+\t * ctx->m and ctx->to_include may be non-NULL.\n+\t *\n+\t * When ctx->m is NULL we are writing a new MIDX without reusing\n+\t * any packs from the previous layer(s). In that case, we care\n+\t * that both:\n+\t *\n+\t *   - the new layer's base MIDX (ctx->base_midx) does not\n+\t *     already contain the pack we are considering, or the new\n+\t *     layer has no base (i.e., it is a non-incremental MIDX)\n+\t *\n+\t *   - the pack appears in ctx->to_include, or ctx->to_include\n+\t *     is NULL, meaning that we can include any pack provided\n+\t *     the above condition is met.\n+\t *\n+\t * When ctx->m is non-NULL, we are writing a new MIDX that will\n+\t * subsume ctx->m and thus includes its packs. In this case, we\n+\t * could support respecting ctx->to_include, but currently\n+\t * don't.\n+\t *\n+\t * The only caller of this function which permits\n+\t * ctx->to_include being non-NULL restricts setting ctx->m when\n+\t * this is the case. So in this setting it is impossible that\n+\t * both will be non-NULL.\n \t *\n \t * If we added support for that, these next two conditional\n \t * should be performed independently (likely checking\n--- >8 ---\n\nThanks,\nTaylor\n"},{"id":"551499","messageId":"apUbQ4S-zJGtBeu2@pks.im","threadId":"65799","inReplyTo":"ao945zRwDt9ThFTG@com-79390","subject":"Re: [PATCH 3/3] midx-write: include packs above custom incremental base","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-31T06:12:19Z","receivedAt":"2026-08-31T06:12:29Z","isPatch":true,"body":"On Wed, Aug 26, 2026 at 06:38:15PM -0500, Taylor Blau wrote:\n> On Wed, Aug 26, 2026 at 02:37:23PM -0700, Junio C Hamano wrote:\n> > Taylor Blau <ttaylorr@openai.com> writes:\n> >\n> > > `ctx->to_include` can indeed be non-NULL.\n> > > ...\n> > > The filtering still does the right thing: packs covered by the selected\n> > > base are excluded, and the remaining packs are checked against the stdin\n> > > list. But the comment is wrong, so I'll fix it.\n> >\n> > Has anything happened since we saw this comment on Aug 13th?\n> \n> Not until you sent this message ;-).\n> \n> I had a small reroll prepped that I had meant to send a couple of weeks\n> ago but never got around to doing so. When I looked at it just now, I\n> found that I wasn't quite satisfied with the range-diff in that the\n> resulting block comment was somewhat confusing.\n> \n> Instead of sending a new round immediately, let me instead share the\n> comment that I wrote instead. Patrick (or others): does this comment\n> seem clear, or do you think there are ways to tighten it up further?\n> \n> --- 8< ---\n> diff --git a/midx-write.c b/midx-write.c\n> index 66da608370..ff94076104 100644\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@ -143,15 +143,31 @@ static int should_include_pack(const struct write_midx_context *ctx,\n>  \t\tm = ctx->base_midx;\n> \n>  \t/*\n> -\t * Note that m and ctx->to_include may both be set,\n> -\t * so we are testing midx_contains_pack() and\n> -\t * string_list_has_string() independently (guarded by the\n> -\t * appropriate NULL checks).\n> -\t *\n> -\t * We could support passing to_include while reusing an existing\n> -\t * MIDX, but don't currently since the reuse process drags\n> -\t * forward all packs from an existing MIDX (without checking\n> -\t * whether or not they appear in the to_include list).\n> +\t * Note that it is OK for both ctx->base_midx and\n> +\t * ctx->to_include may both be non-NULL, but at most one of\n> +\t * ctx->m and ctx->to_include may be non-NULL.\n\nThat reads a bit off. Should that be \"Note that it is OK for both ... to\nbe non-NULL\" instead?\n\n> +\t * When ctx->m is NULL we are writing a new MIDX without reusing\n> +\t * any packs from the previous layer(s). In that case, we care\n> +\t * that both:\n> +\t *\n> +\t *   - the new layer's base MIDX (ctx->base_midx) does not\n> +\t *     already contain the pack we are considering, or the new\n> +\t *     layer has no base (i.e., it is a non-incremental MIDX)\n> +\t *\n> +\t *   - the pack appears in ctx->to_include, or ctx->to_include\n> +\t *     is NULL, meaning that we can include any pack provided\n> +\t *     the above condition is met.\n> +\t *\n> +\t * When ctx->m is non-NULL, we are writing a new MIDX that will\n> +\t * subsume ctx->m and thus includes its packs. In this case, we\n> +\t * could support respecting ctx->to_include, but currently\n> +\t * don't.\n> +\t *\n> +\t * The only caller of this function which permits\n> +\t * ctx->to_include being non-NULL restricts setting ctx->m when\n> +\t * this is the case. So in this setting it is impossible that\n> +\t * both will be non-NULL.\n\nI feel like this last paragraph could be dropped -- it's something that\nwe could mention as part of the commit message, but in this function\nhere I think it's very likely to go stale fast.\n\nOther than that I think this is good. It's quite long, but I don't have\nany good ideas for how to tighten this up significantly.\n\nThanks!\n\nPatrick\n"}]}