Re: [PATCH 4/6] pack-bitmap.c: factor out manual `map_pos` manipulation

[Taylor Blau <me@xxxxxxxxxxxx>]

On Fri, Mar 24, 2023 at 02:29:29PM -0400, Jeff King wrote:
> We know the advance will succeed because we checked ahead of time that
> we had enough bytes. So it really is a BUG() if we don't, as it would
> indicate somebody missed the earlier check. On the other hand, it is a
> weird spot for an extra check, because by definition we'll have just
> read off the array just before the seek.

Here you claim that we want bitmap_index_seek_to() to call BUG() if we
end up with map_pos >= map_size. But...

> The case where we _do_ seek directly to a file-provided offset, rather
> than incrementing, is an important check that this series adds, but that
> one should be a die() and not a BUG().

...here you say that it should be a die().

I think it does depend on the context. When seeking directly to a
position before reading something, die()-ing is appropriate. The case
where you seek to a relative position to reflect that you just read
something, a BUG() is appropriate.

So really, I think you want something like this:

    static void bitmap_index_seek_set(struct bitmap_index *bitmap_git, size_t pos)
    {
      if (pos >= bitmap_git->map_size)
        die(_("bitmap position exceeds size (%"PRIuMAX" >= %"PRIuMAX")"),
            (uintmax_t)bitmap_git->map_pos,
            (uintmax_t)bitmap_git->map_size);

      bitmap_git->map_pos = pos;
    }

    static void bitmap_index_seek_ahead(struct bitmap_index *bitmap_git,
                                        size_t offset)
    {
      if (bitmap_git->map_pos + offset >= bitmap_git->map_size)
        BUG("cannot seek %"PRIuMAX" byte(s) ahead of %"PRIuMAX" "
            "(%"PRIuMAX" >= %"PRIuMAX")",
            (uintmax_t)offset,
            (uintmax_t)bitmap_git->map_pos,
            (uintmax_t)(bitmap_git->map_pos + offset),
            (uintmax_t)bitmap_git->map_size);

      bitmap_git->map_pos += offset;
    }

Does that match what you were thinking?

Thanks,
Taylor