Re: [PATCH 1/6] pack-bitmap.c: hide bitmap internals in `read_u8()`

[Derrick Stolee <derrickstolee@github.com>]

On 3/21/2023 1:35 PM, Jeff King wrote:
> On Mon, Mar 20, 2023 at 04:02:40PM -0400, Taylor Blau wrote:
> 
>> So b5007211b6 got rid of the above convention and replaced it by reading
>> individual fields of that structure with a `read_u8()` helper that reads
>> from the region of memory pointed to by `->map`, and updates the
>> `->map_pos` pointer accordingly.
>>
>> But this forces callers to be intimately aware of `bitmap_git->map` and
>> `bitmap_git->map_pos`. Instead, teach `read_u8()` to take a `struct
>> bitmap_index *` directly, and avoid having callers deal with the
>> internals themselves.
> 
> OK. I always felt like this read_u8() and read_be32() were trying to
> match get_be32(), etc, just with an auto-incrementing "pos" pointer. And
> this patch makes them a lot less generic.
> 
> But that is probably OK. They are static-local to the bitmap file, and
> we have not found another caller who wanted them in the intervening
> years. Arguably they could be given more descriptive names, like
> read_bitmap_u8() or something, but again, being static-local to the
> file, the generic names are fine.

Since the names don't match the "get" of get_be32(), we don't run the
risk of a future public get_be8() colliding with these static-local
methods, so I agree.

Thanks,
-Stolee