Re: [PATCH 5/6] pack-bitmap.c: use `bitmap_index_seek()` where possible

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> Yes, I think the SEEK_SET cases really do need to be doing more
> checking. AFAICT they are blindly trusting the offsets in the file
> (which is locally generated, so it's more of a corruption problem than a
> security one, but still). And this series improves that, which is good
> (but I still think it should be a die() and not a BUG()).

Yes, I think by mistake I merged the topic way too early than it has
been discussed sufficiently.  I haven't reverted the merge into 'next'
but it may not be a bad idea if the concensus is that the seek-like
whence interface is too ugly to live.  BUG() that triggers on data
errors should be updated to die(), whether we do it as a follow-on
patch or with a replacement iteration.

> Certainly there could be more consistency in the magic numbers. E.g., in
> this code:
>
>                 if (bitmap_git->map_size - bitmap_git->map_pos < bitmap_header_size) {
>                         error(_("corrupt ewah bitmap: truncated header for bitmap of commit \"%s\""),
>                                 oid_to_hex(&xor_item->oid));
>                         goto corrupt;
>                 }
>
>                 bitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);
>                 xor_flags = read_u8(bitmap_git->map, &bitmap_git->map_pos);
>                 bitmap = read_bitmap_1(bitmap_git);
>
> There is an assumption that sizeof(uint32_t) + sizeof(uint8_t) is equal
> to bitmap_header_size - 1. That's not wrong, but it's hard to verify
> that it's doing the right thing, and it's potentially fragile to changes
> (though such changes seem unlikely).

Yeah, that was the part of the code I was looking at when I wrote
the message you are responding to X-<.

Thanks.