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@xxxxxxxxx/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> --- 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(¬e); - 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
