| Message ID | 20201010185521.23032-2-rafaeloliveira.cs@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Teach "worktree list" to annotate locked worktrees | expand |
On Sat, Oct 10, 2020 at 2:56 PM Rafael Silva <rafaeloliveira.cs@gmail.com> wrote: > The "git worktree list" shows the absolute path to the working tree, > the commit that is checked out and the name of the branch. It is not > immediately obvious which of the worktrees, if any, are locked. > > "git worktree remove" refuses to remove a locked worktree with > an error message. If "git worktree list" told which worktrees > are locked in its output, the user would not even attempt to > remove such a worktree or would know how to use > `git worktree remove -f -f <path>` I would drop "how" from "would know how to" so it instead reads "would know to" since seeing the `locked` annotation only lets the user know that removal must be forced; the `locked` annotation doesn't teach the user _how_ to remove the worktree using force. But, perhaps, my original suggestion[1], which did not use "how", was confusing. Maybe it could be worded instead: ... not even attempt to remove such a worktree, or would realize that `git worktree remove -f -f <path>` is required. Anyhow, this is a very minor nit about the commit message; not necessarily worth a re-roll. More comments below... [1]: https://lore.kernel.org/git/CAPig+cQHDuWy1vc_ngXbMQZQ=a9fd6S5_cCU-2sb_+Te5aEOhw@mail.gmail.com/ > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > @@ -97,7 +97,8 @@ list:: > List details of each working tree. The main working tree is listed first, > followed by each of the linked working trees. The output details include > whether the working tree is bare, the revision currently checked out, and the > -branch currently checked out (or "detached HEAD" if none). > +branch currently checked out (or "detached HEAD" if none). For a locked > +worktree the `locked` annotation is also shown. I might have dropped the "and" in the final context line and instead written this as: ... branch currently checked out (or "detached HEAD" if none), and "locked" if the worktree is locked. But not worth a re-roll. > diff --git a/builtin/worktree.c b/builtin/worktree.c > @@ -676,8 +676,11 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len) > } else > strbuf_addstr(&sb, "(error)"); > > + if (!is_main_worktree(wt) && worktree_lock_reason(wt)) > + strbuf_addstr(&sb, " locked"); I was going to ask if "locked" should be localizable like this: strbuf_addf(&sb, " %s", _("locked")); but I see that none of the other words ("bare", "detached", "error") in this function are localizable, so this is fine as-is. However, all of the other human-consumable text emitted by "git worktree" is localizable, so making these strings localizable, as well, is something that can be added to a To-Do list. Note that I'm talking only about human-consumable "git worktree list" output, not porcelain format. Also, I'm not suggesting you tackle it, and it's certainly not something that this patch or patch series needs to do; just something which someone can tackle in the future. > diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh > @@ -61,6 +61,16 @@ test_expect_success '"list" all worktrees --porcelain' ' > +test_expect_success '"list" all worktress with locked annotation' ' > + test_when_finished "rm -rf locked unlocked out && git worktree prune" && > + git worktree add --detach locked master && > + git worktree add --detach unlocked master && > + git worktree lock locked && > + git worktree list >out && > + grep "/locked *[0-9a-f].* locked" out && > + ! grep "/unlocked *[0-9a-f].* locked" out > +' These grep invocations are a bit loose, thus concern me a little bit. First, in Junio's original example of using grep[2], he had two spaces after the path component, not one as you have here. The two spaces in the regex ensure that there is at least one space separating `/locked` and `/unlocked` from the OID hex string, whereas with just one space in the regex, as is done here, the space following the path component is entirely optional (thus is a less desirable regex). Second, because these regexes are not anchored, they could match with a false-positive if the person's TRASH_DIRECTORY path is something like `/home/proj/unlocked dead locked/git/t/...`. If you anchor the pattern with `$`, then this problem goes away: grep "/locked *[0-9a-f].* locked$" out && ! grep "/unlocked *[0-9a-f].* locked$" out Third, this is checking only that the first character following the path component is a hex digit but then accepts _anything_ before "locked". The regex can be tightened to allow only hex digits: grep "/locked *[0-9a-f][0-9a-f]* locked$" out && ! grep "/unlocked *[0-9a-f][0-9a-f]* locked$" out [2]: https://lore.kernel.org/git/xmqq3631lg8f.fsf@gitster.c.googlers.com/
On Sun, Oct 11, 2020 at 2:26 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > Third, this is checking only that the first character following the > path component is a hex digit but then accepts _anything_ before > "locked". The regex can be tightened to allow only hex digits: > > grep "/locked *[0-9a-f][0-9a-f]* locked$" out && > ! grep "/unlocked *[0-9a-f][0-9a-f]* locked$" out Nevermind this last point. I see that there is other gunk after the hex string but before the `locked` annotation, so this suggestion breaks the test. The other two points -- (1) mandatory whitespace following the path component, and (2) anchoring the pattern -- would be welcome.
On Sun, Oct 11, 2020 at 02:26:31AM -0400, Eric Sunshine wrote: > On Sat, Oct 10, 2020 at 2:56 PM Rafael Silva > <rafaeloliveira.cs@gmail.com> wrote: > > The "git worktree list" shows the absolute path to the working tree, > > the commit that is checked out and the name of the branch. It is not > > immediately obvious which of the worktrees, if any, are locked. > > > > "git worktree remove" refuses to remove a locked worktree with > > an error message. If "git worktree list" told which worktrees > > are locked in its output, the user would not even attempt to > > remove such a worktree or would know how to use > > `git worktree remove -f -f <path>` > > I would drop "how" from "would know how to" so it instead reads "would > know to" since seeing the `locked` annotation only lets the user know > that removal must be forced; the `locked` annotation doesn't teach the > user _how_ to remove the worktree using force. But, perhaps, my > original suggestion[1], which did not use "how", was confusing. Maybe > it could be worded instead: > > ... not even attempt to remove such a worktree, or would > realize that `git worktree remove -f -f <path>` is required. > > Anyhow, this is a very minor nit about the commit message; not > necessarily worth a re-roll. More comments below... > > [1]: https://lore.kernel.org/git/CAPig+cQHDuWy1vc_ngXbMQZQ=a9fd6S5_cCU-2sb_+Te5aEOhw@mail.gmail.com/ The "would realise ..." seems clear to me, will change on the patch as I will address the test changes aforementioned. > > > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > > @@ -97,7 +97,8 @@ list:: > > List details of each working tree. The main working tree is listed first, > > followed by each of the linked working trees. The output details include > > whether the working tree is bare, the revision currently checked out, and the > > -branch currently checked out (or "detached HEAD" if none). > > +branch currently checked out (or "detached HEAD" if none). For a locked > > +worktree the `locked` annotation is also shown. > > I might have dropped the "and" in the final context line and instead > written this as: > > ... branch currently checked out (or "detached HEAD" if none), > and "locked" if the worktree is locked. > > But not worth a re-roll. Agreed. I believe it makes the documentation more concise. > > diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh > > @@ -61,6 +61,16 @@ test_expect_success '"list" all worktrees --porcelain' ' > > +test_expect_success '"list" all worktress with locked annotation' ' > > + test_when_finished "rm -rf locked unlocked out && git worktree prune" && > > + git worktree add --detach locked master && > > + git worktree add --detach unlocked master && > > + git worktree lock locked && > > + git worktree list >out && > > + grep "/locked *[0-9a-f].* locked" out && > > + ! grep "/unlocked *[0-9a-f].* locked" out > > +' > > These grep invocations are a bit loose, thus concern me a little bit. > > First, in Junio's original example of using grep[2], he had two spaces > after the path component, not one as you have here. The two spaces in > the regex ensure that there is at least one space separating `/locked` > and `/unlocked` from the OID hex string, whereas with just one space > in the regex, as is done here, the space following the path component > is entirely optional (thus is a less desirable regex). That is interesting, I didn't know that will be the case - Nice to know :). Thank you for the explanation. > > Second, because these regexes are not anchored, they could match with > a false-positive if the person's TRASH_DIRECTORY path is something > like `/home/proj/unlocked dead locked/git/t/...`. If you anchor the > pattern with `$`, then this problem goes away: > > grep "/locked *[0-9a-f].* locked$" out && > ! grep "/unlocked *[0-9a-f].* locked$" out > That is very good point and I will re-roll the patch with these two points addressing the test cases. Thanks.
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt index 32e8440cde..40ef4c18c5 100644 --- a/Documentation/git-worktree.txt +++ b/Documentation/git-worktree.txt @@ -97,7 +97,8 @@ list:: List details of each working tree. The main working tree is listed first, followed by each of the linked working trees. The output details include whether the working tree is bare, the revision currently checked out, and the -branch currently checked out (or "detached HEAD" if none). +branch currently checked out (or "detached HEAD" if none). For a locked +worktree the `locked` annotation is also shown. lock:: diff --git a/builtin/worktree.c b/builtin/worktree.c index 99abaeec6c..ce56fdaaa9 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -676,8 +676,11 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len) } else strbuf_addstr(&sb, "(error)"); } - printf("%s\n", sb.buf); + if (!is_main_worktree(wt) && worktree_lock_reason(wt)) + strbuf_addstr(&sb, " locked"); + + printf("%s\n", sb.buf); strbuf_release(&sb); } diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh index 52585ec2aa..74275407ee 100755 --- a/t/t2402-worktree-list.sh +++ b/t/t2402-worktree-list.sh @@ -61,6 +61,16 @@ test_expect_success '"list" all worktrees --porcelain' ' test_cmp expect actual ' +test_expect_success '"list" all worktress with locked annotation' ' + test_when_finished "rm -rf locked unlocked out && git worktree prune" && + git worktree add --detach locked master && + git worktree add --detach unlocked master && + git worktree lock locked && + git worktree list >out && + grep "/locked *[0-9a-f].* locked" out && + ! grep "/unlocked *[0-9a-f].* locked" out +' + test_expect_success 'bare repo setup' ' git init --bare bare1 && echo "data" >file1 &&
The "git worktree list" shows the absolute path to the working tree, the commit that is checked out and the name of the branch. It is not immediately obvious which of the worktrees, if any, are locked. "git worktree remove" refuses to remove a locked worktree with an error message. If "git worktree list" told which worktrees are locked in its output, the user would not even attempt to remove such a worktree or would know how to use `git worktree remove -f -f <path>` Teach "git worktree list" to append "locked" to its output. The output from the command becomes like so: $ git worktree list /path/to/main abc123 [master] /path/to/worktree 456def (detached HEAD) /path/to/locked-worktree 123abc (detached HEAD) locked Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com> --- Documentation/git-worktree.txt | 3 ++- builtin/worktree.c | 5 ++++- t/t2402-worktree-list.sh | 10 ++++++++++ 3 files changed, 16 insertions(+), 2 deletions(-)