{"thread":{"id":"64787","subject":"[PATCH 0/2] midx-write.c: do not optimize out writes with corrupt MIDXs","startedAt":"2026-01-12T23:45:01Z","lastAt":"2026-01-13T07:38:28Z","messageCount":5,"participants":["Taylor Blau","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"533690","messageId":"cover.1768261435.git.me@ttaylorr.com","threadId":"64787","inReplyTo":null,"subject":"[PATCH 0/2] midx-write.c: do not optimize out writes with corrupt MIDXs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-01-12T23:44:53Z","receivedAt":"2026-01-12T23:45:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"These two patches came from my work on implementing incremental MIDX\nlayer compaction.\n\nWhen rebasing on top of current 'master' (at the\ntime of writing, 8745eae506f (The 17th batch, 2026-01-11)), I noticed\nan early 'test_done' in t5319 added by 6ce9d558ced (midx-write: skip\nrewriting MIDX with `--stdin-packs` unless needed, 2025-12-10). The\nseries is structured as follows:\n\n - The first patch removes the extraneous 'test_done', which exposes a\n   failing test which is marked as such.\n\n - The second patch explains and fixes the bug, un-marking the test\n   as test_expect_failure.\n\nI was originally planning on adding these onto my series under\ntb/incremental-midx-part-3.2. But I opted to split these patches out\ninto their own topic to ensure they are picked up before v2.53.0 is\ntagged, should the larger series not be ready by then.\n\nThanks in advance for your review!\n\nTaylor Blau (2):\n  t/t5319-multi-pack-index.sh: drop early 'test_done'\n  midx-write.c: assume checksum-invalid MIDXs require an update\n\n midx-write.c                | 14 ++++++++++++++\n t/t5319-multi-pack-index.sh |  2 --\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\n\nbase-commit: 8745eae506f700657882b9e32b2aa00f234a6fb6\n-- \n2.52.0.437.gcc6f76a88cd\n"},{"id":"533691","messageId":"9c5faa5932cdd9e570406bc85ba27f94195a4d3d.1768261435.git.me@ttaylorr.com","threadId":"64787","inReplyTo":"cover.1768261435.git.me@ttaylorr.com","subject":"[PATCH 1/2] t/t5319-multi-pack-index.sh: drop early 'test_done'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-01-12T23:45:03Z","receivedAt":"2026-01-12T23:45:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In 6ce9d558ced (midx-write: skip rewriting MIDX with `--stdin-packs`\nunless needed, 2025-12-10), an extra 'test_done' was added, causing the\ntest script to finish before having run all of its tests.\n\nDropping this extraneous 'test_done' exposes a bug from commit\n6ce9d558ced that causes a subsequent test to fail. Mark that test with a\n'test_expect_failure' for now, and the subsequent commit will explain\nand fix the bug.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t5319-multi-pack-index.sh | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 794f8b5ab4e..b6622849db7 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -415,8 +415,6 @@ test_expect_success 'up-to-date multi-pack-index is retained' '\n \t)\n '\n \n-test_done\n-\n test_expect_success 'verify multi-pack-index success' '\n \tgit multi-pack-index verify --object-dir=$objdir\n '\n@@ -565,7 +563,7 @@ test_expect_success 'git fsck suppresses MIDX output with --no-progress' '\n \t! grep \"Verifying object offsets\" err\n '\n \n-test_expect_success 'corrupt MIDX is not reused' '\n+test_expect_failure 'corrupt MIDX is not reused' '\n \tcorrupt_midx_and_verify $MIDX_BYTE_OFFSET \"\\377\" $objdir \\\n \t\t\"incorrect object offset\" &&\n \tgit multi-pack-index write 2>err &&\n-- \n2.52.0.437.gcc6f76a88cd\n\n"},{"id":"533692","messageId":"952a40c1bef40f5ad2c7f6853f6da64f99350976.1768261435.git.me@ttaylorr.com","threadId":"64787","inReplyTo":"cover.1768261435.git.me@ttaylorr.com","subject":"[PATCH 2/2] midx-write.c: assume checksum-invalid MIDXs require an update","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-01-12T23:45:06Z","receivedAt":"2026-01-12T23:45:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In 6ce9d558ced (midx-write: skip rewriting MIDX with `--stdin-packs`\nunless needed, 2025-12-10), the MIDX machinery learned how to optimize\nout unnecessary writes with \"--stdin-packs\".\n\nIn order to do this, it compares the contents of the in-progress write\nagainst a MIDX loaded directly from the object store. We load a separate\nMIDX (as opposed to checking our update relative to \"ctx.m\") because the\nMIDX code does not reuse an existing MIDX with --stdin-packs, and always\nleaves \"ctx.m\" as NULL. See commit 0c5a62f14bc (midx-write.c: do not\nread existing MIDX with `packs_to_include`, 2024-06-11) for details on\nwhy.\n\nIf \"ctx.m\" is non-NULL, however, it is guaranteed to be checksum-valid,\nsince we only assign \"ctx.m\" when \"midx_checksum_valid()\" returns true.\nSince the same guard does not exist for the MIDX we pass to\n\"midx_needs_update()\", we may ignore on-disk corruption when determining\nwhether or not we can optimize out the write.\n\nAdd a similar guard within \"midx_needs_update()\" to prevent such an\nissue.\n\nA more robust fix would involve revising 0c5a62f14bc and teaching the\nMIDX generation code how to reuse an existing MIDX even when invoked\nwith \"--stdin-packs\", such that we could avoid side-loading the MIDX\ndirectly from the object store in order to call \"midx_needs_update()\".\nFor now, pursue the minimal fix.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx-write.c                | 14 ++++++++++++++\n t/t5319-multi-pack-index.sh |  2 +-\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 87b97c70872..6485cb67068 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1011,6 +1011,20 @@ static bool midx_needs_update(struct multi_pack_index *midx, struct write_midx_c\n \tstruct strbuf buf = STRBUF_INIT;\n \tbool needed = true;\n \n+\t/*\n+\t * Ensure that we have a valid checksum before consulting the\n+\t * exisiting MIDX in order to determine if we can avoid an\n+\t * update.\n+\t *\n+\t * This is necessary because the given MIDX is loaded directly\n+\t * from the object store (because we still compare our proposed\n+\t * update to any on-disk MIDX regardless of whether or not we\n+\t * have assigned \"ctx.m\") and is thus not guaranteed to have a\n+\t * valid checksum.\n+\t */\n+\tif (!midx_checksum_valid(midx))\n+\t\tgoto out;\n+\n \t/*\n \t * Ignore incremental updates for now. The assumption is that any\n \t * incremental update would be either empty (in which case we will bail\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex b6622849db7..faae98c7e76 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -563,7 +563,7 @@ test_expect_success 'git fsck suppresses MIDX output with --no-progress' '\n \t! grep \"Verifying object offsets\" err\n '\n \n-test_expect_failure 'corrupt MIDX is not reused' '\n+test_expect_success 'corrupt MIDX is not reused' '\n \tcorrupt_midx_and_verify $MIDX_BYTE_OFFSET \"\\377\" $objdir \\\n \t\t\"incorrect object offset\" &&\n \tgit multi-pack-index write 2>err &&\n-- \n2.52.0.437.gcc6f76a88cd\n"},{"id":"533713","messageId":"aWX2HSakgzcfi-CL@pks.im","threadId":"64787","inReplyTo":"9c5faa5932cdd9e570406bc85ba27f94195a4d3d.1768261435.git.me@ttaylorr.com","subject":"Re: [PATCH 1/2] t/t5319-multi-pack-index.sh: drop early 'test_done'","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-13T07:37:01Z","receivedAt":"2026-01-13T07:37:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jan 12, 2026 at 06:45:03PM -0500, Taylor Blau wrote:\n> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n> index 794f8b5ab4e..b6622849db7 100755\n> --- a/t/t5319-multi-pack-index.sh\n> +++ b/t/t5319-multi-pack-index.sh\n> @@ -415,8 +415,6 @@ test_expect_success 'up-to-date multi-pack-index is retained' '\n>  \t)\n>  '\n>  \n> -test_done\n> -\n>  test_expect_success 'verify multi-pack-index success' '\n>  \tgit multi-pack-index verify --object-dir=$objdir\n>  '\n\nOh dear, this is embarassing.\n\nPatrick\n"},{"id":"533714","messageId":"aWX2b9vj8olYODwc@pks.im","threadId":"64787","inReplyTo":"952a40c1bef40f5ad2c7f6853f6da64f99350976.1768261435.git.me@ttaylorr.com","subject":"Re: [PATCH 2/2] midx-write.c: assume checksum-invalid MIDXs require an update","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-13T07:38:23Z","receivedAt":"2026-01-13T07:38:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jan 12, 2026 at 06:45:06PM -0500, Taylor Blau wrote:\n> diff --git a/midx-write.c b/midx-write.c\n> index 87b97c70872..6485cb67068 100644\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@ -1011,6 +1011,20 @@ static bool midx_needs_update(struct multi_pack_index *midx, struct write_midx_c\n>  \tstruct strbuf buf = STRBUF_INIT;\n>  \tbool needed = true;\n>  \n> +\t/*\n> +\t * Ensure that we have a valid checksum before consulting the\n> +\t * exisiting MIDX in order to determine if we can avoid an\n> +\t * update.\n> +\t *\n> +\t * This is necessary because the given MIDX is loaded directly\n> +\t * from the object store (because we still compare our proposed\n> +\t * update to any on-disk MIDX regardless of whether or not we\n> +\t * have assigned \"ctx.m\") and is thus not guaranteed to have a\n> +\t * valid checksum.\n> +\t */\n> +\tif (!midx_checksum_valid(midx))\n> +\t\tgoto out;\n> +\n>  \t/*\n>  \t * Ignore incremental updates for now. The assumption is that any\n>  \t * incremental update would be either empty (in which case we will bail\n\nThis looks sensible to me, thanks for the fix!\n\nPatrick\n"}]}