git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
	nish.aravamudan@canonical.com, me@ikke.info,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Subject: [PATCH] Fix branch renaming not updating HEADs correctly
Date: Thu, 24 Aug 2017 17:41:24 +0700	[thread overview]
Message-ID: <20170824104124.6263-1-pclouds@gmail.com> (raw)
In-Reply-To: <20170823201334.bz42s6t5ti4jdaqm@pitfall>

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


  parent reply	other threads:[~2017-08-24 10:41 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-23 20:13 Undocumented change in `git branch -M` behavior Nish Aravamudan
2017-08-24  5:32 ` Kevin Daudt
2017-08-24  8:59 ` Duy Nguyen
2017-08-24 10:41 ` Nguyễn Thái Ngọc Duy [this message]
2017-08-24 19:04   ` [PATCH] Fix branch renaming not updating HEADs correctly Nish Aravamudan
2017-08-27  5:39   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170824104124.6263-1-pclouds@gmail.com \
    --to=pclouds@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=me@ikke.info \
    --cc=nish.aravamudan@canonical.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).