Re: [PATCH 1/7] worktree: move should_prune_worktree() to worktree.c

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

Thank you for the detailed review.

Eric Sunshine writes:

> On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
>> worktree: move should_prune_worktree() to worktree.c
>
> The subject is a bit confusing since this function already lives in
> `worktree.c` (albeit within the `builtin` directory). An alternate way
> to word this might be:
>
>     worktree: libify and publish should_prune_worktree()
>
> or just:
>
>     worktree: libify should_prune_worktree()
>

yes, I think these seems clearer. I will drop the current subject and
use one of these on the next revision.

>> This function will be used by another API function, implemented in the
>> next patch that will accept a pointer to "worktree" structure directly
>> making it easier to implement the "prunable" annotations.
>
> Okay, though I'm not sure this paragraph adds value since the reader
> understands implicitly from the first paragraph that this is the
> reason this patch is libifying the function. (I'd probably drop this
> paragraph if I was composing the message.)
>

Make sense.

>> should_prune_worktree() knows how to select the given worktree for pruning
>> based on an expiration date, however the expiration value is stored on a
>> global variable and it is not local to the function. In order to move the
>> function, teach should_prune_worktree() to take the expiration date as an
>> argument.
>
> Rather than "on a global variable", it would be more accurate to say
> "static file-scope variable".
>

Good point. I will definitely change that to be more accurate on the next
revision.   

>> diff --git a/worktree.c b/worktree.c
>> @@ -700,3 +700,76 @@ void repair_worktree_at_path(const char *path,
>> +/*
>> + * Return true if worktree entry should be pruned, along with the reason for
>> + * pruning. Otherwise, return false and the worktree's path, or NULL if it
>> + * cannot be determined. Caller is responsible for freeing returned path.
>> + */
>> +int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, timestamp_t expire)
>
> The function documentation which was moved here (worktree.c) from
> builtin/worktree.c is duplicated in the header worktree.h. Having it
> in the header is good. Having it here in the source file doesn't
> necessarily hurt, but I worry about it becoming out-of-sync with the
> copy in the header. For that reason, it might make sense to keep it
> only in the header.
>
> All of the above review comments are very minor and several of them
> are subjective, thus not necessarily worth a re-roll.

Make sense as well. 

-- 
Thanks
Rafael