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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]