From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 16 Oct 2026 14:30:41 GMT
Subject: Re: [PATCH 2/5] http: use `size_t` for byte position in fwrite_sha1_file
Message-ID: <xmqq1p9pk99q.fsf@gitster.g>
In-Reply-To: <atHiiye4oi_lF5FP@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> On Wed, Oct 14, 2026 at 06:09:28AM +0000, Johannes Schindelin via GitGitGadget wrote:
>> diff --git a/http.c b/http.c
>> index 4af3c29c76..809471f436 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -2848,7 +2848,7 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,
>>  {
>>  	unsigned char expn[4096];
>>  	size_t size = eltsize * nmemb;
>> -	int posn = 0;
>> +	size_t posn = 0;
>>  	struct http_object_request *freq = data;
>>  	struct active_request_slot *slot = freq->slot;
>
> Right. This is how we access the variable:
>
> 	do {
> 		ssize_t retval = xwrite(freq->localfile,
> 					(char *) ptr + posn, size - posn);
> 		if (retval < 0)
> 			return posn / eltsize;
> 		posn += retval;
> 	} while (posn < size);
>
> So it's clear that it doesn't have to be signed either, as we only ever
> add positive values to it.

I agree that changing the type of posn from int to size_t is the
right move.

But I wondered what happens when (size - posn) is larger than half
the range of the ssize_t type early in the loop.  After all, the
proposed commit log message worries about the case where HTTP
response body exceeds 2GB.

Shouldn't the loop clamp the bytecount passed to xwrite() below
whatever would fit in ssize_t when successful?  It may end up making
two calls of xwrite, writing 1.5GB each, to write out 3GB payload,
but that would be better than attempting to write 3GB, succeed, and
be fooled by ssize_t that is narrower by size_t by at least one bit
that we got a negative return from xwrite() and return an error from
here, when in reality we have written them all?

    ... goes and looks ... 

Ah, nevermind.  The big comment in git-compat-util.h before
MAX_IO_SIZE clarifies all of the above.  In short, xwrite() takes
care of clamping, so its callers including this code do not have to
worry about it.