cleanup: propagate errors from Commit::parents()

The function now returns an iterator over `Result`s, matching
`Operation::parents()`.

I updated callers to also propagate the error where it was trivial.

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/rebase.rs

@@ -392,8 +392,8 @@ fn rebase_descendants_transaction(
     rebase_options: RebaseOptions,
 ) -> Result<(), CommandError> {
     workspace_command.check_rewritable(old_commits.iter().ids())?;
-    let (skipped_commits, old_commits) = old_commits
-        .iter()
+     let (skipped_commits, old_commits) = old_commits
+         .iter()
         .partition::<Vec<_>, _>(|commit| commit.parent_ids().iter().eq(new_parents.iter().ids()));
     let num_skipped_rebases = skipped_commits.len();
     if num_skipped_rebases > 0 {

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,12 +84,8 @@ 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] {
@@ -113,16 +111,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 +156,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,7 +78,8 @@ fn test_initial(backend: TestRepoBackend) {
     let commit = builder.write().unwrap();
     tx.commit("test");
 
-    assert_eq!(commit.parents(), vec![store.root_commit()]);
+    let parents: Vec<_> = commit.parents().try_collect().unwrap();
+    assert_eq!(parents, vec![store.root_commit()]);
     assert_eq!(commit.predecessors(), vec![]);
     assert_eq!(commit.description(), "description");
     assert_eq!(commit.author(), &author_signature);
@@ -152,7 +154,8 @@ 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()]);
+    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()]

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