From ee9d3271c13493e4e52620da47d6e457c1661d3c Mon Sep 17 00:00:00 2001
From: Martin von Zweigbergk <martinvonz@google.com>
Date: Sun, 5 May 2024 10:03:30 -0700
Subject: [PATCH] cleanup: propagate errors from `Commit::predecessors()`

---
 cli/src/commands/obslog.rs       | 23 ++++++++++++-----------
 lib/src/commit.rs                |  5 ++---
 lib/tests/test_commit_builder.rs |  8 +++-----
 lib/tests/test_init.rs           |  2 +-
 4 files changed, 18 insertions(+), 20 deletions(-)

diff --git a/cli/src/commands/obslog.rs b/cli/src/commands/obslog.rs
index 39f65b6aa8..11fc8f5715 100644
--- a/cli/src/commands/obslog.rs
+++ b/cli/src/commands/obslog.rs
@@ -12,8 +12,9 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use itertools::Itertools;
 use jj_lib::commit::Commit;
-use jj_lib::dag_walk::topo_order_reverse;
+use jj_lib::dag_walk::topo_order_reverse_ok;
 use jj_lib::matchers::EverythingMatcher;
 use jj_lib::rewrite::rebase_to_dest_parent;
 use tracing::instrument;
@@ -102,11 +103,11 @@ pub(crate) fn cmd_obslog(
     let mut formatter = ui.stdout_formatter();
     let formatter = formatter.as_mut();
 
-    let mut commits = topo_order_reverse(
-        vec![start_commit],
+    let mut commits = topo_order_reverse_ok(
+        vec![Ok(start_commit)],
         |commit: &Commit| commit.id().clone(),
-        |commit: &Commit| commit.predecessors(),
-    );
+        |commit: &Commit| commit.predecessors().collect_vec(),
+    )?;
     if let Some(n) = args.limit {
         commits.truncate(n);
     }
@@ -114,8 +115,8 @@ pub(crate) fn cmd_obslog(
         let mut graph = get_graphlog(command.settings(), formatter.raw());
         for commit in commits {
             let mut edges = vec![];
-            for predecessor in &commit.predecessors() {
-                edges.push(Edge::Direct(predecessor.id().clone()));
+            for predecessor in commit.predecessors() {
+                edges.push(Edge::Direct(predecessor?.id().clone()));
             }
             let mut buffer = vec![];
             with_content_format.write_graph_text(
@@ -164,13 +165,13 @@ fn show_predecessor_patch(
     commit: &Commit,
     diff_formats: &[DiffFormat],
 ) -> Result<(), CommandError> {
-    let predecessors = commit.predecessors();
-    let predecessor = match predecessors.first() {
-        Some(predecessor) => predecessor,
+    let mut predecessors = commit.predecessors();
+    let predecessor = match predecessors.next() {
+        Some(predecessor) => predecessor?,
         None => return Ok(()),
     };
     let predecessor_tree =
-        rebase_to_dest_parent(workspace_command.repo().as_ref(), predecessor, commit)?;
+        rebase_to_dest_parent(workspace_command.repo().as_ref(), &predecessor, commit)?;
     let tree = commit.tree()?;
     diff_util::show_diff(
         ui,
diff --git a/lib/src/commit.rs b/lib/src/commit.rs
index d498cef27d..2165f12b6d 100644
--- a/lib/src/commit.rs
+++ b/lib/src/commit.rs
@@ -92,12 +92,11 @@ impl Commit {
         &self.data.predecessors
     }
 
-    pub fn predecessors(&self) -> Vec<Commit> {
+    pub fn predecessors(&self) -> impl Iterator<Item = BackendResult<Commit>> + '_ {
         self.data
             .predecessors
             .iter()
-            .map(|id| self.store.get_commit(id).unwrap())
-            .collect()
+            .map(|id| self.store.get_commit(id))
     }
 
     pub fn tree(&self) -> BackendResult<MergedTree> {
diff --git a/lib/tests/test_commit_builder.rs b/lib/tests/test_commit_builder.rs
index ca487f40d0..7ac52c69b1 100644
--- a/lib/tests/test_commit_builder.rs
+++ b/lib/tests/test_commit_builder.rs
@@ -80,7 +80,7 @@ fn test_initial(backend: TestRepoBackend) {
 
     let parents: Vec<_> = commit.parents().try_collect().unwrap();
     assert_eq!(parents, vec![store.root_commit()]);
-    assert_eq!(commit.predecessors(), vec![]);
+    assert!(commit.predecessors().next().is_none());
     assert_eq!(commit.description(), "description");
     assert_eq!(commit.author(), &author_signature);
     assert_eq!(commit.committer(), &committer_signature);
@@ -156,10 +156,8 @@ fn test_rewrite(backend: TestRepoBackend) {
     tx.commit("test");
     let parents: Vec<_> = rewritten_commit.parents().try_collect().unwrap();
     assert_eq!(parents, vec![store.root_commit()]);
-    assert_eq!(
-        rewritten_commit.predecessors(),
-        vec![initial_commit.clone()]
-    );
+    let predecessors: Vec<_> = rewritten_commit.predecessors().try_collect().unwrap();
+    assert_eq!(predecessors, vec![initial_commit.clone()]);
     assert_eq!(rewritten_commit.author().name, settings.user_name());
     assert_eq!(rewritten_commit.author().email, settings.user_email());
     assert_eq!(
diff --git a/lib/tests/test_init.rs b/lib/tests/test_init.rs
index b95bcb4f89..23ca072215 100644
--- a/lib/tests/test_init.rs
+++ b/lib/tests/test_init.rs
@@ -173,7 +173,7 @@ fn test_init_checkout(backend: TestRepoBackend) {
         wc_commit.store_commit().parents,
         vec![repo.store().root_commit_id().clone()]
     );
-    assert_eq!(wc_commit.predecessors(), vec![]);
+    assert!(wc_commit.predecessors().next().is_none());
     assert_eq!(wc_commit.description(), "");
     assert_eq!(wc_commit.author().name, settings.user_name());
     assert_eq!(wc_commit.author().email, settings.user_email());