[PATCH 2/2] midx-write.c: assume checksum-invalid MIDXs require an update
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Jan 12, 2026, 23:45 UTC
- Message-ID
- <952a40c1bef40f5ad2c7f6853f6da64f99350976.1768261435.git.me@ttaylorr.com>
- In-Reply-To
- <cover.1768261435.git.me@ttaylorr.com>
In 6ce9d558ced (midx-write: skip rewriting MIDX with `--stdin-packs` unless needed, 2025-12-10), the MIDX machinery learned how to optimize out unnecessary writes with "--stdin-packs".
In order to do this, it compares the contents of the in-progress write against a MIDX loaded directly from the object store. We load a separate MIDX (as opposed to checking our update relative to "ctx.m") because the MIDX code does not reuse an existing MIDX with --stdin-packs, and always leaves "ctx.m" as NULL. See commit 0c5a62f14bc (midx-write.c: do not read existing MIDX with `packs_to_include`, 2024-06-11) for details on why.
If "ctx.m" is non-NULL, however, it is guaranteed to be checksum-valid, since we only assign "ctx.m" when "midx_checksum_valid()" returns true. Since the same guard does not exist for the MIDX we pass to "midx_needs_update()", we may ignore on-disk corruption when determining whether or not we can optimize out the write.
Add a similar guard within "midx_needs_update()" to prevent such an issue.
A more robust fix would involve revising 0c5a62f14bc and teaching the MIDX generation code how to reuse an existing MIDX even when invoked with "--stdin-packs", such that we could avoid side-loading the MIDX directly from the object store in order to call "midx_needs_update()". For now, pursue the minimal fix.
Signed-off-by: Taylor Blau <me@ttaylorr.com> --- midx-write.c | 14 ++++++++++++++ t/t5319-multi-pack-index.sh | 2 +- 2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/midx-write.c b/midx-write.c index 87b97c70872..6485cb67068 100644 --- a/midx-write.c +++ b/midx-write.c @@ -1011,6 +1011,20 @@ static bool midx_needs_update(struct multi_pack_index *midx, struct write_midx_c struct strbuf buf = STRBUF_INIT; bool needed = true; + /* + * Ensure that we have a valid checksum before consulting the + * exisiting MIDX in order to determine if we can avoid an + * update. + * + * This is necessary because the given MIDX is loaded directly + * from the object store (because we still compare our proposed + * update to any on-disk MIDX regardless of whether or not we + * have assigned "ctx.m") and is thus not guaranteed to have a + * valid checksum. + */ + if (!midx_checksum_valid(midx)) + goto out; + /* * Ignore incremental updates for now. The assumption is that any * incremental update would be either empty (in which case we will bail diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh index b6622849db7..faae98c7e76 100755 --- a/t/t5319-multi-pack-index.sh +++ b/t/t5319-multi-pack-index.sh @@ -563,7 +563,7 @@ test_expect_success 'git fsck suppresses MIDX output with --no-progress' ' ! grep "Verifying object offsets" err ' -test_expect_failure 'corrupt MIDX is not reused' ' +test_expect_success 'corrupt MIDX is not reused' ' corrupt_midx_and_verify $MIDX_BYTE_OFFSET "\377" $objdir \ "incorrect object offset" && git multi-pack-index write 2>err &&
-- 2.52.0.437.gcc6f76a88cd