[PATCH v7 0/3] worktree: worktree.c functions and list builtin command

[PATCH v7 0/3] worktree: worktree.c functions and list builtin command

[Michael Rappazzo]

This series represents a major rewrite of the previous series.  It introduces
a top-level worktree.c which includes `get_worktree_list` and `get_worktree`
functions, and it also moves the `find_shared_symref` from branch.c to 
worktree.c

Michael Rappazzo (3):
  worktree: add top-level worktree.c
  worktree: move/refactor find_shared_symref from branch.c
  worktree: add 'list' command

 Documentation/git-worktree.txt |  10 ++-
 Makefile                       |   1 +
 branch.c                       |  79 +----------------
 branch.h                       |   8 --
 builtin/notes.c                |   1 +
 builtin/worktree.c             |  63 +++++++++++++
 t/t2027-worktree-list.sh       | 122 +++++++++++++++++++++++++
 worktree.c                     | 197 +++++++++++++++++++++++++++++++++++++++++
 worktree.h                     |  55 ++++++++++++
 9 files changed, 449 insertions(+), 87 deletions(-)
 create mode 100755 t/t2027-worktree-list.sh
 create mode 100644 worktree.c
 create mode 100644 worktree.h

-- 
2.5.0

[PATCH v7 1/3] worktree: add top-level worktree.c

[Michael Rappazzo]

Including functions to get the list of all worktrees, and to get
a specific worktree (primary or linked).

Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
---
 Makefile   |   1 +
 worktree.c | 157 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 worktree.h |  48 +++++++++++++++++++
 3 files changed, 206 insertions(+)
 create mode 100644 worktree.c
 create mode 100644 worktree.h

diff --git a/Makefile b/Makefile
index e326fa0..0131fed 100644
--- a/Makefile
+++ b/Makefile
@@ -807,6 +807,7 @@ LIB_OBJS += version.o
 LIB_OBJS += versioncmp.o
 LIB_OBJS += walker.o
 LIB_OBJS += wildmatch.o
+LIB_OBJS += worktree.o
 LIB_OBJS += wrapper.o
 LIB_OBJS += write_or_die.o
 LIB_OBJS += ws.o
diff --git a/worktree.c b/worktree.c
new file mode 100644
index 0000000..33d2e57
--- /dev/null
+++ b/worktree.c
@@ -0,0 +1,157 @@
+#include "worktree.h"
+#include "cache.h"
+#include "git-compat-util.h"
+#include "refs.h"
+#include "strbuf.h"
+
+void worktree_release(struct worktree *worktree)
+{
+	if (worktree) {
+		free(worktree->path);
+		free(worktree->git_dir);
+		if (!worktree->is_detached) {
+			/* could be headless */
+			free(worktree->head_ref);
+		}
+		free(worktree);
+	}
+}
+
+void worktree_list_release(struct worktree_list *worktree_list)
+{
+	while (worktree_list) {
+		struct worktree_list *next = worktree_list->next;
+		worktree_release(worktree_list->worktree);
+		free (worktree_list);
+		worktree_list = next;
+	}
+}
+
+/*
+ * read 'path_to_ref' into 'ref'.  Also set is_detached to 1 if the ref is detatched
+ *
+ * return 1 if the ref is not a proper ref, 0 otherwise (success)
+ */
+int _parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)
+{
+	if (!strbuf_readlink(ref, path_to_ref, 0)) {
+		if (!starts_with(ref->buf, "refs/") || check_refname_format(ref->buf, 0)) {
+			/* invalid ref - something is awry with this repo */
+			return 1;
+		}
+	} else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {
+		if (starts_with(ref->buf, "ref:")) {
+			strbuf_remove(ref, 0, strlen("ref:"));
+			strbuf_trim(ref);
+		} else if (is_detached) {
+			*is_detached = 1;
+		}
+	}
+	return 0;
+}
+
+struct worktree *get_worktree(const char *id)
+{
+	struct worktree *worktree = NULL;
+	struct strbuf path = STRBUF_INIT;
+	struct strbuf worktree_path = STRBUF_INIT;
+	struct strbuf git_dir = STRBUF_INIT;
+	struct strbuf head_ref = STRBUF_INIT;
+	int is_bare = 0;
+	int is_detached = 0;
+
+	if (id) {
+		strbuf_addf(&git_dir, "%s/worktrees/%s", absolute_path(get_git_common_dir()), id);
+		strbuf_addf(&path, "%s/gitdir", git_dir.buf);
+		if (strbuf_read_file(&worktree_path, path.buf, 0) <= 0) {
+			/* invalid git_dir file */
+			goto done;
+		}
+		strbuf_rtrim(&worktree_path);
+		if (!strbuf_strip_suffix(&worktree_path, "/.git")) {
+			strbuf_reset(&worktree_path);
+			strbuf_addstr(&worktree_path, absolute_path("."));
+			strbuf_strip_suffix(&worktree_path, "/.");
+		}
+
+		strbuf_reset(&path);
+		strbuf_addf(&path, "%s/worktrees/%s/HEAD", get_git_common_dir(), id);
+	} else {
+		strbuf_addf(&git_dir, "%s", absolute_path(get_git_common_dir()));
+		strbuf_addf(&worktree_path, "%s", absolute_path(get_git_common_dir()));
+		is_bare = !strbuf_strip_suffix(&worktree_path, "/.git");
+		if (is_bare)
+			strbuf_strip_suffix(&worktree_path, "/.");
+
+		strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
+	}
+
+	/*
+	 * $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside $GIT_DIR so for linked worktrees,
+	 * `resolve_ref_unsafe()` won't work (it uses git_path). Parse the ref ourselves.
+	 */
+	if (_parse_ref(path.buf, &head_ref, &is_detached))
+		goto done;
+
+	worktree = malloc(sizeof(struct worktree));
+	worktree->path = strbuf_detach(&worktree_path, NULL);
+	worktree->git_dir = strbuf_detach(&git_dir, NULL);
+	worktree->is_bare = is_bare;
+	worktree->head_ref = NULL;
+	worktree->is_detached = is_detached;
+	if (strlen(head_ref.buf) > 0) {
+		if (!is_detached) {
+			resolve_ref_unsafe(head_ref.buf, 0, worktree->head_sha1, NULL);
+			worktree->head_ref = strbuf_detach(&head_ref, NULL);
+		} else {
+			get_sha1_hex(head_ref.buf, worktree->head_sha1);
+		}
+	}
+done:
+	strbuf_release(&path);
+	strbuf_release(&git_dir);
+	strbuf_release(&head_ref);
+	strbuf_release(&worktree_path);
+	return worktree;
+}
+
+struct worktree_list *get_worktree_list()
+{
+	struct worktree_list *list = NULL;
+	struct worktree_list *current_entry = NULL;
+	struct worktree *current_worktree = NULL;
+	struct strbuf path = STRBUF_INIT;
+	DIR *dir;
+	struct dirent *d;
+
+	current_worktree = get_worktree(NULL);
+	if (current_worktree) {
+		list = malloc(sizeof(struct worktree_list));
+		list->worktree = current_worktree;
+		list->next = NULL;
+		current_entry = list;
+	}
+	strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
+	dir = opendir(path.buf);
+	strbuf_release(&path);
+	if (dir) {
+		while ((d = readdir(dir)) != NULL) {
+			if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
+				continue;
+
+			current_worktree = get_worktree(d->d_name);
+			if (current_worktree) {
+				current_entry->next = malloc(sizeof(struct worktree_list));
+				current_entry = current_entry->next;
+				current_entry->worktree = current_worktree;
+				current_entry->next = NULL;
+			}
+		}
+		closedir(dir);
+	}
+
+done:
+
+	return list;
+}
+
diff --git a/worktree.h b/worktree.h
new file mode 100644
index 0000000..2bc0ab8
--- /dev/null
+++ b/worktree.h
@@ -0,0 +1,48 @@
+#ifndef WORKTREE_H
+#define WORKTREE_H
+
+struct worktree {
+	char *path;
+	char *git_dir;
+	char *head_ref;
+	unsigned char head_sha1[20];
+	int is_detached;
+	int is_bare;
+};
+
+struct worktree_list {
+	struct worktree *worktree;
+	struct worktree_list *next;
+};
+
+/* Functions for acting on the information about worktrees. */
+
+/*
+ * Get the list of all worktrees.  The primary worktree will always be
+ * the first in the list followed by any other (linked) worktrees created
+ * by `git worktree add`.  No specific ordering is done on the linked
+ * worktrees.
+ *
+ * The caller is responsible for freeing the memory from the returned list.
+ * (See worktree_list_release for this purpose).
+ */
+extern struct worktree_list *get_worktree_list();
+
+/*
+ * generic method to get a worktree
+ *   - if 'id' is NULL, get the from $GIT_COMMON_DIR
+ *   - if 'id' is not NULL, get the worktree found in $GIT_COMMON_DIR/worktrees/id if
+ *     such a worktree exists
+ *
+ * The caller is responsible for freeing the memory from the returned
+ * worktree.  (See worktree_release for this purpose)
+ */
+struct worktree *get_worktree(const char *id);
+
+/*
+ * Free up the memory for a worktree_list/worktree
+ */
+extern void worktree_list_release(struct worktree_list *);
+extern void worktree_release(struct worktree *);
+
+#endif
-- 
2.5.0

[PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c

[Michael Rappazzo]

The code formerly in branch.c was largely the basis of the
get_worktree_list implementation is now moved to worktree.c,
and the find_shared_symref implementation has been refactored
to use get_worktree_list

Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
---
 branch.c        | 79 +--------------------------------------------------------
 branch.h        |  8 ------
 builtin/notes.c |  1 +
 worktree.c      | 40 +++++++++++++++++++++++++++++
 worktree.h      |  7 +++++
 5 files changed, 49 insertions(+), 86 deletions(-)

diff --git a/branch.c b/branch.c
index d013374..77d7f2a 100644
--- a/branch.c
+++ b/branch.c
@@ -4,6 +4,7 @@
 #include "refs.h"
 #include "remote.h"
 #include "commit.h"
+#include "worktree.h"
 
 struct tracking {
 	struct refspec spec;
@@ -311,84 +312,6 @@ void remove_branch_state(void)
 	unlink(git_path_squash_msg());
 }
 
-static char *find_linked_symref(const char *symref, const char *branch,
-				const char *id)
-{
-	struct strbuf sb = STRBUF_INIT;
-	struct strbuf path = STRBUF_INIT;
-	struct strbuf gitdir = STRBUF_INIT;
-	char *existing = NULL;
-
-	/*
-	 * $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside
-	 * $GIT_DIR so resolve_ref_unsafe() won't work (it uses
-	 * git_path). Parse the ref ourselves.
-	 */
-	if (id)
-		strbuf_addf(&path, "%s/worktrees/%s/%s", get_git_common_dir(), id, symref);
-	else
-		strbuf_addf(&path, "%s/%s", get_git_common_dir(), symref);
-
-	if (!strbuf_readlink(&sb, path.buf, 0)) {
-		if (!starts_with(sb.buf, "refs/") ||
-		    check_refname_format(sb.buf, 0))
-			goto done;
-	} else if (strbuf_read_file(&sb, path.buf, 0) >= 0 &&
-	    starts_with(sb.buf, "ref:")) {
-		strbuf_remove(&sb, 0, strlen("ref:"));
-		strbuf_trim(&sb);
-	} else
-		goto done;
-	if (strcmp(sb.buf, branch))
-		goto done;
-	if (id) {
-		strbuf_reset(&path);
-		strbuf_addf(&path, "%s/worktrees/%s/gitdir", get_git_common_dir(), id);
-		if (strbuf_read_file(&gitdir, path.buf, 0) <= 0)
-			goto done;
-		strbuf_rtrim(&gitdir);
-	} else
-		strbuf_addstr(&gitdir, get_git_common_dir());
-	strbuf_strip_suffix(&gitdir, ".git");
-
-	existing = strbuf_detach(&gitdir, NULL);
-done:
-	strbuf_release(&path);
-	strbuf_release(&sb);
-	strbuf_release(&gitdir);
-
-	return existing;
-}
-
-char *find_shared_symref(const char *symref, const char *target)
-{
-	struct strbuf path = STRBUF_INIT;
-	DIR *dir;
-	struct dirent *d;
-	char *existing;
-
-	if ((existing = find_linked_symref(symref, target, NULL)))
-		return existing;
-
-	strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
-	dir = opendir(path.buf);
-	strbuf_release(&path);
-	if (!dir)
-		return NULL;
-
-	while ((d = readdir(dir)) != NULL) {
-		if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
-			continue;
-		existing = find_linked_symref(symref, target, d->d_name);
-		if (existing)
-			goto done;
-	}
-done:
-	closedir(dir);
-
-	return existing;
-}
-
 void die_if_checked_out(const char *branch)
 {
 	char *existing;
diff --git a/branch.h b/branch.h
index d3446ed..58aa45f 100644
--- a/branch.h
+++ b/branch.h
@@ -59,12 +59,4 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name);
  */
 extern void die_if_checked_out(const char *branch);
 
-/*
- * Check if a per-worktree symref points to a ref in the main worktree
- * or any linked worktree, and return the path to the exising worktree
- * if it is.  Returns NULL if there is no existing ref.  The caller is
- * responsible for freeing the returned path.
- */
-extern char *find_shared_symref(const char *symref, const char *target);
-
 #endif
diff --git a/builtin/notes.c b/builtin/notes.c
index 3608c64..8b30334 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -20,6 +20,7 @@
 #include "notes-merge.h"
 #include "notes-utils.h"
 #include "branch.h"
+#include "worktree.h"
 
 static const char * const git_notes_usage[] = {
 	N_("git notes [--ref <notes-ref>] [list [<object>]]"),
diff --git a/worktree.c b/worktree.c
index 33d2e57..e45b651 100644
--- a/worktree.c
+++ b/worktree.c
@@ -155,3 +155,43 @@ done:
 	return list;
 }
 
+char *find_shared_symref(const char *symref, const char *target)
+{
+	char *existing = NULL;
+	struct strbuf path = STRBUF_INIT;
+	struct strbuf sb = STRBUF_INIT;
+	struct worktree_list *worktree_list = get_worktree_list();
+	struct worktree_list *orig_list = worktree_list;
+	struct worktree *matched = NULL;
+
+	while (!matched && worktree_list) {
+		if (strcmp("HEAD", symref)) {
+			strbuf_reset(&path);
+			strbuf_reset(&sb);
+			strbuf_addf(&path, "%s/%s", worktree_list->worktree->git_dir, symref);
+
+			if (_parse_ref(path.buf, &sb, NULL)) {
+				continue;
+			}
+
+			if (!strcmp(sb.buf, target))
+				matched = worktree_list->worktree;
+		} else {
+			if (worktree_list->worktree->head_ref && !strcmp(worktree_list->worktree->head_ref, target))
+				matched = worktree_list->worktree;
+		}
+		worktree_list = worktree_list->next ? worktree_list->next : NULL;
+	}
+
+	if (matched) {
+		existing = malloc(strlen(matched->path) + 1);
+		strcpy(existing, matched->path);
+	}
+
+done:
+	strbuf_release(&path);
+	strbuf_release(&sb);
+	worktree_list_release(orig_list);
+
+	return existing;
+}
diff --git a/worktree.h b/worktree.h
index 2bc0ab8..320f17e 100644
--- a/worktree.h
+++ b/worktree.h
@@ -45,4 +45,11 @@ struct worktree *get_worktree(const char *id);
 extern void worktree_list_release(struct worktree_list *);
 extern void worktree_release(struct worktree *);
 
+/*
+ * Look for a symref in any worktree, and return the path to the first
+ * worktree found or NULL if not found.  The caller is responsible for
+ * freeing the returned path.
+ */
+extern char *find_shared_symref(const char *symref, const char *target);
+
 #endif
-- 
2.5.0

[PATCH v7 3/3] worktree: add 'list' command

[Michael Rappazzo]

'git worktree list' iterates through the worktree list, and outputs
the worktree dir.  By default, only the worktree path is output.

Supported options include:
	--skip-bare: do not output bare worktrees
	--verbose: include the current head and ref (if applicable), also
		decorate bare worktrees

Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
---
 Documentation/git-worktree.txt |  10 +++-
 builtin/worktree.c             |  63 +++++++++++++++++++++
 t/t2027-worktree-list.sh       | 122 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 194 insertions(+), 1 deletion(-)
 create mode 100755 t/t2027-worktree-list.sh

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index fb68156..b9339ed 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 [verse]
 'git worktree add' [-f] [--detach] [-b <new-branch>] <path> [<branch>]
 'git worktree prune' [-n] [-v] [--expire <expire>]
+'git worktree list' [-v] [--skip-bare]
 
 DESCRIPTION
 -----------
@@ -59,6 +60,10 @@ prune::
 
 Prune working tree information in $GIT_DIR/worktrees.
 
+list::
+
+List the main worktree followed by each of the linked worktrees.
+
 OPTIONS
 -------
 
@@ -89,10 +94,14 @@ OPTIONS
 -v::
 --verbose::
 	With `prune`, report all removals.
+	With `list`, show more information about each worktree.  This includes
+	if the worktree is bare, the revision currently checked out, and the
+	branch currently checked out (or 'detached HEAD' if none).
 
 --expire <time>::
 	With `prune`, only expire unused working trees older than <time>.
 
+
 DETAILS
 -------
 Each linked working tree has a private sub-directory in the repository's
@@ -167,7 +176,6 @@ performed manually, such as:
 - `remove` to remove a linked working tree and its administrative files (and
   warn if the working tree is dirty)
 - `mv` to move or rename a working tree and update its administrative files
-- `list` to list linked working trees
 - `lock` to prevent automatic pruning of administrative files (for instance,
   for a working tree on a portable device)
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 71bb770..a0c0fe8 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -8,10 +8,13 @@
 #include "run-command.h"
 #include "sigchain.h"
 #include "refs.h"
+#include "utf8.h"
+#include "worktree.h"
 
 static const char * const worktree_usage[] = {
 	N_("git worktree add [<options>] <path> <branch>"),
 	N_("git worktree prune [<options>]"),
+	N_("git worktree list [<options>]"),
 	NULL
 };
 
@@ -359,6 +362,64 @@ static int add(int ac, const char **av, const char *prefix)
 	return add_worktree(path, branch, &opts);
 }
 
+static int list(int ac, const char **av, const char *prefix)
+{
+	int no_bare = 0;
+
+	struct option options[] = {
+		OPT_BOOL(0, "skip-bare", &no_bare,  N_("exclude bare worktrees from the list")),
+		OPT__VERBOSE(&verbose, N_("include more worktree details")),
+		OPT_END()
+	};
+
+	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+	if (ac)
+		usage_with_options(worktree_usage, options);
+	else {
+		struct worktree_list *worktree_list = get_worktree_list();
+		struct worktree_list *orig = worktree_list;
+		struct strbuf sb = STRBUF_INIT;
+		int path_maxlen = 0;
+
+		if (verbose) {
+			while (worktree_list) {
+				int cur_len = strlen(worktree_list->worktree->path);
+				if (cur_len > path_maxlen)
+					path_maxlen = cur_len;
+				worktree_list = worktree_list->next ? worktree_list->next : NULL;
+			}
+			worktree_list = orig;
+		}
+		while (worktree_list) {
+			/* if this work tree is not bare OR if bare repos are to be shown (default) */
+			if (!worktree_list->worktree->is_bare || !no_bare) {
+				strbuf_reset(&sb);
+				if (!verbose)
+					strbuf_addstr(&sb, worktree_list->worktree->path);
+				else {
+					int cur_len = strlen(worktree_list->worktree->path);
+					int utf8_adj = cur_len - utf8_strwidth(worktree_list->worktree->path);
+					strbuf_addf(&sb, "%-*s ", 1 + path_maxlen + utf8_adj, worktree_list->worktree->path);
+					if (worktree_list->worktree->is_bare)
+						strbuf_addstr(&sb, "(bare)");
+					else {
+						strbuf_addf(&sb, "%s ", find_unique_abbrev(worktree_list->worktree->head_sha1, DEFAULT_ABBREV));
+						if (!worktree_list->worktree->is_detached)
+							strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(worktree_list->worktree->head_ref, 0));
+						else
+							strbuf_addstr(&sb, "(detached HEAD)");
+					}
+				}
+				printf("%s\n", sb.buf);
+			}
+			worktree_list = worktree_list->next ? worktree_list->next : NULL;
+		}
+		worktree_list_release(orig);
+		strbuf_release(&sb);
+	}
+	return 0;
+}
+
 int cmd_worktree(int ac, const char **av, const char *prefix)
 {
 	struct option options[] = {
@@ -371,5 +432,7 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
 		return add(ac - 1, av + 1, prefix);
 	if (!strcmp(av[1], "prune"))
 		return prune(ac - 1, av + 1, prefix);
+	if (!strcmp(av[1], "list"))
+		return list(ac - 1, av + 1, prefix);
 	usage_with_options(worktree_usage, options);
 }
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
new file mode 100755
index 0000000..246890c
--- /dev/null
+++ b/t/t2027-worktree-list.sh
@@ -0,0 +1,122 @@
+#!/bin/sh
+
+test_description='test git worktree list'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	test_commit init
+'
+
+test_expect_success '"list" all worktrees from main' '
+	git rev-parse --show-toplevel >expect &&
+	git worktree add --detach here master &&
+	git -C here rev-parse --show-toplevel >>expect &&
+	git worktree list >actual &&
+	test_cmp expect actual &&
+	rm -rf here &&
+	git worktree prune
+'
+
+test_expect_success '"list" all worktrees from linked' '
+	git rev-parse --show-toplevel >expect &&
+	git worktree add --detach here master &&
+	git -C here rev-parse --show-toplevel >>expect &&
+	git -C here worktree list >actual &&
+	test_cmp expect actual &&
+	rm -rf here &&
+	git worktree prune
+'
+
+test_expect_success '"list" all worktrees --verbose from main' '
+	echo "$(git rev-parse --show-toplevel)       $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]" >expect &&
+	git worktree add --detach here master &&
+   echo "$(git -C here rev-parse --show-toplevel)  $(git rev-parse --short HEAD) (detached HEAD)" >>expect &&
+	git worktree list --verbose >actual &&
+	test_cmp expect actual &&
+	rm -rf here &&
+	git worktree prune
+'
+
+test_expect_success '"list" all worktrees --verbose from linked' '
+	echo "$(git rev-parse --show-toplevel)       $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]" >expect &&
+	git worktree add --detach here master &&
+   echo "$(git -C here rev-parse --show-toplevel)  $(git rev-parse --short HEAD) (detached HEAD)" >>expect &&
+	git -C here worktree list --verbose >actual &&
+	test_cmp expect actual &&
+	rm -rf here &&
+	git worktree prune
+'
+
+test_expect_success 'bare repo setup' '
+	git init --bare bare1 &&
+	echo "data" > file1 &&
+	git add file1 &&
+	git commit -m"File1: add data" &&
+	git push bare1 master &&
+	git reset --hard HEAD^
+'
+
+test_expect_success '"list" all worktrees from bare main' '
+	git -C bare1 worktree add --detach ../there master &&
+	echo "$(cd bare1 && pwd)" >expect &&
+	echo "$(git -C there rev-parse --show-toplevel)" >>expect &&
+	git -C bare1 worktree list >actual &&
+	test_cmp expect actual &&
+	rm -rf there &&
+	git -C bare1 worktree prune
+'
+
+test_expect_success '"list" all worktrees from bare main --skip-bare' '
+	git -C bare1 worktree add --detach ../there master &&
+	echo "$(git -C there rev-parse --show-toplevel)" >expect &&
+	git -C bare1 worktree list --skip-bare >actual &&
+	test_cmp expect actual &&
+	rm -rf there &&
+	git -C bare1 worktree prune
+'
+
+test_expect_success '"list" all worktrees from worktree with a bare main' '
+	git -C bare1 worktree add --detach ../there master &&
+	echo "$(cd bare1 && pwd)" >expect &&
+	echo "$(git -C there rev-parse --show-toplevel)" >>expect &&
+	git -C there worktree list >actual &&
+	test_cmp expect actual &&
+	rm -rf there &&
+	git -C bare1 worktree prune
+'
+
+test_expect_success '"list" all worktrees from worktree with a bare main --skip-bare' '
+	git -C bare1 worktree add --detach ../there master &&
+	echo "$(git -C there rev-parse --show-toplevel)" >expect &&
+	git -C there worktree list --skip-bare >actual &&
+	test_cmp expect actual &&
+	rm -rf there &&
+	git -C bare1 worktree prune
+'
+
+test_expect_success '"list" all worktrees --verbose from bare main' '
+	git -C bare1 worktree add --detach ../there master &&
+   echo "$(cd bare1 && pwd)  (bare)" >expect &&
+   echo "$(git -C there rev-parse --show-toplevel)  $(git -C there rev-parse --short HEAD) (detached HEAD)" >>expect &&
+	git -C bare1 worktree list --verbose >actual &&
+	test_cmp expect actual &&
+	rm -rf there &&
+	git -C bare1 worktree prune
+'
+
+test_expect_success '"list" all worktrees --verbose from worktree with a bare main' '
+	git -C bare1 worktree add --detach ../there master &&
+   echo "$(cd bare1 && pwd)  (bare)" >expect &&
+   echo "$(git -C there rev-parse --show-toplevel)  $(git -C there rev-parse --short HEAD) (detached HEAD)" >>expect &&
+	git -C there worktree list --verbose >actual &&
+	test_cmp expect actual &&
+	rm -rf there &&
+	git -C bare1 worktree prune
+'
+
+test_expect_success 'bare repo cleanup' '
+	rm -rf bare1
+'
+
+test_done
-- 
2.5.0

Re: [PATCH v7 1/3] worktree: add top-level worktree.c

[Junio C Hamano]

Michael Rappazzo <rappazzo@gmail.com> writes:

> Including functions to get the list of all worktrees, and to get
> a specific worktree (primary or linked).

Was this meant as a continuation of the sentence started on the
Subject line, or is s/Including/Include/ necessary?

> diff --git a/worktree.c b/worktree.c
> new file mode 100644
> index 0000000..33d2e57
> --- /dev/null
> +++ b/worktree.c
> @@ -0,0 +1,157 @@
> +#include "worktree.h"
> +#include "cache.h"
> +#include "git-compat-util.h"

The first header to be included must be "git-compat-util.h" or
"cache.h", the latter of which is so well known to include the
former as the first thing (so inclusion of "git-compat-util.h"
becomes unnecessary).

After that, include your own header as necessary.

> +#include "refs.h"
> +#include "strbuf.h"
> +
> +void worktree_release(struct worktree *worktree)
> +{
> +	if (worktree) {
> +		free(worktree->path);
> +		free(worktree->git_dir);
> +		if (!worktree->is_detached) {
> +			/* could be headless */
> +			free(worktree->head_ref);
> +		}

Unnecessary brace {} pair?  It is OK to keep them if later patches
in the series will make this a multi-statement block, though.

> +		free(worktree);
> +	}
> +}
> +
> +void worktree_list_release(struct worktree_list *worktree_list)
> +{
> +	while (worktree_list) {
> +		struct worktree_list *next = worktree_list->next;
> +		worktree_release(worktree_list->worktree);
> +		free (worktree_list);

No SP between function name and the parenthesis that introduces its
arguments.

> +		worktree_list = next;
> +	}
> +}
> +
> +/*
> + * read 'path_to_ref' into 'ref'.  Also set is_detached to 1 if the ref is detatched
> + *
> + * return 1 if the ref is not a proper ref, 0 otherwise (success)

These lines are overly long.

> + */
> +int _parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)
> +{
> +	if (!strbuf_readlink(ref, path_to_ref, 0)) {
> +		if (!starts_with(ref->buf, "refs/") || check_refname_format(ref->buf, 0)) {

An overly long line.  Perhaps 

		if (!starts_with(ref->buf, "refs/") ||
		    check_refname_format(ref->buf, 0)) {

(I see many more "overly long line" after this point, which I won't mention).

> +			/* invalid ref - something is awry with this repo */
> +			return 1;
> +		}
> +	} else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {
> +		if (starts_with(ref->buf, "ref:")) {
> +			strbuf_remove(ref, 0, strlen("ref:"));
> +			strbuf_trim(ref);

We don't do the same "starts_with() and format is sane" check?

> +		} else if (is_detached) {
> +			*is_detached = 1;
> +		}
> +	}

> +	return 0;
> +}
> +

> +struct worktree_list *get_worktree_list()

struct worktree_list *get_worktree_list(void)

> +{
> +	struct worktree_list *list = NULL;
> +	struct worktree_list *current_entry = NULL;
> +	struct worktree *current_worktree = NULL;
> +	struct strbuf path = STRBUF_INIT;
> +	DIR *dir;
> +	struct dirent *d;
> +
> +	current_worktree = get_worktree(NULL);
> +	if (current_worktree) {
> +		list = malloc(sizeof(struct worktree_list));

Always use x*alloc() family of functions.

> +		list->worktree = current_worktree;
> +		list->next = NULL;
> +		current_entry = list;
> +	}

If the above if() is not taken, current_entry is left as NULL

> +	strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
> +	dir = opendir(path.buf);
> +	strbuf_release(&path);
> +	if (dir) {
> +		while ((d = readdir(dir)) != NULL) {
> +			if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
> +				continue;
> +
> +			current_worktree = get_worktree(d->d_name);
> +			if (current_worktree) {
> +				current_entry->next = malloc(sizeof(struct worktree_list));

But this line assumes that current_entry is never NULL.

> +				current_entry = current_entry->next;
> +				current_entry->worktree = current_worktree;
> +				current_entry->next = NULL;
> +			}
> +		}
> +		closedir(dir);
> +	}

An idiomatic way to extend a singly-linked list at the end in our
codebase is to use a pointer to the pointer that has the element at
the end, instead of to use a pointer that points at the element at
the end or NULL (i.e. the pointer the above code calls current_entry
is "struct worktree_list **end_of_list").  Would it make the above
simpler if you followed that pattern?

> +
> +done:
> +
> +	return list;
> +}
> +
> diff --git a/worktree.h b/worktree.h
> new file mode 100644
> index 0000000..2bc0ab8
> --- /dev/null
> +++ b/worktree.h
> @@ -0,0 +1,48 @@
> +#ifndef WORKTREE_H
> +#define WORKTREE_H
> +
> +struct worktree {
> +	char *path;
> +	char *git_dir;
> +	char *head_ref;
> +	unsigned char head_sha1[20];
> +	int is_detached;
> +	int is_bare;
> +};
> +
> +struct worktree_list {
> +	struct worktree *worktree;
> +	struct worktree_list *next;
> +};
> +
> +/* Functions for acting on the information about worktrees. */
> +
> +/*
> + * Get the list of all worktrees.  The primary worktree will always be
> + * the first in the list followed by any other (linked) worktrees created
> + * by `git worktree add`.  No specific ordering is done on the linked
> + * worktrees.
> + *
> + * The caller is responsible for freeing the memory from the returned list.
> + * (See worktree_list_release for this purpose).
> + */
> +extern struct worktree_list *get_worktree_list();

extern struct worktree_list *get_worktree_list(void);

> +
> +/*
> + * generic method to get a worktree
> + *   - if 'id' is NULL, get the from $GIT_COMMON_DIR
> + *   - if 'id' is not NULL, get the worktree found in $GIT_COMMON_DIR/worktrees/id if
> + *     such a worktree exists
> + *
> + * The caller is responsible for freeing the memory from the returned
> + * worktree.  (See worktree_release for this purpose)
> + */
> +struct worktree *get_worktree(const char *id);
> +
> +/*
> + * Free up the memory for a worktree_list/worktree
> + */
> +extern void worktree_list_release(struct worktree_list *);
> +extern void worktree_release(struct worktree *);
> +
> +#endif

Re: [PATCH v7 1/3] worktree: add top-level worktree.c

[Mike Rappazzo]

On Thu, Sep 10, 2015 at 4:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael Rappazzo <rappazzo@gmail.com> writes:
>
>> Including functions to get the list of all worktrees, and to get
>> a specific worktree (primary or linked).
>
> Was this meant as a continuation of the sentence started on the
> Subject line, or is s/Including/Include/ necessary?

I think I was continuing the subject line.  I will make it nicer.

>
>> +             worktree_list = next;
>> +     }
>> +}
>> +
>> +/*
>> + * read 'path_to_ref' into 'ref'.  Also set is_detached to 1 if the ref is detatched
>> + *
>> + * return 1 if the ref is not a proper ref, 0 otherwise (success)
>
> These lines are overly long.
>
>> + */
>> +int _parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)
>> +{
>> +     if (!strbuf_readlink(ref, path_to_ref, 0)) {
>> +             if (!starts_with(ref->buf, "refs/") || check_refname_format(ref->buf, 0)) {
>
> An overly long line.  Perhaps
>
>                 if (!starts_with(ref->buf, "refs/") ||
>                     check_refname_format(ref->buf, 0)) {
>
> (I see many more "overly long line" after this point, which I won't mention).


Is the limit 80 characters?

>
>> +                     /* invalid ref - something is awry with this repo */
>> +                     return 1;
>> +             }
>> +     } else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {
>> +             if (starts_with(ref->buf, "ref:")) {
>> +                     strbuf_remove(ref, 0, strlen("ref:"));
>> +                     strbuf_trim(ref);
>
> We don't do the same "starts_with() and format is sane" check?

The code from this snippet was mostly ripped from branch.c (see the
second commit).
I will add the sanity check.

>
>
> An idiomatic way to extend a singly-linked list at the end in our
> codebase is to use a pointer to the pointer that has the element at
> the end, instead of to use a pointer that points at the element at
> the end or NULL (i.e. the pointer the above code calls current_entry
> is "struct worktree_list **end_of_list").  Would it make the above
> simpler if you followed that pattern?
>

I think I follow what you are saying here.  I will explore this route.
I am also unhappy about
having to separately maintain a point to the head of the list when
using it.  Would it be
"normal" to add a head pointer to the list structure?

Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c

[Junio C Hamano]

Michael Rappazzo <rappazzo@gmail.com> writes:

> The code formerly in branch.c was largely the basis of the
> get_worktree_list implementation is now moved to worktree.c,
> and the find_shared_symref implementation has been refactored
> to use get_worktree_list
>
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---




>  branch.c        | 79 +--------------------------------------------------------
>  branch.h        |  8 ------
>  builtin/notes.c |  1 +
>  worktree.c      | 40 +++++++++++++++++++++++++++++
>  worktree.h      |  7 +++++
>  5 files changed, 49 insertions(+), 86 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index d013374..77d7f2a 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -4,6 +4,7 @@
>  #include "refs.h"
>  #include "remote.h"
>  #include "commit.h"
> +#include "worktree.h"
>  
>  struct tracking {
>  	struct refspec spec;
> @@ -311,84 +312,6 @@ void remove_branch_state(void)
>  	unlink(git_path_squash_msg());
>  }
>  
> -static char *find_linked_symref(const char *symref, const char *branch,
> -				const char *id)

The title refers to a function with a different name ;-).

Copying the bulk of the function in 1/3 and then removing the
original here made it somewhat hard to compare what got changed in
the implementation.

I _think_ the code structure in the end result is more or less
right, though.

> diff --git a/worktree.c b/worktree.c
> index 33d2e57..e45b651 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -155,3 +155,43 @@ done:
>  	return list;
>  }
>  
> +char *find_shared_symref(const char *symref, const char *target)
> +{
> +	char *existing = NULL;
> +	struct strbuf path = STRBUF_INIT;
> +	struct strbuf sb = STRBUF_INIT;
> +	struct worktree_list *worktree_list = get_worktree_list();
> +	struct worktree_list *orig_list = worktree_list;
> +	struct worktree *matched = NULL;
> +
> +	while (!matched && worktree_list) {
> +		if (strcmp("HEAD", symref)) {
> +			strbuf_reset(&path);
> +			strbuf_reset(&sb);
> +			strbuf_addf(&path, "%s/%s", worktree_list->worktree->git_dir, symref);
> +
> +			if (_parse_ref(path.buf, &sb, NULL)) {
> +				continue;
> +			}

I forgot to say this on 1/3, but this helper function _parse_ref()
is obviously an implementation detail; does it have to be visible
outside the file, or can it become file-scope static?  There may
be other helpers that may not want to be global (I didn't check).

Provided that 1/3 will become accepable, this step looks sensible to
me.

Thanks.

Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c

[Mike Rappazzo]

On Fri, Sep 11, 2015 at 12:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael Rappazzo <rappazzo@gmail.com> writes:
>
>> The code formerly in branch.c was largely the basis of the
>> get_worktree_list implementation is now moved to worktree.c,
>> and the find_shared_symref implementation has been refactored
>> to use get_worktree_list
>>
>
> Copying the bulk of the function in 1/3 and then removing the
> original here made it somewhat hard to compare what got changed in
> the implementation.
>
> I _think_ the code structure in the end result is more or less
> right, though.

Should I squash these first two commits together in the next series?

Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c

[Junio C Hamano]

Mike Rappazzo <rappazzo@gmail.com> writes:

> On Fri, Sep 11, 2015 at 12:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Michael Rappazzo <rappazzo@gmail.com> writes:
>>
>>> The code formerly in branch.c was largely the basis of the
>>> get_worktree_list implementation is now moved to worktree.c,
>>> and the find_shared_symref implementation has been refactored
>>> to use get_worktree_list
>>>
>>
>> Copying the bulk of the function in 1/3 and then removing the
>> original here made it somewhat hard to compare what got changed in
>> the implementation.
>>
>> I _think_ the code structure in the end result is more or less
>> right, though.
>
> Should I squash these first two commits together in the next series?

Mashing the two into one patch would not help anybody, I would
suspect.

I didn't try this myself, but if I were doing this and if I were
aiming for perfection, then I would probably try to split it even
finer.  Refactor the original while both the callers and the helpers
are still inside branch.c (hence the early steps in the series will
not benefit worktree.c at all---worktree.c may not even exist in
them yet), move refactored helpers to worktree.[ch] (and at this
step you may not even have get_worktree() etc. yet), and then use
that refactored helper while writing new functions in worktree.c.

Re: [PATCH v7 3/3] worktree: add 'list' command

[Junio C Hamano]

Michael Rappazzo <rappazzo@gmail.com> writes:

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 71bb770..a0c0fe8 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -8,10 +8,13 @@
>  #include "run-command.h"
>  #include "sigchain.h"
>  #include "refs.h"
> +#include "utf8.h"
> +#include "worktree.h"
>  
>  static const char * const worktree_usage[] = {
>  	N_("git worktree add [<options>] <path> <branch>"),
>  	N_("git worktree prune [<options>]"),
> +	N_("git worktree list [<options>]"),
>  	NULL
>  };
>  
> @@ -359,6 +362,64 @@ static int add(int ac, const char **av, const char *prefix)
>  	return add_worktree(path, branch, &opts);
>  }
>  
> +static int list(int ac, const char **av, const char *prefix)
> +{
> +	int no_bare = 0;
> +
> +	struct option options[] = {
> +		OPT_BOOL(0, "skip-bare", &no_bare,  N_("exclude bare worktrees from the list")),
> +		OPT__VERBOSE(&verbose, N_("include more worktree details")),
> +		OPT_END()
> +	};
> +
> +	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
> +	if (ac)
> +		usage_with_options(worktree_usage, options);
> +	else {
> +		struct worktree_list *worktree_list = get_worktree_list();

Call that variable just "list"; you are not dealing with any other
kind of list in this code anyway.

> +		struct worktree_list *orig = worktree_list;
> +		struct strbuf sb = STRBUF_INIT;
> +		int path_maxlen = 0;
> +
> +		if (verbose) {
> +			while (worktree_list) {
> +				int cur_len = strlen(worktree_list->worktree->path);
> +				if (cur_len > path_maxlen)
> +					path_maxlen = cur_len;
> +				worktree_list = worktree_list->next ? worktree_list->next : NULL;

Ehh, (A ? A : NULL), when A is a pointer value, is equivalent to A, no?

		struct worktree_list *orig = get_worktree_list();
		struct worktree_list *list;

		for (list = orig; list; list = list->next) {
			struct worktree *worktree = list->worktree;
			int len = strlen(worktree->path);
                        if (maxlen < len)
                        	maxlen = len;
		}

It seems that "do we really need callback interface?" suggestion
really made the code simple and straight-forward.

> +		while (worktree_list) {
> +			/* if this work tree is not bare OR if bare repos are to be shown (default) */
> +			if (!worktree_list->worktree->is_bare || !no_bare) {
> +				strbuf_reset(&sb);
> +				if (!verbose)
> +					strbuf_addstr(&sb, worktree_list->worktree->path);
> +				else {
> +					int cur_len = strlen(worktree_list->worktree->path);
> +					int utf8_adj = cur_len - utf8_strwidth(worktree_list->worktree->path);
> +					strbuf_addf(&sb, "%-*s ", 1 + path_maxlen + utf8_adj, worktree_list->worktree->path);
> +					if (worktree_list->worktree->is_bare)
> +						strbuf_addstr(&sb, "(bare)");
> +					else {
> +						strbuf_addf(&sb, "%s ", find_unique_abbrev(worktree_list->worktree->head_sha1, DEFAULT_ABBREV));
> +						if (!worktree_list->worktree->is_detached)
> +							strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(worktree_list->worktree->head_ref, 0));
> +						else
> +							strbuf_addstr(&sb, "(detached HEAD)");
> +					}
> +				}
> +				printf("%s\n", sb.buf);
> +			}
> +			worktree_list = worktree_list->next ? worktree_list->next : NULL;

Would it help reduce the indentation level if you inroduced a helper
function that takes a single worktree object and does all the above?

	for (list = orig; list; list = list->next) {
		struct worktree *worktree = list->worktree;
		if (!no_bare || !worktree->is_bare)
	        	show_worktree(worktree);
	}

or something?

Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c

[Eric Sunshine]

On Fri, Sep 11, 2015 at 5:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Mike Rappazzo <rappazzo@gmail.com> writes:
>> On Fri, Sep 11, 2015 at 12:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Michael Rappazzo <rappazzo@gmail.com> writes:
>>>> The code formerly in branch.c was largely the basis of the
>>>> get_worktree_list implementation is now moved to worktree.c,
>>>> and the find_shared_symref implementation has been refactored
>>>> to use get_worktree_list
>>>
>>> Copying the bulk of the function in 1/3 and then removing the
>>> original here made it somewhat hard to compare what got changed in
>>> the implementation.
>>>
>>> I _think_ the code structure in the end result is more or less
>>> right, though.
>>
>> Should I squash these first two commits together in the next series?
>
> Mashing the two into one patch would not help anybody, I would
> suspect.
>
> I didn't try this myself, but if I were doing this and if I were
> aiming for perfection, then I would probably try to split it even
> finer.  Refactor the original while both the callers and the helpers
> are still inside branch.c (hence the early steps in the series will
> not benefit worktree.c at all---worktree.c may not even exist in
> them yet), move refactored helpers to worktree.[ch] (and at this
> step you may not even have get_worktree() etc. yet), and then use
> that refactored helper while writing new functions in worktree.c.

Thanks for bringing this up. I haven't found a sufficient block of
time yet to review these patches, however, I had the same thought upon
a quick initial read, and is how I was hoping to see the patch series
constructed based upon my earlier two reviews suggesting refactoring
the existing branch.c functions into a new get_worktrees(). There are
at least a couple important reasons for taking this approach.

First, it keeps the "blame" trail intact, the full context of which
can be helpful to someone trying to understand why the code is the way
it is. The current approach of having get_worktree_list() materialize
out of thin air (even though it may have been patterned after existing
code) doesn't give the same benefit.

Second, it's easier for reviewers to understand and check the code for
correctness if it mutates to the final form in small increments than
it is to have get_worktrees() come into existence fully matured.

A final minor comment: Since all three branch.c functions,
die_if_checked_out(), find_shared_symref(), and find_linked_symref(),
ought to be moved to top-level worktree.c, I'd probably have patch 1
do the relocation (with no functional changes), and subsequent patches
refactor the moved code into a general purpose get_worktrees(). The
final patch would then implement "git worktrees list".

Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c

[Mike Rappazzo]

On Fri, Sep 11, 2015 at 7:10 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Sep 11, 2015 at 5:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Thanks for bringing this up. I haven't found a sufficient block of
> time yet to review these patches, however, I had the same thought upon
> a quick initial read, and is how I was hoping to see the patch series
> constructed based upon my earlier two reviews suggesting refactoring
> the existing branch.c functions into a new get_worktrees(). There are
> at least a couple important reasons for taking this approach.
>
> First, it keeps the "blame" trail intact, the full context of which
> can be helpful to someone trying to understand why the code is the way
> it is. The current approach of having get_worktree_list() materialize
> out of thin air (even though it may have been patterned after existing
> code) doesn't give the same benefit.
>
> Second, it's easier for reviewers to understand and check the code for
> correctness if it mutates to the final form in small increments than
> it is to have get_worktrees() come into existence fully matured.
>
> A final minor comment: Since all three branch.c functions,
> die_if_checked_out(), find_shared_symref(), and find_linked_symref(),
> ought to be moved to top-level worktree.c, I'd probably have patch 1
> do the relocation (with no functional changes), and subsequent patches
> refactor the moved code into a general purpose get_worktrees(). The
> final patch would then implement "git worktrees list".

I like the way this history works out, and I have reworked the history
to follow this idea.  The only thing
I didn't do was move the die_if_checked_out() function from branch.c,
as I feel that this function is more
of a branch feature than a worktree feature.

Re: [PATCH v7 1/3] worktree: add top-level worktree.c

[Eric Sunshine]

On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:
> Including functions to get the list of all worktrees, and to get
> a specific worktree (primary or linked).

Some comments below in addition to those by Junio...

> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
> diff --git a/worktree.c b/worktree.c
> new file mode 100644
> index 0000000..33d2e57
> --- /dev/null
> +++ b/worktree.c
> @@ -0,0 +1,157 @@
> +#include "worktree.h"
> +#include "cache.h"
> +#include "git-compat-util.h"
> +#include "refs.h"
> +#include "strbuf.h"
> +
> +void worktree_release(struct worktree *worktree)
> +{
> +       if (worktree) {
> +               free(worktree->path);
> +               free(worktree->git_dir);
> +               if (!worktree->is_detached) {
> +                       /* could be headless */
> +                       free(worktree->head_ref);

It's safe to invoke free() on NULL, so as long as worktree->head_ref
holds NULL in the detached case, then there's no need for the
conditional at all (or the comment).

> +               }
> +               free(worktree);
> +       }
> +}
> +
> +void worktree_list_release(struct worktree_list *worktree_list)
> +{
> +       while (worktree_list) {
> +               struct worktree_list *next = worktree_list->next;
> +               worktree_release(worktree_list->worktree);
> +               free (worktree_list);
> +               worktree_list = next;
> +       }
> +}
> +
> +/*
> + * read 'path_to_ref' into 'ref'.  Also set is_detached to 1 if the ref is detatched
> + *
> + * return 1 if the ref is not a proper ref, 0 otherwise (success)
> + */
> +int _parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)

Mentioned already by Junio in his review of patch 2: Make this
function static and drop the leading underscore from the name.

> +{
> +       if (!strbuf_readlink(ref, path_to_ref, 0)) {
> +               if (!starts_with(ref->buf, "refs/") || check_refname_format(ref->buf, 0)) {
> +                       /* invalid ref - something is awry with this repo */
> +                       return 1;

In this codebase, it's common to return -1 on error. You could also
probably drop the unnecessary braces (here and a few other places).

> +               }
> +       } else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {
> +               if (starts_with(ref->buf, "ref:")) {
> +                       strbuf_remove(ref, 0, strlen("ref:"));
> +                       strbuf_trim(ref);
> +               } else if (is_detached) {
> +                       *is_detached = 1;

The code allows the caller to pass in NULL for is_detached if not
interested in this information, however, the comment block documenting
the function doesn't mention this.

Also, don't you need to clear *is_detached when not detached since you
don't know what garbage value *is_detached holds upon entry?

I find the placement of the detached detection logic here a bit
strange. The only case for which it might trigger is the so-called
"main worktree", yet having it in this general purpose parse function
seems to imply misleadingly that any worktree could be detached. Also,
given the current world order[1], wouldn't missing "ref:" indicate an
error for any worktree other than the main one? Consequently, this
detection probably ought to be done only for the main worktree
(outside of this function, probably).

[1]: Though, this might not hold true in a future world order
involving merging of notes, if I correctly understood that discussion,
since notes merging wouldn't require a "worktree", per se.

> +               }
> +       }
> +       return 0;
> +}
> +
> +struct worktree *get_worktree(const char *id)
> +{
> +       struct worktree *worktree = NULL;
> +       struct strbuf path = STRBUF_INIT;
> +       struct strbuf worktree_path = STRBUF_INIT;
> +       struct strbuf git_dir = STRBUF_INIT;
> +       struct strbuf head_ref = STRBUF_INIT;
> +       int is_bare = 0;
> +       int is_detached = 0;
> +
> +       if (id) {
> +               strbuf_addf(&git_dir, "%s/worktrees/%s", absolute_path(get_git_common_dir()), id);
> +               strbuf_addf(&path, "%s/gitdir", git_dir.buf);
> +               if (strbuf_read_file(&worktree_path, path.buf, 0) <= 0) {
> +                       /* invalid git_dir file */
> +                       goto done;
> +               }
> +               strbuf_rtrim(&worktree_path);
> +               if (!strbuf_strip_suffix(&worktree_path, "/.git")) {
> +                       strbuf_reset(&worktree_path);
> +                       strbuf_addstr(&worktree_path, absolute_path("."));
> +                       strbuf_strip_suffix(&worktree_path, "/.");
> +               }
> +
> +               strbuf_reset(&path);
> +               strbuf_addf(&path, "%s/worktrees/%s/HEAD", get_git_common_dir(), id);
> +       } else {
> +               strbuf_addf(&git_dir, "%s", absolute_path(get_git_common_dir()));
> +               strbuf_addf(&worktree_path, "%s", absolute_path(get_git_common_dir()));
> +               is_bare = !strbuf_strip_suffix(&worktree_path, "/.git");
> +               if (is_bare)
> +                       strbuf_strip_suffix(&worktree_path, "/.");
> +
> +               strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
> +       }

I realize that this is modeled closely after existing code in
branch.c, but, with the exception of parsing the ref file and
constructing a worktree structure, the main worktree case (id == NULL)
is entirely disjoint from the linked worktree case (id != NULL). This
suggests strongly that get_worktree() should be split into two
functions, one for the main worktree and one for linked worktrees,
which would make the code easier to understand. You might call the
functions get_main_worktree() and get_linked_worktree(id) (or perhaps
drop "linked" from the latter name).

> +       /*
> +        * $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside $GIT_DIR so for linked worktrees,
> +        * `resolve_ref_unsafe()` won't work (it uses git_path). Parse the ref ourselves.
> +        */

This comment probably belongs inside _parse_ref() as it's explaining
its internal workings, rather than here at the call-site.

> +       if (_parse_ref(path.buf, &head_ref, &is_detached))
> +               goto done;
> +
> +       worktree = malloc(sizeof(struct worktree));
> +       worktree->path = strbuf_detach(&worktree_path, NULL);
> +       worktree->git_dir = strbuf_detach(&git_dir, NULL);
> +       worktree->is_bare = is_bare;
> +       worktree->head_ref = NULL;
> +       worktree->is_detached = is_detached;
> +       if (strlen(head_ref.buf) > 0) {

More idiomatic:

    if (head_ref.len) {

> +               if (!is_detached) {
> +                       resolve_ref_unsafe(head_ref.buf, 0, worktree->head_sha1, NULL);
> +                       worktree->head_ref = strbuf_detach(&head_ref, NULL);
> +               } else {
> +                       get_sha1_hex(head_ref.buf, worktree->head_sha1);
> +               }
> +       }
> +done:
> +       strbuf_release(&path);
> +       strbuf_release(&git_dir);
> +       strbuf_release(&head_ref);
> +       strbuf_release(&worktree_path);
> +       return worktree;
> +}
> +
> +struct worktree_list *get_worktree_list()

Can we be more concise and call this get_worktrees()?

> +{
> +       struct worktree_list *list = NULL;
> +       struct worktree_list *current_entry = NULL;
> +       struct worktree *current_worktree = NULL;
> +       struct strbuf path = STRBUF_INIT;
> +       DIR *dir;
> +       struct dirent *d;
> +
> +       current_worktree = get_worktree(NULL);
> +       if (current_worktree) {
> +               list = malloc(sizeof(struct worktree_list));
> +               list->worktree = current_worktree;
> +               list->next = NULL;
> +               current_entry = list;
> +       }
> +       strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
> +       dir = opendir(path.buf);
> +       strbuf_release(&path);
> +       if (dir) {
> +               while ((d = readdir(dir)) != NULL) {
> +                       if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
> +                               continue;
> +
> +                       current_worktree = get_worktree(d->d_name);
> +                       if (current_worktree) {
> +                               current_entry->next = malloc(sizeof(struct worktree_list));
> +                               current_entry = current_entry->next;
> +                               current_entry->worktree = current_worktree;
> +                               current_entry->next = NULL;
> +                       }
> +               }
> +               closedir(dir);
> +       }
> +
> +done:

What is this label for? It doesn't seem to have any corresponding goto's.

> +
> +       return list;
> +}
> +
> diff --git a/worktree.h b/worktree.h
> new file mode 100644
> index 0000000..2bc0ab8
> --- /dev/null
> +++ b/worktree.h
> @@ -0,0 +1,48 @@
> +#ifndef WORKTREE_H
> +#define WORKTREE_H
> +
> +struct worktree {
> +       char *path;
> +       char *git_dir;
> +       char *head_ref;
> +       unsigned char head_sha1[20];
> +       int is_detached;
> +       int is_bare;
> +};
> +
> +struct worktree_list {
> +       struct worktree *worktree;
> +       struct worktree_list *next;
> +};

I don't care too strongly, but an alternate approach (which I probably
would have taken) would be to have get_worktrees() simply return an
array of 'struct worktree' objects, hence no need for the additional
'struct worktree_list'. The slight complication with this approach,
though, is that get_worktrees() either also needs to return the length
of the array, or the array should end with some sort of end-of-array
sentinel. An obvious sentinel would be path==NULL or git_dir==NULL or
all of the above.

Client iteration is just about the same with the array approach as
with the linked-list approach.

> +/* Functions for acting on the information about worktrees. */
> +
> +/*
> + * Get the list of all worktrees.  The primary worktree will always be
> + * the first in the list followed by any other (linked) worktrees created
> + * by `git worktree add`.  No specific ordering is done on the linked
> + * worktrees.
> + *
> + * The caller is responsible for freeing the memory from the returned list.
> + * (See worktree_list_release for this purpose).
> + */
> +extern struct worktree_list *get_worktree_list();
> +
> +/*
> + * generic method to get a worktree
> + *   - if 'id' is NULL, get the from $GIT_COMMON_DIR
> + *   - if 'id' is not NULL, get the worktree found in $GIT_COMMON_DIR/worktrees/id if
> + *     such a worktree exists
> + *
> + * The caller is responsible for freeing the memory from the returned
> + * worktree.  (See worktree_release for this purpose)
> + */
> +struct worktree *get_worktree(const char *id);
> +
> +/*
> + * Free up the memory for a worktree_list/worktree
> + */
> +extern void worktree_list_release(struct worktree_list *);
> +extern void worktree_release(struct worktree *);
> +
> +#endif
> --
> 2.5.0

Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c

[Eric Sunshine]

On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:
> The code formerly in branch.c was largely the basis of the
> get_worktree_list implementation is now moved to worktree.c,
> and the find_shared_symref implementation has been refactored
> to use get_worktree_list

Some comments below in addition to those by Junio...

> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
> diff --git a/branch.h b/branch.h
> index d3446ed..58aa45f 100644
> --- a/branch.h
> +++ b/branch.h
> @@ -59,12 +59,4 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name);
>   */
>  extern void die_if_checked_out(const char *branch);
>
> -/*
> - * Check if a per-worktree symref points to a ref in the main worktree
> - * or any linked worktree, and return the path to the exising worktree
> - * if it is.  Returns NULL if there is no existing ref.  The caller is
> - * responsible for freeing the returned path.
> - */
> -extern char *find_shared_symref(const char *symref, const char *target);
> -
>  #endif
> diff --git a/worktree.c b/worktree.c
> index 33d2e57..e45b651 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -155,3 +155,43 @@ done:
>         return list;
>  }
>
> +char *find_shared_symref(const char *symref, const char *target)
> +{
> +       char *existing = NULL;
> +       struct strbuf path = STRBUF_INIT;
> +       struct strbuf sb = STRBUF_INIT;
> +       struct worktree_list *worktree_list = get_worktree_list();
> +       struct worktree_list *orig_list = worktree_list;
> +       struct worktree *matched = NULL;
> +
> +       while (!matched && worktree_list) {
> +               if (strcmp("HEAD", symref)) {

The result of strcmp() never changes, so this (expensive) check could
be lifted out of the 'while' loop, however...

> +                       strbuf_reset(&path);
> +                       strbuf_reset(&sb);
> +                       strbuf_addf(&path, "%s/%s", worktree_list->worktree->git_dir, symref);
> +
> +                       if (_parse_ref(path.buf, &sb, NULL)) {
> +                               continue;
> +                       }
> +
> +                       if (!strcmp(sb.buf, target))
> +                               matched = worktree_list->worktree;

The original code in branch.c, which this patch removes, did not need
to make a special case of HEAD, so it's not immediately clear why this
replacement code does so. This is the sort of issue which argues in
favor of mutating the existing code (slowly) over the course of
several patches into the final form, rather than having the final form
come into existence out of thin air. When the changes are made
incrementally, it is easier for reviewers to understand why such
modifications are needed, which (hopefully) should lead to fewer
questions such as this one.

> +               } else {
> +                       if (worktree_list->worktree->head_ref && !strcmp(worktree_list->worktree->head_ref, target))
> +                               matched = worktree_list->worktree;
> +               }
> +               worktree_list = worktree_list->next ? worktree_list->next : NULL;
> +       }
> +
> +       if (matched) {
> +               existing = malloc(strlen(matched->path) + 1);
> +               strcpy(existing, matched->path);

A couple issues:

This can be done more concisely and safely with xstrdup().

In this codebase, it probably would be more idiomatic to use goto to
drop out of the loop rather than setting 'matched' and then having to
check 'matched' in the loop condition. So, for instance, each place
which sets 'matched' could instead say:

    existing = xstrdup(...);
    goto done;

> +       }
> +
> +done:

This label doesn't have any matching goto's.

> +       strbuf_release(&path);
> +       strbuf_release(&sb);
> +       worktree_list_release(orig_list);
> +
> +       return existing;
> +}
> diff --git a/worktree.h b/worktree.h
> index 2bc0ab8..320f17e 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -45,4 +45,11 @@ struct worktree *get_worktree(const char *id);
>  extern void worktree_list_release(struct worktree_list *);
>  extern void worktree_release(struct worktree *);
>
> +/*
> + * Look for a symref in any worktree, and return the path to the first
> + * worktree found or NULL if not found.  The caller is responsible for
> + * freeing the returned path.
> + */

For some reason, this comment differs a fair bit from the original in
branch.h which was removed by this patch, however, the original
comment was a bit more explanatory (I think).

As a general rule, it's better to do code movement and code changes in
separate patches since it's hard for reviewers to detect and
comprehend differences in code which has been both moved and changed
at the same time.

> +extern char *find_shared_symref(const char *symref, const char *target);
> +
>  #endif
> --
> 2.5.0

Re: [PATCH v7 3/3] worktree: add 'list' command

[Eric Sunshine]

On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:
> 'git worktree list' iterates through the worktree list, and outputs
> the worktree dir.  By default, only the worktree path is output.

Comments below in addition to Junio's...

> Supported options include:
>         --skip-bare: do not output bare worktrees
>         --verbose: include the current head and ref (if applicable), also
>                 decorate bare worktrees

I don't care strongly at this point, but an alternate, (possibly) more
reviewer-friendly, approach would be to split this into several
patches: the first would add a bare-bones "list" command, and each
subsequent patch would flesh it out by introducing one option and/or
enhancing the output in some way.

> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index fb68156..b9339ed 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -11,6 +11,7 @@ SYNOPSIS
>  [verse]
>  'git worktree add' [-f] [--detach] [-b <new-branch>] <path> [<branch>]
>  'git worktree prune' [-n] [-v] [--expire <expire>]
> +'git worktree list' [-v] [--skip-bare]

I'm somewhat skeptical of the --skip-bare option. Recalling my v2
review[1] skepticism of the --main-only option:

    The more options we have, the more we have to document, test, and
    support...

Thus, I wonder how much value this option has. Presumably, for
scripters, we will want to have a --porcelain option, the output of
which will contain useful information about a worktree, including
whether it's bare, and a script can, on its own, easily filter out a
bare worktree if desired.

[1]: http://article.gmane.org/gmane.comp.version-control.git/275528

>  DESCRIPTION
>  -----------
> @@ -59,6 +60,10 @@ prune::
>
>  Prune working tree information in $GIT_DIR/worktrees.
>
> +list::
> +
> +List the main worktree followed by each of the linked worktrees.
> +
>  OPTIONS
>  -------
>
> @@ -89,10 +94,14 @@ OPTIONS
>  -v::
>  --verbose::
>         With `prune`, report all removals.
> +       With `list`, show more information about each worktree.  This includes
> +       if the worktree is bare, the revision currently checked out, and the
> +       branch currently checked out (or 'detached HEAD' if none).

Hmm, this prints both the SHA1 and the branch name (or literal string
"detached HEAD")? Is that a good use of screen real-estate? In
particular, I'm wondering how useful the SHA1 is to the user when not
detached. I would expect (perhaps wrongly) that it would be sufficient
to output either the branch name *or* the SHA1 (which implies
"detached" without having to say so literally).

>  --expire <time>::
>         With `prune`, only expire unused working trees older than <time>.

Documentation for --skip-bare (and --no-skip-bare) seems to be missing.

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 71bb770..a0c0fe8 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -359,6 +362,64 @@ static int add(int ac, const char **av, const char *prefix)
> +static int list(int ac, const char **av, const char *prefix)
> +{
> +       int no_bare = 0;
> +
> +       struct option options[] = {
> +               OPT_BOOL(0, "skip-bare", &no_bare,  N_("exclude bare worktrees from the list")),
> +               OPT__VERBOSE(&verbose, N_("include more worktree details")),
> +               OPT_END()
> +       };
> +
> +       ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
> +       if (ac)
> +               usage_with_options(worktree_usage, options);
> +       else {
> +               struct worktree_list *worktree_list = get_worktree_list();
> +               struct worktree_list *orig = worktree_list;
> +               struct strbuf sb = STRBUF_INIT;
> +               int path_maxlen = 0;
> +
> +               if (verbose) {
> +                       while (worktree_list) {
> +                               int cur_len = strlen(worktree_list->worktree->path);
> +                               if (cur_len > path_maxlen)
> +                                       path_maxlen = cur_len;
> +                               worktree_list = worktree_list->next ? worktree_list->next : NULL;
> +                       }
> +                       worktree_list = orig;
> +               }
> +               while (worktree_list) {
> +                       /* if this work tree is not bare OR if bare repos are to be shown (default) */
> +                       if (!worktree_list->worktree->is_bare || !no_bare) {

Double negatives (!no_bare) are hard to grok. 'bare_only' or
'skip_bare' might be better, and then the comment would likely be
superfluous. Also, having the "default" behavior mentioned in the
comment is an invitation for it to become outdated.

> +                               strbuf_reset(&sb);
> +                               if (!verbose)
> +                                       strbuf_addstr(&sb, worktree_list->worktree->path);
> +                               else {
> +                                       int cur_len = strlen(worktree_list->worktree->path);
> +                                       int utf8_adj = cur_len - utf8_strwidth(worktree_list->worktree->path);
> +                                       strbuf_addf(&sb, "%-*s ", 1 + path_maxlen + utf8_adj, worktree_list->worktree->path);

I'm not personally convinced that this sort of alignment is desirable,
however, the new strbuf_utf8_align()[2] might be useful here.

[2]: Once it graduates to 'master', that is; it's currently in 'pu' at
5df5352 (utf8: add function to align a string into given strbuf,
2015-09-10).

> +                                       if (worktree_list->worktree->is_bare)
> +                                               strbuf_addstr(&sb, "(bare)");
> +                                       else {
> +                                               strbuf_addf(&sb, "%s ", find_unique_abbrev(worktree_list->worktree->head_sha1, DEFAULT_ABBREV));
> +                                               if (!worktree_list->worktree->is_detached)
> +                                                       strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(worktree_list->worktree->head_ref, 0));
> +                                               else
> +                                                       strbuf_addstr(&sb, "(detached HEAD)");
> +                                       }
> +                               }
> +                               printf("%s\n", sb.buf);
> +                       }
> +                       worktree_list = worktree_list->next ? worktree_list->next : NULL;
> +               }
> +               worktree_list_release(orig);
> +               strbuf_release(&sb);
> +       }
> +       return 0;
> +}
> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
> new file mode 100755
> index 0000000..246890c
> --- /dev/null
> +++ b/t/t2027-worktree-list.sh
> @@ -0,0 +1,122 @@
> +#!/bin/sh
> +
> +test_description='test git worktree list'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +       test_commit init
> +'
> +
> +test_expect_success '"list" all worktrees from main' '
> +       git rev-parse --show-toplevel >expect &&
> +       git worktree add --detach here master &&
> +       git -C here rev-parse --show-toplevel >>expect &&
> +       git worktree list >actual &&
> +       test_cmp expect actual &&
> +       rm -rf here &&
> +       git worktree prune

If the test fails somewhere above the 'rm', then the cleanup won't
take place, which will result in cascading failures of subsequent
tests. Instead, use test_when_finished() before the "worktree add"
line to ensure cleanup whether successful or not:

    test_when_finished "rm -rf here && git worktree prune" &&
    git worktree add ... &&

Same comment applies to remaining tests.

> +'
> +
> +test_expect_success '"list" all worktrees from linked' '
> +       git rev-parse --show-toplevel >expect &&
> +       git worktree add --detach here master &&
> +       git -C here rev-parse --show-toplevel >>expect &&
> +       git -C here worktree list >actual &&
> +       test_cmp expect actual &&
> +       rm -rf here &&
> +       git worktree prune
> +'
> +
> +test_expect_success 'bare repo setup' '
> +       git init --bare bare1 &&
> +       echo "data" > file1 &&

Style: No space after redirection operator.

Nit: Unnecessary quotes around 'data'.

> +       git add file1 &&
> +       git commit -m"File1: add data" &&
> +       git push bare1 master &&
> +       git reset --hard HEAD^
> +'
> +
> +test_expect_success '"list" all worktrees from bare main' '
> +       git -C bare1 worktree add --detach ../there master &&
> +       echo "$(cd bare1 && pwd)" >expect &&

This may be problematic on Windows. See the discussion of $PWD in
t/README. You may need to use $(pwd) instead. Same comment applies
below, too.

> +       echo "$(git -C there rev-parse --show-toplevel)" >>expect &&
> +       git -C bare1 worktree list >actual &&
> +       test_cmp expect actual &&
> +       rm -rf there &&
> +       git -C bare1 worktree prune
> +'
> +test_expect_success '"list" all worktrees --verbose from bare main' '
> +       git -C bare1 worktree add --detach ../there master &&
> +   echo "$(cd bare1 && pwd)  (bare)" >expect &&
> +   echo "$(git -C there rev-parse --show-toplevel)  $(git -C there rev-parse --short HEAD) (detached HEAD)" >>expect &&

Botched indentation here and in next test.

> +       git -C bare1 worktree list --verbose >actual &&
> +       test_cmp expect actual &&
> +       rm -rf there &&
> +       git -C bare1 worktree prune
> +'
> +
> +test_expect_success '"list" all worktrees --verbose from worktree with a bare main' '
> +       git -C bare1 worktree add --detach ../there master &&
> +   echo "$(cd bare1 && pwd)  (bare)" >expect &&
> +   echo "$(git -C there rev-parse --show-toplevel)  $(git -C there rev-parse --short HEAD) (detached HEAD)" >>expect &&
> +       git -C there worktree list --verbose >actual &&
> +       test_cmp expect actual &&
> +       rm -rf there &&
> +       git -C bare1 worktree prune
> +'
> +
> +test_expect_success 'bare repo cleanup' '
> +       rm -rf bare1
> +'

Meh. Probably unnecessary cleanup.

> +
> +test_done
> --
> 2.5.0

Re: [PATCH v7 1/3] worktree: add top-level worktree.c

[Eric Sunshine]

On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:
>> +               }
>> +       } else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {
>> +               if (starts_with(ref->buf, "ref:")) {
>> +                       strbuf_remove(ref, 0, strlen("ref:"));
>> +                       strbuf_trim(ref);
>> +               } else if (is_detached) {
>> +                       *is_detached = 1;
>
> I find the placement of the detached detection logic here a bit
> strange. The only case for which it might trigger is the so-called
> "main worktree", yet having it in this general purpose parse function
> seems to imply misleadingly that any worktree could be detached. Also,
> given the current world order[1], wouldn't missing "ref:" indicate an
> error for any worktree other than the main one? Consequently, this
> detection probably ought to be done only for the main worktree
> (outside of this function, probably).

Eh, ignore this bit. My brain was conflating 'bare' and 'detached'.

Re: [PATCH v7 1/3] worktree: add top-level worktree.c

[Mike Rappazzo]

On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> I realize that this is modeled closely after existing code in
> branch.c, but, with the exception of parsing the ref file and
> constructing a worktree structure, the main worktree case (id == NULL)
> is entirely disjoint from the linked worktree case (id != NULL). This
> suggests strongly that get_worktree() should be split into two
> functions, one for the main worktree and one for linked worktrees,
> which would make the code easier to understand. You might call the
> functions get_main_worktree() and get_linked_worktree(id) (or perhaps
> drop "linked" from the latter name).

I originally wrote it like that, but I felt that the code looked like
it was mostly duplicated in the functions.
I can give it a relook.

>> +
>> +struct worktree_list *get_worktree_list()
>
> Can we be more concise and call this get_worktrees()?
>

I prefer 'get_worktree_list' because I also added the 'get_worktree'
function, and I wanted to differentiate
the function names.

>> diff --git a/worktree.h b/worktree.h
>> new file mode 100644
>> index 0000000..2bc0ab8
>> --- /dev/null
>> +++ b/worktree.h
>> @@ -0,0 +1,48 @@
>> +#ifndef WORKTREE_H
>> +#define WORKTREE_H
>> +
>> +struct worktree {
>> +       char *path;
>> +       char *git_dir;
>> +       char *head_ref;
>> +       unsigned char head_sha1[20];
>> +       int is_detached;
>> +       int is_bare;
>> +};
>> +
>> +struct worktree_list {
>> +       struct worktree *worktree;
>> +       struct worktree_list *next;
>> +};
>
> I don't care too strongly, but an alternate approach (which I probably
> would have taken) would be to have get_worktrees() simply return an
> array of 'struct worktree' objects, hence no need for the additional
> 'struct worktree_list'. The slight complication with this approach,
> though, is that get_worktrees() either also needs to return the length
> of the array, or the array should end with some sort of end-of-array
> sentinel. An obvious sentinel would be path==NULL or git_dir==NULL or
> all of the above.
>
> Client iteration is just about the same with the array approach as
> with the linked-list approach.
>

I can't see what benefit this would provide.  I would sooner change
the returned list into
an array-backed list struct.  Alternatively, I think adding a
list_head pointer to this structure
could benefit client code.

Re: [PATCH v7 1/3] worktree: add top-level worktree.c

[Junio C Hamano]

Mike Rappazzo <rappazzo@gmail.com> writes:

> On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> +struct worktree_list *get_worktree_list()
>>
>> Can we be more concise and call this get_worktrees()?
>
> I prefer 'get_worktree_list' because I also added the 'get_worktree'
> function, and I wanted to differentiate
> the function names.

I'd say that plural can be differentiating enough; it probably is a
matter of taste.  How often do external callers want to call
get_worktree() and not get_worktrees()?

>>> diff --git a/worktree.h b/worktree.h
>>> new file mode 100644
>>> index 0000000..2bc0ab8
>>> --- /dev/null
>>> +++ b/worktree.h
>>> @@ -0,0 +1,48 @@
>>> +#ifndef WORKTREE_H
>>> +#define WORKTREE_H
>>> +
>>> +struct worktree {
>>> +       char *path;
>>> +       char *git_dir;
>>> +       char *head_ref;
>>> +       unsigned char head_sha1[20];
>>> +       int is_detached;
>>> +       int is_bare;
>>> +};
>>> +
>>> +struct worktree_list {
>>> +       struct worktree *worktree;
>>> +       struct worktree_list *next;
>>> +};
>>
>> I don't care too strongly, but an alternate approach (which I probably
>> would have taken) would be to have get_worktrees() simply return an
>> array of 'struct worktree' objects, hence no need for the additional
>> 'struct worktree_list'.

I do not think we are using this to hold thousands of worktree
objects in core.  Adding "struct worktree *next" pointer to the
worktree object itself would probably be sufficient for the need of
codepaths that want to enumerate and iterate over them and that
would be another way to lose the extra structure.

Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c

[Mike Rappazzo]

On Sat, Sep 12, 2015 at 11:19 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:
>> The code formerly in branch.c was largely the basis of the
>> get_worktree_list implementation is now moved to worktree.c,
>> and the find_shared_symref implementation has been refactored
>> to use get_worktree_list
>
> Some comments below in addition to those by Junio...
>
>> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
>> ---
>> diff --git a/branch.h b/branch.h
>> index d3446ed..58aa45f 100644
>> --- a/branch.h
>> +++ b/branch.h
>> @@ -59,12 +59,4 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name);
>>   */
>>  extern void die_if_checked_out(const char *branch);
>>
>> -/*
>> - * Check if a per-worktree symref points to a ref in the main worktree
>> - * or any linked worktree, and return the path to the exising worktree
>> - * if it is.  Returns NULL if there is no existing ref.  The caller is
>> - * responsible for freeing the returned path.
>> - */
>> -extern char *find_shared_symref(const char *symref, const char *target);
>> -
>>  #endif
>> diff --git a/worktree.c b/worktree.c
>> index 33d2e57..e45b651 100644
>> --- a/worktree.c
>> +++ b/worktree.c
>> @@ -155,3 +155,43 @@ done:
>>         return list;
>>  }
>>
>> +char *find_shared_symref(const char *symref, const char *target)
>> +{
>> +       char *existing = NULL;
>> +       struct strbuf path = STRBUF_INIT;
>> +       struct strbuf sb = STRBUF_INIT;
>> +       struct worktree_list *worktree_list = get_worktree_list();
>> +       struct worktree_list *orig_list = worktree_list;
>> +       struct worktree *matched = NULL;
>> +
>> +       while (!matched && worktree_list) {
>> +               if (strcmp("HEAD", symref)) {
>
> The result of strcmp() never changes, so this (expensive) check could
> be lifted out of the 'while' loop, however...
>
>> +                       strbuf_reset(&path);
>> +                       strbuf_reset(&sb);
>> +                       strbuf_addf(&path, "%s/%s", worktree_list->worktree->git_dir, symref);
>> +
>> +                       if (_parse_ref(path.buf, &sb, NULL)) {
>> +                               continue;
>> +                       }
>> +
>> +                       if (!strcmp(sb.buf, target))
>> +                               matched = worktree_list->worktree;
>
> The original code in branch.c, which this patch removes, did not need
> to make a special case of HEAD, so it's not immediately clear why this
> replacement code does so. This is the sort of issue which argues in
> favor of mutating the existing code (slowly) over the course of
> several patches into the final form, rather than having the final form
> come into existence out of thin air. When the changes are made
> incrementally, it is easier for reviewers to understand why such
> modifications are needed, which (hopefully) should lead to fewer
> questions such as this one.
>

I reversed the the check here; it is intended to check if the symref
is _not_ the head, since the head
ref has already been parsed.  This is used in notes.c to find
"NOTES_MERGE_REF".  I will move the
check out of the loop as you suggest above.

>> +               } else {
>> +                       if (worktree_list->worktree->head_ref && !strcmp(worktree_list->worktree->head_ref, target))
>> +                               matched = worktree_list->worktree;
>> +               }
>> +               worktree_list = worktree_list->next ? worktree_list->next : NULL;
>> +       }
>> +
>> +       if (matched) {
>> +               existing = malloc(strlen(matched->path) + 1);
>> +               strcpy(existing, matched->path);
>
> A couple issues:
>
> This can be done more concisely and safely with xstrdup().
>
> In this codebase, it probably would be more idiomatic to use goto to
> drop out of the loop rather than setting 'matched' and then having to
> check 'matched' in the loop condition. So, for instance, each place
> which sets 'matched' could instead say:
>
>     existing = xstrdup(...);
>     goto done;
>
>> +       }
>> +
>> +done:
>
> This label doesn't have any matching goto's.
>
>> +       strbuf_release(&path);
>> +       strbuf_release(&sb);
>> +       worktree_list_release(orig_list);
>> +
>> +       return existing;
>> +}
>> diff --git a/worktree.h b/worktree.h
>> index 2bc0ab8..320f17e 100644
>> --- a/worktree.h
>> +++ b/worktree.h
>> @@ -45,4 +45,11 @@ struct worktree *get_worktree(const char *id);
>>  extern void worktree_list_release(struct worktree_list *);
>>  extern void worktree_release(struct worktree *);
>>
>> +/*
>> + * Look for a symref in any worktree, and return the path to the first
>> + * worktree found or NULL if not found.  The caller is responsible for
>> + * freeing the returned path.
>> + */
>
> For some reason, this comment differs a fair bit from the original in
> branch.h which was removed by this patch, however, the original
> comment was a bit more explanatory (I think).
>
> As a general rule, it's better to do code movement and code changes in
> separate patches since it's hard for reviewers to detect and
> comprehend differences in code which has been both moved and changed
> at the same time.
>

I have rebased the history a bit for the reroll, and hopefully it will
show the changes a little
more clearly.

>> +extern char *find_shared_symref(const char *symref, const char *target);
>> +
>>  #endif
>> --
>> 2.5.0

Re: [PATCH v7 1/3] worktree: add top-level worktree.c

[Eric Sunshine]

On Mon, Sep 14, 2015 at 8:20 AM, Mike Rappazzo <rappazzo@gmail.com> wrote:
> On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> +struct worktree {
>>> +       char *path;
>>> +       char *git_dir;
>>> +       char *head_ref;
>>> +       unsigned char head_sha1[20];
>>> +       int is_detached;
>>> +       int is_bare;
>>> +};
>>> +
>>> +struct worktree_list {
>>> +       struct worktree *worktree;
>>> +       struct worktree_list *next;
>>> +};
>>
>> I don't care too strongly, but an alternate approach (which I probably
>> would have taken) would be to have get_worktrees() simply return an
>> array of 'struct worktree' objects, hence no need for the additional
>> 'struct worktree_list'. The slight complication with this approach,
>> though, is that get_worktrees() either also needs to return the length
>> of the array, or the array should end with some sort of end-of-array
>> sentinel. An obvious sentinel would be path==NULL or git_dir==NULL or
>> all of the above.
>>
>> Client iteration is just about the same with the array approach as
>> with the linked-list approach.
>
> I can't see what benefit this would provide.  I would sooner change
> the returned list into
> an array-backed list struct.  Alternatively, I think adding a
> list_head pointer to this structure
> could benefit client code.

The benefit is a reduction in complexity, which is an important goal.
Linked lists are inherently more complex than arrays, requiring extra
effort, and especially extra care, to manage the head, each "next"
pointer (and possibly a tail pointer). Arrays are used often in Git,
thus are familiar in this code-base, and Git has decent support for
making their management relatively painless. Your suggestions of
changing into "an array-backed list structure" or "adding list_head
pointer" increase complexity, rather than reduce it.

Aside from the complexity issue, arrays allow random access, whereas
linked lists allow only sequential access. While this limitation may
not impact your current, planned use of get_worktrees(), it places a
potentially unnecessary restriction on future clients.

And, as noted, client iteration is at least as convenient with arrays,
if not moreso, due to the reduction in noise ("p++" rather than "p =
p->next").

    struct worktree *p;
    for (p = get_worktrees(); p->path; p++)
        blarp(p);

There are cases where linked lists are an obvious win, but this
doesn't seem to be such a case. On the other hand, there are obvious
benefits to making this an array, such as reduced complexity and
removal of the sequential-access-only restriction.

Re: [PATCH v7 1/3] worktree: add top-level worktree.c

[Eric Sunshine]

On Mon, Sep 14, 2015 at 1:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Mike Rappazzo <rappazzo@gmail.com> writes:
>> On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> +struct worktree_list *get_worktree_list()
>>>
>>> Can we be more concise and call this get_worktrees()?
>>
>> I prefer 'get_worktree_list' because I also added the 'get_worktree'
>> function, and I wanted to differentiate
>> the function names.
>
> I'd say that plural can be differentiating enough; it probably is a
> matter of taste.  How often do external callers want to call
> get_worktree() and not get_worktrees()?

The shorter name, get_worktrees(), also has the minor benefit of
concision, similar to the way we use short variable names (i, j, n, p,
s) to help reveal and (often) make code structure obvious at a glance;
whereas long, noisy, wordy names tend to obscure code structure.

The "_list" suffix doesn't add any value over the shorter pluralizing
"s"; in fact, it may be (very, very slightly) detrimental in implying
too strongly that the return value must be a linked list.

>>>> +struct worktree {
>>>> +       char *path;
>>>> +       char *git_dir;
>>>> +       char *head_ref;
>>>> +       unsigned char head_sha1[20];
>>>> +       int is_detached;
>>>> +       int is_bare;
>>>> +};
>>>> +
>>>> +struct worktree_list {
>>>> +       struct worktree *worktree;
>>>> +       struct worktree_list *next;
>>>> +};
>>>
>>> I don't care too strongly, but an alternate approach (which I probably
>>> would have taken) would be to have get_worktrees() simply return an
>>> array of 'struct worktree' objects, hence no need for the additional
>>> 'struct worktree_list'.
>
> I do not think we are using this to hold thousands of worktree
> objects in core.  Adding "struct worktree *next" pointer to the
> worktree object itself would probably be sufficient for the need of
> codepaths that want to enumerate and iterate over them and that
> would be another way to lose the extra structure.

I was more concerned with the inherent (and, in this case,
unnecessary) complexity of a linked list. Being able to drop the extra
'worktree_list' structure was just an added benefit of moving to the
simpler array approach.

Re: [PATCH v7 1/3] worktree: add top-level worktree.c

[Mike Rappazzo]

On Wed, Sep 16, 2015 at 4:32 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Sep 14, 2015 at 8:20 AM, Mike Rappazzo <rappazzo@gmail.com> wrote:
>> On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> +struct worktree {
>>>> +       char *path;
>>>> +       char *git_dir;
>>>> +       char *head_ref;
>>>> +       unsigned char head_sha1[20];
>>>> +       int is_detached;
>>>> +       int is_bare;
>>>> +};
>>>> +
>>>> +struct worktree_list {
>>>> +       struct worktree *worktree;
>>>> +       struct worktree_list *next;
>>>> +};
>>>
>>> I don't care too strongly, but an alternate approach (which I probably
>>> would have taken) would be to have get_worktrees() simply return an
>>> array of 'struct worktree' objects, hence no need for the additional
>>> 'struct worktree_list'. The slight complication with this approach,
>>> though, is that get_worktrees() either also needs to return the length
>>> of the array, or the array should end with some sort of end-of-array
>>> sentinel. An obvious sentinel would be path==NULL or git_dir==NULL or
>>> all of the above.
>>>
>>> Client iteration is just about the same with the array approach as
>>> with the linked-list approach.
>>
>> I can't see what benefit this would provide.  I would sooner change
>> the returned list into
>> an array-backed list struct.  Alternatively, I think adding a
>> list_head pointer to this structure
>> could benefit client code.
>
> The benefit is a reduction in complexity, which is an important goal.
> Linked lists are inherently more complex than arrays, requiring extra
> effort, and especially extra care, to manage the head, each "next"
> pointer (and possibly a tail pointer). Arrays are used often in Git,
> thus are familiar in this code-base, and Git has decent support for
> making their management relatively painless. Your suggestions of
> changing into "an array-backed list structure" or "adding list_head
> pointer" increase complexity, rather than reduce it.
>
> Aside from the complexity issue, arrays allow random access, whereas
> linked lists allow only sequential access. While this limitation may
> not impact your current, planned use of get_worktrees(), it places a
> potentially unnecessary restriction on future clients.
>
> And, as noted, client iteration is at least as convenient with arrays,
> if not moreso, due to the reduction in noise ("p++" rather than "p =
> p->next").
>
>     struct worktree *p;
>     for (p = get_worktrees(); p->path; p++)
>         blarp(p);
>
> There are cases where linked lists are an obvious win, but this
> doesn't seem to be such a case. On the other hand, there are obvious
> benefits to making this an array, such as reduced complexity and
> removal of the sequential-access-only restriction.

What you are suggesting makes sense, but I feel it falls short when it
comes to the "sentinel".  Would the last element in the list be a
dummy worktree?  I would sooner add a NULL to the end.  Would that be
an acceptable implementation?

I have re-coded it to put the next pointer in the worktree structure
as Junio has suggested, but I am open to changing it to use the array
approach.

Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c

[Eric Sunshine]

On Mon, Sep 14, 2015 at 1:44 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:
> On Sat, Sep 12, 2015 at 11:19 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:
>>> +       while (!matched && worktree_list) {
>>> +               if (strcmp("HEAD", symref)) {
>>> +                       strbuf_reset(&path);
>>> +                       strbuf_reset(&sb);
>>> +                       strbuf_addf(&path, "%s/%s", worktree_list->worktree->git_dir, symref);
>>> +
>>> +                       if (_parse_ref(path.buf, &sb, NULL)) {
>>> +                               continue;
>>> +                       }
>>> +
>>> +                       if (!strcmp(sb.buf, target))
>>> +                               matched = worktree_list->worktree;
>>
>> The original code in branch.c, which this patch removes, did not need
>> to make a special case of HEAD, so it's not immediately clear why this
>> replacement code does so. This is the sort of issue which argues in
>> favor of mutating the existing code (slowly) over the course of
>> several patches into the final form, rather than having the final form
>> come into existence out of thin air. When the changes are made
>> incrementally, it is easier for reviewers to understand why such
>> modifications are needed, which (hopefully) should lead to fewer
>> questions such as this one.
>
> I reversed the the check here; it is intended to check if the symref
> is _not_ the head, since the head
> ref has already been parsed.  This is used in notes.c to find
> "NOTES_MERGE_REF".

I'm probably being dense, but I still don't understand why the code
now needs a special case for HEAD, whereas the original didn't. But,
my denseness my be indicative of this change not being well-described
(or described at all) by the commit message. Hopefully, when this is
refactored into finer changes, the purpose will become clear.

Thanks.

Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c

[Mike Rappazzo]

On Wed, Sep 16, 2015 at 5:09 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Sep 14, 2015 at 1:44 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:
>> On Sat, Sep 12, 2015 at 11:19 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:
>>>> +       while (!matched && worktree_list) {
>>>> +               if (strcmp("HEAD", symref)) {
>>>> +                       strbuf_reset(&path);
>>>> +                       strbuf_reset(&sb);
>>>> +                       strbuf_addf(&path, "%s/%s", worktree_list->worktree->git_dir, symref);
>>>> +
>>>> +                       if (_parse_ref(path.buf, &sb, NULL)) {
>>>> +                               continue;
>>>> +                       }
>>>> +
>>>> +                       if (!strcmp(sb.buf, target))
>>>> +                               matched = worktree_list->worktree;
>>>
>>> The original code in branch.c, which this patch removes, did not need
>>> to make a special case of HEAD, so it's not immediately clear why this
>>> replacement code does so. This is the sort of issue which argues in
>>> favor of mutating the existing code (slowly) over the course of
>>> several patches into the final form, rather than having the final form
>>> come into existence out of thin air. When the changes are made
>>> incrementally, it is easier for reviewers to understand why such
>>> modifications are needed, which (hopefully) should lead to fewer
>>> questions such as this one.
>>
>> I reversed the the check here; it is intended to check if the symref
>> is _not_ the head, since the head
>> ref has already been parsed.  This is used in notes.c to find
>> "NOTES_MERGE_REF".
>
> I'm probably being dense, but I still don't understand why the code
> now needs a special case for HEAD, whereas the original didn't. But,
> my denseness my be indicative of this change not being well-described
> (or described at all) by the commit message. Hopefully, when this is
> refactored into finer changes, the purpose will become clear.
>
> Thanks.

The special case for HEAD is because the HEAD is already included in
the worktree struct.  This block is intended to save from re-parsing.
If you think the code would be easier to read, the HEAD check could be
removed, and the ref will just be parsed always.

Re: [PATCH v7 1/3] worktree: add top-level worktree.c

[Eric Sunshine]

On Wed, Sep 16, 2015 at 4:49 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:
> On Wed, Sep 16, 2015 at 4:32 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Mon, Sep 14, 2015 at 8:20 AM, Mike Rappazzo <rappazzo@gmail.com> wrote:
>>> On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> I don't care too strongly, but an alternate approach (which I probably
>>>> would have taken) would be to have get_worktrees() simply return an
>>>> array of 'struct worktree' objects, hence no need for the additional
>>>> 'struct worktree_list'. The slight complication with this approach,
>>>> though, is that get_worktrees() either also needs to return the length
>>>> of the array, or the array should end with some sort of end-of-array
>>>> sentinel. An obvious sentinel would be path==NULL or git_dir==NULL or
>>>> all of the above.
>>>>
>>>> Client iteration is just about the same with the array approach as
>>>> with the linked-list approach.
>>>
>>> I can't see what benefit this would provide.  I would sooner change
>>> the returned list into
>>> an array-backed list struct.  Alternatively, I think adding a
>>> list_head pointer to this structure
>>> could benefit client code.
>>
>> The benefit is a reduction in complexity, which is an important goal.
>> Linked lists are inherently more complex than arrays, requiring extra
>> effort, and especially extra care, to manage the head, each "next"
>> pointer (and possibly a tail pointer). Arrays are used often in Git,
>> thus are familiar in this code-base, and Git has decent support for
>> making their management relatively painless. Your suggestions of
>> changing into "an array-backed list structure" or "adding list_head
>> pointer" increase complexity, rather than reduce it.
>>
>> Aside from the complexity issue, arrays allow random access, whereas
>> linked lists allow only sequential access. While this limitation may
>> not impact your current, planned use of get_worktrees(), it places a
>> potentially unnecessary restriction on future clients.
>>
>> And, as noted, client iteration is at least as convenient with arrays,
>> if not moreso, due to the reduction in noise ("p++" rather than "p =
>> p->next").
>>
>>     struct worktree *p;
>>     for (p = get_worktrees(); p->path; p++)
>>         blarp(p);
>>
>> There are cases where linked lists are an obvious win, but this
>> doesn't seem to be such a case. On the other hand, there are obvious
>> benefits to making this an array, such as reduced complexity and
>> removal of the sequential-access-only restriction.
>
> What you are suggesting makes sense, but I feel it falls short when it
> comes to the "sentinel".  Would the last element in the list be a
> dummy worktree?  I would sooner add a NULL to the end.  Would that be
> an acceptable implementation?

The sentinel would indeed be a dummy worktree structure, which may be
very slightly ugly, however, it's dead simple, both in implementation
and from client viewpoint, whereas other approaches increase
complexity.

Making the last entry a NULL means get_worktrees() would have to
return an array of pointers rather than an array of structures, which
is more syntactically noisy, and complex since it's harder to reason
about pointer-to-pointer. In my mind, at least, the simplicity of the
array of structures approach (even with the slight ugliness of the
dummy sentinel) outweighs the complexity of the array-of-pointers
approach.

Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c

[Eric Sunshine]

On Wed, Sep 16, 2015 at 5:36 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:
> On Wed, Sep 16, 2015 at 5:09 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Mon, Sep 14, 2015 at 1:44 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:
>>> On Sat, Sep 12, 2015 at 11:19 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> The original code in branch.c, which this patch removes, did not need
>>>> to make a special case of HEAD, so it's not immediately clear why this
>>>> replacement code does so. This is the sort of issue which argues in
>>>> favor of mutating the existing code (slowly) over the course of
>>>> several patches into the final form, rather than having the final form
>>>> come into existence out of thin air. When the changes are made
>>>> incrementally, it is easier for reviewers to understand why such
>>>> modifications are needed, which (hopefully) should lead to fewer
>>>> questions such as this one.
>>>
>>> I reversed the the check here; it is intended to check if the symref
>>> is _not_ the head, since the head
>>> ref has already been parsed.  This is used in notes.c to find
>>> "NOTES_MERGE_REF".
>>
>> I'm probably being dense, but I still don't understand why the code
>> now needs a special case for HEAD, whereas the original didn't. But,
>> my denseness my be indicative of this change not being well-described
>> (or described at all) by the commit message. Hopefully, when this is
>> refactored into finer changes, the purpose will become clear.
>
> The special case for HEAD is because the HEAD is already included in
> the worktree struct.  This block is intended to save from re-parsing.
> If you think the code would be easier to read, the HEAD check could be
> removed, and the ref will just be parsed always.

I'm unable to judge whether the code would be easier to read since I
still don't understand the purpose of the special case. (But, as
noted, it may just be that I'm being dense, though not intentionally.)

Re: [PATCH v7 1/3] worktree: add top-level worktree.c

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Making the last entry a NULL means get_worktrees() would have to
> return an array of pointers rather than an array of structures, which
> is more syntactically noisy, and complex since it's harder to reason
> about pointer-to-pointer. In my mind, at least, the simplicity of the
> array of structures approach (even with the slight ugliness of the
> dummy sentinel) outweighs the complexity of the array-of-pointers
> approach.

Hmph, I think the bog-standard way in our code is an array of
pointers.  An array of structures have a few downsides:

 - You have to copy a lot when you do realloc(3);

 - You have to move a lot when you insert a new element;

 - Individual structure cannot be pointed at by a pointer sensibly.
   Passing &worktree[4] to a function that expects "struct worktree *"
   is unsafe unless you are sure nobody is mucking with the worktree[]
   array.

For the read-only operation "worktree list", the last one may not
matter much because you would build all the elements before doing
anything else, but once you want to run this inside a library and
maintain the in-core forest of worktrees that are in sync with what
your running process does (i.e. create a new or destroy an existing
worktree), it may become problematic.