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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Dec 28, 2024, 16:04 UTC
Message-ID: <11a36c3d-d42c-45c5-bed7-0e40205d66ea@gmail.com>
In-Reply-To: <xmqq5xn5urhv.fsf@gitster.g>

On 27/12/2024 14:31, Junio C Hamano wrote:

Show 15 quoted lines

> Phillip Wood <phillip.wood123@gmail.com> 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

Show 5 quoted lines

> 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