git: when importing refs, abandon commits that were abandoned in git

Now that I'm using GitHub PRs instead of pushing directly to the main
branch, it's quite annoying to have to abandon the old commits after
GitHub rebases them. This patch makes it so we compare the remote's
previous heads to the new heads and abandons any commits that were
removed on the remote. As usual, that means that descendants get
rebased onto the closest remaining commit.

This is half of #241. The other half is to detect rewritten branches
and rebase on top.

CHANGELOG.md

@@ -50,6 +50,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   `$SSH_AUTH_SOCK` is set). This should help at least macOS users where
   ssh-agent is launched by default and only `$SSH_AUTH_SOCK` is set.
 
+* When commits were abandoned from the remote (because branches pointing to them
+  were deleted), your local commits that were based on them will now be rebased
+  off of the abandoned commits.
+
 ### Fixed bugs
 
 * When rebasing a conflict where one side modified a file and the other side

lib/src/git.rs

@@ -57,9 +57,16 @@ pub fn import_refs(
     git_repo: &git2::Repository,
 ) -> Result<(), GitImportError> {
     let store = mut_repo.store().clone();
-    let git_refs = git_repo.references()?;
     let mut existing_git_refs = mut_repo.view().git_refs().clone();
+    let mut old_git_heads = HashSet::new();
+    for old_target in existing_git_refs.values() {
+        for old_head in old_target.adds() {
+            old_git_heads.insert(old_head);
+        }
+    }
+    let mut new_git_heads = HashSet::new();
     let mut changed_git_refs = BTreeMap::new();
+    let git_refs = git_repo.references()?;
     for git_ref in git_refs {
         let git_ref = git_ref?;
         if !(git_ref.is_tag() || git_ref.is_branch() || git_ref.is_remote())
@@ -83,6 +90,7 @@ pub fn import_refs(
             }
         };
         let id = CommitId::from_bytes(git_commit.id().as_bytes());
+        new_git_heads.insert(id.clone());
         // TODO: Make it configurable which remotes are publishing and update public
         // heads here.
         mut_repo.set_git_ref(full_name.clone(), RefTarget::Normal(id.clone()));
@@ -96,10 +104,6 @@ pub fn import_refs(
     }
     for (full_name, target) in existing_git_refs {
         mut_repo.remove_git_ref(&full_name);
-        // TODO: We should probably also remove heads pointing to the same
-        // commits and commits no longer reachable from other refs.
-        // If the underlying git repo has a branch that gets rewritten, we
-        // should probably not keep the commits it used to point to.
         changed_git_refs.insert(full_name, (Some(target), None));
     }
     for (full_name, (old_git_target, new_git_target)) in changed_git_refs {
@@ -117,6 +121,11 @@ pub fn import_refs(
             }
         }
     }
+    let old_git_heads = old_git_heads.into_iter().collect_vec();
+    let new_git_heads = new_git_heads.into_iter().collect_vec();
+    // TODO: This is only going to find abandoned commits, so explicitly mark the
+    // removed commits as abandoned instead
+    mut_repo.record_rewrites(&old_git_heads, &new_git_heads);
     // 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

lib/src/repo.rs

@@ -864,7 +864,7 @@ impl MutableRepo {
 
     /// Finds and records commits that were rewritten or abandoned between
     /// `old_heads` and `new_heads`.
-    fn record_rewrites(&mut self, old_heads: &[CommitId], new_heads: &[CommitId]) {
+    pub fn record_rewrites(&mut self, old_heads: &[CommitId], new_heads: &[CommitId]) {
         let mut removed_changes: HashMap<ChangeId, Vec<CommitId>> = HashMap::new();
         for removed in self.index.walk_revs(old_heads, new_heads) {
             removed_changes

lib/tests/test_git.rs

@@ -74,6 +74,7 @@ fn test_import_refs() {
     let git_repo = repo.store().git_repo().unwrap();
     let mut tx = repo.start_transaction("test");
     git::import_refs(tx.mut_repo(), &git_repo).unwrap();
+    tx.mut_repo().rebase_descendants(&settings);
     let repo = tx.commit();
     let view = repo.view();
 
@@ -150,7 +151,7 @@ fn test_import_refs_reimport() {
     let commit1 = empty_git_commit(&git_repo, "refs/heads/main", &[]);
     git_ref(&git_repo, "refs/remotes/origin/main", commit1.id());
     let commit2 = empty_git_commit(&git_repo, "refs/heads/main", &[&commit1]);
-    let commit3 = empty_git_commit(&git_repo, "refs/heads/feature1", &[&commit2]);
+    let _commit3 = empty_git_commit(&git_repo, "refs/heads/feature1", &[&commit2]);
     let commit4 = empty_git_commit(&git_repo, "refs/heads/feature2", &[&commit2]);
     let pgp_key_oid = git_repo.blob(b"my PGP key").unwrap();
     git_repo
@@ -159,6 +160,7 @@ fn test_import_refs_reimport() {
 
     let mut tx = repo.start_transaction("test");
     git::import_refs(tx.mut_repo(), &git_repo).unwrap();
+    tx.mut_repo().rebase_descendants(&settings);
     let repo = tx.commit();
 
     // Delete feature1 and rewrite feature2
@@ -179,13 +181,11 @@ fn test_import_refs_reimport() {
 
     let mut tx = repo.start_transaction("test");
     git::import_refs(tx.mut_repo(), &git_repo).unwrap();
+    tx.mut_repo().rebase_descendants(&settings);
     let repo = tx.commit();
 
     let view = repo.view();
-    // TODO: commit3 and commit4 should probably be removed
     let expected_heads = hashset! {
-            commit_id(&commit3),
-            commit_id(&commit4),
             commit_id(&commit5),
             commit6.id().clone(),
     };
@@ -245,13 +245,15 @@ fn test_import_refs_reimport_head_removed() {
     let commit = empty_git_commit(&git_repo, "refs/heads/main", &[]);
     let mut tx = repo.start_transaction("test");
     git::import_refs(tx.mut_repo(), &git_repo).unwrap();
+    tx.mut_repo().rebase_descendants(&settings);
     let commit_id = CommitId::from_bytes(commit.id().as_bytes());
     // Test the setup
     assert!(tx.mut_repo().view().heads().contains(&commit_id));
 
     // Remove the head and re-import
     tx.mut_repo().remove_head(&commit_id);
     git::import_refs(tx.mut_repo(), &git_repo).unwrap();
+    tx.mut_repo().rebase_descendants(&settings);
     assert!(!tx.mut_repo().view().heads().contains(&commit_id));
 }
 
@@ -299,6 +301,7 @@ fn test_import_refs_empty_git_repo() {
     let heads_before = test_data.repo.view().heads().clone();
     let mut tx = test_data.repo.start_transaction("test");
     git::import_refs(tx.mut_repo(), &test_data.git_repo).unwrap();
+    tx.mut_repo().rebase_descendants(&test_data.settings);
     let repo = tx.commit();
     assert_eq!(*repo.view().heads(), heads_before);
     assert_eq!(repo.view().branches().len(), 0);
@@ -323,6 +326,7 @@ fn test_import_refs_detached_head() {
 
     let mut tx = test_data.repo.start_transaction("test");
     git::import_refs(tx.mut_repo(), &test_data.git_repo).unwrap();
+    tx.mut_repo().rebase_descendants(&test_data.settings);
     let repo = tx.commit();
 
     let expected_heads = hashset! {
@@ -345,6 +349,7 @@ fn test_export_refs_initial() {
     git_repo.set_head("refs/heads/main").unwrap();
     let mut tx = test_data.repo.start_transaction("test");
     git::import_refs(tx.mut_repo(), &git_repo).unwrap();
+    tx.mut_repo().rebase_descendants(&test_data.settings);
     test_data.repo = tx.commit();
 
     // The first export shouldn't do anything
@@ -366,6 +371,7 @@ fn test_export_refs_no_op() {
 
     let mut tx = test_data.repo.start_transaction("test");
     git::import_refs(tx.mut_repo(), &git_repo).unwrap();
+    tx.mut_repo().rebase_descendants(&test_data.settings);
     test_data.repo = tx.commit();
 
     assert_eq!(git::export_refs(&test_data.repo, &git_repo), Ok(()));
@@ -392,6 +398,7 @@ fn test_export_refs_branch_changed() {
 
     let mut tx = test_data.repo.start_transaction("test");
     git::import_refs(tx.mut_repo(), &git_repo).unwrap();
+    tx.mut_repo().rebase_descendants(&test_data.settings);
     test_data.repo = tx.commit();
 
     assert_eq!(git::export_refs(&test_data.repo, &git_repo), Ok(()));
@@ -427,6 +434,7 @@ fn test_export_refs_current_branch_changed() {
     git_repo.set_head("refs/heads/main").unwrap();
     let mut tx = test_data.repo.start_transaction("test");
     git::import_refs(tx.mut_repo(), &git_repo).unwrap();
+    tx.mut_repo().rebase_descendants(&test_data.settings);
     test_data.repo = tx.commit();
 
     assert_eq!(git::export_refs(&test_data.repo, &git_repo), Ok(()));
@@ -460,6 +468,7 @@ fn test_export_refs_unborn_git_branch() {
     git_repo.set_head("refs/heads/main").unwrap();
     let mut tx = test_data.repo.start_transaction("test");
     git::import_refs(tx.mut_repo(), &git_repo).unwrap();
+    tx.mut_repo().rebase_descendants(&test_data.settings);
     test_data.repo = tx.commit();
 
     assert_eq!(git::export_refs(&test_data.repo, &git_repo), Ok(()));