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@mit.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/branch.c b/branch.c
index 85777b939b5af21d32cf1be8c8509391c84d0c76..c66b222abd4fbfe4af758bd452e176eb7858fff3 100644 (file)
--- 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 81b5c111cb7b4b5e2700e963dd41678e2dafc493..6c8b0fcc11fbca461884b6459fd1405293e4ec89 100644 (file)
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -192,6 +192,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;
@@ -228,6 +229,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;
@@ -238,7 +242,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'"),
@@ -299,6 +303,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 71c59583a17f8da6eb948248e19a262781091f28..7f60408dbb6ab97234f4dac11d28ec79d54085ca 100644 (file)
--- 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 ddda27c184f709f1cef4d6f69374c502ab164ce9..525b021ec04befaf2fd549ad3f29d5eaaf0228e0 100644 (file)
--- 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)
        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);
+       struct worktree **worktrees = get_worktrees();
+       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)) {
                rp_error("refusing to create funny ref '%s' remotely", name);
-               return "funny refname";
+               ret = "funny refname";
+               goto out;
        }
 
        strbuf_addf(&namespaced_name_buf, "%s%s", get_git_namespace(), name);
@@ -1510,7 +1515,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)
                        rp_error("refusing to update checked out branch: %s", name);
                        if (deny_current_branch == DENY_UNCONFIGURED)
                                refuse_unconfigured_deny();
-                       return "branch is currently checked out";
+                       ret = "branch is currently checked out";
+                       goto out;
                case DENY_UPDATE_INSTEAD:
                        /* pass -- let other checks intervene first */
                        do_update_worktree = 1;
@@ -1521,13 +1527,15 @@ static const char *update(struct command *cmd, struct shallow_info *si)
        if (!is_null_oid(new_oid) && !has_object_file(new_oid)) {
                error("unpack should have generated %s, "
                      "but I can't find it!", oid_to_hex(new_oid));
-               return "bad pack";
+               ret = "bad pack";
+               goto out;
        }
 
        if (!is_null_oid(old_oid) && is_null_oid(new_oid)) {
                if (deny_deletes && starts_with(name, "refs/heads/")) {
                        rp_error("denying ref deletion for %s", name);
-                       return "deletion prohibited";
+                       ret = "deletion prohibited";
+                       goto out;
                }
 
                if (worktree || (head_name && !strcmp(namespaced_name, head_name))) {
@@ -1543,9 +1551,11 @@ static const char *update(struct command *cmd, struct shallow_info *si)
                                if (deny_delete_current == DENY_UNCONFIGURED)
                                        refuse_unconfigured_deny_delete_current();
                                rp_error("refusing to delete the current branch: %s", name);
-                               return "deletion of the current branch prohibited";
+                               ret = "deletion of the current branch prohibited";
+                               goto out;
                        default:
-                               return "Invalid denyDeleteCurrent setting";
+                               ret = "Invalid denyDeleteCurrent setting";
+                               goto out;
                        }
                }
        }
@@ -1563,25 +1573,30 @@ static const char *update(struct command *cmd, struct shallow_info *si)
                    old_object->type != OBJ_COMMIT ||
                    new_object->type != OBJ_COMMIT) {
                        error("bad sha1 objects for %s", name);
-                       return "bad ref";
+                       ret = "bad ref";
+                       goto out;
                }
                old_commit = (struct commit *)old_object;
                new_commit = (struct commit *)new_object;
                if (!in_merge_bases(old_commit, new_commit)) {
                        rp_error("denying non-fast-forward %s"
                                 " (you should pull first)", name);
-                       return "non-fast-forward";
+                       ret = "non-fast-forward";
+                       goto out;
                }
        }
        if (run_update_hook(cmd)) {
                rp_error("hook declined to update %s", name);
-               return "hook declined";
+               ret = "hook declined";
+               goto out;
        }
 
        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;
+                       goto out;
        }
 
        if (is_null_oid(new_oid)) {
@@ -1600,17 +1615,19 @@ static const char *update(struct command *cmd, struct shallow_info *si)
                                           old_oid,
                                           0, "push", &err)) {
                        rp_error("%s", err.buf);
-                       strbuf_release(&err);
-                       return "failed to delete";
+                       ret = "failed to delete";
+               } else {
+                       ret = NULL; /* good */
                }
                strbuf_release(&err);
-               return NULL; /* good */
        }
        else {
                struct strbuf err = STRBUF_INIT;
                if (shallow_update && si->shallow_ref[cmd->index] &&
-                   update_shallow_ref(cmd, si))
-                       return "shallow error";
+                   update_shallow_ref(cmd, si)) {
+                       ret = "shallow error";
+                       goto out;
+               }
 
                if (ref_transaction_update(transaction,
                                           namespaced_name,
@@ -1618,14 +1635,16 @@ static const char *update(struct command *cmd, struct shallow_info *si)
                                           0, "push",
                                           &err)) {
                        rp_error("%s", err.buf);
-                       strbuf_release(&err);
-
-                       return "failed to update ref";
+                       ret = "failed to update ref";
+               } else {
+                       ret = NULL; /* good */
                }
                strbuf_release(&err);
-
-               return NULL; /* good */
        }
+
+out:
+       free_worktrees(worktrees);
+       return ret;
 }
 
 static void run_update_post_hook(struct command *commands)

diff --git a/worktree.c b/worktree.c
index 2c155b1015082e98634e4a5c80563973e2b29cc4..6f598dcfcdf96fa7b3e4f59bb6a028abef89fdf5 100644 (file)
--- a/worktree.c
+++ b/worktree.c
@@ -404,17 +404,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 8b7c408132d1f7c7a81824f635488955a2df3cd6..9e06fcbdf3d53f724ba6746db04db1ffd40e800f 100644 (file)
--- 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);
 
 /*