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

[Junio C Hamano <gitster@pobox.com>]

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.