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

From: Jeff King <peff@peff.net>
Date: Jul 24, 2026, 11:49 UTC
Message-ID: <20260724114948.GA825505@coredump.intra.peff.net>
In-Reply-To: <20260724091152.27794-2-tnyman@openai.com>

On Fri, Jul 24, 2026 at 02:11:53AM -0700, tnyman@openai.com wrote:

Show 5 quoted lines

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

Show 16 quoted lines

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

Show 9 quoted lines

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