{"thread":{"id":"52057","subject":"[PATCH 1/2] Make die_if_checked_out() ignore missing worktree checkouts.","startedAt":"2019-10-17T16:28:39Z","lastAt":"2019-11-09T11:35:08Z","messageCount":15,"participants":["Peter Jones","SZEDER Gábor","Eric Sunshine","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"384268","messageId":"20191017162826.1064257-1-pjones@redhat.com","threadId":"52057","inReplyTo":null,"subject":"[PATCH 1/2] Make die_if_checked_out() ignore missing worktree checkouts.","fromName":"Peter Jones","fromEmail":"pjones@redhat.com","sentAt":"2019-10-17T16:28:25Z","receivedAt":"2019-10-17T16:28:39Z","isPatch":true,"sender":{"key":"pjones@redhat.com","avatar":"https://gravatar.com/avatar/a7ee1bf5628ca7f607facec51285b8328294c3f331d87410fe8558beda46f896?d=mp&s=160"},"body":"Currently if you do, for example:\n\n$ git worktree add path foo\n\nAnd \"foo\" has already been checked out at some other path, but the user\nhas removed it without pruning, you'll get an error that the branch is\nalready checked out.  It isn't meaningfully checked out, the repo's\ndata is just stale and no longer reflects reality.\n\nThis makes it so that if nothing is present where a worktree is\nsupposedly checked out, we ignore that the worktree exists, and let it\nget cleaned up the next time worktrees are pruned.\n\n(I would prune it instead, but prune isn't available from libgit\ncurrently.)\n\nSigned-off-by: Peter Jones <pjones@redhat.com>\n---\n branch.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/branch.c b/branch.c\nindex 579494738a7..60322ded953 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -360,6 +360,9 @@ void die_if_checked_out(const char *branch, int ignore_current_worktree)\n \twt = find_shared_symref(\"HEAD\", branch);\n \tif (!wt || (ignore_current_worktree && wt->is_current))\n \t\treturn;\n+\tif (access(wt->path, F_OK) < 0 &&\n+\t    (errno == ENOENT || errno == ENOTDIR))\n+\t\treturn;\n \tskip_prefix(branch, \"refs/heads/\", &branch);\n \tdie(_(\"'%s' is already checked out at '%s'\"),\n \t    branch, wt->path);\n-- \n2.23.0\n\n"},{"id":"384269","messageId":"20191017162826.1064257-2-pjones@redhat.com","threadId":"52057","inReplyTo":"20191017162826.1064257-1-pjones@redhat.com","subject":"[PATCH 2/2] Make \"git branch -d\" prune missing worktrees automatically.","fromName":"Peter Jones","fromEmail":"pjones@redhat.com","sentAt":"2019-10-17T16:28:26Z","receivedAt":"2019-10-17T16:28:42Z","isPatch":true,"sender":{"key":"pjones@redhat.com","avatar":"https://gravatar.com/avatar/a7ee1bf5628ca7f607facec51285b8328294c3f331d87410fe8558beda46f896?d=mp&s=160"},"body":"Currently, if you do:\n\n$ git branch zonk origin/master\n$ git worktree add zonk zonk\n$ rm -rf zonk\n$ git branch -d zonk\n\nYou get the following error:\n\n$ git branch -d zonk\nerror: Cannot delete branch 'zonk' checked out at '/home/pjones/devel/kernel.org/git/zonk'\n\nIt isn't meaningfully checked out, the repo's data is just stale and no\nlonger reflects reality.\n\nThis makes it so that if nothing is present where a worktree is\nsupposedly checked out, deleting the branch will automatically prune it.\n\nSigned-off-by: Peter Jones <pjones@redhat.com>\n---\n builtin/branch.c   |  2 +-\n builtin/worktree.c | 14 ++++++++++++++\n worktree.h         |  6 ++++++\n 3 files changed, 21 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 2ef214632f0..d611f8183b4 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -236,7 +236,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tif (kinds == FILTER_REFS_BRANCHES) {\n \t\t\tconst struct worktree *wt =\n \t\t\t\tfind_shared_symref(\"HEAD\", name);\n-\t\t\tif (wt) {\n+\t\t\tif (wt && prune_worktree_if_missing(wt) < 0) {\n \t\t\t\terror(_(\"Cannot delete branch '%s' \"\n \t\t\t\t\t\"checked out at '%s'\"),\n \t\t\t\t      bname.buf, wt->path);\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 4de44f579af..b3ad915c3c3 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -133,6 +133,20 @@ static int prune_worktree(const char *id, struct strbuf *reason)\n \treturn 0;\n }\n \n+int prune_worktree_if_missing(const struct worktree *wt)\n+{\n+\tstruct strbuf reason = STRBUF_INIT;\n+\n+\tif (access(wt->path, F_OK) >= 0 ||\n+\t    (errno != ENOENT && errno == ENOTDIR)) {\n+\t\terrno = EEXIST;\n+\t\treturn -1;\n+\t}\n+\n+\tstrbuf_addf(&reason, _(\"Removing worktrees/%s: worktree directory is not present\"), wt->id);\n+\treturn prune_worktree(wt->id, &reason);\n+}\n+\n static void prune_worktrees(void)\n {\n \tstruct strbuf reason = STRBUF_INIT;\ndiff --git a/worktree.h b/worktree.h\nindex caecc7a281c..75762c25752 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -132,4 +132,10 @@ void strbuf_worktree_ref(const struct worktree *wt,\n const char *worktree_ref(const struct worktree *wt,\n \t\t\t const char *refname);\n \n+/*\n+ * Prune a worktree if it is no longer present at the checked out location.\n+ * Returns < 0 if the checkout is there or if pruning fails.\n+ */\n+int prune_worktree_if_missing(const struct worktree *wt);\n+\n #endif\n-- \n2.23.0\n\n"},{"id":"384270","messageId":"20191017164426.GX29845@szeder.dev","threadId":"52057","inReplyTo":"20191017162826.1064257-1-pjones@redhat.com","subject":"Re: [PATCH 1/2] Make die_if_checked_out() ignore missing worktree checkouts.","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-10-17T16:44:26Z","receivedAt":"2019-10-17T16:44:33Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Oct 17, 2019 at 12:28:25PM -0400, Peter Jones wrote:\n> Currently if you do, for example:\n> \n> $ git worktree add path foo\n> \n> And \"foo\" has already been checked out at some other path, but the user\n> has removed it without pruning, you'll get an error that the branch is\n> already checked out.  It isn't meaningfully checked out, the repo's\n> data is just stale and no longer reflects reality.\n> \n> This makes it so that if nothing is present where a worktree is\n> supposedly checked out, we ignore that the worktree exists, and let it\n> get cleaned up the next time worktrees are pruned.\n> \n> (I would prune it instead, but prune isn't available from libgit\n> currently.)\n> \n> Signed-off-by: Peter Jones <pjones@redhat.com>\n> ---\n>  branch.c | 3 +++\n>  1 file changed, 3 insertions(+)\n> \n> diff --git a/branch.c b/branch.c\n> index 579494738a7..60322ded953 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -360,6 +360,9 @@ void die_if_checked_out(const char *branch, int ignore_current_worktree)\n>  \twt = find_shared_symref(\"HEAD\", branch);\n>  \tif (!wt || (ignore_current_worktree && wt->is_current))\n>  \t\treturn;\n> +\tif (access(wt->path, F_OK) < 0 &&\n> +\t    (errno == ENOENT || errno == ENOTDIR))\n> +\t\treturn;\n\nI think this check is insuffient: even if the directory of the working\ntree is not present, the working tree might still exist, and should\nnot be ignored (or deleted/pruned in the second patch).\n\nSee the description of 'git worktree lock' for details.\n\n>  \tskip_prefix(branch, \"refs/heads/\", &branch);\n>  \tdie(_(\"'%s' is already checked out at '%s'\"),\n>  \t    branch, wt->path);\n> -- \n> 2.23.0\n> \n"},{"id":"384271","messageId":"CAPig+cS6SzLdgmzffNkg72YSiDQ9eQRqTK12NsraKpGbkJFY_w@mail.gmail.com","threadId":"52057","inReplyTo":"20191017162826.1064257-2-pjones@redhat.com","subject":"Re: [PATCH 2/2] Make \"git branch -d\" prune missing worktrees automatically.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-10-17T17:28:09Z","receivedAt":"2019-10-17T17:28:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Oct 17, 2019 at 12:28 PM Peter Jones <pjones@redhat.com> wrote:\n> Currently, if you do:\n>\n> $ git branch zonk origin/master\n> $ git worktree add zonk zonk\n> $ rm -rf zonk\n> $ git branch -d zonk\n>\n> You get the following error:\n>\n> $ git branch -d zonk\n> error: Cannot delete branch 'zonk' checked out at '/home/pjones/devel/kernel.org/git/zonk'\n>\n> It isn't meaningfully checked out, the repo's data is just stale and no\n> longer reflects reality.\n\nEchoing SEZDER's comment on patch 1/2, this behavior is an intentional\ndesign choice and safety feature of the worktree implementation since\nworktrees may exist on removable media or remote filesystems which\nmight not always be mounted; hence, the presence of commands \"git\nworktree prune\" and \"git worktree remove\".\n\nA couple comment regarding this patch...\n\n> Signed-off-by: Peter Jones <pjones@redhat.com>\n> ---\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> @@ -133,6 +133,20 @@ static int prune_worktree(const char *id, struct strbuf *reason)\n> +int prune_worktree_if_missing(const struct worktree *wt)\n> +{\n> +       struct strbuf reason = STRBUF_INIT;\n> +\n> +       if (access(wt->path, F_OK) >= 0 ||\n> +           (errno != ENOENT && errno == ENOTDIR)) {\n> +               errno = EEXIST;\n> +               return -1;\n> +       }\n> +\n> +       strbuf_addf(&reason, _(\"Removing worktrees/%s: worktree directory is not present\"), wt->id);\n> +       return prune_worktree(wt->id, &reason);\n> +}\n\n\"git worktree\" tries to clean up after itself as much as possible. For\ninstance, it is careful to remove the .git/worktrees directory when\nthe last worktree itself is removed (or pruned). So, the caller of\nthis function would also want to call delete_worktrees_dir_if_empty()\nto follow suit.\n\n> diff --git a/worktree.h b/worktree.h\n> @@ -132,4 +132,10 @@ void strbuf_worktree_ref(const struct worktree *wt,\n> +/*\n> + * Prune a worktree if it is no longer present at the checked out location.\n> + * Returns < 0 if the checkout is there or if pruning fails.\n> + */\n> +int prune_worktree_if_missing(const struct worktree *wt);\n\nIt's rather ugly that this function is declared in top-level\nworktree.h whereas the actual implementation is in builtin/worktree.c.\nI'd expect to see a preparatory patch which moves prune_worktree()\n(and probably delete_worktrees_dir_if_empty()) to top-level\nworktree.c.\n\nThese minor implementation comments aside, before considering this\npatch series, it would be nice to see a compelling argument as to why\nthis change of behavior, which undercuts a deliberate design decision,\nis really desirable.\n"},{"id":"384402","messageId":"20191018194317.wvqphshpkfskvkyh@redhat.com","threadId":"52057","inReplyTo":"CAPig+cS6SzLdgmzffNkg72YSiDQ9eQRqTK12NsraKpGbkJFY_w@mail.gmail.com","subject":"Re: [PATCH 2/2] Make \"git branch -d\" prune missing worktrees automatically.","fromName":"Peter Jones","fromEmail":"pjones@redhat.com","sentAt":"2019-10-18T19:43:19Z","receivedAt":"2019-10-18T19:43:23Z","isPatch":true,"sender":{"key":"pjones@redhat.com","avatar":"https://gravatar.com/avatar/a7ee1bf5628ca7f607facec51285b8328294c3f331d87410fe8558beda46f896?d=mp&s=160"},"body":"On Thu, Oct 17, 2019 at 06:44:26PM +0200, SZEDER Gábor wrote:\n> >  \tif (!wt || (ignore_current_worktree && wt->is_current))\n> >  \t\treturn;\n> > +\tif (access(wt->path, F_OK) < 0 &&\n> > +\t    (errno == ENOENT || errno == ENOTDIR))\n> > +\t\treturn;\n> \n> I think this check is insuffient: even if the directory of the working\n> tree is not present, the working tree might still exist, and should\n> not be ignored (or deleted/pruned in the second patch).\n> \n> See the description of 'git worktree lock' for details.\n\nAh, thanks for that, I had not realized \"lock\" was relevant here as I\nhave never used it.  That explains some of what seemed to me like a very\nstrange usage model.\n\nOn Thu, Oct 17, 2019 at 01:28:09PM -0400, Eric Sunshine wrote:\n> Echoing SEZDER's comment on patch 1/2, this behavior is an intentional\n> design choice and safety feature of the worktree implementation since\n> worktrees may exist on removable media or remote filesystems which\n> might not always be mounted; hence, the presence of commands \"git\n> worktree prune\" and \"git worktree remove\".\n\nOkay, I see that use case now - I hadn't realized there was an\nintentional design decision here, and honestly that's anything but clear\nfrom the *code*.  It's surprising, for example, that my patches didn't\nbreak a single test case.\n\n> A couple comment regarding this patch...\n\nSure...\n\n> > diff --git a/builtin/worktree.c b/builtin/worktree.c\n> > @@ -133,6 +133,20 @@ static int prune_worktree(const char *id, struct strbuf *reason)\n> > +int prune_worktree_if_missing(const struct worktree *wt)\n> > +{\n> > +       struct strbuf reason = STRBUF_INIT;\n> > +\n> > +       if (access(wt->path, F_OK) >= 0 ||\n> > +           (errno != ENOENT && errno == ENOTDIR)) {\n> > +               errno = EEXIST;\n> > +               return -1;\n> > +       }\n> > +\n> > +       strbuf_addf(&reason, _(\"Removing worktrees/%s: worktree directory is not present\"), wt->id);\n> > +       return prune_worktree(wt->id, &reason);\n> > +}\n> \n> \"git worktree\" tries to clean up after itself as much as possible. For\n> instance, it is careful to remove the .git/worktrees directory when\n> the last worktree itself is removed (or pruned). So, the caller of\n> this function would also want to call delete_worktrees_dir_if_empty()\n> to follow suit.\n\nOkay, will fix.\n\n> > diff --git a/worktree.h b/worktree.h\n> > @@ -132,4 +132,10 @@ void strbuf_worktree_ref(const struct worktree *wt,\n> > +/*\n> > + * Prune a worktree if it is no longer present at the checked out location.\n> > + * Returns < 0 if the checkout is there or if pruning fails.\n> > + */\n> > +int prune_worktree_if_missing(const struct worktree *wt);\n> \n> It's rather ugly that this function is declared in top-level\n> worktree.h whereas the actual implementation is in builtin/worktree.c.\n\nI don't disagree, but I didn't want to move stuff into an exposed API if\nI didn't have to, and that seemed like an appropriate enough header.  I\ncan do it the other way though, no problem.\n\n> I'd expect to see a preparatory patch which moves prune_worktree()\n> (and probably delete_worktrees_dir_if_empty()) to top-level\n> worktree.c.\n\nSure thing.\n\n> These minor implementation comments aside, before considering this\n> patch series, it would be nice to see a compelling argument as to why\n> this change of behavior, which undercuts a deliberate design decision,\n> is really desirable.\n\nOkay, so just for clarity, when you say there's a deliberate design\ndecision, which behavior here are you talking about?  If you mean making\n\"lock\" work, I don't have any issue with that.  If you mean not cleaning\nup when we do other commands, then I don't see why that's a concern -\nafter all, that's exactly what \"lock\" is for.\n\nAssuming it is the \"lock\" behavior we're talking about, I don't think I\nactually have any intention of breaking this design decision, just\nmaking my workflow (without \"lock\") nag at me less for what seem like\npretty trivial issues.\n\nI can easily accommodate \"git worktree lock\".  What bugs me though, is\nthat using worktrees basically means I have to replace fairly regular\nfilesystem activities with worktree commands, and it doesn't seem to be\n*necessary* in any way.  And I'm going to forget.  A lot.\n\nTo me, there doesn't seem to be any reason these need to behave any different:\n\n$ git worktree add foo foo\n$ rm -rf foo\nvs\n$ git worktree add foo foo\n$ git worktree remove foo\n\nAnd in fact the only difference right now, aside from some very\nminuscule storage requirements that haven't gotten cleaned up, is the\nfirst one leaves an artifact that tells it to give me errors later until\nI run \"git worktree prune\" myself.  \n\nI'll send another revision of this patchset as a reply to this mail,\nwhich should clear up some of our differences.\n\n-- \n  Peter\n"},{"id":"384403","messageId":"20191018194542.1316981-1-pjones@redhat.com","threadId":"52057","inReplyTo":"20191018194317.wvqphshpkfskvkyh@redhat.com","subject":"[PATCH v2 1/4] libgit: Add a read-only helper to test the worktree lock","fromName":"Peter Jones","fromEmail":"pjones@redhat.com","sentAt":"2019-10-18T19:45:39Z","receivedAt":"2019-10-18T19:46:03Z","isPatch":true,"sender":{"key":"pjones@redhat.com","avatar":"https://gravatar.com/avatar/a7ee1bf5628ca7f607facec51285b8328294c3f331d87410fe8558beda46f896?d=mp&s=160"},"body":"Add the function is_worktree_locked(), which is a helper to tell if a\nworktree is locked without having to be able to modify it.\n\nSigned-off-by: Peter Jones <pjones@redhat.com>\n---\n builtin/worktree.c |  2 +-\n worktree.c         | 16 ++++++++++++++++\n worktree.h         |  5 +++++\n 3 files changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 4de44f579af..86305cc1fe1 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -245,7 +245,7 @@ static void validate_worktree_add(const char *path, const struct add_opts *opts)\n \tif (!wt)\n \t\tgoto done;\n \n-\tlocked = !!worktree_lock_reason(wt);\n+\tlocked = is_worktree_locked(wt);\n \tif ((!locked && opts->force) || (locked && opts->force > 1)) {\n \t\tif (delete_git_dir(wt->id))\n \t\t    die(_(\"unable to re-add worktree '%s'\"), path);\ndiff --git a/worktree.c b/worktree.c\nindex 5b4793caa34..4924805c389 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -244,6 +244,22 @@ int is_main_worktree(const struct worktree *wt)\n \treturn !wt->id;\n }\n \n+int is_worktree_locked(const struct worktree *wt)\n+{\n+\tstruct strbuf path = STRBUF_INIT;\n+\tint locked = 0;\n+\n+\tif (wt->lock_reason_valid && wt->lock_reason)\n+\t\treturn 1;\n+\n+\tstrbuf_addstr(&path, worktree_git_path(wt, \"locked\"));\n+\tif (file_exists(path.buf))\n+\t\tlocked = 1;\n+\n+\tstrbuf_release(&path);\n+\treturn locked;\n+}\n+\n const char *worktree_lock_reason(struct worktree *wt)\n {\n \tassert(!is_main_worktree(wt));\ndiff --git a/worktree.h b/worktree.h\nindex caecc7a281c..5ff16c414b5 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -56,6 +56,11 @@ struct worktree *find_worktree(struct worktree **list,\n  */\n int is_main_worktree(const struct worktree *wt);\n \n+/*\n+ * Return true if the given worktree is locked\n+ */\n+int is_worktree_locked(const struct worktree *wt);\n+\n /*\n  * Return the reason string if the given worktree is locked or NULL\n  * otherwise.\n-- \n2.23.0\n\n"},{"id":"384404","messageId":"20191018194542.1316981-2-pjones@redhat.com","threadId":"52057","inReplyTo":"20191018194542.1316981-1-pjones@redhat.com","subject":"[PATCH v2 2/4] libgit: Expose more worktree functionality.","fromName":"Peter Jones","fromEmail":"pjones@redhat.com","sentAt":"2019-10-18T19:45:40Z","receivedAt":"2019-10-18T19:46:06Z","isPatch":true,"sender":{"key":"pjones@redhat.com","avatar":"https://gravatar.com/avatar/a7ee1bf5628ca7f607facec51285b8328294c3f331d87410fe8558beda46f896?d=mp&s=160"},"body":"Add delete_worktrees_dir_if_empty() and prune_worktree() to the public\nAPI, so they can be used from more places.  Also add a new function,\nprune_worktree_if_missing(), which prunes unlocked worktrees if they\naren't present on the filesystem.\n\nSigned-off-by: Peter Jones <pjones@redhat.com>\n---\n builtin/worktree.c | 73 +-------------------------------------\n worktree.c         | 88 ++++++++++++++++++++++++++++++++++++++++++++++\n worktree.h         | 19 ++++++++++\n 3 files changed, 108 insertions(+), 72 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 86305cc1fe1..8ff37309be9 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -62,77 +62,6 @@ static int delete_git_dir(const char *id)\n \treturn ret;\n }\n \n-static void delete_worktrees_dir_if_empty(void)\n-{\n-\trmdir(git_path(\"worktrees\")); /* ignore failed removal */\n-}\n-\n-static int prune_worktree(const char *id, struct strbuf *reason)\n-{\n-\tstruct stat st;\n-\tchar *path;\n-\tint fd;\n-\tsize_t len;\n-\tssize_t read_result;\n-\n-\tif (!is_directory(git_path(\"worktrees/%s\", id))) {\n-\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: not a valid directory\"), id);\n-\t\treturn 1;\n-\t}\n-\tif (file_exists(git_path(\"worktrees/%s/locked\", id)))\n-\t\treturn 0;\n-\tif (stat(git_path(\"worktrees/%s/gitdir\", id), &st)) {\n-\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: gitdir file does not exist\"), id);\n-\t\treturn 1;\n-\t}\n-\tfd = open(git_path(\"worktrees/%s/gitdir\", id), O_RDONLY);\n-\tif (fd < 0) {\n-\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: unable to read gitdir file (%s)\"),\n-\t\t\t    id, strerror(errno));\n-\t\treturn 1;\n-\t}\n-\tlen = xsize_t(st.st_size);\n-\tpath = xmallocz(len);\n-\n-\tread_result = read_in_full(fd, path, len);\n-\tif (read_result < 0) {\n-\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: unable to read gitdir file (%s)\"),\n-\t\t\t    id, strerror(errno));\n-\t\tclose(fd);\n-\t\tfree(path);\n-\t\treturn 1;\n-\t}\n-\tclose(fd);\n-\n-\tif (read_result != len) {\n-\t\tstrbuf_addf(reason,\n-\t\t\t    _(\"Removing worktrees/%s: short read (expected %\"PRIuMAX\" bytes, read %\"PRIuMAX\")\"),\n-\t\t\t    id, (uintmax_t)len, (uintmax_t)read_result);\n-\t\tfree(path);\n-\t\treturn 1;\n-\t}\n-\twhile (len && (path[len - 1] == '\\n' || path[len - 1] == '\\r'))\n-\t\tlen--;\n-\tif (!len) {\n-\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: invalid gitdir file\"), id);\n-\t\tfree(path);\n-\t\treturn 1;\n-\t}\n-\tpath[len] = '\\0';\n-\tif (!file_exists(path)) {\n-\t\tfree(path);\n-\t\tif (stat(git_path(\"worktrees/%s/index\", id), &st) ||\n-\t\t    st.st_mtime <= expire) {\n-\t\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: gitdir file points to non-existent location\"), id);\n-\t\t\treturn 1;\n-\t\t} else {\n-\t\t\treturn 0;\n-\t\t}\n-\t}\n-\tfree(path);\n-\treturn 0;\n-}\n-\n static void prune_worktrees(void)\n {\n \tstruct strbuf reason = STRBUF_INIT;\n@@ -144,7 +73,7 @@ static void prune_worktrees(void)\n \t\tif (is_dot_or_dotdot(d->d_name))\n \t\t\tcontinue;\n \t\tstrbuf_reset(&reason);\n-\t\tif (!prune_worktree(d->d_name, &reason))\n+\t\tif (!prune_worktree(d->d_name, &reason, expire))\n \t\t\tcontinue;\n \t\tif (show_only || verbose)\n \t\t\tprintf(\"%s\\n\", reason.buf);\ndiff --git a/worktree.c b/worktree.c\nindex 4924805c389..08454a4e65d 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -608,3 +608,91 @@ int other_head_refs(each_ref_fn fn, void *cb_data)\n \tfree_worktrees(worktrees);\n \treturn ret;\n }\n+\n+void delete_worktrees_dir_if_empty(void)\n+{\n+\trmdir(git_path(\"worktrees\")); /* ignore failed removal */\n+}\n+\n+int prune_worktree(const char *id, struct strbuf *reason, timestamp_t expire)\n+{\n+\tstruct stat st;\n+\tchar *path;\n+\tint fd;\n+\tsize_t len;\n+\tssize_t read_result;\n+\n+\tif (!is_directory(git_path(\"worktrees/%s\", id))) {\n+\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: not a valid directory\"), id);\n+\t\treturn 1;\n+\t}\n+\tif (file_exists(git_path(\"worktrees/%s/locked\", id)))\n+\t\treturn 0;\n+\tif (stat(git_path(\"worktrees/%s/gitdir\", id), &st)) {\n+\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: gitdir file does not exist\"), id);\n+\t\treturn 1;\n+\t}\n+\tfd = open(git_path(\"worktrees/%s/gitdir\", id), O_RDONLY);\n+\tif (fd < 0) {\n+\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: unable to read gitdir file (%s)\"),\n+\t\t\t    id, strerror(errno));\n+\t\treturn 1;\n+\t}\n+\tlen = xsize_t(st.st_size);\n+\tpath = xmallocz(len);\n+\n+\tread_result = read_in_full(fd, path, len);\n+\tif (read_result < 0) {\n+\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: unable to read gitdir file (%s)\"),\n+\t\t\t    id, strerror(errno));\n+\t\tclose(fd);\n+\t\tfree(path);\n+\t\treturn 1;\n+\t}\n+\tclose(fd);\n+\n+\tif (read_result != len) {\n+\t\tstrbuf_addf(reason,\n+\t\t\t    _(\"Removing worktrees/%s: short read (expected %\"PRIuMAX\" bytes, read %\"PRIuMAX\")\"),\n+\t\t\t    id, (uintmax_t)len, (uintmax_t)read_result);\n+\t\tfree(path);\n+\t\treturn 1;\n+\t}\n+\twhile (len && (path[len - 1] == '\\n' || path[len - 1] == '\\r'))\n+\t\tlen--;\n+\tif (!len) {\n+\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: invalid gitdir file\"), id);\n+\t\tfree(path);\n+\t\treturn 1;\n+\t}\n+\tpath[len] = '\\0';\n+\tif (!file_exists(path)) {\n+\t\tfree(path);\n+\t\tif (stat(git_path(\"worktrees/%s/index\", id), &st) ||\n+\t\t    st.st_mtime <= expire) {\n+\t\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: gitdir file points to non-existent location\"), id);\n+\t\t\treturn 1;\n+\t\t} else {\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n+\tfree(path);\n+\treturn 0;\n+}\n+\n+int prune_worktree_if_missing(const struct worktree *wt)\n+{\n+\tstruct strbuf reason = STRBUF_INIT;\n+\tint ret;\n+\n+\tif (is_worktree_locked(wt) ||\n+\t    access(wt->path, F_OK) >= 0 ||\n+\t    (errno != ENOENT && errno == ENOTDIR)) {\n+\t\terrno = EEXIST;\n+\t\treturn -1;\n+\t}\n+\n+\tstrbuf_addf(&reason, _(\"Removing worktrees/%s: worktree directory is not present\"), wt->id);\n+\tret = prune_worktree(wt->id, &reason, TIME_MAX);\n+\treturn ret;\n+}\ndiff --git a/worktree.h b/worktree.h\nindex 5ff16c414b5..636bbb1c449 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -137,4 +137,23 @@ void strbuf_worktree_ref(const struct worktree *wt,\n const char *worktree_ref(const struct worktree *wt,\n \t\t\t const char *refname);\n \n+/*\n+ * Clean up the 'worktrees' directory, if necessary.\n+ */\n+void delete_worktrees_dir_if_empty(void);\n+\n+/*\n+ * Prune a worktree if it's older than expire.\n+ * Returns 0 on success, < 0 on failure.\n+ */\n+int prune_worktree(const char *id, struct strbuf *reason, timestamp_t expire);\n+\n+/*\n+ * Prune a worktree if it is not locked and is no longer present at the\n+ * checked out location.\n+ * Returns < 0 if the checkout is there, if the worktree is locked, or if\n+ * pruning fails.\n+ */\n+int prune_worktree_if_missing(const struct worktree *wt);\n+\n #endif\n-- \n2.23.0\n\n"},{"id":"384405","messageId":"20191018194542.1316981-4-pjones@redhat.com","threadId":"52057","inReplyTo":"20191018194542.1316981-1-pjones@redhat.com","subject":"[PATCH v2 4/4] Make \"git branch -d\" prune missing worktrees automatically.","fromName":"Peter Jones","fromEmail":"pjones@redhat.com","sentAt":"2019-10-18T19:45:42Z","receivedAt":"2019-10-18T19:46:06Z","isPatch":true,"sender":{"key":"pjones@redhat.com","avatar":"https://gravatar.com/avatar/a7ee1bf5628ca7f607facec51285b8328294c3f331d87410fe8558beda46f896?d=mp&s=160"},"body":"Currently, if you do:\n\n$ git branch zonk origin/master\n$ git worktree add zonk zonk\n$ rm -rf zonk\n$ git branch -d zonk\n\nYou get the following error:\n\n$ git branch -d zonk\nerror: Cannot delete branch 'zonk' checked out at '/home/pjones/devel/kernel.org/git/zonk'\n\nIt isn't meaningfully checked out, the repo's data is just stale and no\nlonger reflects reality.\n\nThis makes it so that if nothing is present where a worktree is\nsupposedly checked out, deleting the branch will automatically prune it.\n\nSigned-off-by: Peter Jones <pjones@redhat.com>\n---\n builtin/branch.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 2ef214632f0..a2a1e89c66b 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -236,13 +236,17 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tif (kinds == FILTER_REFS_BRANCHES) {\n \t\t\tconst struct worktree *wt =\n \t\t\t\tfind_shared_symref(\"HEAD\", name);\n-\t\t\tif (wt) {\n+\t\t\tint rc = -1;\n+\n+\t\t\tif (wt && (rc = prune_worktree_if_missing(wt)) < 0) {\n \t\t\t\terror(_(\"Cannot delete branch '%s' \"\n \t\t\t\t\t\"checked out at '%s'\"),\n \t\t\t\t      bname.buf, wt->path);\n \t\t\t\tret = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (rc >= 0)\n+\t\t\t\tdelete_worktrees_dir_if_empty();\n \t\t}\n \n \t\ttarget = resolve_refdup(name,\n-- \n2.23.0\n\n"},{"id":"384406","messageId":"20191018194542.1316981-3-pjones@redhat.com","threadId":"52057","inReplyTo":"20191018194542.1316981-1-pjones@redhat.com","subject":"[PATCH v2 3/4] Make die_if_checked_out() prune missing checkouts of unlocked worktrees.","fromName":"Peter Jones","fromEmail":"pjones@redhat.com","sentAt":"2019-10-18T19:45:41Z","receivedAt":"2019-10-18T19:46:07Z","isPatch":true,"sender":{"key":"pjones@redhat.com","avatar":"https://gravatar.com/avatar/a7ee1bf5628ca7f607facec51285b8328294c3f331d87410fe8558beda46f896?d=mp&s=160"},"body":"Currently if you do, for example:\n\n$ git worktree add path foo\n\nAnd \"foo\" has already been checked out at some other path, but the user\nhas removed it without pruning, and the worktree is not locked, you'll\nget an error that the branch is already checked out.  It isn't\nmeaningfully checked out, the repo's data is just stale and no longer\nreflects reality.\n\nThis makes it so that if nothing is present where a worktree is\nsupposedly checked out, and it is not locked, we ignore that the\nworktree exists, then it should be cleaned up.\n\nSigned-off-by: Peter Jones <pjones@redhat.com>\n---\n branch.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/branch.c b/branch.c\nindex 579494738a7..760ef387144 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -360,6 +360,12 @@ void die_if_checked_out(const char *branch, int ignore_current_worktree)\n \twt = find_shared_symref(\"HEAD\", branch);\n \tif (!wt || (ignore_current_worktree && wt->is_current))\n \t\treturn;\n+\n+\tif (prune_worktree_if_missing(wt) >= 0) {\n+\t\tdelete_worktrees_dir_if_empty();\n+\t\treturn;\n+\t}\n+\n \tskip_prefix(branch, \"refs/heads/\", &branch);\n \tdie(_(\"'%s' is already checked out at '%s'\"),\n \t    branch, wt->path);\n-- \n2.23.0\n\n"},{"id":"384483","messageId":"xmqqwocynam1.fsf@gitster-ct.c.googlers.com","threadId":"52057","inReplyTo":"20191018194542.1316981-1-pjones@redhat.com","subject":"Re: [PATCH v2 1/4] libgit: Add a read-only helper to test the worktree lock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-21T01:36:06Z","receivedAt":"2019-10-21T01:38:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Jones <pjones@redhat.com> writes:\n\n> Subject: Re: [PATCH v2 1/4] libgit: Add a read-only helper to test the worktree lock\n\nHaving a word \"worktree\" somewhere on the title is good, but have it\nas the \"I am changing this area\"; \"libgit\" does not give readers the\nhint that this is a step about the worktree subsystem.\n\n    Subject: [PATCH v2 1/4] worktree: add is_worktree_locked() helper\n\nWhen the new helper function is properly named, like yours, there is\nnot much need to explain what it does (i.e. \"to test the worktree\nlock\"), so just \"worktree: add is_worktree_locked()\" is sufficient.\n\n> Add the function is_worktree_locked(), which is a helper to tell if a\n> worktree is locked without having to be able to modify it.\n\nI do not see the reason why your proposed title and log message\nstress the fact that this helper can be used even by callers that\nare not permitted to modify the worktree (i.e. the emphasis on\n\"read-only\").  Asking for worktree_lock_reason() can be done by\nanybody, but I do not think we particularly advertise it as\nread-only.\n\nPerhaps drop \"without having to...\"?\n\n> -\tlocked = !!worktree_lock_reason(wt);\n> +\tlocked = is_worktree_locked(wt);\n>  \tif ((!locked && opts->force) || (locked && opts->force > 1)) {\n>  \t\tif (delete_git_dir(wt->id))\n>  \t\t    die(_(\"unable to re-add worktree '%s'\"), path);\n> diff --git a/worktree.c b/worktree.c\n> index 5b4793caa34..4924805c389 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -244,6 +244,22 @@ int is_main_worktree(const struct worktree *wt)\n>  \treturn !wt->id;\n>  }\n>  \n> +int is_worktree_locked(const struct worktree *wt)\n> +{\n> +\tstruct strbuf path = STRBUF_INIT;\n> +\tint locked = 0;\n> +\n> +\tif (wt->lock_reason_valid && wt->lock_reason)\n> +\t\treturn 1;\n> +\n> +\tstrbuf_addstr(&path, worktree_git_path(wt, \"locked\"));\n> +\tif (file_exists(path.buf))\n> +\t\tlocked = 1;\n\nIf you write\n\n\tlocked = file_exists(path.buf);\n\nhere, then readers do not have to scan backwards and find that the\nvariable is initialized to zero, and that no other statement since\nits initialization touches its value, in order to see what value is\nreturned when file does not exist.  Writing the RHS !!file_exists()\nconcisely allows readers to tell that this function returns only 0\nor 1 without having to check what file_exists() returns, but that\nmay probably be overkill.\n\n> +\tstrbuf_release(&path);\n> +\treturn locked;\n> +}\n\nI wondered why this is not just\n\n\t#define is_worktree_locked(wt) (!!worktree_lock_reason(wt))\n\nThere are a few differences compared to worktree_lock_reason():\n\n - this can be called on the main worktree by mistake and would\n   probably yield \"not locked\" (but the existing guard is a mere\n   assert() which probably is stripped away in production builds)\n\n - this can be used by a process that cannot even read the contents\n   of the locked file for the reason;\n\n - because reason is not read, reason or reason_valid fields are not\n   updated, and repeated calls on the same worktree structure would\n   result in repeated lstat() calls.\n\nShouldn't we be advising the callers that the last one as a\npotential downside?  The fact that the new helper is usable even by\nread-only callers hints that any caching of earlier results is\ndisabled, but it is somewhat a round-about way to say so.\n\nAs I do not see why being able to take \"const struct worktree *\", as\nopposed to non-const version is a huge advantage, for this helper, I\nwonder if it would make even more sense to introduce one more level\nto \"lock-reason-valid\" and allow caching of is_worktree_locked().\n\nCurrently, \"lock-reason-valid\" only tells us \"lock-reason may be\nNULL, but that does not necessarily mean it is not locked---you have\nto check it\" boolean, but it could be instead a tristate:\n\n    A: lock-reason may be NULL but that is only because we haven't\n       even tried to see if the lock file exists\n\n    B: NULL-ness of lock-reason reliably tells if the worktree is\n       locked or not because we have tried file_exists(), but if the\n       field has non-NULL value, that is *not* the string we read;\n       if you want to know the reason, you must read the file.\n\n    C: NULL in lock-reason means it is not locked; non-NULL in\n       lock-reason is what we read form the file.\n\nAlso, it may make sense to correct the first difference and in a\nmore meaningful way than assert(), given that the reason why this\nhelper is introduced is eventually to perform an destructive action\nlater in the series.  Perhaps\n\n\tif (is_main_worktree(wt))\n\t\tBUG(\"is-worktree-locked called for the main worktree\");\n\nat the front.\n\nThanks.\n\n>  const char *worktree_lock_reason(struct worktree *wt)\n>  {\n>  \tassert(!is_main_worktree(wt));\n> diff --git a/worktree.h b/worktree.h\n> index caecc7a281c..5ff16c414b5 100644\n> --- a/worktree.h\n> +++ b/worktree.h\n> @@ -56,6 +56,11 @@ struct worktree *find_worktree(struct worktree **list,\n>   */\n>  int is_main_worktree(const struct worktree *wt);\n>  \n> +/*\n> + * Return true if the given worktree is locked\n> + */\n> +int is_worktree_locked(const struct worktree *wt);\n> +\n>  /*\n>   * Return the reason string if the given worktree is locked or NULL\n>   * otherwise.\n"},{"id":"384484","messageId":"xmqqpniqn9ju.fsf@gitster-ct.c.googlers.com","threadId":"52057","inReplyTo":"20191018194542.1316981-2-pjones@redhat.com","subject":"Re: [PATCH v2 2/4] libgit: Expose more worktree functionality.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-21T01:59:01Z","receivedAt":"2019-10-21T01:59:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Jones <pjones@redhat.com> writes:\n\nSame comment on the commit title as 1/4; also, we tend not to upcase\nthe first word after the <area>: word and omit the full-stop on the\ntitle (see \"git shortlog -32 --no-merges\" on our project for\nexamples).\n\n> Add delete_worktrees_dir_if_empty() and prune_worktree() to the public\n> API, so they can be used from more places.  Also add a new function,\n> prune_worktree_if_missing(), which prunes unlocked worktrees if they\n> aren't present on the filesystem.\n\nIt probably is cleaner to do the \"also\" part as a separate step, as\nthat allows readers to skip this step without reading it deeply, but\nlet's see how it is done.\n\n> @@ -144,7 +73,7 @@ static void prune_worktrees(void)\n>  \t\tif (is_dot_or_dotdot(d->d_name))\n>  \t\t\tcontinue;\n>  \t\tstrbuf_reset(&reason);\n> -\t\tif (!prune_worktree(d->d_name, &reason))\n> +\t\tif (!prune_worktree(d->d_name, &reason, expire))\n>  \t\t\tcontinue;\n>  \t\tif (show_only || verbose)\n>  \t\t\tprintf(\"%s\\n\", reason.buf);\n> diff --git a/worktree.c b/worktree.c\n> index 4924805c389..08454a4e65d 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -608,3 +608,91 @@ int other_head_refs(each_ref_fn fn, void *cb_data)\n> +int prune_worktree(const char *id, struct strbuf *reason, timestamp_t expire)\n\nThis is not a mere code movement, because the original relied on the\nfile-scope static \"expire\", and the public version wants to give\ncallers control over the expiration value.  That is a good change\nthat deserves to be advertised and explained in the proposed log\nmessage.\n\n> +int prune_worktree_if_missing(const struct worktree *wt)\n> +{\n> +\tstruct strbuf reason = STRBUF_INIT;\n> +\tint ret;\n> +\n> +\tif (is_worktree_locked(wt) ||\n> +\t    access(wt->path, F_OK) >= 0 ||\n> +\t    (errno != ENOENT && errno == ENOTDIR)) {\n> +\t\terrno = EEXIST;\n> +\t\treturn -1;\n> +\t}\n\nWhen access() failed but not because the named path did not exist\n(i.e. the directory may still exist---it is just this invocation of\nthe process happened to fail to see it---or it may not exist but we\ncannot see far enough to notice that it does not exist) then we play\nsafe, assume it does exist, and refrain from calling prune_worktree()\non it.  Which makes sense, but do we need to set errno to EEXIST\nhere?  Does prune_worktree() ensure the value left in errno when it\nreturns failure in a similar way to allow the caller of this new\nhelper make effective and reliable use of errno?\n\n> +\tstrbuf_addf(&reason, _(\"Removing worktrees/%s: worktree directory is not present\"), wt->id);\n> +\tret = prune_worktree(wt->id, &reason, TIME_MAX);\n> +\treturn ret;\n> +}\n"},{"id":"384485","messageId":"xmqqimoin92j.fsf@gitster-ct.c.googlers.com","threadId":"52057","inReplyTo":"20191018194542.1316981-3-pjones@redhat.com","subject":"Re: [PATCH v2 3/4] Make die_if_checked_out() prune missing checkouts of unlocked worktrees.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-21T02:09:24Z","receivedAt":"2019-10-21T02:09:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Jones <pjones@redhat.com> writes:\n\n[jc: won't repeat comments on the title]\n\n> @@ -360,6 +360,12 @@ void die_if_checked_out(const char *branch, int ignore_current_worktree)\n>  \twt = find_shared_symref(\"HEAD\", branch);\n>  \tif (!wt || (ignore_current_worktree && wt->is_current))\n>  \t\treturn;\n\ndie-if-checked-out is called from callers that expect to be stopped\nbefore they do any harm, so it feels dirty to make a side effect\nlike this.\n\nIf the user tries to check out a branch that used to be checked out\nin an already removed worktree, doesn't that indicate that an\nearlier worktree removal was done incorrectly, which is something\nworth reporting to the user and give the user a chance to think and\nchoose what corrective action(s) need to be taken?\n\nFor that, instead of automatically losing information like this\npatch does, it may make more sense to fail the checkout and stop at\ngiving diagnosis (e.g. \"our record shows that the branch is checked\nout in that worktree, but you seem to have lost it.  if you forgot\nto prune it, then here is the command you can give to do so.\")\nwithout actually touching the filesystem.\n\nThanks.\n\n\n> +\n> +\tif (prune_worktree_if_missing(wt) >= 0) {\n> +\t\tdelete_worktrees_dir_if_empty();\n> +\t\treturn;\n> +\t}\n> +\n>  \tskip_prefix(branch, \"refs/heads/\", &branch);\n>  \tdie(_(\"'%s' is already checked out at '%s'\"),\n>  \t    branch, wt->path);\n"},{"id":"385780","messageId":"CAPig+cTExu1+XyhUaq=yY09CAK6NN_BQViQETU8_fbGxu3jWzg@mail.gmail.com","threadId":"52057","inReplyTo":"20191018194317.wvqphshpkfskvkyh@redhat.com","subject":"Re: [PATCH 2/2] Make \"git branch -d\" prune missing worktrees automatically.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-11-08T10:14:18Z","receivedAt":"2019-11-08T10:14:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"[cc:+duy]\n\nOn Fri, Oct 18, 2019 at 3:43 PM Peter Jones <pjones@redhat.com> wrote:\n> On Thu, Oct 17, 2019 at 01:28:09PM -0400, Eric Sunshine wrote:\n> > Echoing SEZDER's comment on patch 1/2, this behavior is an intentional\n> > design choice and safety feature of the worktree implementation since\n> > worktrees may exist on removable media or remote filesystems which\n> > might not always be mounted; hence, the presence of commands \"git\n> > worktree prune\" and \"git worktree remove\".\n>\n> Okay, I see that use case now - I hadn't realized there was an\n> intentional design decision here, and honestly that's anything but clear\n> from the *code*.\n\nIt can indeed sometimes be difficult to get a high-level functional\noverview by examining code in isolation. In this case, at least,\ngit-worktree documentation tries to be clear about the \"why\" and \"how\"\nof the pruning behavior (which is not to say that the documentation --\nor the code -- can't be improved to communicate this better).\n\n> It's surprising, for example, that my patches didn't break a single\n> test case.\n\nTests suites are never perfect, and an attempt to prune a dangling\nworktree by deleting a branch likely never occurred to the\ngit-worktree implementer(s).\n\n> > These minor implementation comments aside, before considering this\n> > patch series, it would be nice to see a compelling argument as to why\n> > this change of behavior, which undercuts a deliberate design decision,\n> > is really desirable.\n>\n> Okay, so just for clarity, when you say there's a deliberate design\n> decision, which behavior here are you talking about? If you mean making\n> \"lock\" work, I don't have any issue with that. If you mean not cleaning\n> up when we do other commands, then I don't see why that's a concern -\n> after all, that's exactly what \"lock\" is for.\n\nTo clarify, I'm talking about Duy's deliberate design decision to\nmodel git-worktree auto-pruning after Git's own garbage-collection\nbehavior. That model includes, not only explicit locking, but a grace\nperiod before dangling worktree administrative files can be pruned\nautomatically (see the gc.worktreePruneExpire configuration).\n\nThe point of git-worktree's grace period (just like git-gc's grace\nperiod) is to avoid deleting potentially precious information\npermanently. For instance, the worktree-local \"index\" file might have\nsome changes staged but not yet committed. Under the existing model,\nthose staged changes are immune from being accidentally deleted\npermanently until after the grace period expires or until they are\nthrown away deliberately (say, via \"git worktree prune --expire=now\").\n\n> Assuming it is the \"lock\" behavior we're talking about, I don't think I\n> actually have any intention of breaking this design decision, just\n> making my workflow (without \"lock\") nag at me less for what seem like\n> pretty trivial issues.\n\nThe ability to lock a worktree is an extra safety measure built atop\nthe grace period mechanism to provide a way to completely override\nauto-pruning; it is not meant as an alternate or replacement safety\nmechanism to the grace period, but instead augments it. So, a behavior\nchange which respects only one of those safety mechanisms but not the\nother is likely flawed.\n\nAnd, importantly, people may already be relying upon this behavior of\nhaving an automatic grace period -- without having to place a worktree\nlock manually -- so changing behavior arbitrarily could break existing\nworkflows and result in data loss.\n\n> I can easily accommodate \"git worktree lock\". What bugs me though, is\n> that using worktrees basically means I have to replace fairly regular\n> filesystem activities with worktree commands, and it doesn't seem to be\n> *necessary* in any way. And I'm going to forget. A lot.\n>\n> To me, there doesn't seem to be any reason these need to behave any different:\n>\n> $ git worktree add foo foo\n> $ rm -rf foo\n> vs\n> $ git worktree add foo foo\n> $ git worktree remove foo\n>\n> And in fact the only difference right now, aside from some very\n> minuscule storage requirements that haven't gotten cleaned up, is the\n> first one leaves an artifact that tells it to give me errors later until\n> I run \"git worktree prune\" myself.\n\nI understand the pain point, but I also understand Duy's motivation\nfor being very careful about pruning worktree administrative files\nautomatically (so as to avoid data loss, such as changes already\nstaged to a worktree-local \"index\" file). While the proposed change\nmay address the pain point, it nevertheless creates the possibility of\naccidental loss which Duy was careful to avoid when designing worktree\nmechanics. Although annoying, the current behavior gives you the\nopportunity to avoid that accidental loss by forcing you to take\ndeliberate action to remove the worktree administrative files.\n\nPerhaps there is some way to address the pain point without breaking\nthe fundamental promise made by git-worktree about being careful with\nworktree metadata[*], but the changes proposed by this patch series\nseem insufficient (even if the patch is reworked to respect worktree\nlocking). I've cc:'d Duy in case he wants to chime in.\n\n[*] For instance, perhaps before auto-pruning, it could check whether\nthe index is recording staged changes or conflict information, and\nonly allow auto-pruning if the index is clean. *But* there may be\nother ways for information to be lost permanently (beyond a dirty\n\"index\") which don't occur to me at present, so this has to be\nconsidered carefully.\n"},{"id":"385787","messageId":"8c583f0c-c359-0fbe-2ffa-304db82b0a86@gmail.com","threadId":"52057","inReplyTo":"CAPig+cTExu1+XyhUaq=yY09CAK6NN_BQViQETU8_fbGxu3jWzg@mail.gmail.com","subject":"Re: [PATCH 2/2] Make \"git branch -d\" prune missing worktrees automatically.","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-11-08T14:56:43Z","receivedAt":"2019-11-08T14:56:49Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 08/11/2019 10:14, Eric Sunshine wrote:\n> [cc:+duy]\n> \n> On Fri, Oct 18, 2019 at 3:43 PM Peter Jones <pjones@redhat.com> wrote:\n>> On Thu, Oct 17, 2019 at 01:28:09PM -0400, Eric Sunshine wrote:\n>>> Echoing SEZDER's comment on patch 1/2, this behavior is an intentional\n>>> design choice and safety feature of the worktree implementation since\n>>> worktrees may exist on removable media or remote filesystems which\n>>> might not always be mounted; hence, the presence of commands \"git\n>>> worktree prune\" and \"git worktree remove\".\n>>\n>> Okay, I see that use case now - I hadn't realized there was an\n>> intentional design decision here, and honestly that's anything but clear\n>> from the *code*.\n> \n> It can indeed sometimes be difficult to get a high-level functional\n> overview by examining code in isolation. In this case, at least,\n> git-worktree documentation tries to be clear about the \"why\" and \"how\"\n> of the pruning behavior (which is not to say that the documentation --\n> or the code -- can't be improved to communicate this better).\n> \n>> It's surprising, for example, that my patches didn't break a single\n>> test case.\n> \n> Tests suites are never perfect, and an attempt to prune a dangling\n> worktree by deleting a branch likely never occurred to the\n> git-worktree implementer(s).\n> \n>>> These minor implementation comments aside, before considering this\n>>> patch series, it would be nice to see a compelling argument as to why\n>>> this change of behavior, which undercuts a deliberate design decision,\n>>> is really desirable.\n>>\n>> Okay, so just for clarity, when you say there's a deliberate design\n>> decision, which behavior here are you talking about? If you mean making\n>> \"lock\" work, I don't have any issue with that. If you mean not cleaning\n>> up when we do other commands, then I don't see why that's a concern -\n>> after all, that's exactly what \"lock\" is for.\n> \n> To clarify, I'm talking about Duy's deliberate design decision to\n> model git-worktree auto-pruning after Git's own garbage-collection\n> behavior. That model includes, not only explicit locking, but a grace\n> period before dangling worktree administrative files can be pruned\n> automatically (see the gc.worktreePruneExpire configuration).\n> \n> The point of git-worktree's grace period (just like git-gc's grace\n> period) is to avoid deleting potentially precious information\n> permanently. For instance, the worktree-local \"index\" file might have\n> some changes staged but not yet committed. Under the existing model,\n> those staged changes are immune from being accidentally deleted\n> permanently until after the grace period expires or until they are\n> thrown away deliberately (say, via \"git worktree prune --expire=now\").\n> \n>> Assuming it is the \"lock\" behavior we're talking about, I don't think I\n>> actually have any intention of breaking this design decision, just\n>> making my workflow (without \"lock\") nag at me less for what seem like\n>> pretty trivial issues.\n> \n> The ability to lock a worktree is an extra safety measure built atop\n> the grace period mechanism to provide a way to completely override\n> auto-pruning; it is not meant as an alternate or replacement safety\n> mechanism to the grace period, but instead augments it. So, a behavior\n> change which respects only one of those safety mechanisms but not the\n> other is likely flawed.\n> \n> And, importantly, people may already be relying upon this behavior of\n> having an automatic grace period -- without having to place a worktree\n> lock manually -- so changing behavior arbitrarily could break existing\n> workflows and result in data loss.\n> \n>> I can easily accommodate \"git worktree lock\". What bugs me though, is\n>> that using worktrees basically means I have to replace fairly regular\n>> filesystem activities with worktree commands, and it doesn't seem to be\n>> *necessary* in any way. And I'm going to forget. A lot.\n>>\n>> To me, there doesn't seem to be any reason these need to behave any different:\n>>\n>> $ git worktree add foo foo\n>> $ rm -rf foo\n>> vs\n>> $ git worktree add foo foo\n>> $ git worktree remove foo\n>>\n>> And in fact the only difference right now, aside from some very\n>> minuscule storage requirements that haven't gotten cleaned up, is the\n>> first one leaves an artifact that tells it to give me errors later until\n>> I run \"git worktree prune\" myself.\n> \n> I understand the pain point, but I also understand Duy's motivation\n> for being very careful about pruning worktree administrative files\n> automatically (so as to avoid data loss, such as changes already\n> staged to a worktree-local \"index\" file). While the proposed change\n> may address the pain point, it nevertheless creates the possibility of\n> accidental loss which Duy was careful to avoid when designing worktree\n> mechanics. Although annoying, the current behavior gives you the\n> opportunity to avoid that accidental loss by forcing you to take\n> deliberate action to remove the worktree administrative files.\n> \n> Perhaps there is some way to address the pain point without breaking\n> the fundamental promise made by git-worktree about being careful with\n> worktree metadata[*], but the changes proposed by this patch series\n> seem insufficient (even if the patch is reworked to respect worktree\n> locking). I've cc:'d Duy in case he wants to chime in.\n\nI agree that we want to preserve the safe guards in the worktree design. \nI wonder if detaching the HEAD of the missing worktree would solve the \nproblem without losing data. In the case where something wants to \ncheckout the same branch as the missing worktree then I think that is a \ngood solution. I think it should be OK for branch deletion as well.\n\nBest Wishes\n\nPhillip\n\n> [*] For instance, perhaps before auto-pruning, it could check whether\n> the index is recording staged changes or conflict information, and\n> only allow auto-pruning if the index is clean. *But* there may be\n> other ways for information to be lost permanently (beyond a dirty\n> \"index\") which don't occur to me at present, so this has to be\n> considered carefully.\n> \n"},{"id":"385833","messageId":"CAPig+cS9KXAH2+gUTV+q9p95Dc20TOt5naN5uH1_TjSaeL53rw@mail.gmail.com","threadId":"52057","inReplyTo":"8c583f0c-c359-0fbe-2ffa-304db82b0a86@gmail.com","subject":"Re: [PATCH 2/2] Make \"git branch -d\" prune missing worktrees automatically.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-11-09T11:34:53Z","receivedAt":"2019-11-09T11:35:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Nov 8, 2019 at 9:56 AM Phillip Wood <phillip.wood123@gmail.com> wrote:\n> On 08/11/2019 10:14, Eric Sunshine wrote:\n> > Perhaps there is some way to address the pain point without breaking\n> > the fundamental promise made by git-worktree about being careful with\n> > worktree metadata[*], but the changes proposed by this patch series\n> > seem insufficient (even if the patch is reworked to respect worktree\n> > locking). I've cc:'d Duy in case he wants to chime in.\n>\n> I agree that we want to preserve the safe guards in the worktree design.\n> I wonder if detaching the HEAD of the missing worktree would solve the\n> problem without losing data. In the case where something wants to\n> checkout the same branch as the missing worktree then I think that is a\n> good solution. I think it should be OK for branch deletion as well.\n\nI would feel very uncomfortable making \"automatic HEAD detachment\"\n(decapitation?) the default behavior. Although doing so may (in some\nfashion) safeguard precious information in .git/worktrees/<id>, it\npotentially brings its own difficulties. For instance, if someone\ntakes an action which automatically detaches HEAD of a missing\nworktree which had some branch checked out (and possibly some changes\nstaged in the worktree-specific \"index\"), and then builds more commits\non that branch, then that worktree gets into a state akin to rebased\nupstream (for which git-rebase documentation devotes an entire\nsection[1], \"Recovering From Upstream Rebase\"). While a power-user may\nbe able to recover from such a state, allowing the general Git user to\nget into such a situation by default seems contraindicated.\n\nI'm not even convinced that hiding the suggested \"auto-detach\"\nbehavior behind a configuration variable so power-users can enable it\nis entirely a good idea either since, while it may eliminate some\npain, it also potentially allows abandoned worktree entries to\naccumulate.\n\n[1]: https://git-scm.com/docs/git-rebase#_recovering_from_upstream_rebase\n"}]}