threads / patch / 53723

patch, 2 partsdrop specialized knowledge from generic worktree code

Subject: [PATCH 0/2] drop specialized knowledge from generic worktree code

## tl;dr

4 messages between Jun 19, 2020 and Jun 22, 2020. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Eric Sunshine· Jun 19, 2020, 23:35 UTC · lore

This patch series removes specialized knowledge from the libified worktree code about how front-end "git worktree list" wants worktrees sorted, and instead makes it the responsibility of "git worktree list" itself to do the sorting.

It is built atop es/worktree-duplicate-paths since that series adds another caller of get_worktrees() which this series touches.

A possible argument against this patch series is that some other entity may someday want worktrees sorted in the same fashion as "git worktree list", however, that seems a case of YAGNI.

Eric Sunshine (2):
  worktree: drop get_worktrees() special-purpose sorting option
  worktree: drop get_worktrees() unused 'flags' argument
 branch.c                  |  2 +-
 builtin/branch.c          |  2 +-
 builtin/config.c          |  2 +-
 builtin/fsck.c            |  2 +-
 builtin/reflog.c          |  2 +-
 builtin/worktree.c        | 32 ++++++++++++++++++++++++++------
 ref-filter.c              |  2 +-
 revision.c                |  4 ++--
 t/helper/test-ref-store.c |  2 +-
 worktree.c                | 20 +++-----------------
 worktree.h                | 11 +++--------
 11 files changed, 41 insertions(+), 40 deletions(-)
-- 
2.27.0.221.g4d328a12d9
Eric Sunshine· Jun 19, 2020, 23:35 UTC · re: Eric Sunshine · lore

[PATCH 1/2] worktree: drop get_worktrees() special-purpose sorting option

Of all the clients of get_worktrees(), only "git worktree list" wants the list sorted in a very specific way; other clients simply don't care about the order. Rather than imbuing get_worktrees() with special knowledge about how various clients -- now and in the future -- may want the list sorted, drop the sorting capability altogether and make it the client's responsibility to sort the list if needed.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c | 22 +++++++++++++++++++++-
 worktree.c         | 14 --------------
 worktree.h         |  9 ++-------
 3 files changed, 23 insertions(+), 22 deletions(-)
Show changes to 3 files +23 −22

builtin/worktree.c, worktree.c, worktree.h

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 1238b6bab1..13b94d36d9 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -697,6 +697,23 @@ static void measure_widths(struct worktree **wt, int *abbrev, int *maxlen)
 	}
 }
 
+static int pathcmp(const void *a_, const void *b_)
+{
+	const struct worktree *const *a = a_;
+	const struct worktree *const *b = b_;
+	return fspathcmp((*a)->path, (*b)->path);
+}
+
+static void pathsort(struct worktree **wt)
+{
+	int n = 0;
+	struct worktree **p = wt;
+
+	while (*p++)
+		n++;
+	QSORT(wt, n, pathcmp);
+}
+
 static int list(int ac, const char **av, const char *prefix)
 {
 	int porcelain = 0;
@@ -710,9 +727,12 @@ static int list(int ac, const char **av, const char *prefix)
 	if (ac)
 		usage_with_options(worktree_usage, options);
 	else {
-		struct worktree **worktrees = get_worktrees(GWT_SORT_LINKED);
+		struct worktree **worktrees = get_worktrees(0);
 		int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;
 
+		/* sort worktrees by path but keep main worktree at top */
+		pathsort(worktrees + 1);
+
 		if (!porcelain)
 			measure_widths(worktrees, &abbrev, &path_maxlen);
 
diff --git a/worktree.c b/worktree.c
index ee82235f26..8f24fbdc1e 100644
--- a/worktree.c
+++ b/worktree.c
@@ -123,13 +123,6 @@ static void mark_current_worktree(struct worktree **worktrees)
 	free(git_dir);
 }
 
-static int compare_worktree(const void *a_, const void *b_)
-{
-	const struct worktree *const *a = a_;
-	const struct worktree *const *b = b_;
-	return fspathcmp((*a)->path, (*b)->path);
-}
-
 struct worktree **get_worktrees(unsigned flags)
 {
 	struct worktree **list = NULL;
@@ -161,13 +154,6 @@ struct worktree **get_worktrees(unsigned flags)
 	ALLOC_GROW(list, counter + 1, alloc);
 	list[counter] = NULL;
 
-	if (flags & GWT_SORT_LINKED)
-		/*
-		 * don't sort the first item (main worktree), which will
-		 * always be the first
-		 */
-		QSORT(list + 1, counter - 1, compare_worktree);
-
 	mark_current_worktree(list);
 	return list;
 }
diff --git a/worktree.h b/worktree.h
index d242a6e71c..bd2235abe0 100644
--- a/worktree.h
+++ b/worktree.h
@@ -18,17 +18,12 @@ struct worktree {
 	int lock_reason_valid; /* private */
 };
 
-/* Functions for acting on the information about worktrees. */
-
-#define GWT_SORT_LINKED (1 << 0) /* keeps linked worktrees sorted */
-
 /*
  * Get the worktrees.  The primary worktree will always be the first returned,
- * and linked worktrees will be pointed to by 'next' in each subsequent
- * worktree.  No specific ordering is done on the linked worktrees.
+ * and linked worktrees will follow in no particular order.
  *
  * The caller is responsible for freeing the memory from the returned
- * worktree(s).
+ * worktrees by calling free_worktrees().
  */
 struct worktree **get_worktrees(unsigned flags);
 

-- 
2.27.0.221.g4d328a12d9
Eric Sunshine· Jun 19, 2020, 23:35 UTC · re: Eric Sunshine · lore

[PATCH 2/2] worktree: drop get_worktrees() unused 'flags' argument

get_worktrees() accepts a 'flags' argument, however, there are no existing flags (the lone flag GWT_SORT_LINKED was recently retired) and no behavior which can be tweaked. Therefore, drop the 'flags' argument.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 branch.c                  |  2 +-
 builtin/branch.c          |  2 +-
 builtin/config.c          |  2 +-
 builtin/fsck.c            |  2 +-
 builtin/reflog.c          |  2 +-
 builtin/worktree.c        | 12 ++++++------
 ref-filter.c              |  2 +-
 revision.c                |  4 ++--
 t/helper/test-ref-store.c |  2 +-
 worktree.c                |  6 +++---
 worktree.h                |  2 +-
 11 files changed, 19 insertions(+), 19 deletions(-)
Show changes to 11 files +19 −19

branch.c, builtin/branch.c, builtin/config.c, builtin/fsck.c, builtin/reflog.c, builtin/worktree.c, ref-filter.c, revision.c, t/helper/test-ref-store.c, worktree.c, worktree.h

diff --git a/branch.c b/branch.c
index 2d9e7675a6..7095f78058 100644
--- a/branch.c
+++ b/branch.c
@@ -370,7 +370,7 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref,
 				      const char *logmsg)
 {
 	int ret = 0;
-	struct worktree **worktrees = get_worktrees(0);
+	struct worktree **worktrees = get_worktrees();
 	int i;
 
 	for (i = 0; worktrees[i]; i++) {
diff --git a/builtin/branch.c b/builtin/branch.c
index accb61b1aa..d1649e4595 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -468,7 +468,7 @@ static void print_current_branch_name(void)
 
 static void reject_rebase_or_bisect_branch(const char *target)
 {
-	struct worktree **worktrees = get_worktrees(0);
+	struct worktree **worktrees = get_worktrees();
 	int i;
 
 	for (i = 0; worktrees[i]; i++) {
diff --git a/builtin/config.c b/builtin/config.c
index ee4aef6a35..5e39f61885 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -672,7 +672,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		given_config_source.file = git_pathdup("config");
 		given_config_source.scope = CONFIG_SCOPE_LOCAL;
 	} else if (use_worktree_config) {
-		struct worktree **worktrees = get_worktrees(0);
+		struct worktree **worktrees = get_worktrees();
 		if (repository_format_worktree_config)
 			given_config_source.file = git_pathdup("config.worktree");
 		else if (worktrees[0] && worktrees[1])
diff --git a/builtin/fsck.c b/builtin/fsck.c
index f02cbdb439..82332e0e86 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -577,7 +577,7 @@ static void get_default_heads(void)
 
 	for_each_rawref(fsck_handle_ref, NULL);
 
-	worktrees = get_worktrees(0);
+	worktrees = get_worktrees();
 	for (p = worktrees; *p; p++) {
 		struct worktree *wt = *p;
 		struct strbuf ref = STRBUF_INIT;
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 52ecf6d43c..ca1d8079f3 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -615,7 +615,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		int i;
 
 		memset(&collected, 0, sizeof(collected));
-		worktrees = get_worktrees(0);
+		worktrees = get_worktrees();
 		for (p = worktrees; *p; p++) {
 			if (!all_worktrees && !(*p)->is_current)
 				continue;
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 13b94d36d9..f0cbdef718 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -325,7 +325,7 @@ static int add_worktree(const char *path, const char *refname,
 	struct strbuf sb_name = STRBUF_INIT;
 	struct worktree **worktrees;
 
-	worktrees = get_worktrees(0);
+	worktrees = get_worktrees();
 	check_candidate_path(path, opts->force, worktrees, "add");
 	free_worktrees(worktrees);
 	worktrees = NULL;
@@ -727,7 +727,7 @@ static int list(int ac, const char **av, const char *prefix)
 	if (ac)
 		usage_with_options(worktree_usage, options);
 	else {
-		struct worktree **worktrees = get_worktrees(0);
+		struct worktree **worktrees = get_worktrees();
 		int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;
 
 		/* sort worktrees by path but keep main worktree at top */
@@ -761,7 +761,7 @@ static int lock_worktree(int ac, const char **av, const char *prefix)
 	if (ac != 1)
 		usage_with_options(worktree_usage, options);
 
-	worktrees = get_worktrees(0);
+	worktrees = get_worktrees();
 	wt = find_worktree(worktrees, prefix, av[0]);
 	if (!wt)
 		die(_("'%s' is not a working tree"), av[0]);
@@ -794,7 +794,7 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
 	if (ac != 1)
 		usage_with_options(worktree_usage, options);
 
-	worktrees = get_worktrees(0);
+	worktrees = get_worktrees();
 	wt = find_worktree(worktrees, prefix, av[0]);
 	if (!wt)
 		die(_("'%s' is not a working tree"), av[0]);
@@ -868,7 +868,7 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 	strbuf_addstr(&dst, path);
 	free(path);
 
-	worktrees = get_worktrees(0);
+	worktrees = get_worktrees();
 	wt = find_worktree(worktrees, prefix, av[0]);
 	if (!wt)
 		die(_("'%s' is not a working tree"), av[0]);
@@ -994,7 +994,7 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
 	if (ac != 1)
 		usage_with_options(worktree_usage, options);
 
-	worktrees = get_worktrees(0);
+	worktrees = get_worktrees();
 	wt = find_worktree(worktrees, prefix, av[0]);
 	if (!wt)
 		die(_("'%s' is not a working tree"), av[0]);
diff --git a/ref-filter.c b/ref-filter.c
index bf7b70299b..8447cb09be 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1579,7 +1579,7 @@ static void lazy_init_worktree_map(void)
 	if (ref_to_worktree_map.worktrees)
 		return;
 
-	ref_to_worktree_map.worktrees = get_worktrees(0);
+	ref_to_worktree_map.worktrees = get_worktrees();
 	hashmap_init(&(ref_to_worktree_map.map), ref_to_worktree_map_cmpfnc, NULL, 0);
 	populate_worktree_map(&(ref_to_worktree_map.map), ref_to_worktree_map.worktrees);
 }
diff --git a/revision.c b/revision.c
index ebb4d2a0f2..f20d545ed5 100644
--- a/revision.c
+++ b/revision.c
@@ -1609,7 +1609,7 @@ static void add_other_reflogs_to_pending(struct all_refs_cb *cb)
 {
 	struct worktree **worktrees, **p;
 
-	worktrees = get_worktrees(0);
+	worktrees = get_worktrees();
 	for (p = worktrees; *p; p++) {
 		struct worktree *wt = *p;
 
@@ -1697,7 +1697,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 	if (revs->single_worktree)
 		return;
 
-	worktrees = get_worktrees(0);
+	worktrees = get_worktrees();
 	for (p = worktrees; *p; p++) {
 		struct worktree *wt = *p;
 		struct index_state istate = { NULL };
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 799fc00aa1..759e69dc54 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -37,7 +37,7 @@ static const char **get_store(const char **argv, struct ref_store **refs)
 
 		*refs = get_submodule_ref_store(gitdir);
 	} else if (skip_prefix(argv[0], "worktree:", &gitdir)) {
-		struct worktree **p, **worktrees = get_worktrees(0);
+		struct worktree **p, **worktrees = get_worktrees();
 
 		for (p = worktrees; *p; p++) {
 			struct worktree *wt = *p;
diff --git a/worktree.c b/worktree.c
index 8f24fbdc1e..ff9b7d847f 100644
--- a/worktree.c
+++ b/worktree.c
@@ -123,7 +123,7 @@ static void mark_current_worktree(struct worktree **worktrees)
 	free(git_dir);
 }
 
-struct worktree **get_worktrees(unsigned flags)
+struct worktree **get_worktrees(void)
 {
 	struct worktree **list = NULL;
 	struct strbuf path = STRBUF_INIT;
@@ -404,7 +404,7 @@ const struct worktree *find_shared_symref(const char *symref,
 
 	if (worktrees)
 		free_worktrees(worktrees);
-	worktrees = get_worktrees(0);
+	worktrees = get_worktrees();
 
 	for (i = 0; worktrees[i]; i++) {
 		struct worktree *wt = worktrees[i];
@@ -563,7 +563,7 @@ int other_head_refs(each_ref_fn fn, void *cb_data)
 	struct worktree **worktrees, **p;
 	int ret = 0;
 
-	worktrees = get_worktrees(0);
+	worktrees = get_worktrees();
 	for (p = worktrees; *p; p++) {
 		struct worktree *wt = *p;
 		struct object_id oid;
diff --git a/worktree.h b/worktree.h
index bd2235abe0..516744c433 100644
--- a/worktree.h
+++ b/worktree.h
@@ -25,7 +25,7 @@ struct worktree {
  * The caller is responsible for freeing the memory from the returned
  * worktrees by calling free_worktrees().
  */
-struct worktree **get_worktrees(unsigned flags);
+struct worktree **get_worktrees(void);
 
 /*
  * Returns 1 if linked worktrees exist, 0 otherwise.

-- 
2.27.0.221.g4d328a12d9
Junio C Hamano· Jun 22, 2020, 17:34 UTC · re: Eric Sunshine · lore

Re: [PATCH 0/2] drop specialized knowledge from generic worktree code

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 11 quoted lines
> This patch series removes specialized knowledge from the libified
> worktree code about how front-end "git worktree list" wants worktrees
> sorted, and instead makes it the responsibility of "git worktree list"
> itself to do the sorting.
>
> It is built atop es/worktree-duplicate-paths since that series adds
> another caller of get_worktrees() which this series touches.
>
> A possible argument against this patch series is that some other entity
> may someday want worktrees sorted in the same fashion as "git worktree
> list", however, that seems a case of YAGNI.

Is anything hurting to have the "mostly extra" flag and its handling that is added only to support "worktree list"? That would be another possible argument against this clean-up.

Having said that, I think this is good.
Thanks.  Will queue.

← back to recent threads