Subject: Re: [PATCH 1/4] add-patch: Fix type missmatch rom msvc

From: Phillip Wood <[email-masked]>
Date: Sat, 28 Dec 2024 16:04:11 +0000

On 27/12/2024 14:31, Junio C Hamano wrote:
> Phillip Wood <[email-masked]> writes:
> 
>> @@ -1626,7 +1628,7 @@ static int patch_update_file(struct add_p_state *s,
>>   			else
>>   				err(s, Q_("Sorry, only %d hunk available.",
>>   					  "Sorry, only %d hunks available.",
>> -					  file_diff->hunk_nr),
>> +					  (int)file_diff->hunk_nr),
>>   				    (int)file_diff->hunk_nr);
>>   		} else if (s->answer.buf[0] == '/') {
>>   			regex_t regex;
> 
> I skimmed your "how about going this way" illustration patch and
> found all the hunks reasonable, but this one I am not sure.  Is
> there a reason why hunk_nr has to be of type size_t?

We certainly don't need to be able to hold that many hunks but changing 
it to a narrower type generates a truncation warning in ALLOC_GROW_BY() 
that macro declares a local size_t variable to hold the new element 
count and then assigns that to hunk_nr

> When queuing a hunk (and performing an operation that changes the
> number of hunks, like splitting an existing one), the code should be
> careful not to make too many hunks to overflow "int" (if that is the
> more natural type to count them---and "int" being the most natural
> integer type for the platform, I tend to think it should be fine),

Yes it's hard to see anyone wanting to use "git add -p" on INT_MAX hunks

> again, that applies equally if the type of hunk_nr is "size_t".

If we cast to "unsigned long" rather than "int" here then we'd be sure 
that there was no overflow as we only support files with up to ULONG_MAX 
lines so there cannot be more than that number of hunks. "unsigned long" 
would also match the prototype of ngettext().

Best Wishes

Phillip