On Mon, May 05, 2025 at 04:42:40AM -0400, Eric Sunshine wrote:
> On Fri, May 2, 2025 at 4:44 AM Patrick Steinhardt <ps@xxxxxx> wrote:
> > Introduce a function that retrieves worktree names as present in
> > ".git/worktrees". This function will be used in a subsequent commit.
> >
> > Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
>
> I'm not convinced that this patch or the get_worktree_names() function
> which it adds to worktree.[hc] adds value. Aside from the mere act of
> consulting the directory at repo_git_path(r, "worktrees"), there is
> nothing about the function at all related to worktrees. It doesn't
> make any guarantees, such as only returning entries which at least
> superficially look like worktree-metadata directories, or perform any
> sort of validation. I don't see how this is any better than the caller
> just implementing its own bog-standard opendir() / readdir()-loop /
> closedir() over repo_git_path(r, "worktrees"). Or, if you don't want
> the caller to implement its own readdir()-loop, I wouldn't be
> surprised if we already have a function which does exactly this for a
> provided path, though I haven't checked. If there isn't such a generic
> function, perhaps it makes more sense to add one and call it with
> repo_git_path(r, "worktrees") as its input?
I think this is fair criticism indeed, and open-coding this isn't even
any more complex, either, as shown by the below patch. I'll drop this
patch.
Patrick
diff --git a/builtin/gc.c b/builtin/gc.c
index 82f0fac81a4..eb4469b7858 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -351,11 +351,11 @@ static int maintenance_task_worktree_prune(struct maintenance_run_opts *opts UNU
static int worktree_prune_condition(struct gc_config *cfg)
{
- struct strvec worktrees = STRVEC_INIT;
- struct strbuf reason = STRBUF_INIT;
+ struct strbuf buf = STRBUF_INIT;
+ int should_prune = 0, limit = 1;
timestamp_t expiry_date;
- int should_prune = 0;
- int limit = 1;
+ struct dirent *d;
+ DIR *dir = NULL;
git_config_get_int("maintenance.worktree-prune.auto", &limit);
if (limit <= 0) {
@@ -363,15 +363,18 @@ static int worktree_prune_condition(struct gc_config *cfg)
goto out;
}
- if (parse_expiry_date(cfg->prune_worktrees_expire, &expiry_date) ||
- get_worktree_names(the_repository, &worktrees) < 0)
+ if (parse_expiry_date(cfg->prune_worktrees_expire, &expiry_date))
+ goto out;
+
+ dir = opendir(repo_git_path_replace(the_repository, &buf, "worktrees"));
+ if (!dir)
goto out;
- for (size_t i = 0; i < worktrees.nr; i++) {
+ while ((d = readdir_skip_dot_and_dotdot(dir))) {
char *wtpath;
- strbuf_reset(&reason);
- if (should_prune_worktree(worktrees.v[i], &reason, &wtpath, expiry_date)) {
+ strbuf_reset(&buf);
+ if (should_prune_worktree(d->d_name, &buf, &wtpath, expiry_date)) {
limit--;
if (!limit) {
@@ -384,8 +387,9 @@ static int worktree_prune_condition(struct gc_config *cfg)
}
out:
- strvec_clear(&worktrees);
- strbuf_release(&reason);
+ if (dir)
+ closedir(dir);
+ strbuf_release(&buf);
return should_prune;
}
