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

Jeff King <[email protected]> writes:

> The static-analysis CI job uses the ubuntu-22.04 image, for no reason
> that I can really discern. It looks like coccinelle 1.3.0 is in ubuntu
> 25.10, according to:
>
>   https://packages.ubuntu.com/km/questing/coccinelle
>
> Why don't we just use the more recent version instead of trying to work
> around it? That would fix this problem and prevent future ones. Looking
> at the code in question:
>
>> diff --git a/builtin/branch.c b/builtin/branch.c
>> index 42f2221547..2415a275ea 100644
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -797,10 +797,9 @@ static int delete_merged_branches(const struct strvec *upstreams,
>>  	struct strbuf key = STRBUF_INIT;
>>  	struct hashmap_iter iter;
>>  	struct strmap_entry *entry;
>> -	size_t i;
>>  	int ret = 0;
>>  
>> -	for (i = 0; i < upstreams->nr; i++)
>> +	for (size_t i = 0; i < upstreams->nr; i++)
>>  		if (ref_filter_forked_add(&filter, upstreams->v[i]) < 0)
>>  			die(_("'%s' is not a valid branch or pattern"),
>>  			    upstreams->v[i]);
>
> ...there is nothing suspicious or wrong about it. It seems likely that
> somebody else may end up writing something similar and triggering the
> same problem.

Exactly.

> That said, moving the iterator into the loop declaration is perhaps
> nicer anyway, because it avoids two unrelated uses of the same variable.

Exactly again.

> 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.