Re: [PATCH v2 2/2] Prevent git from rehashing 4GiB files

From: Jason Hatton <jhatton@globalfinishing.com>
Date: Oct 17, 2023, 14:49 UTC
Message-ID: <MW5PR16MB50791EDD3D006EFBE8DBBCB3AFD6A@MW5PR16MB5079.namprd16.prod.outlook.com>
In-Reply-To: <20231017000019.GB551672@coredump.intra.peff.net>

From: Jeff King <peff@peff.net>

Show 30 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.
> 
> 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).
> 
> -Peff

I originally wrote the code this way to work exactly like the original code with one exception: Never truncate a nonzero st_size to a zero sd_size. The original code is here in fill_stat_data:

I was attempting to use exactly the same implicit type conversion and types as the original.

We could probably write the below to do the same thing.

void fill_stat_data(struct stat_data *sd, struct stat *st)
{
      sd->sd_ctime.sec = (unsigned int)st->st_ctime;
      sd->sd_mtime.sec = (unsigned int)st->st_mtime;
      sd->sd_ctime.nsec = ST_CTIME_NSEC(*st);
      sd->sd_mtime.nsec = ST_MTIME_NSEC(*st);
      sd->sd_dev = st->st_dev;
      sd->sd_ino = st->st_ino;
      sd->sd_uid = st->st_uid;
      sd->sd_gid = st->st_gid;
      sd->sd_size = st->st_size;
+      if (sd->sd_size == 0 && st->st_size!= 0) {
+            sd->sd_size = 1;
+      }
}

- Jason D. Hatton