merged_tree: propagate errors from TreeEntriesIterator

We shouldn't panic if we fail to read a tree from the backend.

cli/src/commands/cat.rs

@@ -14,6 +14,7 @@
 
 use std::io::{self, Write};
 
+use jj_lib::backend::BackendResult;
 use jj_lib::conflicts::{materialize_tree_value, MaterializedTreeValue};
 use jj_lib::fileset::{FilePattern, FilesetExpression};
 use jj_lib::merge::MergedTreeValue;
@@ -64,7 +65,7 @@ pub(crate) fn cmd_cat(
         }
         if !value.is_tree() {
             ui.request_pager();
-            write_tree_entries(ui, &workspace_command, [(path, value)])?;
+            write_tree_entries(ui, &workspace_command, [(path, Ok(value))])?;
             return Ok(());
         }
     }
@@ -95,10 +96,11 @@ fn get_single_path(expression: &FilesetExpression) -> Option<&RepoPath> {
 fn write_tree_entries<P: AsRef<RepoPath>>(
     ui: &Ui,
     workspace_command: &WorkspaceCommandHelper,
-    entries: impl IntoIterator<Item = (P, MergedTreeValue)>,
+    entries: impl IntoIterator<Item = (P, BackendResult<MergedTreeValue>)>,
 ) -> Result<(), CommandError> {
     let repo = workspace_command.repo();
-    for (path, value) in entries {
+    for (path, result) in entries {
+        let value = result?;
         let materialized = materialize_tree_value(repo.store(), path.as_ref(), value).block_on()?;
         match materialized {
             MaterializedTreeValue::Absent => panic!("absent values should be excluded"),

cli/src/commands/chmod.rs

@@ -71,7 +71,8 @@ pub(crate) fn cmd_chmod(
     let mut tx = workspace_command.start_transaction();
     let store = tree.store();
     let mut tree_builder = MergedTreeBuilder::new(commit.tree_id().clone());
-    for (repo_path, tree_value) in tree.entries_matching(matcher.as_ref()) {
+    for (repo_path, result) in tree.entries_matching(matcher.as_ref()) {
+        let tree_value = result?;
         let user_error_with_path = |msg: &str| {
             user_error(format!(
                 "{msg} at '{}'.",

cli/tests/test_chmod_command.rs

@@ -66,7 +66,7 @@ fn test_chmod_regular_conflict() {
     let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "tree"]);
     insta::assert_snapshot!(stdout, 
     @r###"
-    file: Conflicted([Some(File { id: FileId("587be6b4c3f93f93c489c0111bba5596147a26cb"), executable: true }), Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: false }), Some(File { id: FileId("8ba3a16384aacc37d01564b28401755ce8053f51"), executable: false })])
+    file: Ok(Conflicted([Some(File { id: FileId("587be6b4c3f93f93c489c0111bba5596147a26cb"), executable: true }), Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: false }), Some(File { id: FileId("8ba3a16384aacc37d01564b28401755ce8053f51"), executable: false })]))
     "###);
     let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "file"]);
     insta::assert_snapshot!(stdout, 
@@ -85,7 +85,7 @@ fn test_chmod_regular_conflict() {
     let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "tree"]);
     insta::assert_snapshot!(stdout, 
     @r###"
-    file: Conflicted([Some(File { id: FileId("587be6b4c3f93f93c489c0111bba5596147a26cb"), executable: true }), Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: true }), Some(File { id: FileId("8ba3a16384aacc37d01564b28401755ce8053f51"), executable: true })])
+    file: Ok(Conflicted([Some(File { id: FileId("587be6b4c3f93f93c489c0111bba5596147a26cb"), executable: true }), Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: true }), Some(File { id: FileId("8ba3a16384aacc37d01564b28401755ce8053f51"), executable: true })]))
     "###);
     let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "file"]);
     insta::assert_snapshot!(stdout, 
@@ -102,7 +102,7 @@ fn test_chmod_regular_conflict() {
     let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "tree"]);
     insta::assert_snapshot!(stdout, 
     @r###"
-    file: Conflicted([Some(File { id: FileId("587be6b4c3f93f93c489c0111bba5596147a26cb"), executable: false }), Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: false }), Some(File { id: FileId("8ba3a16384aacc37d01564b28401755ce8053f51"), executable: false })])
+    file: Ok(Conflicted([Some(File { id: FileId("587be6b4c3f93f93c489c0111bba5596147a26cb"), executable: false }), Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: false }), Some(File { id: FileId("8ba3a16384aacc37d01564b28401755ce8053f51"), executable: false })]))
     "###);
     let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "file"]);
     insta::assert_snapshot!(stdout, 
@@ -177,7 +177,7 @@ fn test_chmod_file_dir_deletion_conflicts() {
     let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "tree", "-r=file_dir"]);
     insta::assert_snapshot!(stdout,
     @r###"
-    file: Conflicted([Some(File { id: FileId("78981922613b2afb6025042ff6bd878ac1994e85"), executable: false }), Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: false }), Some(Tree(TreeId("133bb38fc4e4bf6b551f1f04db7e48f04cac2877")))])
+    file: Ok(Conflicted([Some(File { id: FileId("78981922613b2afb6025042ff6bd878ac1994e85"), executable: false }), Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: false }), Some(Tree(TreeId("133bb38fc4e4bf6b551f1f04db7e48f04cac2877")))]))
     "###);
     let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "-r=file_dir", "file"]);
     insta::assert_snapshot!(stdout,
@@ -196,7 +196,7 @@ fn test_chmod_file_dir_deletion_conflicts() {
     let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "tree", "-r=file_deletion"]);
     insta::assert_snapshot!(stdout,
     @r###"
-    file: Conflicted([Some(File { id: FileId("78981922613b2afb6025042ff6bd878ac1994e85"), executable: false }), Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: false }), None])
+    file: Ok(Conflicted([Some(File { id: FileId("78981922613b2afb6025042ff6bd878ac1994e85"), executable: false }), Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: false }), None]))
     "###);
     let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "-r=file_deletion", "file"]);
     insta::assert_snapshot!(stdout,
@@ -229,7 +229,7 @@ fn test_chmod_file_dir_deletion_conflicts() {
     let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "tree", "-r=file_deletion"]);
     insta::assert_snapshot!(stdout,
     @r###"
-    file: Conflicted([Some(File { id: FileId("78981922613b2afb6025042ff6bd878ac1994e85"), executable: true }), Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: true }), None])
+    file: Ok(Conflicted([Some(File { id: FileId("78981922613b2afb6025042ff6bd878ac1994e85"), executable: true }), Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: true }), None]))
     "###);
     let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "-r=file_deletion", "file"]);
     insta::assert_snapshot!(stdout,

cli/tests/test_debug_command.rs

@@ -161,22 +161,22 @@ fn test_debug_tree() {
     // Defaults to showing the tree at the current commit
     let stdout = test_env.jj_cmd_success(&workspace_path, &["debug", "tree"]);
     assert_snapshot!(stdout.replace('\\',"/"), @r###"
-    dir/subdir/file1: Resolved(Some(File { id: FileId("498e9b01d79cb8d31cdf0df1a663cc1fcefd9de3"), executable: false }))
-    dir/subdir/file2: Resolved(Some(File { id: FileId("b2496eaffe394cd50a9db4de5787f45f09fd9722"), executable: false }))
+    dir/subdir/file1: Ok(Resolved(Some(File { id: FileId("498e9b01d79cb8d31cdf0df1a663cc1fcefd9de3"), executable: false })))
+    dir/subdir/file2: Ok(Resolved(Some(File { id: FileId("b2496eaffe394cd50a9db4de5787f45f09fd9722"), executable: false })))
     "###
     );
 
     // Can show the tree at another commit
     let stdout = test_env.jj_cmd_success(&workspace_path, &["debug", "tree", "-r@-"]);
     assert_snapshot!(stdout.replace('\\',"/"), @r###"
-    dir/subdir/file1: Resolved(Some(File { id: FileId("498e9b01d79cb8d31cdf0df1a663cc1fcefd9de3"), executable: false }))
+    dir/subdir/file1: Ok(Resolved(Some(File { id: FileId("498e9b01d79cb8d31cdf0df1a663cc1fcefd9de3"), executable: false })))
     "###
     );
 
     // Can filter by paths
     let stdout = test_env.jj_cmd_success(&workspace_path, &["debug", "tree", "dir/subdir/file2"]);
     assert_snapshot!(stdout.replace('\\',"/"), @r###"
-    dir/subdir/file2: Resolved(Some(File { id: FileId("b2496eaffe394cd50a9db4de5787f45f09fd9722"), executable: false }))
+    dir/subdir/file2: Ok(Resolved(Some(File { id: FileId("b2496eaffe394cd50a9db4de5787f45f09fd9722"), executable: false })))
     "###
     );
 
@@ -190,8 +190,8 @@ fn test_debug_tree() {
         ],
     );
     assert_snapshot!(stdout.replace('\\',"/"), @r###"
-    dir/subdir/file1: Resolved(Some(File { id: FileId("498e9b01d79cb8d31cdf0df1a663cc1fcefd9de3"), executable: false }))
-    dir/subdir/file2: Resolved(Some(File { id: FileId("b2496eaffe394cd50a9db4de5787f45f09fd9722"), executable: false }))
+    dir/subdir/file1: Ok(Resolved(Some(File { id: FileId("498e9b01d79cb8d31cdf0df1a663cc1fcefd9de3"), executable: false })))
+    dir/subdir/file2: Ok(Resolved(Some(File { id: FileId("b2496eaffe394cd50a9db4de5787f45f09fd9722"), executable: false })))
     "###
     );
 
@@ -206,8 +206,8 @@ fn test_debug_tree() {
         ],
     );
     assert_snapshot!(stdout.replace('\\',"/"), @r###"
-    dir/subdir/file1: Resolved(Some(File { id: FileId("498e9b01d79cb8d31cdf0df1a663cc1fcefd9de3"), executable: false }))
-    dir/subdir/file2: Resolved(Some(File { id: FileId("b2496eaffe394cd50a9db4de5787f45f09fd9722"), executable: false }))
+    dir/subdir/file1: Ok(Resolved(Some(File { id: FileId("498e9b01d79cb8d31cdf0df1a663cc1fcefd9de3"), executable: false })))
+    dir/subdir/file2: Ok(Resolved(Some(File { id: FileId("b2496eaffe394cd50a9db4de5787f45f09fd9722"), executable: false })))
     "###
     );
 
@@ -223,7 +223,7 @@ fn test_debug_tree() {
         ],
     );
     assert_snapshot!(stdout.replace('\\',"/"), @r###"
-    dir/subdir/file2: Resolved(Some(File { id: FileId("b2496eaffe394cd50a9db4de5787f45f09fd9722"), executable: false }))
+    dir/subdir/file2: Ok(Resolved(Some(File { id: FileId("b2496eaffe394cd50a9db4de5787f45f09fd9722"), executable: false })))
     "###
     );
 }

lib/src/merged_tree.rs

@@ -596,21 +596,21 @@ impl<'matcher> TreeEntriesIterator<'matcher> {
 }
 
 impl Iterator for TreeEntriesIterator<'_> {
-    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, value)) = top.entries.pop() {
                 if value.is_tree() {
-                    let tree_merge = value
-                        .to_tree_merge(top.tree.store(), &path)
-                        .unwrap()
-                        .unwrap();
+                    let tree_merge = match value.to_tree_merge(top.tree.store(), &path) {
+                        Ok(tree_merge) => tree_merge.unwrap(),
+                        Err(err) => return Some((path, Err(err))),
+                    };
                     let merged_tree = MergedTree::Merge(tree_merge);
                     self.stack
                         .push(TreeEntriesDirItem::new(merged_tree, self.matcher));
                 } else {
-                    return Some((path, value));
+                    return Some((path, Ok(value)));
                 }
             } else {
                 self.stack.pop();

lib/tests/test_local_working_copy.rs

@@ -24,7 +24,7 @@ use jj_lib::backend::{MergedTreeId, TreeId, TreeValue};
 use jj_lib::file_util::{check_symlink_support, try_symlink};
 use jj_lib::fsmonitor::FsmonitorKind;
 use jj_lib::local_working_copy::LocalWorkingCopy;
-use jj_lib::merge::Merge;
+use jj_lib::merge::{Merge, MergedTreeValue};
 use jj_lib::merged_tree::{MergedTree, MergedTreeBuilder};
 use jj_lib::op_store::{OperationId, WorkspaceId};
 use jj_lib::repo::{ReadonlyRepo, Repo};
@@ -41,6 +41,12 @@ fn to_owned_path_vec(paths: &[&RepoPath]) -> Vec<RepoPathBuf> {
     paths.iter().map(|&path| path.to_owned()).collect()
 }
 
+fn tree_entries(tree: &MergedTree) -> Vec<(RepoPathBuf, Option<MergedTreeValue>)> {
+    tree.entries()
+        .map(|(path, result)| (path, result.ok()))
+        .collect_vec()
+}
+
 #[test]
 fn test_root() {
     // Test that the working copy is clean and empty after init.
@@ -669,10 +675,7 @@ fn test_gitignores_in_ignored_dir() {
     testutils::write_working_copy_file(&workspace_root, ignored_path, "contents");
 
     let new_tree = test_workspace.snapshot().unwrap();
-    assert_eq!(
-        new_tree.entries().collect_vec(),
-        tree1.entries().collect_vec()
-    );
+    assert_eq!(tree_entries(&new_tree), tree_entries(&tree1));
 
     // The nested .gitignore is ignored even if it's tracked
     let tree2 = create_tree(
@@ -691,10 +694,7 @@ fn test_gitignores_in_ignored_dir() {
     locked_ws.finish(OperationId::from_hex("abc123")).unwrap();
 
     let new_tree = test_workspace.snapshot().unwrap();
-    assert_eq!(
-        new_tree.entries().collect_vec(),
-        tree2.entries().collect_vec()
-    );
+    assert_eq!(tree_entries(&new_tree), tree_entries(&tree2));
 }
 
 #[test]
@@ -822,10 +822,7 @@ fn test_gitignores_ignored_directory_already_tracked() {
         (unchanged_symlink_path, Kind::Symlink, "contents"),
         (modified_symlink_path, Kind::Symlink, "modified"),
     ]);
-    assert_eq!(
-        new_tree.entries().collect_vec(),
-        expected_tree.entries().collect_vec()
-    );
+    assert_eq!(tree_entries(&new_tree), tree_entries(&expected_tree));
 }
 
 #[test]

lib/tests/test_merged_tree.rs

@@ -229,7 +229,7 @@ fn test_from_legacy_tree() {
     let empty_merged_id = MergedTreeId::Merge(empty_merged_id_builder.build());
     let mut tree_builder = MergedTreeBuilder::new(empty_merged_id);
     for (path, value) in merged_tree.entries() {
-        tree_builder.set_or_remove(path, value);
+        tree_builder.set_or_remove(path, value.unwrap());
     }
     let recreated_merged_id = tree_builder.write_tree(store).unwrap();
     assert_eq!(recreated_merged_id, merged_tree.id());
@@ -349,7 +349,10 @@ fn test_path_value_and_entries() {
     );
 
     // Test entries()
-    let actual_entries = merged_tree.entries().collect_vec();
+    let actual_entries = merged_tree
+        .entries()
+        .map(|(path, result)| (path, result.unwrap()))
+        .collect_vec();
     // missing_path, resolved_dir_path, and file_dir_conflict_sub_path should not
     // appear
     let expected_entries = [
@@ -370,6 +373,7 @@ fn test_path_value_and_entries() {
             &modify_delete_path,
             &file_dir_conflict_sub_path,
         ]))
+        .map(|(path, result)| (path, result.unwrap()))
         .collect_vec();
     let expected_entries = [resolved_file_path, modify_delete_path]
         .iter()

lib/testutils/src/lib.rs

@@ -380,8 +380,8 @@ pub fn dump_tree(store: &Arc<Store>, tree_id: &MergedTreeId) -> String {
     )
     .unwrap();
     let tree = store.get_root_tree(tree_id).unwrap();
-    for (path, value) in tree.entries() {
-        match value.into_resolved() {
+    for (path, result) in tree.entries() {
+        match result.unwrap().into_resolved() {
             Ok(Some(TreeValue::File { id, executable: _ })) => {
                 let file_buf = read_file(store, &path, &id);
                 let file_contents = String::from_utf8_lossy(&file_buf);