[v2,2/7] branch: add branch_checked_out() helper

Commit Message

Derrick Stolee June 7, 2022, 8:42 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 7, 2022, 10:09 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(), instead of
enumerating the list of worktrees and freeing for each of the
branches you need to inspect.

There also was another topic

https://lore.kernel.org/git/[e-mail]/

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 takes a worktree, 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.

Derrick Stolee June 8, 2022, 2:14 a.m. UTC | #2

On 6/7/2022 6:09 PM, Junio C Hamano wrote:
> "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(), instead of
> enumerating the list of worktrees and freeing for each of the
> branches you need to inspect.

Sorry, you had mentioned this in v1 and I just forgot to fix this.

> There also was another topic
> 
> https://lore.kernel.org/git/[e-mail]/
> 
> 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 takes a worktree, 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.

Yes, that's a good plan. I'll take a look.

Thanks,
-Stolee

Derrick Stolee June 8, 2022, 2:43 a.m. UTC | #3

On 6/7/2022 10:14 PM, Derrick Stolee wrote:
> On 6/7/2022 6:09 PM, Junio C Hamano wrote:
>>
>> 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 takes a worktree, 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.
> 
> Yes, that's a good plan. I'll take a look.

Here is a fixup for this patch that should work. I'll put it in v3.

diff --git a/branch.c b/branch.c
index 2e6419cdfa5..514212f5619 100644
--- a/branch.c
+++ b/branch.c
@@ -10,6 +10,7 @@
 #include "worktree.h"
 #include "submodule-config.h"
 #include "run-command.h"
+#include "strmap.h"
 
 struct tracking {
 	struct refspec_item spec;
@@ -369,17 +370,44 @@ int validate_branchname(const char *name, struct strbuf *ref)
 	return ref_exists(ref->buf);
 }
 
-int branch_checked_out(const char *refname, char **path)
+static int initialized_checked_out_branches;
+static struct strmap current_checked_out_branches = STRMAP_INIT;
+
+static void prepare_checked_out_branches(void)
 {
-	struct worktree **worktrees = get_worktrees();
-	const struct worktree *wt = find_shared_symref(worktrees, "HEAD", refname);
-	int result = wt && !wt->is_bare;
+	int i = 0;
+	struct worktree **worktrees;
+
+	if (initialized_checked_out_branches)
+		return;
+	initialized_checked_out_branches = 1;
+
+	worktrees = get_worktrees();
+
+	while (worktrees[i]) {
+		struct worktree *wt = worktrees[i];
 
-	if (result && path)
-		*path = xstrdup(wt->path);
+		if (!wt->is_bare && wt->head_ref)
+			strmap_put(&current_checked_out_branches,
+				   wt->head_ref,
+				   wt->path);
+
+		i++;
+	}
 
 	free_worktrees(worktrees);
-	return result;
+}
+
+int branch_checked_out(const char *refname, char **path)
+{
+	const char *path_in_set;
+	prepare_checked_out_branches();
+
+	path_in_set = strmap_get(&current_checked_out_branches, refname);
+	if (path_in_set && path)
+		*path = xstrdup(path_in_set);
+
+	return !!path_in_set;
 }
 
 /*

>> There also was another topic
>>
>> https://lore.kernel.org/git/[e-mail]/
>>
>> that was triggered by find_shared_symref() being relatively
>> heavy-weight, which suggests a more involved refactoring.

The patch there was this:

diff --git a/builtin/fetch.c b/builtin/fetch.c
index e3791f09ed5..eeee5ac8f15 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1440,6 +1440,7 @@ static void check_not_current_branch(struct ref *ref_map,
 	const struct worktree *wt;
 	for (; ref_map; ref_map = ref_map->next)
 		if (ref_map->peer_ref &&
+		    starts_with(ref_map->peer_ref->name, "refs/heads/") &&
 		    (wt = find_shared_symref(worktrees, "HEAD",
 					     ref_map->peer_ref->name)) &&
 		    !wt->is_bare)

And there is another use of find_shared_symref() in the same file, allowing
us to do the following:

diff --git a/builtin/fetch.c b/builtin/fetch.c
index ac29c2b1ae3..3933c482839 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -885,7 +885,7 @@ static int update_local_ref(struct ref *ref,
 			    struct worktree **worktrees)
 {
 	struct commit *current = NULL, *updated;
-	const struct worktree *wt;
+	char *path = NULL;
 	const char *pretty_ref = prettify_refname(ref->name);
 	int fast_forward = 0;
 
@@ -900,17 +900,17 @@ static int update_local_ref(struct ref *ref,
 	}
 
 	if (!update_head_ok &&
-	    (wt = find_shared_symref(worktrees, "HEAD", ref->name)) &&
-	    !wt->is_bare && !is_null_oid(&ref->old_oid)) {
+	    !is_null_oid(&ref->old_oid) &&
+	    branch_checked_out(ref->name, &path)) {
 		/*
 		 * If this is the head, and it's not okay to update
 		 * the head, and the old value of the head isn't empty...
 		 */
 		format_display(display, '!', _("[rejected]"),
-			       wt->is_current ?
-				       _("can't fetch in current branch") :
-				       _("checked out in another worktree"),
+			       path ? _("can't fetch in current branch") :
+				      _("checked out in another worktree"),
 			       remote, pretty_ref, summary_width);
+		free(path);
 		return 1;
 	}
 
@@ -1437,16 +1437,14 @@ static int prune_refs(struct refspec *rs,
 static void check_not_current_branch(struct ref *ref_map,
 				     struct worktree **worktrees)
 {
-	const struct worktree *wt;
+	char *path;
 	for (; ref_map; ref_map = ref_map->next)
 		if (ref_map->peer_ref &&
 		    starts_with(ref_map->peer_ref->name, "refs/heads/") &&
-		    (wt = find_shared_symref(worktrees, "HEAD",
-					     ref_map->peer_ref->name)) &&
-		    !wt->is_bare)
+		    branch_checked_out(ref_map->peer_ref->name, &path))
 			die(_("refusing to fetch into branch '%s' "
 			      "checked out at '%s'"),
-			    ref_map->peer_ref->name, wt->path);
+			    ref_map->peer_ref->name, path);
 }
 
 static int truncate_fetch_head(void)

I can extract these as patches in their own series if we think
that's something we worth fast-tracking.

Thanks,
-Stolee

Junio C Hamano June 8, 2022, 4:52 a.m. UTC | #4

Derrick Stolee <[e-mail]> writes:

>> Yes, that's a good plan. I'll take a look.
>
> Here is a fixup for this patch that should work. I'll put it in v3.
>
> diff --git a/branch.c b/branch.c
> index 2e6419cdfa5..514212f5619 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -10,6 +10,7 @@
>  #include "worktree.h"
>  #include "submodule-config.h"
>  #include "run-command.h"
> +#include "strmap.h"
>  
>  struct tracking {
>  	struct refspec_item spec;
> @@ -369,17 +370,44 @@ int validate_branchname(const char *name, struct strbuf *ref)
>  	return ref_exists(ref->buf);
>  }
>  
> -int branch_checked_out(const char *refname, char **path)
> +static int initialized_checked_out_branches;
> +static struct strmap current_checked_out_branches = STRMAP_INIT;
> +
> +static void prepare_checked_out_branches(void)
>  {
> +	int i = 0;
> +	struct worktree **worktrees;
> +
> +	if (initialized_checked_out_branches)
> +		return;
> +	initialized_checked_out_branches = 1;
> +
> +	worktrees = get_worktrees();
> +
> +	while (worktrees[i]) {
> +		struct worktree *wt = worktrees[i];
>  
> +		if (!wt->is_bare && wt->head_ref)
> +			strmap_put(&current_checked_out_branches,
> +				   wt->head_ref,
> +				   wt->path);
> +
> +		i++;
> +	}
>  	free_worktrees(worktrees);
> -	return result;
> +}

Yeah, the above illustrates what I had in mind pretty closely.  The
above outline only checks where the HEAD symref points at, and it
does not protect a branch that is in the middle of getting bisected
or rebased, which find_shared_symref() tries to do, IIRC, though.

It should not be too hard to add that to the above to allow us to
achieve feature parity with the original.

> +
> +int branch_checked_out(const char *refname, char **path)
> +{
> +	const char *path_in_set;
> +	prepare_checked_out_branches();
> +
> +	path_in_set = strmap_get(&current_checked_out_branches, refname);
> +	if (path_in_set && path)
> +		*path = xstrdup(path_in_set);
> +
> +	return !!path_in_set;
>  }

This one looks quite straight-forward.

> And there is another use of find_shared_symref() in the same file, allowing
> us to do the following:
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index ac29c2b1ae3..3933c482839 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -885,7 +885,7 @@ static int update_local_ref(struct ref *ref,
>  			    struct worktree **worktrees)
>  {
>  	struct commit *current = NULL, *updated;
> -	const struct worktree *wt;
> +	char *path = NULL;
>  	const char *pretty_ref = prettify_refname(ref->name);
>  	int fast_forward = 0;
>  
> @@ -900,17 +900,17 @@ static int update_local_ref(struct ref *ref,
>  	}
>  
>  	if (!update_head_ok &&
> -	    (wt = find_shared_symref(worktrees, "HEAD", ref->name)) &&
> -	    !wt->is_bare && !is_null_oid(&ref->old_oid)) {
> +	    !is_null_oid(&ref->old_oid) &&
> +	    branch_checked_out(ref->name, &path)) {

Yes.  It looks like a good direction to go in.

Thanks.

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.