[PATCH 0/2] extra cleanups on top of ds/branch-checked-out

[PATCH 0/2] extra cleanups on top of ds/branch-checked-out

[Jeff King]

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

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

[Jeff King]

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(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index fa473fc394..51d9c33f1e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -881,8 +881,7 @@ static void format_display(struct strbuf *display, char code,
 static int update_local_ref(struct ref *ref,
 			    struct ref_transaction *transaction,
 			    const char *remote, const struct ref *remote_ref,
-			    struct strbuf *display, int summary_width,
-			    struct worktree **worktrees)
+			    struct strbuf *display, int summary_width)
 {
 	struct commit *current = NULL, *updated;
 	const char *pretty_ref = prettify_refname(ref->name);
@@ -1107,7 +1106,7 @@ N_("it took %.2f seconds to check forced updates; you can use\n"
 static int store_updated_refs(const char *raw_url, const char *remote_name,
 			      int connectivity_checked,
 			      struct ref_transaction *transaction, struct ref *ref_map,
-			      struct fetch_head *fetch_head, struct worktree **worktrees)
+			      struct fetch_head *fetch_head)
 {
 	int url_len, i, rc = 0;
 	struct strbuf note = STRBUF_INIT, err = STRBUF_INIT;
@@ -1237,8 +1236,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 			strbuf_reset(&note);
 			if (ref) {
 				rc |= update_local_ref(ref, transaction, what,
-						       rm, &note, summary_width,
-						       worktrees);
+						       rm, &note, summary_width);
 				free(ref);
 			} else if (write_fetch_head || dry_run) {
 				/*
@@ -1329,8 +1327,7 @@ static int check_exist_and_connected(struct ref *ref_map)
 static int fetch_and_consume_refs(struct transport *transport,
 				  struct ref_transaction *transaction,
 				  struct ref *ref_map,
-				  struct fetch_head *fetch_head,
-				  struct worktree **worktrees)
+				  struct fetch_head *fetch_head)
 {
 	int connectivity_checked = 1;
 	int ret;
@@ -1353,7 +1350,7 @@ static int fetch_and_consume_refs(struct transport *transport,
 	trace2_region_enter("fetch", "consume_refs", the_repository);
 	ret = store_updated_refs(transport->url, transport->remote->name,
 				 connectivity_checked, transaction, ref_map,
-				 fetch_head, worktrees);
+				 fetch_head);
 	trace2_region_leave("fetch", "consume_refs", the_repository);
 
 out:
@@ -1543,8 +1540,7 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
 static int backfill_tags(struct transport *transport,
 			 struct ref_transaction *transaction,
 			 struct ref *ref_map,
-			 struct fetch_head *fetch_head,
-			 struct worktree **worktrees)
+			 struct fetch_head *fetch_head)
 {
 	int retcode, cannot_reuse;
 
@@ -1565,7 +1561,7 @@ static int backfill_tags(struct transport *transport,
 	transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
 	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
 	transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
-	retcode = fetch_and_consume_refs(transport, transaction, ref_map, fetch_head, worktrees);
+	retcode = fetch_and_consume_refs(transport, transaction, ref_map, fetch_head);
 
 	if (gsecondary) {
 		transport_disconnect(gsecondary);
@@ -1586,7 +1582,6 @@ static int do_fetch(struct transport *transport,
 	struct transport_ls_refs_options transport_ls_refs_options =
 		TRANSPORT_LS_REFS_OPTIONS_INIT;
 	int must_list_refs = 1;
-	struct worktree **worktrees = get_worktrees();
 	struct fetch_head fetch_head = { 0 };
 	struct strbuf err = STRBUF_INIT;
 
@@ -1677,7 +1672,7 @@ static int do_fetch(struct transport *transport,
 			retcode = 1;
 	}
 
-	if (fetch_and_consume_refs(transport, transaction, ref_map, &fetch_head, worktrees)) {
+	if (fetch_and_consume_refs(transport, transaction, ref_map, &fetch_head)) {
 		retcode = 1;
 		goto cleanup;
 	}
@@ -1700,7 +1695,7 @@ static int do_fetch(struct transport *transport,
 			 * the transaction and don't commit anything.
 			 */
 			if (backfill_tags(transport, transaction, tags_ref_map,
-					  &fetch_head, worktrees))
+					  &fetch_head))
 				retcode = 1;
 		}
 
@@ -1785,7 +1780,6 @@ static int do_fetch(struct transport *transport,
 	close_fetch_head(&fetch_head);
 	strbuf_release(&err);
 	free_refs(ref_map);
-	free_worktrees(worktrees);
 	return retcode;
 }
 
-- 
2.37.0.rc1.385.g5f9aa3aa78

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

[Jeff King]

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(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index f875952e7b..55cd9a6e99 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -204,7 +204,6 @@ static void delete_branch_config(const char *branchname)
 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, const char **argv, int force, int kinds,
 			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, const char **argv, int force, int kinds,
 
 	free(name);
 	strbuf_release(&bname);
-	free_worktrees(worktrees);
 
 	return ret;
 }
-- 
2.37.0.rc1.385.g5f9aa3aa78

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

[Ævar Arnfjörð Bjarmason]

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.

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

[Jeff King]

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

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

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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.

>  builtin/branch.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index f875952e7b..55cd9a6e99 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -204,7 +204,6 @@ static void delete_branch_config(const char *branchname)
>  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, const char **argv, int force, int kinds,
>  			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, const char **argv, int force, int kinds,
>  
>  	free(name);
>  	strbuf_release(&bname);
> -	free_worktrees(worktrees);
>  
>  	return ret;
>  }

Re: [PATCH 0/2] extra cleanups on top of ds/branch-checked-out

[Derrick Stolee]

On 6/18/2022 11:52 PM, Jeff King wrote:
> 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

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