Re: [RFC] 'unsigned long' to 'size_t' conversion

From: Brandon Williams <bmwill@google.com>
Date: Dec 6, 2017, 18:10 UTC
Message-ID: <20171206181002.GA59217@google.com>
In-Reply-To: <a660460d-b294-5113-bfaf-d98bcf99bad5@gmail.com>

On 12/06, Derrick Stolee wrote:

Show 35 quoted lines

> There are several places in Git where we refer to the size of an
> object by an 'unsigned long' instead of a 'size_t'. In 64-bit Linux,
> 'unsigned long' is 8 bytes, but in 64-bit Windows it is 4 bytes.
> 
> The main issue with this conversion is that large objects fail to
> load (they seem to hash and store just fine). For example, the
> following 'blob8gb' is an 8 GB file where the ith byte is equal to i
> % 256:
> 
> $ git hash-object -w --no-filters blob8gb
> 5391939346b98600acc0283dda24649450cec51f
> 
> $ git cat-file -s 5391939346b98600acc0283dda24649450cec51f
> error: bad object header
> fatal: git cat-file: could not get object info
> 
> An existing discussion can be found here:
> https://github.com/git-for-windows/git/issues/1063
> 
> The error message results from unpack_object_header_buffer() which
> had its most-recent meaningful change in 'ea4f9685:
> unpack_object_header_buffer(): clear the size field upon error' (in
> 2011).
> 
> In my opinion, the correct thing to do would be to replace all
> 'unsigned long's that refer to an object size and replace them with
> 'size_t'. However, a simple "git grep 'unsigned long size'" reveals
> 194 results, and there are other permutations of names and pointer
> types all over.
> 
> This conversion would be a significant patch, so I wanted to get the
> community's thoughts on this conversion.
> 
> If there are small, isolated chunks that can be done safely, then
> this may be a good target for a first patch.

I think that an effort like this would definitely be worthwhile. Much like the unsigned char[20] -> struct object_id conversion I would think that the best way to go about such a conversion would be to do it in small chunks as you've mentioned. That way you are only causing churn in hopefully small parts of the code base at a time instead of one monolithic change that is sure to cause conflicts.

-- 
Brandon Williams