git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v7 3/3] worktree: add 'list' command

From
Eric Sunshine <sunshine@sunshineco.com>
Date
Sep 13, 2015, 04:25 UTC
Message-ID
<CAPig+cSfduAgj7WP2JmqecVbQMRoZu+9O0f9vPSF1ee8h=1LmA@mail.gmail.com>
In-Reply-To
<1441402769-35897-4-git-send-email-rappazzo@gmail.com>
On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:
> 'git worktree list' iterates through the worktree list, and outputs
> the worktree dir.  By default, only the worktree path is output.
Comments below in addition to Junio's...
> Supported options include:
>         --skip-bare: do not output bare worktrees
>         --verbose: include the current head and ref (if applicable), also
>                 decorate bare worktrees

I don't care strongly at this point, but an alternate, (possibly) more reviewer-friendly, approach would be to split this into several patches: the first would add a bare-bones "list" command, and each subsequent patch would flesh it out by introducing one option and/or enhancing the output in some way.

Show 11 quoted lines
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index fb68156..b9339ed 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -11,6 +11,7 @@ SYNOPSIS
>  [verse]
>  'git worktree add' [-f] [--detach] [-b <new-branch>] <path> [<branch>]
>  'git worktree prune' [-n] [-v] [--expire <expire>]
> +'git worktree list' [-v] [--skip-bare]

I'm somewhat skeptical of the --skip-bare option. Recalling my v2 review[1] skepticism of the --main-only option:

    The more options we have, the more we have to document, test, and
    support...

Thus, I wonder how much value this option has. Presumably, for scripters, we will want to have a --porcelain option, the output of which will contain useful information about a worktree, including whether it's bare, and a script can, on its own, easily filter out a bare worktree if desired.

[1]: http://article.gmane.org/gmane.comp.version-control.git/275528
Show 20 quoted lines
>  DESCRIPTION
>  -----------
> @@ -59,6 +60,10 @@ prune::
>
>  Prune working tree information in $GIT_DIR/worktrees.
>
> +list::
> +
> +List the main worktree followed by each of the linked worktrees.
> +
>  OPTIONS
>  -------
>
> @@ -89,10 +94,14 @@ OPTIONS
>  -v::
>  --verbose::
>         With `prune`, report all removals.
> +       With `list`, show more information about each worktree.  This includes
> +       if the worktree is bare, the revision currently checked out, and the
> +       branch currently checked out (or 'detached HEAD' if none).

Hmm, this prints both the SHA1 and the branch name (or literal string "detached HEAD")? Is that a good use of screen real-estate? In particular, I'm wondering how useful the SHA1 is to the user when not detached. I would expect (perhaps wrongly) that it would be sufficient to output either the branch name *or* the SHA1 (which implies "detached" without having to say so literally).

>  --expire <time>::
>         With `prune`, only expire unused working trees older than <time>.
Documentation for --skip-bare (and --no-skip-bare) seems to be missing.
Show 36 quoted lines
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 71bb770..a0c0fe8 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -359,6 +362,64 @@ static int add(int ac, const char **av, const char *prefix)
> +static int list(int ac, const char **av, const char *prefix)
> +{
> +       int no_bare = 0;
> +
> +       struct option options[] = {
> +               OPT_BOOL(0, "skip-bare", &no_bare,  N_("exclude bare worktrees from the list")),
> +               OPT__VERBOSE(&verbose, N_("include more worktree details")),
> +               OPT_END()
> +       };
> +
> +       ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
> +       if (ac)
> +               usage_with_options(worktree_usage, options);
> +       else {
> +               struct worktree_list *worktree_list = get_worktree_list();
> +               struct worktree_list *orig = worktree_list;
> +               struct strbuf sb = STRBUF_INIT;
> +               int path_maxlen = 0;
> +
> +               if (verbose) {
> +                       while (worktree_list) {
> +                               int cur_len = strlen(worktree_list->worktree->path);
> +                               if (cur_len > path_maxlen)
> +                                       path_maxlen = cur_len;
> +                               worktree_list = worktree_list->next ? worktree_list->next : NULL;
> +                       }
> +                       worktree_list = orig;
> +               }
> +               while (worktree_list) {
> +                       /* if this work tree is not bare OR if bare repos are to be shown (default) */
> +                       if (!worktree_list->worktree->is_bare || !no_bare) {

Double negatives (!no_bare) are hard to grok. 'bare_only' or 'skip_bare' might be better, and then the comment would likely be superfluous. Also, having the "default" behavior mentioned in the comment is an invitation for it to become outdated.

Show 7 quoted lines
> +                               strbuf_reset(&sb);
> +                               if (!verbose)
> +                                       strbuf_addstr(&sb, worktree_list->worktree->path);
> +                               else {
> +                                       int cur_len = strlen(worktree_list->worktree->path);
> +                                       int utf8_adj = cur_len - utf8_strwidth(worktree_list->worktree->path);
> +                                       strbuf_addf(&sb, "%-*s ", 1 + path_maxlen + utf8_adj, worktree_list->worktree->path);

I'm not personally convinced that this sort of alignment is desirable, however, the new strbuf_utf8_align()[2] might be useful here.

[2]: Once it graduates to 'master', that is; it's currently in 'pu' at 5df5352 (utf8: add function to align a string into given strbuf, 2015-09-10).

Show 43 quoted lines
> +                                       if (worktree_list->worktree->is_bare)
> +                                               strbuf_addstr(&sb, "(bare)");
> +                                       else {
> +                                               strbuf_addf(&sb, "%s ", find_unique_abbrev(worktree_list->worktree->head_sha1, DEFAULT_ABBREV));
> +                                               if (!worktree_list->worktree->is_detached)
> +                                                       strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(worktree_list->worktree->head_ref, 0));
> +                                               else
> +                                                       strbuf_addstr(&sb, "(detached HEAD)");
> +                                       }
> +                               }
> +                               printf("%s\n", sb.buf);
> +                       }
> +                       worktree_list = worktree_list->next ? worktree_list->next : NULL;
> +               }
> +               worktree_list_release(orig);
> +               strbuf_release(&sb);
> +       }
> +       return 0;
> +}
> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
> new file mode 100755
> index 0000000..246890c
> --- /dev/null
> +++ b/t/t2027-worktree-list.sh
> @@ -0,0 +1,122 @@
> +#!/bin/sh
> +
> +test_description='test git worktree list'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +       test_commit init
> +'
> +
> +test_expect_success '"list" all worktrees from main' '
> +       git rev-parse --show-toplevel >expect &&
> +       git worktree add --detach here master &&
> +       git -C here rev-parse --show-toplevel >>expect &&
> +       git worktree list >actual &&
> +       test_cmp expect actual &&
> +       rm -rf here &&
> +       git worktree prune

If the test fails somewhere above the 'rm', then the cleanup won't take place, which will result in cascading failures of subsequent tests. Instead, use test_when_finished() before the "worktree add" line to ensure cleanup whether successful or not:

    test_when_finished "rm -rf here && git worktree prune" &&
    git worktree add ... &&
Same comment applies to remaining tests.
Show 15 quoted lines
> +'
> +
> +test_expect_success '"list" all worktrees from linked' '
> +       git rev-parse --show-toplevel >expect &&
> +       git worktree add --detach here master &&
> +       git -C here rev-parse --show-toplevel >>expect &&
> +       git -C here worktree list >actual &&
> +       test_cmp expect actual &&
> +       rm -rf here &&
> +       git worktree prune
> +'
> +
> +test_expect_success 'bare repo setup' '
> +       git init --bare bare1 &&
> +       echo "data" > file1 &&
Style: No space after redirection operator.
Nit: Unnecessary quotes around 'data'.
Show 9 quoted lines
> +       git add file1 &&
> +       git commit -m"File1: add data" &&
> +       git push bare1 master &&
> +       git reset --hard HEAD^
> +'
> +
> +test_expect_success '"list" all worktrees from bare main' '
> +       git -C bare1 worktree add --detach ../there master &&
> +       echo "$(cd bare1 && pwd)" >expect &&

This may be problematic on Windows. See the discussion of $PWD in t/README. You may need to use $(pwd) instead. Same comment applies below, too.

Show 10 quoted lines
> +       echo "$(git -C there rev-parse --show-toplevel)" >>expect &&
> +       git -C bare1 worktree list >actual &&
> +       test_cmp expect actual &&
> +       rm -rf there &&
> +       git -C bare1 worktree prune
> +'
> +test_expect_success '"list" all worktrees --verbose from bare main' '
> +       git -C bare1 worktree add --detach ../there master &&
> +   echo "$(cd bare1 && pwd)  (bare)" >expect &&
> +   echo "$(git -C there rev-parse --show-toplevel)  $(git -C there rev-parse --short HEAD) (detached HEAD)" >>expect &&
Botched indentation here and in next test.
Show 19 quoted lines
> +       git -C bare1 worktree list --verbose >actual &&
> +       test_cmp expect actual &&
> +       rm -rf there &&
> +       git -C bare1 worktree prune
> +'
> +
> +test_expect_success '"list" all worktrees --verbose from worktree with a bare main' '
> +       git -C bare1 worktree add --detach ../there master &&
> +   echo "$(cd bare1 && pwd)  (bare)" >expect &&
> +   echo "$(git -C there rev-parse --show-toplevel)  $(git -C there rev-parse --short HEAD) (detached HEAD)" >>expect &&
> +       git -C there worktree list --verbose >actual &&
> +       test_cmp expect actual &&
> +       rm -rf there &&
> +       git -C bare1 worktree prune
> +'
> +
> +test_expect_success 'bare repo cleanup' '
> +       rm -rf bare1
> +'
Meh. Probably unnecessary cleanup.
> +
> +test_done
> --
> 2.5.0
Previous: Junio C Hamano
Message 27 of 27 in “worktree: worktree.c functions and list builtin command”
  1. 0/3 worktree: worktree.c functions and list builtin commandMichael Rappazzo, Sep 4, 2015
  2. 1/3 worktree: add top-level worktree.cMichael Rappazzo, Sep 4, 2015
  3. Junio C HamanoSep 10, 2015
  4. Mike RappazzoSep 11, 2015
  5. Eric SunshineSep 13, 2015
  6. Eric SunshineSep 13, 2015
  7. Mike RappazzoSep 14, 2015
  8. Junio C HamanoSep 14, 2015
  9. Eric SunshineSep 16, 2015
  10. Eric SunshineSep 16, 2015
  11. Mike RappazzoSep 16, 2015
  12. Eric SunshineSep 22, 2015
  13. Junio C HamanoSep 22, 2015
  14. 2/3 worktree: move/refactor find_shared_symref from branch.cMichael Rappazzo, Sep 4, 2015
  15. Junio C HamanoSep 11, 2015
  16. Mike RappazzoSep 11, 2015
  17. Junio C HamanoSep 11, 2015
  18. Eric SunshineSep 11, 2015
  19. Mike RappazzoSep 12, 2015
  20. Eric SunshineSep 13, 2015
  21. Mike RappazzoSep 14, 2015
  22. Eric SunshineSep 16, 2015
  23. Mike RappazzoSep 16, 2015
  24. Eric SunshineSep 22, 2015
  25. 3/3 worktree: add 'list' commandMichael Rappazzo, Sep 4, 2015
  26. Junio C HamanoSep 11, 2015
  27. Eric SunshineSep 13, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.