[2/4] branch: add branch_checked_out() helper

Commit Message

Derrick Stolee June 3, 2022, 1:37 p.m. UTC

From: Derrick Stolee <[e-mail]>

The validate_new_branchname() method contains a check to see if a branch
is checked out in any non-bare worktree. This is intended to prevent a
force push that will mess up an existing checkout. This helper is not
suitable to performing just that check, because the method will die()
when the branch is checked out instead of returning an error code.

Extract branch_checked_out() and use it within
validate_new_branchname(). Another caller will be added in a coming
change.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 branch.c | 24 ++++++++++++++++--------
 branch.h |  8 ++++++++
 2 files changed, 24 insertions(+), 8 deletions(-)

Comments

Junio C Hamano June 3, 2022, 6:31 p.m. UTC | #1

"Derrick Stolee via GitGitGadget" <[e-mail]> writes:

> +int branch_checked_out(const char *refname, char **path)
> +{
> +	struct worktree **worktrees = get_worktrees();
> +	const struct worktree *wt = find_shared_symref(worktrees, "HEAD", refname);
> +	int result = wt && !wt->is_bare;
> +
> +	if (result && path)
> +		*path = xstrdup(wt->path);
> +
> +	free_worktrees(worktrees);
> +	return result;
> +}

Don't you plan to call this repeatedly from the for_each_deco
iteration?  I am wondering if it should take the result of
get_worktrees() and reuse the result of get_worktrees().

There also was another topic that was triggered by
find_shared_symref() being relatively heavy-weight, which suggests a
more involved refactoring.

I wonder if we rather want to rewrite find_shared_symref() *not* to
take the target parameter at all, and instead introduce a new
function that iterates over worktrees and report the branch that is
checked out (or being operated on via rebase or bisect).  Then we
can

 - create a strset out of its result, i.e. set of branches that
   should not be touched;

 - iterate over refs that point into the history being rebased
   (using for_each_decoration()), and consult that strset to see if
   any of them is being rewritten.  

With the API of find_shared_symref(), we'd need to iterate over all
worktrees for each decoration.  With such a restructuring, we can
iterate over all worktrees just once, and match the result with
decoration, so the problem becomes O(N)+O(M) and not O(N*M) for
number of worktrees N and number of decorations M.

Patch

diff --git a/branch.c b/branch.c
index 2d6569b0c62..2e6419cdfa5 100644
--- a/branch.c
+++ b/branch.c
@@ -369,6 +369,19 @@  int validate_branchname(const char *name, struct strbuf *ref)
 	return ref_exists(ref->buf);
 }
 
+int branch_checked_out(const char *refname, char **path)
+{
+	struct worktree **worktrees = get_worktrees();
+	const struct worktree *wt = find_shared_symref(worktrees, "HEAD", refname);
+	int result = wt && !wt->is_bare;
+
+	if (result && path)
+		*path = xstrdup(wt->path);
+
+	free_worktrees(worktrees);
+	return result;
+}
+
 /*
  * Check if a branch 'name' can be created as a new branch; die otherwise.
  * 'force' can be used when it is OK for the named branch already exists.
@@ -377,9 +390,7 @@  int validate_branchname(const char *name, struct strbuf *ref)
  */
 int validate_new_branchname(const char *name, struct strbuf *ref, int force)
 {
-	struct worktree **worktrees;
-	const struct worktree *wt;
-
+	char *path;
 	if (!validate_branchname(name, ref))
 		return 0;
 
@@ -387,13 +398,10 @@  int validate_new_branchname(const char *name, struct strbuf *ref, int force)
 		die(_("a branch named '%s' already exists"),
 		    ref->buf + strlen("refs/heads/"));
 
-	worktrees = get_worktrees();
-	wt = find_shared_symref(worktrees, "HEAD", ref->buf);
-	if (wt && !wt->is_bare)
+	if (branch_checked_out(ref->buf, &path))
 		die(_("cannot force update the branch '%s' "
 		      "checked out at '%s'"),
-		    ref->buf + strlen("refs/heads/"), wt->path);
-	free_worktrees(worktrees);
+		    ref->buf + strlen("refs/heads/"), path);
 
 	return 1;
 }
diff --git a/branch.h b/branch.h
index 560b6b96a8f..5ea93d217b1 100644
--- a/branch.h
+++ b/branch.h
@@ -101,6 +101,14 @@  void create_branches_recursively(struct repository *r, const char *name,
 				 const char *tracking_name, int force,
 				 int reflog, int quiet, enum branch_track track,
 				 int dry_run);
+
+/*
+ * Returns true if the branch at 'refname' is checked out at any
+ * non-bare worktree. The path of the worktree is stored in the
+ * given 'path', if provided.
+ */
+int branch_checked_out(const char *refname, char **path);
+
 /*
  * Check if 'name' can be a valid name for a branch; die otherwise.
  * Return 1 if the named branch already exists; return 0 otherwise.