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

[Jeff King <peff@peff.net>]

On Mon, Jan 25, 2021 at 06:37:14PM -0500, Taylor Blau wrote:

> +struct revindex_header {
> +	uint32_t signature;
> +	uint32_t version;
> +	uint32_t hash_id;
> +};
> [...]
> +	struct revindex_header *hdr;
> [...]
> +	data = xmmap(NULL, revindex_size, PROT_READ, MAP_PRIVATE, fd, 0);
> +	hdr = data;
> +
> +	if (ntohl(hdr->signature) != RIDX_SIGNATURE) {

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.

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...

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().

The rest of the patch looks good to me.

-Peff