{"thread":{"id":"58034","subject":"[PATCH 0/2] extra cleanups on top of ds/branch-checked-out","startedAt":"2022-06-19T03:53:40Z","lastAt":"2022-07-01T18:24:17Z","messageCount":7,"participants":["Jeff King","Derrick Stolee","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"457518","messageId":"Yq6deQf96g8M5wdg@coredump.intra.peff.net","threadId":"58034","inReplyTo":null,"subject":"[PATCH 0/2] extra cleanups on top of ds/branch-checked-out","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-06-19T03:52:25Z","receivedAt":"2022-06-19T03:53:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here are a few extra cleanups on top of ds/branch-checked-out; that\ntopic made some local \"worktrees\" variables obsolete, but didn't get rid\nof them.\n\nThe first was detected by my local -Wunused-parameter topic. The second\nis a similar case that the compiler doesn't happen to notice, but which\nI went digging for after seeing the first. I think that should be the\nextent of it. There's a third caller in validate_new_branchname(), but\nthe series already got rid of its worktrees variable.\n\n  [1/2]: fetch: stop passing around unused worktrees variable\n  [2/2]: branch: drop unused worktrees variable\n\n builtin/branch.c |  4 ----\n builtin/fetch.c  | 24 +++++++++---------------\n 2 files changed, 9 insertions(+), 19 deletions(-)\n\n-Peff\n"},{"id":"457519","messageId":"Yq6dr6HaILpqJBdM@coredump.intra.peff.net","threadId":"58034","inReplyTo":"Yq6deQf96g8M5wdg@coredump.intra.peff.net","subject":"[PATCH 1/2] fetch: stop passing around unused worktrees variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-06-19T03:53:19Z","receivedAt":"2022-06-19T03:53:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In 12d47e3b1f (fetch: use new branch_checked_out() and add tests,\n2022-06-14), fetch's update_local_ref() function stopped using its\n\"worktrees\" parameter. It doesn't need it, since the\nbranch_checked_out() function examines the global worktrees under the\nhood.\n\nSo we can not only drop the unused parameter from that function, but\nalso from its entire call chain. And as we do so all the way up to\ndo_fetch(), we can see that nobody uses it at all, and we can drop the\nlocal variable there entirely.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/fetch.c | 24 +++++++++---------------\n 1 file changed, 9 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex fa473fc394..51d9c33f1e 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -881,8 +881,7 @@ static void format_display(struct strbuf *display, char code,\n static int update_local_ref(struct ref *ref,\n \t\t\t    struct ref_transaction *transaction,\n \t\t\t    const char *remote, const struct ref *remote_ref,\n-\t\t\t    struct strbuf *display, int summary_width,\n-\t\t\t    struct worktree **worktrees)\n+\t\t\t    struct strbuf *display, int summary_width)\n {\n \tstruct commit *current = NULL, *updated;\n \tconst char *pretty_ref = prettify_refname(ref->name);\n@@ -1107,7 +1106,7 @@ N_(\"it took %.2f seconds to check forced updates; you can use\\n\"\n static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t      int connectivity_checked,\n \t\t\t      struct ref_transaction *transaction, struct ref *ref_map,\n-\t\t\t      struct fetch_head *fetch_head, struct worktree **worktrees)\n+\t\t\t      struct fetch_head *fetch_head)\n {\n \tint url_len, i, rc = 0;\n \tstruct strbuf note = STRBUF_INIT, err = STRBUF_INIT;\n@@ -1237,8 +1236,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\tstrbuf_reset(&note);\n \t\t\tif (ref) {\n \t\t\t\trc |= update_local_ref(ref, transaction, what,\n-\t\t\t\t\t\t       rm, &note, summary_width,\n-\t\t\t\t\t\t       worktrees);\n+\t\t\t\t\t\t       rm, &note, summary_width);\n \t\t\t\tfree(ref);\n \t\t\t} else if (write_fetch_head || dry_run) {\n \t\t\t\t/*\n@@ -1329,8 +1327,7 @@ static int check_exist_and_connected(struct ref *ref_map)\n static int fetch_and_consume_refs(struct transport *transport,\n \t\t\t\t  struct ref_transaction *transaction,\n \t\t\t\t  struct ref *ref_map,\n-\t\t\t\t  struct fetch_head *fetch_head,\n-\t\t\t\t  struct worktree **worktrees)\n+\t\t\t\t  struct fetch_head *fetch_head)\n {\n \tint connectivity_checked = 1;\n \tint ret;\n@@ -1353,7 +1350,7 @@ static int fetch_and_consume_refs(struct transport *transport,\n \ttrace2_region_enter(\"fetch\", \"consume_refs\", the_repository);\n \tret = store_updated_refs(transport->url, transport->remote->name,\n \t\t\t\t connectivity_checked, transaction, ref_map,\n-\t\t\t\t fetch_head, worktrees);\n+\t\t\t\t fetch_head);\n \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n \n out:\n@@ -1543,8 +1540,7 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)\n static int backfill_tags(struct transport *transport,\n \t\t\t struct ref_transaction *transaction,\n \t\t\t struct ref *ref_map,\n-\t\t\t struct fetch_head *fetch_head,\n-\t\t\t struct worktree **worktrees)\n+\t\t\t struct fetch_head *fetch_head)\n {\n \tint retcode, cannot_reuse;\n \n@@ -1565,7 +1561,7 @@ static int backfill_tags(struct transport *transport,\n \ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n \ttransport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n \ttransport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);\n-\tretcode = fetch_and_consume_refs(transport, transaction, ref_map, fetch_head, worktrees);\n+\tretcode = fetch_and_consume_refs(transport, transaction, ref_map, fetch_head);\n \n \tif (gsecondary) {\n \t\ttransport_disconnect(gsecondary);\n@@ -1586,7 +1582,6 @@ static int do_fetch(struct transport *transport,\n \tstruct transport_ls_refs_options transport_ls_refs_options =\n \t\tTRANSPORT_LS_REFS_OPTIONS_INIT;\n \tint must_list_refs = 1;\n-\tstruct worktree **worktrees = get_worktrees();\n \tstruct fetch_head fetch_head = { 0 };\n \tstruct strbuf err = STRBUF_INIT;\n \n@@ -1677,7 +1672,7 @@ static int do_fetch(struct transport *transport,\n \t\t\tretcode = 1;\n \t}\n \n-\tif (fetch_and_consume_refs(transport, transaction, ref_map, &fetch_head, worktrees)) {\n+\tif (fetch_and_consume_refs(transport, transaction, ref_map, &fetch_head)) {\n \t\tretcode = 1;\n \t\tgoto cleanup;\n \t}\n@@ -1700,7 +1695,7 @@ static int do_fetch(struct transport *transport,\n \t\t\t * the transaction and don't commit anything.\n \t\t\t */\n \t\t\tif (backfill_tags(transport, transaction, tags_ref_map,\n-\t\t\t\t\t  &fetch_head, worktrees))\n+\t\t\t\t\t  &fetch_head))\n \t\t\t\tretcode = 1;\n \t\t}\n \n@@ -1785,7 +1780,6 @@ static int do_fetch(struct transport *transport,\n \tclose_fetch_head(&fetch_head);\n \tstrbuf_release(&err);\n \tfree_refs(ref_map);\n-\tfree_worktrees(worktrees);\n \treturn retcode;\n }\n \n-- \n2.37.0.rc1.385.g5f9aa3aa78\n\n"},{"id":"457520","messageId":"Yq6eJFUPPTv/zc0o@coredump.intra.peff.net","threadId":"58034","inReplyTo":"Yq6deQf96g8M5wdg@coredump.intra.peff.net","subject":"[PATCH 2/2] branch: drop unused worktrees variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-06-19T03:55:16Z","receivedAt":"2022-06-19T03:55:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After b489b9d9aa (branch: use branch_checked_out() when deleting refs,\n2022-06-14), we no longer look at our local \"worktrees\" variable, since\nbranch_checked_out() handles it under the hood. The compiler didn't\nnotice the unused variable because we call functions to initialize and\nfree it (so it's not totally unused, it just doesn't do anything\nuseful).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIt would be neat if there was some way to mark a function as \"this is\njust allocating a structure, with no useful side effects\" and another as\n\"this is just freeing\", which would let the compiler notice that we\ndon't do anything useful with the structure in between the two. I have a\nfeeling adding such annotations might be more work than occasionally\nfinding and cleaning up such useless variables, though. :)\n\n builtin/branch.c | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex f875952e7b..55cd9a6e99 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -204,7 +204,6 @@ static void delete_branch_config(const char *branchname)\n static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\t   int quiet)\n {\n-\tstruct worktree **worktrees;\n \tstruct commit *head_rev = NULL;\n \tstruct object_id oid;\n \tchar *name = NULL;\n@@ -242,8 +241,6 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\tdie(_(\"Couldn't look up commit object for HEAD\"));\n \t}\n \n-\tworktrees = get_worktrees();\n-\n \tfor (i = 0; i < argc; i++, strbuf_reset(&bname)) {\n \t\tchar *target = NULL;\n \t\tint flags = 0;\n@@ -314,7 +311,6 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \n \tfree(name);\n \tstrbuf_release(&bname);\n-\tfree_worktrees(worktrees);\n \n \treturn ret;\n }\n-- \n2.37.0.rc1.385.g5f9aa3aa78\n"},{"id":"457538","messageId":"b23d209f-80bf-c741-dc47-68af48fa3220@github.com","threadId":"58034","inReplyTo":"Yq6deQf96g8M5wdg@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] extra cleanups on top of ds/branch-checked-out","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-19T18:12:24Z","receivedAt":"2022-06-19T18:12:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/18/2022 11:52 PM, Jeff King wrote:\n> Here are a few extra cleanups on top of ds/branch-checked-out; that\n> topic made some local \"worktrees\" variables obsolete, but didn't get rid\n> of them.\n> \n> The first was detected by my local -Wunused-parameter topic. The second\n> is a similar case that the compiler doesn't happen to notice, but which\n> I went digging for after seeing the first. I think that should be the\n> extent of it. There's a third caller in validate_new_branchname(), but\n> the series already got rid of its worktrees variable.\n> \n>   [1/2]: fetch: stop passing around unused worktrees variable\n>   [2/2]: branch: drop unused worktrees variable\n\nGood catch. Thanks for identifying these. You are right that it would\nbe nice if a compiler could identify these \"create and free, no use\"\nsituations.\n\nThanks,\n-Stolee\n"},{"id":"457570","messageId":"220620.865ykvw2l4.gmgdl@evledraar.gmail.com","threadId":"58034","inReplyTo":"Yq6eJFUPPTv/zc0o@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] branch: drop unused worktrees variable","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-20T19:09:27Z","receivedAt":"2022-06-20T19:35:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jun 18 2022, Jeff King wrote:\n\n> After b489b9d9aa (branch: use branch_checked_out() when deleting refs,\n> 2022-06-14), we no longer look at our local \"worktrees\" variable, since\n> branch_checked_out() handles it under the hood. The compiler didn't\n> notice the unused variable because we call functions to initialize and\n> free it (so it's not totally unused, it just doesn't do anything\n> useful).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> It would be neat if there was some way to mark a function as \"this is\n> just allocating a structure, with no useful side effects\" and another as\n> \"this is just freeing\", which would let the compiler notice that we\n> don't do anything useful with the structure in between the two. I have a\n> feeling adding such annotations might be more work than occasionally\n> finding and cleaning up such useless variables, though. :)\n\nThere is, at least with coccinelle. Perhaps the compiler can be made\nsmart enough, but I haven't found a way to massage it to do that.\n\nI had this patch the other day, which basically does this, but it didn't\nget any interest / wasn't picked up by Junio:\nhttps://lore.kernel.org/git/patch-1.1-7d90f26b73f-20220520T115426Z-avarab@gmail.com/\n\nI didn't think to add a rule for free_worktrees() specifically, but with\nyour 1/2 applied we find this:\n\t\n\t$ cat contrib/coccinelle/unused.cocci\n\t// Unused decl + assignment + release()\n\t@@\n\tidentifier I;\n\ttype T;\n\t@@\n\t- T I;\n\t  <+...\n\t- I = get_worktrees();\n\t  ... when != \\( I \\| &I \\)\n\t- free_worktrees(I);\n\t  ...+>\n\n\t$ spatch --sp-file contrib/coccinelle/unused.cocci --all-includes builtin/branch.c\n\tinit_defs_builtins: /usr/bin/../lib/coccinelle/standard.h\n\tHANDLING: builtin/branch.c\n\tdiff = \n\t--- builtin/branch.c\n\t+++ /tmp/cocci-output-16842-0b6416-branch.c\n\t@@ -204,7 +204,6 @@ static void delete_branch_config(const c\n\t static int delete_branches(int argc, const char **argv, int force, int kinds,\n\t                           int quiet)\n\t {\n\t-       struct worktree **worktrees;\n\t        struct commit *head_rev = NULL;\n\t        struct object_id oid;\n\t        char *name = NULL;\n\t@@ -242,8 +241,6 @@ static int delete_branches(int argc, con\n\t                        die(_(\"Couldn't look up commit object for HEAD\"));\n\t        }\n\t \n\t-       worktrees = get_worktrees();\n\t-\n\t        for (i = 0; i < argc; i++, strbuf_reset(&bname)) {\n\t                char *target = NULL;\n\t                int flags = 0;\n\t@@ -314,7 +311,6 @@ static int delete_branches(int argc, con\n\t \n\t        free(name);\n\t        strbuf_release(&bname);\n\t-       free_worktrees(worktrees);\n\t \n\t        return ret;\n\t }\n\nThere's a bug in that rule I didn't quite figure out, the \"<+...\" line\nneeds the equivalent of the \"when !=\", but if I add that the \"when\" will\nalso match the \"I = get_worktrees()\" line.\n\nI.e. if we had a \"foo(worktrees)\" line before the \"worktrees =\nget_worktrees()\" we'd still remove these lines, but we don't want\nthat. It just needs to do the appropriate cocci for \"don't match it if\nyou see this variable, unless the line matches...\".\n\nOf coures that only helps after your 1/2, so maybe there's not much\nvalue in it for your case, i.e. it won't be reaching across functions.\n\nBut as that patch shows we could relatively easily be spotting dead\ncode.\n"},{"id":"457623","messageId":"xmqqzgi6hux8.fsf@gitster.g","threadId":"58034","inReplyTo":"Yq6eJFUPPTv/zc0o@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] branch: drop unused worktrees variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-21T15:56:35Z","receivedAt":"2022-06-21T15:58:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> After b489b9d9aa (branch: use branch_checked_out() when deleting refs,\n> 2022-06-14), we no longer look at our local \"worktrees\" variable, since\n> branch_checked_out() handles it under the hood. The compiler didn't\n> notice the unused variable because we call functions to initialize and\n> free it (so it's not totally unused, it just doesn't do anything\n> useful).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> It would be neat if there was some way to mark a function as \"this is\n> just allocating a structure, with no useful side effects\" and another as\n> \"this is just freeing\", which would let the compiler notice that we\n> don't do anything useful with the structure in between the two. I have a\n> feeling adding such annotations might be more work than occasionally\n> finding and cleaning up such useless variables, though. :)\n\nAlso it may be tricky to write correctly ;-)\n\nI recently got rid of a Coccinelle rule I wrote quite a while ago\nthat was suggesting a completely bogus rewrite, and found it quite\nsatisfying.  After that experience, I got allergic to the idea of\nhaving to make sure a mechanical rewrite suggested by the tool if it\ngets too large X-<.\n\nFor this particular pattern, presumably we won't have too many of\nthem, though.\n\nThanks.\n\n>  builtin/branch.c | 4 ----\n>  1 file changed, 4 deletions(-)\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index f875952e7b..55cd9a6e99 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -204,7 +204,6 @@ static void delete_branch_config(const char *branchname)\n>  static int delete_branches(int argc, const char **argv, int force, int kinds,\n>  \t\t\t   int quiet)\n>  {\n> -\tstruct worktree **worktrees;\n>  \tstruct commit *head_rev = NULL;\n>  \tstruct object_id oid;\n>  \tchar *name = NULL;\n> @@ -242,8 +241,6 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n>  \t\t\tdie(_(\"Couldn't look up commit object for HEAD\"));\n>  \t}\n>  \n> -\tworktrees = get_worktrees();\n> -\n>  \tfor (i = 0; i < argc; i++, strbuf_reset(&bname)) {\n>  \t\tchar *target = NULL;\n>  \t\tint flags = 0;\n> @@ -314,7 +311,6 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n>  \n>  \tfree(name);\n>  \tstrbuf_release(&bname);\n> -\tfree_worktrees(worktrees);\n>  \n>  \treturn ret;\n>  }\n"},{"id":"458404","messageId":"Yr87zb6TBTYlwCrw@coredump.intra.peff.net","threadId":"58034","inReplyTo":"220620.865ykvw2l4.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/2] branch: drop unused worktrees variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-01T18:24:13Z","receivedAt":"2022-07-01T18:24:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 20, 2022 at 09:09:27PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> I.e. if we had a \"foo(worktrees)\" line before the \"worktrees =\n> get_worktrees()\" we'd still remove these lines, but we don't want\n> that. It just needs to do the appropriate cocci for \"don't match it if\n> you see this variable, unless the line matches...\".\n> \n> Of coures that only helps after your 1/2, so maybe there's not much\n> value in it for your case, i.e. it won't be reaching across functions.\n\nThe cases that reach across functions are actually easier to find,\nbecause the compiler knows that the function does not even look at its\nparameter. And so you can remove it, and then remove it from the caller,\nand so on.\n\nOf course, that requires us to squash all of the -Wunused-parameter\nnoise. I've done that, but it's like 100 ugly patches. I haven't had the\ntime to polish it all up, but perhaps I'll at least send the start of\nthe transition soon.\n\n-Peff\n"}]}