{"thread":{"id":"54317","subject":"[RFC PATCH 0/2] teach `worktree list` to mark locked worktrees","startedAt":"2020-09-28T15:50:20Z","lastAt":"2020-10-12T02:24:35Z","messageCount":23,"participants":["Rafael Silva","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"406537","messageId":"20200928154953.30396-1-rafaeloliveira.cs@gmail.com","threadId":"54317","inReplyTo":null,"subject":"[RFC PATCH 0/2] teach `worktree list` to mark locked worktrees","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-09-28T15:49:51Z","receivedAt":"2020-09-28T15:50:20Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"This patch series introduces a new information on the git `worktree list`\ncommand output, to mark when a worktree is locked with a (locked) text mark.\n\nThe intent is to improve the user experience to earlier sinalize that a linked\nworktree is locked, instead of realising later when attempting to remove it\nwith `remove` command as it happened to me twice :)\n\nThe patches are divided into two parts. First part introduces\nthe new marker to the worktree list command and small documentation\nchange. And the second adds one test case into t2402 to test\nif the (locked) text will be properly set for a locked worktree, and\nnot mistankely set to a unlocked or master worktree.\n\nThis is the output of the worktree list with locked marker:\n\n $ git worktree list\n /repo/to/main abc123 [master]\n /path/to/unlocked-worktree1 456def [brancha]\n /path/to/locked-worktree 123abc (detached HEAD) (locked)\n\nThis patches are marked with RFC mainly due to:\n\n - This will change the default behaviour of the worktree list, I am\n not sure whether will be better to make this tuned via a config\n and/or a git parameter. (assuming this change is a good idea ;) )\n\n - Perhaps the `(locked)` marker is not the best suitable way to output\n this information and we might need to come with a better way.\n\n - I am a new contributor to the code base, still learning a lot of git\n internals data structure and commands. Likely this patch will require\n updates.\n\nRafael Silva (2):\n teach `list` to mark locked worktree\n t2402: add test to locked linked worktree marker\n\n Documentation/git-worktree.txt | 5 +++--\n builtin/worktree.c | 6 +++++-\n t/t2402-worktree-list.sh | 13 +++++++++++++\n 3 files changed, 21 insertions(+), 3 deletions(-)\n\n-- \n2.28.0.763.ge7086f1eef\n\n"},{"id":"406538","messageId":"20200928154953.30396-2-rafaeloliveira.cs@gmail.com","threadId":"54317","inReplyTo":"20200928154953.30396-1-rafaeloliveira.cs@gmail.com","subject":"[RFC PATCH 1/2] worktree: teach `list` to mark locked worktree","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-09-28T15:49:52Z","receivedAt":"2020-09-28T15:50:22Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"The output of `worktree list` command is extended to mark a locked\nworktree with `(locked)` text. This is used to communicate to the\nuser that a linked worktree is locked instead of learning only when\nattempting to remove it.\n\nThis is the output of the worktree list with locked marker:\n\n $ git worktree list\n /repo/to/main abc123 [master]\n /path/to/unlocked-worktree1 456def [brancha]\n /path/to/locked-worktree 123abc (detached HEAD) (locked)\n\nSigned-off-by: Rafael Silva \n---\n Documentation/git-worktree.txt | 5 +++--\n builtin/worktree.c | 6 +++++-\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex 32e8440cde..a3781dd664 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -96,8 +96,9 @@ list::\n \n List details of each working tree. The main working tree is listed first,\n followed by each of the linked working trees. The output details include\n-whether the working tree is bare, the revision currently checked out, and the\n-branch currently checked out (or \"detached HEAD\" if none).\n+whether the working tree is bare, the revision currently checked out, the\n+branch currently checked out (or \"detached HEAD\" if none), and whether\n+the worktree is locked.\n \n lock::\n \ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 99abaeec6c..8ad2cdd2f9 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -676,8 +676,12 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)\n \t\t} else\n \t\t\tstrbuf_addstr(&sb, \"(error)\");\n \t}\n-\tprintf(\"%s\\n\", sb.buf);\n \n+\tif (!is_main_worktree(wt) &&\n+\t worktree_lock_reason(wt))\n+\t\tstrbuf_addstr(&sb, \" (locked)\");\n+\n+\tprintf(\"%s\\n\", sb.buf);\n \tstrbuf_release(&sb);\n }\n \n-- \n2.28.0.763.ge7086f1eef\n\n"},{"id":"406539","messageId":"20200928154953.30396-3-rafaeloliveira.cs@gmail.com","threadId":"54317","inReplyTo":"20200928154953.30396-1-rafaeloliveira.cs@gmail.com","subject":"[RFC PATCH 2/2] t2402: add test to locked linked worktree marker","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-09-28T15:49:53Z","receivedAt":"2020-09-28T15:50:26Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"Test the output of the `worktree list` command to show when\na linked worktree is locked and test to not mistakenly\nmark main or unlocked worktrees.\n\nSigned-off-by: Rafael Silva \n---\n t/t2402-worktree-list.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh\nindex 52585ec2aa..07bd9a3350 100755\n--- a/t/t2402-worktree-list.sh\n+++ b/t/t2402-worktree-list.sh\n@@ -61,6 +61,19 @@ test_expect_success '\"list\" all worktrees --porcelain' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'show locked worktree with (locked)' '\n+\techo \"$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]\" >expect &&\n+\ttest_when_finished \"rm -rf locked unlocked out actual expect && git worktree prune\" &&\n+\tgit worktree add --detach locked master &&\n+\tgit worktree add --detach unlocked master &&\n+\tgit worktree lock locked &&\n+\techo \"$(git -C locked rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD) (locked)\" >>expect &&\n+\techo \"$(git -C unlocked rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)\" >>expect &&\n+\tgit worktree list >out &&\n+\tsed \"s/ */ /g\" actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'bare repo setup' '\n \tgit init --bare bare1 &&\n \techo \"data\" >file1 &&\n-- \n2.28.0.763.ge7086f1eef\n\n"},{"id":"406582","messageId":"xmqqft71lhty.fsf@gitster.c.googlers.com","threadId":"54317","inReplyTo":"20200928154953.30396-1-rafaeloliveira.cs@gmail.com","subject":"Re: [RFC PATCH 0/2] teach `worktree list` to mark locked worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-28T21:19:53Z","receivedAt":"2020-09-28T21:20:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rafael Silva writes:\n\n> This patch series introduces a new information on the git `worktree list`\n> command output, to mark when a worktree is locked with a (locked) text mark.\n>\n> The intent is to improve the user experience to earlier sinalize that a linked\n> worktree is locked, instead of realising later when attempting to remove it\n> with `remove` command as it happened to me twice :)\n\nChange with a good intention, it seems.\n\n> The patches are divided into two parts. First part introduces\n> the new marker to the worktree list command and small documentation\n> change. And the second adds one test case into t2402 to test\n> if the (locked) text will be properly set for a locked worktree, and\n> not mistankely set to a unlocked or master worktree.\n\nProbably they belong together in a single patch (I am saying this\nafter only seeing the above five lines, without reading either of\nthese two patches, so there may be some things in them that makes it\nmake sense to have them separate).\n\n> This is the output of the worktree list with locked marker:\n>\n> $ git worktree list\n> /repo/to/main abc123 [master]\n> /path/to/unlocked-worktree1 456def [brancha]\n> /path/to/locked-worktree 123abc (detached HEAD) (locked)\n\nLooks OK to me\n\n> This patches are marked with RFC mainly due to:\n>\n> - This will change the default behaviour of the worktree list, I am\n> not sure whether will be better to make this tuned via a config\n> and/or a git parameter. (assuming this change is a good idea ;) )\n\nThe default output is meant for human consumption (scripts that want\nto read from the command and expect stable output would be using the\n\"--porcelain\" option).\n\nThe ideal case is a new output is universally useful for everybody,\nin which case we can just change it without any new configuration or\ncommand line option.\n\n> - Perhaps the `(locked)` marker is not the best suitable way to output\n> this information and we might need to come with a better way.\n\nIt looks good enough to me. I am not qualified to have a strong\nopinion on this part, as I do not use the command all that often.\n\n $ git shortlog --no-merges --since=18.months builtin/worktree.c\n\ntells me that Eric Sunshine (CC'ed) may be a good source of wisdom\non this command.\n\n> - I am a new contributor to the code base, still learning a lot of git\n> internals data structure and commands. Likely this patch will require\n> updates.\n\nWelcome.\n\n> Rafael Silva (2):\n> teach `list` to mark locked worktree\n> t2402: add test to locked linked worktree marker\n>\n> Documentation/git-worktree.txt | 5 +++--\n> builtin/worktree.c | 6 +++++-\n> t/t2402-worktree-list.sh | 13 +++++++++++++\n> 3 files changed, 21 insertions(+), 3 deletions(-)\n"},{"id":"406584","messageId":"xmqq8sctlgzx.fsf@gitster.c.googlers.com","threadId":"54317","inReplyTo":"20200928154953.30396-2-rafaeloliveira.cs@gmail.com","subject":"Re: [RFC PATCH 1/2] worktree: teach `list` to mark locked worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-28T21:37:54Z","receivedAt":"2020-09-28T21:38:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rafael Silva writes:\n\n> The output of `worktree list` command is extended to mark a locked\n> worktree with `(locked)` text. This is used to communicate to the\n> user that a linked worktree is locked instead of learning only when\n> attempting to remove it.\n>\n> This is the output of the worktree list with locked marker:\n>\n> $ git worktree list\n> /repo/to/main abc123 [master]\n> /path/to/unlocked-worktree1 456def [brancha]\n> /path/to/locked-worktree 123abc (detached HEAD) (locked)\n\n\nIn our log message, we tend NOT to say \"This commit does X\" or \"X is\ndone\", because such a statement is often insufficient to illustrate\nif the commit indeed does X, and explain why it is a good thing to\ndo X in the first place.\n\nInstead, we \n\n - first explain that the current system does not do X (in present\n tense, so we do NOT say \"previously we did not do X\"), then\n\n - explain why doing X would be a good thing, and finally\n\n - give an order to the codebase to start doing X.\n\n\nFor this change, it might look like this:\n\n The \"git worktree list\" shows the absolute path to the working\n tree, the commit that is checked out and the name of the branch.\n It is not immediately obvious which of the worktrees, if any,\n are locked.\n\n \"git worktree remove\" refuses to remove a locked worktree with\n an error message. If \"git worktree list\" told which worktrees\n are locked in its output, the user would not even attempt to\n remove such a worktree.\n\n Teach \"git worktree list\" to append \"(locked)\" to its output.\n The output from the command becomes like so:\n\n $ git worktree list\n /repo/to/main abc123 [master]\n /path/to/unlocked-worktree1 456def [brancha]\n /path/to/locked-worktree 123abc (detached HEAD) (locked)\n\n\n\n> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\n> index 32e8440cde..a3781dd664 100644\n> --- a/Documentation/git-worktree.txt\n> +++ b/Documentation/git-worktree.txt\n> @@ -96,8 +96,9 @@ list::\n> \n> List details of each working tree. The main working tree is listed first,\n> followed by each of the linked working trees. The output details include\n> -whether the working tree is bare, the revision currently checked out, and the\n> -branch currently checked out (or \"detached HEAD\" if none).\n> +whether the working tree is bare, the revision currently checked out, the\n> +branch currently checked out (or \"detached HEAD\" if none), and whether\n> +the worktree is locked.\n\nAt the first glance, the above gave me an impression that you'd be\nadding \"(unlocked)\" or \"(locked)\" for each working tree, but that is\nnot the case. How about keeping the original sentence intact, and\nadding something like \"For a locked worktree, the marker (locked) is\nalso shown at the end\"?\n\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 99abaeec6c..8ad2cdd2f9 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -676,8 +676,12 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)\n> \t\t} else\n> \t\t\tstrbuf_addstr(&sb, \"(error)\");\n> \t}\n> -\tprintf(\"%s\\n\", sb.buf);\n> \n> +\tif (!is_main_worktree(wt) &&\n> +\t worktree_lock_reason(wt))\n> +\t\tstrbuf_addstr(&sb, \" (locked)\");\n\nIs this because for the primary worktree, worktree_lock_reason()\nwill always yield true?\n\n ... goes and looks ...\n\nAh, OK, the callers are not even allowed to ask the question on the\nprimary one. That's a bit strange API but OK.\n\nWriting that on a single line would perfectly be readable, by the\nway.\n\n\tif (!is_main_worktree(wt) && worktree_lock_reason(wt))\n\t\tstrbuf_addstr(&sb, \" (locked)\");\n\n> +\tprintf(\"%s\\n\", sb.buf);\n> \tstrbuf_release(&sb);\n> }\n"},{"id":"406585","messageId":"xmqq3631lg8f.fsf@gitster.c.googlers.com","threadId":"54317","inReplyTo":"20200928154953.30396-3-rafaeloliveira.cs@gmail.com","subject":"Re: [RFC PATCH 2/2] t2402: add test to locked linked worktree marker","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-28T21:54:24Z","receivedAt":"2020-09-28T23:13:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rafael Silva writes:\n\n> Test the output of the `worktree list` command to show when\n> a linked worktree is locked and test to not mistakenly\n> mark main or unlocked worktrees.\n>\n> Signed-off-by: Rafael Silva \n> ---\n> t/t2402-worktree-list.sh | 13 +++++++++++++\n> 1 file changed, 13 insertions(+)\n\nI think this should be part of [1/2], as the change necessary to\nimplement the new feature is small enough that there is no reason\nto split the test part out.\n\n> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh\n> index 52585ec2aa..07bd9a3350 100755\n> --- a/t/t2402-worktree-list.sh\n> +++ b/t/t2402-worktree-list.sh\n> @@ -61,6 +61,19 @@ test_expect_success '\"list\" all worktrees --porcelain' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'show locked worktree with (locked)' '\n> +\techo \"$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]\" >expect &&\n> +\ttest_when_finished \"rm -rf locked unlocked out actual expect && git worktree prune\" &&\n> +\tgit worktree add --detach locked master &&\n> +\tgit worktree add --detach unlocked master &&\n> +\tgit worktree lock locked &&\n> +\techo \"$(git -C locked rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD) (locked)\" >>expect &&\n> +\techo \"$(git -C unlocked rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)\" >>expect &&\n> +\tgit worktree list >out &&\n> +\tsed \"s/ */ /g\" actual &&\n> +\ttest_cmp expect actual\n> +'\n\nThis seems to prescribe the output from the command too strictly\n(you do avoid being overly too strict by removing the indentation\nwith 's/ */ /g' though). \n\nIf the leading path to the $TRASH_DIRECTORY has two or more\nconsecutive SPs (and that is not something under our control), the\n'expect' file would keep such a double-SP, but such a double-SP in\n'out' would have been squashed out in the 'actual' file.\n\nI wonder if\n\n\tgrep '/locked *[0-9a-f].* (locked)' out &&\n\t! grep '/unlocked *[0-9a-f].* (locked)' out\n\nmight be a better way to test? That is\n\n - we do not care what the leading directories are called\n - we do not care what branch is checked out or how they are presented\n - we care the one that ends with /locked is (locked)\n - we care the one that ends with /unlocked is not (locked)\n\nAfter all, this new test piece is not about verifying that the\nobject name or branch name is correct.\n"},{"id":"406646","messageId":"20200929213529.GA1336@contrib-buster.localdomain","threadId":"54317","inReplyTo":"xmqqft71lhty.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 0/2] teach `worktree list` to mark locked worktrees","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-09-29T21:35:29Z","receivedAt":"2020-09-29T21:35:36Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"Thank you for all the feedback, I will work on a another\npatch that will address all the feedback provided on this series,\nincluding having the changes into a single patch.\n\nOn Mon, Sep 28, 2020 at 02:19:53PM -0700, Junio C Hamano wrote:\n> Rafael Silva writes:\n> \n> > This patch series introduces a new information on the git `worktree list`\n> > command output, to mark when a worktree is locked with a (locked) text mark.\n> >\n> > The intent is to improve the user experience to earlier sinalize that a linked\n> > worktree is locked, instead of realising later when attempting to remove it\n> > with `remove` command as it happened to me twice :)\n> \n> Change with a good intention, it seems.\n> \n> > The patches are divided into two parts. First part introduces\n> > the new marker to the worktree list command and small documentation\n> > change. And the second adds one test case into t2402 to test\n> > if the (locked) text will be properly set for a locked worktree, and\n> > not mistankely set to a unlocked or master worktree.\n> \n> Probably they belong together in a single patch (I am saying this\n> after only seeing the above five lines, without reading either of\n> these two patches, so there may be some things in them that makes it\n> make sense to have them separate).\n> \n\nYes, I believe it make sense to have them in a single patch.\n\n> > This is the output of the worktree list with locked marker:\n> >\n> > $ git worktree list\n> > /repo/to/main abc123 [master]\n> > /path/to/unlocked-worktree1 456def [brancha]\n> > /path/to/locked-worktree 123abc (detached HEAD) (locked)\n> \n> Looks OK to me\n> \n> > This patches are marked with RFC mainly due to:\n> >\n> > - This will change the default behaviour of the worktree list, I am\n> > not sure whether will be better to make this tuned via a config\n> > and/or a git parameter. (assuming this change is a good idea ;) )\n> \n> The default output is meant for human consumption (scripts that want\n> to read from the command and expect stable output would be using the\n> \"--porcelain\" option).\n> \n> The ideal case is a new output is universally useful for everybody,\n> in which case we can just change it without any new configuration or\n> command line option.\n\nThanks for the explanation, will keep that in mind.\n\n> \n> > - Perhaps the `(locked)` marker is not the best suitable way to output\n> > this information and we might need to come with a better way.\n> \n> It looks good enough to me. I am not qualified to have a strong\n> opinion on this part, as I do not use the command all that often.\n> \n> $ git shortlog --no-merges --since=18.months builtin/worktree.c\n> \n> tells me that Eric Sunshine (CC'ed) may be a good source of wisdom\n> on this command.\n> \n\nThank you, I will keep Eric Sunshine in the loop as well.\n\n> > - I am a new contributor to the code base, still learning a lot of git\n> > internals data structure and commands. Likely this patch will require\n> > updates.\n> \n> Welcome.\n\nThank you.\n\n> \n> > Rafael Silva (2):\n> > teach `list` to mark locked worktree\n> > t2402: add test to locked linked worktree marker\n> >\n> > Documentation/git-worktree.txt | 5 +++--\n> > builtin/worktree.c | 6 +++++-\n> > t/t2402-worktree-list.sh | 13 +++++++++++++\n> > 3 files changed, 21 insertions(+), 3 deletions(-)\n"},{"id":"406647","messageId":"20200929213603.GB1336@contrib-buster.localdomain","threadId":"54317","inReplyTo":"xmqq8sctlgzx.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 1/2] worktree: teach `list` to mark locked worktree","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-09-29T21:36:03Z","receivedAt":"2020-09-29T21:36:09Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"On Mon, Sep 28, 2020 at 02:37:54PM -0700, Junio C Hamano wrote:\n> Rafael Silva writes:\n> \n> > The output of `worktree list` command is extended to mark a locked\n> > worktree with `(locked)` text. This is used to communicate to the\n> > user that a linked worktree is locked instead of learning only when\n> > attempting to remove it.\n> >\n> > This is the output of the worktree list with locked marker:\n> >\n> > $ git worktree list\n> > /repo/to/main abc123 [master]\n> > /path/to/unlocked-worktree1 456def [brancha]\n> > /path/to/locked-worktree 123abc (detached HEAD) (locked)\n> \n> \n> In our log message, we tend NOT to say \"This commit does X\" or \"X is\n> done\", because such a statement is often insufficient to illustrate\n> if the commit indeed does X, and explain why it is a good thing to\n> do X in the first place.\n> \n> Instead, we \n> \n> - first explain that the current system does not do X (in present\n> tense, so we do NOT say \"previously we did not do X\"), then\n> \n> - explain why doing X would be a good thing, and finally\n> \n> - give an order to the codebase to start doing X.\n> \n> \n> For this change, it might look like this:\n> \n> The \"git worktree list\" shows the absolute path to the working\n> tree, the commit that is checked out and the name of the branch.\n> It is not immediately obvious which of the worktrees, if any,\n> are locked.\n> \n> \"git worktree remove\" refuses to remove a locked worktree with\n> an error message. If \"git worktree list\" told which worktrees\n> are locked in its output, the user would not even attempt to\n> remove such a worktree.\n> \n> Teach \"git worktree list\" to append \"(locked)\" to its output.\n> The output from the command becomes like so:\n> \n> $ git worktree list\n> /repo/to/main abc123 [master]\n> /path/to/unlocked-worktree1 456def [brancha]\n> /path/to/locked-worktree 123abc (detached HEAD) (locked)\n>\n\nThank you for such detailed explanation. I totally agree that it seems\nmuch better to organise the commit message this way, I will definitely\ninclude this message (or something very similar) when resending these patches.\n\n> \n> > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\n> > index 32e8440cde..a3781dd664 100644\n> > --- a/Documentation/git-worktree.txt\n> > +++ b/Documentation/git-worktree.txt\n> > @@ -96,8 +96,9 @@ list::\n> > \n> > List details of each working tree. The main working tree is listed first,\n> > followed by each of the linked working trees. The output details include\n> > -whether the working tree is bare, the revision currently checked out, and the\n> > -branch currently checked out (or \"detached HEAD\" if none).\n> > +whether the working tree is bare, the revision currently checked out, the\n> > +branch currently checked out (or \"detached HEAD\" if none), and whether\n> > +the worktree is locked.\n> \n> At the first glance, the above gave me an impression that you'd be\n> adding \"(unlocked)\" or \"(locked)\" for each working tree, but that is\n> not the case. How about keeping the original sentence intact, and\n> adding something like \"For a locked worktree, the marker (locked) is\n> also shown at the end\"?\n\nYes, it sounds good.\n\n> \n> > diff --git a/builtin/worktree.c b/builtin/worktree.c\n> > index 99abaeec6c..8ad2cdd2f9 100644\n> > --- a/builtin/worktree.c\n> > +++ b/builtin/worktree.c\n> > @@ -676,8 +676,12 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)\n> > \t\t} else\n> > \t\t\tstrbuf_addstr(&sb, \"(error)\");\n> > \t}\n> > -\tprintf(\"%s\\n\", sb.buf);\n> > \n> > +\tif (!is_main_worktree(wt) &&\n> > +\t worktree_lock_reason(wt))\n> > +\t\tstrbuf_addstr(&sb, \" (locked)\");\n> \n> Is this because for the primary worktree, worktree_lock_reason()\n> will always yield true?\n> \n> ... goes and looks ...\n> \n> Ah, OK, the callers are not even allowed to ask the question on the\n> primary one. That's a bit strange API but OK.\n> \n> Writing that on a single line would perfectly be readable, by the\n> way.\n> \n> \tif (!is_main_worktree(wt) && worktree_lock_reason(wt))\n> \t\tstrbuf_addstr(&sb, \" (locked)\");\n\nAgreed. will be much better to have it in a single line.\n\n> \n> > +\tprintf(\"%s\\n\", sb.buf);\n> > \tstrbuf_release(&sb);\n> > }\n"},{"id":"406648","messageId":"20200929213747.GC1336@contrib-buster.localdomain","threadId":"54317","inReplyTo":"xmqq3631lg8f.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 2/2] t2402: add test to locked linked worktree marker","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-09-29T21:37:47Z","receivedAt":"2020-09-29T21:37:53Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"On Mon, Sep 28, 2020 at 02:54:24PM -0700, Junio C Hamano wrote:\n> Rafael Silva writes:\n> \n> > Test the output of the `worktree list` command to show when\n> > a linked worktree is locked and test to not mistakenly\n> > mark main or unlocked worktrees.\n> >\n> > Signed-off-by: Rafael Silva \n> > ---\n> > t/t2402-worktree-list.sh | 13 +++++++++++++\n> > 1 file changed, 13 insertions(+)\n> \n> I think this should be part of [1/2], as the change necessary to\n> implement the new feature is small enough that there is no reason\n> to split the test part out.\n\nSounds good\n\n> \n> > diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh\n> > index 52585ec2aa..07bd9a3350 100755\n> > --- a/t/t2402-worktree-list.sh\n> > +++ b/t/t2402-worktree-list.sh\n> > @@ -61,6 +61,19 @@ test_expect_success '\"list\" all worktrees --porcelain' '\n> > \ttest_cmp expect actual\n> > '\n> > \n> > +test_expect_success 'show locked worktree with (locked)' '\n> > +\techo \"$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]\" >expect &&\n> > +\ttest_when_finished \"rm -rf locked unlocked out actual expect && git worktree prune\" &&\n> > +\tgit worktree add --detach locked master &&\n> > +\tgit worktree add --detach unlocked master &&\n> > +\tgit worktree lock locked &&\n> > +\techo \"$(git -C locked rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD) (locked)\" >>expect &&\n> > +\techo \"$(git -C unlocked rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)\" >>expect &&\n> > +\tgit worktree list >out &&\n> > +\tsed \"s/ */ /g\" actual &&\n> > +\ttest_cmp expect actual\n> > +'\n> \n> This seems to prescribe the output from the command too strictly\n> (you do avoid being overly too strict by removing the indentation\n> with 's/ */ /g' though). \n> \n> If the leading path to the $TRASH_DIRECTORY has two or more\n> consecutive SPs (and that is not something under our control), the\n> 'expect' file would keep such a double-SP, but such a double-SP in\n> 'out' would have been squashed out in the 'actual' file.\n> \n> I wonder if\n> \n> \tgrep '/locked *[0-9a-f].* (locked)' out &&\n> \t! grep '/unlocked *[0-9a-f].* (locked)' out\n> \n> might be a better way to test? That is\n> \n> - we do not care what the leading directories are called\n> - we do not care what branch is checked out or how they are presented\n> - we care the one that ends with /locked is (locked)\n> - we care the one that ends with /unlocked is not (locked)\n> \n> After all, this new test piece is not about verifying that the\n> object name or branch name is correct.\n\nSounds good and I agree with the all, and seems even easier\nto understand the test code this way. Will address this change on the next patch,\nalthough I'm not sure whether somebody could run into any issues when\nrunning the `grep` command in other platforms. I'll look into it.\n"},{"id":"406659","messageId":"CAPig+cQXkP8vTNR+LJ4fZRT-an0vEgKxcFpfi+aQ-BdipTgq=A@mail.gmail.com","threadId":"54317","inReplyTo":"20200928154953.30396-1-rafaeloliveira.cs@gmail.com","subject":"Re: [RFC PATCH 0/2] teach `worktree list` to mark locked worktrees","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-30T07:19:27Z","receivedAt":"2020-09-30T07:19:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 28, 2020 at 11:50 AM Rafael Silva\n wrote:\n> This patch series introduces a new information on the git `worktree list`\n> command output, to mark when a worktree is locked with a (locked) text mark.\n\nThanks for working on this. \"locked\" is one of several additional\nannotations to the output of \"git worktree list\" which have long been\nenvisioned. For reference, here are some earlier messages related to\nthis topic:\n\n[1]: https://lore.kernel.org/git/CAPig+cTTrv2C7JLu1dr4+N8xo+7YQ+deiwLDA835wBGD6fhS1g@mail.gmail.com/\n[2]: https://lore.kernel.org/git/CAPig+cQF6V8HNdMX5AZbmz3_w2WhSfA4SFfNhQqxXBqPXTZL+w@mail.gmail.com/\n[3]: https://lore.kernel.org/git/CAPig+cSGXqJuaZPhUhOVX5X=LMrjVfv8ye_6ncMUbyKox1i7QA@mail.gmail.com/\n[4]: https://lore.kernel.org/git/CAPig+cTitWCs5vB=0iXuUyEY22c0gvjXvY1ZtTT90s74ydhE=A@mail.gmail.com/\n\nI'll leave a few review comments to supplement those already by Junio.\n\n> This is the output of the worktree list with locked marker:\n>\n> $ git worktree list\n> /repo/to/main abc123 [master]\n> /path/to/unlocked-worktree1 456def [brancha]\n> /path/to/locked-worktree 123abc (detached HEAD) (locked)\n\nIn [2], I gave an example of output similar to this but without\nencapsulating \"locked\" within parentheses:\n\n % git worktree list\n giggle 89ea799ffc [master]\n ../bobble f172cb543d [feature1] locked\n ../fumple 6453c84b7d (detached HEAD) prunable\n\nI omitted the parentheses partly due to the extra noise they\nintroduce, but mostly because I foresaw that a single entry might\neventually have multiple annotations, for instance:\n\n % git worktree list\n giggle 89ea799ffc [master]\n ../bobble f172cb543d [feature1] locked prunable\n\nin which case, the eyes glide over \"locked prunable\" a bit more easily\nthan over \"(locked) (prunable)\" or \"(locked, prunable)\" or some such.\nGenerally speaking, \"the less noise, the better\".\n\n> This patches are marked with RFC mainly due to:\n>\n> - Perhaps the `(locked)` marker is not the best suitable way to output\n> this information and we might need to come with a better way.\n\nTaking [2] into consideration, I think it's fine to annotate the line\nwith \"locked\" (sans the parentheses), and it's compatible with the the\nverbose mode, also proposed by [2]:\n\n % git worktree list -v\n giggle 89ea799ffc [master]\n ../bobble f172cb543d [feature1]\n locked: worktree on removable media\n ../fumple 6453c84b7d (detached HEAD)\n prunable: directory does not exist\n\nin which case the short \"locked\" annotation gets moved to the next\nline, indented, and expanded to include the reason, if available\n(otherwise would probably not be moved to the next line).\n\nI'm not suggesting that this patch series implement verbose mode, but\nbring it to attention to make sure we don't paint ourselves into a\ncorner when deciding how the \"locked\" annotation should be presented.\n\n> - I am a new contributor to the code base, still learning a lot of git\n> internals data structure and commands. Likely this patch will require\n> updates.\n\nUnder normal circumstances, I would be hesitant to accept a\ncontribution which makes an addition to the human-consumable \"git\nworktree list\" output without also making the corresponding addition\nto the --porcelain format. Thus, for instance, I would expect the\nporcelain format to be updated, as well, to produce output such as\nthis (taken from [4]):\n\n worktree /blah\n branch refs/heads/blah\n locked Sneaker-net removable storage\\nNot always mounted\n\nThat's a bit complicated because the lock reason may need escaping if\nit contains special characters (such as the newline in the example).\nThus, I'm a bit hesitant to expect such a change from a newcomer.\n\nMore problematic, though, with regard to the porcelain format is that\nthe documentation is so woefully under-specified, as explained in [4],\nthat some people may interpret it as meaning that no additional\ninformation can be added. I'm not particularly sympathetic to that\nview since the intention from the start was that the porcelain format\nshould be extensible[4], thus adding new attributes should be allowed.\nBut I'm hesitant to ask a newcomer to undertake the task of addressing\nthese shortcomings. As such, I think it may be okay merely to change\nthe human-consumable output as this series does, and leave porcelain\noutput for a later date if someone wants to tackle it.\n\nA reason that it would be nice to address the shortcomings of\nporcelain format is because there are several additional pieces of\ninformation it could be providing. Summarizing from [1], in addition\nto the worktree path, its head, checked out branch, whether its bare\nor detached, for each worktree, porcelain could also show:\n\n * whether it is locked\n - the lock reason (if available)\n - and whether the worktree is currently accessible (mounted)\n * whether it can be pruned\n - and the prune reason if so\n * worktree ID (the of .git/worktrees//)\n"},{"id":"406661","messageId":"CAPig+cQHDuWy1vc_ngXbMQZQ=a9fd6S5_cCU-2sb_+Te5aEOhw@mail.gmail.com","threadId":"54317","inReplyTo":"xmqq8sctlgzx.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 1/2] worktree: teach `list` to mark locked worktree","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-30T07:35:07Z","receivedAt":"2020-09-30T07:35:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 28, 2020 at 5:38 PM Junio C Hamano wrote:\n> Rafael Silva writes:\n> > The output of `worktree list` command is extended to mark a locked\n> > worktree with `(locked)` text. This is used to communicate to the\n> > user that a linked worktree is locked instead of learning only when\n> > attempting to remove it.\n>\n> For this change, it might look like this:\n>\n> The \"git worktree list\" shows the absolute path to the working\n> tree, the commit that is checked out and the name of the branch.\n> It is not immediately obvious which of the worktrees, if any,\n> are locked.\n>\n> \"git worktree remove\" refuses to remove a locked worktree with\n> an error message. If \"git worktree list\" told which worktrees\n> are locked in its output, the user would not even attempt to\n> remove such a worktree.\n\nNicely written. I might end the final sentence like this:\n\n ... the user would not even attempt to remove such a worktree or\n would know to use `git worktree remove -f -f `.\n\n> > diff --git a/builtin/worktree.c b/builtin/worktree.c\n> > @@ -676,8 +676,12 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)\n> > + if (!is_main_worktree(wt) &&\n> > + worktree_lock_reason(wt))\n> > + strbuf_addstr(&sb, \" (locked)\");\n>\n> Is this because for the primary worktree, worktree_lock_reason()\n> will always yield true?\n>\n> ... goes and looks ...\n>\n> Ah, OK, the callers are not even allowed to ask the question on the\n> primary one. That's a bit strange API but OK.\n\nThat is indeed a slightly hostile API, and it wouldn't hurt to change\nit simply to return 'false' for the main worktree, but that's not\nsomething this patch series need tackle.\n"},{"id":"406663","messageId":"CAPig+cSa4JZ0SaM9p+Jfvv_5h8mexRcKdiz=hSi37BG_sLsk7Q@mail.gmail.com","threadId":"54317","inReplyTo":"20200928154953.30396-3-rafaeloliveira.cs@gmail.com","subject":"Re: [RFC PATCH 2/2] t2402: add test to locked linked worktree marker","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-30T08:06:28Z","receivedAt":"2020-09-30T08:06:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 28, 2020 at 11:50 AM Rafael Silva\n wrote:\n> Test the output of the `worktree list` command to show when\n> a linked worktree is locked and test to not mistakenly\n> mark main or unlocked worktrees.\n\nIn addition to Junio's review comments...\n\n> Signed-off-by: Rafael Silva \n> ---\n> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh\n> @@ -61,6 +61,19 @@ test_expect_success '\"list\" all worktrees --porcelain' '\n> +test_expect_success 'show locked worktree with (locked)' '\n\nExisting test titles in this script seem to follow a particular\npattern (for the most part). Perhaps this could say instead:\n\n test_expect_success '\"list\" all worktrees with \"locked\"' '\n\n> + echo \"$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]\" >expect &&\n\nI see that this is following existing practice in this script of\npiling up $(git ...) invocations which can lose the individual exit\ncodes, so I can't complain about it, but I'll note that these days\nwe'd more likely than not try to preserve the exit codes, perhaps like\nthis:\n\n path=$(git rev-parse --show-toplevel) &&\n rev=$(git rev-parse --short HEAD) &&\n sym=$(git symbolic-ref --short HEAD) &&\n echo \"$path $rev [$sym]\" >expect &&\n\nHowever, that gets verbose since you're doing it for multiple lines (below).\n\n> + test_when_finished \"rm -rf locked unlocked out actual expect && git worktree prune\" &&\n> + git worktree add --detach locked master &&\n> + git worktree add --detach unlocked master &&\n> + git worktree lock locked &&\n> + echo \"$(git -C locked rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD) (locked)\" >>expect &&\n> + echo \"$(git -C unlocked rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)\" >>expect &&\n> + git worktree list >out &&\n> + sed \"s/ */ /g\" actual &&\n> + test_cmp expect actual\n> +'\n\nI realize that Junio proposed an alternate simpler approach which\nchecks specifically the attribute in which you are interested, but\nhere's yet another (typed-in-email) approach you could use:\n\n test_when_finished \"...\" &&\n git worktree add --detach locked master &&\n git worktree add --detach unlocked master &&\n git worktree list >out &&\n sed '/\\/locked/s/$/ (locked)/' out >expect &&\n git worktree lock locked &&\n git worktree list >actual &&\n test_cmp expect actual\n"},{"id":"406826","messageId":"20201002162802.GA15646@contrib-buster.localdomain","threadId":"54317","inReplyTo":"CAPig+cQXkP8vTNR+LJ4fZRT-an0vEgKxcFpfi+aQ-BdipTgq=A@mail.gmail.com","subject":"Re: [RFC PATCH 0/2] teach `worktree list` to mark locked worktrees","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-10-02T16:28:02Z","receivedAt":"2020-10-02T16:28:08Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"On Wed, Sep 30, 2020 at 03:19:27AM -0400, Eric Sunshine wrote:\n\n> On Mon, Sep 28, 2020 at 11:50 AM Rafael Silva\n> wrote:\n> > This patch series introduces a new information on the git `worktree list`\n> > command output, to mark when a worktree is locked with a (locked) text mark.\n> \n> Thanks for working on this. \"locked\" is one of several additional\n> annotations to the output of \"git worktree list\" which have long been\n> envisioned. For reference, here are some earlier messages related to\n> this topic:\n> \n> [1]: https://lore.kernel.org/git/CAPig+cTTrv2C7JLu1dr4+N8xo+7YQ+deiwLDA835wBGD6fhS1g@mail.gmail.com/\n> [2]: https://lore.kernel.org/git/CAPig+cQF6V8HNdMX5AZbmz3_w2WhSfA4SFfNhQqxXBqPXTZL+w@mail.gmail.com/\n> [3]: https://lore.kernel.org/git/CAPig+cSGXqJuaZPhUhOVX5X=LMrjVfv8ye_6ncMUbyKox1i7QA@mail.gmail.com/\n> [4]: https://lore.kernel.org/git/CAPig+cTitWCs5vB=0iXuUyEY22c0gvjXvY1ZtTT90s74ydhE=A@mail.gmail.com/\n\nThank you for all this reference, it's really helpful. It is nice to see\nthat we already have few discussion on this topic that we can use and\nwork on top of that.\n\nSorry for the bit late response.\n\n> \n> I'll leave a few review comments to supplement those already by Junio.\n> \n> > This is the output of the worktree list with locked marker:\n> >\n> > $ git worktree list\n> > /repo/to/main abc123 [master]\n> > /path/to/unlocked-worktree1 456def [brancha]\n> > /path/to/locked-worktree 123abc (detached HEAD) (locked)\n> \n> In [2], I gave an example of output similar to this but without\n> encapsulating \"locked\" within parentheses:\n> \n> % git worktree list\n> giggle 89ea799ffc [master]\n> ../bobble f172cb543d [feature1] locked\n> ../fumple 6453c84b7d (detached HEAD) prunable\n> \n> I omitted the parentheses partly due to the extra noise they\n> introduce, but mostly because I foresaw that a single entry might\n> eventually have multiple annotations, for instance:\n> \n> % git worktree list\n> giggle 89ea799ffc [master]\n> ../bobble f172cb543d [feature1] locked prunable\n> \n> in which case, the eyes glide over \"locked prunable\" a bit more easily\n> than over \"(locked) (prunable)\" or \"(locked, prunable)\" or some such.\n> Generally speaking, \"the less noise, the better\".\n\nAgreed. Without the parentheses is cleaner and it make easier to\nextend to future annotations. I will drop the paretheses for the \nnext patch version.\n\n> > This patches are marked with RFC mainly due to:\n> >\n> > - Perhaps the `(locked)` marker is not the best suitable way to output\n> > this information and we might need to come with a better way.\n> \n> Taking [2] into consideration, I think it's fine to annotate the line\n> with \"locked\" (sans the parentheses), and it's compatible with the the\n> verbose mode, also proposed by [2]:\n> \n> % git worktree list -v\n> giggle 89ea799ffc [master]\n> ../bobble f172cb543d [feature1]\n> locked: worktree on removable media\n> ../fumple 6453c84b7d (detached HEAD)\n> prunable: directory does not exist\n> \n> in which case the short \"locked\" annotation gets moved to the next\n> line, indented, and expanded to include the reason, if available\n> (otherwise would probably not be moved to the next line).\n\nThe verbose mode seems like a very good extension for the worktree list\nwith the extended reason for `locked/prunable` on the next line.\n\n> \n> I'm not suggesting that this patch series implement verbose mode, but\n> bring it to attention to make sure we don't paint ourselves into a\n> corner when deciding how the \"locked\" annotation should be presented.\n> \n\nDoing a little investigation on the code, it seems the machinery for checking\nwhether a worktree is prunable it seems is already there implemented\non the `should_prune_worktree()`.\n\nIn such case, I would love to get started working on a bigger patch that\nwill implemented not only the annotation, but the verbose mode as well.\nSpecially because I was also thinking about how to make the \"locked reason\"\nmessage available to the command output and the design proposed by [2]\nsounds like a good way to manage that.\n\nAdditionally, having the ability to see the annotation and the reason in\ncase you see the annotation seems like more complete work for the intention\nof the patch.\n\nUnless you think that is better to start with the annotation, and some time\nlater addressing the other changes specified by [2].\n\n> [2]: https://lore.kernel.org/git/CAPig+cQF6V8HNdMX5AZbmz3_w2WhSfA4SFfNhQqxXBqPXTZL+w@mail.gmail.com/\n\n> > - I am a new contributor to the code base, still learning a lot of git\n> > internals data structure and commands. Likely this patch will require\n> > updates.\n> \n> Under normal circumstances, I would be hesitant to accept a\n> contribution which makes an addition to the human-consumable \"git\n> worktree list\" output without also making the corresponding addition\n> to the --porcelain format. Thus, for instance, I would expect the\n> porcelain format to be updated, as well, to produce output such as\n> this (taken from [4]):\n> \n> worktree /blah\n> branch refs/heads/blah\n> locked Sneaker-net removable storage\\nNot always mounted\n> \n> That's a bit complicated because the lock reason may need escaping if\n> it contains special characters (such as the newline in the example).\n> Thus, I'm a bit hesitant to expect such a change from a newcomer.\n> \n> More problematic, though, with regard to the porcelain format is that\n> the documentation is so woefully under-specified, as explained in [4],\n> that some people may interpret it as meaning that no additional\n> information can be added. I'm not particularly sympathetic to that\n> view since the intention from the start was that the porcelain format\n> should be extensible[4], thus adding new attributes should be allowed.\n> But I'm hesitant to ask a newcomer to undertake the task of addressing\n> these shortcomings. As such, I think it may be okay merely to change\n> the human-consumable output as this series does, and leave porcelain\n> output for a later date if someone wants to tackle it.\n> \n> A reason that it would be nice to address the shortcomings of\n> porcelain format is because there are several additional pieces of\n> information it could be providing. Summarizing from [1], in addition\n> to the worktree path, its head, checked out branch, whether its bare\n> or detached, for each worktree, porcelain could also show:\n> \n> * whether it is locked\n> - the lock reason (if available)\n> - and whether the worktree is currently accessible (mounted)\n> * whether it can be pruned\n> - and the prune reason if so\n> * worktree ID (the of .git/worktrees//)\n\nThank you for the detailed explanation. much appreciated.\n\nThat something that can also work on. But I agreed that it could be bit\nmore work for a newcomer. I was thinking that I can split the work in \nthree series of patches. \n\n 1. Implementing the annotation for the standards \"list\" command, implementing\n not only the locked but the prunable as on aforementioned in [2].\n\n 2. A second series of patch that will introduce the verbose as defined in [2]\n\n 3. Third and final series that extend the porcelain format.\n\nI would like to kindly ask your opnion on this. Whether you think it will\nbe a good idea to implement all these changes this way and I can start\nworking on that.\n\nI will change this series to become the first part of annotations, specially\nbecause after reading your response and references, it seems this will be\nmuch complete functionality that I would like to have on Git.\n"},{"id":"407253","messageId":"CAPig+cR8D13cM8OewRVYfg7wNjVC05tVQw80-dm4B5XPmjHJWw@mail.gmail.com","threadId":"54317","inReplyTo":"20201002162802.GA15646@contrib-buster.localdomain","subject":"Re: [RFC PATCH 0/2] teach `worktree list` to mark locked worktrees","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-10-09T22:50:02Z","receivedAt":"2020-10-09T22:50:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Oct 2, 2020 at 12:28 PM Rafael Silva\n wrote:\n> On Wed, Sep 30, 2020 at 03:19:27AM -0400, Eric Sunshine wrote:\n> > [...] For reference, here are some earlier messages related to\n> > this topic:\n>\n> Thank you for all this reference, it's really helpful. It is nice to see\n> that we already have few discussion on this topic that we can use and\n> work on top of that.\n>\n> Sorry for the bit late response.\n\nLikewise.\n\n> > I'm not suggesting that this patch series implement verbose mode, but\n> > bring it to attention to make sure we don't paint ourselves into a\n> > corner when deciding how the \"locked\" annotation should be presented.\n>\n> Doing a little investigation on the code, it seems the machinery for checking\n> whether a worktree is prunable it seems is already there implemented\n> on the `should_prune_worktree()`.\n\nYes, when I mentioned that in [1], I envisioned\nshould_prune_worktree() being moved from builtin/worktree.c to\ntop-level worktree.c and possibly generalized a bit if necessary.\n\nOne thing to note is that should_prune_worktree() is somewhat\nexpensive, so we'd probably want to make determination of \"prunable\nreason\" lazy, much like the lock reason is retrieved lazily rather\nthan doing it when get_worktrees() is called. Thus, like the lock\nreason, the prunable reason would be accessed indirectly via a\nfunction, say worktree_prunable_reason(), rather than directly from\n'struct worktree'.\n\n[1]: https://lore.kernel.org/git/CAPig+cTTrv2C7JLu1dr4+N8xo+7YQ+deiwLDA835wBGD6fhS1g@mail.gmail.com/\n\n> In such case, I would love to get started working on a bigger patch that\n> will implemented not only the annotation, but the verbose mode as well.\n> Specially because I was also thinking about how to make the \"locked reason\"\n> message available to the command output and the design proposed by [2]\n> sounds like a good way to manage that.\n\nI'd be happy to see that implemented.\n\n> Additionally, having the ability to see the annotation and the reason in\n> case you see the annotation seems like more complete work for the intention\n> of the patch.\n>\n> Unless you think that is better to start with the annotation, and some time\n> later addressing the other changes specified by [2].\n\nWhatever you feel comfortable tackling is fine. The simple \"locked\"\nannotation is nicely standalone, so it could be resubmitted with the\nchanges suggested by reviewers, and graduate without waiting for the\nmore complex tasks which could be done as follow-up series. Or, expand\nthe current series to tackle verbose mode and/or prunable status or\nboth or any combination.\n\n> > A reason that it would be nice to address the shortcomings of\n> > porcelain format is because there are several additional pieces of\n> > information it could be providing. Summarizing from [1], in addition\n> > to the worktree path, its head, checked out branch, whether its bare\n> > or detached, for each worktree, porcelain could also show:\n> >\n> > * whether it is locked\n> > - the lock reason (if available)\n> > - and whether the worktree is currently accessible (mounted)\n> > * whether it can be pruned\n> > - and the prune reason if so\n> > * worktree ID (the of .git/worktrees//)\n>\n> That something that can also work on. But I agreed that it could be bit\n> more work for a newcomer. I was thinking that I can split the work in\n> three series of patches.\n>\n> 1. Implementing the annotation for the standards \"list\" command, implementing\n> not only the locked but the prunable as on aforementioned in [2].\n>\n> 2. A second series of patch that will introduce the verbose as defined in [2]\n>\n> 3. Third and final series that extend the porcelain format.\n>\n> I would like to kindly ask your opnion on this. Whether you think it will\n> be a good idea to implement all these changes this way and I can start\n> working on that.\n\nSuch an organization would be fine. Tackle what you feel is\nappropriate and what \"scratches your itch\". Breaking the changes down\ninto smaller chunks, as you propose, also helps reviewers since it's\neasier to review a shorter series than a long one.\n\n> I will change this series to become the first part of annotations, specially\n> because after reading your response and references, it seems this will be\n> much complete functionality that I would like to have on Git.\n\nMakes sense.\n"},{"id":"407264","messageId":"20201010185521.23032-1-rafaeloliveira.cs@gmail.com","threadId":"54317","inReplyTo":"20200928154953.30396-1-rafaeloliveira.cs@gmail.com","subject":"[PATCH v2 0/1] Teach \"worktree list\" to annotate locked worktrees","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-10-10T18:55:20Z","receivedAt":"2020-10-10T22:56:08Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"This patch introduces a new information on the \"git worktree list\"\ncommand output to annotate when a worktree is locked with a \"locked\" text.\n\nThe intent is to improve the user experience as the \"git worktree remove\"\nrefuses to remove a locked worktree. By adding the \"locked\" annotation\nto the \"git worktree list\" command, the user would not attempt to remove\na worktree or would know how use \"git worktree remove -f -f \"\nto force a worktree removal.\n\nThe output of the \"worktree list\" command becomes:\n\n $ git worktree list\n /path/to/main abc123 [master]\n /path/to/worktree 456def [brancha]\n /path/to/locked-worktree 123abc (detached HEAD) locked\n\nChanges since v1:\n\n * Drooped the parenthesis in \"(locked)\" to reduce the noise and allow\n easier extensions of more annotations design proposed in [2].\n * Rewrite of the commit message with a much better one\n * Simplification of the test added to `t2402` with only caring about\n the \"locked\" annotation at the end of the \"worktree list\" output.\n\n[2]: https://lore.kernel.org/git/CAPig+cQF6V8HNdMX5AZbmz3_w2WhSfA4SFfNhQqxXBqPXTZL+w@mail.gmail.com/\n\nThank you Junio C Hamano and Eric Sunshine for the detailed review and\nhelping with this patch.\n\nRafael Silva (1):\n worktree: teach `list` to annotate locked worktree\n\n Documentation/git-worktree.txt | 3 ++-\n builtin/worktree.c | 5 ++++-\n t/t2402-worktree-list.sh | 10 ++++++++++\n 3 files changed, 16 insertions(+), 2 deletions(-)\n\n-- \n2.29.0.rc0.253.gc238dab514\n\n"},{"id":"407265","messageId":"20201010190610.GA23099@contrib-buster.localdomain","threadId":"54317","inReplyTo":"CAPig+cR8D13cM8OewRVYfg7wNjVC05tVQw80-dm4B5XPmjHJWw@mail.gmail.com","subject":"Re: [RFC PATCH 0/2] teach `worktree list` to mark locked worktrees","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-10-10T19:06:10Z","receivedAt":"2020-10-10T22:56:15Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"On Fri, Oct 09, 2020 at 06:50:02PM -0400, Eric Sunshine wrote:\n> >\n> > Sorry for the bit late response.\n> \n> Likewise.\n> \n> >\n> > Doing a little investigation on the code, it seems the machinery for checking\n> > whether a worktree is prunable it seems is already there implemented\n> > on the `should_prune_worktree()`.\n> \n> Yes, when I mentioned that in [1], I envisioned\n> should_prune_worktree() being moved from builtin/worktree.c to\n> top-level worktree.c and possibly generalized a bit if necessary.\n> \n> One thing to note is that should_prune_worktree() is somewhat\n> expensive, so we'd probably want to make determination of \"prunable\n> reason\" lazy, much like the lock reason is retrieved lazily rather\n> than doing it when get_worktrees() is called. Thus, like the lock\n> reason, the prunable reason would be accessed indirectly via a\n> function, say worktree_prunable_reason(), rather than directly from\n> 'struct worktree'.\n> \n> [1]: https://lore.kernel.org/git/CAPig+cTTrv2C7JLu1dr4+N8xo+7YQ+deiwLDA835wBGD6fhS1g@mail.gmail.com/\n\nAppreciate the tip, I will be working on the prunable annotations, verbose\nand other information that was proposed previously for the \"worktree list\"\ncommand.\n\n> > Additionally, having the ability to see the annotation and the reason in\n> > case you see the annotation seems like more complete work for the intention\n> > of the patch.\n> >\n> > Unless you think that is better to start with the annotation, and some time\n> > later addressing the other changes specified by [2].\n> \n> Whatever you feel comfortable tackling is fine. The simple \"locked\"\n> annotation is nicely standalone, so it could be resubmitted with the\n> changes suggested by reviewers, and graduate without waiting for the\n> more complex tasks which could be done as follow-up series. Or, expand\n> the current series to tackle verbose mode and/or prunable status or\n> both or any combination.\n> \n\nThanks. I've just resubmitted the \"locked\" annotation patch, as you said,\nit's nice standalone and can be integrated and hopefully will be already\nuseful for other git users and soon (hopefully :) ) will submit new patches\nfor the other changes as proposed by [1].\n\n[1]: https://lore.kernel.org/git/CAPig+cTTrv2C7JLu1dr4+N8xo+7YQ+deiwLDA835wBGD6fhS1g@mail.gmail.com/\n"},{"id":"407286","messageId":"20201010185521.23032-2-rafaeloliveira.cs@gmail.com","threadId":"54317","inReplyTo":"20201010185521.23032-1-rafaeloliveira.cs@gmail.com","subject":"[PATCH v2 1/1] worktree: teach `list` to annotate locked worktree","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-10-10T18:55:21Z","receivedAt":"2020-10-10T23:11:04Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"The \"git worktree list\" shows the absolute path to the working tree,\nthe commit that is checked out and the name of the branch. It is not\nimmediately obvious which of the worktrees, if any, are locked.\n\n\"git worktree remove\" refuses to remove a locked worktree with\nan error message. If \"git worktree list\" told which worktrees\nare locked in its output, the user would not even attempt to\nremove such a worktree or would know how to use\n`git worktree remove -f -f `\n\nTeach \"git worktree list\" to append \"locked\" to its output.\nThe output from the command becomes like so:\n\n $ git worktree list\n /path/to/main abc123 [master]\n /path/to/worktree 456def (detached HEAD)\n /path/to/locked-worktree 123abc (detached HEAD) locked\n\nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \nSigned-off-by: Rafael Silva \n---\n Documentation/git-worktree.txt | 3 ++-\n builtin/worktree.c | 5 ++++-\n t/t2402-worktree-list.sh | 10 ++++++++++\n 3 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex 32e8440cde..40ef4c18c5 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -97,7 +97,8 @@ list::\n List details of each working tree. The main working tree is listed first,\n followed by each of the linked working trees. The output details include\n whether the working tree is bare, the revision currently checked out, and the\n-branch currently checked out (or \"detached HEAD\" if none).\n+branch currently checked out (or \"detached HEAD\" if none). For a locked\n+worktree the `locked` annotation is also shown.\n \n lock::\n \ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 99abaeec6c..ce56fdaaa9 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -676,8 +676,11 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)\n \t\t} else\n \t\t\tstrbuf_addstr(&sb, \"(error)\");\n \t}\n-\tprintf(\"%s\\n\", sb.buf);\n \n+\tif (!is_main_worktree(wt) && worktree_lock_reason(wt))\n+\t\tstrbuf_addstr(&sb, \" locked\");\n+\n+\tprintf(\"%s\\n\", sb.buf);\n \tstrbuf_release(&sb);\n }\n \ndiff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh\nindex 52585ec2aa..74275407ee 100755\n--- a/t/t2402-worktree-list.sh\n+++ b/t/t2402-worktree-list.sh\n@@ -61,6 +61,16 @@ test_expect_success '\"list\" all worktrees --porcelain' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '\"list\" all worktress with locked annotation' '\n+\ttest_when_finished \"rm -rf locked unlocked out && git worktree prune\" &&\n+\tgit worktree add --detach locked master &&\n+\tgit worktree add --detach unlocked master &&\n+\tgit worktree lock locked &&\n+\tgit worktree list >out &&\n+\tgrep \"/locked *[0-9a-f].* locked\" out &&\n+\t! grep \"/unlocked *[0-9a-f].* locked\" out\n+'\n+\n test_expect_success 'bare repo setup' '\n \tgit init --bare bare1 &&\n \techo \"data\" >file1 &&\n-- \n2.29.0.rc0.253.gc238dab514\n\n"},{"id":"407290","messageId":"CAPig+cTq5tz8m0bCJ3GtCa9yzOMNvd7j4fSJNwO9xjqkfK+YOg@mail.gmail.com","threadId":"54317","inReplyTo":"20201010185521.23032-2-rafaeloliveira.cs@gmail.com","subject":"Re: [PATCH v2 1/1] worktree: teach `list` to annotate locked worktree","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-10-11T06:26:31Z","receivedAt":"2020-10-11T06:26:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Oct 10, 2020 at 2:56 PM Rafael Silva\n wrote:\n> The \"git worktree list\" shows the absolute path to the working tree,\n> the commit that is checked out and the name of the branch. It is not\n> immediately obvious which of the worktrees, if any, are locked.\n>\n> \"git worktree remove\" refuses to remove a locked worktree with\n> an error message. If \"git worktree list\" told which worktrees\n> are locked in its output, the user would not even attempt to\n> remove such a worktree or would know how to use\n> `git worktree remove -f -f `\n\nI would drop \"how\" from \"would know how to\" so it instead reads \"would\nknow to\" since seeing the `locked` annotation only lets the user know\nthat removal must be forced; the `locked` annotation doesn't teach the\nuser _how_ to remove the worktree using force. But, perhaps, my\noriginal suggestion[1], which did not use \"how\", was confusing. Maybe\nit could be worded instead:\n\n ... not even attempt to remove such a worktree, or would\n realize that `git worktree remove -f -f ` is required.\n\nAnyhow, this is a very minor nit about the commit message; not\nnecessarily worth a re-roll. More comments below...\n\n[1]: https://lore.kernel.org/git/CAPig+cQHDuWy1vc_ngXbMQZQ=a9fd6S5_cCU-2sb_+Te5aEOhw@mail.gmail.com/\n\n> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\n> @@ -97,7 +97,8 @@ list::\n> List details of each working tree. The main working tree is listed first,\n> followed by each of the linked working trees. The output details include\n> whether the working tree is bare, the revision currently checked out, and the\n> -branch currently checked out (or \"detached HEAD\" if none).\n> +branch currently checked out (or \"detached HEAD\" if none). For a locked\n> +worktree the `locked` annotation is also shown.\n\nI might have dropped the \"and\" in the final context line and instead\nwritten this as:\n\n ... branch currently checked out (or \"detached HEAD\" if none),\n and \"locked\" if the worktree is locked.\n\nBut not worth a re-roll.\n\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> @@ -676,8 +676,11 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)\n> } else\n> strbuf_addstr(&sb, \"(error)\");\n>\n> + if (!is_main_worktree(wt) && worktree_lock_reason(wt))\n> + strbuf_addstr(&sb, \" locked\");\n\nI was going to ask if \"locked\" should be localizable like this:\n\n strbuf_addf(&sb, \" %s\", _(\"locked\"));\n\nbut I see that none of the other words (\"bare\", \"detached\", \"error\")\nin this function are localizable, so this is fine as-is.\n\nHowever, all of the other human-consumable text emitted by \"git\nworktree\" is localizable, so making these strings localizable, as\nwell, is something that can be added to a To-Do list. Note that I'm\ntalking only about human-consumable \"git worktree list\" output, not\nporcelain format. Also, I'm not suggesting you tackle it, and it's\ncertainly not something that this patch or patch series needs to do;\njust something which someone can tackle in the future.\n\n> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh\n> @@ -61,6 +61,16 @@ test_expect_success '\"list\" all worktrees --porcelain' '\n> +test_expect_success '\"list\" all worktress with locked annotation' '\n> + test_when_finished \"rm -rf locked unlocked out && git worktree prune\" &&\n> + git worktree add --detach locked master &&\n> + git worktree add --detach unlocked master &&\n> + git worktree lock locked &&\n> + git worktree list >out &&\n> + grep \"/locked *[0-9a-f].* locked\" out &&\n> + ! grep \"/unlocked *[0-9a-f].* locked\" out\n> +'\n\nThese grep invocations are a bit loose, thus concern me a little bit.\n\nFirst, in Junio's original example of using grep[2], he had two spaces\nafter the path component, not one as you have here. The two spaces in\nthe regex ensure that there is at least one space separating `/locked`\nand `/unlocked` from the OID hex string, whereas with just one space\nin the regex, as is done here, the space following the path component\nis entirely optional (thus is a less desirable regex).\n\nSecond, because these regexes are not anchored, they could match with\na false-positive if the person's TRASH_DIRECTORY path is something\nlike `/home/proj/unlocked dead locked/git/t/...`. If you anchor the\npattern with `$`, then this problem goes away:\n\n grep \"/locked *[0-9a-f].* locked$\" out &&\n ! grep \"/unlocked *[0-9a-f].* locked$\" out\n\nThird, this is checking only that the first character following the\npath component is a hex digit but then accepts _anything_ before\n\"locked\". The regex can be tightened to allow only hex digits:\n\n grep \"/locked *[0-9a-f][0-9a-f]* locked$\" out &&\n ! grep \"/unlocked *[0-9a-f][0-9a-f]* locked$\" out\n\n[2]: https://lore.kernel.org/git/xmqq3631lg8f.fsf@gitster.c.googlers.com/\n"},{"id":"407291","messageId":"CAPig+cQENxQ7nWwMOdd_Tw=LU=+d_r7n9rqAbjbYyCC7av+gHw@mail.gmail.com","threadId":"54317","inReplyTo":"CAPig+cTq5tz8m0bCJ3GtCa9yzOMNvd7j4fSJNwO9xjqkfK+YOg@mail.gmail.com","subject":"Re: [PATCH v2 1/1] worktree: teach `list` to annotate locked worktree","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-10-11T06:34:42Z","receivedAt":"2020-10-11T06:34:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Oct 11, 2020 at 2:26 AM Eric Sunshine wrote:\n> Third, this is checking only that the first character following the\n> path component is a hex digit but then accepts _anything_ before\n> \"locked\". The regex can be tightened to allow only hex digits:\n>\n> grep \"/locked *[0-9a-f][0-9a-f]* locked$\" out &&\n> ! grep \"/unlocked *[0-9a-f][0-9a-f]* locked$\" out\n\nNevermind this last point. I see that there is other gunk after the\nhex string but before the `locked` annotation, so this suggestion\nbreaks the test. The other two points -- (1) mandatory whitespace\nfollowing the path component, and (2) anchoring the pattern -- would\nbe welcome.\n"},{"id":"407296","messageId":"20201011100406.GA5151@contrib-buster.localdomain","threadId":"54317","inReplyTo":"CAPig+cTq5tz8m0bCJ3GtCa9yzOMNvd7j4fSJNwO9xjqkfK+YOg@mail.gmail.com","subject":"Re: [PATCH v2 1/1] worktree: teach `list` to annotate locked worktree","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-10-11T10:04:06Z","receivedAt":"2020-10-11T10:04:14Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"On Sun, Oct 11, 2020 at 02:26:31AM -0400, Eric Sunshine wrote:\n> On Sat, Oct 10, 2020 at 2:56 PM Rafael Silva\n> wrote:\n> > The \"git worktree list\" shows the absolute path to the working tree,\n> > the commit that is checked out and the name of the branch. It is not\n> > immediately obvious which of the worktrees, if any, are locked.\n> >\n> > \"git worktree remove\" refuses to remove a locked worktree with\n> > an error message. If \"git worktree list\" told which worktrees\n> > are locked in its output, the user would not even attempt to\n> > remove such a worktree or would know how to use\n> > `git worktree remove -f -f `\n> \n> I would drop \"how\" from \"would know how to\" so it instead reads \"would\n> know to\" since seeing the `locked` annotation only lets the user know\n> that removal must be forced; the `locked` annotation doesn't teach the\n> user _how_ to remove the worktree using force. But, perhaps, my\n> original suggestion[1], which did not use \"how\", was confusing. Maybe\n> it could be worded instead:\n> \n> ... not even attempt to remove such a worktree, or would\n> realize that `git worktree remove -f -f ` is required.\n> \n> Anyhow, this is a very minor nit about the commit message; not\n> necessarily worth a re-roll. More comments below...\n> \n> [1]: https://lore.kernel.org/git/CAPig+cQHDuWy1vc_ngXbMQZQ=a9fd6S5_cCU-2sb_+Te5aEOhw@mail.gmail.com/\n\nThe \"would realise ...\" seems clear to me, will change on the\npatch as I will address the test changes aforementioned.\n\n> \n> > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\n> > @@ -97,7 +97,8 @@ list::\n> > List details of each working tree. The main working tree is listed first,\n> > followed by each of the linked working trees. The output details include\n> > whether the working tree is bare, the revision currently checked out, and the\n> > -branch currently checked out (or \"detached HEAD\" if none).\n> > +branch currently checked out (or \"detached HEAD\" if none). For a locked\n> > +worktree the `locked` annotation is also shown.\n> \n> I might have dropped the \"and\" in the final context line and instead\n> written this as:\n> \n> ... branch currently checked out (or \"detached HEAD\" if none),\n> and \"locked\" if the worktree is locked.\n> \n> But not worth a re-roll.\n\nAgreed. I believe it makes the documentation more concise.\n\n> > diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh\n> > @@ -61,6 +61,16 @@ test_expect_success '\"list\" all worktrees --porcelain' '\n> > +test_expect_success '\"list\" all worktress with locked annotation' '\n> > + test_when_finished \"rm -rf locked unlocked out && git worktree prune\" &&\n> > + git worktree add --detach locked master &&\n> > + git worktree add --detach unlocked master &&\n> > + git worktree lock locked &&\n> > + git worktree list >out &&\n> > + grep \"/locked *[0-9a-f].* locked\" out &&\n> > + ! grep \"/unlocked *[0-9a-f].* locked\" out\n> > +'\n> \n> These grep invocations are a bit loose, thus concern me a little bit.\n> \n> First, in Junio's original example of using grep[2], he had two spaces\n> after the path component, not one as you have here. The two spaces in\n> the regex ensure that there is at least one space separating `/locked`\n> and `/unlocked` from the OID hex string, whereas with just one space\n> in the regex, as is done here, the space following the path component\n> is entirely optional (thus is a less desirable regex).\n\nThat is interesting, I didn't know that will be the case - Nice to know :).\nThank you for the explanation.\n\n> \n> Second, because these regexes are not anchored, they could match with\n> a false-positive if the person's TRASH_DIRECTORY path is something\n> like `/home/proj/unlocked dead locked/git/t/...`. If you anchor the\n> pattern with `$`, then this problem goes away:\n> \n> grep \"/locked *[0-9a-f].* locked$\" out &&\n> ! grep \"/unlocked *[0-9a-f].* locked$\" out\n> \n\nThat is very good point and I will re-roll the patch with these two points\naddressing the test cases. Thanks. \n\n"},{"id":"407297","messageId":"20201011101152.5291-1-rafaeloliveira.cs@gmail.com","threadId":"54317","inReplyTo":"20201010185521.23032-1-rafaeloliveira.cs@gmail.com","subject":"[PATCH v3 0/1] Teach \"worktree list\" to annotate locked worktrees","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-10-11T10:11:51Z","receivedAt":"2020-10-11T10:12:41Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"This patch introduces a new information on the \"git worktree list\"\ncommand output to annotate when a worktree is locked with a \"locked\" text.\n\nThe intent is to improve the user experience as the \"git worktree remove\"\nrefuses to remove a locked worktree. By adding the \"locked\" annotation\nto the \"git worktree list\" command, the user would not attempt to remove\na worktree or would know to use \"git worktree remove -f -f \"\nto force a worktree removal.\n\nThe output of the \"worktree list\" command becomes:\n\n $ git worktree list\n /path/to/main abc123 [master]\n /path/to/worktree 456def [brancha]\n /path/to/locked-worktree 123abc (detached HEAD) locked\n\nChanges since v2:\n\n * Regexes used to test the \"locked\" anotation are anchored and space is added\n between the worktree path and OID hex string to avoid false-positives and\n to ensure there is space between the worktree path and the OID. \n\nChanges since v1:\n\n * Drooped the parenthesis in \"(locked)\" to reduce the noise and allow\n easier extensions of more annotations design proposed in [2].\n * Rewrite of the commit message with a much better one.\n * Simplification of the test added to `t2402` with only caring about\n the \"locked\" annotation at the end of the \"worktree list\" output.\n\n[2]: https://lore.kernel.org/git/CAPig+cQF6V8HNdMX5AZbmz3_w2WhSfA4SFfNhQqxXBqPXTZL+w@mail.gmail.com/\n\nThank you Junio C Hamano and Eric Sunshine for the detailed review\nand helping with this patch.\n\nRafael Silva (1):\n worktree: teach `list` to annotate locked worktree\n\n Documentation/git-worktree.txt | 3 ++-\n builtin/worktree.c | 5 ++++-\n t/t2402-worktree-list.sh | 10 ++++++++++\n 3 files changed, 16 insertions(+), 2 deletions(-)\n\n-- \n2.29.0.rc0.253.gc238dab514\n\n"},{"id":"407298","messageId":"20201011101152.5291-2-rafaeloliveira.cs@gmail.com","threadId":"54317","inReplyTo":"20201011101152.5291-1-rafaeloliveira.cs@gmail.com","subject":"[PATCH v3] worktree: teach `list` to annotate locked worktree","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-10-11T10:11:52Z","receivedAt":"2020-10-11T10:12:42Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"The \"git worktree list\" shows the absolute path to the working tree,\nthe commit that is checked out and the name of the branch. It is not\nimmediately obvious which of the worktrees, if any, are locked.\n\n\"git worktree remove\" refuses to remove a locked worktree with\nan error message. If \"git worktree list\" told which worktrees\nare locked in its output, the user would not even attempt to\nremove such a worktree, or would realize that\n\"git worktree remove -f -f \" is required.\n\nTeach \"git worktree list\" to append \"locked\" to its output.\nThe output from the command becomes like so:\n\n $ git worktree list\n /path/to/main abc123 [master]\n /path/to/worktree 456def (detached HEAD)\n /path/to/locked-worktree 123abc (detached HEAD) locked\n\nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \nSigned-off-by: Rafael Silva \n---\n Documentation/git-worktree.txt | 3 ++-\n builtin/worktree.c | 5 ++++-\n t/t2402-worktree-list.sh | 10 ++++++++++\n 3 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex 32e8440cde..6a6b7c4ad1 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -97,7 +97,8 @@ list::\n List details of each working tree. The main working tree is listed first,\n followed by each of the linked working trees. The output details include\n whether the working tree is bare, the revision currently checked out, and the\n-branch currently checked out (or \"detached HEAD\" if none).\n+branch currently checked out (or \"detached HEAD\" if none), and \"locked\" if\n+the worktree is locked.\n \n lock::\n \ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 99abaeec6c..ce56fdaaa9 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -676,8 +676,11 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)\n \t\t} else\n \t\t\tstrbuf_addstr(&sb, \"(error)\");\n \t}\n-\tprintf(\"%s\\n\", sb.buf);\n \n+\tif (!is_main_worktree(wt) && worktree_lock_reason(wt))\n+\t\tstrbuf_addstr(&sb, \" locked\");\n+\n+\tprintf(\"%s\\n\", sb.buf);\n \tstrbuf_release(&sb);\n }\n \ndiff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh\nindex 52585ec2aa..b85bd2655d 100755\n--- a/t/t2402-worktree-list.sh\n+++ b/t/t2402-worktree-list.sh\n@@ -61,6 +61,16 @@ test_expect_success '\"list\" all worktrees --porcelain' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '\"list\" all worktress with locked annotation' '\n+\ttest_when_finished \"rm -rf locked unlocked out && git worktree prune\" &&\n+\tgit worktree add --detach locked master &&\n+\tgit worktree add --detach unlocked master &&\n+\tgit worktree lock locked &&\n+\tgit worktree list >out &&\n+\tgrep \"/locked *[0-9a-f].* locked$\" out &&\n+\t! grep \"/unlocked *[0-9a-f].* locked$\" out\n+'\n+\n test_expect_success 'bare repo setup' '\n \tgit init --bare bare1 &&\n \techo \"data\" >file1 &&\n-- \n2.29.0.rc0.253.gc238dab514\n\n"},{"id":"407313","messageId":"CAPig+cS8hvX4GCsnfLBnQ4Q_AkUad=bw7rjVcaOqSEqcLZvx8w@mail.gmail.com","threadId":"54317","inReplyTo":"20201011101152.5291-2-rafaeloliveira.cs@gmail.com","subject":"Re: [PATCH v3] worktree: teach `list` to annotate locked worktree","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-10-12T02:24:19Z","receivedAt":"2020-10-12T02:24:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Oct 11, 2020 at 6:12 AM Rafael Silva\n wrote:\n> Teach \"git worktree list\" to append \"locked\" to its output.\n>\n> Signed-off-by: Rafael Silva \n> ---\n> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\n> @@ -97,7 +97,8 @@ list::\n> followed by each of the linked working trees. The output details include\n> whether the working tree is bare, the revision currently checked out, and the\n\nNit: I'd probably drop the \"and\" just before the end of this line...\n\n> -branch currently checked out (or \"detached HEAD\" if none).\n> +branch currently checked out (or \"detached HEAD\" if none), and \"locked\" if\n> +the worktree is locked.\n\n... which would leave just the one \"and\" here. However, this is minor\nand almost certainly not worth a re-roll.\n\n> +test_expect_success '\"list\" all worktress with locked annotation' '\n> + test_when_finished \"rm -rf locked unlocked out && git worktree prune\" &&\n> + git worktree add --detach locked master &&\n> + git worktree add --detach unlocked master &&\n> + git worktree lock locked &&\n> + git worktree list >out &&\n> + grep \"/locked *[0-9a-f].* locked$\" out &&\n> + ! grep \"/unlocked *[0-9a-f].* locked$\" out\n> +'\n\nLooks good. With or without the minor suggested documentation tweak,\nconsider this patch:\n\n Reviewed-by: Eric Sunshine \n"}]}