Re: [PATCH v7 4/8] worktree: simplify find_shared_symref() memory ownership model
From: Eric Sunshine <hidden>
Date: 2021-12-01 23:10:35
On Wed, Dec 1, 2021 at 5:16 PM Anders Kaseorg [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Storing the worktrees list in a static variable meant that find_shared_symref() had to rebuild the list on each call (which is inefficient when the call site is in a loop), and also that each call invalidated the pointer returned by the previous call (which is confusing). Instead, make it the caller’s responsibility to pass in the worktrees list and manage its lifetime. Signed-off-by: Anders Kaseorg <redacted> ---diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c@@ -1486,12 +1486,17 @@ static const char *update(struct command *cmd, struct shallow_info *si) - const struct worktree *worktree = is_bare_repository() ? NULL : find_shared_symref("HEAD", name); + struct worktree **worktrees = get_worktrees(); + const struct worktree *worktree = + is_bare_repository() ? + NULL : + find_shared_symref(worktrees, "HEAD", name);
As far as I can see, this code only cares whether find_shared_symref()
returned a result; it doesn't actually consult the returned worktree
at all, thus it semantically considers `worktree` as a boolean, not as
a `struct worktree`. I see in [7/8] that you do add an access to the
`worktree.is_bare` field, but that also is used purely in a boolean
fashion. If my understanding is correct, then it seems as if it would
be cleaner and make for a much less noisy patch if you did this
instead:
struct worktree **worktrees = get_worktrees();
int has_worktree = 0;
if (!is_bare_repository()) {
struct worktree *w = find_shared_symref(worktrees, ...);
has_worktree = !!w;
}
free_worktrees(worktrees);
...
if (has_worktree) {
...
}
and in patch [7/8] augment that to:
int is_worktree_bare = 0;
if (!is_bare_repository()) {
struct worktree *w = find_shared_symref(worktrees, ...);
if (w) {
has_worktree = 1;
is_worktree_bare = w->is_bare;
}
}
free_worktrees(worktrees);
which would then allow you to...
if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
rp_error("refusing to create funny ref '%s' remotely", name);
- return "funny refname";
+ ret = "funny refname";
+ goto out;
}... drop this change and all the other similar changes to this file since `worktrees` gets cleaned up as soon as `has_worktree` and `is_worktree_bare` have been determined, so there's no need to introduce an `out` label just to clean up `worktrees` at the end of the function.