On 27/12/2024 14:31, Junio C Hamano wrote:
Phillip Wood <phillip.wood123@xxxxxxxxx> 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
