From: Rafael Silva <rafaeloliveira.cs@gmail.com>
Date: Tue, 19 Jan 2021 10:26:03 GMT
Subject: Re: [PATCH v2 5/6] worktree: teach `list` to annotate prunable worktree
Message-ID: <gohp6kpn216x51.fsf@gmail.com>
In-Reply-To: <CAPig+cRUrN62CDT+e+q02-S_sCD1Qvi5XtgU3D1dr9fXt--YJA@mail.gmail.com>


Eric Sunshine writes:

> On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:

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

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

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

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