From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 22 Sep 2015 19:42:12 GMT
Subject: Re: [PATCH v8 4/4] worktree: add 'list' command
Message-ID: <xmqq37y644x7.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1442583027-47653-5-git-send-email-rappazzo@gmail.com>

Michael Rappazzo <rappazzo@gmail.com> writes:

>  
> +--porcelain::
> +	With `list`, output in an easy-to-parse format for scripts.
> +	This format will remain stable across Git versions and regardless of user
> +	configuration.

... and exactly what does it output?  That would be the first
question a reader of this documentation would ask.


> @@ -93,6 +106,7 @@ OPTIONS
>  --expire <time>::
>  	With `prune`, only expire unused working trees older than <time>.
>  
> +

?

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 71bb770..e6e36ac 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -8,10 +8,13 @@
>  #include "run-command.h"
>  #include "sigchain.h"
>  #include "refs.h"
> +#include "utf8.h"
> +#include "worktree.h"
>  
>  static const char * const worktree_usage[] = {
>  	N_("git worktree add [<options>] <path> <branch>"),
>  	N_("git worktree prune [<options>]"),
> +	N_("git worktree list [<options>]"),
>  	NULL
>  };
>  
> @@ -359,6 +362,79 @@ static int add(int ac, const char **av, const char *prefix)
>  	return add_worktree(path, branch, &opts);
>  }
>  
> +static void show_worktree_porcelain(struct worktree *worktree)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	strbuf_addf(&sb, "worktree %s\n", worktree->path);
> +	if (worktree->is_bare)
> +		strbuf_addstr(&sb, "bare");
> +	else {
> +		if (worktree->is_detached)
> +			strbuf_addf(&sb, "detached at %s", find_unique_abbrev(worktree->head_sha1, DEFAULT_ABBREV));
> +		else
> +			strbuf_addf(&sb, "branch %s", shorten_unambiguous_ref(worktree->head_ref, 0));
> +	}

Writing the above like this:

	if (worktree->is_bare)
        	...
	else if (worktree->is_detached)
		...
	else
                ...

would make it a lot more clear that there are three cases.

Also, I doubt --porcelain output wants shorten or abbrev.

Human-readability is not a goal.  Reproducibility is.  When you run
"worktree list" today and save the output, you want the output from
"worktree list" taken tomorrow to exactly match it, even after
creating many objects and tags with conflicting names with branches,
as long as you didn't change their HEADs in the meantime.

> +
> +	printf("%s\n\n", sb.buf);
> +
> +	strbuf_release(&sb);

I am not sure what the point of use of a strbuf is in this function,
though.  Two printf's for each case (one for the common "worktree
%s", the other inside if/elseif/else cascade) should be sufficient.

> +static void show_worktree(struct worktree *worktree, int path_maxlen)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +

Remove this blank line.  You are still declaring variables.

> +	int cur_len = strlen(worktree->path);
> +	int utf8_adj = cur_len - utf8_strwidth(worktree->path);

Have a blank line here, instead, as now you start your statements.

> +	strbuf_addf(&sb, "%-*s ", 1 + path_maxlen + utf8_adj, worktree->path);
> +	if (worktree->is_bare)
> +		strbuf_addstr(&sb, "(bare)");
> +	else {
> +		strbuf_addf(&sb, "%s ", find_unique_abbrev(worktree->head_sha1, DEFAULT_ABBREV));

Personally I am not a big fan of the the alignment and use of
utf8_strwidth(), but by using find_unique_abbrev() here, you are
breaking the alignment, aren't you?  " [branchname]" that follows
the commit object name would not start at the same column, when
you have many objects that default-abbrev is not enough to uniquely
identify them.

And it can easily be fixed by computing the unique-abbrev length for
all the non-bare worktree's HEADs in the same loop you computed
path_maxlen() in the caller, passing that to this function, and use
that as mininum abbrev length when computing the unique-abbrev.

> +		if (!worktree->is_detached)
> +			strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(worktree->head_ref, 0));
> +		else
> +			strbuf_addstr(&sb, "(detached HEAD)");
> +	}
> +	printf("%s\n", sb.buf);


> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
> new file mode 100755
> index 0000000..b68dfb4
> --- /dev/null
> +++ b/t/t2027-worktree-list.sh
> @@ -0,0 +1,86 @@
> +#!/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' '
> +	echo "$(git rev-parse --show-toplevel)       $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]" >expect &&

Are the number of SPs here significant and if so in what way?  Does
it depend on your environment or will there always be six of them?
Either way feels like an indication of a problem.

> +	git worktree add --detach here master &&
> +	test_when_finished "rm -rf here && git worktree prune" &&

Aren't these two the other way around?  When "add" fails in the
middle, you would want it to be removed to proceed to the next test
without leaving 'here' in the list of worktrees, no?