evolution: rewrite divergence resolution as iterator

This commit rewites the divergence-resolution part of `evolve()` as an
iterator (though not implementing the `Iterator` trait). Iterators are
just much easier to work with: they can easily be stopped, and errors
are easy to propagate. This patch therefore lets us propagate errors
from writing to stdout (typically pipe errors).

lib/src/evolution.rs

@@ -375,18 +375,6 @@ pub struct ReadonlyEvolution {
 pub trait EvolveListener {
     fn orphan_evolved(&mut self, mut_repo: &mut MutableRepo, orphan: &Commit, new_commit: &Commit);
     fn orphan_target_ambiguous(&mut self, mut_repo: &mut MutableRepo, orphan: &Commit);
-    fn divergent_resolved(
-        &mut self,
-        mut_repo: &mut MutableRepo,
-        divergents: &[Commit],
-        resolved: &Commit,
-    );
-    fn divergent_no_common_predecessor(
-        &mut self,
-        mut_repo: &mut MutableRepo,
-        commit1: &Commit,
-        commit2: &Commit,
-    );
 }
 
 impl ReadonlyEvolution {
@@ -463,7 +451,42 @@ impl MutableEvolution {
     }
 }
 
-enum DivergenceResolution {
+pub struct DivergenceResolver<'settings> {
+    user_settings: &'settings UserSettings,
+    remaining_changes: Vec<HashSet<CommitId>>,
+}
+
+impl<'settings> DivergenceResolver<'settings> {
+    pub fn new(user_settings: &'settings UserSettings, mut_repo: &MutableRepo) -> Self {
+        // TODO: Put them in some defined order
+        let divergent_changes: Vec<_> = mut_repo
+            .evolution()
+            .state
+            .non_obsoletes_by_changeid
+            .values()
+            .filter(|non_obsoletes| non_obsoletes.len() > 1)
+            .cloned()
+            .collect();
+        DivergenceResolver {
+            user_settings,
+            remaining_changes: divergent_changes,
+        }
+    }
+
+    pub fn resolve_next(&mut self, mut_repo: &mut MutableRepo) -> Option<DivergenceResolution> {
+        self.remaining_changes.pop().map(|commit_ids| {
+            let store = mut_repo.store().clone();
+            let commits = commit_ids
+                .iter()
+                .map(|id| store.get_commit(&id).unwrap())
+                .collect();
+            evolve_divergent_change(self.user_settings, &store, mut_repo, &commits)
+        })
+    }
+}
+
+#[derive(PartialEq, Eq, Clone, Hash, Debug)]
+pub enum DivergenceResolution {
     Resolved {
         divergents: Vec<Commit>,
         resolved: Commit,
@@ -479,34 +502,6 @@ pub fn evolve(
     mut_repo: &mut MutableRepo,
     listener: &mut dyn EvolveListener,
 ) {
-    let store = mut_repo.store().clone();
-
-    // Resolving divergence can creates new orphans but not vice versa, so resolve
-    // divergence first.
-    let divergent_changes: Vec<_> = mut_repo
-        .evolution()
-        .state
-        .non_obsoletes_by_changeid
-        .values()
-        .filter(|non_obsoletes| non_obsoletes.len() > 1)
-        .cloned()
-        .collect();
-    for commit_ids in divergent_changes {
-        let commits: HashSet<Commit> = commit_ids
-            .iter()
-            .map(|id| store.get_commit(&id).unwrap())
-            .collect();
-        match evolve_divergent_change(user_settings, &store, mut_repo, &commits) {
-            DivergenceResolution::Resolved {
-                divergents,
-                resolved,
-            } => listener.divergent_resolved(mut_repo, &divergents, &resolved),
-            DivergenceResolution::NoCommonPredecessor { .. } => {}
-        }
-    }
-
-    // Don't reuse the state from above, since the divergence-resolution may have
-    // created new orphans, or resolved existing orphans.
     let orphans_topo_order: Vec<_> = mut_repo
         .index()
         .topo_order(mut_repo.evolution().state.orphan_commits.iter())
@@ -514,6 +509,7 @@ pub fn evolve(
         .map(|entry| entry.position())
         .collect();
 
+    let store = mut_repo.store().clone();
     for orphan_pos in orphans_topo_order {
         let orphan_entry = mut_repo.index().entry_by_pos(orphan_pos);
         let mut new_parents = vec![];

lib/tests/test_evolution.rs

@@ -12,9 +12,11 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#![feature(assert_matches)]
+
 use jujube_lib::commit::Commit;
 use jujube_lib::commit_builder::CommitBuilder;
-use jujube_lib::evolution::{evolve, EvolveListener};
+use jujube_lib::evolution::{evolve, DivergenceResolution, DivergenceResolver, EvolveListener};
 use jujube_lib::repo::{MutableRepo, ReadonlyRepo};
 use jujube_lib::repo_path::FileRepoPath;
 use jujube_lib::settings::UserSettings;
@@ -506,14 +508,12 @@ fn test_new_parent_split_forked_pruned(use_git: bool) {
 
 struct RecordingEvolveListener {
     evolved_orphans: Vec<(Commit, Commit)>,
-    evolved_divergents: Vec<(Vec<Commit>, Commit)>,
 }
 
 impl Default for RecordingEvolveListener {
     fn default() -> Self {
         RecordingEvolveListener {
             evolved_orphans: Default::default(),
-            evolved_divergents: Default::default(),
         }
     }
 }
@@ -533,26 +533,6 @@ impl EvolveListener for RecordingEvolveListener {
         // TODO: Record this too and add tests
         panic!("unexpected call to orphan_target_ambiguous");
     }
-
-    fn divergent_resolved(
-        &mut self,
-        _mut_repo: &mut MutableRepo,
-        sources: &[Commit],
-        resolved: &Commit,
-    ) {
-        self.evolved_divergents
-            .push((sources.to_vec(), resolved.clone()));
-    }
-
-    fn divergent_no_common_predecessor(
-        &mut self,
-        _mut_repo: &mut MutableRepo,
-        _commit1: &Commit,
-        _commit2: &Commit,
-    ) {
-        // TODO: Record this too and add tests
-        panic!("unexpected call to divergent_no_common_predecessor");
-    }
 }
 
 #[test_case(false ; "local store")]
@@ -574,7 +554,6 @@ fn test_evolve_orphan(use_git: bool) {
 
     let mut listener = RecordingEvolveListener::default();
     evolve(&settings, mut_repo, &mut listener);
-    assert_eq!(listener.evolved_divergents.len(), 0);
     assert_eq!(listener.evolved_orphans.len(), 2);
     assert_eq!(&listener.evolved_orphans[0].0, &child);
     assert_eq!(&listener.evolved_orphans[0].1.parents(), &vec![rewritten]);
@@ -609,7 +588,6 @@ fn test_evolve_pruned_orphan(use_git: bool) {
 
     let mut listener = RecordingEvolveListener::default();
     evolve(&settings, mut_repo, &mut listener);
-    assert_eq!(listener.evolved_divergents.len(), 0);
     assert_eq!(listener.evolved_orphans.len(), 1);
     assert_eq!(listener.evolved_orphans[0].0.id(), child.id());
 
@@ -636,7 +614,6 @@ fn test_evolve_multiple_orphans(use_git: bool) {
 
     let mut listener = RecordingEvolveListener::default();
     evolve(&settings, mut_repo, &mut listener);
-    assert_eq!(listener.evolved_divergents.len(), 0);
     assert_eq!(listener.evolved_orphans.len(), 3);
     assert_eq!(&listener.evolved_orphans[0].0, &child);
     assert_eq!(&listener.evolved_orphans[0].1.parents(), &vec![rewritten]);
@@ -721,24 +698,26 @@ fn test_evolve_divergent(use_git: bool) {
         .set_committer(later_time)
         .write_to_repo(mut_repo);
 
-    let mut listener = RecordingEvolveListener::default();
+    let mut resolver = DivergenceResolver::new(&settings, mut_repo);
-    evolve(&settings, mut_repo, &mut listener);
+    let resolution = resolver.resolve_next(mut_repo);
-    assert_eq!(listener.evolved_orphans.len(), 0);
+    assert_eq!(resolver.resolve_next(mut_repo), None);
-    assert_eq!(listener.evolved_divergents.len(), 1);
+    assert_matches!(resolution, Some(DivergenceResolution::Resolved { .. }));
-    assert_eq!(
+    if let Some(DivergenceResolution::Resolved {
-        listener.evolved_divergents[0].0,
+        divergents,
-        &[commit6.clone(), commit4.clone()]
+        resolved,
-    );
+    }) = resolution
-    let resolved = listener.evolved_divergents[0].1.clone();
+    {
-    assert_eq!(resolved.predecessors(), &[commit6, commit4]);
+        assert_eq!(divergents, vec![commit6.clone(), commit4.clone()]);
+        assert_eq!(resolved.predecessors(), &[commit6, commit4]);
 
-    let tree = resolved.tree();
+        let tree = resolved.tree();
-    let entries: Vec<_> = tree.entries().collect();
+        let entries: Vec<_> = tree.entries().collect();
-    assert_eq!(entries.len(), 4);
+        assert_eq!(entries.len(), 4);
-    assert_eq!(tree.value("A").unwrap(), tree5.value("A").unwrap());
+        assert_eq!(tree.value("A").unwrap(), tree5.value("A").unwrap());
-    assert_eq!(tree.value("X").unwrap(), tree2.value("X").unwrap());
+        assert_eq!(tree.value("X").unwrap(), tree2.value("X").unwrap());
-    assert_eq!(tree.value("Y").unwrap(), tree4.value("Y").unwrap());
+        assert_eq!(tree.value("Y").unwrap(), tree4.value("Y").unwrap());
-    assert_eq!(tree.value("Z").unwrap(), tree6.value("Z").unwrap());
+        assert_eq!(tree.value("Z").unwrap(), tree6.value("Z").unwrap());
+    }
 
     tx.discard();
 }

src/commands.rs

@@ -31,7 +31,7 @@ use criterion::Criterion;
 use jujube_lib::commit::Commit;
 use jujube_lib::commit_builder::CommitBuilder;
 use jujube_lib::dag_walk::topo_order_reverse;
-use jujube_lib::evolution::{evolve, EvolveListener};
+use jujube_lib::evolution::{evolve, DivergenceResolution, DivergenceResolver, EvolveListener};
 use jujube_lib::files::DiffLine;
 use jujube_lib::git::GitFetchError;
 use jujube_lib::index::HexPrefix;
@@ -1766,6 +1766,41 @@ fn cmd_evolve<'s>(
 ) -> Result<(), CommandError> {
     let mut repo_command = command.repo_helper(ui)?;
 
+    // TODO: This clone is unnecessary. Maybe ui.write() etc should not require a
+    // mutable borrow? But the mutable borrow might be useful for making sure we
+    // have only one Ui instance we write to across threads?
+    let user_settings = ui.settings().clone();
+    let mut tx = repo_command.start_transaction("evolve");
+    let mut_repo = tx.mut_repo();
+    let mut divergence_resolver = DivergenceResolver::new(&user_settings, mut_repo);
+    while let Some(resolution) = divergence_resolver.resolve_next(mut_repo) {
+        match resolution {
+            DivergenceResolution::Resolved {
+                divergents,
+                resolved,
+            } => {
+                ui.write("Resolving divergent commits:\n").unwrap();
+                for source in divergents {
+                    ui.write("  ")?;
+                    ui.write_commit_summary(mut_repo.as_repo_ref(), &source)?;
+                    ui.write("\n")?;
+                }
+                ui.write("Resolved as: ")?;
+                ui.write_commit_summary(mut_repo.as_repo_ref(), &resolved)?;
+                ui.write("\n")?;
+            }
+            DivergenceResolution::NoCommonPredecessor { commit1, commit2 } => {
+                ui.write("Skipping divergent commits with no common predecessor:\n")?;
+                ui.write("  ")?;
+                ui.write_commit_summary(mut_repo.as_repo_ref(), &commit1)?;
+                ui.write("\n")?;
+                ui.write("  ")?;
+                ui.write_commit_summary(mut_repo.as_repo_ref(), &commit2)?;
+                ui.write("\n")?;
+            }
+        }
+    }
+
     struct Listener<'a, 's> {
         ui: &'a mut Ui<'s>,
     }
@@ -1801,56 +1836,8 @@ fn cmd_evolve<'s>(
                 .unwrap();
             self.ui.write("\n").unwrap();
         }
-
-        fn divergent_resolved(
-            &mut self,
-            mut_repo: &mut MutableRepo,
-            sources: &[Commit],
-            resolved: &Commit,
-        ) {
-            self.ui.write("Resolving divergent commits:\n").unwrap();
-            for source in sources {
-                self.ui.write("  ").unwrap();
-                self.ui
-                    .write_commit_summary(mut_repo.as_repo_ref(), &source)
-                    .unwrap();
-                self.ui.write("\n").unwrap();
-            }
-            self.ui.write("Resolved as: ").unwrap();
-            self.ui
-                .write_commit_summary(mut_repo.as_repo_ref(), &resolved)
-                .unwrap();
-            self.ui.write("\n").unwrap();
-        }
-
-        fn divergent_no_common_predecessor(
-            &mut self,
-            mut_repo: &mut MutableRepo,
-            commit1: &Commit,
-            commit2: &Commit,
-        ) {
-            self.ui
-                .write("Skipping divergent commits with no common predecessor:\n")
-                .unwrap();
-            self.ui.write("  ").unwrap();
-            self.ui
-                .write_commit_summary(mut_repo.as_repo_ref(), &commit1)
-                .unwrap();
-            self.ui.write("\n").unwrap();
-            self.ui.write("  ").unwrap();
-            self.ui
-                .write_commit_summary(mut_repo.as_repo_ref(), &commit2)
-                .unwrap();
-            self.ui.write("\n").unwrap();
-        }
     }
-
-    // TODO: This clone is unnecessary. Maybe ui.write() etc should not require a
-    // mutable borrow? But the mutable borrow might be useful for making sure we
-    // have only one Ui instance we write to across threads?
-    let user_settings = ui.settings().clone();
     let mut listener = Listener { ui };
-    let mut tx = repo_command.start_transaction("evolve");
     evolve(&user_settings, tx.mut_repo(), &mut listener);
     repo_command.finish_transaction(ui, tx)?;