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

[Taylor Blau <me@ttaylorr.com>]

On Fri, Mar 24, 2023 at 02:04:05PM -0400, Derrick Stolee wrote:
> The other alternative would be to use an enum instead of an arbitrary int.
> The compiler can warn to some extent, but it's not as helpful as a method
> name distinction.

Yeah, I think that another enum here just to distinguish between seeking
to an absolute position versus a relative one is probably overkill in
this instance.

> >> +	if (bitmap_git->map_pos >= bitmap_git->map_size)
> >> +		BUG("bitmap position exceeds size (%"PRIuMAX" >= %"PRIuMAX")",
> >> +		    (uintmax_t)bitmap_git->map_pos,
> >> +		    (uintmax_t)bitmap_git->map_size);
> >
> > This uses ">=", which is good, because it is valid to walk the pointer
> > to one past the end of the map, which is effectively EOF. But as above,
> > in that condition the callers should be checking for this EOF state
> > before reading the bytes.
>
> In other words, it would be too easy for a strange data shape to trigger
> this BUG() statement, which should be reserved for the most-extreme cases
> of developer mistake. Things like "this is an unacceptable 'whence' value"
> or "this should never be NULL" make sense, but this is too likely to be
> hit due to a runtime error.

> Would it make sense to return an 'int' instead of the size_t of map_pos?
> That way we could return in error if this is exceeded, and then all
> callers can respond "oh wait, that move would exceed the file size, so
> I should fail in my own way..."?

Works for me. I think bitmap_index_seek_to() would probably return the
error() itself, since I don't think it makes sense to require each
caller to come up with the same "bitmap position exceeds size" thing.

We want the message from that error() to appear regardless, but each
caller can decide what it wants to do in the presence of an error (e.g.,
continue on, propagate the error, abort the program, etc).

Something like this:

    static int bitmap_index_seek_to(struct bitmap_index *bitmap_git, size_t pos)
    {
      bitmap_git->map_pos = pos;

      if (bitmap_git->map_pos >= bitmap_git->map_size)
        return error(_("bitmap position exceeds size "
                       "(%"PRIuMAX" >= %"PRIuMAX")"),
                     (uintmax_t)bitmap_git->map_pos,
                     (uintmax_t)bitmap_git->map_size);
      return 0;
    }

Thanks,
Taylor