Re: [PATCH 2/7] worktree: implement worktree_prune_reason() wrapper

> On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
>> worktree: implement worktree_prune_reason() wrapper
>
> We might be able to give the reader more useful information in the
> subject by explaining a bit more the goal of this patch. Perhaps
> something like this:
>
>     worktree: teach worktree to lazy-load "prunable" reason
>

>> The should_prune_worktree() machinery is used by the "prune" command to
>> identify whether a worktree is a candidate for pruning. This function
>> however, is not prepared to work directly with "struct worktree" and
>> refactoring is required not only on the function itself, but also also
>> changing get_worktrees() to return non-valid worktrees and address the
>> changes in all "worktree" sub commands.
>>
>> Instead let's implement worktree_prune_reason() that accepts
>> "struct worktree" and uses should_prune_worktree() and returns whether
>> the given worktree is a candidate for pruning. As the "list" sub command
>> already uses a list of "struct worktree", this allow to simply check if
>> the working tree prunable by passing the structure directly without the
>> others parameters.
>
> Everything through "not prepared to work directly with `struct
> worktree`" makes sense when explaining why you are adding this wrapper
> function, however, most of what follows is describing an aborted idea
> about how you originally intended to implement this. The bit about
> get_worktrees() not being able to return non-valid worktrees is
> certainly an important limitation in the overall scheme of things, but
> doesn't really help to sell the change made by this patch, and it
> probably confuses the reader who didn't also read the cover-letter,
> especially since this patch does nothing to help that situation.
>
> It also isn't really necessary to talk about `git worktree list` at
> this stage since this patch stands on its own by fleshing out the API
> without having to cite a specific client.
>

>> Also, let's add prune_reason field to the worktree structure that will
>> store the reason why the worktree can be pruned that is returned by
>> should_prune_worktree() when such reason is available.
>
> In my opinion, this is the real reason this patch exists, thus should
> be the focus of the commit message. All the description above this
> paragraph can likely be dropped.
>
> Taking the above comments into account, perhaps the entire commit
> message could be collapsed to something like this:
>
>     worktree: teach worktree to lazy-load "prunable" reason
>
>     Add worktree_prune_reason() to allow a caller to discover whether
>     a worktree is prunable and the reason that it is, much like
>     worktree_lock_reason() indicates whether a worktree is locked and
>     the reason for the lock. As with worktree_lock_reason(), retrieve
>     the prunable reason lazily and cache it in the `worktree`
>     structure.
>

>> diff --git a/worktree.c b/worktree.c
>> @@ -15,6 +15,7 @@ void free_worktrees(struct worktree **worktrees)
>>                 free(worktrees[i]->lock_reason);
>> +               free(worktrees[i]->prune_reason);
>>                 free(worktrees[i]);
>
> Remembering to free the prune-reason. Good.
>
>> @@ -245,6 +246,24 @@ const char *worktree_lock_reason(struct worktree *wt)
>> +const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire)
>> +{
>> +       if (!is_main_worktree(wt)) {
>> +               char *path;
>> +               struct strbuf reason = STRBUF_INIT;
>> +
>> +               if (should_prune_worktree(wt->id, &reason, &path, expire))
>> +                       wt->prune_reason = strbuf_detach(&reason, NULL);
>> +               else
>> +                       wt->prune_reason = NULL;
>> +
>> +               free(path);
>> +               strbuf_release(&reason);
>> +       }
>> +
>> +       return wt->prune_reason;
>> +}
>
> A couple observations...
>
> I realize you patterned this after worktree_lock_reason(), however, it
> is more common in this codebase to return early from the function for
> conditions such as `is_main_worktree(wt)` which don't require any
> additional processing. One reason is that doing so allows us to lose
> an indentation level. Another is that it is easier to reason about the
> rest of the function if we get the simple cases out of the way early,
> such that we don't have to think about them again while reading the
> remainder of the code.
>
> If I'm not mistaken, the intention here was to cache `prune_reason`
> for reuse, however, this function just overwrites it each time it's
> called for a particular worktree, thus providing no caching and
> leaking the previously-retrieved reason as well. To fix this, I think
> you need to add a private `prune_reason_valid` member to `struct
> worktree` (similar to `lock_reason_valid`) and check it before calling
> should_prune_worktree().
>

> Taking the above comments into account, perhaps it should be written like this:
>
>     if (is_main_worktree(wt))
>         return NULL;
>     if (wt->prune_reason_valid)
>         return wt->prune_reason;
>     if (should_prune_worktree(wt->id, &reason, &path, expire))
>         wt->prune_reason = strbuf_detach(&reason, NULL);
>     wt_prune_reason_valid = 1;
>     free(path);
>     strbuf_release(&reason);
>

>> diff --git a/worktree.h b/worktree.h
>> @@ -11,6 +11,7 @@ struct worktree {
>>         char *id;
>>         char *head_ref;         /* NULL if HEAD is broken or detached */
>>         char *lock_reason;      /* private - use worktree_lock_reason */
>> +       char *prune_reason;     /* private - use worktree_prune_reason */
>
> As noted above, we also probably need a new `prune_reason_valid`
> member, similar to the existing `lock_reason_valid`.
>

>> @@ -73,6 +74,12 @@ int is_main_worktree(const struct worktree *wt);
>> +/*
>> + * Return the reason string if the given worktree should be pruned
>> + * or NULL otherwise.
>> + */
>> +const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire);
>
> The documentation should also talk about `expire` since its purpose
> and meaning is unclear.
>

> Nit: I realize that you patterned this description after the one for
> worktree_lock_reason(), but it's a bit unclear. Perhaps rephrasing it
> like this would help:
>
>     Return the reason the worktree should be pruned, otherwise
>     NULL if it should not be pruned.