Re: [PATCH v2 2/2] Prevent git from rehashing 4GiB files
- From
brian m. carlson <sandals@crustytoothpaste.net>
- Date
- Oct 18, 2023, 00:42 UTC
- Message-ID
- <ZS8p8pdUiM6NmiVo@tapette.crustytoothpaste.net>
- In-Reply-To
- <20231017000019.GB551672@coredump.intra.peff.net>
On 2023-10-17 at 00:00:19, Jeff King wrote:
Show 20 quoted lines
> On Thu, Oct 12, 2023 at 04:09:30PM +0000, brian m. carlson wrote:
>
> > +static unsigned int munge_st_size(off_t st_size) {
> > + unsigned int sd_size = st_size;
> > +
> > + /*
> > + * If the file is an exact multiple of 4 GiB, modify the value so it
> > + * doesn't get marked as racily clean (zero).
> > + */
> > + if (!sd_size && st_size)
> > + return 0x80000000;
> > + else
> > + return sd_size;
> > +}
>
> Coverity complained that the "true" side of this conditional is
> unreachable, since sd_size is assigned from st_size, so the two values
> cannot be both true and false. But obviously we are depending here on
> the truncation of off_t to "unsigned int". I'm not sure if Coverity is
> just dumb, or if it somehow has a different size for off_t.Technically, on 32-bit machines, you can have a 32-bit off_t if you don't compile with -D_FILE_OFFSET_BITS=64. However, such a program is not very useful on modern systems, since it wouldn't be able to handle files that are 2 GiB or larger, and so everyone considers that a silly and buggy way to compile programs.
Show 7 quoted lines
> I don't _think_ this would ever cause confusion in a real compiler, as > assignment from a larger type to a smaller has well-defined truncation, > as far as I know. > > But I do wonder if an explicit "& 0xFFFFFFFF" would make it more obvious > what is happening (which would also do the right thing if in some > hypothetical platform "unsigned int" ended up larger than 32 bits).
Such a hypothetical platform is of course allowed by the C standard, but it's also going to run near zero real Unix programs or kernels. I am at this point reflecting on the prudent decision Rust made to make almost everything an explicit bit size (e.g., u32, i64).
Nonetheless, I'll reroll this with the other things you mentioned, and I'll switch that "unsigned int" above to "uint32_t", which I think makes this more obvious.
I don't, however, intend to change the constant from 0x8000000 to 1, because I think that's a poorer choice, but I'll try to explain it better in the commit message. (I had originally aimed to avoid editing it as much as possible since it's not my name on the commit to avoid Jason getting blamed unnecessarily for any mistakes I might make.)
-- brian m. carlson (he/him or they/them) Toronto, Ontario, CA