From 92cfffd84377b0172c98ed6b021992d34376695a Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 10 May 2023 19:10:40 +0900 Subject: [PATCH] git: on external HEAD move, do not abandon old branch The current behavior was introduced by 20eb9ecec10f "git: don't abandon HEAD commit when it loses a branch." While the change made HEAD mutation behavior more consistent with a plain ref operation, HEAD can also move on checkout, and checkout shouldn't be considered a history rewriting operation. I'm not saying the new behavior is always correct, but I think it's safer than losing old HEAD branch. I also think this change will help if we want to extract HEAD management function from git::import_refs(). Fixes #1042. --- CHANGELOG.md | 4 ++++ lib/src/git.rs | 6 +++--- lib/tests/test_git.rs | 10 ++++++---- tests/test_git_colocated.rs | 7 +++++-- 4 files changed, 18 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c15dedc74..d210bbbc2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -105,6 +105,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * It is no longer allowed to create branches at the root commit. +* `git checkout` (without using `jj`) in colocated repo no longer abandons + the previously checked-out anonymous branch. + [#1042](https://github.com/martinvonz/jj/issues/1042). + ## [0.7.0] - 2023-02-16 ### Breaking changes diff --git a/lib/src/git.rs b/lib/src/git.rs index 6f17f266b..3df6080c0 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -91,9 +91,6 @@ pub fn import_some_refs( new_git_heads.extend(old_target.adds()); } } - if let Some(old_git_head) = mut_repo.view().git_head() { - old_git_heads.extend(old_git_head.adds()); - } // TODO: Should this be a separate function? We may not always want to import // the Git HEAD (and add it to our set of heads). @@ -101,6 +98,9 @@ pub fn import_some_refs( .head() .and_then(|head_ref| head_ref.peel_to_commit()) { + // Add the current HEAD to `new_git_heads` to pin the branch. It's not added + // to `old_git_heads` because HEAD move doesn't automatically mean the old + // HEAD branch has been rewritten. let head_commit_id = CommitId::from_bytes(head_git_commit.id().as_bytes()); let head_commit = store.get_commit(&head_commit_id).unwrap(); new_git_heads.insert(head_commit_id.clone()); diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index 5133b7632..c72737fd6 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -336,11 +336,13 @@ fn test_import_refs_reimport_git_head_without_ref() { // Move HEAD to commit2 (by e.g. `git checkout` command) git_repo.set_head_detached(git_id(&commit2)).unwrap(); - // Reimport HEAD, which abandons the old HEAD branch because jj thinks it - // would be rewritten by e.g. `git commit --amend` command. + // Reimport HEAD, which doesn't abandon the old HEAD branch because jj thinks it + // would be moved by `git checkout` command. This isn't always true because the + // detached HEAD commit could be rewritten by e.g. `git commit --amend` command, + // but it should be safer than abandoning old checkout branch. git::import_refs(tx.mut_repo(), &git_repo, &git_settings).unwrap(); tx.mut_repo().rebase_descendants(&settings).unwrap(); - assert!(!tx.mut_repo().view().heads().contains(commit1.id())); + assert!(tx.mut_repo().view().heads().contains(commit1.id())); assert!(tx.mut_repo().view().heads().contains(commit2.id())); } @@ -374,7 +376,7 @@ fn test_import_refs_reimport_git_head_with_moved_ref() { .unwrap(); git_repo.set_head_detached(git_id(&commit2)).unwrap(); - // Reimport HEAD and main, which abandons the old HEAD/main branch. + // Reimport HEAD and main, which abandons the old main branch. git::import_refs(tx.mut_repo(), &git_repo, &git_settings).unwrap(); tx.mut_repo().rebase_descendants(&settings).unwrap(); assert!(!tx.mut_repo().view().heads().contains(commit1.id())); diff --git a/tests/test_git_colocated.rs b/tests/test_git_colocated.rs index 5e8e4dfb5..31d2b9a23 100644 --- a/tests/test_git_colocated.rs +++ b/tests/test_git_colocated.rs @@ -297,10 +297,13 @@ fn test_git_colocated_external_checkout() { ) .unwrap(); - // The old HEAD branch gets abandoned because jj thinks it has been rewritten. + // The old working-copy commit gets abandoned, but the whole branch should not + // be abandoned. (#1042) insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ 0521ce3b8c4e29aab79f3c750e2845dcbc4c3874 - ◉ a86754f975f953fa25da4265764adc0c62e9ce6b master + │ ◉ 66f4d1806ae41bd604f69155dece64062a0056cf + ◉ │ a86754f975f953fa25da4265764adc0c62e9ce6b master + ├─╯ ◉ 0000000000000000000000000000000000000000 "###); }