Re: [PATCH] branch: avoid slow strvec Coccinelle matching
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 24, 2026, 15:58 UTC
- Message-ID
- <xmqqpl0c8jml.fsf@gitster.g>
- In-Reply-To
- <20260724114948.GA825505@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 30 quoted lines
> 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.
Show 20 quoted lines
> 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.