[PATCH] branch: report kind of checkout when rejecting delete

[PATCH] branch: report kind of checkout when rejecting delete

[René Scharfe]

git branch refuses to delete branches that are currently checked out
with a message like this: "error: cannot delete branch 'foo' used by
worktree at '/path/of/worktree'".  This can be confusing with internal
checkouts, e.g. if one tries to delete a branch associated with an
active bisect run.

Mention the kind of internal checkout, if any, to spare the user from
remembering that they might have forgotten a bisect or rebase.  To do
that, register the checkout reason in a strintmap alongside the existing
strmap that stores the worktree path.

Suggested-by: stsp <stsp2@yandex.ru>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Original message:
https://lore.kernel.org/git/cae34516-5437-49d3-8d39-16f4059a81a8@yandex.ru/

 branch.c          | 48 ++++++++++++++++++++++++++---------------------
 branch.h          | 15 +++++++++++++++
 builtin/branch.c  | 31 +++++++++++++++++++++++++++---
 t/t3200-branch.sh |  4 ++--
 4 files changed, 72 insertions(+), 26 deletions(-)

diff --git a/branch.c b/branch.c
index 243db7d0fc..aaa54f1b62 100644
--- a/branch.c
+++ b/branch.c
@@ -384,6 +384,16 @@ int validate_branchname(const char *name, struct strbuf *ref)
 
 static int initialized_checked_out_branches;
 static struct strmap current_checked_out_branches = STRMAP_INIT;
+static struct strintmap current_checked_out_branch_kinds = STRINTMAP_INIT;
+
+static void register_checked_out_branch(const char *refname, const char *path,
+					enum branch_checkout_kind kind)
+{
+	char *old = strmap_put(&current_checked_out_branches, refname,
+			       xstrdup(path));
+	free(old);
+	strintmap_set(&current_checked_out_branch_kinds, refname, kind);
+}
 
 static void prepare_checked_out_branches(void)
 {
@@ -397,7 +407,7 @@ static void prepare_checked_out_branches(void)
 	worktrees = get_worktrees();
 
 	while (worktrees[i]) {
-		char *old, *wt_gitdir;
+		char *wt_gitdir;
 		struct wt_status_state state = { 0 };
 		struct worktree *wt = worktrees[i++];
 		struct string_list update_refs = STRING_LIST_INIT_DUP;
@@ -405,22 +415,17 @@ static void prepare_checked_out_branches(void)
 		if (wt->is_bare)
 			continue;
 
-		if (wt->head_ref) {
-			old = strmap_put(&current_checked_out_branches,
-					 wt->head_ref,
-					 xstrdup(wt->path));
-			free(old);
-		}
+		if (wt->head_ref)
+			register_checked_out_branch(wt->head_ref, wt->path,
+				BRANCH_CHECKOUT_KIND_CHECKOUT);
 
 		if (wt_status_check_rebase(wt, &state) &&
 		    (state.rebase_in_progress || state.rebase_interactive_in_progress) &&
 		    state.branch) {
 			struct strbuf ref = STRBUF_INIT;
 			strbuf_addf(&ref, "refs/heads/%s", state.branch);
-			old = strmap_put(&current_checked_out_branches,
-					 ref.buf,
-					 xstrdup(wt->path));
-			free(old);
+			register_checked_out_branch(ref.buf, wt->path,
+				BRANCH_CHECKOUT_KIND_REBASE);
 			strbuf_release(&ref);
 		}
 		wt_status_state_free_buffers(&state);
@@ -429,10 +434,8 @@ static void prepare_checked_out_branches(void)
 		    state.bisecting_from) {
 			struct strbuf ref = STRBUF_INIT;
 			strbuf_addf(&ref, "refs/heads/%s", state.bisecting_from);
-			old = strmap_put(&current_checked_out_branches,
-					 ref.buf,
-					 xstrdup(wt->path));
-			free(old);
+			register_checked_out_branch(ref.buf, wt->path,
+				BRANCH_CHECKOUT_KIND_BISECT);
 			strbuf_release(&ref);
 		}
 		wt_status_state_free_buffers(&state);
@@ -441,12 +444,9 @@ static void prepare_checked_out_branches(void)
 		if (!sequencer_get_update_refs_state(wt_gitdir,
 						     &update_refs)) {
 			struct string_list_item *item;
-			for_each_string_list_item(item, &update_refs) {
-				old = strmap_put(&current_checked_out_branches,
-						 item->string,
-						 xstrdup(wt->path));
-				free(old);
-			}
+			for_each_string_list_item(item, &update_refs)
+				register_checked_out_branch(item->string, wt->path,
+					BRANCH_CHECKOUT_KIND_UPDATE_REF);
 			string_list_clear(&update_refs, 1);
 		}
 
@@ -462,6 +462,12 @@ const char *branch_checked_out(const char *refname)
 	return strmap_get(&current_checked_out_branches, refname);
 }
 
+enum branch_checkout_kind branch_checkout_kind(const char *refname)
+{
+	prepare_checked_out_branches();
+	return strintmap_get(&current_checked_out_branch_kinds, refname);
+}
+
 /*
  * 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.
diff --git a/branch.h b/branch.h
index 3dc6e2a0ff..d1073fe1cd 100644
--- a/branch.h
+++ b/branch.h
@@ -15,6 +15,14 @@ enum branch_track {
 	BRANCH_TRACK_SIMPLE,
 };
 
+enum branch_checkout_kind {
+	BRANCH_CHECKOUT_KIND_UNSPECIFIED = 0,
+	BRANCH_CHECKOUT_KIND_CHECKOUT,
+	BRANCH_CHECKOUT_KIND_REBASE,
+	BRANCH_CHECKOUT_KIND_BISECT,
+	BRANCH_CHECKOUT_KIND_UPDATE_REF,
+};
+
 /* Functions for acting on the information about branches. */
 
 /**
@@ -106,6 +114,13 @@ void create_branches_recursively(struct repository *r, const char *name,
  */
 const char *branch_checked_out(const char *refname);
 
+/*
+ * If the branch at 'refname' is currently checked out in a worktree,
+ * then return the kind of checkout, i.e. whether it was done by an
+ * actual checkout or a rebase etc.
+ */
+enum branch_checkout_kind branch_checkout_kind(const char *refname);
+
 /*
  * Check if 'name' can be a valid name for a branch; die otherwise.
  * Return 1 if the named branch already exists; return 0 otherwise.
diff --git a/builtin/branch.c b/builtin/branch.c
index dede60d27b..3223347129 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -266,9 +266,34 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		if (kinds == FILTER_REFS_BRANCHES) {
 			const char *path;
 			if ((path = branch_checked_out(name))) {
-				error(_("cannot delete branch '%s' "
-					"used by worktree at '%s'"),
-				      bname.buf, path);
+				int kind = branch_checkout_kind(name);
+				switch (kind) {
+				case BRANCH_CHECKOUT_KIND_CHECKOUT:
+					error(_("cannot delete branch '%s' "
+						"used by worktree at '%s'"),
+					      bname.buf, path);
+					break;
+				case BRANCH_CHECKOUT_KIND_REBASE:
+					error(_("cannot delete branch '%s' "
+						"used by worktree at '%s' "
+						"for rebase"),
+					      bname.buf, path);
+					break;
+				case BRANCH_CHECKOUT_KIND_BISECT:
+					error(_("cannot delete branch '%s' "
+						"used by worktree at '%s' "
+						"for bisect"),
+					      bname.buf, path);
+					break;
+				case BRANCH_CHECKOUT_KIND_UPDATE_REF:
+					error(_("cannot delete branch '%s' "
+						"used by worktree at '%s' "
+						"for update-ref"),
+					      bname.buf, path);
+					break;
+				default:
+					BUG("invalid checkout kind %d", kind);
+				}
 				ret = 1;
 				continue;
 			}
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index e2682a83a0..e5df493b66 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -930,7 +930,7 @@ test_expect_success 'deleting currently checked out branch fails' '
 	git worktree add -b my7 my7 &&
 	test_must_fail git -C my7 branch -d my7 &&
 	test_must_fail git branch -d my7 2>actual &&
-	grep "^error: cannot delete branch .my7. used by worktree at " actual &&
+	test_grep "^error: cannot delete branch '"'"'my7'"'"' used by worktree at '"'.*'\$"'" actual &&
 	rm -r my7 &&
 	git worktree prune
 '
@@ -941,7 +941,7 @@ test_expect_success 'deleting in-use branch fails' '
 	git -C my7 bisect start HEAD HEAD~2 &&
 	test_must_fail git -C my7 branch -d my7 &&
 	test_must_fail git branch -d my7 2>actual &&
-	grep "^error: cannot delete branch .my7. used by worktree at " actual &&
+	test_grep "^error: cannot delete branch '"'"'my7'"'"' used by worktree at '"'.*' for bisect\$"'" actual &&
 	rm -r my7 &&
 	git worktree prune
 '
-- 
2.55.0

Re: [PATCH] branch: report kind of checkout when rejecting delete

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> git branch refuses to delete branches that are currently checked out
> with a message like this: "error: cannot delete branch 'foo' used by
> worktree at '/path/of/worktree'".  This can be confusing with internal
> checkouts, e.g. if one tries to delete a branch associated with an
> active bisect run.
>
> Mention the kind of internal checkout, if any, to spare the user from
> remembering that they might have forgotten a bisect or rebase.  To do
> that, register the checkout reason in a strintmap alongside the existing
> strmap that stores the worktree path.
>
> Suggested-by: stsp <stsp2@yandex.ru>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Original message:
> https://lore.kernel.org/git/cae34516-5437-49d3-8d39-16f4059a81a8@yandex.ru/

This reminds me of another recent discussion on rewriting a branch
that is checked out elsewhere, where the "git history" command
forgot to apply the same safety check:

https://lore.kernel.org/git/e7dbcede-4486-459c-aa64-e44690e01fe0@gmail.com/

We definitely need an easy-to-use API to determine consistently
which branches are in use, and to teach all commands that repoint
branch tips to use it to offer the same safety to users.  The
framework that this patch introduces might be a good starting point
for that effort.

> diff --git a/branch.h b/branch.h
> index 3dc6e2a0ff..d1073fe1cd 100644
> --- a/branch.h
> +++ b/branch.h
> @@ -15,6 +15,14 @@ enum branch_track {
>  	BRANCH_TRACK_SIMPLE,
>  };
>  
> +enum branch_checkout_kind {
> +	BRANCH_CHECKOUT_KIND_UNSPECIFIED = 0,
> +	BRANCH_CHECKOUT_KIND_CHECKOUT,
> +	BRANCH_CHECKOUT_KIND_REBASE,
> +	BRANCH_CHECKOUT_KIND_BISECT,
> +	BRANCH_CHECKOUT_KIND_UPDATE_REF,
> +};
> +
> ...
> +/*
> + * If the branch at 'refname' is currently checked out in a worktree,
> + * then return the kind of checkout, i.e. whether it was done by an
> + * actual checkout or a rebase etc.
> + */
> +enum branch_checkout_kind branch_checkout_kind(const char *refname);

OK.

> diff --git a/builtin/branch.c b/builtin/branch.c
> index dede60d27b..3223347129 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -266,9 +266,34 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>  		if (kinds == FILTER_REFS_BRANCHES) {
>  			const char *path;
>  			if ((path = branch_checked_out(name))) {
> -				error(_("cannot delete branch '%s' "
> -					"used by worktree at '%s'"),
> -				      bname.buf, path);
> +				int kind = branch_checkout_kind(name);

Not "enum branch_checkout_kind" but "int"?

> +				switch (kind) {
> +				case BRANCH_CHECKOUT_KIND_CHECKOUT:
> +					error(_("cannot delete branch '%s' "
> +						"used by worktree at '%s'"),
> +					      bname.buf, path);
> +					break;

We may want to be more explicit and say "cannot delete
branch 'frotz' checked out in worktree at '/tmp/nitfol'"
instead.  Unless this is a catch-all entry for states that
are neither 'rebase', 'bisect', nor 'rebase-merges' but are
somehow otherwise in use, that is.

> +				case BRANCH_CHECKOUT_KIND_UPDATE_REF:
> +					error(_("cannot delete branch '%s' "
> +						"used by worktree at '%s' "
> +						"for update-ref"),
> +					      bname.buf, path);
> +					break;

I was quite lost when searching for cases where this 'update-ref'
state might be encountered, and I still lack confidence.  Can
we make the diagnostic message a bit friendlier to our users?

For instance, something like: 'You are rebasing a history with
merges in that other worktree, and the tip of this branch will
be updated when that process completes, so you cannot delete
it from here.'  (Naturally, I may have misidentified the exact
nature of the error, but this illustrates the level of detail and
user-facing clarity I hope to see.)

> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index e2682a83a0..e5df493b66 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -930,7 +930,7 @@ test_expect_success 'deleting currently checked out branch fails' '
>  	git worktree add -b my7 my7 &&
>  	test_must_fail git -C my7 branch -d my7 &&
>  	test_must_fail git branch -d my7 2>actual &&
> -	grep "^error: cannot delete branch .my7. used by worktree at " actual &&
> +	test_grep "^error: cannot delete branch '"'"'my7'"'"' used by worktree at '"'.*'\$"'" actual &&
>  	rm -r my7 &&
>  	git worktree prune
>  '
> @@ -941,7 +941,7 @@ test_expect_success 'deleting in-use branch fails' '
>  	git -C my7 bisect start HEAD HEAD~2 &&
>  	test_must_fail git -C my7 branch -d my7 &&
>  	test_must_fail git branch -d my7 2>actual &&
> -	grep "^error: cannot delete branch .my7. used by worktree at " actual &&
> +	test_grep "^error: cannot delete branch '"'"'my7'"'"' used by worktree at '"'.*' for bisect\$"'" actual &&
>  	rm -r my7 &&
>  	git worktree prune
>  '

We distinguish four kinds in the code but we test only two of them?

Thanks.  I very much like the direction this is taking us.

Re: [PATCH] branch: report kind of checkout when rejecting delete

[René Scharfe]

On 7/18/26 7:34 PM, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
> 
>> git branch refuses to delete branches that are currently checked out
>> with a message like this: "error: cannot delete branch 'foo' used by
>> worktree at '/path/of/worktree'".  This can be confusing with internal
>> checkouts, e.g. if one tries to delete a branch associated with an
>> active bisect run.
>>
>> Mention the kind of internal checkout, if any, to spare the user from
>> remembering that they might have forgotten a bisect or rebase.  To do
>> that, register the checkout reason in a strintmap alongside the existing
>> strmap that stores the worktree path.
>>
>> Suggested-by: stsp <stsp2@yandex.ru>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>> Original message:
>> https://lore.kernel.org/git/cae34516-5437-49d3-8d39-16f4059a81a8@yandex.ru/
> 
> This reminds me of another recent discussion on rewriting a branch
> that is checked out elsewhere, where the "git history" command
> forgot to apply the same safety check:
> 
> https://lore.kernel.org/git/e7dbcede-4486-459c-aa64-e44690e01fe0@gmail.com/
> 
> We definitely need an easy-to-use API to determine consistently
> which branches are in use, and to teach all commands that repoint
> branch tips to use it to offer the same safety to users.  The
> framework that this patch introduces might be a good starting point
> for that effort.

branch_checked_out() already allows to check whether a branch is in use,
but I guess git history needs to respond differently depending on the
kind of use, e.g. leave active bisects and rebases untouched and update
checked out branches.  branch_checkout_kind() would allow that, but I
now wonder if it suffices for cases where branches appear in multiple
worktrees.  So perhaps the query we need to enable are "is this branch
used by a rebase/bisect" and not the more limited "tell me one use of
this branch"?

>> diff --git a/branch.h b/branch.h
>> index 3dc6e2a0ff..d1073fe1cd 100644
>> --- a/branch.h
>> +++ b/branch.h
>> @@ -15,6 +15,14 @@ enum branch_track {
>>  	BRANCH_TRACK_SIMPLE,
>>  };
>>  
>> +enum branch_checkout_kind {
>> +	BRANCH_CHECKOUT_KIND_UNSPECIFIED = 0,
>> +	BRANCH_CHECKOUT_KIND_CHECKOUT,
>> +	BRANCH_CHECKOUT_KIND_REBASE,
>> +	BRANCH_CHECKOUT_KIND_BISECT,
>> +	BRANCH_CHECKOUT_KIND_UPDATE_REF,
>> +};
>> +
>> ...
>> +/*
>> + * If the branch at 'refname' is currently checked out in a worktree,
>> + * then return the kind of checkout, i.e. whether it was done by an
>> + * actual checkout or a rebase etc.
>> + */
>> +enum branch_checkout_kind branch_checkout_kind(const char *refname);
> 
> OK.
> 
>> diff --git a/builtin/branch.c b/builtin/branch.c
>> index dede60d27b..3223347129 100644
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -266,9 +266,34 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>>  		if (kinds == FILTER_REFS_BRANCHES) {
>>  			const char *path;
>>  			if ((path = branch_checked_out(name))) {
>> -				error(_("cannot delete branch '%s' "
>> -					"used by worktree at '%s'"),
>> -				      bname.buf, path);
>> +				int kind = branch_checkout_kind(name);
> 
> Not "enum branch_checkout_kind" but "int"?

Yes, it doesn't matter for the switch and is easier to print.

> 
>> +				switch (kind) {
>> +				case BRANCH_CHECKOUT_KIND_CHECKOUT:
>> +					error(_("cannot delete branch '%s' "
>> +						"used by worktree at '%s'"),
>> +					      bname.buf, path);
>> +					break;
> 
> We may want to be more explicit and say "cannot delete
> branch 'frotz' checked out in worktree at '/tmp/nitfol'"
> instead.  Unless this is a catch-all entry for states that
> are neither 'rebase', 'bisect', nor 'rebase-merges' but are
> somehow otherwise in use, that is.
> 
>> +				case BRANCH_CHECKOUT_KIND_UPDATE_REF:
>> +					error(_("cannot delete branch '%s' "
>> +						"used by worktree at '%s' "
>> +						"for update-ref"),
>> +					      bname.buf, path);
>> +					break;
> 
> I was quite lost when searching for cases where this 'update-ref'
> state might be encountered, and I still lack confidence.  Can
> we make the diagnostic message a bit friendlier to our users?
> 
> For instance, something like: 'You are rebasing a history with
> merges in that other worktree, and the tip of this branch will
> be updated when that process completes, so you cannot delete
> it from here.'  (Naturally, I may have misidentified the exact
> nature of the error, but this illustrates the level of detail and
> user-facing clarity I hope to see.)

That's quite long.  Would it make sense to throw that update-ref
case into the rebase bin, i.e. only distinguish between checkout,
bisect and rebase?

> 
>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
>> index e2682a83a0..e5df493b66 100755
>> --- a/t/t3200-branch.sh
>> +++ b/t/t3200-branch.sh
>> @@ -930,7 +930,7 @@ test_expect_success 'deleting currently checked out branch fails' '
>>  	git worktree add -b my7 my7 &&
>>  	test_must_fail git -C my7 branch -d my7 &&
>>  	test_must_fail git branch -d my7 2>actual &&
>> -	grep "^error: cannot delete branch .my7. used by worktree at " actual &&
>> +	test_grep "^error: cannot delete branch '"'"'my7'"'"' used by worktree at '"'.*'\$"'" actual &&
>>  	rm -r my7 &&
>>  	git worktree prune
>>  '
>> @@ -941,7 +941,7 @@ test_expect_success 'deleting in-use branch fails' '
>>  	git -C my7 bisect start HEAD HEAD~2 &&
>>  	test_must_fail git -C my7 branch -d my7 &&
>>  	test_must_fail git branch -d my7 2>actual &&
>> -	grep "^error: cannot delete branch .my7. used by worktree at " actual &&
>> +	test_grep "^error: cannot delete branch '"'"'my7'"'"' used by worktree at '"'.*' for bisect\$"'" actual &&
>>  	rm -r my7 &&
>>  	git worktree prune
>>  '
> 
> We distinguish four kinds in the code but we test only two of them?
Laziness, and it was enough for the user requirement..

René

Re: [PATCH] branch: report kind of checkout when rejecting delete

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

>>> +				int kind = branch_checkout_kind(name);
>> 
>> Not "enum branch_checkout_kind" but "int"?
>
> Yes, it doesn't matter for the switch and is easier to print.

I do not understand the "print" part.  I was probably in the last
group of people who was forced to switch from CPP macros to enum
and their argument was always "'print kind' in GDB gives symbolic
output".  As "enum" is an glorified "int", wouldn't 

	int i_kind;
	enum branch_checkout_kind e_kind;
	BUG(_("we did not expect %d %d"), e_kind, i_kind);

do just what we expect?

>>> +				switch (kind) {
>>> +				case BRANCH_CHECKOUT_KIND_CHECKOUT:
>>> +					error(_("cannot delete branch '%s' "
>>> +						"used by worktree at '%s'"),
>>> +					      bname.buf, path);
>>> +					break;
>> 
>> We may want to be more explicit and say "cannot delete
>> branch 'frotz' checked out in worktree at '/tmp/nitfol'"
>> instead.  Unless this is a catch-all entry for states that
>> are neither 'rebase', 'bisect', nor 'rebase-merges' but are
>> somehow otherwise in use, that is.
>> 
>>> +				case BRANCH_CHECKOUT_KIND_UPDATE_REF:
>>> +					error(_("cannot delete branch '%s' "
>>> +						"used by worktree at '%s' "
>>> +						"for update-ref"),
>>> +					      bname.buf, path);
>>> +					break;
>> 
>> I was quite lost when searching for cases where this 'update-ref'
>> state might be encountered, and I still lack confidence.  Can
>> we make the diagnostic message a bit friendlier to our users?
>> 
>> For instance, something like: 'You are rebasing a history with
>> merges in that other worktree, and the tip of this branch will
>> be updated when that process completes, so you cannot delete
>> it from here.'  (Naturally, I may have misidentified the exact
>> nature of the error, but this illustrates the level of detail and
>> user-facing clarity I hope to see.)
>
> That's quite long.  Would it make sense to throw that update-ref
> case into the rebase bin, i.e. only distinguish between checkout,
> bisect and rebase?

Shortening a quite long expression down to digestable pieces is left
as an exercise for those with this particular itch to scratch ;-).
I do not personally mind if it ends up indistinguishable from other
"rebase" case (or unified the "kind" enum into one), but others may
have ideas to shorten the message to fit in the pattern we see
above.

Thanks.