{"thread":{"id":"40276","subject":"[PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c","startedAt":"2015-09-04T21:39:26Z","lastAt":"2015-09-22T01:17:16Z","messageCount":27,"participants":["Michael Rappazzo","Junio C Hamano","Mike Rappazzo","Eric Sunshine"],"isPatch":true,"patchVersion":7,"patchTotal":3},"messages":[{"id":"269445","messageId":"1441402769-35897-1-git-send-email-rappazzo@gmail.com","threadId":"40276","inReplyTo":null,"subject":"[PATCH v7 0/3] worktree: worktree.c functions and list builtin command","fromName":"Michael Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2015-09-04T21:39:26Z","receivedAt":"2015-09-04T21:39:26Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"This series represents a major rewrite of the previous series.  It introduces\na top-level worktree.c which includes `get_worktree_list` and `get_worktree`\nfunctions, and it also moves the `find_shared_symref` from branch.c to \nworktree.c\n\nMichael Rappazzo (3):\n  worktree: add top-level worktree.c\n  worktree: move/refactor find_shared_symref from branch.c\n  worktree: add 'list' command\n\n Documentation/git-worktree.txt |  10 ++-\n Makefile                       |   1 +\n branch.c                       |  79 +----------------\n branch.h                       |   8 --\n builtin/notes.c                |   1 +\n builtin/worktree.c             |  63 +++++++++++++\n t/t2027-worktree-list.sh       | 122 +++++++++++++++++++++++++\n worktree.c                     | 197 +++++++++++++++++++++++++++++++++++++++++\n worktree.h                     |  55 ++++++++++++\n 9 files changed, 449 insertions(+), 87 deletions(-)\n create mode 100755 t/t2027-worktree-list.sh\n create mode 100644 worktree.c\n create mode 100644 worktree.h\n\n-- \n2.5.0\n"},{"id":"269446","messageId":"1441402769-35897-2-git-send-email-rappazzo@gmail.com","threadId":"40276","inReplyTo":"1441402769-35897-1-git-send-email-rappazzo@gmail.com","subject":"[PATCH v7 1/3] worktree: add top-level worktree.c","fromName":"Michael Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2015-09-04T21:39:27Z","receivedAt":"2015-09-04T21:39:27Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"Including functions to get the list of all worktrees, and to get\na specific worktree (primary or linked).\n\nSigned-off-by: Michael Rappazzo <rappazzo@gmail.com>\n---\n Makefile   |   1 +\n worktree.c | 157 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n worktree.h |  48 +++++++++++++++++++\n 3 files changed, 206 insertions(+)\n create mode 100644 worktree.c\n create mode 100644 worktree.h\n\ndiff --git a/Makefile b/Makefile\nindex e326fa0..0131fed 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -807,6 +807,7 @@ LIB_OBJS += version.o\n LIB_OBJS += versioncmp.o\n LIB_OBJS += walker.o\n LIB_OBJS += wildmatch.o\n+LIB_OBJS += worktree.o\n LIB_OBJS += wrapper.o\n LIB_OBJS += write_or_die.o\n LIB_OBJS += ws.o\ndiff --git a/worktree.c b/worktree.c\nnew file mode 100644\nindex 0000000..33d2e57\n--- /dev/null\n+++ b/worktree.c\n@@ -0,0 +1,157 @@\n+#include \"worktree.h\"\n+#include \"cache.h\"\n+#include \"git-compat-util.h\"\n+#include \"refs.h\"\n+#include \"strbuf.h\"\n+\n+void worktree_release(struct worktree *worktree)\n+{\n+\tif (worktree) {\n+\t\tfree(worktree->path);\n+\t\tfree(worktree->git_dir);\n+\t\tif (!worktree->is_detached) {\n+\t\t\t/* could be headless */\n+\t\t\tfree(worktree->head_ref);\n+\t\t}\n+\t\tfree(worktree);\n+\t}\n+}\n+\n+void worktree_list_release(struct worktree_list *worktree_list)\n+{\n+\twhile (worktree_list) {\n+\t\tstruct worktree_list *next = worktree_list->next;\n+\t\tworktree_release(worktree_list->worktree);\n+\t\tfree (worktree_list);\n+\t\tworktree_list = next;\n+\t}\n+}\n+\n+/*\n+ * read 'path_to_ref' into 'ref'.  Also set is_detached to 1 if the ref is detatched\n+ *\n+ * return 1 if the ref is not a proper ref, 0 otherwise (success)\n+ */\n+int _parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)\n+{\n+\tif (!strbuf_readlink(ref, path_to_ref, 0)) {\n+\t\tif (!starts_with(ref->buf, \"refs/\") || check_refname_format(ref->buf, 0)) {\n+\t\t\t/* invalid ref - something is awry with this repo */\n+\t\t\treturn 1;\n+\t\t}\n+\t} else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {\n+\t\tif (starts_with(ref->buf, \"ref:\")) {\n+\t\t\tstrbuf_remove(ref, 0, strlen(\"ref:\"));\n+\t\t\tstrbuf_trim(ref);\n+\t\t} else if (is_detached) {\n+\t\t\t*is_detached = 1;\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n+struct worktree *get_worktree(const char *id)\n+{\n+\tstruct worktree *worktree = NULL;\n+\tstruct strbuf path = STRBUF_INIT;\n+\tstruct strbuf worktree_path = STRBUF_INIT;\n+\tstruct strbuf git_dir = STRBUF_INIT;\n+\tstruct strbuf head_ref = STRBUF_INIT;\n+\tint is_bare = 0;\n+\tint is_detached = 0;\n+\n+\tif (id) {\n+\t\tstrbuf_addf(&git_dir, \"%s/worktrees/%s\", absolute_path(get_git_common_dir()), id);\n+\t\tstrbuf_addf(&path, \"%s/gitdir\", git_dir.buf);\n+\t\tif (strbuf_read_file(&worktree_path, path.buf, 0) <= 0) {\n+\t\t\t/* invalid git_dir file */\n+\t\t\tgoto done;\n+\t\t}\n+\t\tstrbuf_rtrim(&worktree_path);\n+\t\tif (!strbuf_strip_suffix(&worktree_path, \"/.git\")) {\n+\t\t\tstrbuf_reset(&worktree_path);\n+\t\t\tstrbuf_addstr(&worktree_path, absolute_path(\".\"));\n+\t\t\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n+\t\t}\n+\n+\t\tstrbuf_reset(&path);\n+\t\tstrbuf_addf(&path, \"%s/worktrees/%s/HEAD\", get_git_common_dir(), id);\n+\t} else {\n+\t\tstrbuf_addf(&git_dir, \"%s\", absolute_path(get_git_common_dir()));\n+\t\tstrbuf_addf(&worktree_path, \"%s\", absolute_path(get_git_common_dir()));\n+\t\tis_bare = !strbuf_strip_suffix(&worktree_path, \"/.git\");\n+\t\tif (is_bare)\n+\t\t\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n+\n+\t\tstrbuf_addf(&path, \"%s/HEAD\", get_git_common_dir());\n+\t}\n+\n+\t/*\n+\t * $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside $GIT_DIR so for linked worktrees,\n+\t * `resolve_ref_unsafe()` won't work (it uses git_path). Parse the ref ourselves.\n+\t */\n+\tif (_parse_ref(path.buf, &head_ref, &is_detached))\n+\t\tgoto done;\n+\n+\tworktree = malloc(sizeof(struct worktree));\n+\tworktree->path = strbuf_detach(&worktree_path, NULL);\n+\tworktree->git_dir = strbuf_detach(&git_dir, NULL);\n+\tworktree->is_bare = is_bare;\n+\tworktree->head_ref = NULL;\n+\tworktree->is_detached = is_detached;\n+\tif (strlen(head_ref.buf) > 0) {\n+\t\tif (!is_detached) {\n+\t\t\tresolve_ref_unsafe(head_ref.buf, 0, worktree->head_sha1, NULL);\n+\t\t\tworktree->head_ref = strbuf_detach(&head_ref, NULL);\n+\t\t} else {\n+\t\t\tget_sha1_hex(head_ref.buf, worktree->head_sha1);\n+\t\t}\n+\t}\n+done:\n+\tstrbuf_release(&path);\n+\tstrbuf_release(&git_dir);\n+\tstrbuf_release(&head_ref);\n+\tstrbuf_release(&worktree_path);\n+\treturn worktree;\n+}\n+\n+struct worktree_list *get_worktree_list()\n+{\n+\tstruct worktree_list *list = NULL;\n+\tstruct worktree_list *current_entry = NULL;\n+\tstruct worktree *current_worktree = NULL;\n+\tstruct strbuf path = STRBUF_INIT;\n+\tDIR *dir;\n+\tstruct dirent *d;\n+\n+\tcurrent_worktree = get_worktree(NULL);\n+\tif (current_worktree) {\n+\t\tlist = malloc(sizeof(struct worktree_list));\n+\t\tlist->worktree = current_worktree;\n+\t\tlist->next = NULL;\n+\t\tcurrent_entry = list;\n+\t}\n+\tstrbuf_addf(&path, \"%s/worktrees\", get_git_common_dir());\n+\tdir = opendir(path.buf);\n+\tstrbuf_release(&path);\n+\tif (dir) {\n+\t\twhile ((d = readdir(dir)) != NULL) {\n+\t\t\tif (!strcmp(d->d_name, \".\") || !strcmp(d->d_name, \"..\"))\n+\t\t\t\tcontinue;\n+\n+\t\t\tcurrent_worktree = get_worktree(d->d_name);\n+\t\t\tif (current_worktree) {\n+\t\t\t\tcurrent_entry->next = malloc(sizeof(struct worktree_list));\n+\t\t\t\tcurrent_entry = current_entry->next;\n+\t\t\t\tcurrent_entry->worktree = current_worktree;\n+\t\t\t\tcurrent_entry->next = NULL;\n+\t\t\t}\n+\t\t}\n+\t\tclosedir(dir);\n+\t}\n+\n+done:\n+\n+\treturn list;\n+}\n+\ndiff --git a/worktree.h b/worktree.h\nnew file mode 100644\nindex 0000000..2bc0ab8\n--- /dev/null\n+++ b/worktree.h\n@@ -0,0 +1,48 @@\n+#ifndef WORKTREE_H\n+#define WORKTREE_H\n+\n+struct worktree {\n+\tchar *path;\n+\tchar *git_dir;\n+\tchar *head_ref;\n+\tunsigned char head_sha1[20];\n+\tint is_detached;\n+\tint is_bare;\n+};\n+\n+struct worktree_list {\n+\tstruct worktree *worktree;\n+\tstruct worktree_list *next;\n+};\n+\n+/* Functions for acting on the information about worktrees. */\n+\n+/*\n+ * Get the list of all worktrees.  The primary worktree will always be\n+ * the first in the list followed by any other (linked) worktrees created\n+ * by `git worktree add`.  No specific ordering is done on the linked\n+ * worktrees.\n+ *\n+ * The caller is responsible for freeing the memory from the returned list.\n+ * (See worktree_list_release for this purpose).\n+ */\n+extern struct worktree_list *get_worktree_list();\n+\n+/*\n+ * generic method to get a worktree\n+ *   - if 'id' is NULL, get the from $GIT_COMMON_DIR\n+ *   - if 'id' is not NULL, get the worktree found in $GIT_COMMON_DIR/worktrees/id if\n+ *     such a worktree exists\n+ *\n+ * The caller is responsible for freeing the memory from the returned\n+ * worktree.  (See worktree_release for this purpose)\n+ */\n+struct worktree *get_worktree(const char *id);\n+\n+/*\n+ * Free up the memory for a worktree_list/worktree\n+ */\n+extern void worktree_list_release(struct worktree_list *);\n+extern void worktree_release(struct worktree *);\n+\n+#endif\n-- \n2.5.0\n"},{"id":"269444","messageId":"1441402769-35897-3-git-send-email-rappazzo@gmail.com","threadId":"40276","inReplyTo":"1441402769-35897-1-git-send-email-rappazzo@gmail.com","subject":"[PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c","fromName":"Michael Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2015-09-04T21:39:28Z","receivedAt":"2015-09-04T21:39:28Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"The code formerly in branch.c was largely the basis of the\nget_worktree_list implementation is now moved to worktree.c,\nand the find_shared_symref implementation has been refactored\nto use get_worktree_list\n\nSigned-off-by: Michael Rappazzo <rappazzo@gmail.com>\n---\n branch.c        | 79 +--------------------------------------------------------\n branch.h        |  8 ------\n builtin/notes.c |  1 +\n worktree.c      | 40 +++++++++++++++++++++++++++++\n worktree.h      |  7 +++++\n 5 files changed, 49 insertions(+), 86 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex d013374..77d7f2a 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -4,6 +4,7 @@\n #include \"refs.h\"\n #include \"remote.h\"\n #include \"commit.h\"\n+#include \"worktree.h\"\n \n struct tracking {\n \tstruct refspec spec;\n@@ -311,84 +312,6 @@ void remove_branch_state(void)\n \tunlink(git_path_squash_msg());\n }\n \n-static char *find_linked_symref(const char *symref, const char *branch,\n-\t\t\t\tconst char *id)\n-{\n-\tstruct strbuf sb = STRBUF_INIT;\n-\tstruct strbuf path = STRBUF_INIT;\n-\tstruct strbuf gitdir = STRBUF_INIT;\n-\tchar *existing = NULL;\n-\n-\t/*\n-\t * $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside\n-\t * $GIT_DIR so resolve_ref_unsafe() won't work (it uses\n-\t * git_path). Parse the ref ourselves.\n-\t */\n-\tif (id)\n-\t\tstrbuf_addf(&path, \"%s/worktrees/%s/%s\", get_git_common_dir(), id, symref);\n-\telse\n-\t\tstrbuf_addf(&path, \"%s/%s\", get_git_common_dir(), symref);\n-\n-\tif (!strbuf_readlink(&sb, path.buf, 0)) {\n-\t\tif (!starts_with(sb.buf, \"refs/\") ||\n-\t\t    check_refname_format(sb.buf, 0))\n-\t\t\tgoto done;\n-\t} else if (strbuf_read_file(&sb, path.buf, 0) >= 0 &&\n-\t    starts_with(sb.buf, \"ref:\")) {\n-\t\tstrbuf_remove(&sb, 0, strlen(\"ref:\"));\n-\t\tstrbuf_trim(&sb);\n-\t} else\n-\t\tgoto done;\n-\tif (strcmp(sb.buf, branch))\n-\t\tgoto done;\n-\tif (id) {\n-\t\tstrbuf_reset(&path);\n-\t\tstrbuf_addf(&path, \"%s/worktrees/%s/gitdir\", get_git_common_dir(), id);\n-\t\tif (strbuf_read_file(&gitdir, path.buf, 0) <= 0)\n-\t\t\tgoto done;\n-\t\tstrbuf_rtrim(&gitdir);\n-\t} else\n-\t\tstrbuf_addstr(&gitdir, get_git_common_dir());\n-\tstrbuf_strip_suffix(&gitdir, \".git\");\n-\n-\texisting = strbuf_detach(&gitdir, NULL);\n-done:\n-\tstrbuf_release(&path);\n-\tstrbuf_release(&sb);\n-\tstrbuf_release(&gitdir);\n-\n-\treturn existing;\n-}\n-\n-char *find_shared_symref(const char *symref, const char *target)\n-{\n-\tstruct strbuf path = STRBUF_INIT;\n-\tDIR *dir;\n-\tstruct dirent *d;\n-\tchar *existing;\n-\n-\tif ((existing = find_linked_symref(symref, target, NULL)))\n-\t\treturn existing;\n-\n-\tstrbuf_addf(&path, \"%s/worktrees\", get_git_common_dir());\n-\tdir = opendir(path.buf);\n-\tstrbuf_release(&path);\n-\tif (!dir)\n-\t\treturn NULL;\n-\n-\twhile ((d = readdir(dir)) != NULL) {\n-\t\tif (!strcmp(d->d_name, \".\") || !strcmp(d->d_name, \"..\"))\n-\t\t\tcontinue;\n-\t\texisting = find_linked_symref(symref, target, d->d_name);\n-\t\tif (existing)\n-\t\t\tgoto done;\n-\t}\n-done:\n-\tclosedir(dir);\n-\n-\treturn existing;\n-}\n-\n void die_if_checked_out(const char *branch)\n {\n \tchar *existing;\ndiff --git a/branch.h b/branch.h\nindex d3446ed..58aa45f 100644\n--- a/branch.h\n+++ b/branch.h\n@@ -59,12 +59,4 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name);\n  */\n extern void die_if_checked_out(const char *branch);\n \n-/*\n- * Check if a per-worktree symref points to a ref in the main worktree\n- * or any linked worktree, and return the path to the exising worktree\n- * if it is.  Returns NULL if there is no existing ref.  The caller is\n- * responsible for freeing the returned path.\n- */\n-extern char *find_shared_symref(const char *symref, const char *target);\n-\n #endif\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 3608c64..8b30334 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -20,6 +20,7 @@\n #include \"notes-merge.h\"\n #include \"notes-utils.h\"\n #include \"branch.h\"\n+#include \"worktree.h\"\n \n static const char * const git_notes_usage[] = {\n \tN_(\"git notes [--ref <notes-ref>] [list [<object>]]\"),\ndiff --git a/worktree.c b/worktree.c\nindex 33d2e57..e45b651 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -155,3 +155,43 @@ done:\n \treturn list;\n }\n \n+char *find_shared_symref(const char *symref, const char *target)\n+{\n+\tchar *existing = NULL;\n+\tstruct strbuf path = STRBUF_INIT;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct worktree_list *worktree_list = get_worktree_list();\n+\tstruct worktree_list *orig_list = worktree_list;\n+\tstruct worktree *matched = NULL;\n+\n+\twhile (!matched && worktree_list) {\n+\t\tif (strcmp(\"HEAD\", symref)) {\n+\t\t\tstrbuf_reset(&path);\n+\t\t\tstrbuf_reset(&sb);\n+\t\t\tstrbuf_addf(&path, \"%s/%s\", worktree_list->worktree->git_dir, symref);\n+\n+\t\t\tif (_parse_ref(path.buf, &sb, NULL)) {\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\n+\t\t\tif (!strcmp(sb.buf, target))\n+\t\t\t\tmatched = worktree_list->worktree;\n+\t\t} else {\n+\t\t\tif (worktree_list->worktree->head_ref && !strcmp(worktree_list->worktree->head_ref, target))\n+\t\t\t\tmatched = worktree_list->worktree;\n+\t\t}\n+\t\tworktree_list = worktree_list->next ? worktree_list->next : NULL;\n+\t}\n+\n+\tif (matched) {\n+\t\texisting = malloc(strlen(matched->path) + 1);\n+\t\tstrcpy(existing, matched->path);\n+\t}\n+\n+done:\n+\tstrbuf_release(&path);\n+\tstrbuf_release(&sb);\n+\tworktree_list_release(orig_list);\n+\n+\treturn existing;\n+}\ndiff --git a/worktree.h b/worktree.h\nindex 2bc0ab8..320f17e 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -45,4 +45,11 @@ struct worktree *get_worktree(const char *id);\n extern void worktree_list_release(struct worktree_list *);\n extern void worktree_release(struct worktree *);\n \n+/*\n+ * Look for a symref in any worktree, and return the path to the first\n+ * worktree found or NULL if not found.  The caller is responsible for\n+ * freeing the returned path.\n+ */\n+extern char *find_shared_symref(const char *symref, const char *target);\n+\n #endif\n-- \n2.5.0\n"},{"id":"269447","messageId":"1441402769-35897-4-git-send-email-rappazzo@gmail.com","threadId":"40276","inReplyTo":"1441402769-35897-1-git-send-email-rappazzo@gmail.com","subject":"[PATCH v7 3/3] worktree: add 'list' command","fromName":"Michael Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2015-09-04T21:39:29Z","receivedAt":"2015-09-04T21:39:29Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"'git worktree list' iterates through the worktree list, and outputs\nthe worktree dir.  By default, only the worktree path is output.\n\nSupported options include:\n\t--skip-bare: do not output bare worktrees\n\t--verbose: include the current head and ref (if applicable), also\n\t\tdecorate bare worktrees\n\nSigned-off-by: Michael Rappazzo <rappazzo@gmail.com>\n---\n Documentation/git-worktree.txt |  10 +++-\n builtin/worktree.c             |  63 +++++++++++++++++++++\n t/t2027-worktree-list.sh       | 122 +++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 194 insertions(+), 1 deletion(-)\n create mode 100755 t/t2027-worktree-list.sh\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex fb68156..b9339ed 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -11,6 +11,7 @@ SYNOPSIS\n [verse]\n 'git worktree add' [-f] [--detach] [-b <new-branch>] <path> [<branch>]\n 'git worktree prune' [-n] [-v] [--expire <expire>]\n+'git worktree list' [-v] [--skip-bare]\n \n DESCRIPTION\n -----------\n@@ -59,6 +60,10 @@ prune::\n \n Prune working tree information in $GIT_DIR/worktrees.\n \n+list::\n+\n+List the main worktree followed by each of the linked worktrees.\n+\n OPTIONS\n -------\n \n@@ -89,10 +94,14 @@ OPTIONS\n -v::\n --verbose::\n \tWith `prune`, report all removals.\n+\tWith `list`, show more information about each worktree.  This includes\n+\tif the worktree is bare, the revision currently checked out, and the\n+\tbranch currently checked out (or 'detached HEAD' if none).\n \n --expire <time>::\n \tWith `prune`, only expire unused working trees older than <time>.\n \n+\n DETAILS\n -------\n Each linked working tree has a private sub-directory in the repository's\n@@ -167,7 +176,6 @@ performed manually, such as:\n - `remove` to remove a linked working tree and its administrative files (and\n   warn if the working tree is dirty)\n - `mv` to move or rename a working tree and update its administrative files\n-- `list` to list linked working trees\n - `lock` to prevent automatic pruning of administrative files (for instance,\n   for a working tree on a portable device)\n \ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 71bb770..a0c0fe8 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -8,10 +8,13 @@\n #include \"run-command.h\"\n #include \"sigchain.h\"\n #include \"refs.h\"\n+#include \"utf8.h\"\n+#include \"worktree.h\"\n \n static const char * const worktree_usage[] = {\n \tN_(\"git worktree add [<options>] <path> <branch>\"),\n \tN_(\"git worktree prune [<options>]\"),\n+\tN_(\"git worktree list [<options>]\"),\n \tNULL\n };\n \n@@ -359,6 +362,64 @@ static int add(int ac, const char **av, const char *prefix)\n \treturn add_worktree(path, branch, &opts);\n }\n \n+static int list(int ac, const char **av, const char *prefix)\n+{\n+\tint no_bare = 0;\n+\n+\tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"skip-bare\", &no_bare,  N_(\"exclude bare worktrees from the list\")),\n+\t\tOPT__VERBOSE(&verbose, N_(\"include more worktree details\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tac = parse_options(ac, av, prefix, options, worktree_usage, 0);\n+\tif (ac)\n+\t\tusage_with_options(worktree_usage, options);\n+\telse {\n+\t\tstruct worktree_list *worktree_list = get_worktree_list();\n+\t\tstruct worktree_list *orig = worktree_list;\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tint path_maxlen = 0;\n+\n+\t\tif (verbose) {\n+\t\t\twhile (worktree_list) {\n+\t\t\t\tint cur_len = strlen(worktree_list->worktree->path);\n+\t\t\t\tif (cur_len > path_maxlen)\n+\t\t\t\t\tpath_maxlen = cur_len;\n+\t\t\t\tworktree_list = worktree_list->next ? worktree_list->next : NULL;\n+\t\t\t}\n+\t\t\tworktree_list = orig;\n+\t\t}\n+\t\twhile (worktree_list) {\n+\t\t\t/* if this work tree is not bare OR if bare repos are to be shown (default) */\n+\t\t\tif (!worktree_list->worktree->is_bare || !no_bare) {\n+\t\t\t\tstrbuf_reset(&sb);\n+\t\t\t\tif (!verbose)\n+\t\t\t\t\tstrbuf_addstr(&sb, worktree_list->worktree->path);\n+\t\t\t\telse {\n+\t\t\t\t\tint cur_len = strlen(worktree_list->worktree->path);\n+\t\t\t\t\tint utf8_adj = cur_len - utf8_strwidth(worktree_list->worktree->path);\n+\t\t\t\t\tstrbuf_addf(&sb, \"%-*s \", 1 + path_maxlen + utf8_adj, worktree_list->worktree->path);\n+\t\t\t\t\tif (worktree_list->worktree->is_bare)\n+\t\t\t\t\t\tstrbuf_addstr(&sb, \"(bare)\");\n+\t\t\t\t\telse {\n+\t\t\t\t\t\tstrbuf_addf(&sb, \"%s \", find_unique_abbrev(worktree_list->worktree->head_sha1, DEFAULT_ABBREV));\n+\t\t\t\t\t\tif (!worktree_list->worktree->is_detached)\n+\t\t\t\t\t\t\tstrbuf_addf(&sb, \"[%s]\", shorten_unambiguous_ref(worktree_list->worktree->head_ref, 0));\n+\t\t\t\t\t\telse\n+\t\t\t\t\t\t\tstrbuf_addstr(&sb, \"(detached HEAD)\");\n+\t\t\t\t\t}\n+\t\t\t\t}\n+\t\t\t\tprintf(\"%s\\n\", sb.buf);\n+\t\t\t}\n+\t\t\tworktree_list = worktree_list->next ? worktree_list->next : NULL;\n+\t\t}\n+\t\tworktree_list_release(orig);\n+\t\tstrbuf_release(&sb);\n+\t}\n+\treturn 0;\n+}\n+\n int cmd_worktree(int ac, const char **av, const char *prefix)\n {\n \tstruct option options[] = {\n@@ -371,5 +432,7 @@ int cmd_worktree(int ac, const char **av, const char *prefix)\n \t\treturn add(ac - 1, av + 1, prefix);\n \tif (!strcmp(av[1], \"prune\"))\n \t\treturn prune(ac - 1, av + 1, prefix);\n+\tif (!strcmp(av[1], \"list\"))\n+\t\treturn list(ac - 1, av + 1, prefix);\n \tusage_with_options(worktree_usage, options);\n }\ndiff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh\nnew file mode 100755\nindex 0000000..246890c\n--- /dev/null\n+++ b/t/t2027-worktree-list.sh\n@@ -0,0 +1,122 @@\n+#!/bin/sh\n+\n+test_description='test git worktree list'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit init\n+'\n+\n+test_expect_success '\"list\" all worktrees from main' '\n+\tgit rev-parse --show-toplevel >expect &&\n+\tgit worktree add --detach here master &&\n+\tgit -C here rev-parse --show-toplevel >>expect &&\n+\tgit worktree list >actual &&\n+\ttest_cmp expect actual &&\n+\trm -rf here &&\n+\tgit worktree prune\n+'\n+\n+test_expect_success '\"list\" all worktrees from linked' '\n+\tgit rev-parse --show-toplevel >expect &&\n+\tgit worktree add --detach here master &&\n+\tgit -C here rev-parse --show-toplevel >>expect &&\n+\tgit -C here worktree list >actual &&\n+\ttest_cmp expect actual &&\n+\trm -rf here &&\n+\tgit worktree prune\n+'\n+\n+test_expect_success '\"list\" all worktrees --verbose from main' '\n+\techo \"$(git rev-parse --show-toplevel)       $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]\" >expect &&\n+\tgit worktree add --detach here master &&\n+   echo \"$(git -C here rev-parse --show-toplevel)  $(git rev-parse --short HEAD) (detached HEAD)\" >>expect &&\n+\tgit worktree list --verbose >actual &&\n+\ttest_cmp expect actual &&\n+\trm -rf here &&\n+\tgit worktree prune\n+'\n+\n+test_expect_success '\"list\" all worktrees --verbose from linked' '\n+\techo \"$(git rev-parse --show-toplevel)       $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]\" >expect &&\n+\tgit worktree add --detach here master &&\n+   echo \"$(git -C here rev-parse --show-toplevel)  $(git rev-parse --short HEAD) (detached HEAD)\" >>expect &&\n+\tgit -C here worktree list --verbose >actual &&\n+\ttest_cmp expect actual &&\n+\trm -rf here &&\n+\tgit worktree prune\n+'\n+\n+test_expect_success 'bare repo setup' '\n+\tgit init --bare bare1 &&\n+\techo \"data\" > file1 &&\n+\tgit add file1 &&\n+\tgit commit -m\"File1: add data\" &&\n+\tgit push bare1 master &&\n+\tgit reset --hard HEAD^\n+'\n+\n+test_expect_success '\"list\" all worktrees from bare main' '\n+\tgit -C bare1 worktree add --detach ../there master &&\n+\techo \"$(cd bare1 && pwd)\" >expect &&\n+\techo \"$(git -C there rev-parse --show-toplevel)\" >>expect &&\n+\tgit -C bare1 worktree list >actual &&\n+\ttest_cmp expect actual &&\n+\trm -rf there &&\n+\tgit -C bare1 worktree prune\n+'\n+\n+test_expect_success '\"list\" all worktrees from bare main --skip-bare' '\n+\tgit -C bare1 worktree add --detach ../there master &&\n+\techo \"$(git -C there rev-parse --show-toplevel)\" >expect &&\n+\tgit -C bare1 worktree list --skip-bare >actual &&\n+\ttest_cmp expect actual &&\n+\trm -rf there &&\n+\tgit -C bare1 worktree prune\n+'\n+\n+test_expect_success '\"list\" all worktrees from worktree with a bare main' '\n+\tgit -C bare1 worktree add --detach ../there master &&\n+\techo \"$(cd bare1 && pwd)\" >expect &&\n+\techo \"$(git -C there rev-parse --show-toplevel)\" >>expect &&\n+\tgit -C there worktree list >actual &&\n+\ttest_cmp expect actual &&\n+\trm -rf there &&\n+\tgit -C bare1 worktree prune\n+'\n+\n+test_expect_success '\"list\" all worktrees from worktree with a bare main --skip-bare' '\n+\tgit -C bare1 worktree add --detach ../there master &&\n+\techo \"$(git -C there rev-parse --show-toplevel)\" >expect &&\n+\tgit -C there worktree list --skip-bare >actual &&\n+\ttest_cmp expect actual &&\n+\trm -rf there &&\n+\tgit -C bare1 worktree prune\n+'\n+\n+test_expect_success '\"list\" all worktrees --verbose from bare main' '\n+\tgit -C bare1 worktree add --detach ../there master &&\n+   echo \"$(cd bare1 && pwd)  (bare)\" >expect &&\n+   echo \"$(git -C there rev-parse --show-toplevel)  $(git -C there rev-parse --short HEAD) (detached HEAD)\" >>expect &&\n+\tgit -C bare1 worktree list --verbose >actual &&\n+\ttest_cmp expect actual &&\n+\trm -rf there &&\n+\tgit -C bare1 worktree prune\n+'\n+\n+test_expect_success '\"list\" all worktrees --verbose from worktree with a bare main' '\n+\tgit -C bare1 worktree add --detach ../there master &&\n+   echo \"$(cd bare1 && pwd)  (bare)\" >expect &&\n+   echo \"$(git -C there rev-parse --show-toplevel)  $(git -C there rev-parse --short HEAD) (detached HEAD)\" >>expect &&\n+\tgit -C there worktree list --verbose >actual &&\n+\ttest_cmp expect actual &&\n+\trm -rf there &&\n+\tgit -C bare1 worktree prune\n+'\n+\n+test_expect_success 'bare repo cleanup' '\n+\trm -rf bare1\n+'\n+\n+test_done\n-- \n2.5.0\n"},{"id":"269731","messageId":"xmqqk2ry2g8e.fsf@gitster.mtv.corp.google.com","threadId":"40276","inReplyTo":"1441402769-35897-2-git-send-email-rappazzo@gmail.com","subject":"Re: [PATCH v7 1/3] worktree: add top-level worktree.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-10T20:04:33Z","receivedAt":"2015-09-10T20:04:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Rappazzo <rappazzo@gmail.com> writes:\n\n> Including functions to get the list of all worktrees, and to get\n> a specific worktree (primary or linked).\n\nWas this meant as a continuation of the sentence started on the\nSubject line, or is s/Including/Include/ necessary?\n\n> diff --git a/worktree.c b/worktree.c\n> new file mode 100644\n> index 0000000..33d2e57\n> --- /dev/null\n> +++ b/worktree.c\n> @@ -0,0 +1,157 @@\n> +#include \"worktree.h\"\n> +#include \"cache.h\"\n> +#include \"git-compat-util.h\"\n\nThe first header to be included must be \"git-compat-util.h\" or\n\"cache.h\", the latter of which is so well known to include the\nformer as the first thing (so inclusion of \"git-compat-util.h\"\nbecomes unnecessary).\n\nAfter that, include your own header as necessary.\n\n> +#include \"refs.h\"\n> +#include \"strbuf.h\"\n> +\n> +void worktree_release(struct worktree *worktree)\n> +{\n> +\tif (worktree) {\n> +\t\tfree(worktree->path);\n> +\t\tfree(worktree->git_dir);\n> +\t\tif (!worktree->is_detached) {\n> +\t\t\t/* could be headless */\n> +\t\t\tfree(worktree->head_ref);\n> +\t\t}\n\nUnnecessary brace {} pair?  It is OK to keep them if later patches\nin the series will make this a multi-statement block, though.\n\n> +\t\tfree(worktree);\n> +\t}\n> +}\n> +\n> +void worktree_list_release(struct worktree_list *worktree_list)\n> +{\n> +\twhile (worktree_list) {\n> +\t\tstruct worktree_list *next = worktree_list->next;\n> +\t\tworktree_release(worktree_list->worktree);\n> +\t\tfree (worktree_list);\n\nNo SP between function name and the parenthesis that introduces its\narguments.\n\n> +\t\tworktree_list = next;\n> +\t}\n> +}\n> +\n> +/*\n> + * read 'path_to_ref' into 'ref'.  Also set is_detached to 1 if the ref is detatched\n> + *\n> + * return 1 if the ref is not a proper ref, 0 otherwise (success)\n\nThese lines are overly long.\n\n> + */\n> +int _parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)\n> +{\n> +\tif (!strbuf_readlink(ref, path_to_ref, 0)) {\n> +\t\tif (!starts_with(ref->buf, \"refs/\") || check_refname_format(ref->buf, 0)) {\n\nAn overly long line.  Perhaps \n\n\t\tif (!starts_with(ref->buf, \"refs/\") ||\n\t\t    check_refname_format(ref->buf, 0)) {\n\n(I see many more \"overly long line\" after this point, which I won't mention).\n\n> +\t\t\t/* invalid ref - something is awry with this repo */\n> +\t\t\treturn 1;\n> +\t\t}\n> +\t} else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {\n> +\t\tif (starts_with(ref->buf, \"ref:\")) {\n> +\t\t\tstrbuf_remove(ref, 0, strlen(\"ref:\"));\n> +\t\t\tstrbuf_trim(ref);\n\nWe don't do the same \"starts_with() and format is sane\" check?\n\n> +\t\t} else if (is_detached) {\n> +\t\t\t*is_detached = 1;\n> +\t\t}\n> +\t}\n\n> +\treturn 0;\n> +}\n> +\n\n> +struct worktree_list *get_worktree_list()\n\nstruct worktree_list *get_worktree_list(void)\n\n> +{\n> +\tstruct worktree_list *list = NULL;\n> +\tstruct worktree_list *current_entry = NULL;\n> +\tstruct worktree *current_worktree = NULL;\n> +\tstruct strbuf path = STRBUF_INIT;\n> +\tDIR *dir;\n> +\tstruct dirent *d;\n> +\n> +\tcurrent_worktree = get_worktree(NULL);\n> +\tif (current_worktree) {\n> +\t\tlist = malloc(sizeof(struct worktree_list));\n\nAlways use x*alloc() family of functions.\n\n> +\t\tlist->worktree = current_worktree;\n> +\t\tlist->next = NULL;\n> +\t\tcurrent_entry = list;\n> +\t}\n\nIf the above if() is not taken, current_entry is left as NULL\n\n> +\tstrbuf_addf(&path, \"%s/worktrees\", get_git_common_dir());\n> +\tdir = opendir(path.buf);\n> +\tstrbuf_release(&path);\n> +\tif (dir) {\n> +\t\twhile ((d = readdir(dir)) != NULL) {\n> +\t\t\tif (!strcmp(d->d_name, \".\") || !strcmp(d->d_name, \"..\"))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tcurrent_worktree = get_worktree(d->d_name);\n> +\t\t\tif (current_worktree) {\n> +\t\t\t\tcurrent_entry->next = malloc(sizeof(struct worktree_list));\n\nBut this line assumes that current_entry is never NULL.\n\n> +\t\t\t\tcurrent_entry = current_entry->next;\n> +\t\t\t\tcurrent_entry->worktree = current_worktree;\n> +\t\t\t\tcurrent_entry->next = NULL;\n> +\t\t\t}\n> +\t\t}\n> +\t\tclosedir(dir);\n> +\t}\n\nAn idiomatic way to extend a singly-linked list at the end in our\ncodebase is to use a pointer to the pointer that has the element at\nthe end, instead of to use a pointer that points at the element at\nthe end or NULL (i.e. the pointer the above code calls current_entry\nis \"struct worktree_list **end_of_list\").  Would it make the above\nsimpler if you followed that pattern?\n\n> +\n> +done:\n> +\n> +\treturn list;\n> +}\n> +\n> diff --git a/worktree.h b/worktree.h\n> new file mode 100644\n> index 0000000..2bc0ab8\n> --- /dev/null\n> +++ b/worktree.h\n> @@ -0,0 +1,48 @@\n> +#ifndef WORKTREE_H\n> +#define WORKTREE_H\n> +\n> +struct worktree {\n> +\tchar *path;\n> +\tchar *git_dir;\n> +\tchar *head_ref;\n> +\tunsigned char head_sha1[20];\n> +\tint is_detached;\n> +\tint is_bare;\n> +};\n> +\n> +struct worktree_list {\n> +\tstruct worktree *worktree;\n> +\tstruct worktree_list *next;\n> +};\n> +\n> +/* Functions for acting on the information about worktrees. */\n> +\n> +/*\n> + * Get the list of all worktrees.  The primary worktree will always be\n> + * the first in the list followed by any other (linked) worktrees created\n> + * by `git worktree add`.  No specific ordering is done on the linked\n> + * worktrees.\n> + *\n> + * The caller is responsible for freeing the memory from the returned list.\n> + * (See worktree_list_release for this purpose).\n> + */\n> +extern struct worktree_list *get_worktree_list();\n\nextern struct worktree_list *get_worktree_list(void);\n\n> +\n> +/*\n> + * generic method to get a worktree\n> + *   - if 'id' is NULL, get the from $GIT_COMMON_DIR\n> + *   - if 'id' is not NULL, get the worktree found in $GIT_COMMON_DIR/worktrees/id if\n> + *     such a worktree exists\n> + *\n> + * The caller is responsible for freeing the memory from the returned\n> + * worktree.  (See worktree_release for this purpose)\n> + */\n> +struct worktree *get_worktree(const char *id);\n> +\n> +/*\n> + * Free up the memory for a worktree_list/worktree\n> + */\n> +extern void worktree_list_release(struct worktree_list *);\n> +extern void worktree_release(struct worktree *);\n> +\n> +#endif\n"},{"id":"269757","messageId":"CANoM8SUA+vk8bbqUaX1H4XQ4LiAVmVwgQdWLE4rO+MMGkS0F7Q@mail.gmail.com","threadId":"40276","inReplyTo":"xmqqk2ry2g8e.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v7 1/3] worktree: add top-level worktree.c","fromName":"Mike Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2015-09-11T10:33:12Z","receivedAt":"2015-09-11T10:33:12Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"On Thu, Sep 10, 2015 at 4:04 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Michael Rappazzo <rappazzo@gmail.com> writes:\n>\n>> Including functions to get the list of all worktrees, and to get\n>> a specific worktree (primary or linked).\n>\n> Was this meant as a continuation of the sentence started on the\n> Subject line, or is s/Including/Include/ necessary?\n\nI think I was continuing the subject line.  I will make it nicer.\n\n>\n>> +             worktree_list = next;\n>> +     }\n>> +}\n>> +\n>> +/*\n>> + * read 'path_to_ref' into 'ref'.  Also set is_detached to 1 if the ref is detatched\n>> + *\n>> + * return 1 if the ref is not a proper ref, 0 otherwise (success)\n>\n> These lines are overly long.\n>\n>> + */\n>> +int _parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)\n>> +{\n>> +     if (!strbuf_readlink(ref, path_to_ref, 0)) {\n>> +             if (!starts_with(ref->buf, \"refs/\") || check_refname_format(ref->buf, 0)) {\n>\n> An overly long line.  Perhaps\n>\n>                 if (!starts_with(ref->buf, \"refs/\") ||\n>                     check_refname_format(ref->buf, 0)) {\n>\n> (I see many more \"overly long line\" after this point, which I won't mention).\n\n\nIs the limit 80 characters?\n\n>\n>> +                     /* invalid ref - something is awry with this repo */\n>> +                     return 1;\n>> +             }\n>> +     } else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {\n>> +             if (starts_with(ref->buf, \"ref:\")) {\n>> +                     strbuf_remove(ref, 0, strlen(\"ref:\"));\n>> +                     strbuf_trim(ref);\n>\n> We don't do the same \"starts_with() and format is sane\" check?\n\nThe code from this snippet was mostly ripped from branch.c (see the\nsecond commit).\nI will add the sanity check.\n\n>\n>\n> An idiomatic way to extend a singly-linked list at the end in our\n> codebase is to use a pointer to the pointer that has the element at\n> the end, instead of to use a pointer that points at the element at\n> the end or NULL (i.e. the pointer the above code calls current_entry\n> is \"struct worktree_list **end_of_list\").  Would it make the above\n> simpler if you followed that pattern?\n>\n\nI think I follow what you are saying here.  I will explore this route.\nI am also unhappy about\nhaving to separately maintain a point to the head of the list when\nusing it.  Would it be\n\"normal\" to add a head pointer to the list structure?\n"},{"id":"269777","messageId":"xmqqk2rx0w54.fsf@gitster.mtv.corp.google.com","threadId":"40276","inReplyTo":"1441402769-35897-3-git-send-email-rappazzo@gmail.com","subject":"Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-11T16:16:07Z","receivedAt":"2015-09-11T16:16:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Rappazzo <rappazzo@gmail.com> writes:\n\n> The code formerly in branch.c was largely the basis of the\n> get_worktree_list implementation is now moved to worktree.c,\n> and the find_shared_symref implementation has been refactored\n> to use get_worktree_list\n>\n> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>\n> ---\n\n\n\n\n>  branch.c        | 79 +--------------------------------------------------------\n>  branch.h        |  8 ------\n>  builtin/notes.c |  1 +\n>  worktree.c      | 40 +++++++++++++++++++++++++++++\n>  worktree.h      |  7 +++++\n>  5 files changed, 49 insertions(+), 86 deletions(-)\n>\n> diff --git a/branch.c b/branch.c\n> index d013374..77d7f2a 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -4,6 +4,7 @@\n>  #include \"refs.h\"\n>  #include \"remote.h\"\n>  #include \"commit.h\"\n> +#include \"worktree.h\"\n>  \n>  struct tracking {\n>  \tstruct refspec spec;\n> @@ -311,84 +312,6 @@ void remove_branch_state(void)\n>  \tunlink(git_path_squash_msg());\n>  }\n>  \n> -static char *find_linked_symref(const char *symref, const char *branch,\n> -\t\t\t\tconst char *id)\n\nThe title refers to a function with a different name ;-).\n\nCopying the bulk of the function in 1/3 and then removing the\noriginal here made it somewhat hard to compare what got changed in\nthe implementation.\n\nI _think_ the code structure in the end result is more or less\nright, though.\n\n> diff --git a/worktree.c b/worktree.c\n> index 33d2e57..e45b651 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -155,3 +155,43 @@ done:\n>  \treturn list;\n>  }\n>  \n> +char *find_shared_symref(const char *symref, const char *target)\n> +{\n> +\tchar *existing = NULL;\n> +\tstruct strbuf path = STRBUF_INIT;\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tstruct worktree_list *worktree_list = get_worktree_list();\n> +\tstruct worktree_list *orig_list = worktree_list;\n> +\tstruct worktree *matched = NULL;\n> +\n> +\twhile (!matched && worktree_list) {\n> +\t\tif (strcmp(\"HEAD\", symref)) {\n> +\t\t\tstrbuf_reset(&path);\n> +\t\t\tstrbuf_reset(&sb);\n> +\t\t\tstrbuf_addf(&path, \"%s/%s\", worktree_list->worktree->git_dir, symref);\n> +\n> +\t\t\tif (_parse_ref(path.buf, &sb, NULL)) {\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n\nI forgot to say this on 1/3, but this helper function _parse_ref()\nis obviously an implementation detail; does it have to be visible\noutside the file, or can it become file-scope static?  There may\nbe other helpers that may not want to be global (I didn't check).\n\nProvided that 1/3 will become accepable, this step looks sensible to\nme.\n\nThanks.\n"},{"id":"269799","messageId":"CANoM8SUGmb=fSFqF4DTuW2F5oPVaim-=SP76rqwwqtzcsNQf=g@mail.gmail.com","threadId":"40276","inReplyTo":"xmqqk2rx0w54.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c","fromName":"Mike Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2015-09-11T21:43:25Z","receivedAt":"2015-09-11T21:43:25Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"On Fri, Sep 11, 2015 at 12:16 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Michael Rappazzo <rappazzo@gmail.com> writes:\n>\n>> The code formerly in branch.c was largely the basis of the\n>> get_worktree_list implementation is now moved to worktree.c,\n>> and the find_shared_symref implementation has been refactored\n>> to use get_worktree_list\n>>\n>\n> Copying the bulk of the function in 1/3 and then removing the\n> original here made it somewhat hard to compare what got changed in\n> the implementation.\n>\n> I _think_ the code structure in the end result is more or less\n> right, though.\n\nShould I squash these first two commits together in the next series?\n"},{"id":"269800","messageId":"xmqqbnd8zks9.fsf@gitster.mtv.corp.google.com","threadId":"40276","inReplyTo":"CANoM8SUGmb=fSFqF4DTuW2F5oPVaim-=SP76rqwwqtzcsNQf=g@mail.gmail.com","subject":"Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-11T21:52:06Z","receivedAt":"2015-09-11T21:52:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Rappazzo <rappazzo@gmail.com> writes:\n\n> On Fri, Sep 11, 2015 at 12:16 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Michael Rappazzo <rappazzo@gmail.com> writes:\n>>\n>>> The code formerly in branch.c was largely the basis of the\n>>> get_worktree_list implementation is now moved to worktree.c,\n>>> and the find_shared_symref implementation has been refactored\n>>> to use get_worktree_list\n>>>\n>>\n>> Copying the bulk of the function in 1/3 and then removing the\n>> original here made it somewhat hard to compare what got changed in\n>> the implementation.\n>>\n>> I _think_ the code structure in the end result is more or less\n>> right, though.\n>\n> Should I squash these first two commits together in the next series?\n\nMashing the two into one patch would not help anybody, I would\nsuspect.\n\nI didn't try this myself, but if I were doing this and if I were\naiming for perfection, then I would probably try to split it even\nfiner.  Refactor the original while both the callers and the helpers\nare still inside branch.c (hence the early steps in the series will\nnot benefit worktree.c at all---worktree.c may not even exist in\nthem yet), move refactored helpers to worktree.[ch] (and at this\nstep you may not even have get_worktree() etc. yet), and then use\nthat refactored helper while writing new functions in worktree.c.\n"},{"id":"269801","messageId":"xmqq7fnwzkbe.fsf@gitster.mtv.corp.google.com","threadId":"40276","inReplyTo":"1441402769-35897-4-git-send-email-rappazzo@gmail.com","subject":"Re: [PATCH v7 3/3] worktree: add 'list' command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-11T22:02:13Z","receivedAt":"2015-09-11T22:02:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Rappazzo <rappazzo@gmail.com> writes:\n\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 71bb770..a0c0fe8 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -8,10 +8,13 @@\n>  #include \"run-command.h\"\n>  #include \"sigchain.h\"\n>  #include \"refs.h\"\n> +#include \"utf8.h\"\n> +#include \"worktree.h\"\n>  \n>  static const char * const worktree_usage[] = {\n>  \tN_(\"git worktree add [<options>] <path> <branch>\"),\n>  \tN_(\"git worktree prune [<options>]\"),\n> +\tN_(\"git worktree list [<options>]\"),\n>  \tNULL\n>  };\n>  \n> @@ -359,6 +362,64 @@ static int add(int ac, const char **av, const char *prefix)\n>  \treturn add_worktree(path, branch, &opts);\n>  }\n>  \n> +static int list(int ac, const char **av, const char *prefix)\n> +{\n> +\tint no_bare = 0;\n> +\n> +\tstruct option options[] = {\n> +\t\tOPT_BOOL(0, \"skip-bare\", &no_bare,  N_(\"exclude bare worktrees from the list\")),\n> +\t\tOPT__VERBOSE(&verbose, N_(\"include more worktree details\")),\n> +\t\tOPT_END()\n> +\t};\n> +\n> +\tac = parse_options(ac, av, prefix, options, worktree_usage, 0);\n> +\tif (ac)\n> +\t\tusage_with_options(worktree_usage, options);\n> +\telse {\n> +\t\tstruct worktree_list *worktree_list = get_worktree_list();\n\nCall that variable just \"list\"; you are not dealing with any other\nkind of list in this code anyway.\n\n> +\t\tstruct worktree_list *orig = worktree_list;\n> +\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\tint path_maxlen = 0;\n> +\n> +\t\tif (verbose) {\n> +\t\t\twhile (worktree_list) {\n> +\t\t\t\tint cur_len = strlen(worktree_list->worktree->path);\n> +\t\t\t\tif (cur_len > path_maxlen)\n> +\t\t\t\t\tpath_maxlen = cur_len;\n> +\t\t\t\tworktree_list = worktree_list->next ? worktree_list->next : NULL;\n\nEhh, (A ? A : NULL), when A is a pointer value, is equivalent to A, no?\n\n\t\tstruct worktree_list *orig = get_worktree_list();\n\t\tstruct worktree_list *list;\n\n\t\tfor (list = orig; list; list = list->next) {\n\t\t\tstruct worktree *worktree = list->worktree;\n\t\t\tint len = strlen(worktree->path);\n                        if (maxlen < len)\n                        \tmaxlen = len;\n\t\t}\n\nIt seems that \"do we really need callback interface?\" suggestion\nreally made the code simple and straight-forward.\n\n> +\t\twhile (worktree_list) {\n> +\t\t\t/* if this work tree is not bare OR if bare repos are to be shown (default) */\n> +\t\t\tif (!worktree_list->worktree->is_bare || !no_bare) {\n> +\t\t\t\tstrbuf_reset(&sb);\n> +\t\t\t\tif (!verbose)\n> +\t\t\t\t\tstrbuf_addstr(&sb, worktree_list->worktree->path);\n> +\t\t\t\telse {\n> +\t\t\t\t\tint cur_len = strlen(worktree_list->worktree->path);\n> +\t\t\t\t\tint utf8_adj = cur_len - utf8_strwidth(worktree_list->worktree->path);\n> +\t\t\t\t\tstrbuf_addf(&sb, \"%-*s \", 1 + path_maxlen + utf8_adj, worktree_list->worktree->path);\n> +\t\t\t\t\tif (worktree_list->worktree->is_bare)\n> +\t\t\t\t\t\tstrbuf_addstr(&sb, \"(bare)\");\n> +\t\t\t\t\telse {\n> +\t\t\t\t\t\tstrbuf_addf(&sb, \"%s \", find_unique_abbrev(worktree_list->worktree->head_sha1, DEFAULT_ABBREV));\n> +\t\t\t\t\t\tif (!worktree_list->worktree->is_detached)\n> +\t\t\t\t\t\t\tstrbuf_addf(&sb, \"[%s]\", shorten_unambiguous_ref(worktree_list->worktree->head_ref, 0));\n> +\t\t\t\t\t\telse\n> +\t\t\t\t\t\t\tstrbuf_addstr(&sb, \"(detached HEAD)\");\n> +\t\t\t\t\t}\n> +\t\t\t\t}\n> +\t\t\t\tprintf(\"%s\\n\", sb.buf);\n> +\t\t\t}\n> +\t\t\tworktree_list = worktree_list->next ? worktree_list->next : NULL;\n\nWould it help reduce the indentation level if you inroduced a helper\nfunction that takes a single worktree object and does all the above?\n\n\tfor (list = orig; list; list = list->next) {\n\t\tstruct worktree *worktree = list->worktree;\n\t\tif (!no_bare || !worktree->is_bare)\n\t        \tshow_worktree(worktree);\n\t}\n\nor something?\n"},{"id":"269807","messageId":"CAPig+cQ4BUbS0ZQ=NoDO9JZmWYRzX0vhoi-W9ahg3yaWM+pKQw@mail.gmail.com","threadId":"40276","inReplyTo":"xmqqbnd8zks9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-11T23:10:48Z","receivedAt":"2015-09-11T23:10:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Sep 11, 2015 at 5:52 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Mike Rappazzo <rappazzo@gmail.com> writes:\n>> On Fri, Sep 11, 2015 at 12:16 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Michael Rappazzo <rappazzo@gmail.com> writes:\n>>>> The code formerly in branch.c was largely the basis of the\n>>>> get_worktree_list implementation is now moved to worktree.c,\n>>>> and the find_shared_symref implementation has been refactored\n>>>> to use get_worktree_list\n>>>\n>>> Copying the bulk of the function in 1/3 and then removing the\n>>> original here made it somewhat hard to compare what got changed in\n>>> the implementation.\n>>>\n>>> I _think_ the code structure in the end result is more or less\n>>> right, though.\n>>\n>> Should I squash these first two commits together in the next series?\n>\n> Mashing the two into one patch would not help anybody, I would\n> suspect.\n>\n> I didn't try this myself, but if I were doing this and if I were\n> aiming for perfection, then I would probably try to split it even\n> finer.  Refactor the original while both the callers and the helpers\n> are still inside branch.c (hence the early steps in the series will\n> not benefit worktree.c at all---worktree.c may not even exist in\n> them yet), move refactored helpers to worktree.[ch] (and at this\n> step you may not even have get_worktree() etc. yet), and then use\n> that refactored helper while writing new functions in worktree.c.\n\nThanks for bringing this up. I haven't found a sufficient block of\ntime yet to review these patches, however, I had the same thought upon\na quick initial read, and is how I was hoping to see the patch series\nconstructed based upon my earlier two reviews suggesting refactoring\nthe existing branch.c functions into a new get_worktrees(). There are\nat least a couple important reasons for taking this approach.\n\nFirst, it keeps the \"blame\" trail intact, the full context of which\ncan be helpful to someone trying to understand why the code is the way\nit is. The current approach of having get_worktree_list() materialize\nout of thin air (even though it may have been patterned after existing\ncode) doesn't give the same benefit.\n\nSecond, it's easier for reviewers to understand and check the code for\ncorrectness if it mutates to the final form in small increments than\nit is to have get_worktrees() come into existence fully matured.\n\nA final minor comment: Since all three branch.c functions,\ndie_if_checked_out(), find_shared_symref(), and find_linked_symref(),\nought to be moved to top-level worktree.c, I'd probably have patch 1\ndo the relocation (with no functional changes), and subsequent patches\nrefactor the moved code into a general purpose get_worktrees(). The\nfinal patch would then implement \"git worktrees list\".\n"},{"id":"269812","messageId":"CANoM8SUhxLAUUAuH1PNRX-Fg09nNWzk2fX1TrggD5XDThaYc4w@mail.gmail.com","threadId":"40276","inReplyTo":"CAPig+cQ4BUbS0ZQ=NoDO9JZmWYRzX0vhoi-W9ahg3yaWM+pKQw@mail.gmail.com","subject":"Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c","fromName":"Mike Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2015-09-12T02:33:34Z","receivedAt":"2015-09-12T02:33:34Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"On Fri, Sep 11, 2015 at 7:10 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Fri, Sep 11, 2015 at 5:52 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Thanks for bringing this up. I haven't found a sufficient block of\n> time yet to review these patches, however, I had the same thought upon\n> a quick initial read, and is how I was hoping to see the patch series\n> constructed based upon my earlier two reviews suggesting refactoring\n> the existing branch.c functions into a new get_worktrees(). There are\n> at least a couple important reasons for taking this approach.\n>\n> First, it keeps the \"blame\" trail intact, the full context of which\n> can be helpful to someone trying to understand why the code is the way\n> it is. The current approach of having get_worktree_list() materialize\n> out of thin air (even though it may have been patterned after existing\n> code) doesn't give the same benefit.\n>\n> Second, it's easier for reviewers to understand and check the code for\n> correctness if it mutates to the final form in small increments than\n> it is to have get_worktrees() come into existence fully matured.\n>\n> A final minor comment: Since all three branch.c functions,\n> die_if_checked_out(), find_shared_symref(), and find_linked_symref(),\n> ought to be moved to top-level worktree.c, I'd probably have patch 1\n> do the relocation (with no functional changes), and subsequent patches\n> refactor the moved code into a general purpose get_worktrees(). The\n> final patch would then implement \"git worktrees list\".\n\nI like the way this history works out, and I have reworked the history\nto follow this idea.  The only thing\nI didn't do was move the die_if_checked_out() function from branch.c,\nas I feel that this function is more\nof a branch feature than a worktree feature.\n"},{"id":"269854","messageId":"CAPig+cQRx-uKZnnx2O0pG34HkcEdg1GbGNFd9PHdw+m0O3aJEA@mail.gmail.com","threadId":"40276","inReplyTo":"1441402769-35897-2-git-send-email-rappazzo@gmail.com","subject":"Re: [PATCH v7 1/3] worktree: add top-level worktree.c","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-13T02:39:26Z","receivedAt":"2015-09-13T02:39:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:\n> Including functions to get the list of all worktrees, and to get\n> a specific worktree (primary or linked).\n\nSome comments below in addition to those by Junio...\n\n> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>\n> ---\n> diff --git a/worktree.c b/worktree.c\n> new file mode 100644\n> index 0000000..33d2e57\n> --- /dev/null\n> +++ b/worktree.c\n> @@ -0,0 +1,157 @@\n> +#include \"worktree.h\"\n> +#include \"cache.h\"\n> +#include \"git-compat-util.h\"\n> +#include \"refs.h\"\n> +#include \"strbuf.h\"\n> +\n> +void worktree_release(struct worktree *worktree)\n> +{\n> +       if (worktree) {\n> +               free(worktree->path);\n> +               free(worktree->git_dir);\n> +               if (!worktree->is_detached) {\n> +                       /* could be headless */\n> +                       free(worktree->head_ref);\n\nIt's safe to invoke free() on NULL, so as long as worktree->head_ref\nholds NULL in the detached case, then there's no need for the\nconditional at all (or the comment).\n\n> +               }\n> +               free(worktree);\n> +       }\n> +}\n> +\n> +void worktree_list_release(struct worktree_list *worktree_list)\n> +{\n> +       while (worktree_list) {\n> +               struct worktree_list *next = worktree_list->next;\n> +               worktree_release(worktree_list->worktree);\n> +               free (worktree_list);\n> +               worktree_list = next;\n> +       }\n> +}\n> +\n> +/*\n> + * read 'path_to_ref' into 'ref'.  Also set is_detached to 1 if the ref is detatched\n> + *\n> + * return 1 if the ref is not a proper ref, 0 otherwise (success)\n> + */\n> +int _parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)\n\nMentioned already by Junio in his review of patch 2: Make this\nfunction static and drop the leading underscore from the name.\n\n> +{\n> +       if (!strbuf_readlink(ref, path_to_ref, 0)) {\n> +               if (!starts_with(ref->buf, \"refs/\") || check_refname_format(ref->buf, 0)) {\n> +                       /* invalid ref - something is awry with this repo */\n> +                       return 1;\n\nIn this codebase, it's common to return -1 on error. You could also\nprobably drop the unnecessary braces (here and a few other places).\n\n> +               }\n> +       } else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {\n> +               if (starts_with(ref->buf, \"ref:\")) {\n> +                       strbuf_remove(ref, 0, strlen(\"ref:\"));\n> +                       strbuf_trim(ref);\n> +               } else if (is_detached) {\n> +                       *is_detached = 1;\n\nThe code allows the caller to pass in NULL for is_detached if not\ninterested in this information, however, the comment block documenting\nthe function doesn't mention this.\n\nAlso, don't you need to clear *is_detached when not detached since you\ndon't know what garbage value *is_detached holds upon entry?\n\nI find the placement of the detached detection logic here a bit\nstrange. The only case for which it might trigger is the so-called\n\"main worktree\", yet having it in this general purpose parse function\nseems to imply misleadingly that any worktree could be detached. Also,\ngiven the current world order[1], wouldn't missing \"ref:\" indicate an\nerror for any worktree other than the main one? Consequently, this\ndetection probably ought to be done only for the main worktree\n(outside of this function, probably).\n\n[1]: Though, this might not hold true in a future world order\ninvolving merging of notes, if I correctly understood that discussion,\nsince notes merging wouldn't require a \"worktree\", per se.\n\n> +               }\n> +       }\n> +       return 0;\n> +}\n> +\n> +struct worktree *get_worktree(const char *id)\n> +{\n> +       struct worktree *worktree = NULL;\n> +       struct strbuf path = STRBUF_INIT;\n> +       struct strbuf worktree_path = STRBUF_INIT;\n> +       struct strbuf git_dir = STRBUF_INIT;\n> +       struct strbuf head_ref = STRBUF_INIT;\n> +       int is_bare = 0;\n> +       int is_detached = 0;\n> +\n> +       if (id) {\n> +               strbuf_addf(&git_dir, \"%s/worktrees/%s\", absolute_path(get_git_common_dir()), id);\n> +               strbuf_addf(&path, \"%s/gitdir\", git_dir.buf);\n> +               if (strbuf_read_file(&worktree_path, path.buf, 0) <= 0) {\n> +                       /* invalid git_dir file */\n> +                       goto done;\n> +               }\n> +               strbuf_rtrim(&worktree_path);\n> +               if (!strbuf_strip_suffix(&worktree_path, \"/.git\")) {\n> +                       strbuf_reset(&worktree_path);\n> +                       strbuf_addstr(&worktree_path, absolute_path(\".\"));\n> +                       strbuf_strip_suffix(&worktree_path, \"/.\");\n> +               }\n> +\n> +               strbuf_reset(&path);\n> +               strbuf_addf(&path, \"%s/worktrees/%s/HEAD\", get_git_common_dir(), id);\n> +       } else {\n> +               strbuf_addf(&git_dir, \"%s\", absolute_path(get_git_common_dir()));\n> +               strbuf_addf(&worktree_path, \"%s\", absolute_path(get_git_common_dir()));\n> +               is_bare = !strbuf_strip_suffix(&worktree_path, \"/.git\");\n> +               if (is_bare)\n> +                       strbuf_strip_suffix(&worktree_path, \"/.\");\n> +\n> +               strbuf_addf(&path, \"%s/HEAD\", get_git_common_dir());\n> +       }\n\nI realize that this is modeled closely after existing code in\nbranch.c, but, with the exception of parsing the ref file and\nconstructing a worktree structure, the main worktree case (id == NULL)\nis entirely disjoint from the linked worktree case (id != NULL). This\nsuggests strongly that get_worktree() should be split into two\nfunctions, one for the main worktree and one for linked worktrees,\nwhich would make the code easier to understand. You might call the\nfunctions get_main_worktree() and get_linked_worktree(id) (or perhaps\ndrop \"linked\" from the latter name).\n\n> +       /*\n> +        * $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside $GIT_DIR so for linked worktrees,\n> +        * `resolve_ref_unsafe()` won't work (it uses git_path). Parse the ref ourselves.\n> +        */\n\nThis comment probably belongs inside _parse_ref() as it's explaining\nits internal workings, rather than here at the call-site.\n\n> +       if (_parse_ref(path.buf, &head_ref, &is_detached))\n> +               goto done;\n> +\n> +       worktree = malloc(sizeof(struct worktree));\n> +       worktree->path = strbuf_detach(&worktree_path, NULL);\n> +       worktree->git_dir = strbuf_detach(&git_dir, NULL);\n> +       worktree->is_bare = is_bare;\n> +       worktree->head_ref = NULL;\n> +       worktree->is_detached = is_detached;\n> +       if (strlen(head_ref.buf) > 0) {\n\nMore idiomatic:\n\n    if (head_ref.len) {\n\n> +               if (!is_detached) {\n> +                       resolve_ref_unsafe(head_ref.buf, 0, worktree->head_sha1, NULL);\n> +                       worktree->head_ref = strbuf_detach(&head_ref, NULL);\n> +               } else {\n> +                       get_sha1_hex(head_ref.buf, worktree->head_sha1);\n> +               }\n> +       }\n> +done:\n> +       strbuf_release(&path);\n> +       strbuf_release(&git_dir);\n> +       strbuf_release(&head_ref);\n> +       strbuf_release(&worktree_path);\n> +       return worktree;\n> +}\n> +\n> +struct worktree_list *get_worktree_list()\n\nCan we be more concise and call this get_worktrees()?\n\n> +{\n> +       struct worktree_list *list = NULL;\n> +       struct worktree_list *current_entry = NULL;\n> +       struct worktree *current_worktree = NULL;\n> +       struct strbuf path = STRBUF_INIT;\n> +       DIR *dir;\n> +       struct dirent *d;\n> +\n> +       current_worktree = get_worktree(NULL);\n> +       if (current_worktree) {\n> +               list = malloc(sizeof(struct worktree_list));\n> +               list->worktree = current_worktree;\n> +               list->next = NULL;\n> +               current_entry = list;\n> +       }\n> +       strbuf_addf(&path, \"%s/worktrees\", get_git_common_dir());\n> +       dir = opendir(path.buf);\n> +       strbuf_release(&path);\n> +       if (dir) {\n> +               while ((d = readdir(dir)) != NULL) {\n> +                       if (!strcmp(d->d_name, \".\") || !strcmp(d->d_name, \"..\"))\n> +                               continue;\n> +\n> +                       current_worktree = get_worktree(d->d_name);\n> +                       if (current_worktree) {\n> +                               current_entry->next = malloc(sizeof(struct worktree_list));\n> +                               current_entry = current_entry->next;\n> +                               current_entry->worktree = current_worktree;\n> +                               current_entry->next = NULL;\n> +                       }\n> +               }\n> +               closedir(dir);\n> +       }\n> +\n> +done:\n\nWhat is this label for? It doesn't seem to have any corresponding goto's.\n\n> +\n> +       return list;\n> +}\n> +\n> diff --git a/worktree.h b/worktree.h\n> new file mode 100644\n> index 0000000..2bc0ab8\n> --- /dev/null\n> +++ b/worktree.h\n> @@ -0,0 +1,48 @@\n> +#ifndef WORKTREE_H\n> +#define WORKTREE_H\n> +\n> +struct worktree {\n> +       char *path;\n> +       char *git_dir;\n> +       char *head_ref;\n> +       unsigned char head_sha1[20];\n> +       int is_detached;\n> +       int is_bare;\n> +};\n> +\n> +struct worktree_list {\n> +       struct worktree *worktree;\n> +       struct worktree_list *next;\n> +};\n\nI don't care too strongly, but an alternate approach (which I probably\nwould have taken) would be to have get_worktrees() simply return an\narray of 'struct worktree' objects, hence no need for the additional\n'struct worktree_list'. The slight complication with this approach,\nthough, is that get_worktrees() either also needs to return the length\nof the array, or the array should end with some sort of end-of-array\nsentinel. An obvious sentinel would be path==NULL or git_dir==NULL or\nall of the above.\n\nClient iteration is just about the same with the array approach as\nwith the linked-list approach.\n\n> +/* Functions for acting on the information about worktrees. */\n> +\n> +/*\n> + * Get the list of all worktrees.  The primary worktree will always be\n> + * the first in the list followed by any other (linked) worktrees created\n> + * by `git worktree add`.  No specific ordering is done on the linked\n> + * worktrees.\n> + *\n> + * The caller is responsible for freeing the memory from the returned list.\n> + * (See worktree_list_release for this purpose).\n> + */\n> +extern struct worktree_list *get_worktree_list();\n> +\n> +/*\n> + * generic method to get a worktree\n> + *   - if 'id' is NULL, get the from $GIT_COMMON_DIR\n> + *   - if 'id' is not NULL, get the worktree found in $GIT_COMMON_DIR/worktrees/id if\n> + *     such a worktree exists\n> + *\n> + * The caller is responsible for freeing the memory from the returned\n> + * worktree.  (See worktree_release for this purpose)\n> + */\n> +struct worktree *get_worktree(const char *id);\n> +\n> +/*\n> + * Free up the memory for a worktree_list/worktree\n> + */\n> +extern void worktree_list_release(struct worktree_list *);\n> +extern void worktree_release(struct worktree *);\n> +\n> +#endif\n> --\n> 2.5.0\n"},{"id":"269855","messageId":"CAPig+cT6JLzPPpJKPxAZGGduEQTRzwa57pHtGOJjPYPxCwJV=w@mail.gmail.com","threadId":"40276","inReplyTo":"1441402769-35897-3-git-send-email-rappazzo@gmail.com","subject":"Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-13T03:19:35Z","receivedAt":"2015-09-13T03:19:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:\n> The code formerly in branch.c was largely the basis of the\n> get_worktree_list implementation is now moved to worktree.c,\n> and the find_shared_symref implementation has been refactored\n> to use get_worktree_list\n\nSome comments below in addition to those by Junio...\n\n> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>\n> ---\n> diff --git a/branch.h b/branch.h\n> index d3446ed..58aa45f 100644\n> --- a/branch.h\n> +++ b/branch.h\n> @@ -59,12 +59,4 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name);\n>   */\n>  extern void die_if_checked_out(const char *branch);\n>\n> -/*\n> - * Check if a per-worktree symref points to a ref in the main worktree\n> - * or any linked worktree, and return the path to the exising worktree\n> - * if it is.  Returns NULL if there is no existing ref.  The caller is\n> - * responsible for freeing the returned path.\n> - */\n> -extern char *find_shared_symref(const char *symref, const char *target);\n> -\n>  #endif\n> diff --git a/worktree.c b/worktree.c\n> index 33d2e57..e45b651 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -155,3 +155,43 @@ done:\n>         return list;\n>  }\n>\n> +char *find_shared_symref(const char *symref, const char *target)\n> +{\n> +       char *existing = NULL;\n> +       struct strbuf path = STRBUF_INIT;\n> +       struct strbuf sb = STRBUF_INIT;\n> +       struct worktree_list *worktree_list = get_worktree_list();\n> +       struct worktree_list *orig_list = worktree_list;\n> +       struct worktree *matched = NULL;\n> +\n> +       while (!matched && worktree_list) {\n> +               if (strcmp(\"HEAD\", symref)) {\n\nThe result of strcmp() never changes, so this (expensive) check could\nbe lifted out of the 'while' loop, however...\n\n> +                       strbuf_reset(&path);\n> +                       strbuf_reset(&sb);\n> +                       strbuf_addf(&path, \"%s/%s\", worktree_list->worktree->git_dir, symref);\n> +\n> +                       if (_parse_ref(path.buf, &sb, NULL)) {\n> +                               continue;\n> +                       }\n> +\n> +                       if (!strcmp(sb.buf, target))\n> +                               matched = worktree_list->worktree;\n\nThe original code in branch.c, which this patch removes, did not need\nto make a special case of HEAD, so it's not immediately clear why this\nreplacement code does so. This is the sort of issue which argues in\nfavor of mutating the existing code (slowly) over the course of\nseveral patches into the final form, rather than having the final form\ncome into existence out of thin air. When the changes are made\nincrementally, it is easier for reviewers to understand why such\nmodifications are needed, which (hopefully) should lead to fewer\nquestions such as this one.\n\n> +               } else {\n> +                       if (worktree_list->worktree->head_ref && !strcmp(worktree_list->worktree->head_ref, target))\n> +                               matched = worktree_list->worktree;\n> +               }\n> +               worktree_list = worktree_list->next ? worktree_list->next : NULL;\n> +       }\n> +\n> +       if (matched) {\n> +               existing = malloc(strlen(matched->path) + 1);\n> +               strcpy(existing, matched->path);\n\nA couple issues:\n\nThis can be done more concisely and safely with xstrdup().\n\nIn this codebase, it probably would be more idiomatic to use goto to\ndrop out of the loop rather than setting 'matched' and then having to\ncheck 'matched' in the loop condition. So, for instance, each place\nwhich sets 'matched' could instead say:\n\n    existing = xstrdup(...);\n    goto done;\n\n> +       }\n> +\n> +done:\n\nThis label doesn't have any matching goto's.\n\n> +       strbuf_release(&path);\n> +       strbuf_release(&sb);\n> +       worktree_list_release(orig_list);\n> +\n> +       return existing;\n> +}\n> diff --git a/worktree.h b/worktree.h\n> index 2bc0ab8..320f17e 100644\n> --- a/worktree.h\n> +++ b/worktree.h\n> @@ -45,4 +45,11 @@ struct worktree *get_worktree(const char *id);\n>  extern void worktree_list_release(struct worktree_list *);\n>  extern void worktree_release(struct worktree *);\n>\n> +/*\n> + * Look for a symref in any worktree, and return the path to the first\n> + * worktree found or NULL if not found.  The caller is responsible for\n> + * freeing the returned path.\n> + */\n\nFor some reason, this comment differs a fair bit from the original in\nbranch.h which was removed by this patch, however, the original\ncomment was a bit more explanatory (I think).\n\nAs a general rule, it's better to do code movement and code changes in\nseparate patches since it's hard for reviewers to detect and\ncomprehend differences in code which has been both moved and changed\nat the same time.\n\n> +extern char *find_shared_symref(const char *symref, const char *target);\n> +\n>  #endif\n> --\n> 2.5.0\n"},{"id":"269856","messageId":"CAPig+cSfduAgj7WP2JmqecVbQMRoZu+9O0f9vPSF1ee8h=1LmA@mail.gmail.com","threadId":"40276","inReplyTo":"1441402769-35897-4-git-send-email-rappazzo@gmail.com","subject":"Re: [PATCH v7 3/3] worktree: add 'list' command","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-13T04:25:29Z","receivedAt":"2015-09-13T04:25:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:\n> 'git worktree list' iterates through the worktree list, and outputs\n> the worktree dir.  By default, only the worktree path is output.\n\nComments below in addition to Junio's...\n\n> Supported options include:\n>         --skip-bare: do not output bare worktrees\n>         --verbose: include the current head and ref (if applicable), also\n>                 decorate bare worktrees\n\nI don't care strongly at this point, but an alternate, (possibly) more\nreviewer-friendly, approach would be to split this into several\npatches: the first would add a bare-bones \"list\" command, and each\nsubsequent patch would flesh it out by introducing one option and/or\nenhancing the output in some way.\n\n> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>\n> ---\n> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\n> index fb68156..b9339ed 100644\n> --- a/Documentation/git-worktree.txt\n> +++ b/Documentation/git-worktree.txt\n> @@ -11,6 +11,7 @@ SYNOPSIS\n>  [verse]\n>  'git worktree add' [-f] [--detach] [-b <new-branch>] <path> [<branch>]\n>  'git worktree prune' [-n] [-v] [--expire <expire>]\n> +'git worktree list' [-v] [--skip-bare]\n\nI'm somewhat skeptical of the --skip-bare option. Recalling my v2\nreview[1] skepticism of the --main-only option:\n\n    The more options we have, the more we have to document, test, and\n    support...\n\nThus, I wonder how much value this option has. Presumably, for\nscripters, we will want to have a --porcelain option, the output of\nwhich will contain useful information about a worktree, including\nwhether it's bare, and a script can, on its own, easily filter out a\nbare worktree if desired.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/275528\n\n>  DESCRIPTION\n>  -----------\n> @@ -59,6 +60,10 @@ prune::\n>\n>  Prune working tree information in $GIT_DIR/worktrees.\n>\n> +list::\n> +\n> +List the main worktree followed by each of the linked worktrees.\n> +\n>  OPTIONS\n>  -------\n>\n> @@ -89,10 +94,14 @@ OPTIONS\n>  -v::\n>  --verbose::\n>         With `prune`, report all removals.\n> +       With `list`, show more information about each worktree.  This includes\n> +       if the worktree is bare, the revision currently checked out, and the\n> +       branch currently checked out (or 'detached HEAD' if none).\n\nHmm, this prints both the SHA1 and the branch name (or literal string\n\"detached HEAD\")? Is that a good use of screen real-estate? In\nparticular, I'm wondering how useful the SHA1 is to the user when not\ndetached. I would expect (perhaps wrongly) that it would be sufficient\nto output either the branch name *or* the SHA1 (which implies\n\"detached\" without having to say so literally).\n\n>  --expire <time>::\n>         With `prune`, only expire unused working trees older than <time>.\n\nDocumentation for --skip-bare (and --no-skip-bare) seems to be missing.\n\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 71bb770..a0c0fe8 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -359,6 +362,64 @@ static int add(int ac, const char **av, const char *prefix)\n> +static int list(int ac, const char **av, const char *prefix)\n> +{\n> +       int no_bare = 0;\n> +\n> +       struct option options[] = {\n> +               OPT_BOOL(0, \"skip-bare\", &no_bare,  N_(\"exclude bare worktrees from the list\")),\n> +               OPT__VERBOSE(&verbose, N_(\"include more worktree details\")),\n> +               OPT_END()\n> +       };\n> +\n> +       ac = parse_options(ac, av, prefix, options, worktree_usage, 0);\n> +       if (ac)\n> +               usage_with_options(worktree_usage, options);\n> +       else {\n> +               struct worktree_list *worktree_list = get_worktree_list();\n> +               struct worktree_list *orig = worktree_list;\n> +               struct strbuf sb = STRBUF_INIT;\n> +               int path_maxlen = 0;\n> +\n> +               if (verbose) {\n> +                       while (worktree_list) {\n> +                               int cur_len = strlen(worktree_list->worktree->path);\n> +                               if (cur_len > path_maxlen)\n> +                                       path_maxlen = cur_len;\n> +                               worktree_list = worktree_list->next ? worktree_list->next : NULL;\n> +                       }\n> +                       worktree_list = orig;\n> +               }\n> +               while (worktree_list) {\n> +                       /* if this work tree is not bare OR if bare repos are to be shown (default) */\n> +                       if (!worktree_list->worktree->is_bare || !no_bare) {\n\nDouble negatives (!no_bare) are hard to grok. 'bare_only' or\n'skip_bare' might be better, and then the comment would likely be\nsuperfluous. Also, having the \"default\" behavior mentioned in the\ncomment is an invitation for it to become outdated.\n\n> +                               strbuf_reset(&sb);\n> +                               if (!verbose)\n> +                                       strbuf_addstr(&sb, worktree_list->worktree->path);\n> +                               else {\n> +                                       int cur_len = strlen(worktree_list->worktree->path);\n> +                                       int utf8_adj = cur_len - utf8_strwidth(worktree_list->worktree->path);\n> +                                       strbuf_addf(&sb, \"%-*s \", 1 + path_maxlen + utf8_adj, worktree_list->worktree->path);\n\nI'm not personally convinced that this sort of alignment is desirable,\nhowever, the new strbuf_utf8_align()[2] might be useful here.\n\n[2]: Once it graduates to 'master', that is; it's currently in 'pu' at\n5df5352 (utf8: add function to align a string into given strbuf,\n2015-09-10).\n\n> +                                       if (worktree_list->worktree->is_bare)\n> +                                               strbuf_addstr(&sb, \"(bare)\");\n> +                                       else {\n> +                                               strbuf_addf(&sb, \"%s \", find_unique_abbrev(worktree_list->worktree->head_sha1, DEFAULT_ABBREV));\n> +                                               if (!worktree_list->worktree->is_detached)\n> +                                                       strbuf_addf(&sb, \"[%s]\", shorten_unambiguous_ref(worktree_list->worktree->head_ref, 0));\n> +                                               else\n> +                                                       strbuf_addstr(&sb, \"(detached HEAD)\");\n> +                                       }\n> +                               }\n> +                               printf(\"%s\\n\", sb.buf);\n> +                       }\n> +                       worktree_list = worktree_list->next ? worktree_list->next : NULL;\n> +               }\n> +               worktree_list_release(orig);\n> +               strbuf_release(&sb);\n> +       }\n> +       return 0;\n> +}\n> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh\n> new file mode 100755\n> index 0000000..246890c\n> --- /dev/null\n> +++ b/t/t2027-worktree-list.sh\n> @@ -0,0 +1,122 @@\n> +#!/bin/sh\n> +\n> +test_description='test git worktree list'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup' '\n> +       test_commit init\n> +'\n> +\n> +test_expect_success '\"list\" all worktrees from main' '\n> +       git rev-parse --show-toplevel >expect &&\n> +       git worktree add --detach here master &&\n> +       git -C here rev-parse --show-toplevel >>expect &&\n> +       git worktree list >actual &&\n> +       test_cmp expect actual &&\n> +       rm -rf here &&\n> +       git worktree prune\n\nIf the test fails somewhere above the 'rm', then the cleanup won't\ntake place, which will result in cascading failures of subsequent\ntests. Instead, use test_when_finished() before the \"worktree add\"\nline to ensure cleanup whether successful or not:\n\n    test_when_finished \"rm -rf here && git worktree prune\" &&\n    git worktree add ... &&\n\nSame comment applies to remaining tests.\n\n> +'\n> +\n> +test_expect_success '\"list\" all worktrees from linked' '\n> +       git rev-parse --show-toplevel >expect &&\n> +       git worktree add --detach here master &&\n> +       git -C here rev-parse --show-toplevel >>expect &&\n> +       git -C here worktree list >actual &&\n> +       test_cmp expect actual &&\n> +       rm -rf here &&\n> +       git worktree prune\n> +'\n> +\n> +test_expect_success 'bare repo setup' '\n> +       git init --bare bare1 &&\n> +       echo \"data\" > file1 &&\n\nStyle: No space after redirection operator.\n\nNit: Unnecessary quotes around 'data'.\n\n> +       git add file1 &&\n> +       git commit -m\"File1: add data\" &&\n> +       git push bare1 master &&\n> +       git reset --hard HEAD^\n> +'\n> +\n> +test_expect_success '\"list\" all worktrees from bare main' '\n> +       git -C bare1 worktree add --detach ../there master &&\n> +       echo \"$(cd bare1 && pwd)\" >expect &&\n\nThis may be problematic on Windows. See the discussion of $PWD in\nt/README. You may need to use $(pwd) instead. Same comment applies\nbelow, too.\n\n> +       echo \"$(git -C there rev-parse --show-toplevel)\" >>expect &&\n> +       git -C bare1 worktree list >actual &&\n> +       test_cmp expect actual &&\n> +       rm -rf there &&\n> +       git -C bare1 worktree prune\n> +'\n> +test_expect_success '\"list\" all worktrees --verbose from bare main' '\n> +       git -C bare1 worktree add --detach ../there master &&\n> +   echo \"$(cd bare1 && pwd)  (bare)\" >expect &&\n> +   echo \"$(git -C there rev-parse --show-toplevel)  $(git -C there rev-parse --short HEAD) (detached HEAD)\" >>expect &&\n\nBotched indentation here and in next test.\n\n> +       git -C bare1 worktree list --verbose >actual &&\n> +       test_cmp expect actual &&\n> +       rm -rf there &&\n> +       git -C bare1 worktree prune\n> +'\n> +\n> +test_expect_success '\"list\" all worktrees --verbose from worktree with a bare main' '\n> +       git -C bare1 worktree add --detach ../there master &&\n> +   echo \"$(cd bare1 && pwd)  (bare)\" >expect &&\n> +   echo \"$(git -C there rev-parse --show-toplevel)  $(git -C there rev-parse --short HEAD) (detached HEAD)\" >>expect &&\n> +       git -C there worktree list --verbose >actual &&\n> +       test_cmp expect actual &&\n> +       rm -rf there &&\n> +       git -C bare1 worktree prune\n> +'\n> +\n> +test_expect_success 'bare repo cleanup' '\n> +       rm -rf bare1\n> +'\n\nMeh. Probably unnecessary cleanup.\n\n> +\n> +test_done\n> --\n> 2.5.0\n"},{"id":"269858","messageId":"CAPig+cQr1WgvPY2QveN-5PeWWZhXXJjEDA27EzDmia2VKbyevw@mail.gmail.com","threadId":"40276","inReplyTo":"CAPig+cQRx-uKZnnx2O0pG34HkcEdg1GbGNFd9PHdw+m0O3aJEA@mail.gmail.com","subject":"Re: [PATCH v7 1/3] worktree: add top-level worktree.c","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-13T06:27:43Z","receivedAt":"2015-09-13T06:27:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:\n>> +               }\n>> +       } else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {\n>> +               if (starts_with(ref->buf, \"ref:\")) {\n>> +                       strbuf_remove(ref, 0, strlen(\"ref:\"));\n>> +                       strbuf_trim(ref);\n>> +               } else if (is_detached) {\n>> +                       *is_detached = 1;\n>\n> I find the placement of the detached detection logic here a bit\n> strange. The only case for which it might trigger is the so-called\n> \"main worktree\", yet having it in this general purpose parse function\n> seems to imply misleadingly that any worktree could be detached. Also,\n> given the current world order[1], wouldn't missing \"ref:\" indicate an\n> error for any worktree other than the main one? Consequently, this\n> detection probably ought to be done only for the main worktree\n> (outside of this function, probably).\n\nEh, ignore this bit. My brain was conflating 'bare' and 'detached'.\n"},{"id":"269913","messageId":"CANoM8SWP9YrZoUx5w9Do18uZf=5RrCV_ShvyaPQF4=9-vuFVbg@mail.gmail.com","threadId":"40276","inReplyTo":"CAPig+cQRx-uKZnnx2O0pG34HkcEdg1GbGNFd9PHdw+m0O3aJEA@mail.gmail.com","subject":"Re: [PATCH v7 1/3] worktree: add top-level worktree.c","fromName":"Mike Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2015-09-14T12:20:02Z","receivedAt":"2015-09-14T12:20:02Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> I realize that this is modeled closely after existing code in\n> branch.c, but, with the exception of parsing the ref file and\n> constructing a worktree structure, the main worktree case (id == NULL)\n> is entirely disjoint from the linked worktree case (id != NULL). This\n> suggests strongly that get_worktree() should be split into two\n> functions, one for the main worktree and one for linked worktrees,\n> which would make the code easier to understand. You might call the\n> functions get_main_worktree() and get_linked_worktree(id) (or perhaps\n> drop \"linked\" from the latter name).\n\nI originally wrote it like that, but I felt that the code looked like\nit was mostly duplicated in the functions.\nI can give it a relook.\n\n>> +\n>> +struct worktree_list *get_worktree_list()\n>\n> Can we be more concise and call this get_worktrees()?\n>\n\nI prefer 'get_worktree_list' because I also added the 'get_worktree'\nfunction, and I wanted to differentiate\nthe function names.\n\n>> diff --git a/worktree.h b/worktree.h\n>> new file mode 100644\n>> index 0000000..2bc0ab8\n>> --- /dev/null\n>> +++ b/worktree.h\n>> @@ -0,0 +1,48 @@\n>> +#ifndef WORKTREE_H\n>> +#define WORKTREE_H\n>> +\n>> +struct worktree {\n>> +       char *path;\n>> +       char *git_dir;\n>> +       char *head_ref;\n>> +       unsigned char head_sha1[20];\n>> +       int is_detached;\n>> +       int is_bare;\n>> +};\n>> +\n>> +struct worktree_list {\n>> +       struct worktree *worktree;\n>> +       struct worktree_list *next;\n>> +};\n>\n> I don't care too strongly, but an alternate approach (which I probably\n> would have taken) would be to have get_worktrees() simply return an\n> array of 'struct worktree' objects, hence no need for the additional\n> 'struct worktree_list'. The slight complication with this approach,\n> though, is that get_worktrees() either also needs to return the length\n> of the array, or the array should end with some sort of end-of-array\n> sentinel. An obvious sentinel would be path==NULL or git_dir==NULL or\n> all of the above.\n>\n> Client iteration is just about the same with the array approach as\n> with the linked-list approach.\n>\n\nI can't see what benefit this would provide.  I would sooner change\nthe returned list into\nan array-backed list struct.  Alternatively, I think adding a\nlist_head pointer to this structure\ncould benefit client code.\n"},{"id":"269946","messageId":"xmqqwpvsx5ie.fsf@gitster.mtv.corp.google.com","threadId":"40276","inReplyTo":"CANoM8SWP9YrZoUx5w9Do18uZf=5RrCV_ShvyaPQF4=9-vuFVbg@mail.gmail.com","subject":"Re: [PATCH v7 1/3] worktree: add top-level worktree.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-14T17:41:45Z","receivedAt":"2015-09-14T17:41:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Rappazzo <rappazzo@gmail.com> writes:\n\n> On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>> +struct worktree_list *get_worktree_list()\n>>\n>> Can we be more concise and call this get_worktrees()?\n>\n> I prefer 'get_worktree_list' because I also added the 'get_worktree'\n> function, and I wanted to differentiate\n> the function names.\n\nI'd say that plural can be differentiating enough; it probably is a\nmatter of taste.  How often do external callers want to call\nget_worktree() and not get_worktrees()?\n\n>>> diff --git a/worktree.h b/worktree.h\n>>> new file mode 100644\n>>> index 0000000..2bc0ab8\n>>> --- /dev/null\n>>> +++ b/worktree.h\n>>> @@ -0,0 +1,48 @@\n>>> +#ifndef WORKTREE_H\n>>> +#define WORKTREE_H\n>>> +\n>>> +struct worktree {\n>>> +       char *path;\n>>> +       char *git_dir;\n>>> +       char *head_ref;\n>>> +       unsigned char head_sha1[20];\n>>> +       int is_detached;\n>>> +       int is_bare;\n>>> +};\n>>> +\n>>> +struct worktree_list {\n>>> +       struct worktree *worktree;\n>>> +       struct worktree_list *next;\n>>> +};\n>>\n>> I don't care too strongly, but an alternate approach (which I probably\n>> would have taken) would be to have get_worktrees() simply return an\n>> array of 'struct worktree' objects, hence no need for the additional\n>> 'struct worktree_list'.\n\nI do not think we are using this to hold thousands of worktree\nobjects in core.  Adding \"struct worktree *next\" pointer to the\nworktree object itself would probably be sufficient for the need of\ncodepaths that want to enumerate and iterate over them and that\nwould be another way to lose the extra structure.\n"},{"id":"269947","messageId":"CANoM8SVJr_B83vU43MFmDiL8phRcTqG3=5krk9iquuH3w5dN_Q@mail.gmail.com","threadId":"40276","inReplyTo":"CAPig+cT6JLzPPpJKPxAZGGduEQTRzwa57pHtGOJjPYPxCwJV=w@mail.gmail.com","subject":"Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c","fromName":"Mike Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2015-09-14T17:44:15Z","receivedAt":"2015-09-14T17:44:15Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"On Sat, Sep 12, 2015 at 11:19 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:\n>> The code formerly in branch.c was largely the basis of the\n>> get_worktree_list implementation is now moved to worktree.c,\n>> and the find_shared_symref implementation has been refactored\n>> to use get_worktree_list\n>\n> Some comments below in addition to those by Junio...\n>\n>> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>\n>> ---\n>> diff --git a/branch.h b/branch.h\n>> index d3446ed..58aa45f 100644\n>> --- a/branch.h\n>> +++ b/branch.h\n>> @@ -59,12 +59,4 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name);\n>>   */\n>>  extern void die_if_checked_out(const char *branch);\n>>\n>> -/*\n>> - * Check if a per-worktree symref points to a ref in the main worktree\n>> - * or any linked worktree, and return the path to the exising worktree\n>> - * if it is.  Returns NULL if there is no existing ref.  The caller is\n>> - * responsible for freeing the returned path.\n>> - */\n>> -extern char *find_shared_symref(const char *symref, const char *target);\n>> -\n>>  #endif\n>> diff --git a/worktree.c b/worktree.c\n>> index 33d2e57..e45b651 100644\n>> --- a/worktree.c\n>> +++ b/worktree.c\n>> @@ -155,3 +155,43 @@ done:\n>>         return list;\n>>  }\n>>\n>> +char *find_shared_symref(const char *symref, const char *target)\n>> +{\n>> +       char *existing = NULL;\n>> +       struct strbuf path = STRBUF_INIT;\n>> +       struct strbuf sb = STRBUF_INIT;\n>> +       struct worktree_list *worktree_list = get_worktree_list();\n>> +       struct worktree_list *orig_list = worktree_list;\n>> +       struct worktree *matched = NULL;\n>> +\n>> +       while (!matched && worktree_list) {\n>> +               if (strcmp(\"HEAD\", symref)) {\n>\n> The result of strcmp() never changes, so this (expensive) check could\n> be lifted out of the 'while' loop, however...\n>\n>> +                       strbuf_reset(&path);\n>> +                       strbuf_reset(&sb);\n>> +                       strbuf_addf(&path, \"%s/%s\", worktree_list->worktree->git_dir, symref);\n>> +\n>> +                       if (_parse_ref(path.buf, &sb, NULL)) {\n>> +                               continue;\n>> +                       }\n>> +\n>> +                       if (!strcmp(sb.buf, target))\n>> +                               matched = worktree_list->worktree;\n>\n> The original code in branch.c, which this patch removes, did not need\n> to make a special case of HEAD, so it's not immediately clear why this\n> replacement code does so. This is the sort of issue which argues in\n> favor of mutating the existing code (slowly) over the course of\n> several patches into the final form, rather than having the final form\n> come into existence out of thin air. When the changes are made\n> incrementally, it is easier for reviewers to understand why such\n> modifications are needed, which (hopefully) should lead to fewer\n> questions such as this one.\n>\n\nI reversed the the check here; it is intended to check if the symref\nis _not_ the head, since the head\nref has already been parsed.  This is used in notes.c to find\n\"NOTES_MERGE_REF\".  I will move the\ncheck out of the loop as you suggest above.\n\n>> +               } else {\n>> +                       if (worktree_list->worktree->head_ref && !strcmp(worktree_list->worktree->head_ref, target))\n>> +                               matched = worktree_list->worktree;\n>> +               }\n>> +               worktree_list = worktree_list->next ? worktree_list->next : NULL;\n>> +       }\n>> +\n>> +       if (matched) {\n>> +               existing = malloc(strlen(matched->path) + 1);\n>> +               strcpy(existing, matched->path);\n>\n> A couple issues:\n>\n> This can be done more concisely and safely with xstrdup().\n>\n> In this codebase, it probably would be more idiomatic to use goto to\n> drop out of the loop rather than setting 'matched' and then having to\n> check 'matched' in the loop condition. So, for instance, each place\n> which sets 'matched' could instead say:\n>\n>     existing = xstrdup(...);\n>     goto done;\n>\n>> +       }\n>> +\n>> +done:\n>\n> This label doesn't have any matching goto's.\n>\n>> +       strbuf_release(&path);\n>> +       strbuf_release(&sb);\n>> +       worktree_list_release(orig_list);\n>> +\n>> +       return existing;\n>> +}\n>> diff --git a/worktree.h b/worktree.h\n>> index 2bc0ab8..320f17e 100644\n>> --- a/worktree.h\n>> +++ b/worktree.h\n>> @@ -45,4 +45,11 @@ struct worktree *get_worktree(const char *id);\n>>  extern void worktree_list_release(struct worktree_list *);\n>>  extern void worktree_release(struct worktree *);\n>>\n>> +/*\n>> + * Look for a symref in any worktree, and return the path to the first\n>> + * worktree found or NULL if not found.  The caller is responsible for\n>> + * freeing the returned path.\n>> + */\n>\n> For some reason, this comment differs a fair bit from the original in\n> branch.h which was removed by this patch, however, the original\n> comment was a bit more explanatory (I think).\n>\n> As a general rule, it's better to do code movement and code changes in\n> separate patches since it's hard for reviewers to detect and\n> comprehend differences in code which has been both moved and changed\n> at the same time.\n>\n\nI have rebased the history a bit for the reroll, and hopefully it will\nshow the changes a little\nmore clearly.\n\n>> +extern char *find_shared_symref(const char *symref, const char *target);\n>> +\n>>  #endif\n>> --\n>> 2.5.0\n"},{"id":"270159","messageId":"CAPig+cRPbFw8oTQ63rPtp1cyKms9zi0QkZNvkOWZoxb+mnpZrA@mail.gmail.com","threadId":"40276","inReplyTo":"CANoM8SWP9YrZoUx5w9Do18uZf=5RrCV_ShvyaPQF4=9-vuFVbg@mail.gmail.com","subject":"Re: [PATCH v7 1/3] worktree: add top-level worktree.c","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-16T20:32:40Z","receivedAt":"2015-09-16T20:32:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 14, 2015 at 8:20 AM, Mike Rappazzo <rappazzo@gmail.com> wrote:\n> On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>> +struct worktree {\n>>> +       char *path;\n>>> +       char *git_dir;\n>>> +       char *head_ref;\n>>> +       unsigned char head_sha1[20];\n>>> +       int is_detached;\n>>> +       int is_bare;\n>>> +};\n>>> +\n>>> +struct worktree_list {\n>>> +       struct worktree *worktree;\n>>> +       struct worktree_list *next;\n>>> +};\n>>\n>> I don't care too strongly, but an alternate approach (which I probably\n>> would have taken) would be to have get_worktrees() simply return an\n>> array of 'struct worktree' objects, hence no need for the additional\n>> 'struct worktree_list'. The slight complication with this approach,\n>> though, is that get_worktrees() either also needs to return the length\n>> of the array, or the array should end with some sort of end-of-array\n>> sentinel. An obvious sentinel would be path==NULL or git_dir==NULL or\n>> all of the above.\n>>\n>> Client iteration is just about the same with the array approach as\n>> with the linked-list approach.\n>\n> I can't see what benefit this would provide.  I would sooner change\n> the returned list into\n> an array-backed list struct.  Alternatively, I think adding a\n> list_head pointer to this structure\n> could benefit client code.\n\nThe benefit is a reduction in complexity, which is an important goal.\nLinked lists are inherently more complex than arrays, requiring extra\neffort, and especially extra care, to manage the head, each \"next\"\npointer (and possibly a tail pointer). Arrays are used often in Git,\nthus are familiar in this code-base, and Git has decent support for\nmaking their management relatively painless. Your suggestions of\nchanging into \"an array-backed list structure\" or \"adding list_head\npointer\" increase complexity, rather than reduce it.\n\nAside from the complexity issue, arrays allow random access, whereas\nlinked lists allow only sequential access. While this limitation may\nnot impact your current, planned use of get_worktrees(), it places a\npotentially unnecessary restriction on future clients.\n\nAnd, as noted, client iteration is at least as convenient with arrays,\nif not moreso, due to the reduction in noise (\"p++\" rather than \"p =\np->next\").\n\n    struct worktree *p;\n    for (p = get_worktrees(); p->path; p++)\n        blarp(p);\n\nThere are cases where linked lists are an obvious win, but this\ndoesn't seem to be such a case. On the other hand, there are obvious\nbenefits to making this an array, such as reduced complexity and\nremoval of the sequential-access-only restriction.\n"},{"id":"270161","messageId":"CAPig+cQ9QpGUZCaZXG3avChVdpXGoKXbKAc9vYUCFWMSUE-B4g@mail.gmail.com","threadId":"40276","inReplyTo":"xmqqwpvsx5ie.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v7 1/3] worktree: add top-level worktree.c","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-16T20:42:15Z","receivedAt":"2015-09-16T20:42:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 14, 2015 at 1:41 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Mike Rappazzo <rappazzo@gmail.com> writes:\n>> On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>>> +struct worktree_list *get_worktree_list()\n>>>\n>>> Can we be more concise and call this get_worktrees()?\n>>\n>> I prefer 'get_worktree_list' because I also added the 'get_worktree'\n>> function, and I wanted to differentiate\n>> the function names.\n>\n> I'd say that plural can be differentiating enough; it probably is a\n> matter of taste.  How often do external callers want to call\n> get_worktree() and not get_worktrees()?\n\nThe shorter name, get_worktrees(), also has the minor benefit of\nconcision, similar to the way we use short variable names (i, j, n, p,\ns) to help reveal and (often) make code structure obvious at a glance;\nwhereas long, noisy, wordy names tend to obscure code structure.\n\nThe \"_list\" suffix doesn't add any value over the shorter pluralizing\n\"s\"; in fact, it may be (very, very slightly) detrimental in implying\ntoo strongly that the return value must be a linked list.\n\n>>>> +struct worktree {\n>>>> +       char *path;\n>>>> +       char *git_dir;\n>>>> +       char *head_ref;\n>>>> +       unsigned char head_sha1[20];\n>>>> +       int is_detached;\n>>>> +       int is_bare;\n>>>> +};\n>>>> +\n>>>> +struct worktree_list {\n>>>> +       struct worktree *worktree;\n>>>> +       struct worktree_list *next;\n>>>> +};\n>>>\n>>> I don't care too strongly, but an alternate approach (which I probably\n>>> would have taken) would be to have get_worktrees() simply return an\n>>> array of 'struct worktree' objects, hence no need for the additional\n>>> 'struct worktree_list'.\n>\n> I do not think we are using this to hold thousands of worktree\n> objects in core.  Adding \"struct worktree *next\" pointer to the\n> worktree object itself would probably be sufficient for the need of\n> codepaths that want to enumerate and iterate over them and that\n> would be another way to lose the extra structure.\n\nI was more concerned with the inherent (and, in this case,\nunnecessary) complexity of a linked list. Being able to drop the extra\n'worktree_list' structure was just an added benefit of moving to the\nsimpler array approach.\n"},{"id":"270163","messageId":"CANoM8SWV0r9sHUTk3Rfu=psZLKmnjD19Dn_atCuDmc=41420-w@mail.gmail.com","threadId":"40276","inReplyTo":"CAPig+cRPbFw8oTQ63rPtp1cyKms9zi0QkZNvkOWZoxb+mnpZrA@mail.gmail.com","subject":"Re: [PATCH v7 1/3] worktree: add top-level worktree.c","fromName":"Mike Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2015-09-16T20:49:27Z","receivedAt":"2015-09-16T20:49:27Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"On Wed, Sep 16, 2015 at 4:32 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Mon, Sep 14, 2015 at 8:20 AM, Mike Rappazzo <rappazzo@gmail.com> wrote:\n>> On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>>> +struct worktree {\n>>>> +       char *path;\n>>>> +       char *git_dir;\n>>>> +       char *head_ref;\n>>>> +       unsigned char head_sha1[20];\n>>>> +       int is_detached;\n>>>> +       int is_bare;\n>>>> +};\n>>>> +\n>>>> +struct worktree_list {\n>>>> +       struct worktree *worktree;\n>>>> +       struct worktree_list *next;\n>>>> +};\n>>>\n>>> I don't care too strongly, but an alternate approach (which I probably\n>>> would have taken) would be to have get_worktrees() simply return an\n>>> array of 'struct worktree' objects, hence no need for the additional\n>>> 'struct worktree_list'. The slight complication with this approach,\n>>> though, is that get_worktrees() either also needs to return the length\n>>> of the array, or the array should end with some sort of end-of-array\n>>> sentinel. An obvious sentinel would be path==NULL or git_dir==NULL or\n>>> all of the above.\n>>>\n>>> Client iteration is just about the same with the array approach as\n>>> with the linked-list approach.\n>>\n>> I can't see what benefit this would provide.  I would sooner change\n>> the returned list into\n>> an array-backed list struct.  Alternatively, I think adding a\n>> list_head pointer to this structure\n>> could benefit client code.\n>\n> The benefit is a reduction in complexity, which is an important goal.\n> Linked lists are inherently more complex than arrays, requiring extra\n> effort, and especially extra care, to manage the head, each \"next\"\n> pointer (and possibly a tail pointer). Arrays are used often in Git,\n> thus are familiar in this code-base, and Git has decent support for\n> making their management relatively painless. Your suggestions of\n> changing into \"an array-backed list structure\" or \"adding list_head\n> pointer\" increase complexity, rather than reduce it.\n>\n> Aside from the complexity issue, arrays allow random access, whereas\n> linked lists allow only sequential access. While this limitation may\n> not impact your current, planned use of get_worktrees(), it places a\n> potentially unnecessary restriction on future clients.\n>\n> And, as noted, client iteration is at least as convenient with arrays,\n> if not moreso, due to the reduction in noise (\"p++\" rather than \"p =\n> p->next\").\n>\n>     struct worktree *p;\n>     for (p = get_worktrees(); p->path; p++)\n>         blarp(p);\n>\n> There are cases where linked lists are an obvious win, but this\n> doesn't seem to be such a case. On the other hand, there are obvious\n> benefits to making this an array, such as reduced complexity and\n> removal of the sequential-access-only restriction.\n\nWhat you are suggesting makes sense, but I feel it falls short when it\ncomes to the \"sentinel\".  Would the last element in the list be a\ndummy worktree?  I would sooner add a NULL to the end.  Would that be\nan acceptable implementation?\n\nI have re-coded it to put the next pointer in the worktree structure\nas Junio has suggested, but I am open to changing it to use the array\napproach.\n"},{"id":"270164","messageId":"CAPig+cTP_Vgtm=D1ESrPMs=r44OpGNUdEBjYMQgnQrtG-WsiaQ@mail.gmail.com","threadId":"40276","inReplyTo":"CANoM8SVJr_B83vU43MFmDiL8phRcTqG3=5krk9iquuH3w5dN_Q@mail.gmail.com","subject":"Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-16T21:09:40Z","receivedAt":"2015-09-16T21:09:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 14, 2015 at 1:44 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:\n> On Sat, Sep 12, 2015 at 11:19 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:\n>>> +       while (!matched && worktree_list) {\n>>> +               if (strcmp(\"HEAD\", symref)) {\n>>> +                       strbuf_reset(&path);\n>>> +                       strbuf_reset(&sb);\n>>> +                       strbuf_addf(&path, \"%s/%s\", worktree_list->worktree->git_dir, symref);\n>>> +\n>>> +                       if (_parse_ref(path.buf, &sb, NULL)) {\n>>> +                               continue;\n>>> +                       }\n>>> +\n>>> +                       if (!strcmp(sb.buf, target))\n>>> +                               matched = worktree_list->worktree;\n>>\n>> The original code in branch.c, which this patch removes, did not need\n>> to make a special case of HEAD, so it's not immediately clear why this\n>> replacement code does so. This is the sort of issue which argues in\n>> favor of mutating the existing code (slowly) over the course of\n>> several patches into the final form, rather than having the final form\n>> come into existence out of thin air. When the changes are made\n>> incrementally, it is easier for reviewers to understand why such\n>> modifications are needed, which (hopefully) should lead to fewer\n>> questions such as this one.\n>\n> I reversed the the check here; it is intended to check if the symref\n> is _not_ the head, since the head\n> ref has already been parsed.  This is used in notes.c to find\n> \"NOTES_MERGE_REF\".\n\nI'm probably being dense, but I still don't understand why the code\nnow needs a special case for HEAD, whereas the original didn't. But,\nmy denseness my be indicative of this change not being well-described\n(or described at all) by the commit message. Hopefully, when this is\nrefactored into finer changes, the purpose will become clear.\n\nThanks.\n"},{"id":"270167","messageId":"CANoM8SUTOgqOsPxO1NYgjPbwSkC=odeRKfbMtds65CZitSqxsA@mail.gmail.com","threadId":"40276","inReplyTo":"CAPig+cTP_Vgtm=D1ESrPMs=r44OpGNUdEBjYMQgnQrtG-WsiaQ@mail.gmail.com","subject":"Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c","fromName":"Mike Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2015-09-16T21:36:14Z","receivedAt":"2015-09-16T21:36:14Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"On Wed, Sep 16, 2015 at 5:09 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Mon, Sep 14, 2015 at 1:44 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:\n>> On Sat, Sep 12, 2015 at 11:19 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>> On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:\n>>>> +       while (!matched && worktree_list) {\n>>>> +               if (strcmp(\"HEAD\", symref)) {\n>>>> +                       strbuf_reset(&path);\n>>>> +                       strbuf_reset(&sb);\n>>>> +                       strbuf_addf(&path, \"%s/%s\", worktree_list->worktree->git_dir, symref);\n>>>> +\n>>>> +                       if (_parse_ref(path.buf, &sb, NULL)) {\n>>>> +                               continue;\n>>>> +                       }\n>>>> +\n>>>> +                       if (!strcmp(sb.buf, target))\n>>>> +                               matched = worktree_list->worktree;\n>>>\n>>> The original code in branch.c, which this patch removes, did not need\n>>> to make a special case of HEAD, so it's not immediately clear why this\n>>> replacement code does so. This is the sort of issue which argues in\n>>> favor of mutating the existing code (slowly) over the course of\n>>> several patches into the final form, rather than having the final form\n>>> come into existence out of thin air. When the changes are made\n>>> incrementally, it is easier for reviewers to understand why such\n>>> modifications are needed, which (hopefully) should lead to fewer\n>>> questions such as this one.\n>>\n>> I reversed the the check here; it is intended to check if the symref\n>> is _not_ the head, since the head\n>> ref has already been parsed.  This is used in notes.c to find\n>> \"NOTES_MERGE_REF\".\n>\n> I'm probably being dense, but I still don't understand why the code\n> now needs a special case for HEAD, whereas the original didn't. But,\n> my denseness my be indicative of this change not being well-described\n> (or described at all) by the commit message. Hopefully, when this is\n> refactored into finer changes, the purpose will become clear.\n>\n> Thanks.\n\nThe special case for HEAD is because the HEAD is already included in\nthe worktree struct.  This block is intended to save from re-parsing.\nIf you think the code would be easier to read, the HEAD check could be\nremoved, and the ref will just be parsed always.\n"},{"id":"270463","messageId":"CAPig+cQjhoVhQtP4_ZYTyLX3-GBXRx2eKM8Yv1=i=pSg=ZuM=Q@mail.gmail.com","threadId":"40276","inReplyTo":"CANoM8SWV0r9sHUTk3Rfu=psZLKmnjD19Dn_atCuDmc=41420-w@mail.gmail.com","subject":"Re: [PATCH v7 1/3] worktree: add top-level worktree.c","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-22T01:05:34Z","receivedAt":"2015-09-22T01:05:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 16, 2015 at 4:49 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:\n> On Wed, Sep 16, 2015 at 4:32 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> On Mon, Sep 14, 2015 at 8:20 AM, Mike Rappazzo <rappazzo@gmail.com> wrote:\n>>> On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>>> I don't care too strongly, but an alternate approach (which I probably\n>>>> would have taken) would be to have get_worktrees() simply return an\n>>>> array of 'struct worktree' objects, hence no need for the additional\n>>>> 'struct worktree_list'. The slight complication with this approach,\n>>>> though, is that get_worktrees() either also needs to return the length\n>>>> of the array, or the array should end with some sort of end-of-array\n>>>> sentinel. An obvious sentinel would be path==NULL or git_dir==NULL or\n>>>> all of the above.\n>>>>\n>>>> Client iteration is just about the same with the array approach as\n>>>> with the linked-list approach.\n>>>\n>>> I can't see what benefit this would provide.  I would sooner change\n>>> the returned list into\n>>> an array-backed list struct.  Alternatively, I think adding a\n>>> list_head pointer to this structure\n>>> could benefit client code.\n>>\n>> The benefit is a reduction in complexity, which is an important goal.\n>> Linked lists are inherently more complex than arrays, requiring extra\n>> effort, and especially extra care, to manage the head, each \"next\"\n>> pointer (and possibly a tail pointer). Arrays are used often in Git,\n>> thus are familiar in this code-base, and Git has decent support for\n>> making their management relatively painless. Your suggestions of\n>> changing into \"an array-backed list structure\" or \"adding list_head\n>> pointer\" increase complexity, rather than reduce it.\n>>\n>> Aside from the complexity issue, arrays allow random access, whereas\n>> linked lists allow only sequential access. While this limitation may\n>> not impact your current, planned use of get_worktrees(), it places a\n>> potentially unnecessary restriction on future clients.\n>>\n>> And, as noted, client iteration is at least as convenient with arrays,\n>> if not moreso, due to the reduction in noise (\"p++\" rather than \"p =\n>> p->next\").\n>>\n>>     struct worktree *p;\n>>     for (p = get_worktrees(); p->path; p++)\n>>         blarp(p);\n>>\n>> There are cases where linked lists are an obvious win, but this\n>> doesn't seem to be such a case. On the other hand, there are obvious\n>> benefits to making this an array, such as reduced complexity and\n>> removal of the sequential-access-only restriction.\n>\n> What you are suggesting makes sense, but I feel it falls short when it\n> comes to the \"sentinel\".  Would the last element in the list be a\n> dummy worktree?  I would sooner add a NULL to the end.  Would that be\n> an acceptable implementation?\n\nThe sentinel would indeed be a dummy worktree structure, which may be\nvery slightly ugly, however, it's dead simple, both in implementation\nand from client viewpoint, whereas other approaches increase\ncomplexity.\n\nMaking the last entry a NULL means get_worktrees() would have to\nreturn an array of pointers rather than an array of structures, which\nis more syntactically noisy, and complex since it's harder to reason\nabout pointer-to-pointer. In my mind, at least, the simplicity of the\narray of structures approach (even with the slight ugliness of the\ndummy sentinel) outweighs the complexity of the array-of-pointers\napproach.\n"},{"id":"270464","messageId":"CAPig+cQ6pH=NzUybg-y_0-=9BoHQNJf8Kx+fos=d+PkB97gcwg@mail.gmail.com","threadId":"40276","inReplyTo":"CANoM8SUTOgqOsPxO1NYgjPbwSkC=odeRKfbMtds65CZitSqxsA@mail.gmail.com","subject":"Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-22T01:07:31Z","receivedAt":"2015-09-22T01:07:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 16, 2015 at 5:36 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:\n> On Wed, Sep 16, 2015 at 5:09 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> On Mon, Sep 14, 2015 at 1:44 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:\n>>> On Sat, Sep 12, 2015 at 11:19 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>>> The original code in branch.c, which this patch removes, did not need\n>>>> to make a special case of HEAD, so it's not immediately clear why this\n>>>> replacement code does so. This is the sort of issue which argues in\n>>>> favor of mutating the existing code (slowly) over the course of\n>>>> several patches into the final form, rather than having the final form\n>>>> come into existence out of thin air. When the changes are made\n>>>> incrementally, it is easier for reviewers to understand why such\n>>>> modifications are needed, which (hopefully) should lead to fewer\n>>>> questions such as this one.\n>>>\n>>> I reversed the the check here; it is intended to check if the symref\n>>> is _not_ the head, since the head\n>>> ref has already been parsed.  This is used in notes.c to find\n>>> \"NOTES_MERGE_REF\".\n>>\n>> I'm probably being dense, but I still don't understand why the code\n>> now needs a special case for HEAD, whereas the original didn't. But,\n>> my denseness my be indicative of this change not being well-described\n>> (or described at all) by the commit message. Hopefully, when this is\n>> refactored into finer changes, the purpose will become clear.\n>\n> The special case for HEAD is because the HEAD is already included in\n> the worktree struct.  This block is intended to save from re-parsing.\n> If you think the code would be easier to read, the HEAD check could be\n> removed, and the ref will just be parsed always.\n\nI'm unable to judge whether the code would be easier to read since I\nstill don't understand the purpose of the special case. (But, as\nnoted, it may just be that I'm being dense, though not intentionally.)\n"},{"id":"270467","messageId":"xmqq7fnj6yn7.fsf@gitster.mtv.corp.google.com","threadId":"40276","inReplyTo":"CAPig+cQjhoVhQtP4_ZYTyLX3-GBXRx2eKM8Yv1=i=pSg=ZuM=Q@mail.gmail.com","subject":"Re: [PATCH v7 1/3] worktree: add top-level worktree.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-22T01:17:16Z","receivedAt":"2015-09-22T01:17:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> Making the last entry a NULL means get_worktrees() would have to\n> return an array of pointers rather than an array of structures, which\n> is more syntactically noisy, and complex since it's harder to reason\n> about pointer-to-pointer. In my mind, at least, the simplicity of the\n> array of structures approach (even with the slight ugliness of the\n> dummy sentinel) outweighs the complexity of the array-of-pointers\n> approach.\n\nHmph, I think the bog-standard way in our code is an array of\npointers.  An array of structures have a few downsides:\n\n - You have to copy a lot when you do realloc(3);\n\n - You have to move a lot when you insert a new element;\n\n - Individual structure cannot be pointed at by a pointer sensibly.\n   Passing &worktree[4] to a function that expects \"struct worktree *\"\n   is unsafe unless you are sure nobody is mucking with the worktree[]\n   array.\n\nFor the read-only operation \"worktree list\", the last one may not\nmatter much because you would build all the elements before doing\nanything else, but once you want to run this inside a library and\nmaintain the in-core forest of worktrees that are in sync with what\nyour running process does (i.e. create a new or destroy an existing\nworktree), it may become problematic.\n"}]}