Skip to content

Commit

Permalink
branch: fix branch renaming not updating HEADs correctly
Browse files Browse the repository at this point in the history
There are two bugs that sort of work together and cause
problems. Let's start with one in replace_each_worktree_head_symref.

Before fa099d2 (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 fa099d2, 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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
pclouds authored and gitster committed Aug 24, 2017
1 parent d026a25 commit 31824d1
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 3 deletions.
5 changes: 3 additions & 2 deletions branch.c
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Expand Down
13 changes: 13 additions & 0 deletions t/t3200-branch.sh
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,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 'git branch -M baz bam should succeed when baz is checked out as linked working tree' '
git checkout master &&
git worktree add -b baz bazdir &&
Expand Down
2 changes: 1 addition & 1 deletion worktree.c
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,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;
Expand Down

0 comments on commit 31824d1

Please sign in to comment.