From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mon, 31 Aug 2015 06:23:18 GMT
Subject: Re: [PATCH v6 2/2] worktree: add 'list' command
Message-ID: <CAPig+cQ0oJbKV1PFPvDfjejyUfHmxzJmmYH3Wf0muNoiTU7Dew@mail.gmail.com>
In-Reply-To: <1440961839-40575-3-git-send-email-rappazzo@gmail.com>

On Sun, Aug 30, 2015 at 3:10 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:
> 'git worktree list' uses the for_each_worktree function to iterate,
> and outputs the worktree dir.  With the verbose option set, it also
> shows the branch or revision currently checked out in that worktree.
>
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index fb68156..867a24a 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|--verbose]

For conciseness and consistency with the "git worktree prune"
synopsis, this should mention only -v (and omit --verbose).

>  DESCRIPTION
>  -----------
> @@ -88,11 +93,13 @@ OPTIONS
>
>  -v::
>  --verbose::
> -       With `prune`, report all removals.
> +       With `prune`, report all removals.
> +       With `list` show the branch or revision currently checked out in that worktree.

Add a comma after "With `list`".
Also: s/in that/in each/
This new line is overly long; wrapping it over a couple lines would help.

>  --expire <time>::
>         With `prune`, only expire unused working trees older than <time>.
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 7b3cb96..6d96cdf 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> +static int print_worktree_details(const char *path, const char *git_dir, void *cb_data)
> +{
> +       printf("%s", path);
> +       if (verbose) {
> +               enter_repo(git_dir, 1);
> +               printf("  [");
> +               unsigned char sha1[20];
> +               int flag;
> +
> +               const char *refname = resolve_ref_unsafe("HEAD", 0, sha1, &flag);

Declarations after statement. Move the declarations of 'sha1', 'flag',
and 'refname' above the statements (enter_repo() and printf()).

 > +               if (!(flag & REF_ISSYMREF)) {
> +                       printf("%s", find_unique_abbrev(sha1, DEFAULT_ABBREV));
> +               } else {
> +                       refname = shorten_unambiguous_ref(refname, 0);
> +                       printf("%s", refname);
> +               }

As mentioned in my review[1] of patch 1/2, it would be better for this
sort of logic to be handled by the worktree iterator itself so that
all callers can benefit, rather than having to repeat the work in each
caller. This information would be encapsulated in a 'struct worktree'
along with 'path' and 'git_dir' and other interesting information
vended by the the iterator function.

> +               printf("]");

Rather than dropping printf()'s here and there to compose the output
piecemeal, it would be cleaner, and facilitate future changes, to
assign the refname/sha1 to a variable, and interpolate that variable
into an all-encompasing printf(), like this:

    printf("  [%s]", branch_or_sha1);

> +       }
> +       printf("\n");
> +
> +       return 0;
> +}

[1]: http://article.gmane.org/gmane.comp.version-control.git/276847