git worktree fails to recreate existing branch even with -B

git worktree fails to recreate existing branch even with -B

[Kirill Likhodedov]

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

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

[Duy Nguyen]

On Tue, Feb 09, 2016 at 05:54:01PM +0300, Kirill Likhodedov wrote:
> 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.

> 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

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

[Nguyễn Thái Ngọc Duy]

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

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

[Nguyễn Thái Ngọc Duy]

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

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

[Junio C Hamano]

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.

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.

>  		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 )

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

[Duy Nguyen]

On Tue, Feb 16, 2016 at 6:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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