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@xxxxxxxxx> 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@xxxxxxxxxxxxxx/ 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.
