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

Thank you for the detailed review.

Eric Sunshine writes:

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
[off-list ref] wrote:

quoted

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.

quoted

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.

quoted

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.   

quoted

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