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

[Duy Nguyen]

On Tue, Feb 16, 2016 at 6:53 AM, Junio C Hamano  wrote:
> Nguyễn Thái Ngọc Duy   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 
>> ---
>>  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(, refname) &&
>> + if (!opts->detach && !strbuf_check_branch_ref(, 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  ".
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   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 
> ---
>  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(, refname) &&
> + if (!opts->detach && !strbuf_check_branch_ref(, 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 )
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 
---
 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(, refname) &&
+   if (!opts->detach && !strbuf_check_branch_ref(, 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html