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

@@ -47,6 +47,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

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<crate::op_store::View, GitExportError> {
+    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)
 }
@@ -267,7 +273,7 @@ pub fn export_refs(
             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)

lib/src/repo.rs

@@ -749,6 +749,10 @@ impl MutableRepo {
         self.view_mut().remove_tag(name);
     }
 
+    pub fn get_git_ref(&self, name: &str) -> Option<RefTarget> {
+        self.view.with_ref(|v| v.get_git_ref(name))
+    }
+
     pub fn set_git_ref(&mut self, name: String, target: RefTarget) {
         self.view_mut().set_git_ref(name, target);
     }

lib/src/view.rs

@@ -232,6 +232,10 @@ impl View {
         self.data.tags.remove(name);
     }
 
+    pub fn get_git_ref(&self, name: &str) -> Option<RefTarget> {
+        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);
     }

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()))
     );
 }
 

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