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

On Fri, Jul 24, 2026 at 02:11:53AM -0700, [email protected] wrote:

> The --delete-merged implementation declares a loop index at function
> scope and reuses it to walk its strvec of upstreams and its list of
> candidate branches. Coccinelle 1.1.1 spends hours matching this against
> the separate_loop_index rule in tools/coccinelle/strvec.cocci, causing
> the static-analysis job on 'seen' to reach its six-hour timeout.

Yuck. So this is really a coccinelle problem. It looks like it has been
fixed (or at least improved) in recent versions. I can reproduce the
slowness locally on 1.2.0 (I couldn't get 1.1.1 to build), but 1.3.0 is
fast. Bisection turns up 58619b8fe (break up envs for e1 & e2,
2024-08-18), which says:

    Since 362937b2a84840e68ae021171df10c7a4cc6fbef, e1 ... e2 has the
    quantifiers for the free variables of e1 around the whole thing, to ensure
    that the when code on the ... refers to the same variables as e1.  This can
    make the semantic patch very slow, as illustrated by kmerr.cocci in
    scripts/coccinelle/null/kmerr.cocci in the Linux kernel.  The slowness
    comes from environments based on multipl metavariable bindings getting very
    large.

    To reduce (but not solve) the problem, for the first & where the left side
    has multiple results, consider these results individually when working on
    the right side, and then union the results.  This may lead to some loss of
    sharing.  Maybe it is not advantageous when the ... contains when any and
    does not contain any explicit when clause containing the variables of e1.

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.

That said, moving the iterator into the loop declaration is perhaps
nicer anyway, because it avoids two unrelated uses of the same variable.
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.

-Peff