patch, 2 partsextra cleanups on top of ds/branch-checked-out

Here are a few extra cleanups on top of ds/branch-checked-out; that topic made some local "worktrees" variables obsolete, but didn't get rid of them.

The first was detected by my local -Wunused-parameter topic. The second is a similar case that the compiler doesn't happen to notice, but which I went digging for after seeing the first. I think that should be the extent of it. There's a third caller in validate_new_branchname(), but the series already got rid of its worktrees variable.

  [1/2]: fetch: stop passing around unused worktrees variable
  [2/2]: branch: drop unused worktrees variable

 builtin/branch.c |  4 ----
 builtin/fetch.c  | 24 +++++++++---------------
 2 files changed, 9 insertions(+), 19 deletions(-)

-Peff

In 12d47e3b1f (fetch: use new branch_checked_out() and add tests, 2022-06-14), fetch's update_local_ref() function stopped using its "worktrees" parameter. It doesn't need it, since the branch_checked_out() function examines the global worktrees under the hood.

So we can not only drop the unused parameter from that function, but also from its entire call chain. And as we do so all the way up to do_fetch(), we can see that nobody uses it at all, and we can drop the local variable there entirely.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fetch.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

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

 builtin/branch.c | 4 ----
 1 file changed, 4 deletions(-)

On Sat, Jun 18 2022, Jeff King wrote:

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.

On Mon, Jun 20, 2022 at 09:09:27PM +0200, Ævar Arnfjörð Bjarmason wrote:

The cases that reach across functions are actually easier to find, because the compiler knows that the function does not even look at its parameter. And so you can remove it, and then remove it from the caller, and so on.

Of course, that requires us to squash all of the -Wunused-parameter noise. I've done that, but it's like 100 ugly patches. I haven't had the time to polish it all up, but perhaps I'll at least send the start of the transition soon.

-Peff

Jeff King <peff@peff.net> writes:

Also it may be tricky to write correctly ;-)

I recently got rid of a Coccinelle rule I wrote quite a while ago that was suggesting a completely bogus rewrite, and found it quite satisfying. After that experience, I got allergic to the idea of having to make sure a mechanical rewrite suggested by the tool if it gets too large X-<.

For this particular pattern, presumably we won't have too many of them, though.

Thanks.

On 6/18/2022 11:52 PM, Jeff King wrote:

Good catch. Thanks for identifying these. You are right that it would be nice if a compiler could identify these "create and free, no use" situations.

Thanks, -Stolee