| Message ID | 20200928154953.30396-2-rafaeloliveira.cs@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | teach `worktree list` to mark locked worktrees | expand |
Rafael Silva <rafaeloliveira.cs@gmail.com> writes: > The output of `worktree list` command is extended to mark a locked > worktree with `(locked)` text. This is used to communicate to the > user that a linked worktree is locked instead of learning only when > attempting to remove it. > > This is the output of the worktree list with locked marker: > > $ git worktree list > /repo/to/main abc123 [master] > /path/to/unlocked-worktree1 456def [brancha] > /path/to/locked-worktree 123abc (detached HEAD) (locked) In our log message, we tend NOT to say "This commit does X" or "X is done", because such a statement is often insufficient to illustrate if the commit indeed does X, and explain why it is a good thing to do X in the first place. Instead, we - first explain that the current system does not do X (in present tense, so we do NOT say "previously we did not do X"), then - explain why doing X would be a good thing, and finally - give an order to the codebase to start doing X. For this change, it might look like this: 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. Teach "git worktree list" to append "(locked)" to its output. The output from the command becomes like so: $ git worktree list /repo/to/main abc123 [master] /path/to/unlocked-worktree1 456def [brancha] /path/to/locked-worktree 123abc (detached HEAD) (locked) > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > index 32e8440cde..a3781dd664 100644 > --- a/Documentation/git-worktree.txt > +++ b/Documentation/git-worktree.txt > @@ -96,8 +96,9 @@ 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). > +whether the working tree is bare, the revision currently checked out, the > +branch currently checked out (or "detached HEAD" if none), and whether > +the worktree is locked. At the first glance, the above gave me an impression that you'd be adding "(unlocked)" or "(locked)" for each working tree, but that is not the case. How about keeping the original sentence intact, and adding something like "For a locked worktree, the marker (locked) is also shown at the end"? > diff --git a/builtin/worktree.c b/builtin/worktree.c > index 99abaeec6c..8ad2cdd2f9 100644 > --- a/builtin/worktree.c > +++ b/builtin/worktree.c > @@ -676,8 +676,12 @@ 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)"); Is this because for the primary worktree, worktree_lock_reason() will always yield true? ... goes and looks ... Ah, OK, the callers are not even allowed to ask the question on the primary one. That's a bit strange API but OK. Writing that on a single line would perfectly be readable, by the way. if (!is_main_worktree(wt) && worktree_lock_reason(wt)) strbuf_addstr(&sb, " (locked)"); > + printf("%s\n", sb.buf); > strbuf_release(&sb); > }
On Mon, Sep 28, 2020 at 02:37:54PM -0700, Junio C Hamano wrote: > Rafael Silva <rafaeloliveira.cs@gmail.com> writes: > > > The output of `worktree list` command is extended to mark a locked > > worktree with `(locked)` text. This is used to communicate to the > > user that a linked worktree is locked instead of learning only when > > attempting to remove it. > > > > This is the output of the worktree list with locked marker: > > > > $ git worktree list > > /repo/to/main abc123 [master] > > /path/to/unlocked-worktree1 456def [brancha] > > /path/to/locked-worktree 123abc (detached HEAD) (locked) > > > In our log message, we tend NOT to say "This commit does X" or "X is > done", because such a statement is often insufficient to illustrate > if the commit indeed does X, and explain why it is a good thing to > do X in the first place. > > Instead, we > > - first explain that the current system does not do X (in present > tense, so we do NOT say "previously we did not do X"), then > > - explain why doing X would be a good thing, and finally > > - give an order to the codebase to start doing X. > > > For this change, it might look like this: > > 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. > > Teach "git worktree list" to append "(locked)" to its output. > The output from the command becomes like so: > > $ git worktree list > /repo/to/main abc123 [master] > /path/to/unlocked-worktree1 456def [brancha] > /path/to/locked-worktree 123abc (detached HEAD) (locked) > Thank you for such detailed explanation. I totally agree that it seems much better to organise the commit message this way, I will definitely include this message (or something very similar) when resending these patches. > > > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > > index 32e8440cde..a3781dd664 100644 > > --- a/Documentation/git-worktree.txt > > +++ b/Documentation/git-worktree.txt > > @@ -96,8 +96,9 @@ 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). > > +whether the working tree is bare, the revision currently checked out, the > > +branch currently checked out (or "detached HEAD" if none), and whether > > +the worktree is locked. > > At the first glance, the above gave me an impression that you'd be > adding "(unlocked)" or "(locked)" for each working tree, but that is > not the case. How about keeping the original sentence intact, and > adding something like "For a locked worktree, the marker (locked) is > also shown at the end"? Yes, it sounds good. > > > diff --git a/builtin/worktree.c b/builtin/worktree.c > > index 99abaeec6c..8ad2cdd2f9 100644 > > --- a/builtin/worktree.c > > +++ b/builtin/worktree.c > > @@ -676,8 +676,12 @@ 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)"); > > Is this because for the primary worktree, worktree_lock_reason() > will always yield true? > > ... goes and looks ... > > Ah, OK, the callers are not even allowed to ask the question on the > primary one. That's a bit strange API but OK. > > Writing that on a single line would perfectly be readable, by the > way. > > if (!is_main_worktree(wt) && worktree_lock_reason(wt)) > strbuf_addstr(&sb, " (locked)"); Agreed. will be much better to have it in a single line. > > > + printf("%s\n", sb.buf); > > strbuf_release(&sb); > > }
On Mon, Sep 28, 2020 at 5:38 PM Junio C Hamano <gitster@pobox.com> wrote: > Rafael Silva <rafaeloliveira.cs@gmail.com> writes: > > The output of `worktree list` command is extended to mark a locked > > worktree with `(locked)` text. This is used to communicate to the > > user that a linked worktree is locked instead of learning only when > > attempting to remove it. > > For this change, it might look like this: > > 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. Nicely written. I might end the final sentence like this: ... the user would not even attempt to remove such a worktree or would know to use `git worktree remove -f -f <path>`. > > diff --git a/builtin/worktree.c b/builtin/worktree.c > > @@ -676,8 +676,12 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len) > > + if (!is_main_worktree(wt) && > > + worktree_lock_reason(wt)) > > + strbuf_addstr(&sb, " (locked)"); > > Is this because for the primary worktree, worktree_lock_reason() > will always yield true? > > ... goes and looks ... > > Ah, OK, the callers are not even allowed to ask the question on the > primary one. That's a bit strange API but OK. That is indeed a slightly hostile API, and it wouldn't hurt to change it simply to return 'false' for the main worktree, but that's not something this patch series need tackle.
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt index 32e8440cde..a3781dd664 100644 --- a/Documentation/git-worktree.txt +++ b/Documentation/git-worktree.txt @@ -96,8 +96,9 @@ 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). +whether the working tree is bare, the revision currently checked out, the +branch currently checked out (or "detached HEAD" if none), and whether +the worktree is locked. lock:: diff --git a/builtin/worktree.c b/builtin/worktree.c index 99abaeec6c..8ad2cdd2f9 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -676,8 +676,12 @@ 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); }
The output of `worktree list` command is extended to mark a locked worktree with `(locked)` text. This is used to communicate to the user that a linked worktree is locked instead of learning only when attempting to remove it. This is the output of the worktree list with locked marker: $ git worktree list /repo/to/main abc123 [master] /path/to/unlocked-worktree1 456def [brancha] /path/to/locked-worktree 123abc (detached HEAD) (locked) Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com> --- Documentation/git-worktree.txt | 5 +++-- builtin/worktree.c | 6 +++++- 2 files changed, 8 insertions(+), 3 deletions(-)