From: Jeff King <peff@peff.net>
Date: Tue, 21 Mar 2023 17:35:56 GMT
Subject: Re: [PATCH 1/6] pack-bitmap.c: hide bitmap internals in `read_u8()`
Message-ID: <20230321173556.GD3119834@coredump.intra.peff.net>
In-Reply-To: <8c70c94a3f5f0463041dbaaa542fd69bd159428b.1679342296.git.me@ttaylorr.com>

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.

-Peff