[PATCH v6 0/2] worktree: for-each function and list command

[PATCH v6 0/2] worktree: for-each function and list command

[Michael Rappazzo]

Changes since v5:
 - used already existing functions instead of reimplementing:
     - resolve_ref_unsafe
     - find_unique_abbrev
     - shorten_unambiguous_ref
 - added more tests for 'worktree list'

Michael Rappazzo (2):
  worktree: add 'for_each_worktree' function
  worktree: add 'list' command

 Documentation/git-worktree.txt |  10 +++-
 builtin/worktree.c             | 125 +++++++++++++++++++++++++++++++++++++++++
 t/t2027-worktree-list.sh       | 100 +++++++++++++++++++++++++++++++++
 3 files changed, 233 insertions(+), 2 deletions(-)
 create mode 100755 t/t2027-worktree-list.sh

-- 
2.5.0

[PATCH v6 1/2] worktree: add 'for_each_worktree' function

[Michael Rappazzo]

for_each_worktree iterates through each worktree and invokes a callback
function.  The main worktree (if not bare) is always encountered first,
followed by worktrees created by `git worktree add`.

If the callback function returns a non-zero value, iteration stops, and
the callback's return value is returned from the for_each_worktree
function.

Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
---
 builtin/worktree.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 430b51e..7b3cb96 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -26,6 +26,14 @@ static int show_only;
 static int verbose;
 static unsigned long expire;
 
+/*
+ * The signature for the callback function for the for_each_worktree()
+ * function below.  The memory pointed to by the callback arguments
+ * is only guaranteed to be valid for the duration of a single
+ * callback invocation.
+ */
+typedef int each_worktree_fn(const char *path, const char *git_dir, void *cb_data);
+
 static int prune_worktree(const char *id, struct strbuf *reason)
 {
 	struct stat st;
@@ -359,6 +367,81 @@ static int add(int ac, const char **av, const char *prefix)
 	return add_worktree(path, branch, &opts);
 }
 
+/*
+ * Iterate through each worktree and invoke the callback function.  If
+ * the callback function returns non-zero, the iteration stops, and
+ * this function returns that return value
+ */
+static int for_each_worktree(each_worktree_fn fn, void *cb_data)
+{
+	struct strbuf worktree_path = STRBUF_INIT;
+	struct strbuf worktree_git = STRBUF_INIT;
+	const char *common_dir;
+	int main_is_bare = 0;
+	int ret = 0;
+
+	common_dir = get_git_common_dir();
+	if (!strcmp(common_dir, get_git_dir())) {
+		/* simple case - this is the main repo */
+		main_is_bare = is_bare_repository();
+		if (!main_is_bare) {
+			strbuf_addstr(&worktree_path, get_git_work_tree());
+		} else {
+			strbuf_addstr(&worktree_path, common_dir);
+		}
+	} else {
+		strbuf_addstr(&worktree_path, common_dir);
+		/* If the common_dir DOES NOT end with '/.git', then it is bare */
+		main_is_bare = !strbuf_strip_suffix(&worktree_path, "/.git");
+	}
+	strbuf_addstr(&worktree_git, worktree_path.buf);
+
+	if (!main_is_bare) {
+		strbuf_addstr(&worktree_git, "/.git");
+
+		ret = fn(worktree_path.buf, worktree_git.buf, cb_data);
+		if (ret)
+			goto done;
+	}
+	strbuf_addstr(&worktree_git, "/worktrees");
+
+	if (is_directory(worktree_git.buf)) {
+		DIR *dir = opendir(worktree_git.buf);
+		if (dir) {
+			struct stat st;
+			struct dirent *d;
+			size_t base_path_len = worktree_git.len;
+
+			while ((d = readdir(dir)) != NULL) {
+				if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
+					continue;
+
+				strbuf_setlen(&worktree_git, base_path_len);
+				strbuf_addf(&worktree_git, "/%s/gitdir", d->d_name);
+
+				if (stat(worktree_git.buf, &st)) {
+					fprintf(stderr, "Unable to read worktree git dir: %s\n", worktree_git.buf);
+					continue;
+				}
+
+				strbuf_reset(&worktree_path);
+				strbuf_read_file(&worktree_path, worktree_git.buf, st.st_size);
+				strbuf_strip_suffix(&worktree_path, "/.git\n");
+
+				strbuf_strip_suffix(&worktree_git, "/gitdir");
+				ret = fn(worktree_path.buf, worktree_git.buf, cb_data);
+				if (ret)
+					break;
+			}
+		}
+		closedir(dir);
+	}
+done:
+	strbuf_release(&worktree_git);
+	strbuf_release(&worktree_path);
+	return ret;
+}
+
 int cmd_worktree(int ac, const char **av, const char *prefix)
 {
 	struct option options[] = {
-- 
2.5.0

Re: [PATCH v6 1/2] worktree: add 'for_each_worktree' function

[Eric Sunshine]

Thanks for working on this. I apologize for not reviewing earlier
rounds (other than v2 [1]); it's been difficult to find a block of
time to do so...

On Sun, Aug 30, 2015 at 3:10 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:
> for_each_worktree iterates through each worktree and invokes a callback
> function.  The main worktree (if not bare) is always encountered first,
> followed by worktrees created by `git worktree add`.

Why does this iteration function specially filter out a bare main
worktree? If it instead unconditionally vends the main worktree (bare
or not), then the caller can make its own decision about what to do
with it, thus empowering the caller, rather than imposing a (possibly)
arbitrary restriction upon it.

For instance, the "git worktree list" command may very well want to
show the main worktree, even if bare (possibly controlled by a
command-line option), annotated appropriately ("[bare]"). This may be
exactly the sort of information a user wants to know, and by leaving
the decision up to the caller, then the caller ("git worktree list" in
this example) has the opportunity to act accordingly, whereas if
for_each_worktree() filters out a bare main worktree unconditionally,
then the caller ("git worktree list") will never be able to offer such
an option.

> If the callback function returns a non-zero value, iteration stops, and
> the callback's return value is returned from the for_each_worktree
> function.

Stepping back a bit, is a for-each-foo()-style interface desirable?
This sort of interface imposes a good deal of complexity on callers,
demanding a callback function and callback data (cb_data), and is
generally (at least in C) more difficult to reason about than other
simpler interfaces. Is such complexity warranted?

An alternate, much simpler interface would be to have a function, say
get_worktrees(), return an array of 'worktree' structures to the
caller, which the caller would iterate over (which is a common
operation in C, thus easily reasoned about).

The one benefit of a for-each-foo()-style interface is that it's
possible to "exit early", thus avoiding the cost of interrogating
meta-data for worktrees in which the caller is not interested,
however, it seems unlikely that there will be so many worktrees linked
to a repository for this early exit to translate into any real
savings.

> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 430b51e..7b3cb96 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -26,6 +26,14 @@ static int show_only;
>  static int verbose;
>  static unsigned long expire;
>
> +/*
> + * The signature for the callback function for the for_each_worktree()
> + * function below.  The memory pointed to by the callback arguments
> + * is only guaranteed to be valid for the duration of a single
> + * callback invocation.
> + */
> +typedef int each_worktree_fn(const char *path, const char *git_dir, void *cb_data);

In my review[1] of v2, I mentioned that (at some point) the "git
worktree list" command might like to show additional information about
the worktree other than just its location. Such information might
include its tag, the checked out branch (or detached HEAD), whether
it's locked (and the lock reason and whether the worktree is currently
"online"), whether it can be pruned (and the prune reason).

Commands such as "git worktree add" and "git checkout" need to know if
a branch is already checked out in some (other) worktree, thus they
also need the "branch" information.

This need by clients for additional worktree meta-data suggests that
the additional information ought to be encapsulated into a 'struct
worktree', and that for_each_worktree() should vend a 'struct
worktree' rather than vending merely the "git dir". (Alternately, the
above-suggested get_worktrees() should return an array of 'struct
worktree' items.)

An important reason for making for_each_worktree() vend a 'struct
worktree' is that it facilitates centralizing all the logic for
determining and computing the extra worktree meta-data in one place,
thus relieving callers of burden of having to compute the extra
information themselves. (Junio alluded to this in his v5 review[2].)

>  static int prune_worktree(const char *id, struct strbuf *reason)
>  {
>         struct stat st;
> @@ -359,6 +367,81 @@ static int add(int ac, const char **av, const char *prefix)
>         return add_worktree(path, branch, &opts);
>  }
>
> +/*
> + * Iterate through each worktree and invoke the callback function.  If
> + * the callback function returns non-zero, the iteration stops, and
> + * this function returns that return value
> + */
> +static int for_each_worktree(each_worktree_fn fn, void *cb_data)

Ultimately, code outside of builtin/worktree.c will want to benefit
from the encapsulation of this worktree iteration logic. For instance,
the support code in (top-level) branch.c invoked by "git worktree add"
and "git checkout" to determine if a branch is already checked out in
some (other) branch could avail itself of this iteration interface.

As such, it would make sense to place this code at location where it
can be accessed by callers other than just builtin/worktree.c. A good
location for it to reside might be in a new top-level file worktree.c.

It doesn't have to be part of the current patch series, but,
eventually, the code in branch.c for determining if a branch is
already checked out elsewhere also should migrate to the new top-level
worktree.c; in particular, die_if_checked_out(), find_shared_symref(),
and find_linked_symref().

> +{
> +       struct strbuf worktree_path = STRBUF_INIT;
> +       struct strbuf worktree_git = STRBUF_INIT;
> +       const char *common_dir;
> +       int main_is_bare = 0;
> +       int ret = 0;
> +
> +       common_dir = get_git_common_dir();
> +       if (!strcmp(common_dir, get_git_dir())) {
> +               /* simple case - this is the main repo */
> +               main_is_bare = is_bare_repository();
> +               if (!main_is_bare) {
> +                       strbuf_addstr(&worktree_path, get_git_work_tree());
> +               } else {
> +                       strbuf_addstr(&worktree_path, common_dir);
> +               }
> +       } else {
> +               strbuf_addstr(&worktree_path, common_dir);
> +               /* If the common_dir DOES NOT end with '/.git', then it is bare */
> +               main_is_bare = !strbuf_strip_suffix(&worktree_path, "/.git");
> +       }
> +       strbuf_addstr(&worktree_git, worktree_path.buf);

I may have missed some discussion in earlier rounds (or perhaps I'm
too simple-minded), but I'm confused about why this logic (and most of
the rest of the function) differs so much from existing logic in
branch.c:find_shared_symref() and find_linked_symref() for iterating
over the worktrees and gleaning information about them. That logic in
branch.c seems to do a pretty good job of reporting the worktree in
which a branch is already checked out, so it's not clear why the above
logic takes a different (and seemingly more complex) approach.

> +       if (!main_is_bare) {
> +               strbuf_addstr(&worktree_git, "/.git");
> +
> +               ret = fn(worktree_path.buf, worktree_git.buf, cb_data);
> +               if (ret)
> +                       goto done;
> +       }
> +       strbuf_addstr(&worktree_git, "/worktrees");
> +
> +       if (is_directory(worktree_git.buf)) {

As mentioned in my v2 review[1], this is_directory() invocation
doesn't buy you anything. The following opendir() will either succeed
or fail anyhow, so checking beforehand if 'worktree_git' is a
directory is wasted work. If you eliminate is_directory(), you avoid
that unnecessary work (and the rest of the code can be less deeply
indented).

> +               DIR *dir = opendir(worktree_git.buf);
> +               if (dir) {
> +                       struct stat st;
> +                       struct dirent *d;
> +                       size_t base_path_len = worktree_git.len;
> +
> +                       while ((d = readdir(dir)) != NULL) {
> +                               if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
> +                                       continue;
> +
> +                               strbuf_setlen(&worktree_git, base_path_len);
> +                               strbuf_addf(&worktree_git, "/%s/gitdir", d->d_name);
> +
> +                               if (stat(worktree_git.buf, &st)) {
> +                                       fprintf(stderr, "Unable to read worktree git dir: %s\n", worktree_git.buf);
> +                                       continue;
> +                               }
> +
> +                               strbuf_reset(&worktree_path);
> +                               strbuf_read_file(&worktree_path, worktree_git.buf, st.st_size);
> +                               strbuf_strip_suffix(&worktree_path, "/.git\n");
> +
> +                               strbuf_strip_suffix(&worktree_git, "/gitdir");
> +                               ret = fn(worktree_path.buf, worktree_git.buf, cb_data);
> +                               if (ret)
> +                                       break;
> +                       }
> +               }
> +               closedir(dir);

This closedir() should be inside the `if (dir) {...}' block rather than outside.

Returning to the issue of this code differing from existing code in
branch.c for iterating worktrees, I'm curious as to why this
functionality was implemented again from scratch here rather than
re-using the existing (somewhat) well proven code and logic in
branch.c:find_shared_symref() and find_linked_symref(). My v2
review[1] hinted at re-use a couple times.

Considering that the existing code in branch.c for iterating worktrees
has gone through several code reviews and is already (somewhat)
proven, re-using that code for a general purpose worktree iterator
makes lots of sense compared with writing it again from scratch, as is
done here. Consequently, I, personally, would be happier to see the
existing code refactored (perhaps over several patches) to the point
that it can be re-used as a general purpose iterator. But, that's my
opinion; I can't speak for others.

> +       }
> +done:
> +       strbuf_release(&worktree_git);
> +       strbuf_release(&worktree_path);
> +       return ret;
> +}

[1]: http://article.gmane.org/gmane.comp.version-control.git/275528
[2]: http://article.gmane.org/gmane.comp.version-control.git/276471

Re: [PATCH v6 1/2] worktree: add 'for_each_worktree' function

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Thanks for working on this....

<aol>metoo</aol>

> Stepping back a bit, is a for-each-foo()-style interface desirable?
> This sort of interface imposes a good deal of complexity on callers,
> demanding a callback function and callback data (cb_data), and is
> generally (at least in C) more difficult to reason about than other
> simpler interfaces. Is such complexity warranted?
>
> An alternate, much simpler interface would be to have a function, say
> get_worktrees(), return an array of 'worktree' structures to the
> caller, which the caller would iterate over (which is a common
> operation in C, thus easily reasoned about).

Nice.

Re: [PATCH v6 1/2] worktree: add 'for_each_worktree' function

[David Turner]

On Mon, 2015-08-31 at 01:11 -0400, Eric Sunshine wrote:
> Stepping back a bit, is a for-each-foo()-style interface desirable?
> This sort of interface imposes a good deal of complexity on callers,
> demanding a callback function and callback data (cb_data), and is
> generally (at least in C) more difficult to reason about than other
> simpler interfaces. Is such complexity warranted?
> 
> An alternate, much simpler interface would be to have a function, say
> get_worktrees(), return an array of 'worktree' structures to the
> caller, which the caller would iterate over (which is a common
> operation in C, thus easily reasoned about).
> 
> The one benefit of a for-each-foo()-style interface is that it's
> possible to "exit early", thus avoiding the cost of interrogating
> meta-data for worktrees in which the caller is not interested,
> however, it seems unlikely that there will be so many worktrees linked
> to a repository for this early exit to translate into any real
> savings.

The other benefit is that there is no need to worry about deallocating
the list.  But that might be too minor to worry about.

Re: [PATCH v6 1/2] worktree: add 'for_each_worktree' function

[Eric Sunshine]

On Mon, Aug 31, 2015 at 2:44 PM, David Turner <dturner@twopensource.com> wrote:
> On Mon, 2015-08-31 at 01:11 -0400, Eric Sunshine wrote:
>> Stepping back a bit, is a for-each-foo()-style interface desirable?
>> This sort of interface imposes a good deal of complexity on callers,
>> demanding a callback function and callback data (cb_data), and is
>> generally (at least in C) more difficult to reason about than other
>> simpler interfaces. Is such complexity warranted?
>>
>> An alternate, much simpler interface would be to have a function, say
>> get_worktrees(), return an array of 'worktree' structures to the
>> caller, which the caller would iterate over (which is a common
>> operation in C, thus easily reasoned about).
>>
>> The one benefit of a for-each-foo()-style interface is that it's
>> possible to "exit early", thus avoiding the cost of interrogating
>> meta-data for worktrees in which the caller is not interested,
>> however, it seems unlikely that there will be so many worktrees linked
>> to a repository for this early exit to translate into any real
>> savings.
>
> The other benefit is that there is no need to worry about deallocating
> the list.  But that might be too minor to worry about.

Probably. The burden of having to deallocate the returned array seems
quite minor compared to the complexity of the callback function
approach.

Also, unstated but implied with the suggestion of a get_worktrees()
function was that there would be a corresponding free_worktrees()
function to make cleanup easy.

Re: [PATCH v6 1/2] worktree: add 'for_each_worktree' function

[David Turner]

On Mon, 2015-08-31 at 15:03 -0400, Eric Sunshine wrote:
> On Mon, Aug 31, 2015 at 2:44 PM, David Turner <dturner@twopensource.com> wrote:
> > On Mon, 2015-08-31 at 01:11 -0400, Eric Sunshine wrote:
> >> Stepping back a bit, is a for-each-foo()-style interface desirable?
> >> This sort of interface imposes a good deal of complexity on callers,
> >> demanding a callback function and callback data (cb_data), and is
> >> generally (at least in C) more difficult to reason about than other
> >> simpler interfaces. Is such complexity warranted?
> >>
> >> An alternate, much simpler interface would be to have a function, say
> >> get_worktrees(), return an array of 'worktree' structures to the
> >> caller, which the caller would iterate over (which is a common
> >> operation in C, thus easily reasoned about).
> >>
> >> The one benefit of a for-each-foo()-style interface is that it's
> >> possible to "exit early", thus avoiding the cost of interrogating
> >> meta-data for worktrees in which the caller is not interested,
> >> however, it seems unlikely that there will be so many worktrees linked
> >> to a repository for this early exit to translate into any real
> >> savings.
> >
> > The other benefit is that there is no need to worry about deallocating
> > the list.  But that might be too minor to worry about.
> 
> Probably. The burden of having to deallocate the returned array seems
> quite minor compared to the complexity of the callback function
> approach.
> 
> Also, unstated but implied with the suggestion of a get_worktrees()
> function was that there would be a corresponding free_worktrees()
> function to make cleanup easy.

That's fine with me.  Sorry for leading you down the wrong path,
Michael.

Re: [PATCH v6 1/2] worktree: add 'for_each_worktree' function

[Mike Rappazzo]

On Mon, Aug 31, 2015 at 1:11 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> Thanks for working on this. I apologize for not reviewing earlier
> rounds (other than v2 [1]); it's been difficult to find a block of
> time to do so...

I appreciate your time and comments.

>
> On Sun, Aug 30, 2015 at 3:10 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:
>> for_each_worktree iterates through each worktree and invokes a callback
>> function.  The main worktree (if not bare) is always encountered first,
>> followed by worktrees created by `git worktree add`.
>
> Why does this iteration function specially filter out a bare main
> worktree? If it instead unconditionally vends the main worktree (bare
> or not), then the caller can make its own decision about what to do
> with it, thus empowering the caller, rather than imposing a (possibly)
> arbitrary restriction upon it.
>
> For instance, the "git worktree list" command may very well want to
> show the main worktree, even if bare (possibly controlled by a
> command-line option), annotated appropriately ("[bare]"). This may be
> exactly the sort of information a user wants to know, and by leaving
> the decision up to the caller, then the caller ("git worktree list" in
> this example) has the opportunity to act accordingly, whereas if
> for_each_worktree() filters out a bare main worktree unconditionally,
> then the caller ("git worktree list") will never be able to offer such
> an option.

I wasn't sure that a bare repo would be considered a worktree.  I
don't think that it would be
a good idea to include it.  In the same vein that I can't checkout a
branch in a bare repo, it
figure that it shouldn't be in the list.

>
>> If the callback function returns a non-zero value, iteration stops, and
>> the callback's return value is returned from the for_each_worktree
>> function.
>
> Stepping back a bit, is a for-each-foo()-style interface desirable?
> This sort of interface imposes a good deal of complexity on callers,
> demanding a callback function and callback data (cb_data), and is
> generally (at least in C) more difficult to reason about than other
> simpler interfaces. Is such complexity warranted?
>
> An alternate, much simpler interface would be to have a function, say
> get_worktrees(), return an array of 'worktree' structures to the
> caller, which the caller would iterate over (which is a common
> operation in C, thus easily reasoned about).
>
> The one benefit of a for-each-foo()-style interface is that it's
> possible to "exit early", thus avoiding the cost of interrogating
> meta-data for worktrees in which the caller is not interested,
> however, it seems unlikely that there will be so many worktrees linked
> to a repository for this early exit to translate into any real
> savings.

I am not opposed to making a simple function as you describe.  I think David was
looking for a callback style function.  I don't think it would be
terrible to keep the
callback and then also include the simple function to return the
struct array.  I like
the memory management of the callback better than the struct array though.


>
>> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
>> ---
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> index 430b51e..7b3cb96 100644
>> --- a/builtin/worktree.c
>> +++ b/builtin/worktree.c
>> @@ -26,6 +26,14 @@ static int show_only;
>>  static int verbose;
>>  static unsigned long expire;
>>
>> +/*
>> + * The signature for the callback function for the for_each_worktree()
>> + * function below.  The memory pointed to by the callback arguments
>> + * is only guaranteed to be valid for the duration of a single
>> + * callback invocation.
>> + */
>> +typedef int each_worktree_fn(const char *path, const char *git_dir, void *cb_data);
>
> In my review[1] of v2, I mentioned that (at some point) the "git
> worktree list" command might like to show additional information about
> the worktree other than just its location. Such information might
> include its tag, the checked out branch (or detached HEAD), whether
> it's locked (and the lock reason and whether the worktree is currently
> "online"), whether it can be pruned (and the prune reason).
>
> Commands such as "git worktree add" and "git checkout" need to know if
> a branch is already checked out in some (other) worktree, thus they
> also need the "branch" information.
>
> This need by clients for additional worktree meta-data suggests that
> the additional information ought to be encapsulated into a 'struct
> worktree', and that for_each_worktree() should vend a 'struct
> worktree' rather than vending merely the "git dir". (Alternately, the
> above-suggested get_worktrees() should return an array of 'struct
> worktree' items.)
>
> An important reason for making for_each_worktree() vend a 'struct
> worktree' is that it facilitates centralizing all the logic for
> determining and computing the extra worktree meta-data in one place,
> thus relieving callers of burden of having to compute the extra
> information themselves. (Junio alluded to this in his v5 review[2].)

I think that this makes sense.  I will refactor to use a struct, and
include that in
the callback signature and/or the struct array function.

>
>>  static int prune_worktree(const char *id, struct strbuf *reason)
>>  {
>>         struct stat st;
>> @@ -359,6 +367,81 @@ static int add(int ac, const char **av, const char *prefix)
>>         return add_worktree(path, branch, &opts);
>>  }
>>
>> +/*
>> + * Iterate through each worktree and invoke the callback function.  If
>> + * the callback function returns non-zero, the iteration stops, and
>> + * this function returns that return value
>> + */
>> +static int for_each_worktree(each_worktree_fn fn, void *cb_data)
>
> Ultimately, code outside of builtin/worktree.c will want to benefit
> from the encapsulation of this worktree iteration logic. For instance,
> the support code in (top-level) branch.c invoked by "git worktree add"
> and "git checkout" to determine if a branch is already checked out in
> some (other) branch could avail itself of this iteration interface.
>
> As such, it would make sense to place this code at location where it
> can be accessed by callers other than just builtin/worktree.c. A good
> location for it to reside might be in a new top-level file worktree.c.
>
> It doesn't have to be part of the current patch series, but,
> eventually, the code in branch.c for determining if a branch is
> already checked out elsewhere also should migrate to the new top-level
> worktree.c; in particular, die_if_checked_out(), find_shared_symref(),
> and find_linked_symref().
>
>> +{
>> +       struct strbuf worktree_path = STRBUF_INIT;
>> +       struct strbuf worktree_git = STRBUF_INIT;
>> +       const char *common_dir;
>> +       int main_is_bare = 0;
>> +       int ret = 0;
>> +
>> +       common_dir = get_git_common_dir();
>> +       if (!strcmp(common_dir, get_git_dir())) {
>> +               /* simple case - this is the main repo */
>> +               main_is_bare = is_bare_repository();
>> +               if (!main_is_bare) {
>> +                       strbuf_addstr(&worktree_path, get_git_work_tree());
>> +               } else {
>> +                       strbuf_addstr(&worktree_path, common_dir);
>> +               }
>> +       } else {
>> +               strbuf_addstr(&worktree_path, common_dir);
>> +               /* If the common_dir DOES NOT end with '/.git', then it is bare */
>> +               main_is_bare = !strbuf_strip_suffix(&worktree_path, "/.git");
>> +       }
>> +       strbuf_addstr(&worktree_git, worktree_path.buf);
>
> I may have missed some discussion in earlier rounds (or perhaps I'm
> too simple-minded), but I'm confused about why this logic (and most of
> the rest of the function) differs so much from existing logic in
> branch.c:find_shared_symref() and find_linked_symref() for iterating
> over the worktrees and gleaning information about them. That logic in
> branch.c seems to do a pretty good job of reporting the worktree in
> which a branch is already checked out, so it's not clear why the above
> logic takes a different (and seemingly more complex) approach.

This is due to my unfamiliarity of the code api.  I keep trying to
look for the right
functions to use, but I miss them.  Sorry.  I will rework using those functions.

>
>> +       if (!main_is_bare) {
>> +               strbuf_addstr(&worktree_git, "/.git");
>> +
>> +               ret = fn(worktree_path.buf, worktree_git.buf, cb_data);
>> +               if (ret)
>> +                       goto done;
>> +       }
>> +       strbuf_addstr(&worktree_git, "/worktrees");
>> +
>> +       if (is_directory(worktree_git.buf)) {
>
> As mentioned in my v2 review[1], this is_directory() invocation
> doesn't buy you anything. The following opendir() will either succeed
> or fail anyhow, so checking beforehand if 'worktree_git' is a
> directory is wasted work. If you eliminate is_directory(), you avoid
> that unnecessary work (and the rest of the code can be less deeply
> indented).

For some reason, in my testing, calling opendir was giving a
segmentation fault regardless of how I checked the return value.
Including the is_directory check allowed me to avoid that.

>
>> +               DIR *dir = opendir(worktree_git.buf);
>> +               if (dir) {
>> +                       struct stat st;
>> +                       struct dirent *d;
>> +                       size_t base_path_len = worktree_git.len;
>> +
>> +                       while ((d = readdir(dir)) != NULL) {
>> +                               if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
>> +                                       continue;
>> +
>> +                               strbuf_setlen(&worktree_git, base_path_len);
>> +                               strbuf_addf(&worktree_git, "/%s/gitdir", d->d_name);
>> +
>> +                               if (stat(worktree_git.buf, &st)) {
>> +                                       fprintf(stderr, "Unable to read worktree git dir: %s\n", worktree_git.buf);
>> +                                       continue;
>> +                               }
>> +
>> +                               strbuf_reset(&worktree_path);
>> +                               strbuf_read_file(&worktree_path, worktree_git.buf, st.st_size);
>> +                               strbuf_strip_suffix(&worktree_path, "/.git\n");
>> +
>> +                               strbuf_strip_suffix(&worktree_git, "/gitdir");
>> +                               ret = fn(worktree_path.buf, worktree_git.buf, cb_data);
>> +                               if (ret)
>> +                                       break;
>> +                       }
>> +               }
>> +               closedir(dir);
>
> This closedir() should be inside the `if (dir) {...}' block rather than outside.
>
> Returning to the issue of this code differing from existing code in
> branch.c for iterating worktrees, I'm curious as to why this
> functionality was implemented again from scratch here rather than
> re-using the existing (somewhat) well proven code and logic in
> branch.c:find_shared_symref() and find_linked_symref(). My v2
> review[1] hinted at re-use a couple times.
>
> Considering that the existing code in branch.c for iterating worktrees
> has gone through several code reviews and is already (somewhat)
> proven, re-using that code for a general purpose worktree iterator
> makes lots of sense compared with writing it again from scratch, as is
> done here. Consequently, I, personally, would be happier to see the
> existing code refactored (perhaps over several patches) to the point
> that it can be re-used as a general purpose iterator. But, that's my
> opinion; I can't speak for others.
>
>> +       }
>> +done:
>> +       strbuf_release(&worktree_git);
>> +       strbuf_release(&worktree_path);
>> +       return ret;
>> +}
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/275528
> [2]: http://article.gmane.org/gmane.comp.version-control.git/276471

Re: [PATCH v6 1/2] worktree: add 'for_each_worktree' function

[Eric Sunshine]

On Mon, Aug 31, 2015 at 2:57 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:
> On Mon, Aug 31, 2015 at 1:11 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sun, Aug 30, 2015 at 3:10 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:
>> Why does this iteration function specially filter out a bare main
>> worktree? If it instead unconditionally vends the main worktree (bare
>> or not), then the caller can make its own decision about what to do
>> with it, thus empowering the caller, rather than imposing a (possibly)
>> arbitrary restriction upon it.
>>
>> For instance, the "git worktree list" command may very well want to
>> show the main worktree, even if bare (possibly controlled by a
>> command-line option), annotated appropriately ("[bare]"). This may be
>> exactly the sort of information a user wants to know, and by leaving
>> the decision up to the caller, then the caller ("git worktree list" in
>> this example) has the opportunity to act accordingly, whereas if
>> for_each_worktree() filters out a bare main worktree unconditionally,
>> then the caller ("git worktree list") will never be able to offer such
>> an option.
>
> I wasn't sure that a bare repo would be considered a worktree.  I
> don't think that it would be
> a good idea to include it.  In the same vein that I can't checkout a
> branch in a bare repo, it
> figure that it shouldn't be in the list.

This is a mechanism vs. policy issue[1]. Low-level worker code, such
as this iteration function, should concern itself only with the
mechanics of retrieving and vending the worktree meta-data, and should
not make decisions (policy) about how that information is used by the
caller. Policy decisions (how the meta-data is used or displayed)
should be pushed to as high a level as possible, often up to the level
of user-interface (which is what "git worktree list" is).

[1]: http://www.catb.org/esr/writings/taoup/html/ch01s06.html#id2877777

>> Stepping back a bit, is a for-each-foo()-style interface desirable?
>> This sort of interface imposes a good deal of complexity on callers,
>> demanding a callback function and callback data (cb_data), and is
>> generally (at least in C) more difficult to reason about than other
>> simpler interfaces. Is such complexity warranted?
>>
>> An alternate, much simpler interface would be to have a function, say
>> get_worktrees(), return an array of 'worktree' structures to the
>> caller, which the caller would iterate over (which is a common
>> operation in C, thus easily reasoned about).
>>
>> The one benefit of a for-each-foo()-style interface is that it's
>> possible to "exit early", thus avoiding the cost of interrogating
>> meta-data for worktrees in which the caller is not interested,
>> however, it seems unlikely that there will be so many worktrees linked
>> to a repository for this early exit to translate into any real
>> savings.
>
> I am not opposed to making a simple function as you describe.  I think David was
> looking for a callback style function.  I don't think it would be
> terrible to keep the
> callback and then also include the simple function to return the
> struct array.  I like
> the memory management of the callback better than the struct array though.

We should stick with one or the other. Having both complicates the
code unnecessarily and increases maintenance costs. I, personally,
prefer the get_worktrees() approach for its client-side simplicity.
With a corresponding free_worktrees() to dispose of the resources
allocated by get_worktrees(), memory management shouldn't be much of a
burden for callers (other than having to remember to call it).

>> I may have missed some discussion in earlier rounds (or perhaps I'm
>> too simple-minded), but I'm confused about why this logic (and most of
>> the rest of the function) differs so much from existing logic in
>> branch.c:find_shared_symref() and find_linked_symref() for iterating
>> over the worktrees and gleaning information about them. That logic in
>> branch.c seems to do a pretty good job of reporting the worktree in
>> which a branch is already checked out, so it's not clear why the above
>> logic takes a different (and seemingly more complex) approach.
>
> This is due to my unfamiliarity of the code api.  I keep trying to
> look for the right
> functions to use, but I miss them.  Sorry.  I will rework using those functions.

The API is indeed large and complex, and it can be difficult to get a
handle on how to accomplish various tasks. That's also a good argument
for re-using the existing (proven) code in
branch.c:find_shared_symref() and find_linked_symref() rather than
re-inventing it from scratch. In its current form, the branch.c code
doesn't look much like a general-purpose iterator function, but it
should be possible to refactor if over the course of a few patches so
that it does satisfy that goal.

>>> +       if (!main_is_bare) {
>>> +               strbuf_addstr(&worktree_git, "/.git");
>>> +
>>> +               ret = fn(worktree_path.buf, worktree_git.buf, cb_data);
>>> +               if (ret)
>>> +                       goto done;
>>> +       }
>>> +       strbuf_addstr(&worktree_git, "/worktrees");
>>> +
>>> +       if (is_directory(worktree_git.buf)) {
>>
>> As mentioned in my v2 review[1], this is_directory() invocation
>> doesn't buy you anything. The following opendir() will either succeed
>> or fail anyhow, so checking beforehand if 'worktree_git' is a
>> directory is wasted work. If you eliminate is_directory(), you avoid
>> that unnecessary work (and the rest of the code can be less deeply
>> indented).
>
> For some reason, in my testing, calling opendir was giving a
> segmentation fault regardless of how I checked the return value.
> Including the is_directory check allowed me to avoid that.

Was it the opendir() that was crashing or the closedir()? Considering
that the closedir() incorrectly resides outside the `if (dir) {...}'
block, rather than inside, I could easily see the closedir() crashing
when called with NULL.

Re: [PATCH v6 1/2] worktree: add 'for_each_worktree' function

[Eric Sunshine]

On Mon, Aug 31, 2015 at 2:57 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:
> I wasn't sure that a bare repo would be considered a worktree.  I
> don't think that it would be
> a good idea to include it.  In the same vein that I can't checkout a
> branch in a bare repo, it
> figure that it shouldn't be in the list.

I forgot to mention in my previous response that I have the opposite
view, and think that a bare repo should be included in the output of
"git worktree list". The reason is that the intention of "git worktree
list" is to give the user a consolidated view of the locations of all
components of his "workspace". By "workspace", I mean the repository
(bare or not) and its worktrees.

In the typical case, the .git directory resides within the main
worktree (the first item output by "git worktree list"), thus is
easily found, however, if "git worktree list" hides bare repos, then
the user will have no way to easily locate the repository (without
resorting to lower-level commands or peeking at configuration files).

Re: [PATCH v6 1/2] worktree: add 'for_each_worktree' function

[Mike Rappazzo]

On Mon, Aug 31, 2015 at 3:47 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Aug 31, 2015 at 2:57 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:
>> I wasn't sure that a bare repo would be considered a worktree.  I
>> don't think that it would be
>> a good idea to include it.  In the same vein that I can't checkout a
>> branch in a bare repo, it
>> figure that it shouldn't be in the list.
>
> I forgot to mention in my previous response that I have the opposite
> view, and think that a bare repo should be included in the output of
> "git worktree list". The reason is that the intention of "git worktree
> list" is to give the user a consolidated view of the locations of all
> components of his "workspace". By "workspace", I mean the repository
> (bare or not) and its worktrees.
>
> In the typical case, the .git directory resides within the main
> worktree (the first item output by "git worktree list"), thus is
> easily found, however, if "git worktree list" hides bare repos, then
> the user will have no way to easily locate the repository (without
> resorting to lower-level commands or peeking at configuration files).

This makes sense, but would we also want to decorate it in the `git
worktree list`
command?  Would porcelain list output be able to differentiate it?

Re: [PATCH v6 1/2] worktree: add 'for_each_worktree' function

[Eric Sunshine]

On Mon, Aug 31, 2015 at 3:54 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:
> On Mon, Aug 31, 2015 at 3:47 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Mon, Aug 31, 2015 at 2:57 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:
>>> I wasn't sure that a bare repo would be considered a worktree.  I
>>> don't think that it would be
>>> a good idea to include it.  In the same vein that I can't checkout a
>>> branch in a bare repo, it
>>> figure that it shouldn't be in the list.
>>
>> I forgot to mention in my previous response that I have the opposite
>> view, and think that a bare repo should be included in the output of
>> "git worktree list". The reason is that the intention of "git worktree
>> list" is to give the user a consolidated view of the locations of all
>> components of his "workspace". By "workspace", I mean the repository
>> (bare or not) and its worktrees.
>>
>> In the typical case, the .git directory resides within the main
>> worktree (the first item output by "git worktree list"), thus is
>> easily found, however, if "git worktree list" hides bare repos, then
>> the user will have no way to easily locate the repository (without
>> resorting to lower-level commands or peeking at configuration files).
>
> This makes sense, but would we also want to decorate it in the `git
> worktree list`
> command?  Would porcelain list output be able to differentiate it?

I don't have strong feelings about decorating the bare repository, if
by "decorate", you mean adding a "[bare]" annotation or something.
Verbose mode can certainly do so.

For porcelain mode, we can (and should) be explicit about it.

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

[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 <rappazzo@gmail.com>
---
 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 <new-branch>] <path> [<branch>]
 'git worktree prune' [-n] [-v] [--expire <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 <time>::
 	With `prune`, only expire unused working trees older than <time>.
 
+
 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 [<options>] <path> <branch>"),
 	N_("git worktree prune [<options>]"),
+	N_("git worktree list [<options>]"),
 	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 0000000..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' '
+	echo "$(git rev-parse --show-toplevel)  [$(git symbolic-ref --short HEAD)]" >expect &&
+	git worktree add --detach here master &&
+	echo "$(git -C here rev-parse --show-toplevel)  [$(git -C here rev-parse --short HEAD)]" >>expect &&
+	git worktree list --verbose >actual &&
+	test_cmp expect actual &&
+	rm -rf here &&
+	git worktree prune
+'
+
+test_expect_success '"list" all worktrees --verbose from linked' '
+	echo "$(git rev-parse --show-toplevel)  [$(git symbolic-ref --short HEAD)]" >expect &&
+	git worktree add --detach here master &&
+	echo "$(git -C here rev-parse --show-toplevel)  [$(git -C here rev-parse --short HEAD)]" >>expect &&
+	git -C here worktree list --verbose >actual &&
+	test_cmp expect actual &&
+	rm -rf here &&
+	git worktree prune
+'
+
+test_expect_success 'bare repo setup' '
+	git init --bare bare1 &&
+	echo "data" > file1 &&
+	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 "$(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 from worktree with a bare main' '
+	git -C bare1 worktree add --detach ../there master &&
+	echo "$(git -C there rev-parse --show-toplevel)" >expect &&
+	git -C there 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 "$(git -C there rev-parse --show-toplevel)  [$(git -C there rev-parse --short HEAD)]" >expect &&
+	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 "$(git -C there rev-parse --show-toplevel)  [$(git -C there rev-parse --short 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
+'
+
+test_done
-- 
2.5.0

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

[Eric Sunshine]

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