* git worktree fails to recreate existing branch even with -B
@ 2016-02-09 14:54 Kirill Likhodedov
2016-02-15 9:41 ` Duy Nguyen
0 siblings, 1 reply; 6+ messages in thread
From: Kirill Likhodedov @ 2016-02-09 14:54 UTC (permalink / raw)
To: git
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: git worktree fails to recreate existing branch even with -B 2016-02-09 14:54 git worktree fails to recreate existing branch even with -B Kirill Likhodedov @ 2016-02-15 9:41 ` Duy Nguyen 2016-02-15 13:35 ` [PATCH 1/2] worktree: fix "add -B" Nguyễn Thái Ngọc Duy 0 siblings, 1 reply; 6+ messages in thread From: Duy Nguyen @ 2016-02-15 9:41 UTC (permalink / raw) To: Kirill Likhodedov; +Cc: git, Eric Sunshine 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 1/2] worktree: fix "add -B" 2016-02-15 9:41 ` Duy Nguyen @ 2016-02-15 13:35 ` Nguyễn Thái Ngọc Duy 2016-02-15 13:35 ` [PATCH 2/2] worktree add -B: do the checkout test before update branch Nguyễn Thái Ngọc Duy 2016-02-15 23:53 ` [PATCH 1/2] worktree: fix "add -B" Junio C Hamano 0 siblings, 2 replies; 6+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-02-15 13:35 UTC (permalink / raw) To: git; +Cc: Eric Sunshine, kirill.likhodedov, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] worktree add -B: do the checkout test before update branch 2016-02-15 13:35 ` [PATCH 1/2] worktree: fix "add -B" Nguyễn Thái Ngọc Duy @ 2016-02-15 13:35 ` Nguyễn Thái Ngọc Duy 2016-02-15 23:53 ` [PATCH 1/2] worktree: fix "add -B" Junio C Hamano 1 sibling, 0 replies; 6+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-02-15 13:35 UTC (permalink / raw) To: git; +Cc: Eric Sunshine, kirill.likhodedov, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] worktree: fix "add -B" 2016-02-15 13:35 ` [PATCH 1/2] worktree: fix "add -B" Nguyễn Thái Ngọc Duy 2016-02-15 13:35 ` [PATCH 2/2] worktree add -B: do the checkout test before update branch Nguyễn Thái Ngọc Duy @ 2016-02-15 23:53 ` Junio C Hamano 2016-02-16 1:15 ` Duy Nguyen 1 sibling, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2016-02-15 23:53 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy Cc: git, Eric Sunshine, kirill.likhodedov 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 ) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] worktree: fix "add -B" 2016-02-15 23:53 ` [PATCH 1/2] worktree: fix "add -B" Junio C Hamano @ 2016-02-16 1:15 ` Duy Nguyen 0 siblings, 0 replies; 6+ messages in thread From: Duy Nguyen @ 2016-02-16 1:15 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, Eric Sunshine, Kirill Likhodedov 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-02-16 1:15 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-02-09 14:54 git worktree fails to recreate existing branch even with -B Kirill Likhodedov 2016-02-15 9:41 ` Duy Nguyen 2016-02-15 13:35 ` [PATCH 1/2] worktree: fix "add -B" Nguyễn Thái Ngọc Duy 2016-02-15 13:35 ` [PATCH 2/2] worktree add -B: do the checkout test before update branch Nguyễn Thái Ngọc Duy 2016-02-15 23:53 ` [PATCH 1/2] worktree: fix "add -B" Junio C Hamano 2016-02-16 1:15 ` Duy Nguyen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).