Undocumented change in `git branch -M` behavior

Undocumented change in `git branch -M` behavior

[Nish Aravamudan]

Hello,

Hopefully, I've got this right -- I noticed a change in behavior in git
with Ubuntu 17.10, which recently got 2.14.1. Specifically, that when in
an orphaned branch, -M ends up moving HEAD to the new branch name,
clobbering the working tree. As far as I know, from the manpages,
orphaned branches are still supported and should work?

I think an example will demonstrate more than words (the following are
done in LXD containers, hence the root user):

# git --version
git version 2.14.1
# mkdir test && cd test && git init .
Initialized empty Git repository in /root/test/.git/
# git checkout -b a
Switched to a new branch 'a'
# touch testfile && git add testfile && git commit -m 'initial commit'
[a (root-commit) 6061193] initial commit
 Committer: root <root@precious-magpie.lxd>
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 testfile
# git checkout --orphan master
Switched to a new branch 'master'
# git status
On branch master

No commits yet

Changes to be committed:
  (use "git rm --cached <file>..." to unstage)

        new file:   testfile

# git reset --hard && git status
On branch master

No commits yet

nothing to commit (create/copy files and use "git add" to track)
# git branch -M a b
# git status
On branch b
Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

        deleted:    testfile

This is very unexpected. I force-renamed a branch I wasn't currently
checked out to and now I'm checked out to it *and* I have staged file
removals (I think what is effectively happening is my current working
directory (empty) is being staged into the new branch, but I'm not
100%).

For comparision, on 17.04:

# git --version
git version 2.11.0
# mkdir test && cd test && git init .
Initialized empty Git repository in /root/test/.git/
# git checkout -b a
Switched to a new branch 'a'
# touch testfile && git add testfile && git commit -m 'initial commit'
[a (root-commit) f8d0d53] initial commit
 Committer: root <root@honest-sturgeon.lxd>
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 testfile
# git checkout --orphan master
Switched to a new branch 'master'
# git status
On branch master

No commits yet

Changes to be committed:
  (use "git rm --cached <file>..." to unstage)

        new file:   testfile

# git reset --hard && git status
On branch master

No commits yet

nothing to commit (create/copy files and use "git add" to track)
# git branch -M a b
# git status
On branch master

Initial commit

nothing to commit (create/copy files and use "git add" to track)

This is what I expect to see, the branch rename has no effect on HEAD.

I haven't yet bisected this (but I can if necessary). My initial
suspicion is
https://github.com/git/git/commit/70999e9ceca47e03b8900bfb310b2f804125811e#diff-d18f86ea14e2f1e5bff391b2e54438cb
where a comparison between the oldname of the branch and HEAD was
performed before attempting to move HEAD (so that HEAD followed to the
new branch name, I believe). That change was dropped, though and perhaps
the new check in replace_each_worktree_head_symref of

        strcmp(oldref, worktrees[i]->head_ref)

does not work for orphaned branches? I am unfamiliar with all the
details of the git internals, so please correct me if I'm wrong!

Thanks,
Nish

-- 
Nishanth Aravamudan
Ubuntu Server
Canonical Ltd

Re: Undocumented change in `git branch -M` behavior

[Kevin Daudt]

On Wed, Aug 23, 2017 at 01:13:34PM -0700, Nish Aravamudan wrote:
> Hello,
> 
> Hopefully, I've got this right -- I noticed a change in behavior in git
> with Ubuntu 17.10, which recently got 2.14.1. Specifically, that when in
> an orphaned branch, -M ends up moving HEAD to the new branch name,
> clobbering the working tree. As far as I know, from the manpages,
> orphaned branches are still supported and should work?
> 
> I think an example will demonstrate more than words (the following are
> done in LXD containers, hence the root user):
> 
> # git --version
> git version 2.14.1
> # mkdir test && cd test && git init .
> Initialized empty Git repository in /root/test/.git/
> # git checkout -b a
> Switched to a new branch 'a'
> # touch testfile && git add testfile && git commit -m 'initial commit'
> [a (root-commit) 6061193] initial commit
>  Committer: root <root@precious-magpie.lxd>
>  1 file changed, 0 insertions(+), 0 deletions(-)
>  create mode 100644 testfile
> # git checkout --orphan master
> Switched to a new branch 'master'
> # git status
> On branch master
> 
> No commits yet
> 
> Changes to be committed:
>   (use "git rm --cached <file>..." to unstage)
> 
>         new file:   testfile
> 
> # git reset --hard && git status
> On branch master
> 
> No commits yet
> 
> nothing to commit (create/copy files and use "git add" to track)
> # git branch -M a b
> # git status
> On branch b
> Changes to be committed:
>   (use "git reset HEAD <file>..." to unstage)
> 
>         deleted:    testfile
> 
> This is very unexpected. I force-renamed a branch I wasn't currently
> checked out to and now I'm checked out to it *and* I have staged file
> removals (I think what is effectively happening is my current working
> directory (empty) is being staged into the new branch, but I'm not
> 100%).
> 
> For comparision, on 17.04:
> 
> # git --version
> git version 2.11.0
> # mkdir test && cd test && git init .
> Initialized empty Git repository in /root/test/.git/
> # git checkout -b a
> Switched to a new branch 'a'
> # touch testfile && git add testfile && git commit -m 'initial commit'
> [a (root-commit) f8d0d53] initial commit
>  Committer: root <root@honest-sturgeon.lxd>
>  1 file changed, 0 insertions(+), 0 deletions(-)
>  create mode 100644 testfile
> # git checkout --orphan master
> Switched to a new branch 'master'
> # git status
> On branch master
> 
> No commits yet
> 
> Changes to be committed:
>   (use "git rm --cached <file>..." to unstage)
> 
>         new file:   testfile
> 
> # git reset --hard && git status
> On branch master
> 
> No commits yet
> 
> nothing to commit (create/copy files and use "git add" to track)
> # git branch -M a b
> # git status
> On branch master
> 
> Initial commit
> 
> nothing to commit (create/copy files and use "git add" to track)
> 
> This is what I expect to see, the branch rename has no effect on HEAD.
> 
> I haven't yet bisected this (but I can if necessary). My initial
> suspicion is
> https://github.com/git/git/commit/70999e9ceca47e03b8900bfb310b2f804125811e#diff-d18f86ea14e2f1e5bff391b2e54438cb
> where a comparison between the oldname of the branch and HEAD was
> performed before attempting to move HEAD (so that HEAD followed to the
> new branch name, I believe). That change was dropped, though and perhaps
> the new check in replace_each_worktree_head_symref of
> 
>         strcmp(oldref, worktrees[i]->head_ref)
> 
> does not work for orphaned branches? I am unfamiliar with all the
> details of the git internals, so please correct me if I'm wrong!
> 
> Thanks,
> Nish
> 
> -- 
> Nishanth Aravamudan
> Ubuntu Server
> Canonical Ltd

Thanks for this report. I've bisected it down to 
fa099d232 (worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe(), 2017-04-24) 

I've CC'ed Duy, who made that commit.

Kevin

Re: Undocumented change in `git branch -M` behavior

[Duy Nguyen]

On Thu, Aug 24, 2017 at 3:13 AM, Nish Aravamudan
<nish.aravamudan@canonical.com> wrote:
> Hello,
>
> Hopefully, I've got this right -- I noticed a change in behavior in git
> with Ubuntu 17.10, which recently got 2.14.1. Specifically, that when in
> an orphaned branch, -M ends up moving HEAD to the new branch name,
> clobbering the working tree.

Thanks for the report. I think I see what's going on. I need some more
time to come up with a patch but yes this is definitely a bug (and
caused by my commit)
-- 
Duy

[PATCH] Fix branch renaming not updating HEADs correctly

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

There are two bugs that sort of work together and cause problems. Let's
start with one in replace_each_worktree_head_symref.

Before fa099d2322 (worktree.c: kill parse_ref() in favor of
refs_resolve_ref_unsafe() - 2017-04-24), this code looks like this:

    if (strcmp(oldref, worktrees[i]->head_ref))
            continue;
    set_worktree_head_symref(...);

After fa099d2322, it is possible that head_ref can be NULL. However, the
updated code takes the wrong exit. In the error case (NULL head_ref), we
should "continue;" to the next worktree. The updated code makes us
_skip_ "continue;" and update HEAD anyway.

The NULL head_ref is triggered by the second bug in add_head_info (in
the same commit). With the flag RESOLVE_REF_READING, resolve_ref_unsafe()
will abort if it cannot resolve the target ref. For orphan checkouts,
HEAD always points to an unborned branch, resolving target ref will
always fail. Now we have NULL head_ref. Now we always update HEAD.

Correct the logic in replace_ function so that we don't accidentally
update HEAD on error. As it turns out, correcting the logic bug above
breaks branch renaming completely, thanks to the second bug.

"git branch -[Mm]" does two steps (on a normal checkout, no orphan!):

- rename the branch on disk (e.g. refs/heads/abc to refs/heads/def)
- update HEAD if it points to the branch being renamed.

At the second step, since the branch pointed to by HEAD (e.g. "abc") no
longer exists on disk, we run into a temporary orphan checkout situation
that has been just corrected to _not_ update HEAD. But we need to update
HEAD since it's not actually an orphan checkout. We need to update HEAD
to move out of that orphan state.

Correct add_head_info(), remove RESOLVE_REF_READING flag. With the flag
gone, we should always return good "head_ref" in orphan checkouts (either
temporary or permanent). With good head_ref, things start to work again.

Noticed-by: Nish Aravamudan <nish.aravamudan@canonical.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 branch.c          |  5 +++--
 t/t3200-branch.sh | 13 +++++++++++++
 worktree.c        |  2 +-
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/branch.c b/branch.c
index 36541d05cd..86360d36b3 100644
--- a/branch.c
+++ b/branch.c
@@ -357,8 +357,9 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref,
 
 		if (worktrees[i]->is_detached)
 			continue;
-		if (worktrees[i]->head_ref &&
-		    strcmp(oldref, worktrees[i]->head_ref))
+		if (!worktrees[i]->head_ref)
+			continue;
+		if (strcmp(oldref, worktrees[i]->head_ref))
 			continue;
 
 		refs = get_worktree_ref_store(worktrees[i]);
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index dd37ac47c5..902cb87ea8 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -162,6 +162,19 @@ test_expect_success 'git branch -M baz bam should add entries to .git/logs/HEAD'
 	grep "^0\{40\}.*$msg$" .git/logs/HEAD
 '
 
+test_expect_success 'git branch -M should leave orphaned HEAD alone' '
+	git init orphan &&
+	(
+		cd orphan &&
+		test_commit initial &&
+		git checkout --orphan lonely &&
+		grep lonely .git/HEAD &&
+		test_path_is_missing .git/refs/head/lonely &&
+		git branch -M master mistress &&
+		grep lonely .git/HEAD
+	)
+'
+
 test_expect_success 'resulting reflog can be shown by log -g' '
 	oid=$(git rev-parse HEAD) &&
 	cat >expect <<-EOF &&
diff --git a/worktree.c b/worktree.c
index e28ffbeb09..c0c5a2b373 100644
--- a/worktree.c
+++ b/worktree.c
@@ -30,7 +30,7 @@ static void add_head_info(struct worktree *wt)
 
 	target = refs_resolve_ref_unsafe(get_worktree_ref_store(wt),
 					 "HEAD",
-					 RESOLVE_REF_READING,
+					 0,
 					 wt->head_sha1, &flags);
 	if (!target)
 		return;
-- 
2.11.0.157.gd943d85

Re: [PATCH] Fix branch renaming not updating HEADs correctly

[Nish Aravamudan]

On 24.08.2017 [17:41:24 +0700], Nguyễn Thái Ngọc Duy wrote:
> There are two bugs that sort of work together and cause problems. Let's
> start with one in replace_each_worktree_head_symref.

<snip>

> Correct add_head_info(), remove RESOLVE_REF_READING flag. With the flag
> gone, we should always return good "head_ref" in orphan checkouts (either
> temporary or permanent). With good head_ref, things start to work again.
> 
> Noticed-by: Nish Aravamudan <nish.aravamudan@canonical.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Tested-by: Nish Aravamudan <nish.aravamudan@canonical.com>

Thank you for the quick turnaround on the patch!

-Nish
-- 
Nishanth Aravamudan
Ubuntu Server
Canonical Ltd

Re: [PATCH] Fix branch renaming not updating HEADs correctly

[Junio C Hamano]

Thanks.  I'll merge this down to at least 'next' before I go
offline for a week.