From 10c90a50997a80ab7b61125c0cb3dbb13992010c Mon Sep 17 00:00:00 2001
From: Martin von Zweigbergk <martinvonz@google.com>
Date: Fri, 22 Nov 2024 22:40:32 -0800
Subject: [PATCH] merged_tree: propagate errors from conflict iterator

---
 cli/src/cli_util.rs           |  7 +++++--
 lib/src/merged_tree.rs        | 27 ++++++++++++++++-----------
 lib/tests/test_merged_tree.rs | 15 ++++++++++++---
 3 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs
index 4c08f48e2..cff16281e 100644
--- a/cli/src/cli_util.rs
+++ b/cli/src/cli_util.rs
@@ -51,6 +51,7 @@ use clap_complete::ArgValueCandidates;
 use indexmap::IndexMap;
 use indexmap::IndexSet;
 use itertools::Itertools;
+use jj_lib::backend::BackendResult;
 use jj_lib::backend::ChangeId;
 use jj_lib::backend::CommitId;
 use jj_lib::backend::MergedTreeId;
@@ -2467,7 +2468,7 @@ fn update_stale_working_copy(
 
 #[instrument(skip_all)]
 pub fn print_conflicted_paths(
-    conflicts: Vec<(RepoPathBuf, MergedTreeValue)>,
+    conflicts: Vec<(RepoPathBuf, BackendResult<MergedTreeValue>)>,
     formatter: &mut dyn Formatter,
     workspace_command: &WorkspaceCommandHelper,
 ) -> Result<(), CommandError> {
@@ -2481,7 +2482,9 @@ pub fn print_conflicted_paths(
         .map(|p| format!("{:width$}", p, width = max_path_len.min(32) + 3));
 
     for ((_, conflict), formatted_path) in std::iter::zip(conflicts, formatted_paths) {
-        let conflict = conflict.simplify();
+        // TODO: Display the error for the path instead of failing the whole command if
+        // `conflict` is an error?
+        let conflict = conflict?.simplify();
         let sides = conflict.num_sides();
         let n_adds = conflict.adds().flatten().count();
         let deletions = sides - n_adds;
diff --git a/lib/src/merged_tree.rs b/lib/src/merged_tree.rs
index 4ce2abe2e..52c06c40a 100644
--- a/lib/src/merged_tree.rs
+++ b/lib/src/merged_tree.rs
@@ -182,7 +182,7 @@ impl MergedTree {
     /// all sides are trees, so tree/file conflicts will be reported as a single
     /// conflict, not one for each path in the tree.
     // TODO: Restrict this by a matcher (or add a separate method for that).
-    pub fn conflicts(&self) -> impl Iterator<Item = (RepoPathBuf, MergedTreeValue)> {
+    pub fn conflicts(&self) -> impl Iterator<Item = (RepoPathBuf, BackendResult<MergedTreeValue>)> {
         ConflictIterator::new(self)
     }
 
@@ -651,20 +651,25 @@ impl ConflictIterator {
 }
 
 impl Iterator for ConflictIterator {
-    type Item = (RepoPathBuf, MergedTreeValue);
+    type Item = (RepoPathBuf, BackendResult<MergedTreeValue>);
 
     fn next(&mut self) -> Option<Self::Item> {
         while let Some(top) = self.stack.last_mut() {
             if let Some((path, tree_values)) = top.entries.pop() {
-                // TODO: propagate errors
-                if let Some(trees) = tree_values.to_tree_merge(&self.store, &path).unwrap() {
-                    // If all sides are trees or missing, descend into the merged tree
-                    self.stack.push(ConflictsDirItem::from(&trees));
-                } else {
-                    // Otherwise this is a conflict between files, trees, etc. If they could
-                    // be automatically resolved, they should have been when the top-level
-                    // tree conflict was written, so we assume that they can't be.
-                    return Some((path, tree_values));
+                match tree_values.to_tree_merge(&self.store, &path) {
+                    Ok(Some(trees)) => {
+                        // If all sides are trees or missing, descend into the merged tree
+                        self.stack.push(ConflictsDirItem::from(&trees));
+                    }
+                    Ok(None) => {
+                        // Otherwise this is a conflict between files, trees, etc. If they could
+                        // be automatically resolved, they should have been when the top-level
+                        // tree conflict was written, so we assume that they can't be.
+                        return Some((path, Ok(tree_values)));
+                    }
+                    Err(err) => {
+                        return Some((path, Err(err)));
+                    }
                 }
             } else {
                 self.stack.pop();
diff --git a/lib/tests/test_merged_tree.rs b/lib/tests/test_merged_tree.rs
index 8ef86e88b..e11501cc2 100644
--- a/lib/tests/test_merged_tree.rs
+++ b/lib/tests/test_merged_tree.rs
@@ -638,7 +638,10 @@ fn test_conflict_iterator() {
         vec![base1.clone()],
         vec![side1.clone(), side2.clone()],
     ));
-    let conflicts = tree.conflicts().collect_vec();
+    let conflicts = tree
+        .conflicts()
+        .map(|(path, conflict)| (path, conflict.unwrap()))
+        .collect_vec();
     let conflict_at = |path: &RepoPath| {
         Merge::from_removes_adds(
             vec![base1.path_value(path).unwrap()],
@@ -672,7 +675,10 @@ fn test_conflict_iterator() {
 
     // After we resolve conflicts, there are only non-trivial conflicts left
     let tree = tree.resolve().unwrap();
-    let conflicts = tree.conflicts().collect_vec();
+    let conflicts = tree
+        .conflicts()
+        .map(|(path, conflict)| (path, conflict.unwrap()))
+        .collect_vec();
     assert_eq!(
         conflicts,
         vec![
@@ -725,7 +731,10 @@ fn test_conflict_iterator_higher_arity() {
         vec![base1.clone(), base2.clone()],
         vec![side1.clone(), side2.clone(), side3.clone()],
     ));
-    let conflicts = tree.conflicts().collect_vec();
+    let conflicts = tree
+        .conflicts()
+        .map(|(path, conflict)| (path, conflict.unwrap()))
+        .collect_vec();
     let conflict_at = |path: &RepoPath| {
         Merge::from_removes_adds(
             vec![