cocci: add and apply a rule to find "unused" variables

Commit Message

Ævar Arnfjörð Bjarmason May 20, 2022, 11:57 a.m. UTC

Add a coccinelle rule to remove variable initialization followed by
calling a "release" function. This rule automatically finds the sort
of issue patched in[1], and more.

We happened to only have occurrences of strbuf_release() matching this
rule, but manual testing reveals that it'll find e.g. the same pattern
if "string_list_clear()" were used instead.

1. https://lore.kernel.org/git/042d[e-mail]/

Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
---

This overlaps but cleanly merges with the small series that [1] is in,
but just adding a coccinelle rule to catch this seemed like a good
thing to have. We even have another such case in builtin/merge.c, near
the change made in [1]!

FWIW I wrote this working rule too, but there were no in-tree hits for
it, so I didn't include it:

	// Unused declaration + malloc + free()
	@@
	identifier I;
	type T;
	// malloc(), xmalloc(), calloc() etc.
	identifier MALLOC =~ "^x?[mc]alloc$";
	@@

	(
	- T *I;
	  ... when != I
	- I = MALLOC(...);
	|
	- T *I = MALLOC(...);
	)
	  ... when != I
	- free(I);

 builtin/fetch.c                 |  3 +--
 builtin/merge.c                 |  4 ----
 builtin/repack.c                |  2 --
 contrib/coccinelle/unused.cocci | 15 +++++++++++++++
 diff.c                          |  2 --
 5 files changed, 16 insertions(+), 10 deletions(-)
 create mode 100644 contrib/coccinelle/unused.cocci

Patch

diff --git a/builtin/fetch.c b/builtin/fetch.c
index e3791f09ed5..600c28fdb75 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1113,7 +1113,7 @@  static int store_updated_refs(const char *raw_url, const char *remote_name,
 			      struct fetch_head *fetch_head, struct worktree **worktrees)
 {
 	int url_len, i, rc = 0;
-	struct strbuf note = STRBUF_INIT, err = STRBUF_INIT;
+	struct strbuf note = STRBUF_INIT;
 	const char *what, *kind;
 	struct ref *rm;
 	char *url;
@@ -1281,7 +1281,6 @@  static int store_updated_refs(const char *raw_url, const char *remote_name,
 
  abort:
 	strbuf_release(&note);
-	strbuf_release(&err);
 	free(url);
 	return rc;
 }
diff --git a/builtin/merge.c b/builtin/merge.c
index f178f5a3ee1..bb6b0580659 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -375,7 +375,6 @@  static void reset_hard(const struct object_id *oid, int verbose)
 static void restore_state(const struct object_id *head,
 			  const struct object_id *stash)
 {
-	struct strbuf sb = STRBUF_INIT;
 	const char *args[] = { "stash", "apply", NULL, NULL };
 
 	if (is_null_oid(stash))
@@ -391,7 +390,6 @@  static void restore_state(const struct object_id *head,
 	 */
 	run_command_v_opt(args, RUN_GIT_CMD);
 
-	strbuf_release(&sb);
 	refresh_cache(REFRESH_QUIET);
 }
 
@@ -501,7 +499,6 @@  static void merge_name(const char *remote, struct strbuf *msg)
 {
 	struct commit *remote_head;
 	struct object_id branch_head;
-	struct strbuf buf = STRBUF_INIT;
 	struct strbuf bname = STRBUF_INIT;
 	struct merge_remote_desc *desc;
 	const char *ptr;
@@ -589,7 +586,6 @@  static void merge_name(const char *remote, struct strbuf *msg)
 		oid_to_hex(&remote_head->object.oid), remote);
 cleanup:
 	free(found_ref);
-	strbuf_release(&buf);
 	strbuf_release(&bname);
 }
 
diff --git a/builtin/repack.c b/builtin/repack.c
index d1a563d5b65..52f8450f1be 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -609,7 +609,6 @@  int cmd_repack(int argc, const char **argv, const char *prefix)
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	struct string_list_item *item;
 	struct string_list names = STRING_LIST_INIT_DUP;
-	struct string_list rollback = STRING_LIST_INIT_NODUP;
 	struct string_list existing_nonkept_packs = STRING_LIST_INIT_DUP;
 	struct string_list existing_kept_packs = STRING_LIST_INIT_DUP;
 	struct pack_geometry *geometry = NULL;
@@ -955,7 +954,6 @@  int cmd_repack(int argc, const char **argv, const char *prefix)
 	}
 
 	string_list_clear(&names, 0);
-	string_list_clear(&rollback, 0);
 	string_list_clear(&existing_nonkept_packs, 0);
 	string_list_clear(&existing_kept_packs, 0);
 	clear_pack_geometry(geometry);
diff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci
new file mode 100644
index 00000000000..52c23e15310
--- /dev/null
+++ b/contrib/coccinelle/unused.cocci
@@ -0,0 +1,15 @@ 
+// Unused init assignment + release()
+@@
+identifier I;
+type T;
+constant INIT =~ "_INIT";
+// stbuf_release(), string_list_clear() etc.
+identifier REL1 =~ "^[a-z_]*_(release|clear|free)$";
+// release_patch(), clear_pathspec() etc.
+identifier REL2 =~ "^(release|clear|free)_[a-z_]*$";
+@@
+
+- T I = INIT;
+  <+... when != \( I \| &I \)
+- \( REL1 \| REL2 \)(&I, ...);
+  ...+>
diff --git a/diff.c b/diff.c
index ef7159968b6..57997937071 100644
--- a/diff.c
+++ b/diff.c
@@ -1289,7 +1289,6 @@  static void emit_diff_symbol_from_struct(struct diff_options *o,
 {
 	static const char *nneof = " No newline at end of file\n";
 	const char *context, *reset, *set, *set_sign, *meta, *fraginfo;
-	struct strbuf sb = STRBUF_INIT;
 
 	enum diff_symbol s = eds->s;
 	const char *line = eds->line;
@@ -1521,7 +1520,6 @@  static void emit_diff_symbol_from_struct(struct diff_options *o,
 	default:
 		BUG("unknown diff symbol");
 	}
-	strbuf_release(&sb);
 }
 
 static void emit_diff_symbol(struct diff_options *o, enum diff_symbol s,