"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
