Re: [PATCH 2/2] branch: drop unused worktrees variable

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 20, 2022, 19:09 UTC
Message-ID: <220620.865ykvw2l4.gmgdl@evledraar.gmail.com>
In-Reply-To: <Yq6eJFUPPTv/zc0o@coredump.intra.peff.net>

On Sat, Jun 18 2022, Jeff King wrote:

Show 15 quoted lines

> After b489b9d9aa (branch: use branch_checked_out() when deleting refs,
> 2022-06-14), we no longer look at our local "worktrees" variable, since
> branch_checked_out() handles it under the hood. The compiler didn't
> notice the unused variable because we call functions to initialize and
> free it (so it's not totally unused, it just doesn't do anything
> useful).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> It would be neat if there was some way to mark a function as "this is
> just allocating a structure, with no useful side effects" and another as
> "this is just freeing", which would let the compiler notice that we
> don't do anything useful with the structure in between the two. I have a
> feeling adding such annotations might be more work than occasionally
> finding and cleaning up such useless variables, though. :)

There is, at least with coccinelle. Perhaps the compiler can be made smart enough, but I haven't found a way to massage it to do that.

I had this patch the other day, which basically does this, but it didn't get any interest / wasn't picked up by Junio: https://lore.kernel.org/git/patch-1.1-7d90f26b73f-20220520T115426Z-avarab@gmail.com/

I didn't think to add a rule for free_worktrees() specifically, but with
your 1/2 applied we find this:
	
	$ cat contrib/coccinelle/unused.cocci
	// Unused decl + assignment + release()
	@@
	identifier I;
	type T;
	@@
	- T I;
	  <+...
	- I = get_worktrees();
	  ... when != \( I \| &I \)
	- free_worktrees(I);
	  ...+>

	$ spatch --sp-file contrib/coccinelle/unused.cocci --all-includes builtin/branch.c
	init_defs_builtins: /usr/bin/../lib/coccinelle/standard.h
	HANDLING: builtin/branch.c
	diff = 
	--- builtin/branch.c
	+++ /tmp/cocci-output-16842-0b6416-branch.c
	@@ -204,7 +204,6 @@ static void delete_branch_config(const c
	 static int delete_branches(int argc, const char **argv, int force, int kinds,
	                           int quiet)
	 {
	-       struct worktree **worktrees;
	        struct commit *head_rev = NULL;
	        struct object_id oid;
	        char *name = NULL;
	@@ -242,8 +241,6 @@ static int delete_branches(int argc, con
	                        die(_("Couldn't look up commit object for HEAD"));
	        }
	 
	-       worktrees = get_worktrees();
	-
	        for (i = 0; i < argc; i++, strbuf_reset(&bname)) {
	                char *target = NULL;
	                int flags = 0;
	@@ -314,7 +311,6 @@ static int delete_branches(int argc, con
	 
	        free(name);
	        strbuf_release(&bname);
	-       free_worktrees(worktrees);
	 
	        return ret;
	 }

There's a bug in that rule I didn't quite figure out, the "<+..." line needs the equivalent of the "when !=", but if I add that the "when" will also match the "I = get_worktrees()" line.

I.e. if we had a "foo(worktrees)" line before the "worktrees = get_worktrees()" we'd still remove these lines, but we don't want that. It just needs to do the appropriate cocci for "don't match it if you see this variable, unless the line matches...".

Of coures that only helps after your 1/2, so maybe there's not much value in it for your case, i.e. it won't be reaching across functions.

But as that patch shows we could relatively easily be spotting dead code.