threads / patch / 58691

patchbranch: error copying or renaming a detached HEAD

Subject: [PATCH] branch: error copying or renaming a detached HEAD

## tl;dr

2 messages between Oct 25, 2022 and Oct 25, 2022. Diffs are folded; open one to read it.

replies: 1people: 2as markdown or json

Rubén Justo· Oct 25, 2022, 23:01 UTC · lore

In c847f53712 (Detached HEAD (experimental), 2007-01-01) an error condition was introduced in rename_branch() to prevent renaming, later also copying, a detached HEAD.

The condition used was checking for NULL in oldname, the source branch to rename/copy. That condition cannot be satisfied because if no source branch is specified, HEAD is going to be used in the call.

The error issued instead is:
	fatal: Invalid branch name: 'HEAD'

Let's remove the condition in copy_or_rename_branch() (the current function name) and check for HEAD before calling it, dying with the original intended error if we're in a detached HEAD.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c  | 28 ++++++++--------------------
 t/t3200-branch.sh | 11 +++++++++++
 2 files changed, 19 insertions(+), 20 deletions(-)
Show changes to 2 files +19 −20

builtin/branch.c, t/t3200-branch.sh

diff --git a/builtin/branch.c b/builtin/branch.c
index 407517ba68..c964ac7bb4 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -520,13 +520,6 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	const char *interpreted_newname = NULL;
 	int recovery = 0;
 
-	if (!oldname) {
-		if (copy)
-			die(_("cannot copy the current branch while not on any."));
-		else
-			die(_("cannot rename the current branch while not on any."));
-	}
-
 	if (strbuf_check_branch_ref(&oldref, oldname)) {
 		/*
 		 * Bad name --- this could be an attempt to rename a
@@ -827,24 +820,19 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		strbuf_release(&buf);
 
 		return ret;
-	} else if (copy) {
-		if (!argc)
-			die(_("branch name required"));
-		else if (argc == 1)
-			copy_or_rename_branch(head, argv[0], 1, copy > 1);
-		else if (argc == 2)
-			copy_or_rename_branch(argv[0], argv[1], 1, copy > 1);
-		else
-			die(_("too many branches for a copy operation"));
-	} else if (rename) {
+	} else if (copy || rename) {
 		if (!argc)
 			die(_("branch name required"));
+		else if ((argc == 1) && filter.detached)
+			die(copy? _("cannot copy the current branch while not on any.")
+				: _("cannot rename the current branch while not on any."));
 		else if (argc == 1)
-			copy_or_rename_branch(head, argv[0], 0, rename > 1);
+			copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
 		else if (argc == 2)
-			copy_or_rename_branch(argv[0], argv[1], 0, rename > 1);
+			copy_or_rename_branch(argv[0], argv[1], copy, copy + rename > 1);
 		else
-			die(_("too many arguments for a rename operation"));
+			die(copy? _("too many branches for a copy operation")
+				: _("too many arguments for a rename operation"));
 	} else if (new_upstream) {
 		struct branch *branch;
 		struct strbuf buf = STRBUF_INIT;
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 7d8edff9c3..38c57de71b 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -268,6 +268,17 @@ test_expect_success 'git branch -M topic topic should work when main is checked
 	git branch -M topic topic
 '
 
+test_expect_success 'git branch -M and -C fail on detached HEAD' '
+	git checkout HEAD^{} &&
+	test_when_finished git checkout - &&
+	echo "fatal: cannot rename the current branch while not on any." >expect &&
+	test_must_fail git branch -M must-fail 2>err &&
+	test_cmp expect err &&
+	echo "fatal: cannot copy the current branch while not on any." >expect &&
+	test_must_fail git branch -C must-fail 2>err &&
+	test_cmp expect err
+'
+
 test_expect_success 'git branch -v -d t should work' '
 	git branch t &&
 	git rev-parse --verify refs/heads/t &&

-- 
2.36.1
Junio C Hamano· Oct 25, 2022, 23:50 UTC · re: Rubén Justo · lore

Re: [PATCH] branch: error copying or renaming a detached HEAD

Rubén Justo <rjusto@gmail.com> writes:
> Let's remove the condition in copy_or_rename_branch() (the current
> function name) and check for HEAD before calling it, dying with the
> original intended error if we're in a detached HEAD.
Makes sense.
Show 20 quoted lines
> @@ -827,24 +820,19 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  		strbuf_release(&buf);
>  
>  		return ret;
> -	} else if (copy) {
> -		if (!argc)
> -			die(_("branch name required"));
> -		else if (argc == 1)
> -			copy_or_rename_branch(head, argv[0], 1, copy > 1);
> -		else if (argc == 2)
> -			copy_or_rename_branch(argv[0], argv[1], 1, copy > 1);
> -		else
> -			die(_("too many branches for a copy operation"));
> -	} else if (rename) {
> +	} else if (copy || rename) {
>  		if (!argc)
>  			die(_("branch name required"));
> +		else if ((argc == 1) && filter.detached)
> +			die(copy? _("cannot copy the current branch while not on any.")
> +				: _("cannot rename the current branch while not on any."));
Missing " " before "?".
>  		else if (argc == 1)
> -			copy_or_rename_branch(head, argv[0], 0, rename > 1);
> +			copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);

The third argument being 'copy' makes sense (as the original has 1 for copy and 0 for rename). As we reject if rename and copy are both set, here we have either copy > 0 or rename > 0 but not both, so the fourth argument makes sense, too.

Show 7 quoted lines
>  		else if (argc == 2)
> -			copy_or_rename_branch(argv[0], argv[1], 0, rename > 1);
> +			copy_or_rename_branch(argv[0], argv[1], copy, copy + rename > 1);
>  		else
> -			die(_("too many arguments for a rename operation"));
> +			die(copy? _("too many branches for a copy operation")
> +				: _("too many arguments for a rename operation"));
Ditto.
Show 18 quoted lines
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 7d8edff9c3..38c57de71b 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -268,6 +268,17 @@ test_expect_success 'git branch -M topic topic should work when main is checked
>  	git branch -M topic topic
>  '
>  
> +test_expect_success 'git branch -M and -C fail on detached HEAD' '
> +	git checkout HEAD^{} &&
> +	test_when_finished git checkout - &&
> +	echo "fatal: cannot rename the current branch while not on any." >expect &&
> +	test_must_fail git branch -M must-fail 2>err &&
> +	test_cmp expect err &&
> +	echo "fatal: cannot copy the current branch while not on any." >expect &&
> +	test_must_fail git branch -C must-fail 2>err &&
> +	test_cmp expect err
> +'
Excellent.

I'll do the whitespace tweak while queueing, so there is no need to resend only to fix them.

Thanks.

← back to recent threads