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

2015-08-30 Thread Eric Sunshine 
On Sun, Aug 30, 2015 at 3:10 PM, Michael Rappazzo  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 
> ---
> 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 ]  []
>  'git worktree prune' [-n] [-v] [--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 ::
> With `prune`, only expire unused working trees older than .
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-08-30 Thread Michael Rappazzo 
'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 
---
 Documentation/git-worktree.txt |  10 -
 builtin/worktree.c |  42 +
 t/t2027-worktree-list.sh   | 100 +
 3 files changed, 150 insertions(+), 2 deletions(-)
 create mode 100755 t/t2027-worktree-list.sh

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 ]  []
 'git worktree prune' [-n] [-v] [--expire ]
+'git worktree list' [-v|--verbose]
 
 DESCRIPTION
 ---
@@ -59,6 +60,10 @@ prune::
 
 Prune working tree information in $GIT_DIR/worktrees.
 
+list::
+
+List the main worktree followed by all of the linked worktrees.
+
 OPTIONS
 ---
 
@@ -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.
 
 --expire ::
With `prune`, only expire unused working trees older than .
 
+
 DETAILS
 ---
 Each linked working tree has a private sub-directory in the repository's
@@ -167,7 +174,6 @@ performed manually, such as:
 - `remove` to remove a linked working tree and its administrative files (and
   warn if the working tree is dirty)
 - `mv` to move or rename a working tree and update its administrative files
-- `list` to list linked working trees
 - `lock` to prevent automatic pruning of administrative files (for instance,
   for a working tree on a portable device)
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7b3cb96..6d96cdf 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -12,6 +12,7 @@
 static const char * const worktree_usage[] = {
N_("git worktree add []  "),
N_("git worktree prune []"),
+   N_("git worktree list []"),
NULL
 };
 
@@ -442,6 +443,45 @@ done:
return ret;
 }
 
+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);
+   if (!(flag & REF_ISSYMREF)) {
+   printf("%s", find_unique_abbrev(sha1, DEFAULT_ABBREV));
+   } else {
+   refname = shorten_unambiguous_ref(refname, 0);
+   printf("%s", refname);
+   }
+
+   printf("]");
+   }
+   printf("\n");
+
+   return 0;
+}
+
+static int list(int ac, const char **av, const char *prefix)
+{
+   struct option options[] = {
+   OPT__VERBOSE(&verbose, N_("show the branch or revision 
currently checked out")),
+   OPT_END()
+   };
+
+   ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+   if (ac)
+   usage_with_options(worktree_usage, options);
+
+   return for_each_worktree(&print_worktree_details, NULL);
+}
+
 int cmd_worktree(int ac, const char **av, const char *prefix)
 {
struct option options[] = {
@@ -454,5 +494,7 @@ int cmd_worktree(int ac, const char **av, const char 
*prefix)
return add(ac - 1, av + 1, prefix);
if (!strcmp(av[1], "prune"))
return prune(ac - 1, av + 1, prefix);
+   if (!strcmp(av[1], "list"))
+   return list(ac - 1, av + 1, prefix);
usage_with_options(worktree_usage, options);
 }
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
new file mode 100755
index 000..85bd243
--- /dev/null
+++ b/t/t2027-worktree-list.sh
@@ -0,0 +1,100 @@
+#!/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
+'
+
+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 '"list" all worktrees --verbose from main'