Propagate errors from Commit::parents() and Commit::predecessors()

cli/src/cli_util.rs

@@ -1244,6 +1244,7 @@ See https://github.com/martinvonz/jj/blob/main/docs/working-copy.md#stale-workin
                 formatter.with_label("working_copy", |fmt| template.format(new_commit, fmt))?;
                 writeln!(formatter)?;
                 for parent in new_commit.parents() {
+                    let parent = parent?;
                     //                "Working copy now at: "
                     write!(formatter, "Parent commit      : ")?;
                     template.format(&parent, formatter.as_mut())?;

cli/src/commands/diffedit.rs

@@ -14,6 +14,7 @@
 
 use std::io::Write;
 
+use itertools::Itertools;
 use jj_lib::matchers::EverythingMatcher;
 use jj_lib::object_id::ObjectId;
 use jj_lib::rewrite::merge_commit_trees;
@@ -81,7 +82,7 @@ pub(crate) fn cmd_diffedit(
     } else {
         target_commit = workspace_command
             .resolve_single_rev(args.revision.as_ref().unwrap_or(&RevisionArg::AT))?;
-        base_commits = target_commit.parents();
+        base_commits = target_commit.parents().try_collect()?;
         diff_description = "The diff initially shows the commit's changes.".to_string();
     };
     workspace_command.check_rewritable([target_commit.id()])?;

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,20 +103,20 @@ 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);
     }
     if !args.no_graph {
         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,

cli/src/commands/rebase.rs

@@ -394,7 +394,7 @@ fn rebase_descendants_transaction(
     workspace_command.check_rewritable(old_commits.iter().ids())?;
     let (skipped_commits, old_commits) = old_commits
         .iter()
-        .partition::<Vec<_>, _>(|commit| commit.parents() == new_parents);
+        .partition::<Vec<_>, _>(|commit| commit.parent_ids().iter().eq(new_parents.iter().ids()));
     let num_skipped_rebases = skipped_commits.len();
     if num_skipped_rebases > 0 {
         writeln!(

cli/src/commands/squash.rs

@@ -106,7 +106,7 @@ pub(crate) fn cmd_squash(
     } else {
         let source = workspace_command
             .resolve_single_rev(args.revision.as_ref().unwrap_or(&RevisionArg::AT))?;
-        let mut parents = source.parents();
+        let mut parents: Vec<_> = source.parents().try_collect()?;
         if parents.len() != 1 {
             return Err(user_error("Cannot squash merge commits"));
         }

cli/src/commands/status.rs

@@ -87,6 +87,7 @@ pub(crate) fn cmd_status(
         formatter.with_label("working_copy", |fmt| template.format(wc_commit, fmt))?;
         writeln!(formatter)?;
         for parent in wc_commit.parents() {
+            let parent = parent?;
             write!(formatter, "Parent commit: ")?;
             template.format(&parent, formatter)?;
             writeln!(formatter)?;

cli/src/commands/unsquash.rs

@@ -60,7 +60,7 @@ pub(crate) fn cmd_unsquash(
     if commit.parent_ids().len() > 1 {
         return Err(user_error("Cannot unsquash merge commits"));
     }
-    let parent = commit.parents().pop().unwrap();
+    let parent = commit.parents().next().unwrap()?;
     workspace_command.check_rewritable([parent.id()])?;
     let interactive_editor = if args.tool.is_some() || args.interactive {
         Some(workspace_command.diff_editor(ui, args.tool.as_deref())?)

cli/src/commands/workspace.rs

@@ -198,7 +198,11 @@ fn cmd_workspace_add(
             .view()
             .get_wc_commit_id(old_workspace_command.workspace_id())
         {
-            tx.repo().store().get_commit(old_wc_commit_id)?.parents()
+            tx.repo()
+                .store()
+                .get_commit(old_wc_commit_id)?
+                .parents()
+                .try_collect()?
         } else {
             vec![tx.repo().store().root_commit()]
         }

cli/src/commit_templater.rs

@@ -463,7 +463,7 @@ fn builtin_commit_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, Comm
         "parents",
         |_language, _build_ctx, self_property, function| {
             template_parser::expect_no_arguments(function)?;
-            let out_property = self_property.map(|commit| commit.parents());
+            let out_property = self_property.map(|commit| commit.parents().try_collect().unwrap());
             Ok(L::wrap_commit_list(out_property))
         },
     );

lib/src/commit.rs

@@ -19,6 +19,8 @@ use std::fmt::{Debug, Error, Formatter};
 use std::hash::{Hash, Hasher};
 use std::sync::Arc;
 
+use itertools::Itertools;
+
 use crate::backend::{self, BackendResult, ChangeId, CommitId, MergedTreeId, Signature};
 use crate::merged_tree::MergedTree;
 use crate::repo::Repo;
@@ -82,24 +84,19 @@ impl Commit {
         &self.data.parents
     }
 
-    pub fn parents(&self) -> Vec<Commit> {
-        self.data
-            .parents
-            .iter()
-            .map(|id| self.store.get_commit(id).unwrap())
-            .collect()
+    pub fn parents(&self) -> impl Iterator<Item = BackendResult<Commit>> + '_ {
+        self.data.parents.iter().map(|id| self.store.get_commit(id))
     }
 
     pub fn predecessor_ids(&self) -> &[CommitId] {
         &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> {
@@ -113,16 +110,18 @@ impl Commit {
     /// Return the parent tree, merging the parent trees if there are multiple
     /// parents.
     pub fn parent_tree(&self, repo: &dyn Repo) -> BackendResult<MergedTree> {
-        merge_commit_trees(repo, &self.parents())
+        let parents: Vec<_> = self.parents().try_collect()?;
+        merge_commit_trees(repo, &parents)
     }
 
     /// Returns whether commit's content is empty. Commit description is not
     /// taken into consideration.
     pub fn is_empty(&self, repo: &dyn Repo) -> BackendResult<bool> {
-        if let [parent] = &self.parents()[..] {
+        let parents: Vec<_> = self.parents().try_collect()?;
+        if let [parent] = &parents[..] {
             return Ok(parent.tree_id() == self.tree_id());
         }
-        let parent_tree = self.parent_tree(repo)?;
+        let parent_tree = merge_commit_trees(repo, &parents)?;
         Ok(*self.tree_id() == parent_tree.id())
     }
 
@@ -156,13 +155,14 @@ impl Commit {
 
     /// A commit is discardable if it has one parent, no change from its
     /// parent, and an empty description.
-    pub fn is_discardable(&self) -> bool {
+    pub fn is_discardable(&self) -> BackendResult<bool> {
         if self.description().is_empty() {
-            if let [parent_commit] = &*self.parents() {
-                return self.tree_id() == parent_commit.tree_id();
+            let parents: Vec<_> = self.parents().try_collect()?;
+            if let [parent_commit] = &*parents {
+                return Ok(self.tree_id() == parent_commit.tree_id());
             }
         }
-        false
+        Ok(false)
     }
 
     /// A quick way to just check if a signature is present.

lib/src/default_index/revset_engine.rs

@@ -1068,7 +1068,7 @@ fn has_diff_from_parent(
     matcher: &dyn Matcher,
 ) -> bool {
     let commit = store.get_commit(&entry.commit_id()).unwrap();
-    let parents = commit.parents();
+    let parents: Vec<_> = commit.parents().try_collect().unwrap();
     if let [parent] = parents.as_slice() {
         // Fast path: no need to load the root tree
         let unchanged = commit.tree_id() == parent.tree_id();

lib/src/repo.rs

@@ -1158,6 +1158,10 @@ impl MutableRepo {
                 visited.insert(commit.id().clone());
                 let mut dependents = vec![];
                 for parent in commit.parents() {
+                    let Ok(parent) = parent else {
+                        dependents.push(parent);
+                        continue;
+                    };
                     if let Some(rewrite) = self.parent_mapping.get(parent.id()) {
                         for target in rewrite.new_parent_ids() {
                             if to_visit_set.contains(target) && !visited.contains(target) {
@@ -1347,7 +1351,7 @@ impl MutableRepo {
                 .store()
                 .get_commit(&wc_commit_id)
                 .map_err(EditCommitError::WorkingCopyCommitNotFound)?;
-            if wc_commit.is_discardable()
+            if wc_commit.is_discardable()?
                 && self
                     .view
                     .with_ref(|v| local_branch_target_ids(v).all(|id| id != wc_commit.id()))
@@ -1762,6 +1766,8 @@ pub enum EditCommitError {
     WorkingCopyCommitNotFound(#[source] BackendError),
     #[error("Cannot rewrite the root commit")]
     RewriteRootCommit,
+    #[error(transparent)]
+    BackendError(#[from] BackendError),
 }
 
 /// Error from attempts to check out a commit

lib/src/rewrite.rs

@@ -199,7 +199,7 @@ impl<'repo> CommitRewriter<'repo> {
         settings: &UserSettings,
         empty: EmptyBehaviour,
     ) -> BackendResult<Option<CommitBuilder<'repo>>> {
-        let old_parents = self.old_commit.parents();
+        let old_parents: Vec<_> = self.old_commit.parents().try_collect()?;
         let old_parent_trees = old_parents
             .iter()
             .map(|parent| parent.tree_id().clone())

lib/tests/test_commit_builder.rs

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use itertools::Itertools;
 use jj_lib::backend::{ChangeId, MillisSinceEpoch, Signature, Timestamp};
 use jj_lib::matchers::EverythingMatcher;
 use jj_lib::merged_tree::DiffSummary;
@@ -77,8 +78,9 @@ fn test_initial(backend: TestRepoBackend) {
     let commit = builder.write().unwrap();
     tx.commit("test");
 
-    assert_eq!(commit.parents(), vec![store.root_commit()]);
-    assert_eq!(commit.predecessors(), vec![]);
+    let parents: Vec<_> = commit.parents().try_collect().unwrap();
+    assert_eq!(parents, vec![store.root_commit()]);
+    assert!(commit.predecessors().next().is_none());
     assert_eq!(commit.description(), "description");
     assert_eq!(commit.author(), &author_signature);
     assert_eq!(commit.committer(), &committer_signature);
@@ -152,11 +154,10 @@ fn test_rewrite(backend: TestRepoBackend) {
         .unwrap();
     tx.mut_repo().rebase_descendants(&settings).unwrap();
     tx.commit("test");
-    assert_eq!(rewritten_commit.parents(), vec![store.root_commit()]);
-    assert_eq!(
-        rewritten_commit.predecessors(),
-        vec![initial_commit.clone()]
-    );
+    let parents: Vec<_> = rewritten_commit.parents().try_collect().unwrap();
+    assert_eq!(parents, vec![store.root_commit()]);
+    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!(

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

lib/tests/test_mut_repo.rs

@@ -56,8 +56,8 @@ fn test_checkout() {
         .check_out(ws_id.clone(), &settings, &wc_commit_parent)
         .unwrap();
     assert_eq!(wc_commit.tree_id(), wc_commit_parent.tree_id());
-    assert_eq!(wc_commit.parents().len(), 1);
-    assert_eq!(wc_commit.parents()[0].id(), wc_commit_parent.id());
+    assert_eq!(wc_commit.parent_ids().len(), 1);
+    assert_eq!(&wc_commit.parent_ids()[0], wc_commit_parent.id());
     let repo = tx.commit("test");
     assert_eq!(repo.view().get_wc_commit_id(&ws_id), Some(wc_commit.id()));
 }