{"thread":{"id":"47270","subject":"[RFC PATCH] builtin/worktree: enhance worktree removal","startedAt":"2017-11-21T15:10:44Z","lastAt":"2017-11-28T16:46:55Z","messageCount":17,"participants":["Kaartic Sivaraam","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"333099","messageId":"20171121150954.3227-1-kaartic.sivaraam@gmail.com","threadId":"47270","inReplyTo":null,"subject":"[RFC PATCH] builtin/worktree: enhance worktree removal","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-11-21T15:09:54Z","receivedAt":"2017-11-21T15:10:44Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"The new feature to 'remove' worktree was handy to remove specific\nworktrees. It didn't cover one particular case of removal. Specifically,\nif there is an \"entry\" (a directory in /.git/worktrees)\nfor a worktree but the worktree repository itself does not exist then\nit means that the \"entry\" is stale and it could just be removed.\n\nSo, in case there's a \"worktree entry\" but not \"worktree direectory\"\nthen just remove the 'stale' entry.\n\nSigned-off-by: Kaartic Sivaraam \n---\n\nHello Duy,\n\nI noticed that your remove command could be enhanced for a particular\ncase. So, I made up an ad-hoc patch \"illustrating\" how it could be done.\nI may have broken something by quieting out 'validate_worktree()'\nfunction, so take note of it.\n\nYou might add this as a separate commit or just incorporate it into\none of your commits if you re-roll your 'nd/worktree-move' branch.\n\nThanks,\nKaartic\n\n builtin/worktree.c | 38 ++++++++++++++++++++++++++++----------\n 1 file changed, 28 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex b5afba164..f70bc0bd8 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -605,6 +605,22 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \treturn update_worktree_location(wt, dst.buf);\n }\n \n+/* Removes the .git/worktrees/worktree_id directory for\n+ the given worktree_id\n+\n+ Returns 0 on success and non-zero value in case of failure */\n+static int remove_worktree_entry(char *worktree_id) {\n+\tint ret = 0;\n+\tstruct strbuf we_path = STRBUF_INIT;\n+\tstrbuf_addstr(&we_path, git_common_path(\"worktrees/%s\", worktree_id));\n+\tif (remove_dir_recursively(&we_path, 0)) {\n+\t\terror_errno(_(\"failed to delete '%s'\"), we_path.buf);\n+\t\tret = -1;\n+\t}\n+\tstrbuf_release(&we_path);\n+\treturn ret;\n+}\n+\n static int remove_worktree(int ac, const char **av, const char *prefix)\n {\n \tint force = 0;\n@@ -634,9 +650,17 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n \t\t\tdie(_(\"already locked, reason: %s\"), reason);\n \t\tdie(_(\"already locked, no reason\"));\n \t}\n-\tif (validate_worktree(wt, 0))\n-\t\treturn -1;\n-\n+\tif (validate_worktree(wt, 1)) {\n+\t\tif (!file_exists(wt->path)) {\n+\t\t\t/* There's a worktree entry but the worktree directory\n+\t\t\t doesn't exist. So, just remove the worktree entry. */\n+\t\t\tret = remove_worktree_entry(wt->id);\n+\t\t\tfree_worktrees(worktrees);\n+\t\t\treturn ret;\n+\t\t} else {\n+\t\t\treturn -1;\n+\t\t}\n+\t}\n \tif (!force) {\n \t\tstruct argv_array child_env = ARGV_ARRAY_INIT;\n \t\tstruct child_process cp;\n@@ -670,13 +694,7 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n \t\terror_errno(_(\"failed to delete '%s'\"), sb.buf);\n \t\tret = -1;\n \t}\n-\tstrbuf_reset(&sb);\n-\tstrbuf_addstr(&sb, git_common_path(\"worktrees/%s\", wt->id));\n-\tif (remove_dir_recursively(&sb, 0)) {\n-\t\terror_errno(_(\"failed to delete '%s'\"), sb.buf);\n-\t\tret = -1;\n-\t}\n-\tstrbuf_release(&sb);\n+\tret = remove_worktree_entry(wt->id);\n \tfree_worktrees(worktrees);\n \treturn ret;\n }\n-- \n2.15.0.345.gf926f18f3\n\n"},{"id":"333169","messageId":"CAPig+cQrC1MiPrwwPqtdX2tzj1ntFHxO28uqnMBcmVpPPrc_Rg@mail.gmail.com","threadId":"47270","inReplyTo":"20171121150954.3227-1-kaartic.sivaraam@gmail.com","subject":"Re: [RFC PATCH] builtin/worktree: enhance worktree removal","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-21T21:37:58Z","receivedAt":"2017-11-21T21:38:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 21, 2017 at 10:09 AM, Kaartic Sivaraam\n wrote:\n> The new feature to 'remove' worktree was handy to remove specific\n> worktrees. It didn't cover one particular case of removal. Specifically,\n> if there is an \"entry\" (a directory in /.git/worktrees)\n> for a worktree but the worktree repository itself does not exist then\n> it means that the \"entry\" is stale and it could just be removed.\n>\n> So, in case there's a \"worktree entry\" but not \"worktree direectory\"\n> then just remove the 'stale' entry.\n\nLet me see if I understand. Sometimes you know that you've deleted the\nworktree directory, in which case 'git worktree prune' is the obvious\nchoice. However, there may be cases when you've forgotten that you\ndeleted the worktree directory (or it got deleted some other way), yet\nit still shows up in \"git worktree list\" output with no indication\nthat it has been deleted (though, ideally, it should tell you so[1]).\nIn this case, you see a worktree that you know you no longer want, so\nyou invoke \"git worktree remove\" but that errors out because the\nactual directory is already gone. This patch make the operation\nsucceed, despite the missing directory. Is that correct?\n\n[1]: Excerpt from:\nhttps://public-inbox.org/git/CAPig+cTTrv2C7JLu1dr4+N8xo+7YQ+deiwLDA835wBGD6fhS1g@mail.gmail.com/\n\nOther information which would be nice to display for each worktree\n[by the 'list' command] (possibly controlled by a --verbose flag):\n\n * the checked out branch or detached head\n * whether it is locked\n - the lock reason (if available)\n - and whether the worktree is currently accessible\n * whether it can be pruned\n - and the prune reason if so\n"},{"id":"333221","messageId":"xmqqa7zfxdru.fsf@gitster.mtv.corp.google.com","threadId":"47270","inReplyTo":"CAPig+cQrC1MiPrwwPqtdX2tzj1ntFHxO28uqnMBcmVpPPrc_Rg@mail.gmail.com","subject":"Re: [RFC PATCH] builtin/worktree: enhance worktree removal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-22T02:12:21Z","receivedAt":"2017-11-22T02:12:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Let me see if I understand. Sometimes you know that you've deleted the\n> worktree directory, in which case 'git worktree prune' is the obvious\n> choice. However, there may be cases when you've forgotten that you\n> deleted the worktree directory (or it got deleted some other way), yet\n> it still shows up in \"git worktree list\" output with no indication\n> that it has been deleted (though, ideally, it should tell you so[1]).\n> In this case, you see a worktree that you know you no longer want, so\n> you invoke \"git worktree remove\" but that errors out because the\n> actual directory is already gone. This patch make the operation\n> succeed, despite the missing directory. Is that correct?\n\nHmph, so the user could be using \"git worktree prune\" after seeing\nthe error? Was there a reason behind \"git worktree remove\" to be\nextra careful to make sure both existed before going forward, or was\nthis a simple oversight?\n"},{"id":"333227","messageId":"CAPig+cTux4dfBsX3DD=5TbM-p4-t66WX3+sufi39-W5Dw+ZvOw@mail.gmail.com","threadId":"47270","inReplyTo":"xmqqa7zfxdru.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH] builtin/worktree: enhance worktree removal","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-22T03:14:49Z","receivedAt":"2017-11-22T03:14:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 21, 2017 at 9:12 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>> Let me see if I understand. Sometimes you know that you've deleted the\n>> worktree directory, in which case 'git worktree prune' is the obvious\n>> choice. However, there may be cases when you've forgotten that you\n>> deleted the worktree directory (or it got deleted some other way), yet\n>> it still shows up in \"git worktree list\" output with no indication\n>> that it has been deleted (though, ideally, it should tell you so[1]).\n>> In this case, you see a worktree that you know you no longer want, so\n>> you invoke \"git worktree remove\" but that errors out because the\n>> actual directory is already gone. This patch make the operation\n>> succeed, despite the missing directory. Is that correct?\n>\n> Hmph, so the user could be using \"git worktree prune\" after seeing\n> the error? Was there a reason behind \"git worktree remove\" to be\n> extra careful to make sure both existed before going forward, or was\n> this a simple oversight?\n\nThe erroring out in this case looks like simple oversight. Most\nlikely, this particular case did not occur to Duy. The code does\nintentionally check the directory to see if it is dirty so that it can\nwarn the user (in which case the user can re-run with --force or take\nother corrective action), but erroring out if the directory is merely\nan indirect (and unintended) result of trying to check for dirtiness.\n\nSo, Kaatic's patch is intended to address that oversight (though I\nhaven't examined the implementation closely; I was just trying to\nunderstand the reason for the patch).\n"},{"id":"333229","messageId":"CAPig+cStgFaeWoNVcwaJVtNnYeLQ2LfS9xXF7uM3Py17HUaFFw@mail.gmail.com","threadId":"47270","inReplyTo":"CAPig+cTux4dfBsX3DD=5TbM-p4-t66WX3+sufi39-W5Dw+ZvOw@mail.gmail.com","subject":"Re: [RFC PATCH] builtin/worktree: enhance worktree removal","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-22T03:23:08Z","receivedAt":"2017-11-22T03:23:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 21, 2017 at 10:14 PM, Eric Sunshine wrote:\n> The erroring out in this case looks like simple oversight. Most\n> likely, this particular case did not occur to Duy. The code does\n> intentionally check the directory to see if it is dirty so that it can\n> warn the user (in which case the user can re-run with --force or take\n> other corrective action), but erroring out if the directory is merely\n\n\"...erroring out if the directory _is missing_ is merely...\"\n\n> an indirect (and unintended) result of trying to check for dirtiness.\n>\n> So, Kaatic's patch is intended to address that oversight (though I\n> haven't examined the implementation closely; I was just trying to\n> understand the reason for the patch).\n"},{"id":"333235","messageId":"xmqqvai3ufu9.fsf@gitster.mtv.corp.google.com","threadId":"47270","inReplyTo":"CAPig+cStgFaeWoNVcwaJVtNnYeLQ2LfS9xXF7uM3Py17HUaFFw@mail.gmail.com","subject":"Re: [RFC PATCH] builtin/worktree: enhance worktree removal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-22T03:55:58Z","receivedAt":"2017-11-22T03:56:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Tue, Nov 21, 2017 at 10:14 PM, Eric Sunshine wrote:\n>> The erroring out in this case looks like simple oversight. Most\n>> likely, this particular case did not occur to Duy. The code does\n>> intentionally check the directory to see if it is dirty so that it can\n>> warn the user (in which case the user can re-run with --force or take\n>> other corrective action), but erroring out if the directory is merely\n>\n> \"...erroring out if the directory _is missing_ is merely...\"\n>\n>> an indirect (and unintended) result of trying to check for dirtiness.\n>>\n>> So, Kaatic's patch is intended to address that oversight (though I\n>> haven't examined the implementation closely; I was just trying to\n>> understand the reason for the patch).\n\nOK, so the proposed log message was a bit confusing for those who\nare *not* the person who wrote it (who knew why existing behaviour\nwas inadequate and did not describe how \"worktree remove\" would fail\nunder such a scenario to illustrate it, incorrectly assuming that\neverybody who reads the proposed log message already *knows* how it\nwould fail).\n\n\t\"git worktree remove\" removes both the named worktree\n\tdirectory and the administrative information for it after\n\tchecking that there is no local modifications that would be\n\tlost (which is a handy safety measure). It however refuses\n\tto work if the worktree directory is _already_ removed.\n\n\tThe user could use \"git worktree prune\" after seeing the\n\terror and realizing the situation, but at that point, there\n\tis nothing gained by leaving only the administrative data\n\tbehind. Teach \"git worktree remove\" to go ahead and remove\n\tthe trace of the worktree in such a case.\n\nor soemthing like that?\n"},{"id":"333238","messageId":"CAPig+cQ1D+MFZkEX5yQ-DXu_CPf4oa507ChNjYqA=vG1CdZ4SQ@mail.gmail.com","threadId":"47270","inReplyTo":"xmqqvai3ufu9.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH] builtin/worktree: enhance worktree removal","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-22T04:37:55Z","receivedAt":"2017-11-22T04:38:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 21, 2017 at 10:55 PM, Junio C Hamano wrote:\n> OK, so the proposed log message was a bit confusing for those who\n> are *not* the person who wrote it (who knew why existing behaviour\n> was inadequate and did not describe how \"worktree remove\" would fail\n> under such a scenario to illustrate it, incorrectly assuming that\n> everybody who reads the proposed log message already *knows* how it\n> would fail).\n\nCorrect. The log message is confusing, enough so that my knee-jerk\nreaction was that the patch was trying to re-invent 'git worktree\nprune'. That seemed odd, so I spent extra time trying to figure out\nwhat it really meant, which led to my rather lengthy response asking\nif I was understanding the situation correctly.\n\n> \"git worktree remove\" removes both the named worktree\n> directory and the administrative information for it after\n> checking that there is no local modifications that would be\n> lost (which is a handy safety measure). It however refuses\n> to work if the worktree directory is _already_ removed.\n\nThe \"refusal\" is by accident, so perhaps phrase it like this:\n\n However, due to an oversight, it aborts with an error if the\n worktree directory is _already_ removed.\n\nor something.\n\n> The user could use \"git worktree prune\" after seeing the\n> error and realizing the situation, but at that point, there\n> is nothing gained by leaving only the administrative data\n> behind. Teach \"git worktree remove\" to go ahead and remove\n> the trace of the worktree in such a case.\n>\n> or soemthing like that?\n\nYes, I quite like it; it conveys the information necessary to\nunderstand the issue.\n\nKaartic: Regarding the actual patch, rather than silencing\nvalidate_worktree() (which seems an unfortunate thing to do), isn't it\npossible simply to do a quick test to see if the worktree directory\nexists before calling validate_worktree()? If it doesn't exist, then\njust skip down to the part of the code which does the 'prune'\noperation.\n"},{"id":"333298","messageId":"fee60b19-bc8e-bbd8-9f5d-347b2f941b2e@gmail.com","threadId":"47270","inReplyTo":"CAPig+cQrC1MiPrwwPqtdX2tzj1ntFHxO28uqnMBcmVpPPrc_Rg@mail.gmail.com","subject":"Re: [RFC PATCH] builtin/worktree: enhance worktree removal","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-11-22T17:09:39Z","receivedAt":"2017-11-22T17:09:51Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Wednesday 22 November 2017 03:07 AM, Eric Sunshine wrote:\n> On Tue, Nov 21, 2017 at 10:09 AM, Kaartic Sivaraam\n> wrote:\n>> The new feature to 'remove' worktree was handy to remove specific\n>> worktrees. It didn't cover one particular case of removal. Specifically,\n>> if there is an \"entry\" (a directory in /.git/worktrees)\n>> for a worktree but the worktree repository itself does not exist then\n>> it means that the \"entry\" is stale and it could just be removed.\n>>\n>> So, in case there's a \"worktree entry\" but not \"worktree direectory\"\n>> then just remove the 'stale' entry.\n> \n> Let me see if I understand. Sometimes you know that you've deleted the\n> worktree directory, in which case 'git worktree prune' is the obvious\n> choice. However, there may be cases when you've forgotten that you\n> deleted the worktree directory (or it got deleted some other way), yet\n> it still shows up in \"git worktree list\" output with no indication\n> that it has been deleted (though, ideally, it should tell you so[1]).\n> In this case, you see a worktree that you know you no longer want, so\n> you invoke \"git worktree remove\" but that errors out because the\n> actual directory is already gone. This patch make the operation\n> succeed, despite the missing directory. Is that correct?\n> \n\nYes. My primary motive for sending this is that, I thought it didn't \nmake much sense for 'remove' to fail just because the directory didn't \nexist. Further, I thought it would be more flexible to allow for \n'selective' pruning of worktrees. The use case in which, I thought, this \nflexibility might prove helpful is that of 'scripts'.\n\nConsider a hypothetical script that plays around with a git repository. \nFurther, consider that spawns a new worktree for temporary use. It's \nquite natural for the script to consider cleaning up it's own mess and \nthus it might consider removing the worktree it created. With the \n'remove' command it is as easy is,\n\ngit worktree remove \n\nBut what if it was the case that the directory might/might not exist \nwhen that part of the script is reached. In case the directory didn't \nexist the 'remove' command errors out that it didn't exist instead of \njust removing the worktree \"entry\" for that . I thought it \nwasn't a good thing and thus this script.\n\nAfter writing this, I feel I haven't justified it well (impostor \nsyndrome, possibly). Anyways, at the end of the day this \"ad-hoc\" patch \nwas just a result of my gut feeling that, \"It didn't make sense for the \n'remove' command to fail when the directory didn't exist\". The \nimplementation is just a \"sloppy\" illustration of how it could be \nachieved and should \"better\" not be used as such.\n\n\n> [1]: Excerpt from:\n> https://public-inbox.org/git/CAPig+cTTrv2C7JLu1dr4+N8xo+7YQ+deiwLDA835wBGD6fhS1g@mail.gmail.com/\n> \n> Other information which would be nice to display for each worktree\n> [by the 'list' command] (possibly controlled by a --verbose flag):\n> \n> * the checked out branch or detached head\n> * whether it is locked\n> - the lock reason (if available)\n> - and whether the worktree is currently accessible\n> * whether it can be pruned\n> - and the prune reason if so\n> \n\nIt would nice to see this information. It would be very useful but \n'list' doesn't seem to be the right sub-sub-command to give such \ninformation. Maybe there should be a new sub-sub-command named 'info' or \nsomething to give such information?\n\n"},{"id":"333299","messageId":"bc4ef82c-083d-c36b-c406-122819248501@gmail.com","threadId":"47270","inReplyTo":"xmqqvai3ufu9.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH] builtin/worktree: enhance worktree removal","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-11-22T17:13:44Z","receivedAt":"2017-11-22T17:13:57Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Wednesday 22 November 2017 09:25 AM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>>> So, Kaatic's patch is intended to address that oversight (though I\n>>> haven't examined the implementation closely; I was just trying to\n>>> understand the reason for the patch).\n> \n> OK, so the proposed log message was a bit confusing for those who\n> are *not* the person who wrote it (who knew why existing behaviour\n> was inadequate and did not describe how \"worktree remove\" would fail\n> under such a scenario to illustrate it, incorrectly assuming that\n> everybody who reads the proposed log message already *knows* how it\n> would fail).\n\nI shouldn't have made the log message as 'ad hoc' as I made the patch, \nsorry :-(\n\n> \n> \t\"git worktree remove\" removes both the named worktree\n> \tdirectory and the administrative information for it after\n> \tchecking that there is no local modifications that would be\n> \tlost (which is a handy safety measure). It however refuses\n> \tto work if the worktree directory is _already_ removed.\n> \n> \tThe user could use \"git worktree prune\" after seeing the\n> \terror and realizing the situation, but at that point, there\n> \tis nothing gained by leaving only the administrative data\n> \tbehind. Teach \"git worktree remove\" to go ahead and remove\n> \tthe trace of the worktree in such a case.\n> \n> or soemthing like that?\n> \n\nMuch better!\n"},{"id":"333306","messageId":"20883815-0d9b-a4d8-cc93-0eb8b918551f@gmail.com","threadId":"47270","inReplyTo":"CAPig+cQ1D+MFZkEX5yQ-DXu_CPf4oa507ChNjYqA=vG1CdZ4SQ@mail.gmail.com","subject":"Re: [RFC PATCH] builtin/worktree: enhance worktree removal","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-11-22T17:51:17Z","receivedAt":"2017-11-22T17:51:31Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"> \n> Kaartic: Regarding the actual patch, rather than silencing\n> validate_worktree() (which seems an unfortunate thing to do), isn't it\n> possible simply to do a quick test to see if the worktree directory\n> exists before calling validate_worktree()? If it doesn't exist, then\n> just skip down to the part of the code which does the 'prune'\n> operation.\n> \n\nNice suggestion. It seems to be a much better thing to do than silencing \n'validate_worktree' (it was an \"ad-hoc\" patch, you see). I'll send an \nupdate incorporating this suggestion (of course, with the suggested \ncommit message), when I find time.\n\nThanks,\nKaartic\n"},{"id":"333310","messageId":"CAPig+cQF6V8HNdMX5AZbmz3_w2WhSfA4SFfNhQqxXBqPXTZL+w@mail.gmail.com","threadId":"47270","inReplyTo":"fee60b19-bc8e-bbd8-9f5d-347b2f941b2e@gmail.com","subject":"Re: [RFC PATCH] builtin/worktree: enhance worktree removal","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-22T18:05:59Z","receivedAt":"2017-11-22T18:06:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 22, 2017 at 12:09 PM, Kaartic Sivaraam\n wrote:\n> On Wednesday 22 November 2017 03:07 AM, Eric Sunshine wrote:\n>> [1]: Excerpt from:\n>> https://public-inbox.org/git/CAPig+cTTrv2C7JLu1dr4+N8xo+7YQ+deiwLDA835wBGD6fhS1g@mail.gmail.com/\n>>\n>> Other information which would be nice to display for each worktree\n>> [by the 'list' command] (possibly controlled by a --verbose flag):\n>>\n>> * the checked out branch or detached head\n>> * whether it is locked\n>> - the lock reason (if available)\n>> - and whether the worktree is currently accessible\n>> * whether it can be pruned\n>> - and the prune reason if so\n>\n> It would nice to see this information. It would be very useful but 'list'\n> doesn't seem to be the right sub-sub-command to give such information. Maybe\n> there should be a new sub-sub-command named 'info' or something to give such\n> information?\n\nI'm perfectly fine with having 'git worktree list' provide this extra\ninformation; that was the intention from the start. The \"quick\"\nsummary of all worktrees provided by 'git worktree list' is exactly\nthe sort of place where you're most likely to notice something\nunexpected, such as the missing worktree directory. And, this extra\ninformation doesn't have to be noisy:\n\n--- 8< ---\n% git worktree list\ngiggle 89ea799ffc [master]\n../bobble f172cb543d [feature1] locked\n../fumple 6453c84b7d (detached HEAD) prunable\n--- 8< ---\n\nAnd, the verbose case:\n\n--- 8< ---\n% git worktree list -v\ngiggle 89ea799ffc [master]\n../bobble f172cb543d [feature1]\n locked: worktree on removable media\n../fumple 6453c84b7d (detached HEAD)\n prunable: directory does not exist\n--- 8< ---\n\nAn \"info\" command you speak of might also have value, but that's\northogonal to this extra information that 'git worktree list' could\nprovide.\n"},{"id":"333623","messageId":"20171127173621.6707-1-kaartic.sivaraam@gmail.com","threadId":"47270","inReplyTo":"20883815-0d9b-a4d8-cc93-0eb8b918551f@gmail.com","subject":"[RFC PATCH v2] builtin/worktree: enhance worktree removal","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-11-27T17:36:21Z","receivedAt":"2017-11-27T17:37:04Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"\"git worktree remove\" removes both the named worktree\ndirectory and the administrative information for it after\nchecking that there is no local modifications that would be\nlost (which is a handy safety measure). However, due to a\npossible oversight, it aborts with an error if the worktree\ndirectory is _already_ removed.\n\nThe user could use \"git worktree prune\" after seeing the\nerror and realizing the situation, but at that point, there\nis nothing gained by leaving only the administrative data\nbehind. Teach \"git worktree remove\" to go ahead and remove\nthe trace of the worktree in such a case.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Kaartic Sivaraam \n---\n Changes in v2:\n\n - incorporated the suggestion to avoid quieting `validate_worktree()`\n to detect inexistent directory (thanks, Eric!)\n\n - used the suggested (much better) commit message\n\n builtin/worktree.c | 35 ++++++++++++++++++++++++++++-------\n 1 file changed, 28 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex b5afba164..6eab91889 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -605,6 +605,23 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \treturn update_worktree_location(wt, dst.buf);\n }\n \n+/* Removes the .git/worktrees/worktree_id directory for\n+ * the given worktree_id\n+ *\n+ * Returns 0 on success and non-zero value in case of failure\n+ */\n+static int remove_worktree_entry(char *worktree_id) {\n+\tint ret = 0;\n+\tstruct strbuf we_path = STRBUF_INIT;\n+\tstrbuf_addstr(&we_path, git_common_path(\"worktrees/%s\", worktree_id));\n+\tif (remove_dir_recursively(&we_path, 0)) {\n+\t\terror_errno(_(\"failed to delete '%s'\"), we_path.buf);\n+\t\tret = -1;\n+\t}\n+\tstrbuf_release(&we_path);\n+\treturn ret;\n+}\n+\n static int remove_worktree(int ac, const char **av, const char *prefix)\n {\n \tint force = 0;\n@@ -634,6 +651,16 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n \t\t\tdie(_(\"already locked, reason: %s\"), reason);\n \t\tdie(_(\"already locked, no reason\"));\n \t}\n+\n+\tif (!file_exists(wt->path)) {\n+\t/* There's a worktree entry but the worktree directory\n+\t * doesn't exist. So, just remove the worktree entry.\n+\t */\n+\t\tret = remove_worktree_entry(wt->id);\n+\t\tfree_worktrees(worktrees);\n+\t\treturn ret;\n+\t}\n+\n \tif (validate_worktree(wt, 0))\n \t\treturn -1;\n \n@@ -670,13 +697,7 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n \t\terror_errno(_(\"failed to delete '%s'\"), sb.buf);\n \t\tret = -1;\n \t}\n-\tstrbuf_reset(&sb);\n-\tstrbuf_addstr(&sb, git_common_path(\"worktrees/%s\", wt->id));\n-\tif (remove_dir_recursively(&sb, 0)) {\n-\t\terror_errno(_(\"failed to delete '%s'\"), sb.buf);\n-\t\tret = -1;\n-\t}\n-\tstrbuf_release(&sb);\n+\tret = remove_worktree_entry(wt->id);\n \tfree_worktrees(worktrees);\n \treturn ret;\n }\n-- \n2.15.0.345.gf926f18f3\n\n"},{"id":"333673","messageId":"xmqq7eub85rz.fsf@gitster.mtv.corp.google.com","threadId":"47270","inReplyTo":"20171127173621.6707-1-kaartic.sivaraam@gmail.com","subject":"Re: [RFC PATCH v2] builtin/worktree: enhance worktree removal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-28T03:02:24Z","receivedAt":"2017-11-28T03:02:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kaartic Sivaraam writes:\n\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index b5afba164..6eab91889 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -605,6 +605,23 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n> \treturn update_worktree_location(wt, dst.buf);\n> }\n> \n> +/* Removes the .git/worktrees/worktree_id directory for\n> + * the given worktree_id\n> + *\n> + * Returns 0 on success and non-zero value in case of failure\n> + */\n\n\t/*\n\t * our multi-line comment should be formatted\n\t * more like this, giving slash-asterisk at the\n\t * beginning and asterisk-slash at the end their\n\t * own line.\n\t */\n\nThere are other instances of the same in this patch, I suspect, but\nbecause this seemed to depend on other things in 'pu' that are not\nready (if it depends on something that is stalled or abandoned, we\nneed to first get it unabandoned before this can even come close to\n'pu'), I didn't create a topic branch for this RFC patch to view the\nresulting file as a whole (this review is based only on the patch\ne-mail).\n\n> +static int remove_worktree_entry(char *worktree_id) {\n> +\tint ret = 0;\n> +\tstruct strbuf we_path = STRBUF_INIT;\n> +\tstrbuf_addstr(&we_path, git_common_path(\"worktrees/%s\", worktree_id));\n> +\tif (remove_dir_recursively(&we_path, 0)) {\n> +\t\terror_errno(_(\"failed to delete '%s'\"), we_path.buf);\n> +\t\tret = -1;\n> +\t}\n> +\tstrbuf_release(&we_path);\n> +\treturn ret;\n> +}\n> +\n\nThis lifts a small section of remove_worktree() to make it usable\nfrom other places. But see below.\n\n> static int remove_worktree(int ac, const char **av, const char *prefix)\n> {\n> \tint force = 0;\n> @@ -634,6 +651,16 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n> \t\t\tdie(_(\"already locked, reason: %s\"), reason);\n> \t\tdie(_(\"already locked, no reason\"));\n> \t}\n> +\n> +\tif (!file_exists(wt->path)) {\n> +\t/* There's a worktree entry but the worktree directory\n> +\t * doesn't exist. So, just remove the worktree entry.\n> +\t */\n> +\t\tret = remove_worktree_entry(wt->id);\n> +\t\tfree_worktrees(worktrees);\n> +\t\treturn ret;\n> +\t}\n> +\n> \tif (validate_worktree(wt, 0))\n> \t\treturn -1;\n\nI actually wonder if this \"early check and return\" is making the\ncode unmaintainable. What if we instead did it with just the\ncodeflow restructuring, perhaps like so?\n\n\tif (!validate_worktree(wt, 0)) {\n\t\t/* OK, work tree is sound */\n\t\tif (!force) {\n\t\t\t/* protect from information lossage */\n\t\t}\n\t\t/* do the actual worktree removal */\n\t}\n\t/* remove the control info */\n\nThere is no need for a new helper function when done that way, which\nallows us not to worry about two clean-up places drifting apart over\ntime. With this patch, we have two 3-line blocks that call\nremove_worktree_entry(wt->id), free_worktrees(worktrees) and returns\nret, and these happen to be identical, but the next person who would\nbe mucking with the code (perhaps adding more variables that need to\nbe reset in this codeflow) can easily miss one of these two places.\n\nThe resulting code would make the body of \"if (!force)\" block too\ndeeply nested, I suspect, but that is an indication that that part\nis overlong and overly complex in the context of this function, and\ncan and should migrate to its own helper function.\n\nHmm?\n\n> @@ -670,13 +697,7 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n> \t\terror_errno(_(\"failed to delete '%s'\"), sb.buf);\n> \t\tret = -1;\n> \t}\n> -\tstrbuf_reset(&sb);\n> -\tstrbuf_addstr(&sb, git_common_path(\"worktrees/%s\", wt->id));\n> -\tif (remove_dir_recursively(&sb, 0)) {\n> -\t\terror_errno(_(\"failed to delete '%s'\"), sb.buf);\n> -\t\tret = -1;\n> -\t}\n> -\tstrbuf_release(&sb);\n> +\tret = remove_worktree_entry(wt->id);\n> \tfree_worktrees(worktrees);\n> \treturn ret;\n> }\n"},{"id":"333676","messageId":"CAPig+cT3jbZ=VRWpggw_tvegAGnjGQ0Mxp8m2DdKKPhfKTVEWA@mail.gmail.com","threadId":"47270","inReplyTo":"xmqq7eub85rz.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH v2] builtin/worktree: enhance worktree removal","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-28T03:48:43Z","receivedAt":"2017-11-28T03:48:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 27, 2017 at 10:02 PM, Junio C Hamano wrote:\n> I actually wonder if this \"early check and return\" is making the\n> code unmaintainable. What if we instead did it with just the\n> codeflow restructuring, perhaps like so?\n>\n> if (!validate_worktree(wt, 0)) {\n> /* OK, work tree is sound */\n> if (!force) {\n> /* protect from information lossage */\n> }\n> /* do the actual worktree removal */\n> }\n> /* remove the control info */\n\nWith this approach, validate_worktree() will print an error message\nsaying that the worktree directory is missing before the control info\nis actually removed. Kaartic's original patch silenced the message\n(and _all_ error messages from validate_worktree()) by passing 1 as\nthe second argument. That seemed heavy-handed, so I suggested keeping\nthe validate_worktree() call as-is but doing the directory-missing\ncheck first as a special case.\n\nBut perhaps that special case isn't necessary. Perhaps I was\novercautious about having those error messages silenced. After all,\nthe user has explicitly asked to delete the worktree, so (presumably)\nnothing is lost by honoring that request, even if there are validation\nerrors. (We could even add a --verbose option to 'remove' to re-enable\nthose messages, but that's probably overkill.)\n\n> There is no need for a new helper function when done that way, which\n> allows us not to worry about two clean-up places drifting apart over\n> time.\n\nI had envisioned a simple 'goto remove_control_info' rather than extra\nnesting or refactoring, but that's a minor point.\n"},{"id":"333677","messageId":"xmqqy3mr6ocp.fsf@gitster.mtv.corp.google.com","threadId":"47270","inReplyTo":"CAPig+cT3jbZ=VRWpggw_tvegAGnjGQ0Mxp8m2DdKKPhfKTVEWA@mail.gmail.com","subject":"Re: [RFC PATCH v2] builtin/worktree: enhance worktree removal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-28T04:04:06Z","receivedAt":"2017-11-28T04:04:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> With this approach, validate_worktree() will print an error message\n> saying that the worktree directory is missing before the control info\n> is actually removed. Kaartic's original patch silenced the message\n> (and _all_ error messages from validate_worktree()) by passing 1 as\n> the second argument. That seemed heavy-handed, so I suggested keeping\n> the validate_worktree() call as-is but doing the directory-missing\n> check first as a special case.\n>\n> But perhaps that special case isn't necessary.\n\nI do not think the user minds to see \"there is no such directory\nthere\"; actually that would be beneficial, even.\n\nBut you are right; validate_worktree() would need to become aware of\nthe possibility that it can be passed such a \"corrupt\" worktree to\nhandle if that approach is followed.\n\nThe case we are discussing, i.e. the user removed the directory\nwithout telling Git to give it a chance to remove control\ninformation, may be common enough that it becomes a worthwhile\naddition to make the \"quiet\" boolean that occupies a whole int to an\nunsigned that is a collection of bits, and pass \"missing_ok\" bit in\nthat flag word to the validate_worktree() to tell that the caller\ncan tolerate the \"user removed it while we were looking the other\nway\" case and can handle it gracefully. But that would be a lot\nlarger change, and \"the caller checks before calling validate\" as\ndone with this [RFC v2] may be a good way to add the feature with\nthe least impact to the codebase.\n\n> I had envisioned a simple 'goto remove_control_info' rather than extra\n> nesting or refactoring, but that's a minor point.\n\nYes, use of goto is also a good way to avoid having to worry about\nthe future evolution of the codeflow in this function.\n\nSo perhaps\n\n\tif (the directory is no longer there)\n\t\tgoto cleanup;\n\tif (the worktree does not validate)\n\t\treturn -1;\n\t... the original code to (carefully) remove the \n\t... worktree itself\n\n\tcleanup:\n\n\t... remove control info\n\t... free resources held in variables\n\t... return from the function\n\nis what we want?\n\nIn any case, I think this needs thawing nd/worktree-move topic\nbefore it can move forward, as all the functions we discussed in\nthis thread are the ones that were introduced in that topic and do\nnot exist in the mainline.\n\n"},{"id":"333681","messageId":"CAPig+cRuR2cE9RtdOfvgzGbPVLBEtb38v24Myh9rn4zKk525yA@mail.gmail.com","threadId":"47270","inReplyTo":"xmqqy3mr6ocp.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH v2] builtin/worktree: enhance worktree removal","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-28T06:02:36Z","receivedAt":"2017-11-28T06:02:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 27, 2017 at 11:04 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>> I had envisioned a simple 'goto remove_control_info' rather than extra\n>> nesting or refactoring, but that's a minor point.\n>\n> Yes, use of goto is also a good way to avoid having to worry about\n> the future evolution of the codeflow in this function.\n>\n> So perhaps\n>\n> if (the directory is no longer there)\n> goto cleanup;\n> if (the worktree does not validate)\n> return -1;\n> ... the original code to (carefully) remove the\n> ... worktree itself\n>\n> cleanup:\n>\n> ... remove control info\n> ... free resources held in variables\n> ... return from the function\n>\n> is what we want?\n\nYes, that's what I had in mind.\n"},{"id":"333699","messageId":"c2e041ab-4c29-99a8-2de9-a68e986c225c@gmail.com","threadId":"47270","inReplyTo":"xmqqy3mr6ocp.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH v2] builtin/worktree: enhance worktree removal","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-11-28T16:46:44Z","receivedAt":"2017-11-28T16:46:55Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Tuesday 28 November 2017 09:34 AM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n> \n>> With this approach, validate_worktree() will print an error message\n>> saying that the worktree directory is missing before the control info\n>> is actually removed. Kaartic's original patch silenced the message\n>> (and _all_ error messages from validate_worktree()) by passing 1 as\n>> the second argument. That seemed heavy-handed, so I suggested keeping\n>> the validate_worktree() call as-is but doing the directory-missing\n>> check first as a special case.\n>>\n>> But perhaps that special case isn't necessary.\n> \n> I do not think the user minds to see \"there is no such directory\n> there\"; actually that would be beneficial, even.\n> \n> But you are right; validate_worktree() would need to become aware of\n> the possibility that it can be passed such a \"corrupt\" worktree to\n> handle if that approach is followed.\n> \n> The case we are discussing, i.e. the user removed the directory\n> without telling Git to give it a chance to remove control\n> information, may be common enough that it becomes a worthwhile\n> addition to make the \"quiet\" boolean that occupies a whole int to an\n> unsigned that is a collection of bits, and pass \"missing_ok\" bit in\n> that flag word to the validate_worktree() to tell that the caller\n> can tolerate the \"user removed it while we were looking the other\n> way\" case and can handle it gracefully. But that would be a lot\n> larger change, and \"the caller checks before calling validate\" as\n> done with this [RFC v2] may be a good way to add the feature with\n> the least impact to the codebase.\n> \n>> I had envisioned a simple 'goto remove_control_info' rather than extra\n>> nesting or refactoring, but that's a minor point.\n> \n> Yes, use of goto is also a good way to avoid having to worry about\n> the future evolution of the codeflow in this function.\n> \n> So perhaps\n> \n> \tif (the directory is no longer there)\n> \t\tgoto cleanup;\n> \tif (the worktree does not validate)\n> \t\treturn -1;\n> \t... the original code to (carefully) remove the\n> \t... worktree itself\n> \n> \tcleanup:\n> \n> \t... remove control info\n> \t... free resources held in variables\n> \t... return from the function\n> \n> is what we want?\n>\n\nProbably but I'm not interested in going for a v3 that does that as I \njust wanted to show that worktree remove could be enhanced in this \naspect and show how it could be done. So, I'll leave this in the \n#leftoverbits for the person who would be re-rolling nd/worktree-move.\n\n---\nKaartic\n"}]}