Re: [PATCH] branch: avoid slow strvec Coccinelle matching

Junio C Hamano <[email protected]> writes:

>> Notably:
>>
>>> @@ -809,7 +808,7 @@ static int delete_merged_branches(const struct strvec *upstreams,
>>>  	filter.name_patterns = argv;
>>>  	filter_refs(&candidates, &filter, filter.kind);
>>>  
>>> -	for (i = 0; i < (size_t)candidates.nr; i++) {
>>> +	for (size_t i = 0; i < (size_t)candidates.nr; i++) {
>>>  		const char *branch_refname = candidates.items[i]->refname;
>>>  		const char *branch_name;
>>>  		struct branch *branch;
>>
>> This hunk is not using a strvec at all. Because it uses the same
>> variable, if we did not change this loop, then we'd still have to
>> declare "i" at the top of the function and the other loop would
>> introduce a shadowed variable. That's not wrong, but it is confusing.
>>
>> However, if we are going to have our own variable here, perhaps it
>> should use the correct type? candidate.nr is an int, so probably this
>> should also be an int, and then the gross cast can go away.
>
> Ah, very good eyes.  It is a disease to try appeasing -Wsign-compare
> without thinking, instead of questioning the value of the warning
> first, and in this case there is no reason to try forcing the use of
> size_t, even with the unnecessary casting.

Having said that, another fix might be to standardize the way we
count the number of things in an array and update 'ref-filter.h' to
use size_t in 'struct ref_array' as well.

It is not as though 2 billion refs are too few to satisfy our
needs, and in general, the platform-natural int should be used to
count things unless there is a compelling reason to deviate from
that norm.  However, "somehow we ended up counting many things in
size_t, so it is better to count everything using the same type"
could serve as "the compelling reason" to make such a change.