{"thread":{"id":"57903","subject":"[PATCH] cocci: add and apply a rule to find \"unused\" variables","startedAt":"2022-05-20T11:57:29Z","lastAt":"2022-07-11T11:25:48Z","messageCount":24,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Jeff King","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"455564","messageId":"patch-1.1-7d90f26b73f-20220520T115426Z-avarab@gmail.com","threadId":"57903","inReplyTo":null,"subject":"[PATCH] cocci: add and apply a rule to find \"unused\" variables","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-20T11:57:05Z","receivedAt":"2022-05-20T11:57:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a coccinelle rule to remove variable initialization followed by\ncalling a \"release\" function. This rule automatically finds the sort\nof issue patched in[1], and more.\n\nWe happened to only have occurrences of strbuf_release() matching this\nrule, but manual testing reveals that it'll find e.g. the same pattern\nif \"string_list_clear()\" were used instead.\n\n1. https://lore.kernel.org/git/042d624b8159364229e95d35e9309f12b67f8173.1652977582.git.gitgitgadget@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nThis overlaps but cleanly merges with the small series that [1] is in,\nbut just adding a coccinelle rule to catch this seemed like a good\nthing to have. We even have another such case in builtin/merge.c, near\nthe change made in [1]!\n\nFWIW I wrote this working rule too, but there were no in-tree hits for\nit, so I didn't include it:\n\n\t// Unused declaration + malloc + free()\n\t@@\n\tidentifier I;\n\ttype T;\n\t// malloc(), xmalloc(), calloc() etc.\n\tidentifier MALLOC =~ \"^x?[mc]alloc$\";\n\t@@\n\n\t(\n\t- T *I;\n\t ... when != I\n\t- I = MALLOC(...);\n\t|\n\t- T *I = MALLOC(...);\n\t)\n\t ... when != I\n\t- free(I);\n\n builtin/fetch.c | 3 +--\n builtin/merge.c | 4 ----\n builtin/repack.c | 2 --\n contrib/coccinelle/unused.cocci | 15 +++++++++++++++\n diff.c | 2 --\n 5 files changed, 16 insertions(+), 10 deletions(-)\n create mode 100644 contrib/coccinelle/unused.cocci\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e3791f09ed5..600c28fdb75 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1113,7 +1113,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t struct fetch_head *fetch_head, struct worktree **worktrees)\n {\n \tint url_len, i, rc = 0;\n-\tstruct strbuf note = STRBUF_INIT, err = STRBUF_INIT;\n+\tstruct strbuf note = STRBUF_INIT;\n \tconst char *what, *kind;\n \tstruct ref *rm;\n \tchar *url;\n@@ -1281,7 +1281,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n abort:\n \tstrbuf_release(¬e);\n-\tstrbuf_release(&err);\n \tfree(url);\n \treturn rc;\n }\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex f178f5a3ee1..bb6b0580659 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -375,7 +375,6 @@ static void reset_hard(const struct object_id *oid, int verbose)\n static void restore_state(const struct object_id *head,\n \t\t\t const struct object_id *stash)\n {\n-\tstruct strbuf sb = STRBUF_INIT;\n \tconst char *args[] = { \"stash\", \"apply\", NULL, NULL };\n \n \tif (is_null_oid(stash))\n@@ -391,7 +390,6 @@ static void restore_state(const struct object_id *head,\n \t */\n \trun_command_v_opt(args, RUN_GIT_CMD);\n \n-\tstrbuf_release(&sb);\n \trefresh_cache(REFRESH_QUIET);\n }\n \n@@ -501,7 +499,6 @@ static void merge_name(const char *remote, struct strbuf *msg)\n {\n \tstruct commit *remote_head;\n \tstruct object_id branch_head;\n-\tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf bname = STRBUF_INIT;\n \tstruct merge_remote_desc *desc;\n \tconst char *ptr;\n@@ -589,7 +586,6 @@ static void merge_name(const char *remote, struct strbuf *msg)\n \t\toid_to_hex(&remote_head->object.oid), remote);\n cleanup:\n \tfree(found_ref);\n-\tstrbuf_release(&buf);\n \tstrbuf_release(&bname);\n }\n \ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex d1a563d5b65..52f8450f1be 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -609,7 +609,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct string_list_item *item;\n \tstruct string_list names = STRING_LIST_INIT_DUP;\n-\tstruct string_list rollback = STRING_LIST_INIT_NODUP;\n \tstruct string_list existing_nonkept_packs = STRING_LIST_INIT_DUP;\n \tstruct string_list existing_kept_packs = STRING_LIST_INIT_DUP;\n \tstruct pack_geometry *geometry = NULL;\n@@ -955,7 +954,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t}\n \n \tstring_list_clear(&names, 0);\n-\tstring_list_clear(&rollback, 0);\n \tstring_list_clear(&existing_nonkept_packs, 0);\n \tstring_list_clear(&existing_kept_packs, 0);\n \tclear_pack_geometry(geometry);\ndiff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci\nnew file mode 100644\nindex 00000000000..52c23e15310\n--- /dev/null\n+++ b/contrib/coccinelle/unused.cocci\n@@ -0,0 +1,15 @@\n+// Unused init assignment + release()\n+@@\n+identifier I;\n+type T;\n+constant INIT =~ \"_INIT\";\n+// stbuf_release(), string_list_clear() etc.\n+identifier REL1 =~ \"^[a-z_]*_(release|clear|free)$\";\n+// release_patch(), clear_pathspec() etc.\n+identifier REL2 =~ \"^(release|clear|free)_[a-z_]*$\";\n+@@\n+\n+- T I = INIT;\n+ <+... when != \\( I \\| &I \\)\n+- \\( REL1 \\| REL2 \\)(&I, ...);\n+ ...+>\ndiff --git a/diff.c b/diff.c\nindex ef7159968b6..57997937071 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1289,7 +1289,6 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n {\n \tstatic const char *nneof = \" No newline at end of file\\n\";\n \tconst char *context, *reset, *set, *set_sign, *meta, *fraginfo;\n-\tstruct strbuf sb = STRBUF_INIT;\n \n \tenum diff_symbol s = eds->s;\n \tconst char *line = eds->line;\n@@ -1521,7 +1520,6 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \tdefault:\n \t\tBUG(\"unknown diff symbol\");\n \t}\n-\tstrbuf_release(&sb);\n }\n \n static void emit_diff_symbol(struct diff_options *o, enum diff_symbol s,\n-- \n2.36.1.960.g7a4e2fc85c9\n\n"},{"id":"457664","messageId":"cover-v2-0.2-00000000000-20220621T223954Z-avarab@gmail.com","threadId":"57903","inReplyTo":"patch-1.1-7d90f26b73f-20220520T115426Z-avarab@gmail.com","subject":"[PATCH v2 0/2] add and apply a rule to find \"unused\" init+free","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-21T22:44:01Z","receivedAt":"2022-06-21T22:44:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This re-roll of [1] gets to the same end-state as far as the changed C\ncode is concerned, but I rewrote the coccinelle rule to be much more\ngeneral, based on the more recent discussion at [2].\n\nSee the \"contrib/coccinelle/unused.cocci\" part of 1/2 for extensive\ncommentary and what the rule does & doesn't spot, which I won't repeat\nhere.\n\nThe 2/2 is then split up to show the effect that \"when strict\" has on\nthis.\n\nI noted in [2] that I had a WIP rule to to remove that unused\n\"get_worktrees()\" but couldn't figure out a bug, this coccinelle code\nwill properly remove that sort of code, but only if it's actually\nunused.\n\n1. https://lore.kernel.org/git/patch-1.1-7d90f26b73f-20220520T115426Z-avarab@gmail.com/\n2. https://lore.kernel.org/git/220620.865ykvw2l4.gmgdl@evledraar.gmail.com/\n\nÆvar Arnfjörð Bjarmason (2):\n cocci: add and apply a rule to find \"unused\" variables\n cocci: remove \"when strict\" from unused.cocci\n\n builtin/fetch.c | 3 +-\n builtin/merge.c | 4 ---\n builtin/repack.c | 2 --\n contrib/coccinelle/unused.cocci | 64 +++++++++++++++++++++++++++++++++\n contrib/scalar/scalar.c | 3 +-\n diff.c | 2 --\n 6 files changed, 66 insertions(+), 12 deletions(-)\n create mode 100644 contrib/coccinelle/unused.cocci\n\nRange-diff against v1:\n1: 7d90f26b73f < -: ----------- cocci: add and apply a rule to find \"unused\" variables\n-: ----------- > 1: d14036521ab cocci: add and apply a rule to find \"unused\" variables\n-: ----------- > 2: 4130dc15287 cocci: remove \"when strict\" from unused.cocci\n-- \n2.36.1.1239.gfba91521d90\n\n"},{"id":"457665","messageId":"patch-v2-1.2-d14036521ab-20220621T223954Z-avarab@gmail.com","threadId":"57903","inReplyTo":"cover-v2-0.2-00000000000-20220621T223954Z-avarab@gmail.com","subject":"[PATCH v2 1/2] cocci: add and apply a rule to find \"unused\" variables","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-21T22:44:02Z","receivedAt":"2022-06-21T22:44:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a coccinelle rule to remove variable initialization followed by\ncalling a \"release\" function. See extensive commentary in the new\n\"unused.cocci\" for how it works, and what it's intended to find and\nreplace.\n\nThe inclusion of \"contrib/scalar/scalar.c\" is because \"spatch\" was\nmanually run on it (we don't usually run spatch on contrib).\n\nThe use of \"with strict\" here will be explained and amended in the\nfollowing commit.\n\n1. https://lore.kernel.org/git/042d624b8159364229e95d35e9309f12b67f8173.1652977582.git.gitgitgadget@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/fetch.c | 3 +-\n builtin/merge.c | 2 -\n contrib/coccinelle/unused.cocci | 66 +++++++++++++++++++++++++++++++++\n contrib/scalar/scalar.c | 3 +-\n diff.c | 2 -\n 5 files changed, 68 insertions(+), 8 deletions(-)\n create mode 100644 contrib/coccinelle/unused.cocci\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ac29c2b1ae3..8a3ae71fed0 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1113,7 +1113,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t struct fetch_head *fetch_head, struct worktree **worktrees)\n {\n \tint url_len, i, rc = 0;\n-\tstruct strbuf note = STRBUF_INIT, err = STRBUF_INIT;\n+\tstruct strbuf note = STRBUF_INIT;\n \tconst char *what, *kind;\n \tstruct ref *rm;\n \tchar *url;\n@@ -1281,7 +1281,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n abort:\n \tstrbuf_release(¬e);\n-\tstrbuf_release(&err);\n \tfree(url);\n \treturn rc;\n }\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex d9784d4891c..bbd70b17bc6 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -502,7 +502,6 @@ static void merge_name(const char *remote, struct strbuf *msg)\n {\n \tstruct commit *remote_head;\n \tstruct object_id branch_head;\n-\tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf bname = STRBUF_INIT;\n \tstruct merge_remote_desc *desc;\n \tconst char *ptr;\n@@ -590,7 +589,6 @@ static void merge_name(const char *remote, struct strbuf *msg)\n \t\toid_to_hex(&remote_head->object.oid), remote);\n cleanup:\n \tfree(found_ref);\n-\tstrbuf_release(&buf);\n \tstrbuf_release(&bname);\n }\n \ndiff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci\nnew file mode 100644\nindex 00000000000..45452f8979a\n--- /dev/null\n+++ b/contrib/coccinelle/unused.cocci\n@@ -0,0 +1,66 @@\n+// This rule finds sequences of \"unused\" declerations, init and\n+// release(). E.g.:\n+//\n+//\tstruct strbuf buf = STRBUF_INIT;\n+// [.. no other use of \"buf\" in the function ..]\n+//\tstrbuf_release(&buf)\n+//\n+// To do do this we find (continued below)...\n+@@\n+type T;\n+identifier I;\n+// STRBUF_INIT, but also e.g. STRING_LIST_INIT_DUP (so no anchoring)\n+constant INIT =~ \"_INIT\";\n+// I = get_worktrees() etc.\n+identifier INIT_ASSIGN1 =~ \"^get_worktrees$\";\n+// strbuf_init(&I, ...) etc.\n+identifier INIT_CALL1 =~ \"^[a-z_]*_init$\";\n+// stbuf_release(), string_list_clear() etc.\n+identifier REL1 =~ \"^[a-z_]*_(release|clear|free)$\";\n+// release_patch(), clear_pathspec() etc.\n+identifier REL2 =~ \"^(release|clear|free)_[a-z_]*$\";\n+@@\n+\n+// .. A declaration like \"struct strbuf buf;\"...\n+(\n+- T I;\n+// ... or \"struct STRBUF buf = STRBUF_INIT;\" ...\n+|\n+- T I = INIT;\n+)\n+\n+// ... Optionally followed by lines that make no use of \"buf\", \"&buf\"\n+// etc., but which ...\n+<... when != \\( I \\| &I \\)\n+ when strict\n+// .. (only) make use of \"buf\" or \"&buf\" to call something like\n+// \"strbuf_init(&buf, ...)\" ...\n+(\n+- \\( INIT_CALL1 \\)( \\( I \\| &I \\), ...);\n+|\n+// .. or e.g. \"worktrees = get_worktrees();\", i.e. a known \"assignment\n+// init\" ...\n+- I = \\( INIT_ASSIGN1 \\)(...);\n+)\n+...>\n+\n+// ... and then no mention of \"buf\" or \"&buf\" until we get to a\n+// strbuf_release(&buf) at the end ...\n+(\n+- \\( REL1 \\| REL2 \\)( \\( I \\| &I \\), ...);\n+|\n+- \\( REL1 \\| REL2 \\)( \\( &I \\| I \\) );\n+)\n+// ... and no use *after* either, e.g. we don't want to delete\n+// init/strbuf_release() patterns, where \"&buf\" could be used\n+// afterwards.\n+ ... when != \\( I \\| &I \\)\n+ when strict\n+// Note that we're intentionally loose in accepting e.g. a\n+// \"strbuf_init(&buf)\" followed by a \"string_list_clear(&buf,\n+// 0)\". It's assumed that the compiler will catch any such invalid\n+// code, i.e. that our constructors/destructors don't take a \"void *\".\n+//\n+// This rule also isn't capable of finding cases where &buf is used,\n+// but only to e.g. pass that variable to a static function which\n+// doesn't use it. The analysis is only function-local.\ndiff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c\nindex 28176914e57..97e71fe19cd 100644\n--- a/contrib/scalar/scalar.c\n+++ b/contrib/scalar/scalar.c\n@@ -687,7 +687,7 @@ static int cmd_diagnose(int argc, const char **argv)\n \tint stdout_fd = -1, archiver_fd = -1;\n \ttime_t now = time(NULL);\n \tstruct tm tm;\n-\tstruct strbuf path = STRBUF_INIT, buf = STRBUF_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n \tint res = 0;\n \n \targc = parse_options(argc, argv, NULL, options,\n@@ -779,7 +779,6 @@ static int cmd_diagnose(int argc, const char **argv)\n \tfree(argv_copy);\n \tstrvec_clear(&archiver_args);\n \tstrbuf_release(&zip_path);\n-\tstrbuf_release(&path);\n \tstrbuf_release(&buf);\n \n \treturn res;\ndiff --git a/diff.c b/diff.c\nindex e71cf758861..d4290615aaa 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1289,7 +1289,6 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n {\n \tstatic const char *nneof = \" No newline at end of file\\n\";\n \tconst char *context, *reset, *set, *set_sign, *meta, *fraginfo;\n-\tstruct strbuf sb = STRBUF_INIT;\n \n \tenum diff_symbol s = eds->s;\n \tconst char *line = eds->line;\n@@ -1521,7 +1520,6 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \tdefault:\n \t\tBUG(\"unknown diff symbol\");\n \t}\n-\tstrbuf_release(&sb);\n }\n \n static void emit_diff_symbol(struct diff_options *o, enum diff_symbol s,\n-- \n2.36.1.1239.gfba91521d90\n\n"},{"id":"457666","messageId":"patch-v2-2.2-4130dc15287-20220621T223954Z-avarab@gmail.com","threadId":"57903","inReplyTo":"cover-v2-0.2-00000000000-20220621T223954Z-avarab@gmail.com","subject":"[PATCH v2 2/2] cocci: remove \"when strict\" from unused.cocci","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-21T22:44:03Z","receivedAt":"2022-06-21T22:44:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the \"when strict\" constraint from the newly introduced rule to\nfind unused code. As seen in the change this will help us find cases\nwhere a \"return\" was causing spatch in the middle of our match was\ncausing spatch to abort its analysis.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/merge.c | 2 --\n builtin/repack.c | 2 --\n contrib/coccinelle/unused.cocci | 2 --\n 3 files changed, 6 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex bbd70b17bc6..23170f2d2a6 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -375,7 +375,6 @@ static void reset_hard(const struct object_id *oid, int verbose)\n static void restore_state(const struct object_id *head,\n \t\t\t const struct object_id *stash)\n {\n-\tstruct strbuf sb = STRBUF_INIT;\n \tconst char *args[] = { \"stash\", \"apply\", NULL, NULL };\n \n \tif (is_null_oid(stash))\n@@ -391,7 +390,6 @@ static void restore_state(const struct object_id *head,\n \t */\n \trun_command_v_opt(args, RUN_GIT_CMD);\n \n-\tstrbuf_release(&sb);\n \trefresh_cache(REFRESH_QUIET);\n }\n \ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 4a7ae4cf489..482b66f57d6 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -727,7 +727,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct string_list_item *item;\n \tstruct string_list names = STRING_LIST_INIT_DUP;\n-\tstruct string_list rollback = STRING_LIST_INIT_NODUP;\n \tstruct string_list existing_nonkept_packs = STRING_LIST_INIT_DUP;\n \tstruct string_list existing_kept_packs = STRING_LIST_INIT_DUP;\n \tstruct pack_geometry *geometry = NULL;\n@@ -1117,7 +1116,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t}\n \n \tstring_list_clear(&names, 0);\n-\tstring_list_clear(&rollback, 0);\n \tstring_list_clear(&existing_nonkept_packs, 0);\n \tstring_list_clear(&existing_kept_packs, 0);\n \tclear_pack_geometry(geometry);\ndiff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci\nindex 45452f8979a..6eaea01380b 100644\n--- a/contrib/coccinelle/unused.cocci\n+++ b/contrib/coccinelle/unused.cocci\n@@ -32,7 +32,6 @@ identifier REL2 =~ \"^(release|clear|free)_[a-z_]*$\";\n // ... Optionally followed by lines that make no use of \"buf\", \"&buf\"\n // etc., but which ...\n <... when != \\( I \\| &I \\)\n- when strict\n // .. (only) make use of \"buf\" or \"&buf\" to call something like\n // \"strbuf_init(&buf, ...)\" ...\n (\n@@ -55,7 +54,6 @@ identifier REL2 =~ \"^(release|clear|free)_[a-z_]*$\";\n // init/strbuf_release() patterns, where \"&buf\" could be used\n // afterwards.\n ... when != \\( I \\| &I \\)\n- when strict\n // Note that we're intentionally loose in accepting e.g. a\n // \"strbuf_init(&buf)\" followed by a \"string_list_clear(&buf,\n // 0)\". It's assumed that the compiler will catch any such invalid\n-- \n2.36.1.1239.gfba91521d90\n\n"},{"id":"457718","messageId":"xmqqsfnw65zu.fsf@gitster.g","threadId":"57903","inReplyTo":"patch-v2-1.2-d14036521ab-20220621T223954Z-avarab@gmail.com","subject":"Re: [PATCH v2 1/2] cocci: add and apply a rule to find \"unused\" variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-22T16:02:45Z","receivedAt":"2022-06-22T16:02:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> +identifier INIT_ASSIGN1 =~ \"^get_worktrees$\";\n> +// strbuf_init(&I, ...) etc.\n> +identifier INIT_CALL1 =~ \"^[a-z_]*_init$\";\n> +// stbuf_release(), string_list_clear() etc.\n\nstrbuf?\n\n> +identifier REL1 =~ \"^[a-z_]*_(release|clear|free)$\";\n> +// release_patch(), clear_pathspec() etc.\n> +identifier REL2 =~ \"^(release|clear|free)_[a-z_]*$\";\n> +@@\n\nI am hesitant to see this broad set of patterns that could match\ninit/release functions (and possible false positive matches).\n\nEspecially given that it ended up finding only 4 instances, all of\nthe same \"STRBUF_INIT\" followed by \"strbuf_release()\", which means\nthat all other possible matches, when they actually are found, will\nbe seen by developers who are not necessarily familiar with these\nrules before they are inspected by those who are for correctness.\n\nIt would be nice to have a step that catch only strbuf_init(),\nSTRBUF_INIT, strbuf_release(), and nothing else, possibly with\nanother step with concrete function names, with other \"presumably\nfunctions whose name match this loose pattern are all release\nfunctions\" patterns in a separate follow-up patch so that the last\none can easily be reverted.\n\n> +// .. A declaration like \"struct strbuf buf;\"...\n> +(\n> +- T I;\n> +// ... or \"struct STRBUF buf = STRBUF_INIT;\" ...\n> +|\n> +- T I = INIT;\n> +)\n\nPresumably, if either of the above followed by foo_release(I) should\nbe caught, then we should catch \"T I = { 0 };\" followed by a release\nas well. Initialization \"T I = { 1, };\" for a type without _INIT\nmacro is also the same story.\n\nGiven that, do we even need to limit the forms of declaration? The\nonly thing we care about is that I is new in this scope, and I is\nnot used otherwise, in a way other than (1) calling _init() function\non it, or (2) calling _release() function on it, before leaving the\nscope, right?\n\nThanks.\n"},{"id":"458351","messageId":"cover-v3-0.4-00000000000-20220701T102506Z-avarab@gmail.com","threadId":"57903","inReplyTo":"cover-v2-0.2-00000000000-20220621T223954Z-avarab@gmail.com","subject":"[PATCH v3 0/4] add and apply a rule to find \"unused\" init+free","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-01T10:30:55Z","receivedAt":"2022-07-01T10:31:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series adds a coccinelle rule to find and remove code where the\nonly reference to a variable in a given function is to malloc() &\nfree() it, where \"malloc\" and \"free\" also match\n\"strbuf_init/strbuf_release\", and then later in the series anything\nthat looks like a init/free pattern.\n\nChanges since v2:\n\n * Make the wider rule revert-able, as requested by Junio in\n https://lore.kernel.org/git/xmqqsfnw65zu.fsf@gitster.g/\n\n * We now find and remove \"malloc\" followed by an optional \"init\" and\n \"release\".\n\n * We now match { 0 } initializers, in addition to things that look\n like \"INIT\" macros.\n\nÆvar Arnfjörð Bjarmason (4):\n cocci: add and apply a rule to find \"unused\" strbufs\n cocci: catch unused \"strbuf\" using an xmalloc() pattern\n cocci: remove \"when strict\" from unused.cocci\n cocci: generalize \"unused\" rule to cover more than \"strbuf\"\n\n builtin/fetch.c | 3 +-\n builtin/merge.c | 4 --\n builtin/repack.c | 2 -\n contrib/coccinelle/unused.cocci | 88 +++++++++++++++++++++++++++++++++\n contrib/scalar/scalar.c | 3 +-\n diff.c | 2 -\n 6 files changed, 90 insertions(+), 12 deletions(-)\n create mode 100644 contrib/coccinelle/unused.cocci\n\nRange-diff against v2:\n1: d14036521ab ! 1: 49e9ccb5819 cocci: add and apply a rule to find \"unused\" variables\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - cocci: add and apply a rule to find \"unused\" variables\n + cocci: add and apply a rule to find \"unused\" strbufs\n \n - Add a coccinelle rule to remove variable initialization followed by\n - calling a \"release\" function. See extensive commentary in the new\n - \"unused.cocci\" for how it works, and what it's intended to find and\n - replace.\n + Add a coccinelle rule to remove \"struct strbuf\" initialization\n + followed by calling \"strbuf_release()\" function.\n +\n + See extensive commentary in the new \"unused.cocci\" for how it works,\n + and what it's intended to find and replace.\n \n The inclusion of \"contrib/scalar/scalar.c\" is because \"spatch\" was\n manually run on it (we don't usually run spatch on contrib).\n @@ Commit message\n The use of \"with strict\" here will be explained and amended in the\n following commit.\n \n - 1. https://lore.kernel.org/git/042d624b8159364229e95d35e9309f12b67f8173.1652977582.git.gitgitgadget@gmail.com/\n -\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## builtin/fetch.c ##\n @@ builtin/merge.c: static void merge_name(const char *remote, struct strbuf *msg)\n \n ## contrib/coccinelle/unused.cocci (new) ##\n @@\n -+// This rule finds sequences of \"unused\" declerations, init and\n -+// release(). E.g.:\n ++// This rule finds sequences of \"unused\" declerations and uses of\n ++// \"struct strbuf\".\n ++//\n ++// I.e. this finds cases where we only declare the variable, and then\n ++// release it, e.g.:\n +//\n +//\tstruct strbuf buf = STRBUF_INIT;\n +// [.. no other use of \"buf\" in the function ..]\n +//\tstrbuf_release(&buf)\n +//\n ++// Or:\n ++//\n ++//\tstruct strbuf buf;\n ++//\t[.. no other use of \"buf\" in the function ..]\n ++//\tstrbuf_init(&buf, 0);\n ++//\t[.. no other use of \"buf\" in the function ..]\n ++//\tstrbuf_release(&buf)\n ++//\n +// To do do this we find (continued below)...\n +@@\n +type T;\n +identifier I;\n -+// STRBUF_INIT, but also e.g. STRING_LIST_INIT_DUP (so no anchoring)\n -+constant INIT =~ \"_INIT\";\n -+// I = get_worktrees() etc.\n -+identifier INIT_ASSIGN1 =~ \"^get_worktrees$\";\n ++// STRBUF_INIT\n ++constant INIT_MACRO =~ \"^STRBUF_INIT$\";\n +// strbuf_init(&I, ...) etc.\n -+identifier INIT_CALL1 =~ \"^[a-z_]*_init$\";\n -+// stbuf_release(), string_list_clear() etc.\n -+identifier REL1 =~ \"^[a-z_]*_(release|clear|free)$\";\n -+// release_patch(), clear_pathspec() etc.\n -+identifier REL2 =~ \"^(release|clear|free)_[a-z_]*$\";\n ++identifier INIT_CALL1 =~ \"^strbuf_init$\";\n ++// strbuf_release()\n ++identifier REL1 =~ \"^strbuf_release$\";\n +@@\n +\n +// .. A declaration like \"struct strbuf buf;\"...\n +(\n +- T I;\n ++// ... or \"struct strbuf buf = { 0 };\" ...\n ++|\n ++- T I = { 0 };\n +// ... or \"struct STRBUF buf = STRBUF_INIT;\" ...\n +|\n -+- T I = INIT;\n ++- T I = INIT_MACRO;\n +)\n +\n +// ... Optionally followed by lines that make no use of \"buf\", \"&buf\"\n @@ contrib/coccinelle/unused.cocci (new)\n + when strict\n +// .. (only) make use of \"buf\" or \"&buf\" to call something like\n +// \"strbuf_init(&buf, ...)\" ...\n -+(\n +- \\( INIT_CALL1 \\)( \\( I \\| &I \\), ...);\n -+|\n -+// .. or e.g. \"worktrees = get_worktrees();\", i.e. a known \"assignment\n -+// init\" ...\n -+- I = \\( INIT_ASSIGN1 \\)(...);\n -+)\n +...>\n +\n +// ... and then no mention of \"buf\" or \"&buf\" until we get to a\n +// strbuf_release(&buf) at the end ...\n -+(\n -+- \\( REL1 \\| REL2 \\)( \\( I \\| &I \\), ...);\n -+|\n -+- \\( REL1 \\| REL2 \\)( \\( &I \\| I \\) );\n -+)\n ++- \\( REL1 \\)( \\( &I \\| I \\) );\n +// ... and no use *after* either, e.g. we don't want to delete\n +// init/strbuf_release() patterns, where \"&buf\" could be used\n +// afterwards.\n + ... when != \\( I \\| &I \\)\n + when strict\n -+// Note that we're intentionally loose in accepting e.g. a\n -+// \"strbuf_init(&buf)\" followed by a \"string_list_clear(&buf,\n -+// 0)\". It's assumed that the compiler will catch any such invalid\n -+// code, i.e. that our constructors/destructors don't take a \"void *\".\n -+//\n +// This rule also isn't capable of finding cases where &buf is used,\n +// but only to e.g. pass that variable to a static function which\n +// doesn't use it. The analysis is only function-local.\n-: ----------- > 2: 6324d3956ed cocci: catch unused \"strbuf\" using an xmalloc() pattern\n2: 4130dc15287 ! 3: 9a5e7208dec cocci: remove \"when strict\" from unused.cocci\n @@ builtin/merge.c: static void restore_state(const struct object_id *head,\n }\n \n \n - ## builtin/repack.c ##\n -@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)\n - \tstruct child_process cmd = CHILD_PROCESS_INIT;\n - \tstruct string_list_item *item;\n - \tstruct string_list names = STRING_LIST_INIT_DUP;\n --\tstruct string_list rollback = STRING_LIST_INIT_NODUP;\n - \tstruct string_list existing_nonkept_packs = STRING_LIST_INIT_DUP;\n - \tstruct string_list existing_kept_packs = STRING_LIST_INIT_DUP;\n - \tstruct pack_geometry *geometry = NULL;\n -@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)\n - \t}\n - \n - \tstring_list_clear(&names, 0);\n --\tstring_list_clear(&rollback, 0);\n - \tstring_list_clear(&existing_nonkept_packs, 0);\n - \tstring_list_clear(&existing_kept_packs, 0);\n - \tclear_pack_geometry(geometry);\n -\n ## contrib/coccinelle/unused.cocci ##\n -@@ contrib/coccinelle/unused.cocci: identifier REL2 =~ \"^(release|clear|free)_[a-z_]*$\";\n +@@ contrib/coccinelle/unused.cocci: identifier REL1 =~ \"^strbuf_release$\";\n // ... Optionally followed by lines that make no use of \"buf\", \"&buf\"\n // etc., but which ...\n <... when != \\( I \\| &I \\)\n - when strict\n + (\n // .. (only) make use of \"buf\" or \"&buf\" to call something like\n // \"strbuf_init(&buf, ...)\" ...\n - (\n -@@ contrib/coccinelle/unused.cocci: identifier REL2 =~ \"^(release|clear|free)_[a-z_]*$\";\n +@@ contrib/coccinelle/unused.cocci: identifier REL1 =~ \"^strbuf_release$\";\n // init/strbuf_release() patterns, where \"&buf\" could be used\n // afterwards.\n ... when != \\( I \\| &I \\)\n - when strict\n - // Note that we're intentionally loose in accepting e.g. a\n - // \"strbuf_init(&buf)\" followed by a \"string_list_clear(&buf,\n - // 0)\". It's assumed that the compiler will catch any such invalid\n + // This rule also isn't capable of finding cases where &buf is used,\n + // but only to e.g. pass that variable to a static function which\n + // doesn't use it. The analysis is only function-local.\n-: ----------- > 4: 45a429b9cc9 cocci: generalize \"unused\" rule to cover more than \"strbuf\"\n-- \n2.37.0.900.g4d0de1cceb2\n\n"},{"id":"458352","messageId":"patch-v3-1.4-49e9ccb5819-20220701T102506Z-avarab@gmail.com","threadId":"57903","inReplyTo":"cover-v3-0.4-00000000000-20220701T102506Z-avarab@gmail.com","subject":"[PATCH v3 1/4] cocci: add and apply a rule to find \"unused\" strbufs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-01T10:30:56Z","receivedAt":"2022-07-01T10:31:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a coccinelle rule to remove \"struct strbuf\" initialization\nfollowed by calling \"strbuf_release()\" function.\n\nSee extensive commentary in the new \"unused.cocci\" for how it works,\nand what it's intended to find and replace.\n\nThe inclusion of \"contrib/scalar/scalar.c\" is because \"spatch\" was\nmanually run on it (we don't usually run spatch on contrib).\n\nThe use of \"with strict\" here will be explained and amended in the\nfollowing commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/fetch.c | 3 +-\n builtin/merge.c | 2 --\n contrib/coccinelle/unused.cocci | 61 +++++++++++++++++++++++++++++++++\n contrib/scalar/scalar.c | 3 +-\n diff.c | 2 --\n 5 files changed, 63 insertions(+), 8 deletions(-)\n create mode 100644 contrib/coccinelle/unused.cocci\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ac29c2b1ae3..8a3ae71fed0 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1113,7 +1113,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t struct fetch_head *fetch_head, struct worktree **worktrees)\n {\n \tint url_len, i, rc = 0;\n-\tstruct strbuf note = STRBUF_INIT, err = STRBUF_INIT;\n+\tstruct strbuf note = STRBUF_INIT;\n \tconst char *what, *kind;\n \tstruct ref *rm;\n \tchar *url;\n@@ -1281,7 +1281,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n abort:\n \tstrbuf_release(¬e);\n-\tstrbuf_release(&err);\n \tfree(url);\n \treturn rc;\n }\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex d9784d4891c..bbd70b17bc6 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -502,7 +502,6 @@ static void merge_name(const char *remote, struct strbuf *msg)\n {\n \tstruct commit *remote_head;\n \tstruct object_id branch_head;\n-\tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf bname = STRBUF_INIT;\n \tstruct merge_remote_desc *desc;\n \tconst char *ptr;\n@@ -590,7 +589,6 @@ static void merge_name(const char *remote, struct strbuf *msg)\n \t\toid_to_hex(&remote_head->object.oid), remote);\n cleanup:\n \tfree(found_ref);\n-\tstrbuf_release(&buf);\n \tstrbuf_release(&bname);\n }\n \ndiff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci\nnew file mode 100644\nindex 00000000000..bc26d39b313\n--- /dev/null\n+++ b/contrib/coccinelle/unused.cocci\n@@ -0,0 +1,61 @@\n+// This rule finds sequences of \"unused\" declerations and uses of\n+// \"struct strbuf\".\n+//\n+// I.e. this finds cases where we only declare the variable, and then\n+// release it, e.g.:\n+//\n+//\tstruct strbuf buf = STRBUF_INIT;\n+// [.. no other use of \"buf\" in the function ..]\n+//\tstrbuf_release(&buf)\n+//\n+// Or:\n+//\n+//\tstruct strbuf buf;\n+//\t[.. no other use of \"buf\" in the function ..]\n+//\tstrbuf_init(&buf, 0);\n+//\t[.. no other use of \"buf\" in the function ..]\n+//\tstrbuf_release(&buf)\n+//\n+// To do do this we find (continued below)...\n+@@\n+type T;\n+identifier I;\n+// STRBUF_INIT\n+constant INIT_MACRO =~ \"^STRBUF_INIT$\";\n+// strbuf_init(&I, ...) etc.\n+identifier INIT_CALL1 =~ \"^strbuf_init$\";\n+// strbuf_release()\n+identifier REL1 =~ \"^strbuf_release$\";\n+@@\n+\n+// .. A declaration like \"struct strbuf buf;\"...\n+(\n+- T I;\n+// ... or \"struct strbuf buf = { 0 };\" ...\n+|\n+- T I = { 0 };\n+// ... or \"struct STRBUF buf = STRBUF_INIT;\" ...\n+|\n+- T I = INIT_MACRO;\n+)\n+\n+// ... Optionally followed by lines that make no use of \"buf\", \"&buf\"\n+// etc., but which ...\n+<... when != \\( I \\| &I \\)\n+ when strict\n+// .. (only) make use of \"buf\" or \"&buf\" to call something like\n+// \"strbuf_init(&buf, ...)\" ...\n+- \\( INIT_CALL1 \\)( \\( I \\| &I \\), ...);\n+...>\n+\n+// ... and then no mention of \"buf\" or \"&buf\" until we get to a\n+// strbuf_release(&buf) at the end ...\n+- \\( REL1 \\)( \\( &I \\| I \\) );\n+// ... and no use *after* either, e.g. we don't want to delete\n+// init/strbuf_release() patterns, where \"&buf\" could be used\n+// afterwards.\n+ ... when != \\( I \\| &I \\)\n+ when strict\n+// This rule also isn't capable of finding cases where &buf is used,\n+// but only to e.g. pass that variable to a static function which\n+// doesn't use it. The analysis is only function-local.\ndiff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c\nindex 28176914e57..97e71fe19cd 100644\n--- a/contrib/scalar/scalar.c\n+++ b/contrib/scalar/scalar.c\n@@ -687,7 +687,7 @@ static int cmd_diagnose(int argc, const char **argv)\n \tint stdout_fd = -1, archiver_fd = -1;\n \ttime_t now = time(NULL);\n \tstruct tm tm;\n-\tstruct strbuf path = STRBUF_INIT, buf = STRBUF_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n \tint res = 0;\n \n \targc = parse_options(argc, argv, NULL, options,\n@@ -779,7 +779,6 @@ static int cmd_diagnose(int argc, const char **argv)\n \tfree(argv_copy);\n \tstrvec_clear(&archiver_args);\n \tstrbuf_release(&zip_path);\n-\tstrbuf_release(&path);\n \tstrbuf_release(&buf);\n \n \treturn res;\ndiff --git a/diff.c b/diff.c\nindex e71cf758861..d4290615aaa 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1289,7 +1289,6 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n {\n \tstatic const char *nneof = \" No newline at end of file\\n\";\n \tconst char *context, *reset, *set, *set_sign, *meta, *fraginfo;\n-\tstruct strbuf sb = STRBUF_INIT;\n \n \tenum diff_symbol s = eds->s;\n \tconst char *line = eds->line;\n@@ -1521,7 +1520,6 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \tdefault:\n \t\tBUG(\"unknown diff symbol\");\n \t}\n-\tstrbuf_release(&sb);\n }\n \n static void emit_diff_symbol(struct diff_options *o, enum diff_symbol s,\n-- \n2.37.0.900.g4d0de1cceb2\n\n"},{"id":"458353","messageId":"patch-v3-2.4-6324d3956ed-20220701T102506Z-avarab@gmail.com","threadId":"57903","inReplyTo":"cover-v3-0.4-00000000000-20220701T102506Z-avarab@gmail.com","subject":"[PATCH v3 2/4] cocci: catch unused \"strbuf\" using an xmalloc() pattern","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-01T10:30:57Z","receivedAt":"2022-07-01T10:31:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"There's no current matches for this rule, but it will match both:\n\n\tstruct strbuf *buf = xmalloc(sizeof(struct strbuf));\n\tstrbuf_init(buf, 0);\n\tstrbuf_release(buf);\n\nAnd:\n\n\tstruct strbuf *buf;\n\n\tbuf = xmalloc(sizeof(struct strbuf));\n\tstrbuf_init(buf, 0);\n\tstrbuf_release(buf);\n\nNote that we'd also match a strbuf_init() before the xmalloc(), but\nwe're not seeking to be so strict as to make checks that the compiler\nwill catch for us redundant, and saying we'll match either \"init\" or\n\"xmalloc\" lines makes the rule simpler.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n contrib/coccinelle/unused.cocci | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci\nindex bc26d39b313..43942f3cd4f 100644\n--- a/contrib/coccinelle/unused.cocci\n+++ b/contrib/coccinelle/unused.cocci\n@@ -22,6 +22,8 @@ type T;\n identifier I;\n // STRBUF_INIT\n constant INIT_MACRO =~ \"^STRBUF_INIT$\";\n+// x[mc]alloc() etc.\n+identifier MALLOC1 =~ \"^x?[mc]alloc$\";\n // strbuf_init(&I, ...) etc.\n identifier INIT_CALL1 =~ \"^strbuf_init$\";\n // strbuf_release()\n@@ -37,15 +39,25 @@ identifier REL1 =~ \"^strbuf_release$\";\n // ... or \"struct STRBUF buf = STRBUF_INIT;\" ...\n |\n - T I = INIT_MACRO;\n+|\n+// ... or \"struct strbuf *buf = xmalloc(...)\" etc. ...\n+- T I = MALLOC1(...);\n )\n \n // ... Optionally followed by lines that make no use of \"buf\", \"&buf\"\n // etc., but which ...\n <... when != \\( I \\| &I \\)\n when strict\n+(\n // .. (only) make use of \"buf\" or \"&buf\" to call something like\n // \"strbuf_init(&buf, ...)\" ...\n - \\( INIT_CALL1 \\)( \\( I \\| &I \\), ...);\n+|\n+// .. or to follow-up a \"struct strbuf *buf\" with e.g. \"buf =\n+// xmalloc(...)\" (which may in turn be followed-up by a\n+// \"strbuf_init()\", which we'll match with INIT_CALL1) ...\n+- I = MALLOC1(...);\n+)\n ...>\n \n // ... and then no mention of \"buf\" or \"&buf\" until we get to a\n-- \n2.37.0.900.g4d0de1cceb2\n\n"},{"id":"458354","messageId":"patch-v3-4.4-45a429b9cc9-20220701T102506Z-avarab@gmail.com","threadId":"57903","inReplyTo":"cover-v3-0.4-00000000000-20220701T102506Z-avarab@gmail.com","subject":"[PATCH v3 4/4] cocci: generalize \"unused\" rule to cover more than \"strbuf\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-01T10:30:59Z","receivedAt":"2022-07-01T10:31:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Generalize the newly added \"unused.cocci\" rule to find more than just\n\"struct strbuf\", let's have it find the same unused patterns for\n\"struct string_list\", as well as other code that uses\nsimilar-looking *_{release,clear,free}() and {release,clear,free}_*()\nfunctions.\n\nSee [1] for example of code that would be covered by the\n\"get_worktrees()\" part of this rule. We'd still need work that the\nseries is based on (we were passing \"worktrees\" to a function), but\ncould now do the change in [1] automatically.\n\n1. https://lore.kernel.org/git/Yq6eJFUPPTv%2Fzc0o@coredump.intra.peff.net/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/repack.c | 2 --\n contrib/coccinelle/unused.cocci | 31 ++++++++++++++++++++++++-------\n 2 files changed, 24 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 4a7ae4cf489..482b66f57d6 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -727,7 +727,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct string_list_item *item;\n \tstruct string_list names = STRING_LIST_INIT_DUP;\n-\tstruct string_list rollback = STRING_LIST_INIT_NODUP;\n \tstruct string_list existing_nonkept_packs = STRING_LIST_INIT_DUP;\n \tstruct string_list existing_kept_packs = STRING_LIST_INIT_DUP;\n \tstruct pack_geometry *geometry = NULL;\n@@ -1117,7 +1116,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t}\n \n \tstring_list_clear(&names, 0);\n-\tstring_list_clear(&rollback, 0);\n \tstring_list_clear(&existing_nonkept_packs, 0);\n \tstring_list_clear(&existing_kept_packs, 0);\n \tclear_pack_geometry(geometry);\ndiff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci\nindex 9f0101c1350..a1b09d2d73d 100644\n--- a/contrib/coccinelle/unused.cocci\n+++ b/contrib/coccinelle/unused.cocci\n@@ -1,5 +1,5 @@\n // This rule finds sequences of \"unused\" declerations and uses of\n-// \"struct strbuf\".\n+// \"struct strbuf\" and other common types.\n //\n // I.e. this finds cases where we only declare the variable, and then\n // release it, e.g.:\n@@ -20,14 +20,18 @@\n @@\n type T;\n identifier I;\n-// STRBUF_INIT\n-constant INIT_MACRO =~ \"^STRBUF_INIT$\";\n+// STRBUF_INIT, but also e.g. STRING_LIST_INIT_DUP (so no anchoring)\n+constant INIT_MACRO =~ \"_INIT\";\n // x[mc]alloc() etc.\n identifier MALLOC1 =~ \"^x?[mc]alloc$\";\n+// I = get_worktrees() etc.\n+identifier INIT_ASSIGN1 =~ \"^get_worktrees$\";\n // strbuf_init(&I, ...) etc.\n-identifier INIT_CALL1 =~ \"^strbuf_init$\";\n-// strbuf_release()\n-identifier REL1 =~ \"^strbuf_release$\";\n+identifier INIT_CALL1 =~ \"^[a-z_]*_init$\";\n+// stbuf_release(), string_list_clear() etc.\n+identifier REL1 =~ \"^[a-z_]*_(release|clear|free)$\";\n+// release_patch(), clear_pathspec() etc.\n+identifier REL2 =~ \"^(release|clear|free)_[a-z_]*$\";\n @@\n \n // .. A declaration like \"struct strbuf buf;\"...\n@@ -52,6 +56,10 @@ identifier REL1 =~ \"^strbuf_release$\";\n // \"strbuf_init(&buf, ...)\" ...\n - \\( INIT_CALL1 \\)( \\( I \\| &I \\), ...);\n |\n+// .. or e.g. \"worktrees = get_worktrees();\", i.e. a known \"assignment\n+// init\" ...\n+- I = \\( INIT_ASSIGN1 \\)(...);\n+|\n // .. or to follow-up a \"struct strbuf *buf\" with e.g. \"buf =\n // xmalloc(...)\" (which may in turn be followed-up by a\n // \"strbuf_init()\", which we'll match with INIT_CALL1) ...\n@@ -61,11 +69,20 @@ identifier REL1 =~ \"^strbuf_release$\";\n \n // ... and then no mention of \"buf\" or \"&buf\" until we get to a\n // strbuf_release(&buf) at the end ...\n-- \\( REL1 \\)( \\( &I \\| I \\) );\n+(\n+- \\( REL1 \\| REL2 \\)( \\( I \\| &I \\), ...);\n+|\n+- \\( REL1 \\| REL2 \\)( \\( &I \\| I \\) );\n+)\n // ... and no use *after* either, e.g. we don't want to delete\n // init/strbuf_release() patterns, where \"&buf\" could be used\n // afterwards.\n ... when != \\( I \\| &I \\)\n+// Note that we're intentionally loose in accepting e.g. a\n+// \"strbuf_init(&buf)\" followed by a \"string_list_clear(&buf,\n+// 0)\". It's assumed that the compiler will catch any such invalid\n+// code, i.e. that our constructors/destructors don't take a \"void *\".\n+//\n // This rule also isn't capable of finding cases where &buf is used,\n // but only to e.g. pass that variable to a static function which\n // doesn't use it. The analysis is only function-local.\n-- \n2.37.0.900.g4d0de1cceb2\n\n"},{"id":"458355","messageId":"patch-v3-3.4-9a5e7208dec-20220701T102506Z-avarab@gmail.com","threadId":"57903","inReplyTo":"cover-v3-0.4-00000000000-20220701T102506Z-avarab@gmail.com","subject":"[PATCH v3 3/4] cocci: remove \"when strict\" from unused.cocci","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-01T10:30:58Z","receivedAt":"2022-07-01T10:31:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the \"when strict\" constraint from the newly introduced rule to\nfind unused code. As seen in the change this will help us find cases\nwhere a \"return\" was causing spatch in the middle of our match was\ncausing spatch to abort its analysis.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/merge.c | 2 --\n contrib/coccinelle/unused.cocci | 2 --\n 2 files changed, 4 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex bbd70b17bc6..23170f2d2a6 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -375,7 +375,6 @@ static void reset_hard(const struct object_id *oid, int verbose)\n static void restore_state(const struct object_id *head,\n \t\t\t const struct object_id *stash)\n {\n-\tstruct strbuf sb = STRBUF_INIT;\n \tconst char *args[] = { \"stash\", \"apply\", NULL, NULL };\n \n \tif (is_null_oid(stash))\n@@ -391,7 +390,6 @@ static void restore_state(const struct object_id *head,\n \t */\n \trun_command_v_opt(args, RUN_GIT_CMD);\n \n-\tstrbuf_release(&sb);\n \trefresh_cache(REFRESH_QUIET);\n }\n \ndiff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci\nindex 43942f3cd4f..9f0101c1350 100644\n--- a/contrib/coccinelle/unused.cocci\n+++ b/contrib/coccinelle/unused.cocci\n@@ -47,7 +47,6 @@ identifier REL1 =~ \"^strbuf_release$\";\n // ... Optionally followed by lines that make no use of \"buf\", \"&buf\"\n // etc., but which ...\n <... when != \\( I \\| &I \\)\n- when strict\n (\n // .. (only) make use of \"buf\" or \"&buf\" to call something like\n // \"strbuf_init(&buf, ...)\" ...\n@@ -67,7 +66,6 @@ identifier REL1 =~ \"^strbuf_release$\";\n // init/strbuf_release() patterns, where \"&buf\" could be used\n // afterwards.\n ... when != \\( I \\| &I \\)\n- when strict\n // This rule also isn't capable of finding cases where &buf is used,\n // but only to e.g. pass that variable to a static function which\n // doesn't use it. The analysis is only function-local.\n-- \n2.37.0.900.g4d0de1cceb2\n\n"},{"id":"458399","messageId":"Yr83SjgGMMwN0r0s@coredump.intra.peff.net","threadId":"57903","inReplyTo":"patch-v3-1.4-49e9ccb5819-20220701T102506Z-avarab@gmail.com","subject":"Re: [PATCH v3 1/4] cocci: add and apply a rule to find \"unused\" strbufs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-01T18:04:58Z","receivedAt":"2022-07-01T18:05:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 01, 2022 at 12:30:56PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> The use of \"with strict\" here will be explained and amended in the\n> following commit.\n\nReally s/the following/a following/ now that there's another commit in\nbetween. :)\n\nI didn't find the comment there very enlightening, though. It says we're\ndropping \"with strict\" to catch some new cases around early returns. But\nit's not clear to me what \"strict\" does in the first place, why we'd\nwant it, or what we might be losing by dropping it.\n\nI know, I can probably go read the coccinelle docs to figure it out, but\nit might be worth saying why it's worth including in the first place\nsomewhere (either here or in the follow-up commit).\n\n-Peff\n"},{"id":"458400","messageId":"Yr84aglgBwIvnwC7@coredump.intra.peff.net","threadId":"57903","inReplyTo":"cover-v3-0.4-00000000000-20220701T102506Z-avarab@gmail.com","subject":"Re: [PATCH v3 0/4] add and apply a rule to find \"unused\" init+free","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-01T18:09:46Z","receivedAt":"2022-07-01T18:09:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 01, 2022 at 12:30:55PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> This series adds a coccinelle rule to find and remove code where the\n> only reference to a variable in a given function is to malloc() &\n> free() it, where \"malloc\" and \"free\" also match\n> \"strbuf_init/strbuf_release\", and then later in the series anything\n> that looks like a init/free pattern.\n\nOverall, I hated this much less than I expected to. ;)\n\nMy main concern was that we'd have a jumble of init/release rules for\nevery data type, but the regex matching on identifier names lets us do\nthings broadly. And for data types that don't match our usual naming\nconventions, that seems like a good opportunity to consider changing\ntheir names.\n\nI'm not too concerned about false positives here, since there are humans\nin the loop. The resulting diffs are pretty obvious to review, and a\ncompiler would obviously catch any case where the variable really did\nget used in between.\n\nI do wish we could do it with the compiler and not coccinelle, just\nbecause the latter has been such a source of frustration in terms of\nperformance issues and head-scratching \"why didn't it catch this one\"\ncases. But this is IMHO much better than nothing, as shown by the\ncleanups it already found.\n\n-Peff\n"},{"id":"458413","messageId":"CAPig+cSWpzHtv-Kv7=fibkPot=NTwKgc5_Eo_7G4fT=FnaGrxA@mail.gmail.com","threadId":"57903","inReplyTo":"patch-v3-1.4-49e9ccb5819-20220701T102506Z-avarab@gmail.com","subject":"Re: [PATCH v3 1/4] cocci: add and apply a rule to find \"unused\" strbufs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-07-01T19:55:57Z","receivedAt":"2022-07-01T19:56:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jul 1, 2022 at 6:36 AM Ævar Arnfjörð Bjarmason wrote:\n> Add a coccinelle rule to remove \"struct strbuf\" initialization\n> followed by calling \"strbuf_release()\" function.\n\nI'm probably being overly pedantic, but I tripped over this failing to\nmention that the strbuf is not used between the initialization and the\nrelease. That is, I had expected it to say:\n\n Add a coccinelle rule to remove \"struct strbuf\" initialization\n followed by calling \"strbuf_release()\" function without any uses\n of the strbuf in between.\n\n> See extensive commentary in the new \"unused.cocci\" for how it works,\n> and what it's intended to find and replace.\n>\n> The inclusion of \"contrib/scalar/scalar.c\" is because \"spatch\" was\n> manually run on it (we don't usually run spatch on contrib).\n>\n> The use of \"with strict\" here will be explained and amended in the\n> following commit.\n\nDid you mean \"when strict\"?\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> diff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci\n> @@ -0,0 +1,61 @@\n> +// This rule finds sequences of \"unused\" declerations and uses of\n\ns/declerations/declarations/\n\n> +// To do do this we find (continued below)...\n\ns/do do/do/\n"},{"id":"458421","messageId":"CAPig+cT4CF+fmMNOkuf+Yq5O1-p=UsHOXXOGUpn1wy4HNJZ+TA@mail.gmail.com","threadId":"57903","inReplyTo":"patch-v3-3.4-9a5e7208dec-20220701T102506Z-avarab@gmail.com","subject":"Re: [PATCH v3 3/4] cocci: remove \"when strict\" from unused.cocci","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-07-01T21:33:49Z","receivedAt":"2022-07-01T21:34:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jul 1, 2022 at 6:36 AM Ævar Arnfjörð Bjarmason wrote:\n> Remove the \"when strict\" constraint from the newly introduced rule to\n> find unused code. As seen in the change this will help us find cases\n> where a \"return\" was causing spatch in the middle of our match was\n> causing spatch to abort its analysis.\n\nThere seem to be some extraneous words in this paragraph. Did you mean:\n\n Remove the \"when strict\" constraint from the newly introduced\n rule to find unused code. As seen in the change this will help us\n find cases where a \"return\" in the middle of our match was\n causing spatch to abort its analysis.\n\n?\n"},{"id":"458478","messageId":"cover-v4-0.6-00000000000-20220705T134033Z-avarab@gmail.com","threadId":"57903","inReplyTo":"cover-v3-0.4-00000000000-20220701T102506Z-avarab@gmail.com","subject":"[PATCH v4 0/6] add and apply a rule to find \"unused\" init+free","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-05T13:46:54Z","receivedAt":"2022-07-05T14:02:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series adds a coccinelle rule to find and remove code where the\nonly reference to a variable in a given function is to malloc() &\nfree() it, where \"malloc\" and \"free\" also match\n\"strbuf_init/strbuf_release\", and then later in the series anything\nthat looks like a init/free pattern.\n\nChanges since v3[1]\n\n * Add a \"coccicheck-test\" target in an early and new patch, the\n structure mirrors that of coccinelle.git's own tests. As the\n diffstat shows we have a *.c and *.res file which is C code\n before/after a *.cocci rule is applied.\n * The extensive commentary in the proposed rule is gone in favor of\n self-explanatory test cases\n * We now catch init/reset patterns as well as init/release fully\n (i.e. for the \"struct strbuf\" early on)\n * Squashed the \"when strict\" change in, and there's now a test-case\n for it.\n * 1-2/6 are new minor cleanup patches for what follows.\n\n1. https://lore.kernel.org/git/cover-v3-0.4-00000000000-20220701T102506Z-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (6):\n Makefile: remove mandatory \"spatch\" arguments from SPATCH_FLAGS\n Makefile & .gitignore: ignore & clean \"git.res\", not \"*.res\"\n cocci: add a \"coccicheck-test\" target and test *.cocci rules\n cocci: have \"coccicheck{,-pending}\" depend on \"coccicheck-test\"\n cocci: add and apply a rule to find \"unused\" strbufs\n cocci: generalize \"unused\" rule to cover more than \"strbuf\"\n\n .gitignore | 2 +-\n Makefile | 28 ++++++++--\n builtin/fetch.c | 3 +-\n builtin/merge.c | 4 --\n builtin/repack.c | 2 -\n contrib/coccinelle/tests/free.c | 11 ++++\n contrib/coccinelle/tests/free.res | 9 ++++\n contrib/coccinelle/tests/unused.c | 82 +++++++++++++++++++++++++++++\n contrib/coccinelle/tests/unused.res | 45 ++++++++++++++++\n contrib/coccinelle/unused.cocci | 43 +++++++++++++++\n contrib/scalar/scalar.c | 3 +-\n diff.c | 2 -\n shared.mak | 1 +\n 13 files changed, 219 insertions(+), 16 deletions(-)\n create mode 100644 contrib/coccinelle/tests/free.c\n create mode 100644 contrib/coccinelle/tests/free.res\n create mode 100644 contrib/coccinelle/tests/unused.c\n create mode 100644 contrib/coccinelle/tests/unused.res\n create mode 100644 contrib/coccinelle/unused.cocci\n\nRange-diff against v3:\n-: ----------- > 1: fbdc2c3d66b Makefile: remove mandatory \"spatch\" arguments from SPATCH_FLAGS\n-: ----------- > 2: d7e85d4c4a6 Makefile & .gitignore: ignore & clean \"git.res\", not \"*.res\"\n-: ----------- > 3: 540186e69dc cocci: add a \"coccicheck-test\" target and test *.cocci rules\n-: ----------- > 4: 48810f7390c cocci: have \"coccicheck{,-pending}\" depend on \"coccicheck-test\"\n1: 49e9ccb5819 ! 5: d1c6833c8d5 cocci: add and apply a rule to find \"unused\" strbufs\n @@ Commit message\n cocci: add and apply a rule to find \"unused\" strbufs\n \n Add a coccinelle rule to remove \"struct strbuf\" initialization\n - followed by calling \"strbuf_release()\" function.\n + followed by calling \"strbuf_release()\" function, without any uses of\n + the strbuf in the same function.\n \n - See extensive commentary in the new \"unused.cocci\" for how it works,\n - and what it's intended to find and replace.\n + See the tests in contrib/coccinelle/tests/unused.{c,res} for what it's\n + intended to find and replace.\n \n The inclusion of \"contrib/scalar/scalar.c\" is because \"spatch\" was\n manually run on it (we don't usually run spatch on contrib).\n \n - The use of \"with strict\" here will be explained and amended in the\n - following commit.\n + Per the \"buggy code\" comment we also match a strbuf_init() before the\n + xmalloc(), but we're not seeking to be so strict as to make checks\n + that the compiler will catch for us redundant. Saying we'll match\n + either \"init\" or \"xmalloc\" lines makes the rule simpler.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n @@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *\n }\n \n ## builtin/merge.c ##\n +@@ builtin/merge.c: static void reset_hard(const struct object_id *oid, int verbose)\n + static void restore_state(const struct object_id *head,\n + \t\t\t const struct object_id *stash)\n + {\n +-\tstruct strbuf sb = STRBUF_INIT;\n + \tconst char *args[] = { \"stash\", \"apply\", NULL, NULL };\n + \n + \tif (is_null_oid(stash))\n +@@ builtin/merge.c: static void restore_state(const struct object_id *head,\n + \t */\n + \trun_command_v_opt(args, RUN_GIT_CMD);\n + \n +-\tstrbuf_release(&sb);\n + \trefresh_cache(REFRESH_QUIET);\n + }\n + \n @@ builtin/merge.c: static void merge_name(const char *remote, struct strbuf *msg)\n {\n \tstruct commit *remote_head;\n @@ builtin/merge.c: static void merge_name(const char *remote, struct strbuf *msg)\n }\n \n \n + ## contrib/coccinelle/tests/unused.c (new) ##\n +@@\n ++void test_strbuf(void)\n ++{\n ++\tstruct strbuf sb1 = STRBUF_INIT;\n ++\tstruct strbuf sb2 = STRBUF_INIT;\n ++\tstruct strbuf sb3 = STRBUF_INIT;\n ++\tstruct strbuf sb4 = STRBUF_INIT;\n ++\tstruct strbuf sb5;\n ++\tstruct strbuf sb6 = { 0 };\n ++\tstruct strbuf sb7 = STRBUF_INIT;\n ++\tstruct strbuf sb8 = STRBUF_INIT;\n ++\tstruct strbuf *sp1;\n ++\tstruct strbuf *sp2;\n ++\tstruct strbuf *sp3;\n ++\tstruct strbuf *sp4 = xmalloc(sizeof(struct strbuf));\n ++\tstruct strbuf *sp5 = xmalloc(sizeof(struct strbuf));\n ++\tstruct strbuf *sp6 = xmalloc(sizeof(struct strbuf));\n ++\tstruct strbuf *sp7;\n ++\n ++\tstrbuf_init(&sb5, 0);\n ++\tstrbuf_init(sp1, 0);\n ++\tstrbuf_init(sp2, 0);\n ++\tstrbuf_init(sp3, 0);\n ++\tstrbuf_init(sp4, 0);\n ++\tstrbuf_init(sp5, 0);\n ++\tstrbuf_init(sp6, 0);\n ++\tstrbuf_init(sp7, 0);\n ++\tsp7 = xmalloc(sizeof(struct strbuf));\n ++\n ++\tuse_before(&sb3);\n ++\tuse_as_str(\"%s\", sb7.buf);\n ++\tuse_as_str(\"%s\", sp1->buf);\n ++\tuse_as_str(\"%s\", sp6->buf);\n ++\tpass_pp(&sp3);\n ++\n ++\tstrbuf_release(&sb1);\n ++\tstrbuf_reset(&sb2);\n ++\tstrbuf_release(&sb3);\n ++\tstrbuf_release(&sb4);\n ++\tstrbuf_release(&sb5);\n ++\tstrbuf_release(&sb6);\n ++\tstrbuf_release(&sb7);\n ++\tstrbuf_release(sp1);\n ++\tstrbuf_release(sp2);\n ++\tstrbuf_release(sp3);\n ++\tstrbuf_release(sp4);\n ++\tstrbuf_release(sp5);\n ++\tstrbuf_release(sp6);\n ++\tstrbuf_release(sp7);\n ++\n ++\tuse_after(&sb4);\n ++\n ++\tif (when_strict())\n ++\t\treturn;\n ++\tstrbuf_release(&sb8);\n ++}\n +\n + ## contrib/coccinelle/tests/unused.res (new) ##\n +@@\n ++void test_strbuf(void)\n ++{\n ++\tstruct strbuf sb3 = STRBUF_INIT;\n ++\tstruct strbuf sb4 = STRBUF_INIT;\n ++\tstruct strbuf sb7 = STRBUF_INIT;\n ++\tstruct strbuf *sp1;\n ++\tstruct strbuf *sp3;\n ++\tstruct strbuf *sp6 = xmalloc(sizeof(struct strbuf));\n ++\tstrbuf_init(sp1, 0);\n ++\tstrbuf_init(sp3, 0);\n ++\tstrbuf_init(sp6, 0);\n ++\n ++\tuse_before(&sb3);\n ++\tuse_as_str(\"%s\", sb7.buf);\n ++\tuse_as_str(\"%s\", sp1->buf);\n ++\tuse_as_str(\"%s\", sp6->buf);\n ++\tpass_pp(&sp3);\n ++\n ++\tstrbuf_release(&sb3);\n ++\tstrbuf_release(&sb4);\n ++\tstrbuf_release(&sb7);\n ++\tstrbuf_release(sp1);\n ++\tstrbuf_release(sp3);\n ++\tstrbuf_release(sp6);\n ++\n ++\tuse_after(&sb4);\n ++\n ++\tif (when_strict())\n ++\t\treturn;\n ++}\n +\n ## contrib/coccinelle/unused.cocci (new) ##\n @@\n -+// This rule finds sequences of \"unused\" declerations and uses of\n -+// \"struct strbuf\".\n -+//\n -+// I.e. this finds cases where we only declare the variable, and then\n -+// release it, e.g.:\n -+//\n -+//\tstruct strbuf buf = STRBUF_INIT;\n -+// [.. no other use of \"buf\" in the function ..]\n -+//\tstrbuf_release(&buf)\n -+//\n -+// Or:\n -+//\n -+//\tstruct strbuf buf;\n -+//\t[.. no other use of \"buf\" in the function ..]\n -+//\tstrbuf_init(&buf, 0);\n -+//\t[.. no other use of \"buf\" in the function ..]\n -+//\tstrbuf_release(&buf)\n -+//\n -+// To do do this we find (continued below)...\n ++// This rule finds sequences of \"unused\" declerations and uses of a\n ++// variable, where \"unused\" is defined to include only calling the\n ++// equivalent of alloc, init & free functions on the variable.\n +@@\n +type T;\n +identifier I;\n -+// STRBUF_INIT\n +constant INIT_MACRO =~ \"^STRBUF_INIT$\";\n -+// strbuf_init(&I, ...) etc.\n ++identifier MALLOC1 =~ \"^x?[mc]alloc$\";\n +identifier INIT_CALL1 =~ \"^strbuf_init$\";\n -+// strbuf_release()\n -+identifier REL1 =~ \"^strbuf_release$\";\n ++identifier REL1 =~ \"^strbuf_(release|reset)$\";\n +@@\n +\n -+// .. A declaration like \"struct strbuf buf;\"...\n +(\n +- T I;\n -+// ... or \"struct strbuf buf = { 0 };\" ...\n +|\n +- T I = { 0 };\n -+// ... or \"struct STRBUF buf = STRBUF_INIT;\" ...\n +|\n +- T I = INIT_MACRO;\n ++|\n ++- T I = MALLOC1(...);\n +)\n +\n -+// ... Optionally followed by lines that make no use of \"buf\", \"&buf\"\n -+// etc., but which ...\n +<... when != \\( I \\| &I \\)\n -+ when strict\n -+// .. (only) make use of \"buf\" or \"&buf\" to call something like\n -+// \"strbuf_init(&buf, ...)\" ...\n ++(\n +- \\( INIT_CALL1 \\)( \\( I \\| &I \\), ...);\n ++|\n ++- I = MALLOC1(...);\n ++)\n +...>\n +\n -+// ... and then no mention of \"buf\" or \"&buf\" until we get to a\n -+// strbuf_release(&buf) at the end ...\n +- \\( REL1 \\)( \\( &I \\| I \\) );\n -+// ... and no use *after* either, e.g. we don't want to delete\n -+// init/strbuf_release() patterns, where \"&buf\" could be used\n -+// afterwards.\n + ... when != \\( I \\| &I \\)\n -+ when strict\n -+// This rule also isn't capable of finding cases where &buf is used,\n -+// but only to e.g. pass that variable to a static function which\n -+// doesn't use it. The analysis is only function-local.\n \n ## contrib/scalar/scalar.c ##\n @@ contrib/scalar/scalar.c: static int cmd_diagnose(int argc, const char **argv)\n2: 6324d3956ed < -: ----------- cocci: catch unused \"strbuf\" using an xmalloc() pattern\n3: 9a5e7208dec < -: ----------- cocci: remove \"when strict\" from unused.cocci\n4: 45a429b9cc9 ! 6: dafd59d5ded cocci: generalize \"unused\" rule to cover more than \"strbuf\"\n @@ Commit message\n similar-looking *_{release,clear,free}() and {release,clear,free}_*()\n functions.\n \n + We're intentionally loose in accepting e.g. a \"strbuf_init(&sb)\"\n + followed by a \"string_list_clear(&sb, 0)\". It's assumed that the\n + compiler will catch any such invalid code, i.e. that our\n + constructors/destructors don't take a \"void *\".\n +\n See [1] for example of code that would be covered by the\n \"get_worktrees()\" part of this rule. We'd still need work that the\n series is based on (we were passing \"worktrees\" to a function), but\n @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix\n \tstring_list_clear(&existing_kept_packs, 0);\n \tclear_pack_geometry(geometry);\n \n + ## contrib/coccinelle/tests/unused.c ##\n +@@ contrib/coccinelle/tests/unused.c: void test_strbuf(void)\n + \t\treturn;\n + \tstrbuf_release(&sb8);\n + }\n ++\n ++void test_other(void)\n ++{\n ++\tstruct string_list l = STRING_LIST_INIT_DUP;\n ++\tstruct strbuf sb = STRBUF_INIT;\n ++\n ++\tstring_list_clear(&l, 0);\n ++\tstring_list_clear(&sb, 0);\n ++}\n ++\n ++void test_worktrees(void)\n ++{\n ++\tstruct worktree **w1 = get_worktrees();\n ++\tstruct worktree **w2 = get_worktrees();\n ++\tstruct worktree **w3;\n ++\tstruct worktree **w4;\n ++\n ++\tw3 = get_worktrees();\n ++\tw4 = get_worktrees();\n ++\n ++\tuse_it(w4);\n ++\n ++\tfree_worktrees(w1);\n ++\tfree_worktrees(w2);\n ++\tfree_worktrees(w3);\n ++\tfree_worktrees(w4);\n ++}\n +\n + ## contrib/coccinelle/tests/unused.res ##\n +@@ contrib/coccinelle/tests/unused.res: void test_strbuf(void)\n + \tif (when_strict())\n + \t\treturn;\n + }\n ++\n ++void test_other(void)\n ++{\n ++}\n ++\n ++void test_worktrees(void)\n ++{\n ++\tstruct worktree **w4;\n ++\n ++\tw4 = get_worktrees();\n ++\n ++\tuse_it(w4);\n ++\n ++\tfree_worktrees(w4);\n ++}\n +\n ## contrib/coccinelle/unused.cocci ##\n -@@\n - // This rule finds sequences of \"unused\" declerations and uses of\n --// \"struct strbuf\".\n -+// \"struct strbuf\" and other common types.\n - //\n - // I.e. this finds cases where we only declare the variable, and then\n - // release it, e.g.:\n @@\n @@\n type T;\n identifier I;\n --// STRBUF_INIT\n -constant INIT_MACRO =~ \"^STRBUF_INIT$\";\n +// STRBUF_INIT, but also e.g. STRING_LIST_INIT_DUP (so no anchoring)\n +constant INIT_MACRO =~ \"_INIT\";\n - // x[mc]alloc() etc.\n identifier MALLOC1 =~ \"^x?[mc]alloc$\";\n -+// I = get_worktrees() etc.\n -+identifier INIT_ASSIGN1 =~ \"^get_worktrees$\";\n - // strbuf_init(&I, ...) etc.\n -identifier INIT_CALL1 =~ \"^strbuf_init$\";\n --// strbuf_release()\n --identifier REL1 =~ \"^strbuf_release$\";\n +-identifier REL1 =~ \"^strbuf_(release|reset)$\";\n ++identifier INIT_ASSIGN1 =~ \"^get_worktrees$\";\n +identifier INIT_CALL1 =~ \"^[a-z_]*_init$\";\n -+// stbuf_release(), string_list_clear() etc.\n -+identifier REL1 =~ \"^[a-z_]*_(release|clear|free)$\";\n -+// release_patch(), clear_pathspec() etc.\n ++identifier REL1 =~ \"^[a-z_]*_(release|reset|clear|free)$\";\n +identifier REL2 =~ \"^(release|clear|free)_[a-z_]*$\";\n @@\n \n - // .. A declaration like \"struct strbuf buf;\"...\n -@@ contrib/coccinelle/unused.cocci: identifier REL1 =~ \"^strbuf_release$\";\n - // \"strbuf_init(&buf, ...)\" ...\n + (\n +@@ contrib/coccinelle/unused.cocci: identifier REL1 =~ \"^strbuf_(release|reset)$\";\n + - T I = INIT_MACRO;\n + |\n + - T I = MALLOC1(...);\n ++|\n ++- T I = INIT_ASSIGN1(...);\n + )\n + \n + <... when != \\( I \\| &I \\)\n + (\n - \\( INIT_CALL1 \\)( \\( I \\| &I \\), ...);\n |\n -+// .. or e.g. \"worktrees = get_worktrees();\", i.e. a known \"assignment\n -+// init\" ...\n +- I = \\( INIT_ASSIGN1 \\)(...);\n +|\n - // .. or to follow-up a \"struct strbuf *buf\" with e.g. \"buf =\n - // xmalloc(...)\" (which may in turn be followed-up by a\n - // \"strbuf_init()\", which we'll match with INIT_CALL1) ...\n -@@ contrib/coccinelle/unused.cocci: identifier REL1 =~ \"^strbuf_release$\";\n + - I = MALLOC1(...);\n + )\n + ...>\n \n - // ... and then no mention of \"buf\" or \"&buf\" until we get to a\n - // strbuf_release(&buf) at the end ...\n -- \\( REL1 \\)( \\( &I \\| I \\) );\n +(\n +- \\( REL1 \\| REL2 \\)( \\( I \\| &I \\), ...);\n +|\n +- \\( REL1 \\| REL2 \\)( \\( &I \\| I \\) );\n +)\n - // ... and no use *after* either, e.g. we don't want to delete\n - // init/strbuf_release() patterns, where \"&buf\" could be used\n - // afterwards.\n ... when != \\( I \\| &I \\)\n -+// Note that we're intentionally loose in accepting e.g. a\n -+// \"strbuf_init(&buf)\" followed by a \"string_list_clear(&buf,\n -+// 0)\". It's assumed that the compiler will catch any such invalid\n -+// code, i.e. that our constructors/destructors don't take a \"void *\".\n -+//\n - // This rule also isn't capable of finding cases where &buf is used,\n - // but only to e.g. pass that variable to a static function which\n - // doesn't use it. The analysis is only function-local.\n-- \n2.37.0.913.g50625c3f077\n\n"},{"id":"458479","messageId":"patch-v4-2.6-d7e85d4c4a6-20220705T134033Z-avarab@gmail.com","threadId":"57903","inReplyTo":"cover-v4-0.6-00000000000-20220705T134033Z-avarab@gmail.com","subject":"[PATCH v4 2/6] Makefile & .gitignore: ignore & clean \"git.res\", not \"*.res\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-05T13:46:56Z","receivedAt":"2022-07-05T14:02:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Adjust the overly broad .gitignore and \"make clean\" rule added in\nce39c2e04ce (Provide a Windows version resource for the git\nexecutables., 2012-05-24).\n\nFor now this is merely a correctness fix, but needed because a\nsubsequent commit will want to check in *.res files elsewhere in the\ntree, which we shouldn't have to \"git add -f\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n .gitignore | 2 +-\n Makefile | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/.gitignore b/.gitignore\nindex a4522157641..42fd7253b44 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -185,6 +185,7 @@\n /git-worktree\n /git-write-tree\n /git-core-*/?*\n+/git.res\n /gitweb/GITWEB-BUILD-OPTIONS\n /gitweb/gitweb.cgi\n /gitweb/static/gitweb.js\n@@ -225,7 +226,6 @@\n *.hcc\n *.obj\n *.lib\n-*.res\n *.sln\n *.sp\n *.suo\ndiff --git a/Makefile b/Makefile\nindex c1d02f04499..1ccf13595de 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3409,7 +3409,7 @@ cocciclean:\n clean: profile-clean coverage-clean cocciclean\n \t$(RM) -r .build\n \t$(RM) po/git.pot po/git-core.pot\n-\t$(RM) *.res\n+\t$(RM) git.res\n \t$(RM) $(OBJECTS)\n \t$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)\n \t$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X\n-- \n2.37.0.913.g50625c3f077\n\n"},{"id":"458480","messageId":"patch-v4-1.6-fbdc2c3d66b-20220705T134033Z-avarab@gmail.com","threadId":"57903","inReplyTo":"cover-v4-0.6-00000000000-20220705T134033Z-avarab@gmail.com","subject":"[PATCH v4 1/6] Makefile: remove mandatory \"spatch\" arguments from SPATCH_FLAGS","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-05T13:46:55Z","receivedAt":"2022-07-05T14:02:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The \"--patch .\" part of SPATCH_FLAGS added in f57d11728d1 (coccinelle:\nput sane filenames into output patches, 2018-07-23) should have been\nadded unconditionally to the \"spatch\" invocation instead, using it\nisn't optional.\n\nLet's also move the other mandatory flag to come after\n$(SPATCH_FLAGS), to ensure that our \"--sp-file\" overrides any provided\nin the environment, both --sp-file and --patch are\nlast-option-wins as far as spatch(1) option parsing is concerned.\n\nThe environment variable override was initially added in\na9a884aea57 (coccicheck: use --all-includes by default,\n2016-09-30). In practice there's probably nobody that's using\nSPATCH_FLAGS to try to intentionally break our invocations, but since\nwe're changing this let's make it clear what (if anything) we expect\nto be overridden by user-supplied flags.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Makefile | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 04d0fd1fe60..c1d02f04499 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1286,7 +1286,7 @@ SANITIZE_ADDRESS =\n # For the 'coccicheck' target; setting SPATCH_BATCH_SIZE higher will\n # usually result in less CPU usage at the cost of higher peak memory.\n # Setting it to 0 will feed all files in a single spatch invocation.\n-SPATCH_FLAGS = --all-includes --patch .\n+SPATCH_FLAGS = --all-includes\n SPATCH_BATCH_SIZE = 1\n \n include config.mak.uname\n@@ -3131,7 +3131,8 @@ check: $(GENERATED_H)\n \t\tlimit='-n $(SPATCH_BATCH_SIZE)'; \\\n \tfi; \\\n \tif ! echo $(COCCI_SOURCES) | xargs $$limit \\\n-\t\t$(SPATCH) --sp-file $< $(SPATCH_FLAGS) \\\n+\t\t$(SPATCH) $(SPATCH_FLAGS) \\\n+\t\t--sp-file $< --patch . \\\n \t\t>$@+ 2>$@.log; \\\n \tthen \\\n \t\tcat $@.log; \\\n-- \n2.37.0.913.g50625c3f077\n\n"},{"id":"458481","messageId":"patch-v4-3.6-540186e69dc-20220705T134033Z-avarab@gmail.com","threadId":"57903","inReplyTo":"cover-v4-0.6-00000000000-20220705T134033Z-avarab@gmail.com","subject":"[PATCH v4 3/6] cocci: add a \"coccicheck-test\" target and test *.cocci rules","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-05T13:46:57Z","receivedAt":"2022-07-05T14:02:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a \"coccicheck-test\" target to test our *.cocci rules, and as a\ndemonstration add tests for the rules added in 39ea59a2570 (remove\nunnecessary NULL check before free(3), 2016-10-08) and\n1b83d1251ed (coccinelle: add a rule to make \"expression\" code use\nFREE_AND_NULL(), 2017-06-15).\n\nI considered making use of the \"spatch --test\" option, and the choice\nof a \"tests\" over a \"t\" directory is to make these tests compatible\nwith such a future change.\n\nUnfortunately \"spatch --test\" doesn't return meaningful exit codes,\nAFAICT you need to \"grep\" its output to see if the *.res is what you\nexpect. There's \"--test-okfailed\", but I didn't find a way to sensibly\nintegrate those (it relies on some in-between status files, but\ndoesn't help with the status codes).\n\nInstead let's use a \"--sp-file\" pattern similar to the main\n\"coccicheck\" rule, with the difference that we use and compare the\ntwo *.res files with cmp(1).\n\nThe --very-quiet and --no-show-diff options ensure that we don't need\nto pipe stdout and stderr somewhere. Unlike the \"%.cocci.patch\" rule\nwe're not using the diff.\n\nThe \"cmp || git diff\" is optimistically giving us better output on\nfailure, but even if we only have POSIX cmp and no system git\ninstalled we'll still fail with the \"cmp\", just with an error message\nthat isn't as friendly. The \"2>/dev/null\" is in case we don't have a\n\"git\" installed.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Makefile | 19 +++++++++++++++++++\n contrib/coccinelle/tests/free.c | 11 +++++++++++\n contrib/coccinelle/tests/free.res | 9 +++++++++\n shared.mak | 1 +\n 4 files changed, 40 insertions(+)\n create mode 100644 contrib/coccinelle/tests/free.c\n create mode 100644 contrib/coccinelle/tests/free.res\n\ndiff --git a/Makefile b/Makefile\nindex 1ccf13595de..c4064f8394f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3123,6 +3123,8 @@ check: $(GENERATED_H)\n \t\texit 1; \\\n \tfi\n \n+COCCI_TEST_RES = $(wildcard contrib/coccinelle/tests/*.res)\n+\n %.cocci.patch: %.cocci $(COCCI_SOURCES)\n \t$(QUIET_SPATCH) \\\n \tif test $(SPATCH_BATCH_SIZE) = 0; then \\\n@@ -3143,6 +3145,22 @@ check: $(GENERATED_H)\n \tthen \\\n \t\techo ' ' SPATCH result: $@; \\\n \tfi\n+\n+COCCI_TEST_RES_GEN = $(addprefix .build/,$(COCCI_TEST_RES))\n+$(COCCI_TEST_RES_GEN): .build/%.res : %.c\n+$(COCCI_TEST_RES_GEN): .build/%.res : %.res\n+$(COCCI_TEST_RES_GEN): .build/contrib/coccinelle/tests/%.res : contrib/coccinelle/%.cocci\n+\t$(call mkdir_p_parent_template)\n+\t$(QUIET_SPATCH_T)$(SPATCH) $(SPATCH_FLAGS) \\\n+\t\t--very-quiet --no-show-diff \\\n+\t\t--sp-file $< -o $@ \\\n+\t\t$(@:.build/%.res=%.c) && \\\n+\tcmp $(@:.build/%=%) $@ || \\\n+\tgit -P diff --no-index $(@:.build/%=%) $@ 2>/dev/null; \\\n+\n+.PHONY: coccicheck-test\n+coccicheck-test: $(COCCI_TEST_RES_GEN)\n+\n coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/coccinelle/*.cocci)))\n \n # See contrib/coccinelle/README\n@@ -3404,6 +3422,7 @@ profile-clean:\n \t$(RM) $(addsuffix *.gcno,$(addprefix $(PROFILE_DIR)/, $(object_dirs)))\n \n cocciclean:\n+\t$(RM) -r .build/contrib/coccinelle\n \t$(RM) contrib/coccinelle/*.cocci.patch*\n \n clean: profile-clean coverage-clean cocciclean\ndiff --git a/contrib/coccinelle/tests/free.c b/contrib/coccinelle/tests/free.c\nnew file mode 100644\nindex 00000000000..96d4abc0c78\n--- /dev/null\n+++ b/contrib/coccinelle/tests/free.c\n@@ -0,0 +1,11 @@\n+int use_FREE_AND_NULL(int *v)\n+{\n+\tfree(*v);\n+\t*v = NULL;\n+}\n+\n+int need_no_if(int *v)\n+{\n+\tif (v)\n+\t\tfree(v);\n+}\ndiff --git a/contrib/coccinelle/tests/free.res b/contrib/coccinelle/tests/free.res\nnew file mode 100644\nindex 00000000000..f90fd9f48e3\n--- /dev/null\n+++ b/contrib/coccinelle/tests/free.res\n@@ -0,0 +1,9 @@\n+int use_FREE_AND_NULL(int *v)\n+{\n+\tFREE_AND_NULL(*v);\n+}\n+\n+int need_no_if(int *v)\n+{\n+\tfree(v);\n+}\ndiff --git a/shared.mak b/shared.mak\nindex 4330192e9c3..33f43edbf9a 100644\n--- a/shared.mak\n+++ b/shared.mak\n@@ -70,6 +70,7 @@ ifndef V\n \tQUIET_HDR = @echo ' ' HDR $(<:hcc=h);\n \tQUIET_RC = @echo ' ' RC $@;\n \tQUIET_SPATCH = @echo ' ' SPATCH $<;\n+\tQUIET_SPATCH_T = @echo ' ' SPATCH TEST $(@:.build/%=%);\n \n ## Used in \"Documentation/Makefile\"\n \tQUIET_ASCIIDOC\t= @echo ' ' ASCIIDOC $@;\n-- \n2.37.0.913.g50625c3f077\n\n"},{"id":"458482","messageId":"patch-v4-4.6-48810f7390c-20220705T134033Z-avarab@gmail.com","threadId":"57903","inReplyTo":"cover-v4-0.6-00000000000-20220705T134033Z-avarab@gmail.com","subject":"[PATCH v4 4/6] cocci: have \"coccicheck{,-pending}\" depend on \"coccicheck-test\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-05T13:46:58Z","receivedAt":"2022-07-05T14:02:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Have the newly introduced \"coccicheck-test\" target run implicitly when\n\"coccicheck\" itself is run. As with e.g. the \"check-chainlint\"\ntarget (see [1]) it makes sense to run this unconditionally before we\nrun other \"spatch\" rules as a basic sanity check. See\n\n1. 803394459d4 (t/Makefile: add machinery to check correctness of\n chainlint.sed, 2018-07-11)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Makefile | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex c4064f8394f..669828a2a6b 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3161,9 +3161,11 @@ $(COCCI_TEST_RES_GEN): .build/contrib/coccinelle/tests/%.res : contrib/coccinell\n .PHONY: coccicheck-test\n coccicheck-test: $(COCCI_TEST_RES_GEN)\n \n+coccicheck: coccicheck-test\n coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/coccinelle/*.cocci)))\n \n # See contrib/coccinelle/README\n+coccicheck-pending: coccicheck-test\n coccicheck-pending: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.pending.cocci))\n \n .PHONY: coccicheck coccicheck-pending\n-- \n2.37.0.913.g50625c3f077\n\n"},{"id":"458483","messageId":"patch-v4-5.6-d1c6833c8d5-20220705T134033Z-avarab@gmail.com","threadId":"57903","inReplyTo":"cover-v4-0.6-00000000000-20220705T134033Z-avarab@gmail.com","subject":"[PATCH v4 5/6] cocci: add and apply a rule to find \"unused\" strbufs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-05T13:46:59Z","receivedAt":"2022-07-05T14:02:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a coccinelle rule to remove \"struct strbuf\" initialization\nfollowed by calling \"strbuf_release()\" function, without any uses of\nthe strbuf in the same function.\n\nSee the tests in contrib/coccinelle/tests/unused.{c,res} for what it's\nintended to find and replace.\n\nThe inclusion of \"contrib/scalar/scalar.c\" is because \"spatch\" was\nmanually run on it (we don't usually run spatch on contrib).\n\nPer the \"buggy code\" comment we also match a strbuf_init() before the\nxmalloc(), but we're not seeking to be so strict as to make checks\nthat the compiler will catch for us redundant. Saying we'll match\neither \"init\" or \"xmalloc\" lines makes the rule simpler.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/fetch.c | 3 +-\n builtin/merge.c | 4 ---\n contrib/coccinelle/tests/unused.c | 55 +++++++++++++++++++++++++++++\n contrib/coccinelle/tests/unused.res | 30 ++++++++++++++++\n contrib/coccinelle/unused.cocci | 32 +++++++++++++++++\n contrib/scalar/scalar.c | 3 +-\n diff.c | 2 --\n 7 files changed, 119 insertions(+), 10 deletions(-)\n create mode 100644 contrib/coccinelle/tests/unused.c\n create mode 100644 contrib/coccinelle/tests/unused.res\n create mode 100644 contrib/coccinelle/unused.cocci\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ac29c2b1ae3..8a3ae71fed0 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1113,7 +1113,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t struct fetch_head *fetch_head, struct worktree **worktrees)\n {\n \tint url_len, i, rc = 0;\n-\tstruct strbuf note = STRBUF_INIT, err = STRBUF_INIT;\n+\tstruct strbuf note = STRBUF_INIT;\n \tconst char *what, *kind;\n \tstruct ref *rm;\n \tchar *url;\n@@ -1281,7 +1281,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n abort:\n \tstrbuf_release(¬e);\n-\tstrbuf_release(&err);\n \tfree(url);\n \treturn rc;\n }\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex d9784d4891c..23170f2d2a6 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -375,7 +375,6 @@ static void reset_hard(const struct object_id *oid, int verbose)\n static void restore_state(const struct object_id *head,\n \t\t\t const struct object_id *stash)\n {\n-\tstruct strbuf sb = STRBUF_INIT;\n \tconst char *args[] = { \"stash\", \"apply\", NULL, NULL };\n \n \tif (is_null_oid(stash))\n@@ -391,7 +390,6 @@ static void restore_state(const struct object_id *head,\n \t */\n \trun_command_v_opt(args, RUN_GIT_CMD);\n \n-\tstrbuf_release(&sb);\n \trefresh_cache(REFRESH_QUIET);\n }\n \n@@ -502,7 +500,6 @@ static void merge_name(const char *remote, struct strbuf *msg)\n {\n \tstruct commit *remote_head;\n \tstruct object_id branch_head;\n-\tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf bname = STRBUF_INIT;\n \tstruct merge_remote_desc *desc;\n \tconst char *ptr;\n@@ -590,7 +587,6 @@ static void merge_name(const char *remote, struct strbuf *msg)\n \t\toid_to_hex(&remote_head->object.oid), remote);\n cleanup:\n \tfree(found_ref);\n-\tstrbuf_release(&buf);\n \tstrbuf_release(&bname);\n }\n \ndiff --git a/contrib/coccinelle/tests/unused.c b/contrib/coccinelle/tests/unused.c\nnew file mode 100644\nindex 00000000000..495ae58ccf1\n--- /dev/null\n+++ b/contrib/coccinelle/tests/unused.c\n@@ -0,0 +1,55 @@\n+void test_strbuf(void)\n+{\n+\tstruct strbuf sb1 = STRBUF_INIT;\n+\tstruct strbuf sb2 = STRBUF_INIT;\n+\tstruct strbuf sb3 = STRBUF_INIT;\n+\tstruct strbuf sb4 = STRBUF_INIT;\n+\tstruct strbuf sb5;\n+\tstruct strbuf sb6 = { 0 };\n+\tstruct strbuf sb7 = STRBUF_INIT;\n+\tstruct strbuf sb8 = STRBUF_INIT;\n+\tstruct strbuf *sp1;\n+\tstruct strbuf *sp2;\n+\tstruct strbuf *sp3;\n+\tstruct strbuf *sp4 = xmalloc(sizeof(struct strbuf));\n+\tstruct strbuf *sp5 = xmalloc(sizeof(struct strbuf));\n+\tstruct strbuf *sp6 = xmalloc(sizeof(struct strbuf));\n+\tstruct strbuf *sp7;\n+\n+\tstrbuf_init(&sb5, 0);\n+\tstrbuf_init(sp1, 0);\n+\tstrbuf_init(sp2, 0);\n+\tstrbuf_init(sp3, 0);\n+\tstrbuf_init(sp4, 0);\n+\tstrbuf_init(sp5, 0);\n+\tstrbuf_init(sp6, 0);\n+\tstrbuf_init(sp7, 0);\n+\tsp7 = xmalloc(sizeof(struct strbuf));\n+\n+\tuse_before(&sb3);\n+\tuse_as_str(\"%s\", sb7.buf);\n+\tuse_as_str(\"%s\", sp1->buf);\n+\tuse_as_str(\"%s\", sp6->buf);\n+\tpass_pp(&sp3);\n+\n+\tstrbuf_release(&sb1);\n+\tstrbuf_reset(&sb2);\n+\tstrbuf_release(&sb3);\n+\tstrbuf_release(&sb4);\n+\tstrbuf_release(&sb5);\n+\tstrbuf_release(&sb6);\n+\tstrbuf_release(&sb7);\n+\tstrbuf_release(sp1);\n+\tstrbuf_release(sp2);\n+\tstrbuf_release(sp3);\n+\tstrbuf_release(sp4);\n+\tstrbuf_release(sp5);\n+\tstrbuf_release(sp6);\n+\tstrbuf_release(sp7);\n+\n+\tuse_after(&sb4);\n+\n+\tif (when_strict())\n+\t\treturn;\n+\tstrbuf_release(&sb8);\n+}\ndiff --git a/contrib/coccinelle/tests/unused.res b/contrib/coccinelle/tests/unused.res\nnew file mode 100644\nindex 00000000000..b3b71053ed6\n--- /dev/null\n+++ b/contrib/coccinelle/tests/unused.res\n@@ -0,0 +1,30 @@\n+void test_strbuf(void)\n+{\n+\tstruct strbuf sb3 = STRBUF_INIT;\n+\tstruct strbuf sb4 = STRBUF_INIT;\n+\tstruct strbuf sb7 = STRBUF_INIT;\n+\tstruct strbuf *sp1;\n+\tstruct strbuf *sp3;\n+\tstruct strbuf *sp6 = xmalloc(sizeof(struct strbuf));\n+\tstrbuf_init(sp1, 0);\n+\tstrbuf_init(sp3, 0);\n+\tstrbuf_init(sp6, 0);\n+\n+\tuse_before(&sb3);\n+\tuse_as_str(\"%s\", sb7.buf);\n+\tuse_as_str(\"%s\", sp1->buf);\n+\tuse_as_str(\"%s\", sp6->buf);\n+\tpass_pp(&sp3);\n+\n+\tstrbuf_release(&sb3);\n+\tstrbuf_release(&sb4);\n+\tstrbuf_release(&sb7);\n+\tstrbuf_release(sp1);\n+\tstrbuf_release(sp3);\n+\tstrbuf_release(sp6);\n+\n+\tuse_after(&sb4);\n+\n+\tif (when_strict())\n+\t\treturn;\n+}\ndiff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci\nnew file mode 100644\nindex 00000000000..56530498d17\n--- /dev/null\n+++ b/contrib/coccinelle/unused.cocci\n@@ -0,0 +1,32 @@\n+// This rule finds sequences of \"unused\" declerations and uses of a\n+// variable, where \"unused\" is defined to include only calling the\n+// equivalent of alloc, init & free functions on the variable.\n+@@\n+type T;\n+identifier I;\n+constant INIT_MACRO =~ \"^STRBUF_INIT$\";\n+identifier MALLOC1 =~ \"^x?[mc]alloc$\";\n+identifier INIT_CALL1 =~ \"^strbuf_init$\";\n+identifier REL1 =~ \"^strbuf_(release|reset)$\";\n+@@\n+\n+(\n+- T I;\n+|\n+- T I = { 0 };\n+|\n+- T I = INIT_MACRO;\n+|\n+- T I = MALLOC1(...);\n+)\n+\n+<... when != \\( I \\| &I \\)\n+(\n+- \\( INIT_CALL1 \\)( \\( I \\| &I \\), ...);\n+|\n+- I = MALLOC1(...);\n+)\n+...>\n+\n+- \\( REL1 \\)( \\( &I \\| I \\) );\n+ ... when != \\( I \\| &I \\)\ndiff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c\nindex 28176914e57..97e71fe19cd 100644\n--- a/contrib/scalar/scalar.c\n+++ b/contrib/scalar/scalar.c\n@@ -687,7 +687,7 @@ static int cmd_diagnose(int argc, const char **argv)\n \tint stdout_fd = -1, archiver_fd = -1;\n \ttime_t now = time(NULL);\n \tstruct tm tm;\n-\tstruct strbuf path = STRBUF_INIT, buf = STRBUF_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n \tint res = 0;\n \n \targc = parse_options(argc, argv, NULL, options,\n@@ -779,7 +779,6 @@ static int cmd_diagnose(int argc, const char **argv)\n \tfree(argv_copy);\n \tstrvec_clear(&archiver_args);\n \tstrbuf_release(&zip_path);\n-\tstrbuf_release(&path);\n \tstrbuf_release(&buf);\n \n \treturn res;\ndiff --git a/diff.c b/diff.c\nindex e71cf758861..d4290615aaa 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1289,7 +1289,6 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n {\n \tstatic const char *nneof = \" No newline at end of file\\n\";\n \tconst char *context, *reset, *set, *set_sign, *meta, *fraginfo;\n-\tstruct strbuf sb = STRBUF_INIT;\n \n \tenum diff_symbol s = eds->s;\n \tconst char *line = eds->line;\n@@ -1521,7 +1520,6 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \tdefault:\n \t\tBUG(\"unknown diff symbol\");\n \t}\n-\tstrbuf_release(&sb);\n }\n \n static void emit_diff_symbol(struct diff_options *o, enum diff_symbol s,\n-- \n2.37.0.913.g50625c3f077\n\n"},{"id":"458484","messageId":"patch-v4-6.6-dafd59d5ded-20220705T134033Z-avarab@gmail.com","threadId":"57903","inReplyTo":"cover-v4-0.6-00000000000-20220705T134033Z-avarab@gmail.com","subject":"[PATCH v4 6/6] cocci: generalize \"unused\" rule to cover more than \"strbuf\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-05T13:47:00Z","receivedAt":"2022-07-05T14:02:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Generalize the newly added \"unused.cocci\" rule to find more than just\n\"struct strbuf\", let's have it find the same unused patterns for\n\"struct string_list\", as well as other code that uses\nsimilar-looking *_{release,clear,free}() and {release,clear,free}_*()\nfunctions.\n\nWe're intentionally loose in accepting e.g. a \"strbuf_init(&sb)\"\nfollowed by a \"string_list_clear(&sb, 0)\". It's assumed that the\ncompiler will catch any such invalid code, i.e. that our\nconstructors/destructors don't take a \"void *\".\n\nSee [1] for example of code that would be covered by the\n\"get_worktrees()\" part of this rule. We'd still need work that the\nseries is based on (we were passing \"worktrees\" to a function), but\ncould now do the change in [1] automatically.\n\n1. https://lore.kernel.org/git/Yq6eJFUPPTv%2Fzc0o@coredump.intra.peff.net/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/repack.c | 2 --\n contrib/coccinelle/tests/unused.c | 27 +++++++++++++++++++++++++++\n contrib/coccinelle/tests/unused.res | 15 +++++++++++++++\n contrib/coccinelle/unused.cocci | 19 +++++++++++++++----\n 4 files changed, 57 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 4a7ae4cf489..482b66f57d6 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -727,7 +727,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct string_list_item *item;\n \tstruct string_list names = STRING_LIST_INIT_DUP;\n-\tstruct string_list rollback = STRING_LIST_INIT_NODUP;\n \tstruct string_list existing_nonkept_packs = STRING_LIST_INIT_DUP;\n \tstruct string_list existing_kept_packs = STRING_LIST_INIT_DUP;\n \tstruct pack_geometry *geometry = NULL;\n@@ -1117,7 +1116,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t}\n \n \tstring_list_clear(&names, 0);\n-\tstring_list_clear(&rollback, 0);\n \tstring_list_clear(&existing_nonkept_packs, 0);\n \tstring_list_clear(&existing_kept_packs, 0);\n \tclear_pack_geometry(geometry);\ndiff --git a/contrib/coccinelle/tests/unused.c b/contrib/coccinelle/tests/unused.c\nindex 495ae58ccf1..8294d734ba4 100644\n--- a/contrib/coccinelle/tests/unused.c\n+++ b/contrib/coccinelle/tests/unused.c\n@@ -53,3 +53,30 @@ void test_strbuf(void)\n \t\treturn;\n \tstrbuf_release(&sb8);\n }\n+\n+void test_other(void)\n+{\n+\tstruct string_list l = STRING_LIST_INIT_DUP;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tstring_list_clear(&l, 0);\n+\tstring_list_clear(&sb, 0);\n+}\n+\n+void test_worktrees(void)\n+{\n+\tstruct worktree **w1 = get_worktrees();\n+\tstruct worktree **w2 = get_worktrees();\n+\tstruct worktree **w3;\n+\tstruct worktree **w4;\n+\n+\tw3 = get_worktrees();\n+\tw4 = get_worktrees();\n+\n+\tuse_it(w4);\n+\n+\tfree_worktrees(w1);\n+\tfree_worktrees(w2);\n+\tfree_worktrees(w3);\n+\tfree_worktrees(w4);\n+}\ndiff --git a/contrib/coccinelle/tests/unused.res b/contrib/coccinelle/tests/unused.res\nindex b3b71053ed6..6d3e745683c 100644\n--- a/contrib/coccinelle/tests/unused.res\n+++ b/contrib/coccinelle/tests/unused.res\n@@ -28,3 +28,18 @@ void test_strbuf(void)\n \tif (when_strict())\n \t\treturn;\n }\n+\n+void test_other(void)\n+{\n+}\n+\n+void test_worktrees(void)\n+{\n+\tstruct worktree **w4;\n+\n+\tw4 = get_worktrees();\n+\n+\tuse_it(w4);\n+\n+\tfree_worktrees(w4);\n+}\ndiff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci\nindex 56530498d17..d84046f82ea 100644\n--- a/contrib/coccinelle/unused.cocci\n+++ b/contrib/coccinelle/unused.cocci\n@@ -4,10 +4,13 @@\n @@\n type T;\n identifier I;\n-constant INIT_MACRO =~ \"^STRBUF_INIT$\";\n+// STRBUF_INIT, but also e.g. STRING_LIST_INIT_DUP (so no anchoring)\n+constant INIT_MACRO =~ \"_INIT\";\n identifier MALLOC1 =~ \"^x?[mc]alloc$\";\n-identifier INIT_CALL1 =~ \"^strbuf_init$\";\n-identifier REL1 =~ \"^strbuf_(release|reset)$\";\n+identifier INIT_ASSIGN1 =~ \"^get_worktrees$\";\n+identifier INIT_CALL1 =~ \"^[a-z_]*_init$\";\n+identifier REL1 =~ \"^[a-z_]*_(release|reset|clear|free)$\";\n+identifier REL2 =~ \"^(release|clear|free)_[a-z_]*$\";\n @@\n \n (\n@@ -18,15 +21,23 @@ identifier REL1 =~ \"^strbuf_(release|reset)$\";\n - T I = INIT_MACRO;\n |\n - T I = MALLOC1(...);\n+|\n+- T I = INIT_ASSIGN1(...);\n )\n \n <... when != \\( I \\| &I \\)\n (\n - \\( INIT_CALL1 \\)( \\( I \\| &I \\), ...);\n |\n+- I = \\( INIT_ASSIGN1 \\)(...);\n+|\n - I = MALLOC1(...);\n )\n ...>\n \n-- \\( REL1 \\)( \\( &I \\| I \\) );\n+(\n+- \\( REL1 \\| REL2 \\)( \\( I \\| &I \\), ...);\n+|\n+- \\( REL1 \\| REL2 \\)( \\( &I \\| I \\) );\n+)\n ... when != \\( I \\| &I \\)\n-- \n2.37.0.913.g50625c3f077\n\n"},{"id":"458540","messageId":"xmqqedyyghsc.fsf@gitster.g","threadId":"57903","inReplyTo":"cover-v4-0.6-00000000000-20220705T134033Z-avarab@gmail.com","subject":"Re: [PATCH v4 0/6] add and apply a rule to find \"unused\" init+free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-06T19:30:59Z","receivedAt":"2022-07-06T19:31:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> builtin/merge.c | 4 --\n\nThis finds and re-fixes what has already been fixed in another\ntopic, which is unfortunate, but it is minor. If this topic takes\nlonger before en/merge-restore-to-pristine graduates, it may want to\nget rerolled to avoid duplication, but for now I'll queue the series\nas-is.\n\nThanks.\n"},{"id":"458761","messageId":"YsvwLIfjZffUWOP/@coredump.intra.peff.net","threadId":"57903","inReplyTo":"cover-v4-0.6-00000000000-20220705T134033Z-avarab@gmail.com","subject":"Re: [PATCH v4 0/6] add and apply a rule to find \"unused\" init+free","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-11T09:41:00Z","receivedAt":"2022-07-11T10:31:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 05, 2022 at 03:46:54PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> This series adds a coccinelle rule to find and remove code where the\n> only reference to a variable in a given function is to malloc() &\n> free() it, where \"malloc\" and \"free\" also match\n> \"strbuf_init/strbuf_release\", and then later in the series anything\n> that looks like a init/free pattern.\n\nAs before, I'm perfectly happy with the actual transformations here.\n\n> Changes since v3[1]\n> \n> * Add a \"coccicheck-test\" target in an early and new patch, the\n> structure mirrors that of coccinelle.git's own tests. As the\n> diffstat shows we have a *.c and *.res file which is C code\n> before/after a *.cocci rule is applied.\n\nI have mixed feelings on this. I'm not opposed per se, and I even like\nthe fact that it provides examples of what we expect a rule to do. But I\nworry about a world where the cost of using coccinelle goes up because\nnow transformations need to have tests along with them. The idea of\ncocci is to make your life a little simpler and more efficient when\nfixing up the code base (as opposed to perl, etc). But if it comes with\na bunch of boilerplate tasks, that might no longer be the case. I'm\nalready often on the fence about using cocci just because of the hours\nI've sunk into debugging and puzzling things out.\n\nI dunno. Maybe the bar is higher for stuff like this that we expect to\ncontinue to find problems as time goes on (because it is not \"here is a\ntransition\", but \"this is a common and easy mistake to make\").\n\nSo again, I'm not really opposed to this patch in particular, but I want\nto express caution at the direction we might be going, and at applying\nnew rules over-zealously.\n\n> * We now catch init/reset patterns as well as init/release fully\n> (i.e. for the \"struct strbuf\" early on)\n\nYeah, that makes sense.\n\n-Peff\n"},{"id":"458769","messageId":"220711.86wncj7w8p.gmgdl@evledraar.gmail.com","threadId":"57903","inReplyTo":"YsvwLIfjZffUWOP/@coredump.intra.peff.net","subject":"Re: [PATCH v4 0/6] add and apply a rule to find \"unused\" init+free","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-11T10:54:03Z","receivedAt":"2022-07-11T11:25:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jul 11 2022, Jeff King wrote:\n\n> On Tue, Jul 05, 2022 at 03:46:54PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> This series adds a coccinelle rule to find and remove code where the\n>> only reference to a variable in a given function is to malloc() &\n>> free() it, where \"malloc\" and \"free\" also match\n>> \"strbuf_init/strbuf_release\", and then later in the series anything\n>> that looks like a init/free pattern.\n>\n> As before, I'm perfectly happy with the actual transformations here.\n\nThanks!\n\n>> Changes since v3[1]\n>> \n>> * Add a \"coccicheck-test\" target in an early and new patch, the\n>> structure mirrors that of coccinelle.git's own tests. As the\n>> diffstat shows we have a *.c and *.res file which is C code\n>> before/after a *.cocci rule is applied.\n>\n> I have mixed feelings on this. I'm not opposed per se, and I even like\n> the fact that it provides examples of what we expect a rule to do. But I\n> worry about a world where the cost of using coccinelle goes up because\n> now transformations need to have tests along with them. The idea of\n> cocci is to make your life a little simpler and more efficient when\n> fixing up the code base (as opposed to perl, etc).\n\nThat's fair, and I suppose we could add some really complex tests that\ne.g. include the original headers.\n\nBut right now these just work on stand-alone *.c files with no includes,\nand the runtime is thus trivial. So I thought it was OK to run it along\nwith the main job.\n\n> But if it comes with a bunch of boilerplate tasks, that might no\n> longer be the case. I'm already often on the fence about using cocci\n> just because of the hours I've sunk into debugging and puzzling things\n> out.\n>\n> I dunno. Maybe the bar is higher for stuff like this that we expect to\n> continue to find problems as time goes on (because it is not \"here is a\n> transition\", but \"this is a common and easy mistake to make\").\n>\n> So again, I'm not really opposed to this patch in particular, but I want\n> to express caution at the direction we might be going, and at applying\n> new rules over-zealously.\n\nYeah, we definitely need to be more careful with \"ongoing\" rules like\nthese, but hopefully between the tests & lack of false positives this\none looks sane...\n\n>> * We now catch init/reset patterns as well as init/release fully\n>> (i.e. for the \"struct strbuf\" early on)\n>\n> Yeah, that makes sense.\n>\n> -Peff\n\n"}]}