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

Jeff King [off-list ref] 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:

quoted

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:

quoted

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