Skip to content

Commit

Permalink
git: on external HEAD move, do not abandon old branch
Browse files Browse the repository at this point in the history
The current behavior was introduced by 20eb9ec "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.
  • Loading branch information
yuja committed May 11, 2023
1 parent 66d405f commit 92cfffd
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 9 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions lib/src/git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,16 +91,16 @@ 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).
if let Ok(head_git_commit) = git_repo
.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());
Expand Down
10 changes: 6 additions & 4 deletions lib/tests/test_git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
}

Expand Down Expand Up @@ -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()));
Expand Down
7 changes: 5 additions & 2 deletions tests/test_git_colocated.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
"###);
}
Expand Down

0 comments on commit 92cfffd

Please sign in to comment.