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?
> 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
> 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.
