Re: [PATCH 1/7] worktree: move should_prune_worktree() to worktree.c
flat view
From: Eric Sunshine <hidden>
Date: 2021-01-06 06:56:27
On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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. 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.