git.rs: properly abandon commits from moved/deleted branches on remote (

#864)

This bug concerns the way `import_refs` that gets called by `fetch` computes
the heads that should be visible after the import.

Previously, the list of such heads was computed *before* local branches were
updated based on changes to the remote branches. So, commits that should have
been abandoned based on this update of the local branches weren't properly
abandoned.

Now, `import_refs` tracks the heads that need to be visible because of some ref
in a mapping keyed by the ref. If the ref moves or is deleted, the
corresponding heads are updated.

Fixes #864

CHANGELOG.md

@@ -115,6 +115,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   the previously checked-out anonymous branch.
   [#1042](https://github.com/martinvonz/jj/issues/1042).
 
+* `jj git fetch` in a colocated repo now abandons branches deleted on the
+  remote, just like in a non-colocated repo.
+  [#864](https://github.com/martinvonz/jj/issues/864)
+
 ## [0.7.0] - 2023-02-16
 
 ### Breaking changes

docs/git-compatibility.md

@@ -98,7 +98,7 @@ commands and readonly Git commands. It's also useful when tools (e.g. build
 tools) expect a Git repo to be present.
 
 There are some bugs and surprising behavior related to `jj undo` in this mode,
-such as #864 and #922.
+such as #922.
 
 ## Creating a repo by cloning a Git repo
 

lib/src/git.rs

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use std::collections::{BTreeMap, HashSet};
+use std::collections::{BTreeMap, HashMap, HashSet};
 use std::default::Default;
 use std::path::PathBuf;
 
@@ -83,12 +83,12 @@ pub fn import_some_refs(
     let store = mut_repo.store().clone();
     let mut jj_view_git_refs = mut_repo.view().git_refs().clone();
     let mut old_git_heads = vec![];
-    let mut new_git_heads = HashSet::new();
+    let mut new_git_heads = HashMap::new();
     for (ref_name, old_target) in &jj_view_git_refs {
         if git_ref_filter(ref_name) {
             old_git_heads.extend(old_target.adds());
         } else {
-            new_git_heads.extend(old_target.adds());
+            new_git_heads.insert(ref_name.to_string(), old_target.adds().clone());
         }
     }
 
@@ -103,7 +103,7 @@ pub fn import_some_refs(
         // 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());
+        new_git_heads.insert("HEAD".to_string(), vec![head_commit_id.clone()]);
         prevent_gc(git_repo, &head_commit_id);
         mut_repo.add_head(&head_commit);
         mut_repo.set_git_head(RefTarget::Normal(head_commit_id));
@@ -136,7 +136,7 @@ pub fn import_some_refs(
             }
         };
         let id = CommitId::from_bytes(git_commit.id().as_bytes());
-        new_git_heads.insert(id.clone());
+        new_git_heads.insert(full_name.to_string(), vec![id.clone()]);
         if !git_ref_filter(&full_name) {
             continue;
         }
@@ -169,28 +169,35 @@ pub fn import_some_refs(
             }
             if let RefName::RemoteBranch { branch, remote: _ } = ref_name {
                 mut_repo.merge_single_ref(
-                    &RefName::LocalBranch(branch),
+                    &RefName::LocalBranch(branch.clone()),
                     old_git_target.as_ref(),
                     new_git_target.as_ref(),
                 );
+                match mut_repo.get_local_branch(&branch) {
+                    None => new_git_heads.remove(&format!("refs/heads/{branch}")),
+                    Some(target) => {
+                        new_git_heads.insert(format!("refs/heads/{branch}"), target.adds())
+                    }
+                };
             }
         }
     }
 
     // Find commits that are no longer referenced in the git repo and abandon them
     // in jj as well. We must remove non-existing commits from new_git_heads, as
     // they could have come from branches which were never fetched.
-    let new_git_heads = new_git_heads
-        .into_iter()
-        .filter(|id| mut_repo.index().has_id(id))
-        .collect_vec();
+    let mut new_git_heads_set = HashSet::new();
+    for heads_for_ref in new_git_heads.into_values() {
+        new_git_heads_set.extend(heads_for_ref.into_iter());
+    }
+    new_git_heads_set.retain(|id| mut_repo.index().has_id(id));
     // We could use mut_repo.record_rewrites() here but we know we only need to care
     // about abandoned commits for now. We may want to change this if we ever
     // add a way of preserving change IDs across rewrites by `git` (e.g. by
     // putting them in the commit message).
     let abandoned_commits = mut_repo
         .index()
-        .walk_revs(&old_git_heads, &new_git_heads)
+        .walk_revs(&old_git_heads, &new_git_heads_set.into_iter().collect_vec())
         .map(|entry| entry.commit_id())
         .collect_vec();
     let root_commit_id = mut_repo.store().root_commit_id().clone();

lib/tests/test_git.rs

@@ -480,11 +480,7 @@ fn test_import_refs_reimport_with_deleted_remote_ref() {
             jj_id(&commit_main),
             // Neither commit_remote_only nor commit_remote_and_local should be
             // listed as a head. commit_remote_only was never affected by #864,
-            // but commit_remote_and_local is.
-            // BUG: commit_remote_and_local is still listed as a head (#864)
-            // even though the feature-remote branch was deleted as we'll see
-            // below.
-            jj_id(&commit_remote_and_local),
+            // but commit_remote_and_local was.
     };
     assert_eq!(*view.heads(), expected_heads);
 }
@@ -601,8 +597,7 @@ fn test_import_refs_reimport_with_moved_remote_ref() {
             jj_id(&new_commit_remote_only),
             // Neither commit_remote_only nor commit_remote_and_local should be
             // listed as a head. commit_remote_only was never affected by #864,
-            // but commit_remote_and_local is.
-            jj_id(&commit_remote_and_local),
+            // but commit_remote_and_local was.
     };
     assert_eq!(*view.heads(), expected_heads);
 }

tests/test_git_colocated.rs

@@ -265,15 +265,12 @@ fn test_git_colocated_fetch_deleted_or_moved_branch() {
     test_env.jj_cmd_success(&origin_path, &["describe", "C_to_move", "-m", "moved C"]);
     let stdout = test_env.jj_cmd_success(&clone_path, &["git", "fetch"]);
     insta::assert_snapshot!(stdout, @"");
-    // TODO: 929e and 8d4e should have been abandoned (#864)
+    // 929e and 8d4e are abandoned, as the corresponding branches were deleted or
+    // moved on the remote (#864)
     insta::assert_snapshot!(get_log_output(&test_env, &clone_path), @r###"
     ◉  04fd29df05638156b20044b3b6136b42abcb09ab C_to_move
     │ @  0335878796213c3a701f1c9c34dcae242bee4131
     ├─╯
-    │ ◉  8d4e006fd63547965fbc3a26556a9aa531076d32
-    ├─╯
-    │ ◉  929e298ae9edf969b405a304c75c10457c47d52c
-    ├─╯
     ◉  a86754f975f953fa25da4265764adc0c62e9ce6b A master HEAD@git
     ◉  0000000000000000000000000000000000000000
     "###);