diff mbox series

[v2,6/6] worktree: teach `list` verbose mode

Message ID 20210117234244.95106-7-rafaeloliveira.cs@gmail.com (mailing list archive)
State New
Headers show
Series teach `worktree list` verbose mode and prunable annotations | expand

Commit Message

Rafael Silva Jan. 17, 2021, 11:42 p.m. UTC
"git worktree list" annotates each worktree according to its state such
as "prunable" or "locked", however it is not immediately obvious why
these worktrees are being annotated. For prunable worktrees a reason
is available that is returned by should_prune_worktree() and for locked
worktrees a reason might be available provided by the user via `lock`
command.

Let's teach "git worktree list" to output the reason why the worktrees
are being annotated. The reason is a text that can take virtually any
size and appending the text on the default columned format will make it
difficult to extend the command with other annotations and not fit nicely
on the screen. In order to address this shortcoming the annotation is
then moved to the next line indented followed by the reason, if the
reason is not available the annotation stays on the same line as the
worktree itself.

The output of "git worktree list" with verbose becomes like so:

    $ git worktree list --verbose
    ...
    /path/to/locked                acb124 [branch-a] locked
    /path/to/locked-with-reason    acc125 [branch-b]
        locked: worktree with a locked reason
    /path/to/prunable-reason       ace127 [branch-d]
        prunable: gitdir file points to non-existent location
    ...

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
 Documentation/git-worktree.txt | 20 ++++++++++++++++++++
 builtin/worktree.c             | 11 +++++++++--
 t/t2402-worktree-list.sh       | 27 +++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 2 deletions(-)

Comments

Eric Sunshine Jan. 18, 2021, 5:15 a.m. UTC | #1
On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva
<rafaeloliveira.cs@gmail.com> wrote:
> "git worktree list" annotates each worktree according to its state such
> as "prunable" or "locked", however it is not immediately obvious why
> these worktrees are being annotated. For prunable worktrees a reason
> is available that is returned by should_prune_worktree() and for locked
> worktrees a reason might be available provided by the user via `lock`
> command.
>
> Let's teach "git worktree list" to output the reason why the worktrees
> are being annotated. The reason is a text that can take virtually any
> size and appending the text on the default columned format will make it
> difficult to extend the command with other annotations and not fit nicely
> on the screen. In order to address this shortcoming the annotation is
> then moved to the next line indented followed by the reason, if the
> reason is not available the annotation stays on the same line as the
> worktree itself.

If you're re-rolling, let's mention the new `--verbose` option
somewhere in the commit message since that is the focus of this patch.
The second paragraph would be a good place:

    Let's teach "git worktree list" a --verbose mode which
    outputs the reason...

Also, the final sentence is a bit difficult to follow due to the comma
before "if the reason is not available". If you make the "if the
reason is not available" a separate sentence, it becomes simple to
understand.

> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
> @@ -135,6 +135,33 @@ test_expect_success '"list" all worktrees with prunable consistent with "prune"'
> +test_expect_success '"list" all worktrees --verbose with locked' '
> +       test_when_finished "rm -rf locked out actual expect && git worktree prune" &&
> +       git worktree add locked --detach &&
> +       git worktree lock locked --reason "with reason" &&
> +       test_when_finished "git worktree unlock locked" &&
> +       echo "$(git -C locked rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >expect &&
> +       printf "\tlocked: with reason\n" >>expect &&
> +       git worktree list --verbose >out &&
> +       sed -n "s/  */ /g;/\/locked  *[0-9a-f].*$/,/locked: .*$/p" <out >actual &&
> +       test_cmp actual expect
> +'

At first, I wondered if we would also want this test to have a
locked-no-reason worktree to ensure that its `locked` annotation stays
on the same line as the worktree, but that's not needed because that
case is already covered by the existing test. Fine.

> +test_expect_success '"list" all worktrees --verbose with prunable' '
> +       test_when_finished "rm -rf prunable out actual expect && git worktree prune" &&
> +       git worktree add prunable --detach &&
> +       echo "$(git -C prunable rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >expect &&
> +       printf "\tprunable: gitdir file points to non-existent location\n" >>expect &&
> +       rm -rf prunable &&
> +       git worktree list --verbose >out &&
> +       sed -n "s/  */ /g;/\/prunable  *[0-9a-f].*$/,/prunable: .*$/p" <out >actual &&
> +       test_i18ncmp actual expect
> +'

An alternative would be to have a single test of --verbose which
includes a locked-no-reason worktree, a locked-with-reason worktree,
and a prunable worktree. However, that's a very minor and subjective
point and certainly not worth a re-roll or changing unless you think
it's a nice simplification.
Eric Sunshine Jan. 18, 2021, 7:40 p.m. UTC | #2
On Mon, Jan 18, 2021 at 12:15 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
> > +test_expect_success '"list" all worktrees --verbose with locked' '
> > +       test_when_finished "rm -rf locked out actual expect && git worktree prune" &&
> > +       git worktree add locked --detach &&
> > +       git worktree lock locked --reason "with reason" &&
> > +       test_when_finished "git worktree unlock locked" &&
> > +       echo "$(git -C locked rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >expect &&
> > +       printf "\tlocked: with reason\n" >>expect &&
> > +       git worktree list --verbose >out &&
> > +       sed -n "s/  */ /g;/\/locked  *[0-9a-f].*$/,/locked: .*$/p" <out >actual &&
> > +       test_cmp actual expect
> > +'
>
> At first, I wondered if we would also want this test to have a
> locked-no-reason worktree to ensure that its `locked` annotation stays
> on the same line as the worktree, but that's not needed because that
> case is already covered by the existing test. Fine.

Erm, I think what I said is wrong. There is no existing test to show
that `locked` without a reason stays on the same line as the worktree
in --verbose mode. So having a locked-no-reason worktree in this test
could be beneficial.
diff mbox series

Patch

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 3d8c14dbdf..d34bf121f3 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -232,6 +232,8 @@  This can also be set up as the default behaviour by using the
 -v::
 --verbose::
 	With `prune`, report all removals.
++
+With `list`, output additional information about worktrees (see below).
 
 --expire <time>::
 	With `prune`, only expire unused working trees older than `<time>`.
@@ -389,6 +391,24 @@  $ git worktree list
 /path/to/prunable-worktree      5678abc  (detached HEAD) prunable
 ------------
 
+For these annotations, a reason might also be available and this can be
+seen using the verbose mode. The annotation is then moved to the next line
+indented followed by the additional information.
+
+------------
+$ git worktree list --verbose
+/path/to/linked-worktree        abcd1234 [master]
+/path/to/locked-worktreee       acbd5678 (brancha)
+	locked: working tree path is mounted on a removable device
+/path/to/locked-no-reason       abcd578  (detached HEAD) locked
+/path/to/prunable-worktree      5678abc  (detached HEAD)
+	prunable: gitdir file points to non-existent location
+------------
+
+Note that the annotation is moved to the next line if the additional
+information is available, otherwise it stays on the same line as the
+working tree itself.
+
 Porcelain Format
 ~~~~~~~~~~~~~~~~
 The porcelain format has a line per attribute.  Attributes are listed with a
diff --git a/builtin/worktree.c b/builtin/worktree.c
index fb82d7bb64..a59cbfa0d2 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -623,11 +623,15 @@  static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
 	}
 
 	reason = worktree_lock_reason(wt);
-	if (reason)
+	if (verbose && reason && *reason)
+		strbuf_addf(&sb, "\n\tlocked: %s", reason);
+	else if (reason)
 		strbuf_addstr(&sb, " locked");
 
 	reason = worktree_prune_reason(wt, expire);
-	if (reason)
+	if (verbose && reason && *reason)
+		strbuf_addf(&sb, "\n\tprunable: %s", reason);
+	else if (reason)
 		strbuf_addstr(&sb, " prunable");
 
 	printf("%s\n", sb.buf);
@@ -673,6 +677,7 @@  static int list(int ac, const char **av, const char *prefix)
 
 	struct option options[] = {
 		OPT_BOOL(0, "porcelain", &porcelain, N_("machine-readable output")),
+		OPT__VERBOSE(&verbose, N_("show extended annotations and reasons, if available")),
 		OPT_EXPIRY_DATE(0, "expire", &expire,
 				N_("add 'prunable' annotation to worktrees older than <time>")),
 		OPT_END()
@@ -682,6 +687,8 @@  static int list(int ac, const char **av, const char *prefix)
 	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
 	if (ac)
 		usage_with_options(worktree_usage, options);
+	else if (verbose && porcelain)
+		die(_("--verbose and --porcelain are mutually exclusive"));
 	else {
 		struct worktree **worktrees = get_worktrees();
 		int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;
diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
index e9b410b69e..4de37f896c 100755
--- a/t/t2402-worktree-list.sh
+++ b/t/t2402-worktree-list.sh
@@ -135,6 +135,33 @@  test_expect_success '"list" all worktrees with prunable consistent with "prune"'
 	test_i18ngrep ! "^Removing worktrees/unprunable" out
 '
 
+test_expect_success '"list" --verbose and --porcelain mutually exclusive' '
+	test_must_fail git worktree list --verbose --porcelain
+'
+
+test_expect_success '"list" all worktrees --verbose with locked' '
+	test_when_finished "rm -rf locked out actual expect && git worktree prune" &&
+	git worktree add locked --detach &&
+	git worktree lock locked --reason "with reason" &&
+	test_when_finished "git worktree unlock locked" &&
+	echo "$(git -C locked rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >expect &&
+	printf "\tlocked: with reason\n" >>expect &&
+	git worktree list --verbose >out &&
+	sed -n "s/  */ /g;/\/locked  *[0-9a-f].*$/,/locked: .*$/p" <out >actual &&
+	test_cmp actual expect
+'
+
+test_expect_success '"list" all worktrees --verbose with prunable' '
+	test_when_finished "rm -rf prunable out actual expect && git worktree prune" &&
+	git worktree add prunable --detach &&
+	echo "$(git -C prunable rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >expect &&
+	printf "\tprunable: gitdir file points to non-existent location\n" >>expect &&
+	rm -rf prunable &&
+	git worktree list --verbose >out &&
+	sed -n "s/  */ /g;/\/prunable  *[0-9a-f].*$/,/prunable: .*$/p" <out >actual &&
+	test_i18ncmp actual expect
+'
+
 test_expect_success 'bare repo setup' '
 	git init --bare bare1 &&
 	echo "data" >file1 &&