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

[email protected] writes:

> From: Ted Nyman <[email protected]>
>
> 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.
> ...
> The CI failure reproduces locally with Coccinelle 1.1.1: applying
> strvec.cocci to the original builtin/branch.c still times out with
> "spatch --timeout 120". With this change, the same check completes in
> 0.06 seconds.

Impressive.  Nicely analyzed.

Even though this is very much like bending the code only to appease
the checker, the resulting code is arguably better in this
particular case, so I do not feel as bad as I have on other
occasions when we had to work around deficiencies in our tools [*].

I see Harald already took this in the latest update.  Thanks for
working well together.

[*]

 * Here, I do not blame Coccinelle alone.  The performance bug is
   caused by a combination of Coccinelle and the 'strvec' check that
   makes it so inefficient.  I wonder if there are ways to make the
   checks in 'strvec.cocci' more efficient?


> 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]);
> @@ -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;