{"thread":{"id":"40222","subject":"[PATCH v6 0/2] worktree: for-each function and list command","startedAt":"2015-08-30T19:10:37Z","lastAt":"2015-08-31T21:37:02Z","messageCount":14,"participants":["Michael Rappazzo","Eric Sunshine","Junio C Hamano","David Turner","Mike Rappazzo"],"isPatch":true,"patchVersion":6,"patchTotal":2},"messages":[{"id":"268942","messageId":"1440961839-40575-1-git-send-email-rappazzo@gmail.com","threadId":"40222","inReplyTo":null,"subject":"[PATCH v6 0/2] worktree: for-each function and list command","fromName":"Michael Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2015-08-30T19:10:37Z","receivedAt":"2015-08-30T19:10:37Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"Changes since v5:\n - used already existing functions instead of reimplementing:\n     - resolve_ref_unsafe\n     - find_unique_abbrev\n     - shorten_unambiguous_ref\n - added more tests for 'worktree list'\n\nMichael Rappazzo (2):\n  worktree: add 'for_each_worktree' function\n  worktree: add 'list' command\n\n Documentation/git-worktree.txt |  10 +++-\n builtin/worktree.c             | 125 +++++++++++++++++++++++++++++++++++++++++\n t/t2027-worktree-list.sh       | 100 +++++++++++++++++++++++++++++++++\n 3 files changed, 233 insertions(+), 2 deletions(-)\n create mode 100755 t/t2027-worktree-list.sh\n\n-- \n2.5.0\n"},{"id":"268943","messageId":"1440961839-40575-2-git-send-email-rappazzo@gmail.com","threadId":"40222","inReplyTo":"1440961839-40575-1-git-send-email-rappazzo@gmail.com","subject":"[PATCH v6 1/2] worktree: add 'for_each_worktree' function","fromName":"Michael Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2015-08-30T19:10:38Z","receivedAt":"2015-08-30T19:10:38Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"for_each_worktree iterates through each worktree and invokes a callback\nfunction.  The main worktree (if not bare) is always encountered first,\nfollowed by worktrees created by `git worktree add`.\n\nIf the callback function returns a non-zero value, iteration stops, and\nthe callback's return value is returned from the for_each_worktree\nfunction.\n\nSigned-off-by: Michael Rappazzo <rappazzo@gmail.com>\n---\n builtin/worktree.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 83 insertions(+)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 430b51e..7b3cb96 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -26,6 +26,14 @@ static int show_only;\n static int verbose;\n static unsigned long expire;\n \n+/*\n+ * The signature for the callback function for the for_each_worktree()\n+ * function below.  The memory pointed to by the callback arguments\n+ * is only guaranteed to be valid for the duration of a single\n+ * callback invocation.\n+ */\n+typedef int each_worktree_fn(const char *path, const char *git_dir, void *cb_data);\n+\n static int prune_worktree(const char *id, struct strbuf *reason)\n {\n \tstruct stat st;\n@@ -359,6 +367,81 @@ static int add(int ac, const char **av, const char *prefix)\n \treturn add_worktree(path, branch, &opts);\n }\n \n+/*\n+ * Iterate through each worktree and invoke the callback function.  If\n+ * the callback function returns non-zero, the iteration stops, and\n+ * this function returns that return value\n+ */\n+static int for_each_worktree(each_worktree_fn fn, void *cb_data)\n+{\n+\tstruct strbuf worktree_path = STRBUF_INIT;\n+\tstruct strbuf worktree_git = STRBUF_INIT;\n+\tconst char *common_dir;\n+\tint main_is_bare = 0;\n+\tint ret = 0;\n+\n+\tcommon_dir = get_git_common_dir();\n+\tif (!strcmp(common_dir, get_git_dir())) {\n+\t\t/* simple case - this is the main repo */\n+\t\tmain_is_bare = is_bare_repository();\n+\t\tif (!main_is_bare) {\n+\t\t\tstrbuf_addstr(&worktree_path, get_git_work_tree());\n+\t\t} else {\n+\t\t\tstrbuf_addstr(&worktree_path, common_dir);\n+\t\t}\n+\t} else {\n+\t\tstrbuf_addstr(&worktree_path, common_dir);\n+\t\t/* If the common_dir DOES NOT end with '/.git', then it is bare */\n+\t\tmain_is_bare = !strbuf_strip_suffix(&worktree_path, \"/.git\");\n+\t}\n+\tstrbuf_addstr(&worktree_git, worktree_path.buf);\n+\n+\tif (!main_is_bare) {\n+\t\tstrbuf_addstr(&worktree_git, \"/.git\");\n+\n+\t\tret = fn(worktree_path.buf, worktree_git.buf, cb_data);\n+\t\tif (ret)\n+\t\t\tgoto done;\n+\t}\n+\tstrbuf_addstr(&worktree_git, \"/worktrees\");\n+\n+\tif (is_directory(worktree_git.buf)) {\n+\t\tDIR *dir = opendir(worktree_git.buf);\n+\t\tif (dir) {\n+\t\t\tstruct stat st;\n+\t\t\tstruct dirent *d;\n+\t\t\tsize_t base_path_len = worktree_git.len;\n+\n+\t\t\twhile ((d = readdir(dir)) != NULL) {\n+\t\t\t\tif (!strcmp(d->d_name, \".\") || !strcmp(d->d_name, \"..\"))\n+\t\t\t\t\tcontinue;\n+\n+\t\t\t\tstrbuf_setlen(&worktree_git, base_path_len);\n+\t\t\t\tstrbuf_addf(&worktree_git, \"/%s/gitdir\", d->d_name);\n+\n+\t\t\t\tif (stat(worktree_git.buf, &st)) {\n+\t\t\t\t\tfprintf(stderr, \"Unable to read worktree git dir: %s\\n\", worktree_git.buf);\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n+\n+\t\t\t\tstrbuf_reset(&worktree_path);\n+\t\t\t\tstrbuf_read_file(&worktree_path, worktree_git.buf, st.st_size);\n+\t\t\t\tstrbuf_strip_suffix(&worktree_path, \"/.git\\n\");\n+\n+\t\t\t\tstrbuf_strip_suffix(&worktree_git, \"/gitdir\");\n+\t\t\t\tret = fn(worktree_path.buf, worktree_git.buf, cb_data);\n+\t\t\t\tif (ret)\n+\t\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tclosedir(dir);\n+\t}\n+done:\n+\tstrbuf_release(&worktree_git);\n+\tstrbuf_release(&worktree_path);\n+\treturn ret;\n+}\n+\n int cmd_worktree(int ac, const char **av, const char *prefix)\n {\n \tstruct option options[] = {\n-- \n2.5.0\n"},{"id":"268944","messageId":"1440961839-40575-3-git-send-email-rappazzo@gmail.com","threadId":"40222","inReplyTo":"1440961839-40575-1-git-send-email-rappazzo@gmail.com","subject":"[PATCH v6 2/2] worktree: add 'list' command","fromName":"Michael Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2015-08-30T19:10:39Z","receivedAt":"2015-08-30T19:10:39Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"'git worktree list' uses the for_each_worktree function to iterate,\nand outputs the worktree dir.  With the verbose option set, it also\nshows the branch or revision currently checked out in that worktree.\n\nSigned-off-by: Michael Rappazzo <rappazzo@gmail.com>\n---\n Documentation/git-worktree.txt |  10 ++++-\n builtin/worktree.c             |  42 +++++++++++++++++\n t/t2027-worktree-list.sh       | 100 +++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 150 insertions(+), 2 deletions(-)\n create mode 100755 t/t2027-worktree-list.sh\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex fb68156..867a24a 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|--verbose]\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 all of the linked worktrees.\n+\n OPTIONS\n -------\n \n@@ -88,11 +93,13 @@ OPTIONS\n \n -v::\n --verbose::\n-\tWith `prune`, report all removals.\n+\tWith `prune`, report all removals.  \n+\tWith `list` show the branch or revision currently checked out in that worktree.\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 +174,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 7b3cb96..6d96cdf 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -12,6 +12,7 @@\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@@ -442,6 +443,45 @@ done:\n \treturn ret;\n }\n \n+static int print_worktree_details(const char *path, const char *git_dir, void *cb_data)\n+{\n+\tprintf(\"%s\", path);\n+\n+\tif (verbose) {\n+\t\tenter_repo(git_dir, 1);\n+\t\tprintf(\"  [\");\n+\t\tunsigned char sha1[20];\n+\t\tint flag;\n+\n+\t\tconst char *refname = resolve_ref_unsafe(\"HEAD\", 0, sha1, &flag);\n+\t\tif (!(flag & REF_ISSYMREF)) {\n+\t\t\tprintf(\"%s\", find_unique_abbrev(sha1, DEFAULT_ABBREV));\n+\t\t} else {\n+\t\t\trefname = shorten_unambiguous_ref(refname, 0);\n+\t\t\tprintf(\"%s\", refname);\n+\t\t}\n+\n+\t\tprintf(\"]\");\n+\t}\n+\tprintf(\"\\n\");\n+\n+\treturn 0;\n+}\n+\n+static int list(int ac, const char **av, const char *prefix)\n+{\n+\tstruct option options[] = {\n+\t\tOPT__VERBOSE(&verbose, N_(\"show the branch or revision currently checked out\")),\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+\n+\treturn for_each_worktree(&print_worktree_details, NULL);\n+}\n+\n int cmd_worktree(int ac, const char **av, const char *prefix)\n {\n \tstruct option options[] = {\n@@ -454,5 +494,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..85bd243\n--- /dev/null\n+++ b/t/t2027-worktree-list.sh\n@@ -0,0 +1,100 @@\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 symbolic-ref --short HEAD)]\" >expect &&\n+\tgit worktree add --detach here master &&\n+\techo \"$(git -C here rev-parse --show-toplevel)  [$(git -C here rev-parse --short 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 symbolic-ref --short HEAD)]\" >expect &&\n+\tgit worktree add --detach here master &&\n+\techo \"$(git -C here rev-parse --show-toplevel)  [$(git -C here rev-parse --short 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 \"$(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 worktree with a bare main' '\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 >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+\techo \"$(git -C there rev-parse --show-toplevel)  [$(git -C there rev-parse --short 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+\techo \"$(git -C there rev-parse --show-toplevel)  [$(git -C there rev-parse --short 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":"268959","messageId":"CAPig+cTHZrQn8LpfftcsAQhFAykgDorbR97tkcuSCFYD_ngs9g@mail.gmail.com","threadId":"40222","inReplyTo":"1440961839-40575-2-git-send-email-rappazzo@gmail.com","subject":"Re: [PATCH v6 1/2] worktree: add 'for_each_worktree' function","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-31T05:11:46Z","receivedAt":"2015-08-31T05:11:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Thanks for working on this. I apologize for not reviewing earlier\nrounds (other than v2 [1]); it's been difficult to find a block of\ntime to do so...\n\nOn Sun, Aug 30, 2015 at 3:10 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:\n> for_each_worktree iterates through each worktree and invokes a callback\n> function.  The main worktree (if not bare) is always encountered first,\n> followed by worktrees created by `git worktree add`.\n\nWhy does this iteration function specially filter out a bare main\nworktree? If it instead unconditionally vends the main worktree (bare\nor not), then the caller can make its own decision about what to do\nwith it, thus empowering the caller, rather than imposing a (possibly)\narbitrary restriction upon it.\n\nFor instance, the \"git worktree list\" command may very well want to\nshow the main worktree, even if bare (possibly controlled by a\ncommand-line option), annotated appropriately (\"[bare]\"). This may be\nexactly the sort of information a user wants to know, and by leaving\nthe decision up to the caller, then the caller (\"git worktree list\" in\nthis example) has the opportunity to act accordingly, whereas if\nfor_each_worktree() filters out a bare main worktree unconditionally,\nthen the caller (\"git worktree list\") will never be able to offer such\nan option.\n\n> If the callback function returns a non-zero value, iteration stops, and\n> the callback's return value is returned from the for_each_worktree\n> function.\n\nStepping back a bit, is a for-each-foo()-style interface desirable?\nThis sort of interface imposes a good deal of complexity on callers,\ndemanding a callback function and callback data (cb_data), and is\ngenerally (at least in C) more difficult to reason about than other\nsimpler interfaces. Is such complexity warranted?\n\nAn alternate, much simpler interface would be to have a function, say\nget_worktrees(), return an array of 'worktree' structures to the\ncaller, which the caller would iterate over (which is a common\noperation in C, thus easily reasoned about).\n\nThe one benefit of a for-each-foo()-style interface is that it's\npossible to \"exit early\", thus avoiding the cost of interrogating\nmeta-data for worktrees in which the caller is not interested,\nhowever, it seems unlikely that there will be so many worktrees linked\nto a repository for this early exit to translate into any real\nsavings.\n\n> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>\n> ---\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 430b51e..7b3cb96 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -26,6 +26,14 @@ static int show_only;\n>  static int verbose;\n>  static unsigned long expire;\n>\n> +/*\n> + * The signature for the callback function for the for_each_worktree()\n> + * function below.  The memory pointed to by the callback arguments\n> + * is only guaranteed to be valid for the duration of a single\n> + * callback invocation.\n> + */\n> +typedef int each_worktree_fn(const char *path, const char *git_dir, void *cb_data);\n\nIn my review[1] of v2, I mentioned that (at some point) the \"git\nworktree list\" command might like to show additional information about\nthe worktree other than just its location. Such information might\ninclude its tag, the checked out branch (or detached HEAD), whether\nit's locked (and the lock reason and whether the worktree is currently\n\"online\"), whether it can be pruned (and the prune reason).\n\nCommands such as \"git worktree add\" and \"git checkout\" need to know if\na branch is already checked out in some (other) worktree, thus they\nalso need the \"branch\" information.\n\nThis need by clients for additional worktree meta-data suggests that\nthe additional information ought to be encapsulated into a 'struct\nworktree', and that for_each_worktree() should vend a 'struct\nworktree' rather than vending merely the \"git dir\". (Alternately, the\nabove-suggested get_worktrees() should return an array of 'struct\nworktree' items.)\n\nAn important reason for making for_each_worktree() vend a 'struct\nworktree' is that it facilitates centralizing all the logic for\ndetermining and computing the extra worktree meta-data in one place,\nthus relieving callers of burden of having to compute the extra\ninformation themselves. (Junio alluded to this in his v5 review[2].)\n\n>  static int prune_worktree(const char *id, struct strbuf *reason)\n>  {\n>         struct stat st;\n> @@ -359,6 +367,81 @@ static int add(int ac, const char **av, const char *prefix)\n>         return add_worktree(path, branch, &opts);\n>  }\n>\n> +/*\n> + * Iterate through each worktree and invoke the callback function.  If\n> + * the callback function returns non-zero, the iteration stops, and\n> + * this function returns that return value\n> + */\n> +static int for_each_worktree(each_worktree_fn fn, void *cb_data)\n\nUltimately, code outside of builtin/worktree.c will want to benefit\nfrom the encapsulation of this worktree iteration logic. For instance,\nthe support code in (top-level) branch.c invoked by \"git worktree add\"\nand \"git checkout\" to determine if a branch is already checked out in\nsome (other) branch could avail itself of this iteration interface.\n\nAs such, it would make sense to place this code at location where it\ncan be accessed by callers other than just builtin/worktree.c. A good\nlocation for it to reside might be in a new top-level file worktree.c.\n\nIt doesn't have to be part of the current patch series, but,\neventually, the code in branch.c for determining if a branch is\nalready checked out elsewhere also should migrate to the new top-level\nworktree.c; in particular, die_if_checked_out(), find_shared_symref(),\nand find_linked_symref().\n\n> +{\n> +       struct strbuf worktree_path = STRBUF_INIT;\n> +       struct strbuf worktree_git = STRBUF_INIT;\n> +       const char *common_dir;\n> +       int main_is_bare = 0;\n> +       int ret = 0;\n> +\n> +       common_dir = get_git_common_dir();\n> +       if (!strcmp(common_dir, get_git_dir())) {\n> +               /* simple case - this is the main repo */\n> +               main_is_bare = is_bare_repository();\n> +               if (!main_is_bare) {\n> +                       strbuf_addstr(&worktree_path, get_git_work_tree());\n> +               } else {\n> +                       strbuf_addstr(&worktree_path, common_dir);\n> +               }\n> +       } else {\n> +               strbuf_addstr(&worktree_path, common_dir);\n> +               /* If the common_dir DOES NOT end with '/.git', then it is bare */\n> +               main_is_bare = !strbuf_strip_suffix(&worktree_path, \"/.git\");\n> +       }\n> +       strbuf_addstr(&worktree_git, worktree_path.buf);\n\nI may have missed some discussion in earlier rounds (or perhaps I'm\ntoo simple-minded), but I'm confused about why this logic (and most of\nthe rest of the function) differs so much from existing logic in\nbranch.c:find_shared_symref() and find_linked_symref() for iterating\nover the worktrees and gleaning information about them. That logic in\nbranch.c seems to do a pretty good job of reporting the worktree in\nwhich a branch is already checked out, so it's not clear why the above\nlogic takes a different (and seemingly more complex) approach.\n\n> +       if (!main_is_bare) {\n> +               strbuf_addstr(&worktree_git, \"/.git\");\n> +\n> +               ret = fn(worktree_path.buf, worktree_git.buf, cb_data);\n> +               if (ret)\n> +                       goto done;\n> +       }\n> +       strbuf_addstr(&worktree_git, \"/worktrees\");\n> +\n> +       if (is_directory(worktree_git.buf)) {\n\nAs mentioned in my v2 review[1], this is_directory() invocation\ndoesn't buy you anything. The following opendir() will either succeed\nor fail anyhow, so checking beforehand if 'worktree_git' is a\ndirectory is wasted work. If you eliminate is_directory(), you avoid\nthat unnecessary work (and the rest of the code can be less deeply\nindented).\n\n> +               DIR *dir = opendir(worktree_git.buf);\n> +               if (dir) {\n> +                       struct stat st;\n> +                       struct dirent *d;\n> +                       size_t base_path_len = worktree_git.len;\n> +\n> +                       while ((d = readdir(dir)) != NULL) {\n> +                               if (!strcmp(d->d_name, \".\") || !strcmp(d->d_name, \"..\"))\n> +                                       continue;\n> +\n> +                               strbuf_setlen(&worktree_git, base_path_len);\n> +                               strbuf_addf(&worktree_git, \"/%s/gitdir\", d->d_name);\n> +\n> +                               if (stat(worktree_git.buf, &st)) {\n> +                                       fprintf(stderr, \"Unable to read worktree git dir: %s\\n\", worktree_git.buf);\n> +                                       continue;\n> +                               }\n> +\n> +                               strbuf_reset(&worktree_path);\n> +                               strbuf_read_file(&worktree_path, worktree_git.buf, st.st_size);\n> +                               strbuf_strip_suffix(&worktree_path, \"/.git\\n\");\n> +\n> +                               strbuf_strip_suffix(&worktree_git, \"/gitdir\");\n> +                               ret = fn(worktree_path.buf, worktree_git.buf, cb_data);\n> +                               if (ret)\n> +                                       break;\n> +                       }\n> +               }\n> +               closedir(dir);\n\nThis closedir() should be inside the `if (dir) {...}' block rather than outside.\n\nReturning to the issue of this code differing from existing code in\nbranch.c for iterating worktrees, I'm curious as to why this\nfunctionality was implemented again from scratch here rather than\nre-using the existing (somewhat) well proven code and logic in\nbranch.c:find_shared_symref() and find_linked_symref(). My v2\nreview[1] hinted at re-use a couple times.\n\nConsidering that the existing code in branch.c for iterating worktrees\nhas gone through several code reviews and is already (somewhat)\nproven, re-using that code for a general purpose worktree iterator\nmakes lots of sense compared with writing it again from scratch, as is\ndone here. Consequently, I, personally, would be happier to see the\nexisting code refactored (perhaps over several patches) to the point\nthat it can be re-used as a general purpose iterator. But, that's my\nopinion; I can't speak for others.\n\n> +       }\n> +done:\n> +       strbuf_release(&worktree_git);\n> +       strbuf_release(&worktree_path);\n> +       return ret;\n> +}\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/275528\n[2]: http://article.gmane.org/gmane.comp.version-control.git/276471\n"},{"id":"268960","messageId":"CAPig+cQ0oJbKV1PFPvDfjejyUfHmxzJmmYH3Wf0muNoiTU7Dew@mail.gmail.com","threadId":"40222","inReplyTo":"1440961839-40575-3-git-send-email-rappazzo@gmail.com","subject":"Re: [PATCH v6 2/2] worktree: add 'list' command","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-31T06:23:18Z","receivedAt":"2015-08-31T06:23:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 30, 2015 at 3:10 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:\n> 'git worktree list' uses the for_each_worktree function to iterate,\n> and outputs the worktree dir.  With the verbose option set, it also\n> shows the branch or revision currently checked out in that worktree.\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..867a24a 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|--verbose]\n\nFor conciseness and consistency with the \"git worktree prune\"\nsynopsis, this should mention only -v (and omit --verbose).\n\n>  DESCRIPTION\n>  -----------\n> @@ -88,11 +93,13 @@ OPTIONS\n>\n>  -v::\n>  --verbose::\n> -       With `prune`, report all removals.\n> +       With `prune`, report all removals.\n> +       With `list` show the branch or revision currently checked out in that worktree.\n\nAdd a comma after \"With `list`\".\nAlso: s/in that/in each/\nThis new line is overly long; wrapping it over a couple lines would help.\n\n>  --expire <time>::\n>         With `prune`, only expire unused working trees older than <time>.\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 7b3cb96..6d96cdf 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> +static int print_worktree_details(const char *path, const char *git_dir, void *cb_data)\n> +{\n> +       printf(\"%s\", path);\n> +       if (verbose) {\n> +               enter_repo(git_dir, 1);\n> +               printf(\"  [\");\n> +               unsigned char sha1[20];\n> +               int flag;\n> +\n> +               const char *refname = resolve_ref_unsafe(\"HEAD\", 0, sha1, &flag);\n\nDeclarations after statement. Move the declarations of 'sha1', 'flag',\nand 'refname' above the statements (enter_repo() and printf()).\n\n > +               if (!(flag & REF_ISSYMREF)) {\n> +                       printf(\"%s\", find_unique_abbrev(sha1, DEFAULT_ABBREV));\n> +               } else {\n> +                       refname = shorten_unambiguous_ref(refname, 0);\n> +                       printf(\"%s\", refname);\n> +               }\n\nAs mentioned in my review[1] of patch 1/2, it would be better for this\nsort of logic to be handled by the worktree iterator itself so that\nall callers can benefit, rather than having to repeat the work in each\ncaller. This information would be encapsulated in a 'struct worktree'\nalong with 'path' and 'git_dir' and other interesting information\nvended by the the iterator function.\n\n> +               printf(\"]\");\n\nRather than dropping printf()'s here and there to compose the output\npiecemeal, it would be cleaner, and facilitate future changes, to\nassign the refname/sha1 to a variable, and interpolate that variable\ninto an all-encompasing printf(), like this:\n\n    printf(\"  [%s]\", branch_or_sha1);\n\n> +       }\n> +       printf(\"\\n\");\n> +\n> +       return 0;\n> +}\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/276847\n"},{"id":"269011","messageId":"xmqqd1y3wdcr.fsf@gitster.mtv.corp.google.com","threadId":"40222","inReplyTo":"CAPig+cTHZrQn8LpfftcsAQhFAykgDorbR97tkcuSCFYD_ngs9g@mail.gmail.com","subject":"Re: [PATCH v6 1/2] worktree: add 'for_each_worktree' function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-31T17:56:52Z","receivedAt":"2015-08-31T17:56:52Z","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> Thanks for working on this....\n\n<aol>metoo</aol>\n\n> Stepping back a bit, is a for-each-foo()-style interface desirable?\n> This sort of interface imposes a good deal of complexity on callers,\n> demanding a callback function and callback data (cb_data), and is\n> generally (at least in C) more difficult to reason about than other\n> simpler interfaces. Is such complexity warranted?\n>\n> An alternate, much simpler interface would be to have a function, say\n> get_worktrees(), return an array of 'worktree' structures to the\n> caller, which the caller would iterate over (which is a common\n> operation in C, thus easily reasoned about).\n\nNice.\n"},{"id":"269019","messageId":"1441046674.25570.15.camel@twopensource.com","threadId":"40222","inReplyTo":"CAPig+cTHZrQn8LpfftcsAQhFAykgDorbR97tkcuSCFYD_ngs9g@mail.gmail.com","subject":"Re: [PATCH v6 1/2] worktree: add 'for_each_worktree' function","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-08-31T18:44:34Z","receivedAt":"2015-08-31T18:44:34Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Mon, 2015-08-31 at 01:11 -0400, Eric Sunshine wrote:\n> Stepping back a bit, is a for-each-foo()-style interface desirable?\n> This sort of interface imposes a good deal of complexity on callers,\n> demanding a callback function and callback data (cb_data), and is\n> generally (at least in C) more difficult to reason about than other\n> simpler interfaces. Is such complexity warranted?\n> \n> An alternate, much simpler interface would be to have a function, say\n> get_worktrees(), return an array of 'worktree' structures to the\n> caller, which the caller would iterate over (which is a common\n> operation in C, thus easily reasoned about).\n> \n> The one benefit of a for-each-foo()-style interface is that it's\n> possible to \"exit early\", thus avoiding the cost of interrogating\n> meta-data for worktrees in which the caller is not interested,\n> however, it seems unlikely that there will be so many worktrees linked\n> to a repository for this early exit to translate into any real\n> savings.\n\nThe other benefit is that there is no need to worry about deallocating\nthe list.  But that might be too minor to worry about.\n"},{"id":"269026","messageId":"CANoM8SUoJAPBwEO=udhaR0WcUkZropEJ5G_SESnkHVtAQHxJQQ@mail.gmail.com","threadId":"40222","inReplyTo":"CAPig+cTHZrQn8LpfftcsAQhFAykgDorbR97tkcuSCFYD_ngs9g@mail.gmail.com","subject":"Re: [PATCH v6 1/2] worktree: add 'for_each_worktree' function","fromName":"Mike Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2015-08-31T18:57:09Z","receivedAt":"2015-08-31T18:57:09Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"On Mon, Aug 31, 2015 at 1:11 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> Thanks for working on this. I apologize for not reviewing earlier\n> rounds (other than v2 [1]); it's been difficult to find a block of\n> time to do so...\n\nI appreciate your time and comments.\n\n>\n> On Sun, Aug 30, 2015 at 3:10 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:\n>> for_each_worktree iterates through each worktree and invokes a callback\n>> function.  The main worktree (if not bare) is always encountered first,\n>> followed by worktrees created by `git worktree add`.\n>\n> Why does this iteration function specially filter out a bare main\n> worktree? If it instead unconditionally vends the main worktree (bare\n> or not), then the caller can make its own decision about what to do\n> with it, thus empowering the caller, rather than imposing a (possibly)\n> arbitrary restriction upon it.\n>\n> For instance, the \"git worktree list\" command may very well want to\n> show the main worktree, even if bare (possibly controlled by a\n> command-line option), annotated appropriately (\"[bare]\"). This may be\n> exactly the sort of information a user wants to know, and by leaving\n> the decision up to the caller, then the caller (\"git worktree list\" in\n> this example) has the opportunity to act accordingly, whereas if\n> for_each_worktree() filters out a bare main worktree unconditionally,\n> then the caller (\"git worktree list\") will never be able to offer such\n> an option.\n\nI wasn't sure that a bare repo would be considered a worktree.  I\ndon't think that it would be\na good idea to include it.  In the same vein that I can't checkout a\nbranch in a bare repo, it\nfigure that it shouldn't be in the list.\n\n>\n>> If the callback function returns a non-zero value, iteration stops, and\n>> the callback's return value is returned from the for_each_worktree\n>> function.\n>\n> Stepping back a bit, is a for-each-foo()-style interface desirable?\n> This sort of interface imposes a good deal of complexity on callers,\n> demanding a callback function and callback data (cb_data), and is\n> generally (at least in C) more difficult to reason about than other\n> simpler interfaces. Is such complexity warranted?\n>\n> An alternate, much simpler interface would be to have a function, say\n> get_worktrees(), return an array of 'worktree' structures to the\n> caller, which the caller would iterate over (which is a common\n> operation in C, thus easily reasoned about).\n>\n> The one benefit of a for-each-foo()-style interface is that it's\n> possible to \"exit early\", thus avoiding the cost of interrogating\n> meta-data for worktrees in which the caller is not interested,\n> however, it seems unlikely that there will be so many worktrees linked\n> to a repository for this early exit to translate into any real\n> savings.\n\nI am not opposed to making a simple function as you describe.  I think David was\nlooking for a callback style function.  I don't think it would be\nterrible to keep the\ncallback and then also include the simple function to return the\nstruct array.  I like\nthe memory management of the callback better than the struct array though.\n\n\n>\n>> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>\n>> ---\n>> diff --git a/builtin/worktree.c b/builtin/worktree.c\n>> index 430b51e..7b3cb96 100644\n>> --- a/builtin/worktree.c\n>> +++ b/builtin/worktree.c\n>> @@ -26,6 +26,14 @@ static int show_only;\n>>  static int verbose;\n>>  static unsigned long expire;\n>>\n>> +/*\n>> + * The signature for the callback function for the for_each_worktree()\n>> + * function below.  The memory pointed to by the callback arguments\n>> + * is only guaranteed to be valid for the duration of a single\n>> + * callback invocation.\n>> + */\n>> +typedef int each_worktree_fn(const char *path, const char *git_dir, void *cb_data);\n>\n> In my review[1] of v2, I mentioned that (at some point) the \"git\n> worktree list\" command might like to show additional information about\n> the worktree other than just its location. Such information might\n> include its tag, the checked out branch (or detached HEAD), whether\n> it's locked (and the lock reason and whether the worktree is currently\n> \"online\"), whether it can be pruned (and the prune reason).\n>\n> Commands such as \"git worktree add\" and \"git checkout\" need to know if\n> a branch is already checked out in some (other) worktree, thus they\n> also need the \"branch\" information.\n>\n> This need by clients for additional worktree meta-data suggests that\n> the additional information ought to be encapsulated into a 'struct\n> worktree', and that for_each_worktree() should vend a 'struct\n> worktree' rather than vending merely the \"git dir\". (Alternately, the\n> above-suggested get_worktrees() should return an array of 'struct\n> worktree' items.)\n>\n> An important reason for making for_each_worktree() vend a 'struct\n> worktree' is that it facilitates centralizing all the logic for\n> determining and computing the extra worktree meta-data in one place,\n> thus relieving callers of burden of having to compute the extra\n> information themselves. (Junio alluded to this in his v5 review[2].)\n\nI think that this makes sense.  I will refactor to use a struct, and\ninclude that in\nthe callback signature and/or the struct array function.\n\n>\n>>  static int prune_worktree(const char *id, struct strbuf *reason)\n>>  {\n>>         struct stat st;\n>> @@ -359,6 +367,81 @@ static int add(int ac, const char **av, const char *prefix)\n>>         return add_worktree(path, branch, &opts);\n>>  }\n>>\n>> +/*\n>> + * Iterate through each worktree and invoke the callback function.  If\n>> + * the callback function returns non-zero, the iteration stops, and\n>> + * this function returns that return value\n>> + */\n>> +static int for_each_worktree(each_worktree_fn fn, void *cb_data)\n>\n> Ultimately, code outside of builtin/worktree.c will want to benefit\n> from the encapsulation of this worktree iteration logic. For instance,\n> the support code in (top-level) branch.c invoked by \"git worktree add\"\n> and \"git checkout\" to determine if a branch is already checked out in\n> some (other) branch could avail itself of this iteration interface.\n>\n> As such, it would make sense to place this code at location where it\n> can be accessed by callers other than just builtin/worktree.c. A good\n> location for it to reside might be in a new top-level file worktree.c.\n>\n> It doesn't have to be part of the current patch series, but,\n> eventually, the code in branch.c for determining if a branch is\n> already checked out elsewhere also should migrate to the new top-level\n> worktree.c; in particular, die_if_checked_out(), find_shared_symref(),\n> and find_linked_symref().\n>\n>> +{\n>> +       struct strbuf worktree_path = STRBUF_INIT;\n>> +       struct strbuf worktree_git = STRBUF_INIT;\n>> +       const char *common_dir;\n>> +       int main_is_bare = 0;\n>> +       int ret = 0;\n>> +\n>> +       common_dir = get_git_common_dir();\n>> +       if (!strcmp(common_dir, get_git_dir())) {\n>> +               /* simple case - this is the main repo */\n>> +               main_is_bare = is_bare_repository();\n>> +               if (!main_is_bare) {\n>> +                       strbuf_addstr(&worktree_path, get_git_work_tree());\n>> +               } else {\n>> +                       strbuf_addstr(&worktree_path, common_dir);\n>> +               }\n>> +       } else {\n>> +               strbuf_addstr(&worktree_path, common_dir);\n>> +               /* If the common_dir DOES NOT end with '/.git', then it is bare */\n>> +               main_is_bare = !strbuf_strip_suffix(&worktree_path, \"/.git\");\n>> +       }\n>> +       strbuf_addstr(&worktree_git, worktree_path.buf);\n>\n> I may have missed some discussion in earlier rounds (or perhaps I'm\n> too simple-minded), but I'm confused about why this logic (and most of\n> the rest of the function) differs so much from existing logic in\n> branch.c:find_shared_symref() and find_linked_symref() for iterating\n> over the worktrees and gleaning information about them. That logic in\n> branch.c seems to do a pretty good job of reporting the worktree in\n> which a branch is already checked out, so it's not clear why the above\n> logic takes a different (and seemingly more complex) approach.\n\nThis is due to my unfamiliarity of the code api.  I keep trying to\nlook for the right\nfunctions to use, but I miss them.  Sorry.  I will rework using those functions.\n\n>\n>> +       if (!main_is_bare) {\n>> +               strbuf_addstr(&worktree_git, \"/.git\");\n>> +\n>> +               ret = fn(worktree_path.buf, worktree_git.buf, cb_data);\n>> +               if (ret)\n>> +                       goto done;\n>> +       }\n>> +       strbuf_addstr(&worktree_git, \"/worktrees\");\n>> +\n>> +       if (is_directory(worktree_git.buf)) {\n>\n> As mentioned in my v2 review[1], this is_directory() invocation\n> doesn't buy you anything. The following opendir() will either succeed\n> or fail anyhow, so checking beforehand if 'worktree_git' is a\n> directory is wasted work. If you eliminate is_directory(), you avoid\n> that unnecessary work (and the rest of the code can be less deeply\n> indented).\n\nFor some reason, in my testing, calling opendir was giving a\nsegmentation fault regardless of how I checked the return value.\nIncluding the is_directory check allowed me to avoid that.\n\n>\n>> +               DIR *dir = opendir(worktree_git.buf);\n>> +               if (dir) {\n>> +                       struct stat st;\n>> +                       struct dirent *d;\n>> +                       size_t base_path_len = worktree_git.len;\n>> +\n>> +                       while ((d = readdir(dir)) != NULL) {\n>> +                               if (!strcmp(d->d_name, \".\") || !strcmp(d->d_name, \"..\"))\n>> +                                       continue;\n>> +\n>> +                               strbuf_setlen(&worktree_git, base_path_len);\n>> +                               strbuf_addf(&worktree_git, \"/%s/gitdir\", d->d_name);\n>> +\n>> +                               if (stat(worktree_git.buf, &st)) {\n>> +                                       fprintf(stderr, \"Unable to read worktree git dir: %s\\n\", worktree_git.buf);\n>> +                                       continue;\n>> +                               }\n>> +\n>> +                               strbuf_reset(&worktree_path);\n>> +                               strbuf_read_file(&worktree_path, worktree_git.buf, st.st_size);\n>> +                               strbuf_strip_suffix(&worktree_path, \"/.git\\n\");\n>> +\n>> +                               strbuf_strip_suffix(&worktree_git, \"/gitdir\");\n>> +                               ret = fn(worktree_path.buf, worktree_git.buf, cb_data);\n>> +                               if (ret)\n>> +                                       break;\n>> +                       }\n>> +               }\n>> +               closedir(dir);\n>\n> This closedir() should be inside the `if (dir) {...}' block rather than outside.\n>\n> Returning to the issue of this code differing from existing code in\n> branch.c for iterating worktrees, I'm curious as to why this\n> functionality was implemented again from scratch here rather than\n> re-using the existing (somewhat) well proven code and logic in\n> branch.c:find_shared_symref() and find_linked_symref(). My v2\n> review[1] hinted at re-use a couple times.\n>\n> Considering that the existing code in branch.c for iterating worktrees\n> has gone through several code reviews and is already (somewhat)\n> proven, re-using that code for a general purpose worktree iterator\n> makes lots of sense compared with writing it again from scratch, as is\n> done here. Consequently, I, personally, would be happier to see the\n> existing code refactored (perhaps over several patches) to the point\n> that it can be re-used as a general purpose iterator. But, that's my\n> opinion; I can't speak for others.\n>\n>> +       }\n>> +done:\n>> +       strbuf_release(&worktree_git);\n>> +       strbuf_release(&worktree_path);\n>> +       return ret;\n>> +}\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/275528\n> [2]: http://article.gmane.org/gmane.comp.version-control.git/276471\n"},{"id":"269027","messageId":"CAPig+cTwtKgm4U64nZhRY+F5HuQvKk1RLdyAsS6sJfYp85go2A@mail.gmail.com","threadId":"40222","inReplyTo":"1441046674.25570.15.camel@twopensource.com","subject":"Re: [PATCH v6 1/2] worktree: add 'for_each_worktree' function","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-31T19:03:05Z","receivedAt":"2015-08-31T19:03:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 31, 2015 at 2:44 PM, David Turner <dturner@twopensource.com> wrote:\n> On Mon, 2015-08-31 at 01:11 -0400, Eric Sunshine wrote:\n>> Stepping back a bit, is a for-each-foo()-style interface desirable?\n>> This sort of interface imposes a good deal of complexity on callers,\n>> demanding a callback function and callback data (cb_data), and is\n>> generally (at least in C) more difficult to reason about than other\n>> simpler interfaces. Is such complexity warranted?\n>>\n>> An alternate, much simpler interface would be to have a function, say\n>> get_worktrees(), return an array of 'worktree' structures to the\n>> caller, which the caller would iterate over (which is a common\n>> operation in C, thus easily reasoned about).\n>>\n>> The one benefit of a for-each-foo()-style interface is that it's\n>> possible to \"exit early\", thus avoiding the cost of interrogating\n>> meta-data for worktrees in which the caller is not interested,\n>> however, it seems unlikely that there will be so many worktrees linked\n>> to a repository for this early exit to translate into any real\n>> savings.\n>\n> The other benefit is that there is no need to worry about deallocating\n> the list.  But that might be too minor to worry about.\n\nProbably. The burden of having to deallocate the returned array seems\nquite minor compared to the complexity of the callback function\napproach.\n\nAlso, unstated but implied with the suggestion of a get_worktrees()\nfunction was that there would be a corresponding free_worktrees()\nfunction to make cleanup easy.\n"},{"id":"269033","messageId":"CAPig+cTFQNcBEJ7QwsLjd1ncmxsG+uZFBkc_uyHzwBkU7a7hMQ@mail.gmail.com","threadId":"40222","inReplyTo":"CANoM8SUoJAPBwEO=udhaR0WcUkZropEJ5G_SESnkHVtAQHxJQQ@mail.gmail.com","subject":"Re: [PATCH v6 1/2] worktree: add 'for_each_worktree' function","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-31T19:22:15Z","receivedAt":"2015-08-31T19:22:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 31, 2015 at 2:57 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:\n> On Mon, Aug 31, 2015 at 1:11 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> On Sun, Aug 30, 2015 at 3:10 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:\n>> Why does this iteration function specially filter out a bare main\n>> worktree? If it instead unconditionally vends the main worktree (bare\n>> or not), then the caller can make its own decision about what to do\n>> with it, thus empowering the caller, rather than imposing a (possibly)\n>> arbitrary restriction upon it.\n>>\n>> For instance, the \"git worktree list\" command may very well want to\n>> show the main worktree, even if bare (possibly controlled by a\n>> command-line option), annotated appropriately (\"[bare]\"). This may be\n>> exactly the sort of information a user wants to know, and by leaving\n>> the decision up to the caller, then the caller (\"git worktree list\" in\n>> this example) has the opportunity to act accordingly, whereas if\n>> for_each_worktree() filters out a bare main worktree unconditionally,\n>> then the caller (\"git worktree list\") will never be able to offer such\n>> an option.\n>\n> I wasn't sure that a bare repo would be considered a worktree.  I\n> don't think that it would be\n> a good idea to include it.  In the same vein that I can't checkout a\n> branch in a bare repo, it\n> figure that it shouldn't be in the list.\n\nThis is a mechanism vs. policy issue[1]. Low-level worker code, such\nas this iteration function, should concern itself only with the\nmechanics of retrieving and vending the worktree meta-data, and should\nnot make decisions (policy) about how that information is used by the\ncaller. Policy decisions (how the meta-data is used or displayed)\nshould be pushed to as high a level as possible, often up to the level\nof user-interface (which is what \"git worktree list\" is).\n\n[1]: http://www.catb.org/esr/writings/taoup/html/ch01s06.html#id2877777\n\n>> Stepping back a bit, is a for-each-foo()-style interface desirable?\n>> This sort of interface imposes a good deal of complexity on callers,\n>> demanding a callback function and callback data (cb_data), and is\n>> generally (at least in C) more difficult to reason about than other\n>> simpler interfaces. Is such complexity warranted?\n>>\n>> An alternate, much simpler interface would be to have a function, say\n>> get_worktrees(), return an array of 'worktree' structures to the\n>> caller, which the caller would iterate over (which is a common\n>> operation in C, thus easily reasoned about).\n>>\n>> The one benefit of a for-each-foo()-style interface is that it's\n>> possible to \"exit early\", thus avoiding the cost of interrogating\n>> meta-data for worktrees in which the caller is not interested,\n>> however, it seems unlikely that there will be so many worktrees linked\n>> to a repository for this early exit to translate into any real\n>> savings.\n>\n> I am not opposed to making a simple function as you describe.  I think David was\n> looking for a callback style function.  I don't think it would be\n> terrible to keep the\n> callback and then also include the simple function to return the\n> struct array.  I like\n> the memory management of the callback better than the struct array though.\n\nWe should stick with one or the other. Having both complicates the\ncode unnecessarily and increases maintenance costs. I, personally,\nprefer the get_worktrees() approach for its client-side simplicity.\nWith a corresponding free_worktrees() to dispose of the resources\nallocated by get_worktrees(), memory management shouldn't be much of a\nburden for callers (other than having to remember to call it).\n\n>> I may have missed some discussion in earlier rounds (or perhaps I'm\n>> too simple-minded), but I'm confused about why this logic (and most of\n>> the rest of the function) differs so much from existing logic in\n>> branch.c:find_shared_symref() and find_linked_symref() for iterating\n>> over the worktrees and gleaning information about them. That logic in\n>> branch.c seems to do a pretty good job of reporting the worktree in\n>> which a branch is already checked out, so it's not clear why the above\n>> logic takes a different (and seemingly more complex) approach.\n>\n> This is due to my unfamiliarity of the code api.  I keep trying to\n> look for the right\n> functions to use, but I miss them.  Sorry.  I will rework using those functions.\n\nThe API is indeed large and complex, and it can be difficult to get a\nhandle on how to accomplish various tasks. That's also a good argument\nfor re-using the existing (proven) code in\nbranch.c:find_shared_symref() and find_linked_symref() rather than\nre-inventing it from scratch. In its current form, the branch.c code\ndoesn't look much like a general-purpose iterator function, but it\nshould be possible to refactor if over the course of a few patches so\nthat it does satisfy that goal.\n\n>>> +       if (!main_is_bare) {\n>>> +               strbuf_addstr(&worktree_git, \"/.git\");\n>>> +\n>>> +               ret = fn(worktree_path.buf, worktree_git.buf, cb_data);\n>>> +               if (ret)\n>>> +                       goto done;\n>>> +       }\n>>> +       strbuf_addstr(&worktree_git, \"/worktrees\");\n>>> +\n>>> +       if (is_directory(worktree_git.buf)) {\n>>\n>> As mentioned in my v2 review[1], this is_directory() invocation\n>> doesn't buy you anything. The following opendir() will either succeed\n>> or fail anyhow, so checking beforehand if 'worktree_git' is a\n>> directory is wasted work. If you eliminate is_directory(), you avoid\n>> that unnecessary work (and the rest of the code can be less deeply\n>> indented).\n>\n> For some reason, in my testing, calling opendir was giving a\n> segmentation fault regardless of how I checked the return value.\n> Including the is_directory check allowed me to avoid that.\n\nWas it the opendir() that was crashing or the closedir()? Considering\nthat the closedir() incorrectly resides outside the `if (dir) {...}'\nblock, rather than inside, I could easily see the closedir() crashing\nwhen called with NULL.\n"},{"id":"269037","messageId":"CAPig+cSweTu=t4vekV6ihAYSzdDHZMQdx-PRt9_+rYr9SrujPw@mail.gmail.com","threadId":"40222","inReplyTo":"CANoM8SUoJAPBwEO=udhaR0WcUkZropEJ5G_SESnkHVtAQHxJQQ@mail.gmail.com","subject":"Re: [PATCH v6 1/2] worktree: add 'for_each_worktree' function","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-31T19:47:39Z","receivedAt":"2015-08-31T19:47:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 31, 2015 at 2:57 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:\n> I wasn't sure that a bare repo would be considered a worktree.  I\n> don't think that it would be\n> a good idea to include it.  In the same vein that I can't checkout a\n> branch in a bare repo, it\n> figure that it shouldn't be in the list.\n\nI forgot to mention in my previous response that I have the opposite\nview, and think that a bare repo should be included in the output of\n\"git worktree list\". The reason is that the intention of \"git worktree\nlist\" is to give the user a consolidated view of the locations of all\ncomponents of his \"workspace\". By \"workspace\", I mean the repository\n(bare or not) and its worktrees.\n\nIn the typical case, the .git directory resides within the main\nworktree (the first item output by \"git worktree list\"), thus is\neasily found, however, if \"git worktree list\" hides bare repos, then\nthe user will have no way to easily locate the repository (without\nresorting to lower-level commands or peeking at configuration files).\n"},{"id":"269038","messageId":"1441050857.25570.21.camel@twopensource.com","threadId":"40222","inReplyTo":"CAPig+cTwtKgm4U64nZhRY+F5HuQvKk1RLdyAsS6sJfYp85go2A@mail.gmail.com","subject":"Re: [PATCH v6 1/2] worktree: add 'for_each_worktree' function","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-08-31T19:54:17Z","receivedAt":"2015-08-31T19:54:17Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Mon, 2015-08-31 at 15:03 -0400, Eric Sunshine wrote:\n> On Mon, Aug 31, 2015 at 2:44 PM, David Turner <dturner@twopensource.com> wrote:\n> > On Mon, 2015-08-31 at 01:11 -0400, Eric Sunshine wrote:\n> >> Stepping back a bit, is a for-each-foo()-style interface desirable?\n> >> This sort of interface imposes a good deal of complexity on callers,\n> >> demanding a callback function and callback data (cb_data), and is\n> >> generally (at least in C) more difficult to reason about than other\n> >> simpler interfaces. Is such complexity warranted?\n> >>\n> >> An alternate, much simpler interface would be to have a function, say\n> >> get_worktrees(), return an array of 'worktree' structures to the\n> >> caller, which the caller would iterate over (which is a common\n> >> operation in C, thus easily reasoned about).\n> >>\n> >> The one benefit of a for-each-foo()-style interface is that it's\n> >> possible to \"exit early\", thus avoiding the cost of interrogating\n> >> meta-data for worktrees in which the caller is not interested,\n> >> however, it seems unlikely that there will be so many worktrees linked\n> >> to a repository for this early exit to translate into any real\n> >> savings.\n> >\n> > The other benefit is that there is no need to worry about deallocating\n> > the list.  But that might be too minor to worry about.\n> \n> Probably. The burden of having to deallocate the returned array seems\n> quite minor compared to the complexity of the callback function\n> approach.\n> \n> Also, unstated but implied with the suggestion of a get_worktrees()\n> function was that there would be a corresponding free_worktrees()\n> function to make cleanup easy.\n\nThat's fine with me.  Sorry for leading you down the wrong path,\nMichael.\n"},{"id":"269039","messageId":"CANoM8SUy_MGbLzaLrkaaNxbXYiisiu8cwnWuy6QDtGvw6HCf9w@mail.gmail.com","threadId":"40222","inReplyTo":"CAPig+cSweTu=t4vekV6ihAYSzdDHZMQdx-PRt9_+rYr9SrujPw@mail.gmail.com","subject":"Re: [PATCH v6 1/2] worktree: add 'for_each_worktree' function","fromName":"Mike Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2015-08-31T19:54:34Z","receivedAt":"2015-08-31T19:54:34Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"On Mon, Aug 31, 2015 at 3:47 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Mon, Aug 31, 2015 at 2:57 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:\n>> I wasn't sure that a bare repo would be considered a worktree.  I\n>> don't think that it would be\n>> a good idea to include it.  In the same vein that I can't checkout a\n>> branch in a bare repo, it\n>> figure that it shouldn't be in the list.\n>\n> I forgot to mention in my previous response that I have the opposite\n> view, and think that a bare repo should be included in the output of\n> \"git worktree list\". The reason is that the intention of \"git worktree\n> list\" is to give the user a consolidated view of the locations of all\n> components of his \"workspace\". By \"workspace\", I mean the repository\n> (bare or not) and its worktrees.\n>\n> In the typical case, the .git directory resides within the main\n> worktree (the first item output by \"git worktree list\"), thus is\n> easily found, however, if \"git worktree list\" hides bare repos, then\n> the user will have no way to easily locate the repository (without\n> resorting to lower-level commands or peeking at configuration files).\n\nThis makes sense, but would we also want to decorate it in the `git\nworktree list`\ncommand?  Would porcelain list output be able to differentiate it?\n"},{"id":"269058","messageId":"CAPig+cTV517TizuBUSd=NbF=-wvPtEpVLTeFVFivYLJ1dyNwLQ@mail.gmail.com","threadId":"40222","inReplyTo":"CANoM8SUy_MGbLzaLrkaaNxbXYiisiu8cwnWuy6QDtGvw6HCf9w@mail.gmail.com","subject":"Re: [PATCH v6 1/2] worktree: add 'for_each_worktree' function","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-31T21:37:02Z","receivedAt":"2015-08-31T21:37:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 31, 2015 at 3:54 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:\n> On Mon, Aug 31, 2015 at 3:47 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> On Mon, Aug 31, 2015 at 2:57 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:\n>>> I wasn't sure that a bare repo would be considered a worktree.  I\n>>> don't think that it would be\n>>> a good idea to include it.  In the same vein that I can't checkout a\n>>> branch in a bare repo, it\n>>> figure that it shouldn't be in the list.\n>>\n>> I forgot to mention in my previous response that I have the opposite\n>> view, and think that a bare repo should be included in the output of\n>> \"git worktree list\". The reason is that the intention of \"git worktree\n>> list\" is to give the user a consolidated view of the locations of all\n>> components of his \"workspace\". By \"workspace\", I mean the repository\n>> (bare or not) and its worktrees.\n>>\n>> In the typical case, the .git directory resides within the main\n>> worktree (the first item output by \"git worktree list\"), thus is\n>> easily found, however, if \"git worktree list\" hides bare repos, then\n>> the user will have no way to easily locate the repository (without\n>> resorting to lower-level commands or peeking at configuration files).\n>\n> This makes sense, but would we also want to decorate it in the `git\n> worktree list`\n> command?  Would porcelain list output be able to differentiate it?\n\nI don't have strong feelings about decorating the bare repository, if\nby \"decorate\", you mean adding a \"[bare]\" annotation or something.\nVerbose mode can certainly do so.\n\nFor porcelain mode, we can (and should) be explicit about it.\n"}]}