Thank you for the detailed review. Eric Sunshine writes: > On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva > <rafaeloliveira.cs@xxxxxxxxx> 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
