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

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

[Ævar Arnfjörð Bjarmason]

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/042d624b8159364229e95d35e9309f12b67f8173.1652977582.git.gitgitgadget@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

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

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,
-- 
2.36.1.960.g7a4e2fc85c9

[PATCH v2 0/2] add and apply a rule to find "unused" init+free

[Ævar Arnfjörð Bjarmason]

This re-roll of [1] gets to the same end-state as far as the changed C
code is concerned, but I rewrote the coccinelle rule to be much more
general, based on the more recent discussion at [2].

See the "contrib/coccinelle/unused.cocci" part of 1/2 for extensive
commentary and what the rule does & doesn't spot, which I won't repeat
here.

The 2/2 is then split up to show the effect that "when strict" has on
this.

I noted in [2] that I had a WIP rule to to remove that unused
"get_worktrees()" but couldn't figure out a bug, this coccinelle code
will properly remove that sort of code, but only if it's actually
unused.

1. https://lore.kernel.org/git/patch-1.1-7d90f26b73f-20220520T115426Z-avarab@gmail.com/
2. https://lore.kernel.org/git/220620.865ykvw2l4.gmgdl@evledraar.gmail.com/

Ævar Arnfjörð Bjarmason (2):
  cocci: add and apply a rule to find "unused" variables
  cocci: remove "when strict" from unused.cocci

 builtin/fetch.c                 |  3 +-
 builtin/merge.c                 |  4 ---
 builtin/repack.c                |  2 --
 contrib/coccinelle/unused.cocci | 64 +++++++++++++++++++++++++++++++++
 contrib/scalar/scalar.c         |  3 +-
 diff.c                          |  2 --
 6 files changed, 66 insertions(+), 12 deletions(-)
 create mode 100644 contrib/coccinelle/unused.cocci

Range-diff against v1:
1:  7d90f26b73f < -:  ----------- cocci: add and apply a rule to find "unused" variables
-:  ----------- > 1:  d14036521ab cocci: add and apply a rule to find "unused" variables
-:  ----------- > 2:  4130dc15287 cocci: remove "when strict" from unused.cocci
-- 
2.36.1.1239.gfba91521d90

[PATCH v2 1/2] cocci: add and apply a rule to find "unused" variables

[Ævar Arnfjörð Bjarmason]

Add a coccinelle rule to remove variable initialization followed by
calling a "release" function. See extensive commentary in the new
"unused.cocci" for how it works, and what it's intended to find and
replace.

The inclusion of "contrib/scalar/scalar.c" is because "spatch" was
manually run on it (we don't usually run spatch on contrib).

The use of "with strict" here will be explained and amended in the
following commit.

1. https://lore.kernel.org/git/042d624b8159364229e95d35e9309f12b67f8173.1652977582.git.gitgitgadget@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/fetch.c                 |  3 +-
 builtin/merge.c                 |  2 -
 contrib/coccinelle/unused.cocci | 66 +++++++++++++++++++++++++++++++++
 contrib/scalar/scalar.c         |  3 +-
 diff.c                          |  2 -
 5 files changed, 68 insertions(+), 8 deletions(-)
 create mode 100644 contrib/coccinelle/unused.cocci

diff --git a/builtin/fetch.c b/builtin/fetch.c
index ac29c2b1ae3..8a3ae71fed0 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 d9784d4891c..bbd70b17bc6 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -502,7 +502,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;
@@ -590,7 +589,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/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci
new file mode 100644
index 00000000000..45452f8979a
--- /dev/null
+++ b/contrib/coccinelle/unused.cocci
@@ -0,0 +1,66 @@
+// This rule finds sequences of "unused" declerations, init and
+// release(). E.g.:
+//
+//	struct strbuf buf = STRBUF_INIT;
+//      [.. no other use of "buf" in the function ..]
+//	strbuf_release(&buf)
+//
+// To do do this we find (continued below)...
+@@
+type T;
+identifier I;
+// STRBUF_INIT, but also e.g. STRING_LIST_INIT_DUP (so no anchoring)
+constant INIT =~ "_INIT";
+// I = get_worktrees() etc.
+identifier INIT_ASSIGN1 =~ "^get_worktrees$";
+// strbuf_init(&I, ...) etc.
+identifier INIT_CALL1 =~ "^[a-z_]*_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_]*$";
+@@
+
+// .. A declaration like "struct strbuf buf;"...
+(
+- T I;
+// ... or "struct STRBUF buf = STRBUF_INIT;" ...
+|
+- T I = INIT;
+)
+
+// ... Optionally followed by lines that make no use of "buf", "&buf"
+// etc., but which ...
+<... when != \( I \| &I \)
+     when strict
+// .. (only) make use of "buf" or "&buf" to call something like
+// "strbuf_init(&buf, ...)" ...
+(
+- \( INIT_CALL1 \)( \( I \| &I \), ...);
+|
+// .. or e.g. "worktrees = get_worktrees();", i.e. a known "assignment
+// init" ...
+- I = \( INIT_ASSIGN1 \)(...);
+)
+...>
+
+// ... and then no mention of "buf" or "&buf" until we get to a
+// strbuf_release(&buf) at the end ...
+(
+- \( REL1 \| REL2 \)( \( I \| &I \), ...);
+|
+- \( REL1 \| REL2 \)( \( &I \| I \) );
+)
+// ... and no use *after* either, e.g. we don't want to delete
+// init/strbuf_release() patterns, where "&buf" could be used
+// afterwards.
+  ... when != \( I \| &I \)
+      when strict
+// Note that we're intentionally loose in accepting e.g. a
+// "strbuf_init(&buf)" followed by a "string_list_clear(&buf,
+// 0)". It's assumed that the compiler will catch any such invalid
+// code, i.e. that our constructors/destructors don't take a "void *".
+//
+// This rule also isn't capable of finding cases where &buf is used,
+// but only to e.g. pass that variable to a static function which
+// doesn't use it. The analysis is only function-local.
diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 28176914e57..97e71fe19cd 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -687,7 +687,7 @@ static int cmd_diagnose(int argc, const char **argv)
 	int stdout_fd = -1, archiver_fd = -1;
 	time_t now = time(NULL);
 	struct tm tm;
-	struct strbuf path = STRBUF_INIT, buf = STRBUF_INIT;
+	struct strbuf buf = STRBUF_INIT;
 	int res = 0;
 
 	argc = parse_options(argc, argv, NULL, options,
@@ -779,7 +779,6 @@ static int cmd_diagnose(int argc, const char **argv)
 	free(argv_copy);
 	strvec_clear(&archiver_args);
 	strbuf_release(&zip_path);
-	strbuf_release(&path);
 	strbuf_release(&buf);
 
 	return res;
diff --git a/diff.c b/diff.c
index e71cf758861..d4290615aaa 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,
-- 
2.36.1.1239.gfba91521d90

[PATCH v2 2/2] cocci: remove "when strict" from unused.cocci

[Ævar Arnfjörð Bjarmason]

Remove the "when strict" constraint from the newly introduced rule to
find unused code. As seen in the change this will help us find cases
where a "return" was causing spatch in the middle of our match was
causing spatch to abort its analysis.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/merge.c                 | 2 --
 builtin/repack.c                | 2 --
 contrib/coccinelle/unused.cocci | 2 --
 3 files changed, 6 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index bbd70b17bc6..23170f2d2a6 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);
 }
 
diff --git a/builtin/repack.c b/builtin/repack.c
index 4a7ae4cf489..482b66f57d6 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -727,7 +727,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;
@@ -1117,7 +1116,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
index 45452f8979a..6eaea01380b 100644
--- a/contrib/coccinelle/unused.cocci
+++ b/contrib/coccinelle/unused.cocci
@@ -32,7 +32,6 @@ identifier REL2 =~ "^(release|clear|free)_[a-z_]*$";
 // ... Optionally followed by lines that make no use of "buf", "&buf"
 // etc., but which ...
 <... when != \( I \| &I \)
-     when strict
 // .. (only) make use of "buf" or "&buf" to call something like
 // "strbuf_init(&buf, ...)" ...
 (
@@ -55,7 +54,6 @@ identifier REL2 =~ "^(release|clear|free)_[a-z_]*$";
 // init/strbuf_release() patterns, where "&buf" could be used
 // afterwards.
   ... when != \( I \| &I \)
-      when strict
 // Note that we're intentionally loose in accepting e.g. a
 // "strbuf_init(&buf)" followed by a "string_list_clear(&buf,
 // 0)". It's assumed that the compiler will catch any such invalid
-- 
2.36.1.1239.gfba91521d90

Re: [PATCH v2 1/2] cocci: add and apply a rule to find "unused" variables

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> +identifier INIT_ASSIGN1 =~ "^get_worktrees$";
> +// strbuf_init(&I, ...) etc.
> +identifier INIT_CALL1 =~ "^[a-z_]*_init$";
> +// stbuf_release(), string_list_clear() etc.

strbuf?

> +identifier REL1 =~ "^[a-z_]*_(release|clear|free)$";
> +// release_patch(), clear_pathspec() etc.
> +identifier REL2 =~ "^(release|clear|free)_[a-z_]*$";
> +@@

I am hesitant to see this broad set of patterns that could match
init/release functions (and possible false positive matches).

Especially given that it ended up finding only 4 instances, all of
the same "STRBUF_INIT" followed by "strbuf_release()", which means
that all other possible matches, when they actually are found, will
be seen by developers who are not necessarily familiar with these
rules before they are inspected by those who are for correctness.

It would be nice to have a step that catch only strbuf_init(),
STRBUF_INIT, strbuf_release(), and nothing else, possibly with
another step with concrete function names, with other "presumably
functions whose name match this loose pattern are all release
functions" patterns in a separate follow-up patch so that the last
one can easily be reverted.

> +// .. A declaration like "struct strbuf buf;"...
> +(
> +- T I;
> +// ... or "struct STRBUF buf = STRBUF_INIT;" ...
> +|
> +- T I = INIT;
> +)

Presumably, if either of the above followed by foo_release(I) should
be caught, then we should catch "T I = { 0 };" followed by a release
as well.  Initialization "T I = { 1, };" for a type without _INIT
macro is also the same story.

Given that, do we even need to limit the forms of declaration?  The
only thing we care about is that I is new in this scope, and I is
not used otherwise, in a way other than (1) calling _init() function
on it, or (2) calling _release() function on it, before leaving the
scope, right?

Thanks.

[PATCH v3 0/4] add and apply a rule to find "unused" init+free

[Ævar Arnfjörð Bjarmason]

This series adds a coccinelle rule to find and remove code where the
only reference to a variable in a given function is to malloc() &
free() it, where "malloc" and "free" also match
"strbuf_init/strbuf_release", and then later in the series anything
that looks like a init/free pattern.

Changes since v2:

 * Make the wider rule revert-able, as requested by Junio in
   https://lore.kernel.org/git/xmqqsfnw65zu.fsf@gitster.g/

 * We now find and remove "malloc" followed by an optional "init" and
   "release".

 * We now match { 0 } initializers, in addition to things that look
   like "INIT" macros.

Ævar Arnfjörð Bjarmason (4):
  cocci: add and apply a rule to find "unused" strbufs
  cocci: catch unused "strbuf" using an xmalloc() pattern
  cocci: remove "when strict" from unused.cocci
  cocci: generalize "unused" rule to cover more than "strbuf"

 builtin/fetch.c                 |  3 +-
 builtin/merge.c                 |  4 --
 builtin/repack.c                |  2 -
 contrib/coccinelle/unused.cocci | 88 +++++++++++++++++++++++++++++++++
 contrib/scalar/scalar.c         |  3 +-
 diff.c                          |  2 -
 6 files changed, 90 insertions(+), 12 deletions(-)
 create mode 100644 contrib/coccinelle/unused.cocci

Range-diff against v2:
1:  d14036521ab ! 1:  49e9ccb5819 cocci: add and apply a rule to find "unused" variables
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    cocci: add and apply a rule to find "unused" variables
    +    cocci: add and apply a rule to find "unused" strbufs
     
    -    Add a coccinelle rule to remove variable initialization followed by
    -    calling a "release" function. See extensive commentary in the new
    -    "unused.cocci" for how it works, and what it's intended to find and
    -    replace.
    +    Add a coccinelle rule to remove "struct strbuf" initialization
    +    followed by calling "strbuf_release()" function.
    +
    +    See extensive commentary in the new "unused.cocci" for how it works,
    +    and what it's intended to find and replace.
     
         The inclusion of "contrib/scalar/scalar.c" is because "spatch" was
         manually run on it (we don't usually run spatch on contrib).
    @@ Commit message
         The use of "with strict" here will be explained and amended in the
         following commit.
     
    -    1. https://lore.kernel.org/git/042d624b8159364229e95d35e9309f12b67f8173.1652977582.git.gitgitgadget@gmail.com/
    -
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/fetch.c ##
    @@ builtin/merge.c: static void merge_name(const char *remote, struct strbuf *msg)
     
      ## contrib/coccinelle/unused.cocci (new) ##
     @@
    -+// This rule finds sequences of "unused" declerations, init and
    -+// release(). E.g.:
    ++// This rule finds sequences of "unused" declerations and uses of
    ++// "struct strbuf".
    ++//
    ++// I.e. this finds cases where we only declare the variable, and then
    ++// release it, e.g.:
     +//
     +//	struct strbuf buf = STRBUF_INIT;
     +//      [.. no other use of "buf" in the function ..]
     +//	strbuf_release(&buf)
     +//
    ++// Or:
    ++//
    ++//	struct strbuf buf;
    ++//	[.. no other use of "buf" in the function ..]
    ++//	strbuf_init(&buf, 0);
    ++//	[.. no other use of "buf" in the function ..]
    ++//	strbuf_release(&buf)
    ++//
     +// To do do this we find (continued below)...
     +@@
     +type T;
     +identifier I;
    -+// STRBUF_INIT, but also e.g. STRING_LIST_INIT_DUP (so no anchoring)
    -+constant INIT =~ "_INIT";
    -+// I = get_worktrees() etc.
    -+identifier INIT_ASSIGN1 =~ "^get_worktrees$";
    ++// STRBUF_INIT
    ++constant INIT_MACRO =~ "^STRBUF_INIT$";
     +// strbuf_init(&I, ...) etc.
    -+identifier INIT_CALL1 =~ "^[a-z_]*_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_]*$";
    ++identifier INIT_CALL1 =~ "^strbuf_init$";
    ++// strbuf_release()
    ++identifier REL1 =~ "^strbuf_release$";
     +@@
     +
     +// .. A declaration like "struct strbuf buf;"...
     +(
     +- T I;
    ++// ... or "struct strbuf buf = { 0 };" ...
    ++|
    ++- T I = { 0 };
     +// ... or "struct STRBUF buf = STRBUF_INIT;" ...
     +|
    -+- T I = INIT;
    ++- T I = INIT_MACRO;
     +)
     +
     +// ... Optionally followed by lines that make no use of "buf", "&buf"
    @@ contrib/coccinelle/unused.cocci (new)
     +     when strict
     +// .. (only) make use of "buf" or "&buf" to call something like
     +// "strbuf_init(&buf, ...)" ...
    -+(
     +- \( INIT_CALL1 \)( \( I \| &I \), ...);
    -+|
    -+// .. or e.g. "worktrees = get_worktrees();", i.e. a known "assignment
    -+// init" ...
    -+- I = \( INIT_ASSIGN1 \)(...);
    -+)
     +...>
     +
     +// ... and then no mention of "buf" or "&buf" until we get to a
     +// strbuf_release(&buf) at the end ...
    -+(
    -+- \( REL1 \| REL2 \)( \( I \| &I \), ...);
    -+|
    -+- \( REL1 \| REL2 \)( \( &I \| I \) );
    -+)
    ++- \( REL1 \)( \( &I \| I \) );
     +// ... and no use *after* either, e.g. we don't want to delete
     +// init/strbuf_release() patterns, where "&buf" could be used
     +// afterwards.
     +  ... when != \( I \| &I \)
     +      when strict
    -+// Note that we're intentionally loose in accepting e.g. a
    -+// "strbuf_init(&buf)" followed by a "string_list_clear(&buf,
    -+// 0)". It's assumed that the compiler will catch any such invalid
    -+// code, i.e. that our constructors/destructors don't take a "void *".
    -+//
     +// This rule also isn't capable of finding cases where &buf is used,
     +// but only to e.g. pass that variable to a static function which
     +// doesn't use it. The analysis is only function-local.
-:  ----------- > 2:  6324d3956ed cocci: catch unused "strbuf" using an xmalloc() pattern
2:  4130dc15287 ! 3:  9a5e7208dec cocci: remove "when strict" from unused.cocci
    @@ builtin/merge.c: static void restore_state(const struct object_id *head,
      }
      
     
    - ## builtin/repack.c ##
    -@@ builtin/repack.c: 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;
    -@@ builtin/repack.c: 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);
    -
      ## contrib/coccinelle/unused.cocci ##
    -@@ contrib/coccinelle/unused.cocci: identifier REL2 =~ "^(release|clear|free)_[a-z_]*$";
    +@@ contrib/coccinelle/unused.cocci: identifier REL1 =~ "^strbuf_release$";
      // ... Optionally followed by lines that make no use of "buf", "&buf"
      // etc., but which ...
      <... when != \( I \| &I \)
     -     when strict
    + (
      // .. (only) make use of "buf" or "&buf" to call something like
      // "strbuf_init(&buf, ...)" ...
    - (
    -@@ contrib/coccinelle/unused.cocci: identifier REL2 =~ "^(release|clear|free)_[a-z_]*$";
    +@@ contrib/coccinelle/unused.cocci: identifier REL1 =~ "^strbuf_release$";
      // init/strbuf_release() patterns, where "&buf" could be used
      // afterwards.
        ... when != \( I \| &I \)
     -      when strict
    - // Note that we're intentionally loose in accepting e.g. a
    - // "strbuf_init(&buf)" followed by a "string_list_clear(&buf,
    - // 0)". It's assumed that the compiler will catch any such invalid
    + // This rule also isn't capable of finding cases where &buf is used,
    + // but only to e.g. pass that variable to a static function which
    + // doesn't use it. The analysis is only function-local.
-:  ----------- > 4:  45a429b9cc9 cocci: generalize "unused" rule to cover more than "strbuf"
-- 
2.37.0.900.g4d0de1cceb2

[PATCH v3 1/4] cocci: add and apply a rule to find "unused" strbufs

[Ævar Arnfjörð Bjarmason]

Add a coccinelle rule to remove "struct strbuf" initialization
followed by calling "strbuf_release()" function.

See extensive commentary in the new "unused.cocci" for how it works,
and what it's intended to find and replace.

The inclusion of "contrib/scalar/scalar.c" is because "spatch" was
manually run on it (we don't usually run spatch on contrib).

The use of "with strict" here will be explained and amended in the
following commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/fetch.c                 |  3 +-
 builtin/merge.c                 |  2 --
 contrib/coccinelle/unused.cocci | 61 +++++++++++++++++++++++++++++++++
 contrib/scalar/scalar.c         |  3 +-
 diff.c                          |  2 --
 5 files changed, 63 insertions(+), 8 deletions(-)
 create mode 100644 contrib/coccinelle/unused.cocci

diff --git a/builtin/fetch.c b/builtin/fetch.c
index ac29c2b1ae3..8a3ae71fed0 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 d9784d4891c..bbd70b17bc6 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -502,7 +502,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;
@@ -590,7 +589,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/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci
new file mode 100644
index 00000000000..bc26d39b313
--- /dev/null
+++ b/contrib/coccinelle/unused.cocci
@@ -0,0 +1,61 @@
+// This rule finds sequences of "unused" declerations and uses of
+// "struct strbuf".
+//
+// I.e. this finds cases where we only declare the variable, and then
+// release it, e.g.:
+//
+//	struct strbuf buf = STRBUF_INIT;
+//      [.. no other use of "buf" in the function ..]
+//	strbuf_release(&buf)
+//
+// Or:
+//
+//	struct strbuf buf;
+//	[.. no other use of "buf" in the function ..]
+//	strbuf_init(&buf, 0);
+//	[.. no other use of "buf" in the function ..]
+//	strbuf_release(&buf)
+//
+// To do do this we find (continued below)...
+@@
+type T;
+identifier I;
+// STRBUF_INIT
+constant INIT_MACRO =~ "^STRBUF_INIT$";
+// strbuf_init(&I, ...) etc.
+identifier INIT_CALL1 =~ "^strbuf_init$";
+// strbuf_release()
+identifier REL1 =~ "^strbuf_release$";
+@@
+
+// .. A declaration like "struct strbuf buf;"...
+(
+- T I;
+// ... or "struct strbuf buf = { 0 };" ...
+|
+- T I = { 0 };
+// ... or "struct STRBUF buf = STRBUF_INIT;" ...
+|
+- T I = INIT_MACRO;
+)
+
+// ... Optionally followed by lines that make no use of "buf", "&buf"
+// etc., but which ...
+<... when != \( I \| &I \)
+     when strict
+// .. (only) make use of "buf" or "&buf" to call something like
+// "strbuf_init(&buf, ...)" ...
+- \( INIT_CALL1 \)( \( I \| &I \), ...);
+...>
+
+// ... and then no mention of "buf" or "&buf" until we get to a
+// strbuf_release(&buf) at the end ...
+- \( REL1 \)( \( &I \| I \) );
+// ... and no use *after* either, e.g. we don't want to delete
+// init/strbuf_release() patterns, where "&buf" could be used
+// afterwards.
+  ... when != \( I \| &I \)
+      when strict
+// This rule also isn't capable of finding cases where &buf is used,
+// but only to e.g. pass that variable to a static function which
+// doesn't use it. The analysis is only function-local.
diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 28176914e57..97e71fe19cd 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -687,7 +687,7 @@ static int cmd_diagnose(int argc, const char **argv)
 	int stdout_fd = -1, archiver_fd = -1;
 	time_t now = time(NULL);
 	struct tm tm;
-	struct strbuf path = STRBUF_INIT, buf = STRBUF_INIT;
+	struct strbuf buf = STRBUF_INIT;
 	int res = 0;
 
 	argc = parse_options(argc, argv, NULL, options,
@@ -779,7 +779,6 @@ static int cmd_diagnose(int argc, const char **argv)
 	free(argv_copy);
 	strvec_clear(&archiver_args);
 	strbuf_release(&zip_path);
-	strbuf_release(&path);
 	strbuf_release(&buf);
 
 	return res;
diff --git a/diff.c b/diff.c
index e71cf758861..d4290615aaa 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,
-- 
2.37.0.900.g4d0de1cceb2

[PATCH v3 2/4] cocci: catch unused "strbuf" using an xmalloc() pattern

[Ævar Arnfjörð Bjarmason]

There's no current matches for this rule, but it will match both:

	struct strbuf *buf = xmalloc(sizeof(struct strbuf));
	strbuf_init(buf, 0);
	strbuf_release(buf);

And:

	struct strbuf *buf;

	buf = xmalloc(sizeof(struct strbuf));
	strbuf_init(buf, 0);
	strbuf_release(buf);

Note that we'd also match a strbuf_init() before the xmalloc(), but
we're not seeking to be so strict as to make checks that the compiler
will catch for us redundant, and saying we'll match either "init" or
"xmalloc" lines makes the rule simpler.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/coccinelle/unused.cocci | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci
index bc26d39b313..43942f3cd4f 100644
--- a/contrib/coccinelle/unused.cocci
+++ b/contrib/coccinelle/unused.cocci
@@ -22,6 +22,8 @@ type T;
 identifier I;
 // STRBUF_INIT
 constant INIT_MACRO =~ "^STRBUF_INIT$";
+// x[mc]alloc() etc.
+identifier MALLOC1 =~ "^x?[mc]alloc$";
 // strbuf_init(&I, ...) etc.
 identifier INIT_CALL1 =~ "^strbuf_init$";
 // strbuf_release()
@@ -37,15 +39,25 @@ identifier REL1 =~ "^strbuf_release$";
 // ... or "struct STRBUF buf = STRBUF_INIT;" ...
 |
 - T I = INIT_MACRO;
+|
+// ... or "struct strbuf *buf = xmalloc(...)" etc. ...
+- T I = MALLOC1(...);
 )
 
 // ... Optionally followed by lines that make no use of "buf", "&buf"
 // etc., but which ...
 <... when != \( I \| &I \)
      when strict
+(
 // .. (only) make use of "buf" or "&buf" to call something like
 // "strbuf_init(&buf, ...)" ...
 - \( INIT_CALL1 \)( \( I \| &I \), ...);
+|
+// .. or to follow-up a "struct strbuf *buf" with e.g. "buf =
+// xmalloc(...)" (which may in turn be followed-up by a
+// "strbuf_init()", which we'll match with INIT_CALL1) ...
+- I = MALLOC1(...);
+)
 ...>
 
 // ... and then no mention of "buf" or "&buf" until we get to a
-- 
2.37.0.900.g4d0de1cceb2

[PATCH v3 3/4] cocci: remove "when strict" from unused.cocci

[Ævar Arnfjörð Bjarmason]

Remove the "when strict" constraint from the newly introduced rule to
find unused code. As seen in the change this will help us find cases
where a "return" was causing spatch in the middle of our match was
causing spatch to abort its analysis.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/merge.c                 | 2 --
 contrib/coccinelle/unused.cocci | 2 --
 2 files changed, 4 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index bbd70b17bc6..23170f2d2a6 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);
 }
 
diff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci
index 43942f3cd4f..9f0101c1350 100644
--- a/contrib/coccinelle/unused.cocci
+++ b/contrib/coccinelle/unused.cocci
@@ -47,7 +47,6 @@ identifier REL1 =~ "^strbuf_release$";
 // ... Optionally followed by lines that make no use of "buf", "&buf"
 // etc., but which ...
 <... when != \( I \| &I \)
-     when strict
 (
 // .. (only) make use of "buf" or "&buf" to call something like
 // "strbuf_init(&buf, ...)" ...
@@ -67,7 +66,6 @@ identifier REL1 =~ "^strbuf_release$";
 // init/strbuf_release() patterns, where "&buf" could be used
 // afterwards.
   ... when != \( I \| &I \)
-      when strict
 // This rule also isn't capable of finding cases where &buf is used,
 // but only to e.g. pass that variable to a static function which
 // doesn't use it. The analysis is only function-local.
-- 
2.37.0.900.g4d0de1cceb2

[PATCH v3 4/4] cocci: generalize "unused" rule to cover more than "strbuf"

[Ævar Arnfjörð Bjarmason]

Generalize the newly added "unused.cocci" rule to find more than just
"struct strbuf", let's have it find the same unused patterns for
"struct string_list", as well as other code that uses
similar-looking *_{release,clear,free}() and {release,clear,free}_*()
functions.

See [1] for example of code that would be covered by the
"get_worktrees()" part of this rule. We'd still need work that the
series is based on (we were passing "worktrees" to a function), but
could now do the change in [1] automatically.

1. https://lore.kernel.org/git/Yq6eJFUPPTv%2Fzc0o@coredump.intra.peff.net/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/repack.c                |  2 --
 contrib/coccinelle/unused.cocci | 31 ++++++++++++++++++++++++-------
 2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 4a7ae4cf489..482b66f57d6 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -727,7 +727,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;
@@ -1117,7 +1116,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
index 9f0101c1350..a1b09d2d73d 100644
--- a/contrib/coccinelle/unused.cocci
+++ b/contrib/coccinelle/unused.cocci
@@ -1,5 +1,5 @@
 // This rule finds sequences of "unused" declerations and uses of
-// "struct strbuf".
+// "struct strbuf" and other common types.
 //
 // I.e. this finds cases where we only declare the variable, and then
 // release it, e.g.:
@@ -20,14 +20,18 @@
 @@
 type T;
 identifier I;
-// STRBUF_INIT
-constant INIT_MACRO =~ "^STRBUF_INIT$";
+// STRBUF_INIT, but also e.g. STRING_LIST_INIT_DUP (so no anchoring)
+constant INIT_MACRO =~ "_INIT";
 // x[mc]alloc() etc.
 identifier MALLOC1 =~ "^x?[mc]alloc$";
+// I = get_worktrees() etc.
+identifier INIT_ASSIGN1 =~ "^get_worktrees$";
 // strbuf_init(&I, ...) etc.
-identifier INIT_CALL1 =~ "^strbuf_init$";
-// strbuf_release()
-identifier REL1 =~ "^strbuf_release$";
+identifier INIT_CALL1 =~ "^[a-z_]*_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_]*$";
 @@
 
 // .. A declaration like "struct strbuf buf;"...
@@ -52,6 +56,10 @@ identifier REL1 =~ "^strbuf_release$";
 // "strbuf_init(&buf, ...)" ...
 - \( INIT_CALL1 \)( \( I \| &I \), ...);
 |
+// .. or e.g. "worktrees = get_worktrees();", i.e. a known "assignment
+// init" ...
+- I = \( INIT_ASSIGN1 \)(...);
+|
 // .. or to follow-up a "struct strbuf *buf" with e.g. "buf =
 // xmalloc(...)" (which may in turn be followed-up by a
 // "strbuf_init()", which we'll match with INIT_CALL1) ...
@@ -61,11 +69,20 @@ identifier REL1 =~ "^strbuf_release$";
 
 // ... and then no mention of "buf" or "&buf" until we get to a
 // strbuf_release(&buf) at the end ...
-- \( REL1 \)( \( &I \| I \) );
+(
+- \( REL1 \| REL2 \)( \( I \| &I \), ...);
+|
+- \( REL1 \| REL2 \)( \( &I \| I \) );
+)
 // ... and no use *after* either, e.g. we don't want to delete
 // init/strbuf_release() patterns, where "&buf" could be used
 // afterwards.
   ... when != \( I \| &I \)
+// Note that we're intentionally loose in accepting e.g. a
+// "strbuf_init(&buf)" followed by a "string_list_clear(&buf,
+// 0)". It's assumed that the compiler will catch any such invalid
+// code, i.e. that our constructors/destructors don't take a "void *".
+//
 // This rule also isn't capable of finding cases where &buf is used,
 // but only to e.g. pass that variable to a static function which
 // doesn't use it. The analysis is only function-local.
-- 
2.37.0.900.g4d0de1cceb2

Re: [PATCH v3 1/4] cocci: add and apply a rule to find "unused" strbufs

[Jeff King]

On Fri, Jul 01, 2022 at 12:30:56PM +0200, Ævar Arnfjörð Bjarmason wrote:

> The use of "with strict" here will be explained and amended in the
> following commit.

Really s/the following/a following/ now that there's another commit in
between. :)

I didn't find the comment there very enlightening, though. It says we're
dropping "with strict" to catch some new cases around early returns. But
it's not clear to me what "strict" does in the first place, why we'd
want it, or what we might be losing by dropping it.

I know, I can probably go read the coccinelle docs to figure it out, but
it might be worth saying why it's worth including in the first place
somewhere (either here or in the follow-up commit).

-Peff

Re: [PATCH v3 0/4] add and apply a rule to find "unused" init+free

[Jeff King]

On Fri, Jul 01, 2022 at 12:30:55PM +0200, Ævar Arnfjörð Bjarmason wrote:

> This series adds a coccinelle rule to find and remove code where the
> only reference to a variable in a given function is to malloc() &
> free() it, where "malloc" and "free" also match
> "strbuf_init/strbuf_release", and then later in the series anything
> that looks like a init/free pattern.

Overall, I hated this much less than I expected to. ;)

My main concern was that we'd have a jumble of init/release rules for
every data type, but the regex matching on identifier names lets us do
things broadly. And for data types that don't match our usual naming
conventions, that seems like a good opportunity to consider changing
their names.

I'm not too concerned about false positives here, since there are humans
in the loop. The resulting diffs are pretty obvious to review, and a
compiler would obviously catch any case where the variable really did
get used in between.

I do wish we could do it with the compiler and not coccinelle, just
because the latter has been such a source of frustration in terms of
performance issues and head-scratching "why didn't it catch this one"
cases. But this is IMHO much better than nothing, as shown by the
cleanups it already found.

-Peff

Re: [PATCH v3 1/4] cocci: add and apply a rule to find "unused" strbufs

[Eric Sunshine]

On Fri, Jul 1, 2022 at 6:36 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> Add a coccinelle rule to remove "struct strbuf" initialization
> followed by calling "strbuf_release()" function.

I'm probably being overly pedantic, but I tripped over this failing to
mention that the strbuf is not used between the initialization and the
release. That is, I had expected it to say:

    Add a coccinelle rule to remove "struct strbuf" initialization
    followed by calling "strbuf_release()" function without any uses
    of the strbuf in between.

> See extensive commentary in the new "unused.cocci" for how it works,
> and what it's intended to find and replace.
>
> The inclusion of "contrib/scalar/scalar.c" is because "spatch" was
> manually run on it (we don't usually run spatch on contrib).
>
> The use of "with strict" here will be explained and amended in the
> following commit.

Did you mean "when strict"?

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci
> @@ -0,0 +1,61 @@
> +// This rule finds sequences of "unused" declerations and uses of

s/declerations/declarations/

> +// To do do this we find (continued below)...

s/do do/do/

Re: [PATCH v3 3/4] cocci: remove "when strict" from unused.cocci

[Eric Sunshine]

On Fri, Jul 1, 2022 at 6:36 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> Remove the "when strict" constraint from the newly introduced rule to
> find unused code. As seen in the change this will help us find cases
> where a "return" was causing spatch in the middle of our match was
> causing spatch to abort its analysis.

There seem to be some extraneous words in this paragraph. Did you mean:

    Remove the "when strict" constraint from the newly introduced
    rule to find unused code. As seen in the change this will help us
    find cases where a "return" in the middle of our match was
    causing spatch to abort its analysis.

?

[PATCH v4 0/6] add and apply a rule to find "unused" init+free

[Ævar Arnfjörð Bjarmason]

This series adds a coccinelle rule to find and remove code where the
only reference to a variable in a given function is to malloc() &
free() it, where "malloc" and "free" also match
"strbuf_init/strbuf_release", and then later in the series anything
that looks like a init/free pattern.

Changes since v3[1]

 * Add a "coccicheck-test" target in an early and new patch, the
   structure mirrors that of coccinelle.git's own tests. As the
   diffstat shows we have a *.c and *.res file which is C code
   before/after a *.cocci rule is applied.
 * The extensive commentary in the proposed rule is gone in favor of
   self-explanatory test cases
 * We now catch init/reset patterns as well as init/release fully
   (i.e. for the "struct strbuf" early on)
 * Squashed the "when strict" change in, and there's now a test-case
   for it.
 * 1-2/6 are new minor cleanup patches for what follows.

1. https://lore.kernel.org/git/cover-v3-0.4-00000000000-20220701T102506Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (6):
  Makefile: remove mandatory "spatch" arguments from SPATCH_FLAGS
  Makefile & .gitignore: ignore & clean "git.res", not "*.res"
  cocci: add a "coccicheck-test" target and test *.cocci rules
  cocci: have "coccicheck{,-pending}" depend on "coccicheck-test"
  cocci: add and apply a rule to find "unused" strbufs
  cocci: generalize "unused" rule to cover more than "strbuf"

 .gitignore                          |  2 +-
 Makefile                            | 28 ++++++++--
 builtin/fetch.c                     |  3 +-
 builtin/merge.c                     |  4 --
 builtin/repack.c                    |  2 -
 contrib/coccinelle/tests/free.c     | 11 ++++
 contrib/coccinelle/tests/free.res   |  9 ++++
 contrib/coccinelle/tests/unused.c   | 82 +++++++++++++++++++++++++++++
 contrib/coccinelle/tests/unused.res | 45 ++++++++++++++++
 contrib/coccinelle/unused.cocci     | 43 +++++++++++++++
 contrib/scalar/scalar.c             |  3 +-
 diff.c                              |  2 -
 shared.mak                          |  1 +
 13 files changed, 219 insertions(+), 16 deletions(-)
 create mode 100644 contrib/coccinelle/tests/free.c
 create mode 100644 contrib/coccinelle/tests/free.res
 create mode 100644 contrib/coccinelle/tests/unused.c
 create mode 100644 contrib/coccinelle/tests/unused.res
 create mode 100644 contrib/coccinelle/unused.cocci

Range-diff against v3:
-:  ----------- > 1:  fbdc2c3d66b Makefile: remove mandatory "spatch" arguments from SPATCH_FLAGS
-:  ----------- > 2:  d7e85d4c4a6 Makefile & .gitignore: ignore & clean "git.res", not "*.res"
-:  ----------- > 3:  540186e69dc cocci: add a "coccicheck-test" target and test *.cocci rules
-:  ----------- > 4:  48810f7390c cocci: have "coccicheck{,-pending}" depend on "coccicheck-test"
1:  49e9ccb5819 ! 5:  d1c6833c8d5 cocci: add and apply a rule to find "unused" strbufs
    @@ Commit message
         cocci: add and apply a rule to find "unused" strbufs
     
         Add a coccinelle rule to remove "struct strbuf" initialization
    -    followed by calling "strbuf_release()" function.
    +    followed by calling "strbuf_release()" function, without any uses of
    +    the strbuf in the same function.
     
    -    See extensive commentary in the new "unused.cocci" for how it works,
    -    and what it's intended to find and replace.
    +    See the tests in contrib/coccinelle/tests/unused.{c,res} for what it's
    +    intended to find and replace.
     
         The inclusion of "contrib/scalar/scalar.c" is because "spatch" was
         manually run on it (we don't usually run spatch on contrib).
     
    -    The use of "with strict" here will be explained and amended in the
    -    following commit.
    +    Per the "buggy code" comment we also match a strbuf_init() before the
    +    xmalloc(), but we're not seeking to be so strict as to make checks
    +    that the compiler will catch for us redundant. Saying we'll match
    +    either "init" or "xmalloc" lines makes the rule simpler.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *
      }
     
      ## builtin/merge.c ##
    +@@ builtin/merge.c: 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))
    +@@ builtin/merge.c: static void restore_state(const struct object_id *head,
    + 	 */
    + 	run_command_v_opt(args, RUN_GIT_CMD);
    + 
    +-	strbuf_release(&sb);
    + 	refresh_cache(REFRESH_QUIET);
    + }
    + 
     @@ builtin/merge.c: static void merge_name(const char *remote, struct strbuf *msg)
      {
      	struct commit *remote_head;
    @@ builtin/merge.c: static void merge_name(const char *remote, struct strbuf *msg)
      }
      
     
    + ## contrib/coccinelle/tests/unused.c (new) ##
    +@@
    ++void test_strbuf(void)
    ++{
    ++	struct strbuf sb1 = STRBUF_INIT;
    ++	struct strbuf sb2 = STRBUF_INIT;
    ++	struct strbuf sb3 = STRBUF_INIT;
    ++	struct strbuf sb4 = STRBUF_INIT;
    ++	struct strbuf sb5;
    ++	struct strbuf sb6 = { 0 };
    ++	struct strbuf sb7 = STRBUF_INIT;
    ++	struct strbuf sb8 = STRBUF_INIT;
    ++	struct strbuf *sp1;
    ++	struct strbuf *sp2;
    ++	struct strbuf *sp3;
    ++	struct strbuf *sp4 = xmalloc(sizeof(struct strbuf));
    ++	struct strbuf *sp5 = xmalloc(sizeof(struct strbuf));
    ++	struct strbuf *sp6 = xmalloc(sizeof(struct strbuf));
    ++	struct strbuf *sp7;
    ++
    ++	strbuf_init(&sb5, 0);
    ++	strbuf_init(sp1, 0);
    ++	strbuf_init(sp2, 0);
    ++	strbuf_init(sp3, 0);
    ++	strbuf_init(sp4, 0);
    ++	strbuf_init(sp5, 0);
    ++	strbuf_init(sp6, 0);
    ++	strbuf_init(sp7, 0);
    ++	sp7 = xmalloc(sizeof(struct strbuf));
    ++
    ++	use_before(&sb3);
    ++	use_as_str("%s", sb7.buf);
    ++	use_as_str("%s", sp1->buf);
    ++	use_as_str("%s", sp6->buf);
    ++	pass_pp(&sp3);
    ++
    ++	strbuf_release(&sb1);
    ++	strbuf_reset(&sb2);
    ++	strbuf_release(&sb3);
    ++	strbuf_release(&sb4);
    ++	strbuf_release(&sb5);
    ++	strbuf_release(&sb6);
    ++	strbuf_release(&sb7);
    ++	strbuf_release(sp1);
    ++	strbuf_release(sp2);
    ++	strbuf_release(sp3);
    ++	strbuf_release(sp4);
    ++	strbuf_release(sp5);
    ++	strbuf_release(sp6);
    ++	strbuf_release(sp7);
    ++
    ++	use_after(&sb4);
    ++
    ++	if (when_strict())
    ++		return;
    ++	strbuf_release(&sb8);
    ++}
    +
    + ## contrib/coccinelle/tests/unused.res (new) ##
    +@@
    ++void test_strbuf(void)
    ++{
    ++	struct strbuf sb3 = STRBUF_INIT;
    ++	struct strbuf sb4 = STRBUF_INIT;
    ++	struct strbuf sb7 = STRBUF_INIT;
    ++	struct strbuf *sp1;
    ++	struct strbuf *sp3;
    ++	struct strbuf *sp6 = xmalloc(sizeof(struct strbuf));
    ++	strbuf_init(sp1, 0);
    ++	strbuf_init(sp3, 0);
    ++	strbuf_init(sp6, 0);
    ++
    ++	use_before(&sb3);
    ++	use_as_str("%s", sb7.buf);
    ++	use_as_str("%s", sp1->buf);
    ++	use_as_str("%s", sp6->buf);
    ++	pass_pp(&sp3);
    ++
    ++	strbuf_release(&sb3);
    ++	strbuf_release(&sb4);
    ++	strbuf_release(&sb7);
    ++	strbuf_release(sp1);
    ++	strbuf_release(sp3);
    ++	strbuf_release(sp6);
    ++
    ++	use_after(&sb4);
    ++
    ++	if (when_strict())
    ++		return;
    ++}
    +
      ## contrib/coccinelle/unused.cocci (new) ##
     @@
    -+// This rule finds sequences of "unused" declerations and uses of
    -+// "struct strbuf".
    -+//
    -+// I.e. this finds cases where we only declare the variable, and then
    -+// release it, e.g.:
    -+//
    -+//	struct strbuf buf = STRBUF_INIT;
    -+//      [.. no other use of "buf" in the function ..]
    -+//	strbuf_release(&buf)
    -+//
    -+// Or:
    -+//
    -+//	struct strbuf buf;
    -+//	[.. no other use of "buf" in the function ..]
    -+//	strbuf_init(&buf, 0);
    -+//	[.. no other use of "buf" in the function ..]
    -+//	strbuf_release(&buf)
    -+//
    -+// To do do this we find (continued below)...
    ++// This rule finds sequences of "unused" declerations and uses of a
    ++// variable, where "unused" is defined to include only calling the
    ++// equivalent of alloc, init & free functions on the variable.
     +@@
     +type T;
     +identifier I;
    -+// STRBUF_INIT
     +constant INIT_MACRO =~ "^STRBUF_INIT$";
    -+// strbuf_init(&I, ...) etc.
    ++identifier MALLOC1 =~ "^x?[mc]alloc$";
     +identifier INIT_CALL1 =~ "^strbuf_init$";
    -+// strbuf_release()
    -+identifier REL1 =~ "^strbuf_release$";
    ++identifier REL1 =~ "^strbuf_(release|reset)$";
     +@@
     +
    -+// .. A declaration like "struct strbuf buf;"...
     +(
     +- T I;
    -+// ... or "struct strbuf buf = { 0 };" ...
     +|
     +- T I = { 0 };
    -+// ... or "struct STRBUF buf = STRBUF_INIT;" ...
     +|
     +- T I = INIT_MACRO;
    ++|
    ++- T I = MALLOC1(...);
     +)
     +
    -+// ... Optionally followed by lines that make no use of "buf", "&buf"
    -+// etc., but which ...
     +<... when != \( I \| &I \)
    -+     when strict
    -+// .. (only) make use of "buf" or "&buf" to call something like
    -+// "strbuf_init(&buf, ...)" ...
    ++(
     +- \( INIT_CALL1 \)( \( I \| &I \), ...);
    ++|
    ++- I = MALLOC1(...);
    ++)
     +...>
     +
    -+// ... and then no mention of "buf" or "&buf" until we get to a
    -+// strbuf_release(&buf) at the end ...
     +- \( REL1 \)( \( &I \| I \) );
    -+// ... and no use *after* either, e.g. we don't want to delete
    -+// init/strbuf_release() patterns, where "&buf" could be used
    -+// afterwards.
     +  ... when != \( I \| &I \)
    -+      when strict
    -+// This rule also isn't capable of finding cases where &buf is used,
    -+// but only to e.g. pass that variable to a static function which
    -+// doesn't use it. The analysis is only function-local.
     
      ## contrib/scalar/scalar.c ##
     @@ contrib/scalar/scalar.c: static int cmd_diagnose(int argc, const char **argv)
2:  6324d3956ed < -:  ----------- cocci: catch unused "strbuf" using an xmalloc() pattern
3:  9a5e7208dec < -:  ----------- cocci: remove "when strict" from unused.cocci
4:  45a429b9cc9 ! 6:  dafd59d5ded cocci: generalize "unused" rule to cover more than "strbuf"
    @@ Commit message
         similar-looking *_{release,clear,free}() and {release,clear,free}_*()
         functions.
     
    +    We're intentionally loose in accepting e.g. a "strbuf_init(&sb)"
    +    followed by a "string_list_clear(&sb, 0)".  It's assumed that the
    +    compiler will catch any such invalid code, i.e. that our
    +    constructors/destructors don't take a "void *".
    +
         See [1] for example of code that would be covered by the
         "get_worktrees()" part of this rule. We'd still need work that the
         series is based on (we were passing "worktrees" to a function), but
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
      	string_list_clear(&existing_kept_packs, 0);
      	clear_pack_geometry(geometry);
     
    + ## contrib/coccinelle/tests/unused.c ##
    +@@ contrib/coccinelle/tests/unused.c: void test_strbuf(void)
    + 		return;
    + 	strbuf_release(&sb8);
    + }
    ++
    ++void test_other(void)
    ++{
    ++	struct string_list l = STRING_LIST_INIT_DUP;
    ++	struct strbuf sb = STRBUF_INIT;
    ++
    ++	string_list_clear(&l, 0);
    ++	string_list_clear(&sb, 0);
    ++}
    ++
    ++void test_worktrees(void)
    ++{
    ++	struct worktree **w1 = get_worktrees();
    ++	struct worktree **w2 = get_worktrees();
    ++	struct worktree **w3;
    ++	struct worktree **w4;
    ++
    ++	w3 = get_worktrees();
    ++	w4 = get_worktrees();
    ++
    ++	use_it(w4);
    ++
    ++	free_worktrees(w1);
    ++	free_worktrees(w2);
    ++	free_worktrees(w3);
    ++	free_worktrees(w4);
    ++}
    +
    + ## contrib/coccinelle/tests/unused.res ##
    +@@ contrib/coccinelle/tests/unused.res: void test_strbuf(void)
    + 	if (when_strict())
    + 		return;
    + }
    ++
    ++void test_other(void)
    ++{
    ++}
    ++
    ++void test_worktrees(void)
    ++{
    ++	struct worktree **w4;
    ++
    ++	w4 = get_worktrees();
    ++
    ++	use_it(w4);
    ++
    ++	free_worktrees(w4);
    ++}
    +
      ## contrib/coccinelle/unused.cocci ##
    -@@
    - // This rule finds sequences of "unused" declerations and uses of
    --// "struct strbuf".
    -+// "struct strbuf" and other common types.
    - //
    - // I.e. this finds cases where we only declare the variable, and then
    - // release it, e.g.:
     @@
      @@
      type T;
      identifier I;
    --// STRBUF_INIT
     -constant INIT_MACRO =~ "^STRBUF_INIT$";
     +// STRBUF_INIT, but also e.g. STRING_LIST_INIT_DUP (so no anchoring)
     +constant INIT_MACRO =~ "_INIT";
    - // x[mc]alloc() etc.
      identifier MALLOC1 =~ "^x?[mc]alloc$";
    -+// I = get_worktrees() etc.
    -+identifier INIT_ASSIGN1 =~ "^get_worktrees$";
    - // strbuf_init(&I, ...) etc.
     -identifier INIT_CALL1 =~ "^strbuf_init$";
    --// strbuf_release()
    --identifier REL1 =~ "^strbuf_release$";
    +-identifier REL1 =~ "^strbuf_(release|reset)$";
    ++identifier INIT_ASSIGN1 =~ "^get_worktrees$";
     +identifier INIT_CALL1 =~ "^[a-z_]*_init$";
    -+// stbuf_release(), string_list_clear() etc.
    -+identifier REL1 =~ "^[a-z_]*_(release|clear|free)$";
    -+// release_patch(), clear_pathspec() etc.
    ++identifier REL1 =~ "^[a-z_]*_(release|reset|clear|free)$";
     +identifier REL2 =~ "^(release|clear|free)_[a-z_]*$";
      @@
      
    - // .. A declaration like "struct strbuf buf;"...
    -@@ contrib/coccinelle/unused.cocci: identifier REL1 =~ "^strbuf_release$";
    - // "strbuf_init(&buf, ...)" ...
    + (
    +@@ contrib/coccinelle/unused.cocci: identifier REL1 =~ "^strbuf_(release|reset)$";
    + - T I = INIT_MACRO;
    + |
    + - T I = MALLOC1(...);
    ++|
    ++- T I = INIT_ASSIGN1(...);
    + )
    + 
    + <... when != \( I \| &I \)
    + (
      - \( INIT_CALL1 \)( \( I \| &I \), ...);
      |
    -+// .. or e.g. "worktrees = get_worktrees();", i.e. a known "assignment
    -+// init" ...
     +- I = \( INIT_ASSIGN1 \)(...);
     +|
    - // .. or to follow-up a "struct strbuf *buf" with e.g. "buf =
    - // xmalloc(...)" (which may in turn be followed-up by a
    - // "strbuf_init()", which we'll match with INIT_CALL1) ...
    -@@ contrib/coccinelle/unused.cocci: identifier REL1 =~ "^strbuf_release$";
    + - I = MALLOC1(...);
    + )
    + ...>
      
    - // ... and then no mention of "buf" or "&buf" until we get to a
    - // strbuf_release(&buf) at the end ...
     -- \( REL1 \)( \( &I \| I \) );
     +(
     +- \( REL1 \| REL2 \)( \( I \| &I \), ...);
     +|
     +- \( REL1 \| REL2 \)( \( &I \| I \) );
     +)
    - // ... and no use *after* either, e.g. we don't want to delete
    - // init/strbuf_release() patterns, where "&buf" could be used
    - // afterwards.
        ... when != \( I \| &I \)
    -+// Note that we're intentionally loose in accepting e.g. a
    -+// "strbuf_init(&buf)" followed by a "string_list_clear(&buf,
    -+// 0)". It's assumed that the compiler will catch any such invalid
    -+// code, i.e. that our constructors/destructors don't take a "void *".
    -+//
    - // This rule also isn't capable of finding cases where &buf is used,
    - // but only to e.g. pass that variable to a static function which
    - // doesn't use it. The analysis is only function-local.
-- 
2.37.0.913.g50625c3f077

[PATCH v4 1/6] Makefile: remove mandatory "spatch" arguments from SPATCH_FLAGS

[Ævar Arnfjörð Bjarmason]

The "--patch ." part of SPATCH_FLAGS added in f57d11728d1 (coccinelle:
put sane filenames into output patches, 2018-07-23) should have been
added unconditionally to the "spatch" invocation instead, using it
isn't optional.

Let's also move the other mandatory flag to come after
$(SPATCH_FLAGS), to ensure that our "--sp-file" overrides any provided
in the environment, both --sp-file <arg> and --patch <arg> are
last-option-wins as far as spatch(1) option parsing is concerned.

The environment variable override was initially added in
a9a884aea57 (coccicheck: use --all-includes by default,
2016-09-30). In practice there's probably nobody that's using
SPATCH_FLAGS to try to intentionally break our invocations, but since
we're changing this let's make it clear what (if anything) we expect
to be overridden by user-supplied flags.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 04d0fd1fe60..c1d02f04499 100644
--- a/Makefile
+++ b/Makefile
@@ -1286,7 +1286,7 @@ SANITIZE_ADDRESS =
 # For the 'coccicheck' target; setting SPATCH_BATCH_SIZE higher will
 # usually result in less CPU usage at the cost of higher peak memory.
 # Setting it to 0 will feed all files in a single spatch invocation.
-SPATCH_FLAGS = --all-includes --patch .
+SPATCH_FLAGS = --all-includes
 SPATCH_BATCH_SIZE = 1
 
 include config.mak.uname
@@ -3131,7 +3131,8 @@ check: $(GENERATED_H)
 		limit='-n $(SPATCH_BATCH_SIZE)'; \
 	fi; \
 	if ! echo $(COCCI_SOURCES) | xargs $$limit \
-		$(SPATCH) --sp-file $< $(SPATCH_FLAGS) \
+		$(SPATCH) $(SPATCH_FLAGS) \
+		--sp-file $< --patch . \
 		>$@+ 2>$@.log; \
 	then \
 		cat $@.log; \
-- 
2.37.0.913.g50625c3f077

[PATCH v4 2/6] Makefile & .gitignore: ignore & clean "git.res", not "*.res"

[Ævar Arnfjörð Bjarmason]

Adjust the overly broad .gitignore and "make clean" rule added in
ce39c2e04ce (Provide a Windows version resource for the git
executables., 2012-05-24).

For now this is merely a correctness fix, but needed because a
subsequent commit will want to check in *.res files elsewhere in the
tree, which we shouldn't have to "git add -f".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .gitignore | 2 +-
 Makefile   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index a4522157641..42fd7253b44 100644
--- a/.gitignore
+++ b/.gitignore
@@ -185,6 +185,7 @@
 /git-worktree
 /git-write-tree
 /git-core-*/?*
+/git.res
 /gitweb/GITWEB-BUILD-OPTIONS
 /gitweb/gitweb.cgi
 /gitweb/static/gitweb.js
@@ -225,7 +226,6 @@
 *.hcc
 *.obj
 *.lib
-*.res
 *.sln
 *.sp
 *.suo
diff --git a/Makefile b/Makefile
index c1d02f04499..1ccf13595de 100644
--- a/Makefile
+++ b/Makefile
@@ -3409,7 +3409,7 @@ cocciclean:
 clean: profile-clean coverage-clean cocciclean
 	$(RM) -r .build
 	$(RM) po/git.pot po/git-core.pot
-	$(RM) *.res
+	$(RM) git.res
 	$(RM) $(OBJECTS)
 	$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
 	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
-- 
2.37.0.913.g50625c3f077

[PATCH v4 3/6] cocci: add a "coccicheck-test" target and test *.cocci rules

[Ævar Arnfjörð Bjarmason]

Add a "coccicheck-test" target to test our *.cocci rules, and as a
demonstration add tests for the rules added in 39ea59a2570 (remove
unnecessary NULL check before free(3), 2016-10-08) and
1b83d1251ed (coccinelle: add a rule to make "expression" code use
FREE_AND_NULL(), 2017-06-15).

I considered making use of the "spatch --test" option, and the choice
of a "tests" over a "t" directory is to make these tests compatible
with such a future change.

Unfortunately "spatch --test" doesn't return meaningful exit codes,
AFAICT you need to "grep" its output to see if the *.res is what you
expect. There's "--test-okfailed", but I didn't find a way to sensibly
integrate those (it relies on some in-between status files, but
doesn't help with the status codes).

Instead let's use a "--sp-file" pattern similar to the main
"coccicheck" rule, with the difference that we use and compare the
two *.res files with cmp(1).

The --very-quiet and --no-show-diff options ensure that we don't need
to pipe stdout and stderr somewhere. Unlike the "%.cocci.patch" rule
we're not using the diff.

The "cmp || git diff" is optimistically giving us better output on
failure, but even if we only have POSIX cmp and no system git
installed we'll still fail with the "cmp", just with an error message
that isn't as friendly. The "2>/dev/null" is in case we don't have a
"git" installed.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile                          | 19 +++++++++++++++++++
 contrib/coccinelle/tests/free.c   | 11 +++++++++++
 contrib/coccinelle/tests/free.res |  9 +++++++++
 shared.mak                        |  1 +
 4 files changed, 40 insertions(+)
 create mode 100644 contrib/coccinelle/tests/free.c
 create mode 100644 contrib/coccinelle/tests/free.res

diff --git a/Makefile b/Makefile
index 1ccf13595de..c4064f8394f 100644
--- a/Makefile
+++ b/Makefile
@@ -3123,6 +3123,8 @@ check: $(GENERATED_H)
 		exit 1; \
 	fi
 
+COCCI_TEST_RES = $(wildcard contrib/coccinelle/tests/*.res)
+
 %.cocci.patch: %.cocci $(COCCI_SOURCES)
 	$(QUIET_SPATCH) \
 	if test $(SPATCH_BATCH_SIZE) = 0; then \
@@ -3143,6 +3145,22 @@ check: $(GENERATED_H)
 	then \
 		echo '    ' SPATCH result: $@; \
 	fi
+
+COCCI_TEST_RES_GEN = $(addprefix .build/,$(COCCI_TEST_RES))
+$(COCCI_TEST_RES_GEN): .build/%.res : %.c
+$(COCCI_TEST_RES_GEN): .build/%.res : %.res
+$(COCCI_TEST_RES_GEN): .build/contrib/coccinelle/tests/%.res : contrib/coccinelle/%.cocci
+	$(call mkdir_p_parent_template)
+	$(QUIET_SPATCH_T)$(SPATCH) $(SPATCH_FLAGS) \
+		--very-quiet --no-show-diff \
+		--sp-file $< -o $@ \
+		$(@:.build/%.res=%.c) && \
+	cmp $(@:.build/%=%) $@ || \
+	git -P diff --no-index $(@:.build/%=%) $@ 2>/dev/null; \
+
+.PHONY: coccicheck-test
+coccicheck-test: $(COCCI_TEST_RES_GEN)
+
 coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/coccinelle/*.cocci)))
 
 # See contrib/coccinelle/README
@@ -3404,6 +3422,7 @@ profile-clean:
 	$(RM) $(addsuffix *.gcno,$(addprefix $(PROFILE_DIR)/, $(object_dirs)))
 
 cocciclean:
+	$(RM) -r .build/contrib/coccinelle
 	$(RM) contrib/coccinelle/*.cocci.patch*
 
 clean: profile-clean coverage-clean cocciclean
diff --git a/contrib/coccinelle/tests/free.c b/contrib/coccinelle/tests/free.c
new file mode 100644
index 00000000000..96d4abc0c78
--- /dev/null
+++ b/contrib/coccinelle/tests/free.c
@@ -0,0 +1,11 @@
+int use_FREE_AND_NULL(int *v)
+{
+	free(*v);
+	*v = NULL;
+}
+
+int need_no_if(int *v)
+{
+	if (v)
+		free(v);
+}
diff --git a/contrib/coccinelle/tests/free.res b/contrib/coccinelle/tests/free.res
new file mode 100644
index 00000000000..f90fd9f48e3
--- /dev/null
+++ b/contrib/coccinelle/tests/free.res
@@ -0,0 +1,9 @@
+int use_FREE_AND_NULL(int *v)
+{
+	FREE_AND_NULL(*v);
+}
+
+int need_no_if(int *v)
+{
+	free(v);
+}
diff --git a/shared.mak b/shared.mak
index 4330192e9c3..33f43edbf9a 100644
--- a/shared.mak
+++ b/shared.mak
@@ -70,6 +70,7 @@ ifndef V
 	QUIET_HDR      = @echo '   ' HDR $(<:hcc=h);
 	QUIET_RC       = @echo '   ' RC $@;
 	QUIET_SPATCH   = @echo '   ' SPATCH $<;
+	QUIET_SPATCH_T = @echo '   ' SPATCH TEST $(@:.build/%=%);
 
 ## Used in "Documentation/Makefile"
 	QUIET_ASCIIDOC	= @echo '   ' ASCIIDOC $@;
-- 
2.37.0.913.g50625c3f077

[PATCH v4 4/6] cocci: have "coccicheck{,-pending}" depend on "coccicheck-test"

[Ævar Arnfjörð Bjarmason]

Have the newly introduced "coccicheck-test" target run implicitly when
"coccicheck" itself is run. As with e.g. the "check-chainlint"
target (see [1]) it makes sense to run this unconditionally before we
run other "spatch" rules as a basic sanity check. See

1. 803394459d4 (t/Makefile: add machinery to check correctness of
   chainlint.sed, 2018-07-11)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Makefile b/Makefile
index c4064f8394f..669828a2a6b 100644
--- a/Makefile
+++ b/Makefile
@@ -3161,9 +3161,11 @@ $(COCCI_TEST_RES_GEN): .build/contrib/coccinelle/tests/%.res : contrib/coccinell
 .PHONY: coccicheck-test
 coccicheck-test: $(COCCI_TEST_RES_GEN)
 
+coccicheck: coccicheck-test
 coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/coccinelle/*.cocci)))
 
 # See contrib/coccinelle/README
+coccicheck-pending: coccicheck-test
 coccicheck-pending: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.pending.cocci))
 
 .PHONY: coccicheck coccicheck-pending
-- 
2.37.0.913.g50625c3f077

[PATCH v4 5/6] cocci: add and apply a rule to find "unused" strbufs

[Ævar Arnfjörð Bjarmason]

Add a coccinelle rule to remove "struct strbuf" initialization
followed by calling "strbuf_release()" function, without any uses of
the strbuf in the same function.

See the tests in contrib/coccinelle/tests/unused.{c,res} for what it's
intended to find and replace.

The inclusion of "contrib/scalar/scalar.c" is because "spatch" was
manually run on it (we don't usually run spatch on contrib).

Per the "buggy code" comment we also match a strbuf_init() before the
xmalloc(), but we're not seeking to be so strict as to make checks
that the compiler will catch for us redundant. Saying we'll match
either "init" or "xmalloc" lines makes the rule simpler.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/fetch.c                     |  3 +-
 builtin/merge.c                     |  4 ---
 contrib/coccinelle/tests/unused.c   | 55 +++++++++++++++++++++++++++++
 contrib/coccinelle/tests/unused.res | 30 ++++++++++++++++
 contrib/coccinelle/unused.cocci     | 32 +++++++++++++++++
 contrib/scalar/scalar.c             |  3 +-
 diff.c                              |  2 --
 7 files changed, 119 insertions(+), 10 deletions(-)
 create mode 100644 contrib/coccinelle/tests/unused.c
 create mode 100644 contrib/coccinelle/tests/unused.res
 create mode 100644 contrib/coccinelle/unused.cocci

diff --git a/builtin/fetch.c b/builtin/fetch.c
index ac29c2b1ae3..8a3ae71fed0 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 d9784d4891c..23170f2d2a6 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);
 }
 
@@ -502,7 +500,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;
@@ -590,7 +587,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/contrib/coccinelle/tests/unused.c b/contrib/coccinelle/tests/unused.c
new file mode 100644
index 00000000000..495ae58ccf1
--- /dev/null
+++ b/contrib/coccinelle/tests/unused.c
@@ -0,0 +1,55 @@
+void test_strbuf(void)
+{
+	struct strbuf sb1 = STRBUF_INIT;
+	struct strbuf sb2 = STRBUF_INIT;
+	struct strbuf sb3 = STRBUF_INIT;
+	struct strbuf sb4 = STRBUF_INIT;
+	struct strbuf sb5;
+	struct strbuf sb6 = { 0 };
+	struct strbuf sb7 = STRBUF_INIT;
+	struct strbuf sb8 = STRBUF_INIT;
+	struct strbuf *sp1;
+	struct strbuf *sp2;
+	struct strbuf *sp3;
+	struct strbuf *sp4 = xmalloc(sizeof(struct strbuf));
+	struct strbuf *sp5 = xmalloc(sizeof(struct strbuf));
+	struct strbuf *sp6 = xmalloc(sizeof(struct strbuf));
+	struct strbuf *sp7;
+
+	strbuf_init(&sb5, 0);
+	strbuf_init(sp1, 0);
+	strbuf_init(sp2, 0);
+	strbuf_init(sp3, 0);
+	strbuf_init(sp4, 0);
+	strbuf_init(sp5, 0);
+	strbuf_init(sp6, 0);
+	strbuf_init(sp7, 0);
+	sp7 = xmalloc(sizeof(struct strbuf));
+
+	use_before(&sb3);
+	use_as_str("%s", sb7.buf);
+	use_as_str("%s", sp1->buf);
+	use_as_str("%s", sp6->buf);
+	pass_pp(&sp3);
+
+	strbuf_release(&sb1);
+	strbuf_reset(&sb2);
+	strbuf_release(&sb3);
+	strbuf_release(&sb4);
+	strbuf_release(&sb5);
+	strbuf_release(&sb6);
+	strbuf_release(&sb7);
+	strbuf_release(sp1);
+	strbuf_release(sp2);
+	strbuf_release(sp3);
+	strbuf_release(sp4);
+	strbuf_release(sp5);
+	strbuf_release(sp6);
+	strbuf_release(sp7);
+
+	use_after(&sb4);
+
+	if (when_strict())
+		return;
+	strbuf_release(&sb8);
+}
diff --git a/contrib/coccinelle/tests/unused.res b/contrib/coccinelle/tests/unused.res
new file mode 100644
index 00000000000..b3b71053ed6
--- /dev/null
+++ b/contrib/coccinelle/tests/unused.res
@@ -0,0 +1,30 @@
+void test_strbuf(void)
+{
+	struct strbuf sb3 = STRBUF_INIT;
+	struct strbuf sb4 = STRBUF_INIT;
+	struct strbuf sb7 = STRBUF_INIT;
+	struct strbuf *sp1;
+	struct strbuf *sp3;
+	struct strbuf *sp6 = xmalloc(sizeof(struct strbuf));
+	strbuf_init(sp1, 0);
+	strbuf_init(sp3, 0);
+	strbuf_init(sp6, 0);
+
+	use_before(&sb3);
+	use_as_str("%s", sb7.buf);
+	use_as_str("%s", sp1->buf);
+	use_as_str("%s", sp6->buf);
+	pass_pp(&sp3);
+
+	strbuf_release(&sb3);
+	strbuf_release(&sb4);
+	strbuf_release(&sb7);
+	strbuf_release(sp1);
+	strbuf_release(sp3);
+	strbuf_release(sp6);
+
+	use_after(&sb4);
+
+	if (when_strict())
+		return;
+}
diff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci
new file mode 100644
index 00000000000..56530498d17
--- /dev/null
+++ b/contrib/coccinelle/unused.cocci
@@ -0,0 +1,32 @@
+// This rule finds sequences of "unused" declerations and uses of a
+// variable, where "unused" is defined to include only calling the
+// equivalent of alloc, init & free functions on the variable.
+@@
+type T;
+identifier I;
+constant INIT_MACRO =~ "^STRBUF_INIT$";
+identifier MALLOC1 =~ "^x?[mc]alloc$";
+identifier INIT_CALL1 =~ "^strbuf_init$";
+identifier REL1 =~ "^strbuf_(release|reset)$";
+@@
+
+(
+- T I;
+|
+- T I = { 0 };
+|
+- T I = INIT_MACRO;
+|
+- T I = MALLOC1(...);
+)
+
+<... when != \( I \| &I \)
+(
+- \( INIT_CALL1 \)( \( I \| &I \), ...);
+|
+- I = MALLOC1(...);
+)
+...>
+
+- \( REL1 \)( \( &I \| I \) );
+  ... when != \( I \| &I \)
diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 28176914e57..97e71fe19cd 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -687,7 +687,7 @@ static int cmd_diagnose(int argc, const char **argv)
 	int stdout_fd = -1, archiver_fd = -1;
 	time_t now = time(NULL);
 	struct tm tm;
-	struct strbuf path = STRBUF_INIT, buf = STRBUF_INIT;
+	struct strbuf buf = STRBUF_INIT;
 	int res = 0;
 
 	argc = parse_options(argc, argv, NULL, options,
@@ -779,7 +779,6 @@ static int cmd_diagnose(int argc, const char **argv)
 	free(argv_copy);
 	strvec_clear(&archiver_args);
 	strbuf_release(&zip_path);
-	strbuf_release(&path);
 	strbuf_release(&buf);
 
 	return res;
diff --git a/diff.c b/diff.c
index e71cf758861..d4290615aaa 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,
-- 
2.37.0.913.g50625c3f077

[PATCH v4 6/6] cocci: generalize "unused" rule to cover more than "strbuf"

[Ævar Arnfjörð Bjarmason]

Generalize the newly added "unused.cocci" rule to find more than just
"struct strbuf", let's have it find the same unused patterns for
"struct string_list", as well as other code that uses
similar-looking *_{release,clear,free}() and {release,clear,free}_*()
functions.

We're intentionally loose in accepting e.g. a "strbuf_init(&sb)"
followed by a "string_list_clear(&sb, 0)".  It's assumed that the
compiler will catch any such invalid code, i.e. that our
constructors/destructors don't take a "void *".

See [1] for example of code that would be covered by the
"get_worktrees()" part of this rule. We'd still need work that the
series is based on (we were passing "worktrees" to a function), but
could now do the change in [1] automatically.

1. https://lore.kernel.org/git/Yq6eJFUPPTv%2Fzc0o@coredump.intra.peff.net/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/repack.c                    |  2 --
 contrib/coccinelle/tests/unused.c   | 27 +++++++++++++++++++++++++++
 contrib/coccinelle/tests/unused.res | 15 +++++++++++++++
 contrib/coccinelle/unused.cocci     | 19 +++++++++++++++----
 4 files changed, 57 insertions(+), 6 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 4a7ae4cf489..482b66f57d6 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -727,7 +727,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;
@@ -1117,7 +1116,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/tests/unused.c b/contrib/coccinelle/tests/unused.c
index 495ae58ccf1..8294d734ba4 100644
--- a/contrib/coccinelle/tests/unused.c
+++ b/contrib/coccinelle/tests/unused.c
@@ -53,3 +53,30 @@ void test_strbuf(void)
 		return;
 	strbuf_release(&sb8);
 }
+
+void test_other(void)
+{
+	struct string_list l = STRING_LIST_INIT_DUP;
+	struct strbuf sb = STRBUF_INIT;
+
+	string_list_clear(&l, 0);
+	string_list_clear(&sb, 0);
+}
+
+void test_worktrees(void)
+{
+	struct worktree **w1 = get_worktrees();
+	struct worktree **w2 = get_worktrees();
+	struct worktree **w3;
+	struct worktree **w4;
+
+	w3 = get_worktrees();
+	w4 = get_worktrees();
+
+	use_it(w4);
+
+	free_worktrees(w1);
+	free_worktrees(w2);
+	free_worktrees(w3);
+	free_worktrees(w4);
+}
diff --git a/contrib/coccinelle/tests/unused.res b/contrib/coccinelle/tests/unused.res
index b3b71053ed6..6d3e745683c 100644
--- a/contrib/coccinelle/tests/unused.res
+++ b/contrib/coccinelle/tests/unused.res
@@ -28,3 +28,18 @@ void test_strbuf(void)
 	if (when_strict())
 		return;
 }
+
+void test_other(void)
+{
+}
+
+void test_worktrees(void)
+{
+	struct worktree **w4;
+
+	w4 = get_worktrees();
+
+	use_it(w4);
+
+	free_worktrees(w4);
+}
diff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci
index 56530498d17..d84046f82ea 100644
--- a/contrib/coccinelle/unused.cocci
+++ b/contrib/coccinelle/unused.cocci
@@ -4,10 +4,13 @@
 @@
 type T;
 identifier I;
-constant INIT_MACRO =~ "^STRBUF_INIT$";
+// STRBUF_INIT, but also e.g. STRING_LIST_INIT_DUP (so no anchoring)
+constant INIT_MACRO =~ "_INIT";
 identifier MALLOC1 =~ "^x?[mc]alloc$";
-identifier INIT_CALL1 =~ "^strbuf_init$";
-identifier REL1 =~ "^strbuf_(release|reset)$";
+identifier INIT_ASSIGN1 =~ "^get_worktrees$";
+identifier INIT_CALL1 =~ "^[a-z_]*_init$";
+identifier REL1 =~ "^[a-z_]*_(release|reset|clear|free)$";
+identifier REL2 =~ "^(release|clear|free)_[a-z_]*$";
 @@
 
 (
@@ -18,15 +21,23 @@ identifier REL1 =~ "^strbuf_(release|reset)$";
 - T I = INIT_MACRO;
 |
 - T I = MALLOC1(...);
+|
+- T I = INIT_ASSIGN1(...);
 )
 
 <... when != \( I \| &I \)
 (
 - \( INIT_CALL1 \)( \( I \| &I \), ...);
 |
+- I = \( INIT_ASSIGN1 \)(...);
+|
 - I = MALLOC1(...);
 )
 ...>
 
-- \( REL1 \)( \( &I \| I \) );
+(
+- \( REL1 \| REL2 \)( \( I \| &I \), ...);
+|
+- \( REL1 \| REL2 \)( \( &I \| I \) );
+)
   ... when != \( I \| &I \)
-- 
2.37.0.913.g50625c3f077

Re: [PATCH v4 0/6] add and apply a rule to find "unused" init+free

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

>  builtin/merge.c                     |  4 --

This finds and re-fixes what has already been fixed in another
topic, which is unfortunate, but it is minor.  If this topic takes
longer before en/merge-restore-to-pristine graduates, it may want to
get rerolled to avoid duplication, but for now I'll queue the series
as-is.

Thanks.

Re: [PATCH v4 0/6] add and apply a rule to find "unused" init+free

[Jeff King]

On Tue, Jul 05, 2022 at 03:46:54PM +0200, Ævar Arnfjörð Bjarmason wrote:

> This series adds a coccinelle rule to find and remove code where the
> only reference to a variable in a given function is to malloc() &
> free() it, where "malloc" and "free" also match
> "strbuf_init/strbuf_release", and then later in the series anything
> that looks like a init/free pattern.

As before, I'm perfectly happy with the actual transformations here.

> Changes since v3[1]
> 
>  * Add a "coccicheck-test" target in an early and new patch, the
>    structure mirrors that of coccinelle.git's own tests. As the
>    diffstat shows we have a *.c and *.res file which is C code
>    before/after a *.cocci rule is applied.

I have mixed feelings on this. I'm not opposed per se, and I even like
the fact that it provides examples of what we expect a rule to do. But I
worry about a world where the cost of using coccinelle goes up because
now transformations need to have tests along with them. The idea of
cocci is to make your life a little simpler and more efficient when
fixing up the code base (as opposed to perl, etc). But if it comes with
a bunch of boilerplate tasks, that might no longer be the case. I'm
already often on the fence about using cocci just because of the hours
I've sunk into debugging and puzzling things out.

I dunno. Maybe the bar is higher for stuff like this that we expect to
continue to find problems as time goes on (because it is not "here is a
transition", but "this is a common and easy mistake to make").

So again, I'm not really opposed to this patch in particular, but I want
to express caution at the direction we might be going, and at applying
new rules over-zealously.

>  * We now catch init/reset patterns as well as init/release fully
>    (i.e. for the "struct strbuf" early on)

Yeah, that makes sense.

-Peff

Re: [PATCH v4 0/6] add and apply a rule to find "unused" init+free

[Ævar Arnfjörð Bjarmason]

On Mon, Jul 11 2022, Jeff King wrote:

> On Tue, Jul 05, 2022 at 03:46:54PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> This series adds a coccinelle rule to find and remove code where the
>> only reference to a variable in a given function is to malloc() &
>> free() it, where "malloc" and "free" also match
>> "strbuf_init/strbuf_release", and then later in the series anything
>> that looks like a init/free pattern.
>
> As before, I'm perfectly happy with the actual transformations here.

Thanks!

>> Changes since v3[1]
>> 
>>  * Add a "coccicheck-test" target in an early and new patch, the
>>    structure mirrors that of coccinelle.git's own tests. As the
>>    diffstat shows we have a *.c and *.res file which is C code
>>    before/after a *.cocci rule is applied.
>
> I have mixed feelings on this. I'm not opposed per se, and I even like
> the fact that it provides examples of what we expect a rule to do. But I
> worry about a world where the cost of using coccinelle goes up because
> now transformations need to have tests along with them. The idea of
> cocci is to make your life a little simpler and more efficient when
> fixing up the code base (as opposed to perl, etc).

That's fair, and I suppose we could add some really complex tests that
e.g. include the original headers.

But right now these just work on stand-alone *.c files with no includes,
and the runtime is thus trivial. So I thought it was OK to run it along
with the main job.

> But if it comes with a bunch of boilerplate tasks, that might no
> longer be the case. I'm already often on the fence about using cocci
> just because of the hours I've sunk into debugging and puzzling things
> out.
>
> I dunno. Maybe the bar is higher for stuff like this that we expect to
> continue to find problems as time goes on (because it is not "here is a
> transition", but "this is a common and easy mistake to make").
>
> So again, I'm not really opposed to this patch in particular, but I want
> to express caution at the direction we might be going, and at applying
> new rules over-zealously.

Yeah, we definitely need to be more careful with "ongoing" rules like
these, but hopefully between the tests & lack of false positives this
one looks sane...

>>  * We now catch init/reset patterns as well as init/release fully
>>    (i.e. for the "struct strbuf" early on)
>
> Yeah, that makes sense.
>
> -Peff