{"thread":{"id":"53723","subject":"[PATCH 0/2] drop specialized knowledge from generic worktree code","startedAt":"2020-06-19T23:36:33Z","lastAt":"2020-06-22T17:34:18Z","messageCount":4,"participants":["Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"400244","messageId":"20200619233544.42025-1-sunshine@sunshineco.com","threadId":"53723","inReplyTo":null,"subject":"[PATCH 0/2] drop specialized knowledge from generic worktree code","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-19T23:35:42Z","receivedAt":"2020-06-19T23:36:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"This patch series removes specialized knowledge from the libified\nworktree code about how front-end \"git worktree list\" wants worktrees\nsorted, and instead makes it the responsibility of \"git worktree list\"\nitself to do the sorting.\n\nIt is built atop es/worktree-duplicate-paths since that series adds\nanother caller of get_worktrees() which this series touches.\n\nA possible argument against this patch series is that some other entity\nmay someday want worktrees sorted in the same fashion as \"git worktree\nlist\", however, that seems a case of YAGNI.\n\nEric Sunshine (2):\n  worktree: drop get_worktrees() special-purpose sorting option\n  worktree: drop get_worktrees() unused 'flags' argument\n\n branch.c                  |  2 +-\n builtin/branch.c          |  2 +-\n builtin/config.c          |  2 +-\n builtin/fsck.c            |  2 +-\n builtin/reflog.c          |  2 +-\n builtin/worktree.c        | 32 ++++++++++++++++++++++++++------\n ref-filter.c              |  2 +-\n revision.c                |  4 ++--\n t/helper/test-ref-store.c |  2 +-\n worktree.c                | 20 +++-----------------\n worktree.h                | 11 +++--------\n 11 files changed, 41 insertions(+), 40 deletions(-)\n\n-- \n2.27.0.221.g4d328a12d9\n\n"},{"id":"400245","messageId":"20200619233544.42025-2-sunshine@sunshineco.com","threadId":"53723","inReplyTo":"20200619233544.42025-1-sunshine@sunshineco.com","subject":"[PATCH 1/2] worktree: drop get_worktrees() special-purpose sorting option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-19T23:35:43Z","receivedAt":"2020-06-19T23:36:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Of all the clients of get_worktrees(), only \"git worktree list\" wants\nthe list sorted in a very specific way; other clients simply don't care\nabout the order. Rather than imbuing get_worktrees() with special\nknowledge about how various clients -- now and in the future -- may want\nthe list sorted, drop the sorting capability altogether and make it the\nclient's responsibility to sort the list if needed.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n builtin/worktree.c | 22 +++++++++++++++++++++-\n worktree.c         | 14 --------------\n worktree.h         |  9 ++-------\n 3 files changed, 23 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 1238b6bab1..13b94d36d9 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -697,6 +697,23 @@ static void measure_widths(struct worktree **wt, int *abbrev, int *maxlen)\n \t}\n }\n \n+static int pathcmp(const void *a_, const void *b_)\n+{\n+\tconst struct worktree *const *a = a_;\n+\tconst struct worktree *const *b = b_;\n+\treturn fspathcmp((*a)->path, (*b)->path);\n+}\n+\n+static void pathsort(struct worktree **wt)\n+{\n+\tint n = 0;\n+\tstruct worktree **p = wt;\n+\n+\twhile (*p++)\n+\t\tn++;\n+\tQSORT(wt, n, pathcmp);\n+}\n+\n static int list(int ac, const char **av, const char *prefix)\n {\n \tint porcelain = 0;\n@@ -710,9 +727,12 @@ static int list(int ac, const char **av, const char *prefix)\n \tif (ac)\n \t\tusage_with_options(worktree_usage, options);\n \telse {\n-\t\tstruct worktree **worktrees = get_worktrees(GWT_SORT_LINKED);\n+\t\tstruct worktree **worktrees = get_worktrees(0);\n \t\tint path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;\n \n+\t\t/* sort worktrees by path but keep main worktree at top */\n+\t\tpathsort(worktrees + 1);\n+\n \t\tif (!porcelain)\n \t\t\tmeasure_widths(worktrees, &abbrev, &path_maxlen);\n \ndiff --git a/worktree.c b/worktree.c\nindex ee82235f26..8f24fbdc1e 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -123,13 +123,6 @@ static void mark_current_worktree(struct worktree **worktrees)\n \tfree(git_dir);\n }\n \n-static int compare_worktree(const void *a_, const void *b_)\n-{\n-\tconst struct worktree *const *a = a_;\n-\tconst struct worktree *const *b = b_;\n-\treturn fspathcmp((*a)->path, (*b)->path);\n-}\n-\n struct worktree **get_worktrees(unsigned flags)\n {\n \tstruct worktree **list = NULL;\n@@ -161,13 +154,6 @@ struct worktree **get_worktrees(unsigned flags)\n \tALLOC_GROW(list, counter + 1, alloc);\n \tlist[counter] = NULL;\n \n-\tif (flags & GWT_SORT_LINKED)\n-\t\t/*\n-\t\t * don't sort the first item (main worktree), which will\n-\t\t * always be the first\n-\t\t */\n-\t\tQSORT(list + 1, counter - 1, compare_worktree);\n-\n \tmark_current_worktree(list);\n \treturn list;\n }\ndiff --git a/worktree.h b/worktree.h\nindex d242a6e71c..bd2235abe0 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -18,17 +18,12 @@ struct worktree {\n \tint lock_reason_valid; /* private */\n };\n \n-/* Functions for acting on the information about worktrees. */\n-\n-#define GWT_SORT_LINKED (1 << 0) /* keeps linked worktrees sorted */\n-\n /*\n  * Get the worktrees.  The primary worktree will always be the first returned,\n- * and linked worktrees will be pointed to by 'next' in each subsequent\n- * worktree.  No specific ordering is done on the linked worktrees.\n+ * and linked worktrees will follow in no particular order.\n  *\n  * The caller is responsible for freeing the memory from the returned\n- * worktree(s).\n+ * worktrees by calling free_worktrees().\n  */\n struct worktree **get_worktrees(unsigned flags);\n \n-- \n2.27.0.221.g4d328a12d9\n\n"},{"id":"400246","messageId":"20200619233544.42025-3-sunshine@sunshineco.com","threadId":"53723","inReplyTo":"20200619233544.42025-1-sunshine@sunshineco.com","subject":"[PATCH 2/2] worktree: drop get_worktrees() unused 'flags' argument","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-19T23:35:44Z","receivedAt":"2020-06-19T23:36:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"get_worktrees() accepts a 'flags' argument, however, there are no\nexisting flags (the lone flag GWT_SORT_LINKED was recently retired) and\nno behavior which can be tweaked. Therefore, drop the 'flags' argument.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n branch.c                  |  2 +-\n builtin/branch.c          |  2 +-\n builtin/config.c          |  2 +-\n builtin/fsck.c            |  2 +-\n builtin/reflog.c          |  2 +-\n builtin/worktree.c        | 12 ++++++------\n ref-filter.c              |  2 +-\n revision.c                |  4 ++--\n t/helper/test-ref-store.c |  2 +-\n worktree.c                |  6 +++---\n worktree.h                |  2 +-\n 11 files changed, 19 insertions(+), 19 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 2d9e7675a6..7095f78058 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -370,7 +370,7 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref,\n \t\t\t\t      const char *logmsg)\n {\n \tint ret = 0;\n-\tstruct worktree **worktrees = get_worktrees(0);\n+\tstruct worktree **worktrees = get_worktrees();\n \tint i;\n \n \tfor (i = 0; worktrees[i]; i++) {\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex accb61b1aa..d1649e4595 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -468,7 +468,7 @@ static void print_current_branch_name(void)\n \n static void reject_rebase_or_bisect_branch(const char *target)\n {\n-\tstruct worktree **worktrees = get_worktrees(0);\n+\tstruct worktree **worktrees = get_worktrees();\n \tint i;\n \n \tfor (i = 0; worktrees[i]; i++) {\ndiff --git a/builtin/config.c b/builtin/config.c\nindex ee4aef6a35..5e39f61885 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -672,7 +672,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tgiven_config_source.file = git_pathdup(\"config\");\n \t\tgiven_config_source.scope = CONFIG_SCOPE_LOCAL;\n \t} else if (use_worktree_config) {\n-\t\tstruct worktree **worktrees = get_worktrees(0);\n+\t\tstruct worktree **worktrees = get_worktrees();\n \t\tif (repository_format_worktree_config)\n \t\t\tgiven_config_source.file = git_pathdup(\"config.worktree\");\n \t\telse if (worktrees[0] && worktrees[1])\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex f02cbdb439..82332e0e86 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -577,7 +577,7 @@ static void get_default_heads(void)\n \n \tfor_each_rawref(fsck_handle_ref, NULL);\n \n-\tworktrees = get_worktrees(0);\n+\tworktrees = get_worktrees();\n \tfor (p = worktrees; *p; p++) {\n \t\tstruct worktree *wt = *p;\n \t\tstruct strbuf ref = STRBUF_INIT;\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 52ecf6d43c..ca1d8079f3 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -615,7 +615,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tint i;\n \n \t\tmemset(&collected, 0, sizeof(collected));\n-\t\tworktrees = get_worktrees(0);\n+\t\tworktrees = get_worktrees();\n \t\tfor (p = worktrees; *p; p++) {\n \t\t\tif (!all_worktrees && !(*p)->is_current)\n \t\t\t\tcontinue;\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 13b94d36d9..f0cbdef718 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -325,7 +325,7 @@ static int add_worktree(const char *path, const char *refname,\n \tstruct strbuf sb_name = STRBUF_INIT;\n \tstruct worktree **worktrees;\n \n-\tworktrees = get_worktrees(0);\n+\tworktrees = get_worktrees();\n \tcheck_candidate_path(path, opts->force, worktrees, \"add\");\n \tfree_worktrees(worktrees);\n \tworktrees = NULL;\n@@ -727,7 +727,7 @@ static int list(int ac, const char **av, const char *prefix)\n \tif (ac)\n \t\tusage_with_options(worktree_usage, options);\n \telse {\n-\t\tstruct worktree **worktrees = get_worktrees(0);\n+\t\tstruct worktree **worktrees = get_worktrees();\n \t\tint path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;\n \n \t\t/* sort worktrees by path but keep main worktree at top */\n@@ -761,7 +761,7 @@ static int lock_worktree(int ac, const char **av, const char *prefix)\n \tif (ac != 1)\n \t\tusage_with_options(worktree_usage, options);\n \n-\tworktrees = get_worktrees(0);\n+\tworktrees = get_worktrees();\n \twt = find_worktree(worktrees, prefix, av[0]);\n \tif (!wt)\n \t\tdie(_(\"'%s' is not a working tree\"), av[0]);\n@@ -794,7 +794,7 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)\n \tif (ac != 1)\n \t\tusage_with_options(worktree_usage, options);\n \n-\tworktrees = get_worktrees(0);\n+\tworktrees = get_worktrees();\n \twt = find_worktree(worktrees, prefix, av[0]);\n \tif (!wt)\n \t\tdie(_(\"'%s' is not a working tree\"), av[0]);\n@@ -868,7 +868,7 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \tstrbuf_addstr(&dst, path);\n \tfree(path);\n \n-\tworktrees = get_worktrees(0);\n+\tworktrees = get_worktrees();\n \twt = find_worktree(worktrees, prefix, av[0]);\n \tif (!wt)\n \t\tdie(_(\"'%s' is not a working tree\"), av[0]);\n@@ -994,7 +994,7 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n \tif (ac != 1)\n \t\tusage_with_options(worktree_usage, options);\n \n-\tworktrees = get_worktrees(0);\n+\tworktrees = get_worktrees();\n \twt = find_worktree(worktrees, prefix, av[0]);\n \tif (!wt)\n \t\tdie(_(\"'%s' is not a working tree\"), av[0]);\ndiff --git a/ref-filter.c b/ref-filter.c\nindex bf7b70299b..8447cb09be 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1579,7 +1579,7 @@ static void lazy_init_worktree_map(void)\n \tif (ref_to_worktree_map.worktrees)\n \t\treturn;\n \n-\tref_to_worktree_map.worktrees = get_worktrees(0);\n+\tref_to_worktree_map.worktrees = get_worktrees();\n \thashmap_init(&(ref_to_worktree_map.map), ref_to_worktree_map_cmpfnc, NULL, 0);\n \tpopulate_worktree_map(&(ref_to_worktree_map.map), ref_to_worktree_map.worktrees);\n }\ndiff --git a/revision.c b/revision.c\nindex ebb4d2a0f2..f20d545ed5 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1609,7 +1609,7 @@ static void add_other_reflogs_to_pending(struct all_refs_cb *cb)\n {\n \tstruct worktree **worktrees, **p;\n \n-\tworktrees = get_worktrees(0);\n+\tworktrees = get_worktrees();\n \tfor (p = worktrees; *p; p++) {\n \t\tstruct worktree *wt = *p;\n \n@@ -1697,7 +1697,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)\n \tif (revs->single_worktree)\n \t\treturn;\n \n-\tworktrees = get_worktrees(0);\n+\tworktrees = get_worktrees();\n \tfor (p = worktrees; *p; p++) {\n \t\tstruct worktree *wt = *p;\n \t\tstruct index_state istate = { NULL };\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 799fc00aa1..759e69dc54 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -37,7 +37,7 @@ static const char **get_store(const char **argv, struct ref_store **refs)\n \n \t\t*refs = get_submodule_ref_store(gitdir);\n \t} else if (skip_prefix(argv[0], \"worktree:\", &gitdir)) {\n-\t\tstruct worktree **p, **worktrees = get_worktrees(0);\n+\t\tstruct worktree **p, **worktrees = get_worktrees();\n \n \t\tfor (p = worktrees; *p; p++) {\n \t\t\tstruct worktree *wt = *p;\ndiff --git a/worktree.c b/worktree.c\nindex 8f24fbdc1e..ff9b7d847f 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -123,7 +123,7 @@ static void mark_current_worktree(struct worktree **worktrees)\n \tfree(git_dir);\n }\n \n-struct worktree **get_worktrees(unsigned flags)\n+struct worktree **get_worktrees(void)\n {\n \tstruct worktree **list = NULL;\n \tstruct strbuf path = STRBUF_INIT;\n@@ -404,7 +404,7 @@ const struct worktree *find_shared_symref(const char *symref,\n \n \tif (worktrees)\n \t\tfree_worktrees(worktrees);\n-\tworktrees = get_worktrees(0);\n+\tworktrees = get_worktrees();\n \n \tfor (i = 0; worktrees[i]; i++) {\n \t\tstruct worktree *wt = worktrees[i];\n@@ -563,7 +563,7 @@ int other_head_refs(each_ref_fn fn, void *cb_data)\n \tstruct worktree **worktrees, **p;\n \tint ret = 0;\n \n-\tworktrees = get_worktrees(0);\n+\tworktrees = get_worktrees();\n \tfor (p = worktrees; *p; p++) {\n \t\tstruct worktree *wt = *p;\n \t\tstruct object_id oid;\ndiff --git a/worktree.h b/worktree.h\nindex bd2235abe0..516744c433 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -25,7 +25,7 @@ struct worktree {\n  * The caller is responsible for freeing the memory from the returned\n  * worktrees by calling free_worktrees().\n  */\n-struct worktree **get_worktrees(unsigned flags);\n+struct worktree **get_worktrees(void);\n \n /*\n  * Returns 1 if linked worktrees exist, 0 otherwise.\n-- \n2.27.0.221.g4d328a12d9\n\n"},{"id":"400288","messageId":"xmqq7dvzdmy2.fsf@gitster.c.googlers.com","threadId":"53723","inReplyTo":"20200619233544.42025-1-sunshine@sunshineco.com","subject":"Re: [PATCH 0/2] drop specialized knowledge from generic worktree code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-22T17:34:13Z","receivedAt":"2020-06-22T17:34:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> This patch series removes specialized knowledge from the libified\n> worktree code about how front-end \"git worktree list\" wants worktrees\n> sorted, and instead makes it the responsibility of \"git worktree list\"\n> itself to do the sorting.\n>\n> It is built atop es/worktree-duplicate-paths since that series adds\n> another caller of get_worktrees() which this series touches.\n>\n> A possible argument against this patch series is that some other entity\n> may someday want worktrees sorted in the same fashion as \"git worktree\n> list\", however, that seems a case of YAGNI.\n\nIs anything hurting to have the \"mostly extra\" flag and its handling\nthat is added only to support \"worktree list\"?  That would be\nanother possible argument against this clean-up.\n\nHaving said that, I think this is good.\n\nThanks.  Will queue.\n"}]}