[PATCH v3] worktree: add 'list' command

[PATCH v3] worktree: add 'list' command

[Michael Rappazzo]

Differences from [v2](http://www.mail-archive.com/git@vger.kernel.org/msg75467.html)
   - removed unintended whitespace changes
   - cleanup based on comments from v2

Michael Rappazzo (1):
  worktree: add 'list' command

 Documentation/git-worktree.txt |  6 +++-
 builtin/worktree.c             | 67 ++++++++++++++++++++++++++++++++++++++++++
 t/t2027-worktree-list.sh       | 30 +++++++++++++++++++
 3 files changed, 102 insertions(+), 1 deletion(-)
 create mode 100755 t/t2027-worktree-list.sh

-- 
2.5.0

[PATCH v3] worktree: add 'list' command

[Michael Rappazzo]

'git worktree list' will list the main worktree followed by any linked
worktrees which were created using 'git worktree add'.

Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
---
 Documentation/git-worktree.txt |  6 +++-
 builtin/worktree.c             | 67 ++++++++++++++++++++++++++++++++++++++++++
 t/t2027-worktree-list.sh       | 30 +++++++++++++++++++
 3 files changed, 102 insertions(+), 1 deletion(-)
 create mode 100755 t/t2027-worktree-list.sh

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 3387e2f..efb8e9d 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'
 
 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
 -------
 
@@ -167,7 +172,6 @@ performed manually, such as:
 - `remove` to remove a linked worktree and its administrative files (and
   warn if the worktree is dirty)
 - `mv` to move or rename a worktree and update its administrative files
-- `list` to list linked worktrees
 - `lock` to prevent automatic pruning of administrative files (for instance,
   for a worktree on a portable device)
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 6a264ee..d90bdee 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -10,6 +10,7 @@
 static const char * const worktree_usage[] = {
 	N_("git worktree add [<options>] <path> <branch>"),
 	N_("git worktree prune [<options>]"),
+	N_("git worktree list [<options>]"),
 	NULL
 };
 
@@ -316,6 +317,70 @@ static int add(int ac, const char **av, const char *prefix)
 	return add_worktree(path, cmd.argv);
 }
 
+static int list(int ac, const char **av, const char *prefix)
+{
+	int main_only = 0;
+	struct option options[] = {
+		OPT_BOOL(0, "main-only", &main_only, N_("only list the main worktree")),
+		OPT_END()
+	};
+
+	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+	if (ac)
+		usage_with_options(worktree_usage, options);
+
+	struct strbuf main_path = STRBUF_INIT;
+	const char* common_dir = get_git_common_dir();
+	int is_bare = is_bare_repository();
+	if (is_bare) {
+		strbuf_addstr(&main_path, absolute_path(common_dir));
+		strbuf_strip_suffix(&main_path, "/.");
+	} else if (!strcmp(common_dir, ".git")) {
+		/* within a worktree, the common dir only returns ".git" */
+		strbuf_addstr(&main_path, get_git_work_tree());
+	} else {
+		strbuf_addstr(&main_path, common_dir);
+		strbuf_strip_suffix(&main_path, "/.git");
+	}
+
+	printf("%s\n", main_path.buf);
+
+	if (!is_bare) {
+		strbuf_addstr(&main_path, "/.git");
+	}
+	strbuf_addstr(&main_path, "/worktrees");
+
+	if (is_directory(main_path.buf)) {
+		DIR *dir = opendir(main_path.buf);
+		if (dir) {
+			struct dirent *d;
+			struct stat st;
+			struct strbuf path = STRBUF_INIT;
+			struct strbuf other_worktree_path = STRBUF_INIT;
+			while ((d = readdir(dir)) != NULL) {
+				if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
+					continue;
+				strbuf_reset(&other_worktree_path);
+				strbuf_addf(&other_worktree_path, "%s/%s/gitdir", main_path.buf, d->d_name);
+
+				if (stat(other_worktree_path.buf, &st))
+					continue;
+
+				strbuf_reset(&path);
+				strbuf_read_file(&path, other_worktree_path.buf, st.st_size);
+				strbuf_strip_suffix(&path, "/.git\n");
+
+				printf("%s\n", path.buf);
+			}
+			strbuf_release(&other_worktree_path);
+			strbuf_release(&path);
+		}
+		closedir(dir);
+	}
+	strbuf_release(&main_path);
+	return 0;
+}
+
 int cmd_worktree(int ac, const char **av, const char *prefix)
 {
 	struct option options[] = {
@@ -328,5 +393,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 0000000..8e3dbbc
--- /dev/null
+++ b/t/t2027-worktree-list.sh
@@ -0,0 +1,30 @@
+#!/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_done
-- 
2.5.0

Re: [PATCH v3] worktree: add 'list' command

[Junio C Hamano]

Michael Rappazzo <rappazzo@gmail.com> writes:

> +static int list(int ac, const char **av, const char *prefix)
> +{
> +	int main_only = 0;
> +	struct option options[] = {
> +		OPT_BOOL(0, "main-only", &main_only, N_("only list the main worktree")),
> +		OPT_END()
> +	};

Hmm, main-only is still there?

> +
> +	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
> +	if (ac)
> +		usage_with_options(worktree_usage, options);
> +
> +	struct strbuf main_path = STRBUF_INIT;
> +	const char* common_dir = get_git_common_dir();

Asterisks stick to the variable names, not types.

> +	int is_bare = is_bare_repository();

Please do not introduce decl-after-stmt.

> +	if (is_bare) {
> +		strbuf_addstr(&main_path, absolute_path(common_dir));

Hmm, interesting.

Because .git/config is shared, core.bare read from that tells us if
the "main" one is bare, even if you start this command from one of
its linked worktrees.  So in that sense, this test of is_bare
correctly tells if "main" one is a bare repository.

But that by itself feels wrong.  Doesn't the presense of a working
tree mean that you should not get "is_bare==true" in such a case
(i.e. your "main" one is bare, you are in a linked worktree of it
that has the index and the working tree)?

Duy?  Eric?  What do you guys think?

There are many codepaths that change their behaviour (e.g. if we
create reflogs by default) based on the return value of
is_bare_repository().  If I am reading this correctly, I _think_ a
new working area that was prepared with "git worktree add" out of a
bare repository would not work well, as these operations behave as
if we do not have a working tree.  Perhaps is_bare_repository() in
such a working area _should_ say "No", even if core.bare in the
shared bare one is set to true.

> +		strbuf_strip_suffix(&main_path, "/.");

In any case, what is that stripping of "/." about?  Who is adding
that extra trailing string?

What I am getting at is (1) perhaps it shouldn't be adding that in
the first place, and (2) if some other code is randomly adding "/."
at the end, what guarantees you that you would need to strip it only
once here---if the other code added that twice, don't you have to
repeatedly remove "/." from the end?

Re: [PATCH v3] worktree: add 'list' command

[Mike Rappazzo]

On Mon, Aug 10, 2015 at 6:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael Rappazzo <rappazzo@gmail.com> writes:
>
>> +static int list(int ac, const char **av, const char *prefix)
>> +{
>> +     int main_only = 0;
>> +     struct option options[] = {
>> +             OPT_BOOL(0, "main-only", &main_only, N_("only list the main worktree")),
>> +             OPT_END()
>> +     };
>
> Hmm, main-only is still there?

Sorry, I missed that.

>
>> +     int is_bare = is_bare_repository();
>
> Please do not introduce decl-after-stmt.

Since I reused this value below, I thought it would be acceptable.

>> +     if (is_bare) {
>> +             strbuf_addstr(&main_path, absolute_path(common_dir));
>
> Hmm, interesting.
>
> Because .git/config is shared, core.bare read from that tells us if
> the "main" one is bare, even if you start this command from one of
> its linked worktrees.  So in that sense, this test of is_bare
> correctly tells if "main" one is a bare repository.
>
> But that by itself feels wrong.  Doesn't the presense of a working
> tree mean that you should not get "is_bare==true" in such a case
> (i.e. your "main" one is bare, you are in a linked worktree of it
> that has the index and the working tree)?

Is is even correct for a bare repo to be included in the list?  I
think that is part of what you are asking here.

>
>> +             strbuf_strip_suffix(&main_path, "/.");
>
> In any case, what is that stripping of "/." about?  Who is adding
> that extra trailing string?
>
> What I am getting at is (1) perhaps it shouldn't be adding that in
> the first place, and (2) if some other code is randomly adding "/."
> at the end, what guarantees you that you would need to strip it only
> once here---if the other code added that twice, don't you have to
> repeatedly remove "/." from the end?

In the case of a common dir, the value returned is just '.'.  I wanted
to resolve that to the full path, so I called
`absolute_path(common_dir)`.  Hence the trailing "/.".  Similarly, in
the main repo, the common dir is just ".git", and I handled that by
using `get_git_work_tree()`.

Re: [PATCH v3] worktree: add 'list' command

[Junio C Hamano]

Mike Rappazzo <rappazzo@gmail.com> writes:

>>> +     int is_bare = is_bare_repository();
>>
>> Please do not introduce decl-after-stmt.
>
> Since I reused this value below, I thought it would be acceptable.

Use of a new variable is fine.  "Do not declare one in a block after
you already wrote statement" is what "decl-after-stmt not allowed"
means.  In your patch:

+static int list(int ac, const char **av, const char *prefix)
+{
+	int main_only = 0;
+	struct option options[] = {
+		OPT_BOOL(0, "main-only", &main_only, N_("only list the main worktree")),
+		OPT_END()
+	};
+
+	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+	if (ac)
+		usage_with_options(worktree_usage, options);
+
+	struct strbuf main_path = STRBUF_INIT;
+	const char* common_dir = get_git_common_dir();
+	int is_bare = is_bare_repository();

Three variables, main_path, common_dir and is_bare are declared here
after statements such as a call to parse_options().  Don't.

+static int list(int ac, const char **av, const char *prefix)
+{
+	int main_only = 0;
+	struct strbuf main_path = STRBUF_INIT;
+	const char *common_dir;
+	int is_bare;
+	struct option options[] = {
+		OPT_BOOL(0, "main-only", &main_only, N_("only list the main worktree")),
+		OPT_END()
+	};
+
+	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+	if (ac)
+		usage_with_options(worktree_usage, options);
+
+	common_dir = get_git_common_dir();
+	int is_bare = is_bare_repository();

Re: [PATCH v3] worktree: add 'list' command

[David Turner]

On Mon, 2015-08-10 at 16:53 -0400, Michael Rappazzo wrote:
> +			while ((d = readdir(dir)) != NULL) {

I think it would be useful to break this loop out into a
for_each_worktree function.

While looking into per-worktree ref stuff, I have just noticed that git
prune will delete objects that are only referenced in a different
worktree's detached HEAD.  To fix this, git prune will need to walk over
each worktree, looking at that worktree's HEAD (and other per-worktree
refs).  It would be useful to be able to reuse some of this code for
that task.

Re: [PATCH v3] worktree: add 'list' command

[Mike Rappazzo]

On Mon, Aug 10, 2015 at 10:55 PM, David Turner <dturner@twopensource.com> wrote:
> On Mon, 2015-08-10 at 16:53 -0400, Michael Rappazzo wrote:
>> +                     while ((d = readdir(dir)) != NULL) {
>
> I think it would be useful to break this loop out into a
> for_each_worktree function.
>
> While looking into per-worktree ref stuff, I have just noticed that git
> prune will delete objects that are only referenced in a different
> worktree's detached HEAD.  To fix this, git prune will need to walk over
> each worktree, looking at that worktree's HEAD (and other per-worktree
> refs).  It would be useful to be able to reuse some of this code for
> that task.
>

I agree, but I will save that for another round.

Re: [PATCH v3] worktree: add 'list' command

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> On Mon, 2015-08-10 at 16:53 -0400, Michael Rappazzo wrote:
>> +			while ((d = readdir(dir)) != NULL) {
>
> I think it would be useful to break this loop out into a
> for_each_worktree function.

Very good point.