From 23e82ced049352be8c90ae0412693666b5a7907f Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Wed, 2 Nov 2022 22:30:18 -0700 Subject: [PATCH] git: update our record of Git branches on export When we export branches to Git, we didn't update our own record of Git's refs. This frequently led to spurious conflicts in these refs (e.g. #463). This is typically what happened: 1. Import a branch pointing to commit A from Git 2. Modify the branch in jj to point to commit B 3. Export the branch to Git 4. Update the branch in Git to point to commit C 5. Import refs from Git In step 3, we forgot to update our record of the branch in the repo view's `git_refs` field. That led to the import in step 5 to think that the branch moved from A to C in Git, which conflicts with the internal branch target of B. This commit fixes the bug by updating the refs in the `MutableRepo`. Closes #463. --- CHANGELOG.md | 4 ++++ lib/src/git.rs | 10 ++++++++-- lib/src/repo.rs | 4 ++++ lib/src/view.rs | 4 ++++ lib/tests/test_git.rs | 39 ++++++++++++++++++++++++++++++++----- tests/test_git_colocated.rs | 6 +++--- 6 files changed, 57 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd640b6de3f..880c776c258 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed bugs +* (#463) A bug in the export of branches to Git caused spurious conflicted + branches. This typically occurred when running in a working copy colocated + with Git (created by running `jj init --git-dir=.`). + * `jj edit root` now fails gracefully. * `jj git import` used to abandon a commit if Git branches and tags referring diff --git a/lib/src/git.rs b/lib/src/git.rs index 707128e131d..b3c7eaaa06a 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -179,10 +179,11 @@ pub enum GitExportError { /// Returns a stripped-down repo view of the state we just exported, to be used /// as `old_view` next time. fn export_changes( + mut_repo: &mut MutableRepo, old_view: &View, - new_view: &View, git_repo: &git2::Repository, ) -> Result { + let new_view = mut_repo.view(); let old_branches: HashSet<_> = old_view.branches().keys().cloned().collect(); let new_branches: HashSet<_> = new_view.branches().keys().cloned().collect(); let mut exported_view = old_view.store_view().clone(); @@ -239,11 +240,16 @@ fn export_changes( } for (git_ref_name, new_target) in refs_to_update { git_repo.reference(&git_ref_name, new_target, true, "export from jj")?; + mut_repo.set_git_ref( + git_ref_name, + RefTarget::Normal(CommitId::from_bytes(new_target.as_bytes())), + ); } for git_ref_name in refs_to_delete { if let Ok(mut git_ref) = git_repo.find_reference(&git_ref_name) { git_ref.delete()?; } + mut_repo.remove_git_ref(&git_ref_name); } Ok(exported_view) } @@ -268,7 +274,7 @@ pub fn export_refs( crate::op_store::View::default() }; let last_export_view = View::new(last_export_store_view); - let new_export_store_view = export_changes(&last_export_view, mut_repo.view(), git_repo)?; + let new_export_store_view = export_changes(mut_repo, &last_export_view, git_repo)?; if let Ok(mut last_export_file) = OpenOptions::new() .write(true) .create(true) diff --git a/lib/src/repo.rs b/lib/src/repo.rs index a995aa8dd71..817c3f8ea72 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -770,6 +770,10 @@ impl MutableRepo { self.view_mut().remove_tag(name); } + pub fn get_git_ref(&self, name: &str) -> Option { + self.view.borrow().get_git_ref(name) + } + pub fn set_git_ref(&mut self, name: String, target: RefTarget) { self.view_mut().set_git_ref(name, target); } diff --git a/lib/src/view.rs b/lib/src/view.rs index e9f22259566..7da62587178 100644 --- a/lib/src/view.rs +++ b/lib/src/view.rs @@ -232,6 +232,10 @@ impl View { self.data.tags.remove(name); } + pub fn get_git_ref(&self, name: &str) -> Option { + self.data.git_refs.get(name).cloned() + } + pub fn set_git_ref(&mut self, name: String, target: RefTarget) { self.data.git_refs.insert(name, target); } diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index 70d7f98f31b..2e7fe641b14 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -417,6 +417,10 @@ fn test_export_refs_no_detach() { // Do an initial export to make sure `main` is considered assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(())); + assert_eq!( + mut_repo.get_git_ref("refs/heads/main"), + Some(RefTarget::Normal(jj_id(&commit1))) + ); assert_eq!(git_repo.head().unwrap().name(), Some("refs/heads/main")); assert_eq!( git_repo.find_reference("refs/heads/main").unwrap().target(), @@ -441,6 +445,10 @@ fn test_export_refs_no_op() { // The export should be a no-op since nothing changed on the jj side since last // export assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(())); + assert_eq!( + mut_repo.get_git_ref("refs/heads/main"), + Some(RefTarget::Normal(jj_id(&commit1))) + ); assert_eq!(git_repo.head().unwrap().name(), Some("refs/heads/main")); assert_eq!( git_repo.find_reference("refs/heads/main").unwrap().target(), @@ -477,6 +485,11 @@ fn test_export_refs_branch_changed() { ); mut_repo.remove_local_branch("delete-me"); assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(())); + assert_eq!( + mut_repo.get_git_ref("refs/heads/main"), + Some(RefTarget::Normal(new_commit.id().clone())) + ); + assert_eq!(mut_repo.get_git_ref("refs/heads/delete-me"), None); assert_eq!( git_repo .find_reference("refs/heads/main") @@ -512,6 +525,10 @@ fn test_export_refs_current_branch_changed() { RefTarget::Normal(new_commit.id().clone()), ); assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(())); + assert_eq!( + mut_repo.get_git_ref("refs/heads/main"), + Some(RefTarget::Normal(new_commit.id().clone())) + ); assert_eq!( git_repo .find_reference("refs/heads/main") @@ -543,6 +560,10 @@ fn test_export_refs_unborn_git_branch() { RefTarget::Normal(new_commit.id().clone()), ); assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(())); + assert_eq!( + mut_repo.get_git_ref("refs/heads/main"), + Some(RefTarget::Normal(new_commit.id().clone())) + ); assert_eq!( git_repo .find_reference("refs/heads/main") @@ -579,12 +600,20 @@ fn test_export_import_sequence() { .reference("refs/heads/main", git_id(&commit_a), true, "test") .unwrap(); git::import_refs(mut_repo, &git_repo).unwrap(); + assert_eq!( + mut_repo.get_git_ref("refs/heads/main"), + Some(RefTarget::Normal(commit_a.id().clone())) + ); // Modify the branch in jj to point to B mut_repo.set_local_branch("main".to_string(), RefTarget::Normal(commit_b.id().clone())); // Export the branch to git assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(())); + assert_eq!( + mut_repo.get_git_ref("refs/heads/main"), + Some(RefTarget::Normal(commit_b.id().clone())) + ); // Modify the branch in git to point to C git_repo @@ -593,13 +622,13 @@ fn test_export_import_sequence() { // Import from git git::import_refs(mut_repo, &git_repo).unwrap(); - // TODO: The branch should point to C, it shouldn't be a conflict + assert_eq!( + mut_repo.get_git_ref("refs/heads/main"), + Some(RefTarget::Normal(commit_c.id().clone())) + ); assert_eq!( mut_repo.view().get_local_branch("main"), - Some(RefTarget::Conflict { - removes: vec![commit_a.id().clone()], - adds: vec![commit_b.id().clone(), commit_c.id().clone()], - }) + Some(RefTarget::Normal(commit_c.id().clone())) ); } diff --git a/tests/test_git_colocated.rs b/tests/test_git_colocated.rs index bf04a94d450..e8dd45d5b72 100644 --- a/tests/test_git_colocated.rs +++ b/tests/test_git_colocated.rs @@ -168,10 +168,10 @@ fn test_git_colocated_branches() { "test", ) .unwrap(); - // TODO: Shouldn't be a conflict insta::assert_snapshot!(get_log_output(&test_env, &workspace_root), @r###" - @ 086821b6c35f5fdf07da884b859a14dcf85b5e36 master? - | o 6c0e140886d181602ae7a8e1ac41bc3094842370 master? + Working copy now at: eb08b363bb5e (no description set) + @ eb08b363bb5ef8ee549314260488980d7bbe8f63 + | o 6c0e140886d181602ae7a8e1ac41bc3094842370 master |/ o 230dd059e1b059aefc0da06a2e5a7dbf22362f22 o 0000000000000000000000000000000000000000