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

[Rafael Silva <rafaeloliveira.cs@gmail.com>]

Eric Sunshine writes:

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

yeah, that's sounds better. will change it on the next revision.

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

Make sense.

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

Interesting point. Rewording the commit message like this seems better,
and as you mentioned this patch can stands on its own and can be more
clear about what the commit is introducing to the code instead of all
the previous message trying to explain why this exists.

Thank you for suggesting this commit message. I will revise and change on
the next revision.

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

Good point. I totally missed the caching aspect of the implementation
and I greed about getting the simple case out earlier in order to make it
simple to reason about it.

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

Thanks. I will add this suggestion in the next revision. 

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

Indeed. will add on the next revision.

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

Good point. I missed adding the message here for the `expire` parameter,
will add it. 

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

Also make sense to rephrase like this.


Thank you for this review will address all this changes on the v2.

-- 
Thanks
Rafael