Re: [PATCH v6 2/2] worktree: add 'list' command

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 31, 2015, 06:23 UTC
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:

Show 15 quoted lines

> '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).

Show 9 quoted lines

>  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.

Show 16 quoted lines

>  --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)) {

Show 5 quoted lines

> +                       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);

Show 5 quoted lines

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

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