Re: [PATCH v2 5/6] worktree: teach `list` to annotate prunable worktree

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Jan 17, 2021 at 6:43 PM 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, however, it does not indicate
> whether the worktree is prunable.
> [...]
> Let's teach "git worktree list" to show when a worktree is a prunable
> candidate for both default and porcelain format.

Good.

> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -234,6 +235,9 @@ This can also be set up as the default behaviour by using the
>  --expire <time>::
>         With `prune`, only expire unused working trees older than `<time>`.
> ++
> +With `list`, annotate missing working trees as prunable if they are
> +older than `<mtime>`.

s/mtime/time/

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -592,6 +592,10 @@ static void show_worktree_porcelain(struct worktree *wt)
> +       reason = worktree_prune_reason(wt, expire);
> +       if (reason && *reason)
> +               printf("prunable %s\n", reason);

I lean toward not including `*reason` in the condition here. While one
may argue that `*reason` is future-proofing the condition, we know
today that worktree_prune_reason() will only ever return NULL or a
non-empty string. So, having `*reason` in the condition is misleading
and confuses readers into thinking that worktree_prune_reason() could
return an empty string or that perhaps it did in the past. And
studying the history of this file or even this commit won't help them
understand why the author included `*reason` in the condition.

> @@ -617,9 +622,14 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
> -       if (!is_main_worktree(wt) && worktree_lock_reason(wt))
> +       reason = worktree_lock_reason(wt);
> +       if (reason)
>                 strbuf_addstr(&sb, " locked");

Although I understand what happened here because I read the entire
series, for anyone reading this patch in the future or even someone
reading this patch in isolation, the disappearance of
is_main_worktree() from the condition is mysterious. They won't know
if its removal was an accident or intentional, or if the change
introduces a bug. Therefore, it would be better to drop
is_main_worktree() from this conditional in patch [3/6], which is the
patch which makes it safe to call worktree_lock_reason() even for the
main worktree. Thus, in [3/6], this would change from:

    if (!is_main_worktree(wt) && worktree_lock_reason(wt))

to:

    if (worktree_lock_reason(wt))

and then in this patch [5/6], it becomes:

    reason = worktree_lock_reason(wt);
    if (reason)

> +       reason = worktree_prune_reason(wt, expire);
> +       if (reason)
> +               strbuf_addstr(&sb, " prunable");

Looking at this also makes me wonder if patches [5/6] and [6/6] should
be swapped since it's not clear to the reader why you're adding the
`reason` variable in this patch when the same could be accomplished
more simply:

    if (worktree_lock_reason(wt))
        strbuf_addstr(&sb, " locked");
    if (worktree_prune_reason(wt, expire))
        strbuf_addstr(&sb, " prunable");

If you swap the patches and add --verbose mode first which requires
this new `reason` variable, then the mystery goes away, and the use of
`reason` is obvious when `prunable` annotation is added in the
subsequent patch.

Having said that, I'm not trying to make extra work for you by
expecting patch perfection. Sometimes it's fine to craft a patch in
such a way that it makes subsequent patches simpler, even if it looks
slightly odd in the current patch, and I haven't read [6/6] yet, so
whatever opinion I express here is based only upon what I've read up
to this point.