Re: [PATCH/RFC v1 1/1] Use size_t instead of unsigned long

From: Torsten Bögershausen <tboegi@web.de>
Date: Nov 19, 2018, 05:33 UTC
Message-ID: <f0c0e2e8-717d-9973-5533-8b806474e119@web.de>
In-Reply-To: <xmqqwopadkf9.fsf@gitster-ct.c.googlers.com>

On 2018-11-19 00:40, Junio C Hamano wrote:

Show 22 quoted lines

> Derrick Stolee <stolee@gmail.com> writes:
> 
>>> This needs to go on top of pu, to cover all the good stuff
>>>    cooking here.
>>
>> Better to work on top of 'master', as the work in 'pu' will be
>> rewritten several times, probably.
> 
> We may not be able to find any good moment to update some codepaths
> with deep callchains that reaches a basic API function that take
> ulong that way, as things are always in motion, but hopefully a lot
> of areas that need changes are rather isolated.  
> 
> For example, the changes I see around "offset" (which is "ulong" and
> the patch wants to change it to "size_t") in archive-tar.c in the
> patch do not have any interaction with the changes in this patch
> outside that single file, and I do not think any topic in-flight
> would interact with this change badly, either.  I didn't carefully
> look at the remainder of the patches, but I have a feeling that many
> can be separated out into independent and focused set of smaller
> patches that can be evaluated on their own.
>

The archive-tar.c is actually a good example, why a step-by-step update is not ideal (the code would not work any more on Win64).

If we look here:

static int stream_blocked(const struct object_id *oid)
{
	struct git_istream *st;
	enum object_type type;
	size_t sz;
	char buf[BLOCKSIZE];
	ssize_t readlen;

	st = open_istream(oid, &type, &sz, NULL);
                                      ^^^^^
	if (!st)
		return error(_("cannot stream blob %s"), oid_to_hex(oid));
	for (;;) {

The sz variable must follow whatever open_istream() uses, so if we start with archive-tar.c, we must use either size_t or ulong, whatever open_istream() needs. Otherwise things will break: archive-tar.c uses ulong, open_istream() size_t, but we are passing pointers around, and here &ulong != &size_t

If we only update open_istream(), but not archive-tar.c, then things are not better: &size_t != &ulong.

I don't have a good idea how to split the patch. However, "add a coccinelle script" may be a solution.