Re: [PATCH v4 1/3] branch: avoid unnecessary worktrees traversals

["Rubén Justo" <rjusto@gmail.com>]

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@gmail.com>
> ---
>  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.