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

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