On 22-feb-2023 23:52:51, Rubén Justo wrote:
> "reject_rebase_or_bisect_branch()" was introduced [1] to prevent a
> branch under bisect or rebase from being renamed or copied. It
> traverses all worktrees in the repository and dies if the specified
> branch is being rebased or bisected in any of them.
>
> "replace_each_worktree_head_symref()" was introduced [2] to adjust the
> HEAD in all worktrees after a branch rename succeeded. It traverses all
> worktrees in the repository and if any of them have their HEAD pointing
> to the renamed ref, it adjusts it.
>
> Considering that both functions are only called from within
> copy_or_rename_branch() and each of them traverses all worktrees, which
> might involve disk access and resolving multiple references, inlining
> these two functions to do the traversing once, makes sense.
>
> 1.- 14ace5b (branch: do not rename a branch under bisect or rebase,
> 2016-04-22)
>
> 2.- 70999e9 (branch -m: update all per-worktree HEADs, 2016-03-27)
>
> Signed-off-by: Rubén Justo <rjusto@xxxxxxxxx>
> ---
> branch.c | 27 --------------------
> branch.h | 8 ------
> builtin/branch.c | 64 ++++++++++++++++++++++++++++--------------------
> 3 files changed, 38 insertions(+), 61 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index e5614b53b3..f64062be71 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -830,30 +830,3 @@ void die_if_checked_out(const char *branch, int ignore_current_worktree)
>
> free_worktrees(worktrees);
> }
> -
> -int replace_each_worktree_head_symref(const char *oldref, const char *newref,
> - const char *logmsg)
> -{
> - int ret = 0;
> - struct worktree **worktrees = get_worktrees();
> - int i;
> -
> - for (i = 0; worktrees[i]; i++) {
> - struct ref_store *refs;
> -
> - if (worktrees[i]->is_detached)
> - continue;
> - if (!worktrees[i]->head_ref)
> - continue;
> - if (strcmp(oldref, worktrees[i]->head_ref))
> - continue;
> -
> - refs = get_worktree_ref_store(worktrees[i]);
> - if (refs_create_symref(refs, "HEAD", newref, logmsg))
> - ret = error(_("HEAD of working tree %s is not updated"),
> - worktrees[i]->path);
> - }
> -
> - free_worktrees(worktrees);
> - return ret;
> -}
> diff --git a/branch.h b/branch.h
> index ef56103c05..30c01aed73 100644
> --- a/branch.h
> +++ b/branch.h
> @@ -155,12 +155,4 @@ int read_branch_desc(struct strbuf *, const char *branch_name);
> */
> void die_if_checked_out(const char *branch, int ignore_current_worktree);
>
> -/*
> - * Update all per-worktree HEADs pointing at the old ref to point the new ref.
> - * This will be used when renaming a branch. Returns 0 if successful, non-zero
> - * otherwise.
> - */
> -int replace_each_worktree_head_symref(const char *oldref, const char *newref,
> - const char *logmsg);
> -
> #endif
> diff --git a/builtin/branch.c b/builtin/branch.c
> index f63fd45edb..a32ae64006 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -486,28 +486,6 @@ static void print_current_branch_name(void)
> die(_("HEAD (%s) points outside of refs/heads/"), refname);
> }
>
> -static void reject_rebase_or_bisect_branch(const char *target)
> -{
> - struct worktree **worktrees = get_worktrees();
> - int i;
> -
> - for (i = 0; worktrees[i]; i++) {
> - struct worktree *wt = worktrees[i];
> -
> - if (!wt->is_detached)
> - continue;
> -
> - if (is_worktree_being_rebased(wt, target))
> - die(_("Branch %s is being rebased at %s"),
> - target, wt->path);
> -
> - if (is_worktree_being_bisected(wt, target))
> - die(_("Branch %s is being bisected at %s"),
> - target, wt->path);
> - }
> -
> - free_worktrees(worktrees);
> -}
>
> static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)
> {
> @@ -516,6 +494,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
> const char *interpreted_oldname = NULL;
> const char *interpreted_newname = NULL;
> int recovery = 0;
> + struct worktree **worktrees = get_worktrees();
>
> if (strbuf_check_branch_ref(&oldref, oldname)) {
> /*
> @@ -544,7 +523,20 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
> else
> validate_new_branchname(newname, &newref, force);
>
> - reject_rebase_or_bisect_branch(oldref.buf);
> + for (int i = 0; worktrees[i]; i++) {
> + struct worktree *wt = worktrees[i];
> +
> + if (!wt->is_detached)
> + continue;
> +
> + if (is_worktree_being_rebased(wt, oldref.buf))
> + die(_("Branch %s is being rebased at %s"),
> + oldref.buf, wt->path);
> +
> + if (is_worktree_being_bisected(wt, oldref.buf))
> + die(_("Branch %s is being bisected at %s"),
> + oldref.buf, wt->path);
> + }
>
> if (!skip_prefix(oldref.buf, "refs/heads/", &interpreted_oldname) ||
> !skip_prefix(newref.buf, "refs/heads/", &interpreted_newname)) {
> @@ -574,9 +566,29 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
> interpreted_oldname);
> }
>
> - if (!copy &&
> - replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))
> - die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
> + if (!copy) {
> + /*
> + * Update all per-worktree HEADs pointing at the old ref to
> + * point the new ref.
> + */
> + for (int i = 0; worktrees[i]; i++) {
> + struct ref_store *refs;
> +
> + if (worktrees[i]->is_detached)
> + continue;
> + if (!worktrees[i]->head_ref)
> + continue;
> + if (strcmp(oldref.buf, worktrees[i]->head_ref))
> + continue;
> +
> + refs = get_worktree_ref_store(worktrees[i]);
> + if (refs_create_symref(refs, "HEAD", newref.buf, logmsg.buf))
> + die(_("Branch renamed to %s, but HEAD is not updated!"),
> + newname);
Reviewing this, I noticed I made a mistake here. The original code
doesn't stop iterating whenever refs_create_symref() fails; it continues
trying to update the remaining worktrees. When the iteration ends, if
any of the updates failed, then die(). Also, the error message "HEAD of
working tree %s is not updated" is lost. I'll reroll with this errors
fixed but maybe some review is already underway, so I'll wait some
time. Sorry for the inconvenience.
