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

[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:
>> diff --git a/worktree.h b/worktree.h
>> @@ -73,6 +73,16 @@ int is_main_worktree(const struct worktree *wt);
>> +/*
>> + * 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);
>
> A few more comments...
>
> It would be good to update the documentation to explain what `expire`
> is since it's not necessarily obvious. The documentation could also be
> tweaked to say that the worktree's path is returned in `wtpath` rather
> than saying only that it is returned. If you choose to make these
> changes, they should be probably done in a separate patch from the
> patch which moves the code. This is a very minor issue, not
> necessarily worth a re-roll.
>

Good idea. I will include these changes on the next revision. specially
the documentation about the `expire` field.

>
> Now that this is a public function, it might make sense for `wtpath`
> to be optional; if the caller is not interested in it, then NULL would
> be passed in for this argument. The implementation of
> should_prune_worktree() would need to be updated to check that
> `wtpath` is not NULL before assigning to it. This change, too, would
> be done in a separate patch. However, this is just a "would be nice to
> have" item, not at all required, and I don't think it should be added
> to this series since it's not needed by anything in this series, thus
> would just be noise (wasting your time and reviewer time). It's just
> something we can keep in mind for the future.

Agreed. These seems like reasonable changes and might not be good fit
for this patches but definitely something that we can follow up after it.

-- 
Thanks
Rafael