git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/2] extra cleanups on top of ds/branch-checked-out
@ 2022-06-19  3:52 Jeff King
  2022-06-19  3:53 ` [PATCH 1/2] fetch: stop passing around unused worktrees variable Jeff King
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Jeff King @ 2022-06-19  3:52 UTC (permalink / raw)
  To: git; +Cc: Derrick Stolee

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] fetch: stop passing around unused worktrees variable
  2022-06-19  3:52 [PATCH 0/2] extra cleanups on top of ds/branch-checked-out Jeff King
@ 2022-06-19  3:53 ` Jeff King
  2022-06-19  3:55 ` [PATCH 2/2] branch: drop " Jeff King
  2022-06-19 18:12 ` [PATCH 0/2] extra cleanups on top of ds/branch-checked-out Derrick Stolee
  2 siblings, 0 replies; 6+ messages in thread
From: Jeff King @ 2022-06-19  3:53 UTC (permalink / raw)
  To: git; +Cc: Derrick Stolee

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/2] branch: drop unused worktrees variable
  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 ` Jeff King
  2022-06-20 19:09   ` Ævar Arnfjörð Bjarmason
  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
  2 siblings, 2 replies; 6+ messages in thread
From: Jeff King @ 2022-06-19  3:55 UTC (permalink / raw)
  To: git; +Cc: Derrick Stolee

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/2] extra cleanups on top of ds/branch-checked-out
  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-19 18:12 ` Derrick Stolee
  2 siblings, 0 replies; 6+ messages in thread
From: Derrick Stolee @ 2022-06-19 18:12 UTC (permalink / raw)
  To: Jeff King, git

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] branch: drop unused worktrees variable
  2022-06-19  3:55 ` [PATCH 2/2] branch: drop " Jeff King
@ 2022-06-20 19:09   ` Ævar Arnfjörð Bjarmason
  2022-06-21 15:56   ` Junio C Hamano
  1 sibling, 0 replies; 6+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-20 19:09 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Derrick Stolee


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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] branch: drop unused worktrees variable
  2022-06-19  3:55 ` [PATCH 2/2] branch: drop " Jeff King
  2022-06-20 19:09   ` Ævar Arnfjörð Bjarmason
@ 2022-06-21 15:56   ` Junio C Hamano
  1 sibling, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2022-06-21 15:56 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Derrick Stolee

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;
>  }

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-06-21 16:01 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).