git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org, Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH 2/2] branch: drop unused worktrees variable
Date: Mon, 20 Jun 2022 21:09:27 +0200	[thread overview]
Message-ID: <220620.865ykvw2l4.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <Yq6eJFUPPTv/zc0o@coredump.intra.peff.net>


On Sat, Jun 18 2022, Jeff King wrote:

> 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.

  reply	other threads:[~2022-06-20 19:35 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-19  3:52 [PATCH 0/2] extra cleanups on top of ds/branch-checked-out Jeff King
2022-06-19  3:53 ` [PATCH 1/2] fetch: stop passing around unused worktrees variable Jeff King
2022-06-19  3:55 ` [PATCH 2/2] branch: drop " Jeff King
2022-06-20 19:09   ` Ævar Arnfjörð Bjarmason [this message]
2022-07-01 18:24     ` Jeff King
2022-06-21 15:56   ` Junio C Hamano
2022-06-19 18:12 ` [PATCH 0/2] extra cleanups on top of ds/branch-checked-out Derrick Stolee

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=220620.865ykvw2l4.gmgdl@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).