Re: [PATCH v3 01/10] packfile: prepare for the existence of '*.rev' files

[Taylor Blau <me@ttaylorr.com>]

On Thu, Jan 28, 2021 at 07:27:09PM -0500, Jeff King wrote:
> I guess this technique was pulled from the .idx code paths, but IMHO we
> should avoid this kind of struct-casting. It relies on the compiler's
> struct packing, and I'm not 100% sure it doesn't violate some weird
> pointer-aliasing rules.

Yeah, that's exactly where it came from. I briefly wondered about this
myself, but decided not to worry since (a) it closely follows an
existing pattern and (b) no compiler should ever pad anywhere but the
end of this struct since all other fields are aligned without padding.

> OTOH, as long as we do not ever care about sizeof(revindex_header), this
> is likely to work in practice, since every field is at least 4-byte
> aligned (but there is probably an extra 4 bytes of padding at the end).
>
> The "right" way is probably something like:
>
>   const char *header = data;
>   if (get_be32(header) != RIDX_SIGNATURE)
>           error...;
>   header += 4;
>   if (get_be32(header) != 1)
>           error...;
>   header += 4;
>   ...etc...

Makes sense.

> I thought we had helpers to read and advance the pointer, but it looks
> like those are specific to the bitmap code (e.g., read_be32(), though it
> uses a separate offset variable).
>
> I dunno. Maybe I am being overly picky. The .idx code already does it
> like this, and I believe the index (as in .git/index) does, too. We have
> run into problems (as in b5007211b6 (pack-bitmap: do not use gcc packed
> attribute, 2014-11-27)), but that was due to a more odd-sized struct, as
> well as using sizeof().

How about a set of follow-up patches to address all of these spots at
the same time? That would allow us to move forward here (which is safe
to do, as you note), and address all of these instances together
uniformly.

Sound good?

Thanks,
Taylor