Re: [PATCH 4/7] worktree: teach `list` prunable annotation and verbose

> On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
>> The "git worktree list" command shows the absolute path to the worktree,
>> the commit that is checked out, the name of the branch, and a "locked"
>> annotation if the worktree is locked. It is not clear whether a worktree,
>> if any, is prunable.
>
> Maybe this could just say...
>
>     ... "locked" annotation if the worktree is locked,
>     however, it does not indicate whether it is prunable.
>

>> The "prune" command will remove a worktree in case
>> is a prunable candidate unless --dry-run option is specified. This could
>
> s/case is/case it is/
>
> Or better:
>
>     ... will remove a worktree if it is prunable unless...
>

>> lead to a worktree being removed without the user realizing before is to
>> late, in case the user forgets to pass --dry-run for instance.
>
> s/before is/before it is/
> s/to/too/
>

>> If the "list" command shows which worktree is prunable, the user could
>> verify before running "git worktree prune" and hopefully prevents the
>> working tree to be removed "accidently" on the worse case scenario.
>>
>> Let's teach "git worktree list" to show when a worktree is prunable by
>> appending "prunable" text to its output by default and show a prunable
>> label for the porcelain format followed by the reason, if the avaiable.
>> While at it, let's do the same for the "locked" annotation.
>
> s/avaiable/available/
>

>> Also, the worktree_prune_reason() stores the reason why git is selecting
>> the worktree to be pruned, so let's leverage that and also display this
>> information to the user. However, the reason is human readable text that
>> can take virtually any size which might make harder to extend the "list"
>> command with additional annotations and not fit nicely on the screen.
>>
>> In order to address this shortcoming, let's teach "git worktree list" to
>> take a verbose option that will output the prune reason on the next line
>> indented, if the reason is available, otherwise the annotation is kept on
>> the same line. While at it, let's do the same for the "locked"
>> annotation.
>
> This is a lot of changes for one patch to be making, and it's hard for
> a reader to digest all those changes from the commit message. I think
> I counted four distinct changes being made here:
>
> 1. extend porcelain to include lock line (with optional reason)
> 2. add prunable annotation to normal list output
> 3. add prune line (with optional reason) to porcelain output
> 4. extend normal list output with --verbose to include reasons
>
> The patch itself is not overly large or complicated, so perhaps
> combining all these changes together is reasonable, although I'm quite
> tempted to ask for them to be separated into at least four patches
> (probably in the order shown above). A benefit of splitting them into
> distinct patches is that you can add targeted documentation and test
> updates to each individual patch rather than saving all the
> documentation and test updates for a single (large) patch at the end
> of the series. This helps reviewers reason about the changes more
> easily since they get to see how each change impacts the documentation
> and tests rather than having to wait for a patch late in the series to
> make all the documentation and test updates, at which point the
> reviewers may have forgotten details of the earlier patches.
>
> (I've come back here after reading and reviewing the patch itself, and
> I'm on the fence as to whether to suggest splitting this into multiple
> patches. The changes in this patch are easy enough to understand and
> digest, so I'm not convinced it makes sense to ask you to do all the
> extra work of splitting it into smaller pieces. On the other hand, it
> might be nice to have each documentation and test update done in the
> patch which makes each particular change. So, I don't have a good
> answer. Use your best judgment and do the amount of work you feel is
> appropriate.)
>

>> The output of "git worktree list" with verbose becomes like so:
>>
>>     $ git worktree list --verbose
>>     /path/to/main             aba123 [main]
>>     /path/to/locked           acb124 [branch-a] locked
>>     /path/to/locked-reason    acc125 [branch-b]
>>         locked: worktree with locked reason
>>     /path/to/prunable         acd126 [branch-c] prunable
>>     /path/to/prunable-reason  ace127 [branch-d]
>>         prunable: gitdir file points to non-existent location
>
> One "weird" aesthetic issue is that if the lock reason contains
> newlines, the subsequent lines of the lock reason are not indented.
> However, this is a very minor point and I don't think this patch
> series should worry about it. We can think about how to address it
> some time in the future if someone ever complains about it.
>

>> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
>> ---
>>  builtin/worktree.c | 35 +++++++++++++++++++++++++++++++++--
>>  1 file changed, 33 insertions(+), 2 deletions(-)
>>
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> index eeb3ffaed0..dedd4089e5 100644
>> --- a/builtin/worktree.c
>> +++ b/builtin/worktree.c
>> @@ -578,6 +578,20 @@ static void show_worktree_porcelain(struct worktree *wt)
>> +               if (worktree_lock_reason(wt)) {
>> +                       if (*wt->lock_reason)
>> +                               printf("locked %s\n", wt->lock_reason);
>> +                       else
>> +                               printf("locked\n");
>> +               }
>> +
>> +               if (worktree_prune_reason(wt, expire)) {
>> +                       if (*wt->prune_reason)
>> +                               printf("prunable %s\n", wt->prune_reason);
>> +                       else
>> +                               printf("prunable\n");
>> +               }
>
> A couple observations...
>
> As a consumer of `struct worktree`, builtin/worktree.c should not be
> poking at or accessing the structure's private fields `prune_reason`
> and `lock_reason`; instead it should be retrieving those values via
> worktree_prune_reason() and worktree_lock_reason() which are part of
> the public API.
>

> If a worktree is prunable, then worktree_prune_reason() will
> unconditionally return a (non-empty) string; if it is not prunable,
> then it will unconditionally return NULL. This means that the
> `printf("prunable\n")` case is dead-code; it will never be reached.
> This differs from worktree_lock_reason() which can return an empty
> string to indicate a locked worktree for which no reason has been
> specified.
>

> Taking the above observations into account, a reasonable rewrite would be:
>
>     const char *reason;
>
>     reason = worktree_lock_reason(wt);
>     if (reason && *reason)
>         printf("locked %s\n", reason);
>     else if (reason)
>         printf("locked\n");
>
>     reason = worktree_prune_reason(wt, expire);
>     if (reason)
>         printf("prunable %s\n", reason);
>

>> @@ -604,8 +618,19 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
>>                         strbuf_addstr(&sb, "(error)");
>>         }
>>
>> -       if (!is_main_worktree(wt) && worktree_lock_reason(wt))
>> -               strbuf_addstr(&sb, " locked");
>> +       if (worktree_lock_reason(wt)) {
>> +               if (verbose && *wt->lock_reason)
>> +                       strbuf_addf(&sb, "\n\tlocked: %s", wt->lock_reason);
>> +               else
>> +                       strbuf_addstr(&sb, " locked");
>> +       }
>> +
>> +       if (worktree_prune_reason(wt, expire)) {
>> +               if (verbose && *wt->prune_reason)
>> +                       strbuf_addf(&sb, "\n\tprunable: %s", wt->prune_reason);
>> +               else
>> +                       strbuf_addstr(&sb, " prunable");
>> +       }
>
> Same observations here about using public API rather than touching
> private `struct worktree` fields, and about the final `else` case
> being dead code.
>

>> @@ -650,12 +675,18 @@ static int list(int ac, const char **av, const char *prefix)
>>                 OPT_BOOL(0, "porcelain", &porcelain, N_("machine-readable output")),
>> +               OPT__VERBOSE(&verbose, N_("show extended annotations and reasons, if available")),
>> +               OPT_EXPIRY_DATE(0, "expire", &expire,
>> +                               N_("show working trees that is candidate to be pruned older than <time>")),
>
> Perhaps:
>
>     "add 'prunable' annotation to worktrees older than <time>"
>

>>                 OPT_END()
>>         };
>>
>> +       expire = TIME_MAX;
>
> Good, this mirrors how prune() initializes this variable.