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

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

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

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

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

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/fetch.c b/builtin/fetch.c
index ac29c2b1ae34df79f8a2bd62318dc714a1f37e24..8a3ae71fed0550c706c60e6c0b1d68d1323b9af1 100644 (file)
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1113,7 +1113,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
                              struct fetch_head *fetch_head, struct worktree **worktrees)
 {
        int url_len, i, rc = 0;
-       struct strbuf note = STRBUF_INIT, err = STRBUF_INIT;
+       struct strbuf note = STRBUF_INIT;
        const char *what, *kind;
        struct ref *rm;
        char *url;
@@ -1281,7 +1281,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 
  abort:
        strbuf_release(&note);
-       strbuf_release(&err);
        free(url);
        return rc;
 }

diff --git a/builtin/merge.c b/builtin/merge.c
index d9784d4891c92bbc27f7e0ecdcf1d17b7bcded5b..23170f2d2a644c2ace8899bf32b4ea642dbf2384 100644 (file)
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -375,7 +375,6 @@ static void reset_hard(const struct object_id *oid, int verbose)
 static void restore_state(const struct object_id *head,
                          const struct object_id *stash)
 {
-       struct strbuf sb = STRBUF_INIT;
        const char *args[] = { "stash", "apply", NULL, NULL };
 
        if (is_null_oid(stash))
@@ -391,7 +390,6 @@ static void restore_state(const struct object_id *head,
         */
        run_command_v_opt(args, RUN_GIT_CMD);
 
-       strbuf_release(&sb);
        refresh_cache(REFRESH_QUIET);
 }
 
@@ -502,7 +500,6 @@ static void merge_name(const char *remote, struct strbuf *msg)
 {
        struct commit *remote_head;
        struct object_id branch_head;
-       struct strbuf buf = STRBUF_INIT;
        struct strbuf bname = STRBUF_INIT;
        struct merge_remote_desc *desc;
        const char *ptr;
@@ -590,7 +587,6 @@ static void merge_name(const char *remote, struct strbuf *msg)
                oid_to_hex(&remote_head->object.oid), remote);
 cleanup:
        free(found_ref);
-       strbuf_release(&buf);
        strbuf_release(&bname);
 }
 

diff --git a/contrib/coccinelle/tests/unused.c b/contrib/coccinelle/tests/unused.c
new file mode 100644 (file)
index 0000000..495ae58
--- /dev/null
+++ b/contrib/coccinelle/tests/unused.c
@@ -0,0 +1,55 @@
+void test_strbuf(void)
+{
+       struct strbuf sb1 = STRBUF_INIT;
+       struct strbuf sb2 = STRBUF_INIT;
+       struct strbuf sb3 = STRBUF_INIT;
+       struct strbuf sb4 = STRBUF_INIT;
+       struct strbuf sb5;
+       struct strbuf sb6 = { 0 };
+       struct strbuf sb7 = STRBUF_INIT;
+       struct strbuf sb8 = STRBUF_INIT;
+       struct strbuf *sp1;
+       struct strbuf *sp2;
+       struct strbuf *sp3;
+       struct strbuf *sp4 = xmalloc(sizeof(struct strbuf));
+       struct strbuf *sp5 = xmalloc(sizeof(struct strbuf));
+       struct strbuf *sp6 = xmalloc(sizeof(struct strbuf));
+       struct strbuf *sp7;
+
+       strbuf_init(&sb5, 0);
+       strbuf_init(sp1, 0);
+       strbuf_init(sp2, 0);
+       strbuf_init(sp3, 0);
+       strbuf_init(sp4, 0);
+       strbuf_init(sp5, 0);
+       strbuf_init(sp6, 0);
+       strbuf_init(sp7, 0);
+       sp7 = xmalloc(sizeof(struct strbuf));
+
+       use_before(&sb3);
+       use_as_str("%s", sb7.buf);
+       use_as_str("%s", sp1->buf);
+       use_as_str("%s", sp6->buf);
+       pass_pp(&sp3);
+
+       strbuf_release(&sb1);
+       strbuf_reset(&sb2);
+       strbuf_release(&sb3);
+       strbuf_release(&sb4);
+       strbuf_release(&sb5);
+       strbuf_release(&sb6);
+       strbuf_release(&sb7);
+       strbuf_release(sp1);
+       strbuf_release(sp2);
+       strbuf_release(sp3);
+       strbuf_release(sp4);
+       strbuf_release(sp5);
+       strbuf_release(sp6);
+       strbuf_release(sp7);
+
+       use_after(&sb4);
+
+       if (when_strict())
+               return;
+       strbuf_release(&sb8);
+}

diff --git a/contrib/coccinelle/tests/unused.res b/contrib/coccinelle/tests/unused.res
new file mode 100644 (file)
index 0000000..b3b7105
--- /dev/null
+++ b/contrib/coccinelle/tests/unused.res
@@ -0,0 +1,30 @@
+void test_strbuf(void)
+{
+       struct strbuf sb3 = STRBUF_INIT;
+       struct strbuf sb4 = STRBUF_INIT;
+       struct strbuf sb7 = STRBUF_INIT;
+       struct strbuf *sp1;
+       struct strbuf *sp3;
+       struct strbuf *sp6 = xmalloc(sizeof(struct strbuf));
+       strbuf_init(sp1, 0);
+       strbuf_init(sp3, 0);
+       strbuf_init(sp6, 0);
+
+       use_before(&sb3);
+       use_as_str("%s", sb7.buf);
+       use_as_str("%s", sp1->buf);
+       use_as_str("%s", sp6->buf);
+       pass_pp(&sp3);
+
+       strbuf_release(&sb3);
+       strbuf_release(&sb4);
+       strbuf_release(&sb7);
+       strbuf_release(sp1);
+       strbuf_release(sp3);
+       strbuf_release(sp6);
+
+       use_after(&sb4);
+
+       if (when_strict())
+               return;
+}

diff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci
new file mode 100644 (file)
index 0000000..5653049
--- /dev/null
+++ b/contrib/coccinelle/unused.cocci
@@ -0,0 +1,32 @@
+// This rule finds sequences of "unused" declerations and uses of a
+// variable, where "unused" is defined to include only calling the
+// equivalent of alloc, init & free functions on the variable.
+@@
+type T;
+identifier I;
+constant INIT_MACRO =~ "^STRBUF_INIT$";
+identifier MALLOC1 =~ "^x?[mc]alloc$";
+identifier INIT_CALL1 =~ "^strbuf_init$";
+identifier REL1 =~ "^strbuf_(release|reset)$";
+@@
+
+(
+- T I;
+|
+- T I = { 0 };
+|
+- T I = INIT_MACRO;
+|
+- T I = MALLOC1(...);
+)
+
+<... when != \( I \| &I \)
+(
+- \( INIT_CALL1 \)( \( I \| &I \), ...);
+|
+- I = MALLOC1(...);
+)
+...>
+
+- \( REL1 \)( \( &I \| I \) );
+  ... when != \( I \| &I \)

diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 28176914e57b91e823d6197a926909a87cfe1d16..97e71fe19cd5e503a19241439e21e1dbdcbc0e35 100644 (file)
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -687,7 +687,7 @@ static int cmd_diagnose(int argc, const char **argv)
        int stdout_fd = -1, archiver_fd = -1;
        time_t now = time(NULL);
        struct tm tm;
-       struct strbuf path = STRBUF_INIT, buf = STRBUF_INIT;
+       struct strbuf buf = STRBUF_INIT;
        int res = 0;
 
        argc = parse_options(argc, argv, NULL, options,
@@ -779,7 +779,6 @@ diagnose_cleanup:
        free(argv_copy);
        strvec_clear(&archiver_args);
        strbuf_release(&zip_path);
-       strbuf_release(&path);
        strbuf_release(&buf);
 
        return res;

diff --git a/diff.c b/diff.c
index e71cf758861bd7596ce122611a4c92fe6b27d8c5..d4290615aaa5ec9822b4961891971427e7f41437 100644 (file)
--- a/diff.c
+++ b/diff.c
@@ -1289,7 +1289,6 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 {
        static const char *nneof = " No newline at end of file\n";
        const char *context, *reset, *set, *set_sign, *meta, *fraginfo;
-       struct strbuf sb = STRBUF_INIT;
 
        enum diff_symbol s = eds->s;
        const char *line = eds->line;
@@ -1521,7 +1520,6 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
        default:
                BUG("unknown diff symbol");
        }
-       strbuf_release(&sb);
 }
 
 static void emit_diff_symbol(struct diff_options *o, enum diff_symbol s,