merged_tree: propagate backend errors in diff iterator

I want to fix error propagation before I start using async in this
code. This makes the diff iterator propagate errors from reading tree
objects.

Errors include the path and don't stop the iteration. The idea is that
we should be able to show the user an error inline in diff output if
we failed to read a tree. That's going to be especially useful for
backends that can return `BackendError::AccessDenied`. That error
variant doesn't yet exist, but I plan to add it, and use it in
Google's internal backend.

cli/src/cli_util.rs

@@ -1567,7 +1567,8 @@ impl WorkspaceCommandTransaction<'_> {
             Ok(right_tree.id().clone())
         } else {
             let mut tree_builder = MergedTreeBuilder::new(left_tree.id().clone());
-            for (repo_path, _left, right) in left_tree.diff(right_tree, matcher) {
+            for (repo_path, diff) in left_tree.diff(right_tree, matcher) {
+                let (_left, right) = diff?;
                 tree_builder.set_or_remove(repo_path, right);
             }
             Ok(tree_builder.write_tree(self.repo().store())?)

cli/src/commands/mod.rs

@@ -3175,7 +3175,8 @@ fn cmd_restore(
         let matcher = workspace_command.matcher_from_values(&args.paths)?;
         let mut tree_builder = MergedTreeBuilder::new(to_commit.tree_id().clone());
         let to_tree = to_commit.tree()?;
-        for (repo_path, before, _after) in from_tree.diff(&to_tree, matcher.as_ref()) {
+        for (repo_path, diff) in from_tree.diff(&to_tree, matcher.as_ref()) {
+            let (before, _after) = diff?;
             tree_builder.set_or_remove(repo_path, before);
         }
         tree_builder.write_tree(workspace_command.repo().store())?

cli/src/diff_util.rs

@@ -408,8 +408,9 @@ pub fn show_color_words_diff(
 ) -> Result<(), CommandError> {
     let repo = workspace_command.repo();
     formatter.push_label("diff")?;
-    for (path, left_value, right_value) in tree_diff {
+    for (path, diff) in tree_diff {
         let ui_path = workspace_command.format_file_path(&path);
+        let (left_value, right_value) = diff?;
         if left_value.is_absent() {
             let right_content = diff_content(repo, &path, &right_value)?;
             let description = basic_diff_file_type(&right_value);
@@ -686,8 +687,9 @@ pub fn show_git_diff(
 ) -> Result<(), CommandError> {
     let repo = workspace_command.repo();
     formatter.push_label("diff")?;
-    for (path, left_value, right_value) in tree_diff {
+    for (path, diff) in tree_diff {
         let path_string = path.to_internal_file_string();
+        let (left_value, right_value) = diff?;
         if left_value.is_absent() {
             let right_part = git_diff_part(repo, &path, &right_value)?;
             formatter.with_label("file_header", |formatter| {
@@ -746,7 +748,8 @@ pub fn show_diff_summary(
     tree_diff: TreeDiffIterator,
 ) -> io::Result<()> {
     formatter.with_label("diff", |formatter| {
-        for (repo_path, before, after) in tree_diff {
+        for (repo_path, diff) in tree_diff {
+            let (before, after) = diff.unwrap();
             if before.is_present() && after.is_present() {
                 writeln!(
                     formatter.labeled("modified"),
@@ -806,7 +809,8 @@ pub fn show_diff_stat(
     let mut stats: Vec<DiffStat> = vec![];
     let mut max_path_width = 0;
     let mut max_diffs = 0;
-    for (repo_path, left, right) in tree_diff {
+    for (repo_path, diff) in tree_diff {
+        let (left, right) = diff?;
         let path = workspace_command.format_file_path(&repo_path);
         let left_content = diff_content(workspace_command.repo(), &repo_path, &left)?;
         let right_content = diff_content(workspace_command.repo(), &repo_path, &right)?;
@@ -873,7 +877,8 @@ pub fn show_types(
     tree_diff: TreeDiffIterator,
 ) -> io::Result<()> {
     formatter.with_label("diff", |formatter| {
-        for (repo_path, before, after) in tree_diff {
+        for (repo_path, diff) in tree_diff {
+            let (before, after) = diff.unwrap();
             writeln!(
                 formatter.labeled("modified"),
                 "{}{} {}",

cli/src/merge_tools/builtin.rs

@@ -408,10 +408,10 @@ pub fn edit_diff_builtin(
     matcher: &dyn Matcher,
 ) -> Result<MergedTreeId, BuiltinToolError> {
     let store = left_tree.store().clone();
-    let changed_files = left_tree
+    let changed_files: Vec<_> = left_tree
         .diff(right_tree, matcher)
-        .map(|(path, _left, _right)| path)
-        .collect_vec();
+        .map(|(path, diff)| diff.map(|_| path))
+        .try_collect()?;
     let files = make_diff_files(&store, left_tree, right_tree, &changed_files)?;
     let recorder = scm_record::Recorder::new(
         scm_record::RecordState {

cli/src/merge_tools/external.rs

@@ -240,7 +240,7 @@ fn check_out_trees(
 ) -> Result<DiffWorkingCopies, DiffCheckoutError> {
     let changed_files = left_tree
         .diff(right_tree, matcher)
-        .map(|(path, _left, _right)| path)
+        .map(|(path, _diff)| path)
         .collect_vec();
 
     let temp_dir = new_utf8_temp_dir("jj-diff-").map_err(DiffCheckoutError::SetUpDir)?;

lib/src/local_working_copy.rs

@@ -1152,7 +1152,8 @@ impl TreeState {
             removed_files: 0,
             skipped_files: 0,
         };
-        for (path, before, after) in old_tree.diff(new_tree, matcher) {
+        for (path, diff) in old_tree.diff(new_tree, matcher) {
+            let (before, after) = diff?;
             if after.is_absent() {
                 stats.removed_files += 1;
             } else if before.is_absent() {
@@ -1226,7 +1227,8 @@ impl TreeState {
             other => ResetError::InternalBackendError(other),
         })?;
 
-        for (path, _before, after) in old_tree.diff(new_tree, self.sparse_matcher().as_ref()) {
+        for (path, diff) in old_tree.diff(new_tree, self.sparse_matcher().as_ref()) {
+            let (_before, after) = diff?;
             if after.is_absent() {
                 self.file_states.remove(&path);
             } else {

lib/src/merged_tree.rs

@@ -22,6 +22,7 @@ use std::{iter, vec};
 
 use futures::executor::block_on;
 use futures::stream::StreamExt;
+use futures::TryStreamExt;
 use itertools::Itertools;
 
 use crate::backend::{BackendError, BackendResult, ConflictId, MergedTreeId, TreeId, TreeValue};
@@ -337,11 +338,16 @@ impl MergedTree {
 
     /// Collects lists of modified, added, and removed files between this tree
     /// and another tree.
-    pub fn diff_summary(&self, other: &MergedTree, matcher: &dyn Matcher) -> DiffSummary {
+    pub fn diff_summary(
+        &self,
+        other: &MergedTree,
+        matcher: &dyn Matcher,
+    ) -> BackendResult<DiffSummary> {
         let mut modified = vec![];
         let mut added = vec![];
         let mut removed = vec![];
-        for (file, before, after) in self.diff(other, matcher) {
+        for (file, diff) in self.diff(other, matcher) {
+            let (before, after) = diff?;
             if before.is_absent() {
                 added.push(file);
             } else if after.is_absent() {
@@ -353,11 +359,11 @@ impl MergedTree {
         modified.sort();
         added.sort();
         removed.sort();
-        DiffSummary {
+        Ok(DiffSummary {
             modified,
             added,
             removed,
-        }
+        })
     }
 
     /// Merges this tree with `other`, using `base` as base.
@@ -814,29 +820,37 @@ impl<'matcher> TreeDiffIterator<'matcher> {
         Self { stack, matcher }
     }
 
-    async fn single_tree(store: &Arc<Store>, dir: &RepoPath, value: Option<&TreeValue>) -> Tree {
+    async fn single_tree(
+        store: &Arc<Store>,
+        dir: &RepoPath,
+        value: Option<&TreeValue>,
+    ) -> BackendResult<Tree> {
         match value {
-            Some(TreeValue::Tree(tree_id)) => store.get_tree_async(dir, tree_id).await.unwrap(),
-            _ => Tree::null(store.clone(), dir.clone()),
+            Some(TreeValue::Tree(tree_id)) => store.get_tree_async(dir, tree_id).await,
+            _ => Ok(Tree::null(store.clone(), dir.clone())),
         }
     }
 
     /// Gets the given tree if `value` is a tree, otherwise an empty tree.
-    async fn tree(tree: &MergedTree, dir: &RepoPath, values: &MergedTreeValue) -> MergedTree {
+    async fn tree(
+        tree: &MergedTree,
+        dir: &RepoPath,
+        values: &MergedTreeValue,
+    ) -> BackendResult<MergedTree> {
         let trees = if values.is_tree() {
             let builder: MergeBuilder<Tree> = futures::stream::iter(values.iter())
                 .then(|value| Self::single_tree(tree.store(), dir, value.as_ref()))
-                .collect()
-                .await;
+                .try_collect()
+                .await?;
             builder.build()
         } else {
             Merge::resolved(Tree::null(tree.store().clone(), dir.clone()))
         };
         // Maintain the type of tree, so we resolve `TreeValue::Conflict` as necessary
         // in the subtree
         match tree {
-            MergedTree::Legacy(_) => MergedTree::Legacy(trees.into_resolved().unwrap()),
-            MergedTree::Merge(_) => MergedTree::Merge(trees),
+            MergedTree::Legacy(_) => Ok(MergedTree::Legacy(trees.into_resolved().unwrap())),
+            MergedTree::Merge(_) => Ok(MergedTree::Merge(trees)),
         }
     }
 }
@@ -857,7 +871,7 @@ impl TreeDiffDirItem {
 }
 
 impl Iterator for TreeDiffIterator<'_> {
-    type Item = (RepoPath, MergedTreeValue, MergedTreeValue);
+    type Item = (RepoPath, BackendResult<(MergedTreeValue, MergedTreeValue)>);
 
     fn next(&mut self) -> Option<Self::Item> {
         while let Some(top) = self.stack.last_mut() {
@@ -872,7 +886,7 @@ impl Iterator for TreeDiffIterator<'_> {
                 }
                 TreeDiffItem::File(..) => {
                     if let TreeDiffItem::File(name, before, after) = self.stack.pop().unwrap() {
-                        return Some((name, before, after));
+                        return Some((name, Ok((before, after))));
                     } else {
                         unreachable!();
                     }
@@ -889,6 +903,18 @@ impl Iterator for TreeDiffIterator<'_> {
                         let after_tree = Self::tree(dir.tree2.as_ref(), &path, &after);
                         futures::join!(before_tree, after_tree)
                     });
+                    let before_tree = match before_tree {
+                        Ok(tree) => tree,
+                        Err(err) => {
+                            return Some((path, Err(err)));
+                        }
+                    };
+                    let after_tree = match after_tree {
+                        Ok(tree) => tree,
+                        Err(err) => {
+                            return Some((path, Err(err)));
+                        }
+                    };
                     let subdir = TreeDiffDirItem::new(path.clone(), before_tree, after_tree);
                     self.stack.push(TreeDiffItem::Dir(subdir));
                     self.stack.len() - 1
@@ -898,7 +924,7 @@ impl Iterator for TreeDiffIterator<'_> {
             if self.matcher.matches(&path) {
                 if !tree_before && tree_after {
                     if before.is_present() {
-                        return Some((path, before, Merge::absent()));
+                        return Some((path, Ok((before, Merge::absent()))));
                     }
                 } else if tree_before && !tree_after {
                     if after.is_present() {
@@ -908,7 +934,7 @@ impl Iterator for TreeDiffIterator<'_> {
                         );
                     }
                 } else if !tree_before && !tree_after {
-                    return Some((path, before, after));
+                    return Some((path, Ok((before, after))));
                 }
             }
         }

lib/tests/test_commit_builder.rs

@@ -83,7 +83,8 @@ fn test_initial(backend: TestRepoBackend) {
             .root_commit()
             .tree()
             .unwrap()
-            .diff_summary(&commit.tree().unwrap(), &EverythingMatcher),
+            .diff_summary(&commit.tree().unwrap(), &EverythingMatcher)
+            .unwrap(),
         DiffSummary {
             modified: vec![],
             added: vec![dir_file_path, root_file_path],
@@ -167,7 +168,8 @@ fn test_rewrite(backend: TestRepoBackend) {
             .root_commit()
             .tree()
             .unwrap()
-            .diff_summary(&rewritten_commit.tree().unwrap(), &EverythingMatcher),
+            .diff_summary(&rewritten_commit.tree().unwrap(), &EverythingMatcher)
+            .unwrap(),
         DiffSummary {
             modified: vec![],
             added: vec![dir_file_path.clone(), root_file_path],
@@ -178,7 +180,8 @@ fn test_rewrite(backend: TestRepoBackend) {
         initial_commit
             .tree()
             .unwrap()
-            .diff_summary(&rewritten_commit.tree().unwrap(), &EverythingMatcher),
+            .diff_summary(&rewritten_commit.tree().unwrap(), &EverythingMatcher)
+            .unwrap(),
         DiffSummary {
             modified: vec![dir_file_path],
             added: vec![],