On Wed, 1 Dec 2021, Eric Sunshine wrote:
> If we instead hoist ownership of `worktrees` up to execute_commands()
> -- which calls execute_commands_atomic() or
> execute_commands_non_atomic() -- then we can get by with retrieving
> the worktrees just once, and all those noise changes in update() can
> be dropped since it will no longer be responsible for allocating or
> freeing `worktrees`.
Seems reasonable to me. It’s a smaller textual change, potentially a
larger conceptual change, but the efficiency improvement is probably
worthwhile.
That would modify patch 4 to the below. Patches 5 through 8 cleanly
rebase past this modification.
Anders
-- >8 --
Subject: [PATCH v7½ 4/8] worktree: simplify find_shared_symref() memory ownership model
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 <andersk@xxxxxxx>
---
branch.c | 14 ++++++++------
builtin/branch.c | 7 ++++++-
builtin/notes.c | 6 +++++-
builtin/receive-pack.c | 29 ++++++++++++++++++++---------
worktree.c | 8 ++------
worktree.h | 5 +++--
6 files changed, 44 insertions(+), 25 deletions(-)
diff --git a/branch.c b/branch.c
index 85777b939b..c66b222abd 100644
--- a/branch.c
+++ b/branch.c
@@ -357,14 +357,16 @@ void remove_branch_state(struct repository *r, int verbose)
void die_if_checked_out(const char *branch, int ignore_current_worktree)
{
+ struct worktree **worktrees = get_worktrees();
const struct worktree *wt;
- wt = find_shared_symref("HEAD", branch);
- if (!wt || (ignore_current_worktree && wt->is_current))
- return;
- skip_prefix(branch, "refs/heads/", &branch);
- die(_("'%s' is already checked out at '%s'"),
- branch, wt->path);
+ wt = find_shared_symref(worktrees, "HEAD", branch);
+ if (wt && (!ignore_current_worktree || !wt->is_current)) {
+ skip_prefix(branch, "refs/heads/", &branch);
+ die(_("'%s' is already checked out at '%s'"), branch, wt->path);
+ }
+
+ free_worktrees(worktrees);
}
int replace_each_worktree_head_symref(const char *oldref, const char *newref,
diff --git a/builtin/branch.c b/builtin/branch.c
index 7a1d1eeb07..d8f2164cd7 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -193,6 +193,7 @@ static void delete_branch_config(const char *branchname)
static int delete_branches(int argc, const char **argv, int force, int kinds,
int quiet)
{
+ struct worktree **worktrees;
struct commit *head_rev = NULL;
struct object_id oid;
char *name = NULL;
@@ -229,6 +230,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
if (!head_rev)
die(_("Couldn't look up commit object for HEAD"));
}
+
+ worktrees = get_worktrees();
+
for (i = 0; i < argc; i++, strbuf_reset(&bname)) {
char *target = NULL;
int flags = 0;
@@ -239,7 +243,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
if (kinds == FILTER_REFS_BRANCHES) {
const struct worktree *wt =
- find_shared_symref("HEAD", name);
+ find_shared_symref(worktrees, "HEAD", name);
if (wt) {
error(_("Cannot delete branch '%s' "
"checked out at '%s'"),
@@ -300,6 +304,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
free(name);
strbuf_release(&bname);
+ free_worktrees(worktrees);
return ret;
}
diff --git a/builtin/notes.c b/builtin/notes.c
index 71c59583a1..7f60408dbb 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -861,15 +861,19 @@ static int merge(int argc, const char **argv, const char *prefix)
update_ref(msg.buf, default_notes_ref(), &result_oid, NULL, 0,
UPDATE_REFS_DIE_ON_ERR);
else { /* Merge has unresolved conflicts */
+ struct worktree **worktrees;
const struct worktree *wt;
/* Update .git/NOTES_MERGE_PARTIAL with partial merge result */
update_ref(msg.buf, "NOTES_MERGE_PARTIAL", &result_oid, NULL,
0, UPDATE_REFS_DIE_ON_ERR);
/* Store ref-to-be-updated into .git/NOTES_MERGE_REF */
- wt = find_shared_symref("NOTES_MERGE_REF", default_notes_ref());
+ worktrees = get_worktrees();
+ wt = find_shared_symref(worktrees, "NOTES_MERGE_REF",
+ default_notes_ref());
if (wt)
die(_("a notes merge into %s is already in-progress at %s"),
default_notes_ref(), wt->path);
+ free_worktrees(worktrees);
if (create_symref("NOTES_MERGE_REF", default_notes_ref(), NULL))
die(_("failed to store link to current notes ref (%s)"),
default_notes_ref());
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index ddda27c184..925ce7f32d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1477,7 +1477,8 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w
return retval;
}
-static const char *update(struct command *cmd, struct shallow_info *si)
+static const char *update(struct command *cmd, struct shallow_info *si,
+ struct worktree **worktrees)
{
const char *name = cmd->ref_name;
struct strbuf namespaced_name_buf = STRBUF_INIT;
@@ -1486,7 +1487,10 @@ static const char *update(struct command *cmd, struct shallow_info *si)
struct object_id *old_oid = &cmd->old_oid;
struct object_id *new_oid = &cmd->new_oid;
int do_update_worktree = 0;
- const struct worktree *worktree = is_bare_repository() ? NULL : find_shared_symref("HEAD", name);
+ const struct worktree *worktree =
+ is_bare_repository() ?
+ NULL :
+ find_shared_symref(worktrees, "HEAD", name);
/* only refs/... are allowed */
if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
@@ -1579,7 +1583,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)
}
if (do_update_worktree) {
- ret = update_worktree(new_oid->hash, find_shared_symref("HEAD", name));
+ ret = update_worktree(new_oid->hash,
+ find_shared_symref(worktrees, "HEAD",
+ name));
if (ret)
return ret;
}
@@ -1842,7 +1848,8 @@ static void warn_if_skipped_connectivity_check(struct command *commands,
}
static void execute_commands_non_atomic(struct command *commands,
- struct shallow_info *si)
+ struct shallow_info *si,
+ struct worktree **worktrees)
{
struct command *cmd;
struct strbuf err = STRBUF_INIT;
@@ -1859,7 +1866,7 @@ static void execute_commands_non_atomic(struct command *commands,
continue;
}
- cmd->error_string = update(cmd, si);
+ cmd->error_string = update(cmd, si, worktrees);
if (!cmd->error_string
&& ref_transaction_commit(transaction, &err)) {
@@ -1873,7 +1880,8 @@ static void execute_commands_non_atomic(struct command *commands,
}
static void execute_commands_atomic(struct command *commands,
- struct shallow_info *si)
+ struct shallow_info *si,
+ struct worktree **worktrees)
{
struct command *cmd;
struct strbuf err = STRBUF_INIT;
@@ -1891,7 +1899,7 @@ static void execute_commands_atomic(struct command *commands,
if (!should_process_cmd(cmd) || cmd->run_proc_receive)
continue;
- cmd->error_string = update(cmd, si);
+ cmd->error_string = update(cmd, si, worktrees);
if (cmd->error_string)
goto failure;
@@ -1925,6 +1933,7 @@ static void execute_commands(struct command *commands,
struct async muxer;
int err_fd = 0;
int run_proc_receive = 0;
+ struct worktree **worktrees;
if (unpacker_error) {
for (cmd = commands; cmd; cmd = cmd->next)
@@ -2004,10 +2013,12 @@ static void execute_commands(struct command *commands,
(cmd->run_proc_receive || use_atomic))
cmd->error_string = "fail to run proc-receive hook";
+ worktrees = get_worktrees();
if (use_atomic)
- execute_commands_atomic(commands, si);
+ execute_commands_atomic(commands, si, worktrees);
else
- execute_commands_non_atomic(commands, si);
+ execute_commands_non_atomic(commands, si, worktrees);
+ free_worktrees(worktrees);
if (shallow_update)
warn_if_skipped_connectivity_check(commands, si);
diff --git a/worktree.c b/worktree.c
index 092a4f92ad..cf13d63845 100644
--- a/worktree.c
+++ b/worktree.c
@@ -402,17 +402,13 @@ int is_worktree_being_bisected(const struct worktree *wt,
* bisect). New commands that do similar things should update this
* function as well.
*/
-const struct worktree *find_shared_symref(const char *symref,
+const struct worktree *find_shared_symref(struct worktree **worktrees,
+ const char *symref,
const char *target)
{
const struct worktree *existing = NULL;
- static struct worktree **worktrees;
int i = 0;
- if (worktrees)
- free_worktrees(worktrees);
- worktrees = get_worktrees();
-
for (i = 0; worktrees[i]; i++) {
struct worktree *wt = worktrees[i];
const char *symref_target;
diff --git a/worktree.h b/worktree.h
index 8b7c408132..9e06fcbdf3 100644
--- a/worktree.h
+++ b/worktree.h
@@ -143,9 +143,10 @@ void free_worktrees(struct worktree **);
/*
* Check if a per-worktree symref points to a ref in the main worktree
* or any linked worktree, and return the worktree that holds the ref,
- * or NULL otherwise. The result may be destroyed by the next call.
+ * or NULL otherwise.
*/
-const struct worktree *find_shared_symref(const char *symref,
+const struct worktree *find_shared_symref(struct worktree **worktrees,
+ const char *symref,
const char *target);
/*
--
2.34.1
