Re: What's cooking in git.git (Jan 2025, #05; Fri, 17)

From: Jeff King <peff@peff.net>
Date: Jan 19, 2025, 12:51 UTC
Message-ID: <20250119125146.GB1538605@coredump.intra.peff.net>
In-Reply-To: <xmqq34hg3utv.fsf@gitster.g>

On Sat, Jan 18, 2025 at 09:17:32AM -0800, Junio C Hamano wrote:

Show 5 quoted lines

> > (I'd also be interested in any comments on the "maybe we should just
> > align these buffers" approach; I'm undecided on it).
> 
> Unless we have the buffer _inside_ the helper function that may
> perform the possibly-unaligned access, I am not sure how it helps.

We sort-of do. The offending code is all static local to unpack-objects.c, and always operates on the same buffer (directly for writing, and for reading through the static fill() macro which returns it directly). And likewise in index-pack.c.

I think these two are oddballs in that they read parts of a pack into a buffer. Whereas all of the more generic pack code will mmap() it, and presumably that ends up with suitable alignment. I guess platforms with NO_MMAP would read into a malloc'd buffer, but that should likewise be prepared for any alignment. (I suppose another way of achieving alignment would simply be to turn "buffer" into a pointer and malloc it at the program start, but that still leaves the need to fix sizeof() calls).

> I guess that we can align buffers used by two existing callers,
> document that the helper function takes an aligned buffer and that
> it is a fault of the caller if somebody passes an unaligned buffer,
> but I am not sure if that is where we want to go.

The functions themselves aren't really reusable, so any new code which wants to do the same thing would end up rewriting it and potentially creating the same problem. But that's probably an argument for switching away from the cast and to put/get_be32(). It provides a more obviously better example for people to copy from.

I'll post a re-roll in a bit.

-Peff