Re: [PATCH v3 4/7] worktree: expose function to retrieve worktree names

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 5, 2025, 08:42 UTC
Message-ID: <CAPig+cSDDbhGrym8j=PFKBCUxBQhZPzAHXGvKy-Z6POA4Ju3sw@mail.gmail.com>
In-Reply-To: <20250502-pks-maintenance-missing-tasks-v3-4-13e130d36640@pks.im>

On Fri, May 2, 2025 at 4:44 AM Patrick Steinhardt <ps@pks.im> 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@pks.im>

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?

Show 26 quoted lines

> diff --git a/worktree.c b/worktree.c
> @@ -988,6 +988,36 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,
> +int get_worktree_names(struct repository *repo, struct strvec *out)
> +{
> +       worktrees_dir = repo_git_path(repo, "worktrees");
> +       dir = opendir(worktrees_dir);
> +       if (!dir) {
> +               if (errno == ENOENT) {
> +                       ret = 0;
> +                       goto out;
> +               }
> +
> +               ret = -1;
> +               goto out;
> +       }
> +
> +       while ((d = readdir_skip_dot_and_dotdot(dir)) != NULL)
> +               strvec_push(out, d->d_name);
> +
> +       ret = 0;
> +out:
> +       if (dir)
> +               closedir(dir);
> +       free(worktrees_dir);
> +       return ret;
> +}

It's subjective, but although we often recommend the `goto` approach on this project to ensure proper cleanup, the above seems to place an unnecessarily high cognitive load on the reader. I would think that the following straightforward `goto`-free approach would suffice:

    worktrees_dir = repo_git_path(repo, "worktrees");
    dir = opendir(worktrees_dir);
    FREE_AND_NULL(worktrees_dir);
    if (!dir)
        return errno == ENOENT ? 0 : -1;
    while ((d = readdir_skip_dot_and_dotdot(dir)) != NULL)
        strvec_push(out, d->d_name);
    closedir(dir);
    return 0

Show 9 quoted lines

> diff --git a/worktree.h b/worktree.h
> @@ -38,6 +38,14 @@ struct worktree **get_worktrees(void);
> +/*
> + * Retrieve all worktree names. Not all names may correspond to a fully
> + * functional worktree. Returns 0 on success, a negative error code on failure.
> + * Calling the function on a repository that doesn't have any worktrees is not
> + * considered an error.
> + */
> +int get_worktree_names(struct repository *repo, struct strvec *out);

As I was reading through the patch, I was worried that you might have overlooked the fact that the names returned by the function might not be fully functional worktrees or that the caller of this function might not realize that, so I'm glad to see that you documented this potential downside.

In fact, there is no guarantee that the returned entries are even directories; they could be anything that someone happened to create in the .git/worktrees directory, such as plain files, special files, etc. So, aside from the objections I wrote above, the term "names" here is potentially unclear and misleading, as used both in the documentation and the function name itself, and the fact that I can't come up with a better term further leads me to believe that this is not a function we really want to be publishing.