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

Eric Sunshine writes:

On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva
[off-list ref] wrote:

quoted

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/

Oops. thanks for catching that. Will fix it in the next revision.

quoted

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.

Fair enough. The `*reason` condition was indeed added to be safe and
future-proofing and, as you pointed it out, we know that currently
the worktree_prune_reason() returns a non-empty string when its true
and when the code reaches the `*reason` condition in
"if (reason && *reason)" this will always evaluates to true.

Agreed that removing this part of the condition will make the code
clearer and easier to followed. So I will drop this in the next
revision.

quoted

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

That's a good point, and even worse this is not mentioned in my commit
message at all. It make sense to move this change into [3/6] where the
API is changed and all the reason is explained in the commit message.

quoted

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

That's a good point. I'm inclined to leave the [5/6] with the following:

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

And move up the changes that includes the `reason` into the [5/6]
patches that introduces the --verbose option. This line seems easier to
follow when the reader is looking on this patch alone and only care
about a reason when the --verbose comes into play in the next patch
[6/6].

Although your suggestion about changing the patch also sounds reasonable
and I'll take into consideration when I re-roll this series.

Btw, I don't mind spending extra work on and I'm all forward
with the changes so we it would be easier to understand not only now where
all the patches are being reviewed together but in the future when
someone is looking at the history of the project, specially for
debugging/bisecting reasons.

Thanks for the insightful comments.

-- 
Thanks
Rafael