"Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > if (!hashfile_checksum_valid((const unsigned char *)p->revindex_map, p->revindex_size)) { > error(_("invalid checksum")); > - return -1; > + res = -1; > } > > - return 0; > + /* This may fail due to a broken .idx. */ > + if (create_pack_revindex_in_memory(p)) > + return res; Here, if the revindex file itself is self consistent, res would still be 0, so we will end up silently returning. Is the idea that while whatever causes in-memory revindex fail to be computed is a concern from the point of view of the repository's health, it would be caught elsewhere as a problem for the <pack,idx> pair we are seeing here? > + for (size_t i = 0; i < p->num_objects; i++) { > + uint32_t nr = p->revindex[i].nr; > + uint32_t rev_val = get_be32(p->revindex_data + i); > + > + if (nr != rev_val) { > + error(_("invalid rev-index position at %"PRIu64": %"PRIu32" != %"PRIu32""), > + (uint64_t)i, nr, rev_val); > + res = -1; > + } > + } > + > + return res; > } > > int load_midx_revindex(struct multi_pack_index *m) > diff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh > index 6b7c709a1f6..5c3c80f88f0 100755 > --- a/t/t5325-reverse-index.sh > +++ b/t/t5325-reverse-index.sh > @@ -185,4 +185,9 @@ test_expect_success 'fsck catches invalid checksum' ' > "invalid checksum" > ' > > +test_expect_success 'fsck catches invalid row position' ' > + corrupt_rev_and_verify 14 "\07" \ ;-) I wondered how the patch made this "\07" portably available; it is fed as the format string to printf in the helper, which is clever but obviously correct way to do this. Nice. > + "invalid rev-index position" > +' > + > test_done