threads / discuss / 41376

git worktree fails to recreate existing branch even with -B

Subject: git worktree fails to recreate existing branch even with -B

## tl;dr

6 messages between Feb 9, 2016 and Feb 16, 2016.

replies: 5people: 3as markdown or json

Kirill Likhodedov· Feb 9, 2016, 14:54 UTC · lore
Git doesn’t allow me to execute 
    git worktree add -B <branch> <path> <start-point>
where <branch> already exists in the repository.
The command prints the following:
    Preparing <path> (identifier <branch>)
    fatal: Refusing to point HEAD outside of refs/
I’m trying to create a worktree on an existing branch <branch>, which should point to <start-point>. This obviously should fail with “-b”, but I use “-B” and expect it to be reset to <start-point> as mentioned in the docs:
    By default, -b refuses to create a new branch if it already exists.  
    -B overrides this safeguard, resetting <new-branch> to <branch>.
Do I miss something or there is a bug?
Steps to reproduce:

git init wt cd wt echo 'asd' > a.txt ; git add a.txt ; git commit -m initial git branch br1 git worktree add -B br1 ~/temp/br1 master

Error message: Preparing /Users/loki/temp/br1 (identifier br1) fatal: Refusing to point HEAD outside of refs/

Thanks a lot! -- Kirill

Duy Nguyen· Feb 15, 2016, 09:41 UTC · re: Kirill Likhodedov · lore

Re: git worktree fails to recreate existing branch even with -B

On Tue, Feb 09, 2016 at 05:54:01PM +0300, Kirill Likhodedov wrote:
Show 17 quoted lines
> Git doesn’t allow me to execute 
>     git worktree add -B <branch> <path> <start-point>
> where <branch> already exists in the repository.
> 
> The command prints the following:
>     Preparing <path> (identifier <branch>)
>     fatal: Refusing to point HEAD outside of refs/
> 
> I’m trying to create a worktree on an existing branch <branch>,
> which should point to <start-point>. This obviously should fail with
> “-b”, but I use “-B” and expect it to be reset to <start-point> as
> mentioned in the docs:
> 
>     By default, -b refuses to create a new branch if it already exists.  
>     -B overrides this safeguard, resetting <new-branch> to <branch>.
> 
> Do I miss something or there is a bug?
According to the man page, this looks like a bug.
Show 11 quoted lines
> Steps to reproduce:
> 
> git init wt
> cd wt
> echo 'asd' > a.txt ; git add a.txt ; git commit -m initial
> git branch br1
> git worktree add -B br1 ~/temp/br1 master
> 
> Error message:
> Preparing /Users/loki/temp/br1 (identifier br1)
> fatal: Refusing to point HEAD outside of refs/
GIT_TRACE=2 gives me

trace: built-in: git 'symbolic-ref' 'HEAD' '' fatal: Refusing to point HEAD outside of refs/

So we pass wrong argument to symbolic-ref. The '' should be 'refs/heads/br1'. This patch seems to fix it.

-- 8< --
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 475b958..d5b319f 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -202,7 +202,7 @@ static int add_worktree(const char *path, const char *refname,
 
 	/* is 'refname' a branch or commit? */
 	if (opts->force_new_branch) /* definitely a branch */
-		;
+		strbuf_addf(&symref, "refs/heads/%s", refname);
 	else if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
 		 ref_exists(symref.buf)) { /* it's a branch */
 		if (!opts->force)
-- 8< --
--
Duy
Nguyễn Thái Ngọc Duy· Feb 15, 2016, 13:35 UTC · re: Duy Nguyen · lore

[PATCH 1/2] worktree: fix "add -B"

Current code does not update "symref" when -B is used. This string contains the new HEAD. Because it's empty "git worktree add -B" fails at symbolic-ref step.

Because branch creation is already done before calling add_worktree(), -B is equivalent to -b from add_worktree() point of view. We do not need the special case for -B.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Complete patch.
 builtin/worktree.c      | 4 +---
 t/t2025-worktree-add.sh | 8 ++++++++
 2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 475b958..6b9c946 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -201,9 +201,7 @@ static int add_worktree(const char *path, const char *refname,
 		die(_("'%s' already exists"), path);
 
 	/* is 'refname' a branch or commit? */
-	if (opts->force_new_branch) /* definitely a branch */
-		;
-	else if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
+	if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
 		 ref_exists(symref.buf)) { /* it's a branch */
 		if (!opts->force)
 			die_if_checked_out(symref.buf);
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 0a804da..a4d36c0 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -193,6 +193,14 @@ test_expect_success '"add" -B/--detach mutually exclusive' '
 	test_must_fail git worktree add -B poodle --detach bamboo master
 '
 
+test_expect_success 'add -B' '
+	git worktree add -B poodle bamboo2 master^ &&
+	git -C bamboo2 symbolic-ref HEAD >actual &&
+	echo refs/heads/poodle >expected &&
+	test_cmp expected actual &&
+	test_cmp_rev master^ poodle
+'
+
 test_expect_success 'local clone from linked checkout' '
 	git clone --local here here-clone &&
 	( cd here-clone && git fsck )

-- 
2.7.0.377.g4cd97dd
Nguyễn Thái Ngọc Duy· Feb 15, 2016, 13:35 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 2/2] worktree add -B: do the checkout test before update branch

If --force is not given but -B is, we should not proceed if the given branch is already checked out elsewhere. add_worktree() has this test, but it kicks in too late when "git branch --force" is already executed. As a result, even though we correctly refuse to create a new worktree, we have already updated the branch and mess up the other checkout.

Repeat the die_if_checked_out() test again for this specific case before "git branch" runs.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Something extra while I was studying this code. I'm not so sure if
 this is the right way.
 
 Another option is do it in "git branch" which rejects with "Cannot
 force update the current branch", but only for current worktree.
 builtin/worktree.c      | 11 ++++++++++-
 t/t2025-worktree-add.sh |  7 +++++++
 2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 6b9c946..20cf67a 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -334,9 +334,18 @@ static int add(int ac, const char **av, const char *prefix)
 	branch = ac < 2 ? "HEAD" : av[1];
 
 	opts.force_new_branch = !!new_branch_force;
-	if (opts.force_new_branch)
+	if (opts.force_new_branch) {
+		struct strbuf symref = STRBUF_INIT;
+
 		opts.new_branch = new_branch_force;
 
+		if (!opts.force &&
+		    !strbuf_check_branch_ref(&symref, opts.new_branch) &&
+		    ref_exists(symref.buf))
+			die_if_checked_out(symref.buf);
+		strbuf_release(&symref);
+	}
+
 	if (ac < 2 && !opts.new_branch && !opts.detach) {
 		int n;
 		const char *s = worktree_basename(path, &n);
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index a4d36c0..cbfa41e 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -193,6 +193,13 @@ test_expect_success '"add" -B/--detach mutually exclusive' '
 	test_must_fail git worktree add -B poodle --detach bamboo master
 '
 
+test_expect_success '"add -B" fails if the branch is checked out' '
+	git rev-parse newmaster >before &&
+	test_must_fail git worktree add -B newmaster bamboo master &&
+	git rev-parse newmaster >after &&
+	test_cmp before after
+'
+
 test_expect_success 'add -B' '
 	git worktree add -B poodle bamboo2 master^ &&
 	git -C bamboo2 symbolic-ref HEAD >actual &&

-- 
2.7.0.377.g4cd97dd
Junio C Hamano· Feb 15, 2016, 23:53 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 1/2] worktree: fix "add -B"

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 29 quoted lines
> Current code does not update "symref" when -B is used. This string
> contains the new HEAD. Because it's empty "git worktree add -B" fails at
> symbolic-ref step.
>
> Because branch creation is already done before calling add_worktree(),
> -B is equivalent to -b from add_worktree() point of view. We do not need
> the special case for -B.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Complete patch.
>
>  builtin/worktree.c      | 4 +---
>  t/t2025-worktree-add.sh | 8 ++++++++
>  2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 475b958..6b9c946 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -201,9 +201,7 @@ static int add_worktree(const char *path, const char *refname,
>  		die(_("'%s' already exists"), path);
>  
>  	/* is 'refname' a branch or commit? */
> -	if (opts->force_new_branch) /* definitely a branch */
> -		;
> -	else if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
> +	if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
>  		 ref_exists(symref.buf)) { /* it's a branch */

Makes a reader wonder why the original thought it was OK to do nothing when -B is given here.

What does symref.buf have at this point in the codeflow? Will it always an existing branch? In what case can it be the name of a branch that does not yet exist?

Thanks.
Show 21 quoted lines
>  		if (!opts->force)
>  			die_if_checked_out(symref.buf);
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index 0a804da..a4d36c0 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -193,6 +193,14 @@ test_expect_success '"add" -B/--detach mutually exclusive' '
>  	test_must_fail git worktree add -B poodle --detach bamboo master
>  '
>  
> +test_expect_success 'add -B' '
> +	git worktree add -B poodle bamboo2 master^ &&
> +	git -C bamboo2 symbolic-ref HEAD >actual &&
> +	echo refs/heads/poodle >expected &&
> +	test_cmp expected actual &&
> +	test_cmp_rev master^ poodle
> +'
> +
>  test_expect_success 'local clone from linked checkout' '
>  	git clone --local here here-clone &&
>  	( cd here-clone && git fsck )
Duy Nguyen· Feb 16, 2016, 01:15 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] worktree: fix "add -B"

On Tue, Feb 16, 2016 at 6:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 34 quoted lines
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Current code does not update "symref" when -B is used. This string
>> contains the new HEAD. Because it's empty "git worktree add -B" fails at
>> symbolic-ref step.
>>
>> Because branch creation is already done before calling add_worktree(),
>> -B is equivalent to -b from add_worktree() point of view. We do not need
>> the special case for -B.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  Complete patch.
>>
>>  builtin/worktree.c      | 4 +---
>>  t/t2025-worktree-add.sh | 8 ++++++++
>>  2 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> index 475b958..6b9c946 100644
>> --- a/builtin/worktree.c
>> +++ b/builtin/worktree.c
>> @@ -201,9 +201,7 @@ static int add_worktree(const char *path, const char *refname,
>>               die(_("'%s' already exists"), path);
>>
>>       /* is 'refname' a branch or commit? */
>> -     if (opts->force_new_branch) /* definitely a branch */
>> -             ;
>> -     else if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
>> +     if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
>>                ref_exists(symref.buf)) { /* it's a branch */
>
> Makes a reader wonder why the original thought it was OK to do
> nothing when -B is given here.

The bug is quite subtle. This code is added in f7c9dac. At that commit, I believe symref is simply a temporary var, to be used by ref_exists() and nothing else. The next commit 7f44e3d replaces git-checkout with git-symbolic-ref and symref is used again. But the symref initialization code is not in the diff context lines, so it's hard to see that there's one case where symref remains empty.

> What does symref.buf have at this point in the codeflow?
Empty.
> Will it always an existing branch?
It should be.
> In what case can it be the name of a branch that does not yet exist?
You can do "worktree add <path> <non-existing-ref>".
-- 
Duy

← back to recent threads