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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 12, 2023, 17:58 UTC
Message-ID: <xmqqpm1jn2nh.fsf@gitster.g>
In-Reply-To: <20231012160930.330618-3-sandals@crustytoothpaste.net>

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> An example would be to have a 2^32 sized file in the index of
> patched git. Patched git would save the file as 2^31 in the cache.
> An unpatched git would very much see the file has changed in size
> and force it to rehash the file, which is safe.

The reason why this is "safe" is because an older Git will would keep rehashing whether 2^31 or 0 is stored as its sd_size, so the change is not making things worse? With older git, "git diff-files" will report that such a file is not up to date, and then the user will refresh the index, which will store 0 as its sd_file, so tentatively "git status" may give a wrong information, but we probalby do not care? Is that how the reasoning goes?

Show 15 quoted lines

> +/*
> + * Munge st_size into an unsigned int.
> + */
> +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;
> +}

This assumes typeof(sd_size) aka "unsigned int" is always 32-bit, which does not sound reasonable. Reference to 4GiB, 2^32 and 2^31 in the code and the proposed commit log message need to be qualified with "on a platform whose uint is 32-bit" or something, or better yet, phrased in a way that is agnostic to the integer size. At the very least, the hardcoded 0x80000000 needs to be rethought, I am afraid.

Other than that, the workaround for the racy-git avoidance code does sound good. I actually wonder if we should always use 1 regardless of integer size.