git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Rubén Justo <rjusto@gmail.com>
Date
Feb 25, 2023, 15:08 UTC
Message-ID
<4e6aeffe-98ef-b4d7-7c8f-782e0a617653@gmail.com>
In-Reply-To
<50dd7c7a-5656-e010-1c0b-819a40a1f1a0@gmail.com>
On 22-feb-2023 23:52:51, Rubén Justo wrote:
Show 168 quoted lines
> "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.

Previous: Rubén JustoNext: Jonathan Tan
Message 32 of 49 in “branch: operations on orphan branches”
  1. 0/2 branch: operations on orphan branchesRubén Justo, Dec 30, 2022
  2. 1/2 branch: description for orphan branch errorsRubén Justo, Dec 30, 2022
  3. Junio C HamanoJan 1, 2023
  4. Rubén JustoJan 3, 2023
  5. Junio C HamanoJan 4, 2023
  6. Rubén JustoJan 6, 2023
  7. Junio C HamanoJan 6, 2023
  8. Rubén JustoJan 7, 2023
  9. Junio C HamanoJan 7, 2023
  10. 2/2 branch: rename orphan branches in any worktreeRubén Justo, Dec 30, 2022
  11. 0/3 branch: operations on orphan branchesRubén Justo, Jan 15, 2023
  12. 1/3 avoid unnecessary worktrees traversingRubén Justo, Jan 16, 2023
  13. Junio C HamanoJan 19, 2023
  14. Rubén JustoJan 19, 2023
  15. 2/3 branch: description for orphan branch errorsRubén Justo, Jan 16, 2023
  16. 3/3 branch: rename orphan branches in any worktreeRubén Justo, Jan 16, 2023
  17. Junio C HamanoJan 19, 2023
  18. Rubén JustoJan 19, 2023
  19. 3/3 branch: rename orphan branches in any worktreeRubén Justo, Jan 16, 2023
  20. 0/3 branch: operations on orphan branchesRubén Justo, Feb 6, 2023
  21. 1/3 branch: avoid unnecessary worktrees traversalsRubén Justo, Feb 6, 2023
  22. Jonathan TanFeb 11, 2023
  23. Rubén JustoFeb 15, 2023
  24. 2/3 branch: description for orphan branch errorsRubén Justo, Feb 6, 2023
  25. 3/3 branch: rename orphan branches in any worktreeRubén Justo, Feb 6, 2023
  26. Junio C HamanoFeb 7, 2023
  27. Ævar Arnfjörð BjarmasonFeb 7, 2023
  28. Rubén JustoFeb 8, 2023
  29. Junio C HamanoFeb 8, 2023
  30. 0/3 branch: operations on orphan branchesRubén Justo, Feb 22, 2023
  31. 1/3 branch: avoid unnecessary worktrees traversalsRubén Justo, Feb 22, 2023
  32. Rubén JustoFeb 25, 2023
  33. Jonathan TanFeb 27, 2023
  34. Rubén JustoFeb 28, 2023
  35. 2/3 branch: description for orphan branch errorsRubén Justo, Feb 22, 2023
  36. Jonathan TanFeb 27, 2023
  37. Junio C HamanoFeb 27, 2023
  38. Rubén JustoFeb 28, 2023
  39. 3/3 branch: rename orphan branches in any worktreeRubén Justo, Feb 22, 2023
  40. Jonathan TanFeb 27, 2023
  41. Rubén JustoFeb 28, 2023
  42. 0/5 branch: operations on orphan branchesRubén Justo, Mar 26, 2023
  43. 1/5 branch: test for failures while renaming branchesRubén Justo, Mar 26, 2023
  44. 2/5 branch: use get_worktrees() in copy_or_rename_branch()Rubén Justo, Mar 26, 2023
  45. 3/5 branch: description for orphan branch errorsRubén Justo, Mar 26, 2023
  46. 4/5 branch: rename orphan branches in any worktreeRubén Justo, Mar 26, 2023
  47. 5/5 branch: avoid unnecessary worktrees traversalsRubén Justo, Mar 26, 2023
  48. Junio C HamanoMar 27, 2023
  49. Junio C HamanoMay 1, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.