Re: [PATCH v2 2/2] string-list API: change "nr" and "alloc" to "size_t"

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 7, 2022, 20:43 UTC
Message-ID: <xmqqy21lqyrk.fsf@gitster.g>
In-Reply-To: <9c9ca557-122e-3bcb-db4b-3b5ba7bf9369@iee.email>

Philip Oakley <philipoakley@iee.email> writes:

Show 14 quoted lines

>> @@ -448,10 +448,10 @@ void shortlog_output(struct shortlog *log)
>>  				(int)UTIL_TO_INT(item), item->string);
>>  		} else {
>>  			struct string_list *onelines = item->util;
>> -			fprintf(log->file, "%s (%d):\n",
>> -				item->string, onelines->nr);
>> -			for (j = onelines->nr - 1; j >= 0; j--) {
>> -				const char *msg = onelines->items[j].string;
>> +			fprintf(log->file, "%s (%"PRIuMAX"):\n",
>> +				item->string, (uintmax_t)onelines->nr);
>> +			for (j = onelines->nr; j >= 1; j--) {
>> +				const char *msg = onelines->items[j - 1].string;
> Does this change of iteration limits and j's offset need a commit
> message comment?

Good eyes. I also tried to made sure this is the only loop that counts toward zero that use a variable whose type has become size_t with this patch, which is unsigned and cannot use "var >= 0" as the loop termination condition, but what is not in the patch is hard to know. If there were loops that used to count down with the .nr or the .alloc members of string_list structure, they would need to be updated, but if such a code exists, it is likely a bug already.

I think many internal index string-list.c uses are still "int" (cf. get_entry_index() and functions on the call graph that leads to it, like add_entry() and string_list_remove()) that wants to be updated to "ssize_t"; if the string_list has very many elements, add_entry() may not find an existing string and add a duplicate at the end, because the bisection search get_entry_index() uses cannot move the upper-bound pointer beyond "int".

It would have been better if you culled the parts of the patch you are not referring to, by the way.

Thanks.