Re: [PATCH 6/6] pack-bitmap.c: factor out `bitmap_index_seek_commit()`

From: Jeff King <peff@peff.net>
Date: Mar 21, 2023, 18:27 UTC
Message-ID: <20230321182732.GK3119834@coredump.intra.peff.net>
In-Reply-To: <ZBn0iFE5qOJKVeel@nand.local>

On Tue, Mar 21, 2023 at 02:16:40PM -0400, Taylor Blau wrote:

Show 8 quoted lines

> On Tue, Mar 21, 2023 at 02:13:15PM -0400, Jeff King wrote:
> > I'm not 100% sure on where these offsets come from. But it looks like
> > they're coming from the bitmap lookup table. In which case a bogus value
> > there should be an error(), and not a BUG(), I would think.
> 
> They do come from the lookup table, yes. I'm not sure that I agree that
> bogus values here should be an error() or a BUG(), or if I even have a
> strong preference between one and the other.

The usual philosophy we've applied is: a BUG() should not be trigger-able, even if Git is fed bad data. A BUG() should indicate an error in the program logic, and if we see one, there should be a code fix that handles the case.

Whereas if I understand this correctly, if I corrupt the bitmap file on disk, we'd trigger this BUG().

In many cases I think one could argue that it's kind of academic. But in this case we should be able to say "oops, the bitmap file seems corrupt" and skip using it, rather than bailing completely from the process.

Show 8 quoted lines

> But I do think that trying to make it an error() makes it awkward for
> all of the other callers that want it to be a BUG(), since the detail of
> whether to call one or the other is private to bitmap_index_seek().
>
> We *could* open-code it, introduce a variant of bitmap_index_seek(),
> make it take an additional parameter specifying whether to call one over
> the other, *or* check the bounds ourselves before even calling
> bitmap_index_seek().

I'm mostly unconvinced of the value of bitmap_index_seek() doing checking at all, because it is too late in most of the cases. In fact it is only in this case that it is doing something useful, which makes me think that the check should be open-coded here.

-Peff