[v3,1/3] branch: avoid unnecessary worktrees traversals

Commit Message

Rubén Justo Feb. 6, 2023, 11:06 p.m. UTC

"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.

If we could know in advance if the renamed branch is not HEAD in any
worktree we could avoid calling "replace_each_worktree_head_symref()",
and so avoid that unnecessary second traversing.

Let's rename "reject_rebase_or_bisect_branch()" to a more meaningful
name "die_if_branch_is_being_rebased_or_bisected()" and make it return,
if it does not die(), if the specified branch is HEAD in any worktree.
Use this new information to avoid unnecessary calls to
"replace_each_worktree_head_symref()".

  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 <[e-mail]>
---
 builtin/branch.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

Comments

Jonathan Tan Feb. 11, 2023, 4:16 a.m. UTC | #1

Rubén Justo <[e-mail]> writes:
> "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.

Thanks for the references to why these were introduced!

> If we could know in advance if the renamed branch is not HEAD in any
> worktree we could avoid calling "replace_each_worktree_head_symref()",
> and so avoid that unnecessary second traversing.

When I first read this paragraph, I thought that the traversing involved
was just a loop through an in-memory data structure, which is not that
costly. It turns out that a travesal involves not only constructing
said data structure but also reading from disk to get the necessary
information, which indeed is very costly. I would include that in the
commit message, but won't insist on that (perhaps it's clear to others
what is meant by traversal).

> Let's rename "reject_rebase_or_bisect_branch()" to a more meaningful
> name "die_if_branch_is_being_rebased_or_bisected()" and make it return,
> if it does not die(), if the specified branch is HEAD in any worktree.
> Use this new information to avoid unnecessary calls to
> "replace_each_worktree_head_symref()".

In later patches, I see that the return value can also indicate that a
branch is an orphan, and that for the sake of code clarity, the calling
function had to have a variable assignment of the form oldref_is_orphan
= (oldref_is_head > 1). If this is so, it is probably better to have
this function return something with names. So something like

  #define IS_HEAD 4
  #define IS_ORPHAN 8
  int get_branch_usage_in_worktrees(...) {...}

and then the caller can use these constants whenever it needs to know
what kind of branch this is.

I also see in patch 2 that we're changing what the user sees under
certain inputs. That can be avoided if we move the dying to the caller,
and have this function merely return when the branch is being rebased
or bisected.

  #define IS_BISECTED 1
  #define IS_REBASED 2

or something like that. I would prefer if user-visible behavior didn't
change unnecessarily, and this does not seem like a necessary case.

Other than that, everything looks good.

Rubén Justo Feb. 15, 2023, 10 p.m. UTC | #2

On 10-feb-2023 20:16:44, Jonathan Tan wrote:

> > If we could know in advance if the renamed branch is not HEAD in any
> > worktree we could avoid calling "replace_each_worktree_head_symref()",
> > and so avoid that unnecessary second traversing.
> 
> When I first read this paragraph, I thought that the traversing involved
> was just a loop through an in-memory data structure, which is not that
> costly. It turns out that a travesal involves not only constructing
> said data structure but also reading from disk to get the necessary
> information, which indeed is very costly. I would include that in the
> commit message, but won't insist on that (perhaps it's clear to others
> what is meant by traversal).

Sorry, I should have included details about why it's costly.  I'll
include some in the message.

> 
> > Let's rename "reject_rebase_or_bisect_branch()" to a more meaningful
> > name "die_if_branch_is_being_rebased_or_bisected()" and make it return,
> > if it does not die(), if the specified branch is HEAD in any worktree.
> > Use this new information to avoid unnecessary calls to
> > "replace_each_worktree_head_symref()".
> 
> In later patches, I see that the return value can also indicate that a
> branch is an orphan, and that for the sake of code clarity, the calling
> function had to have a variable assignment of the form oldref_is_orphan
> = (oldref_is_head > 1). If this is so, it is probably better to have
> this function return something with names. So something like
> 
>   #define IS_HEAD 4
>   #define IS_ORPHAN 8

OK.  I'll use names.

>   int get_branch_usage_in_worktrees(...) {...}
> 
> and then the caller can use these constants whenever it needs to know
> what kind of branch this is.
> 
> I also see in patch 2 that we're changing what the user sees under
> certain inputs. That can be avoided if we move the dying to the caller,
> and have this function merely return when the branch is being rebased
> or bisected.
> 
>   #define IS_BISECTED 1
>   #define IS_REBASED 2
> 
> or something like that. I would prefer if user-visible behavior didn't
> change unnecessarily, and this does not seem like a necessary case.

OK.

> 
> Other than that, everything looks good.

Thanks for your review and suggestions!

Patch

diff --git a/builtin/branch.c b/builtin/branch.c
index f63fd45edb..89198fa5bf 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -486,14 +486,21 @@  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)
+/*
+ * Dies if the specified branch is being rebased or bisected.  Otherwise returns
+ * 0 or, if the branch is HEAD in any worktree, returns 1.
+ */
+static int die_if_branch_is_being_rebased_or_bisected(const char *target)
 {
 	struct worktree **worktrees = get_worktrees();
-	int i;
+	int i, ret = 0;
 
 	for (i = 0; worktrees[i]; i++) {
 		struct worktree *wt = worktrees[i];
 
+		if (wt->head_ref && !strcmp(target, wt->head_ref))
+			ret = 1;
+
 		if (!wt->is_detached)
 			continue;
 
@@ -507,6 +514,7 @@  static void reject_rebase_or_bisect_branch(const char *target)
 	}
 
 	free_worktrees(worktrees);
+	return ret;
 }
 
 static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)
@@ -515,7 +523,7 @@  static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
 	const char *interpreted_oldname = NULL;
 	const char *interpreted_newname = NULL;
-	int recovery = 0;
+	int recovery = 0, oldref_is_head;
 
 	if (strbuf_check_branch_ref(&oldref, oldname)) {
 		/*
@@ -544,7 +552,7 @@  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);
+	oldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);
 
 	if (!skip_prefix(oldref.buf, "refs/heads/", &interpreted_oldname) ||
 	    !skip_prefix(newref.buf, "refs/heads/", &interpreted_newname)) {
@@ -574,7 +582,7 @@  static void copy_or_rename_branch(const char *oldname, const char *newname, int
 				interpreted_oldname);
 	}
 
-	if (!copy &&
+	if (!copy && oldref_is_head &&
 	    replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))
 		die(_("Branch renamed to %s, but HEAD is not updated!"), newname);