From: Mike Rappazzo <rappazzo@gmail.com>
Date: Thu, 24 Sep 2015 02:58:53 GMT
Subject: Re: [PATCH v8 4/4] worktree: add 'list' command
Message-ID: <CANoM8SX28TPt_U8v9HO4yo=M46-JKn5S11DrhZ0-EdrjkDT3EQ@mail.gmail.com>
In-Reply-To: <xmqq37y644x7.fsf@gitster.mtv.corp.google.com>

On Tue, Sep 22, 2015 at 3:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.
>

I will add that description.  I have mostly followed what Eric
suggested in the v7 series for a porcelain format.  Does the porcelain
format restrict additive changes?  That is, is it OK for a future
patch to add another field in the format, as long as it doesn't alter
the other values?  Is the format that I have used here acceptable
(assuming the changes proposed below are made)?

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

I only intended for the path to be right padded, since paths can vary
in length so widely.  I found it much easier to read with the path
right-padded.  I think that doing a full column align isn't the best
looking output in this case.  I tried to model this output after `git
branch -vv`.  I didn't notice if that does a full align if the
shortened refs are differently sized.  I will try it out, and see if
it makes a significant visual impact.

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

The number of spaces is significant in this case, but it should not be
platform dependent.  It is just the padding for the different worktree
paths.

>
>> +     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?

Sure, I'll switch it.