Re: [PATCH 1/7] worktree: move should_prune_worktree() to worktree.c
flat view
From: Eric Sunshine <hidden>
Date: 2021-01-06 06:00:24
On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva [off-list ref] 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()
As part of teaching "git worktree list" to annotate worktree that is a candidate for pruning, let's move should_prune_worktree() from builtin/worktree.c to worktree.c in order to make part of the worktree public API.
Good.
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.)
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".
quoted hunk ↗ jump to hunk
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.