tree_builder: propagate errors from write_tree()

lib/src/merged_tree.rs

@@ -131,7 +131,7 @@ impl MergedTree {
         let new_trees: Vec<_> = tree_builders
             .into_iter()
             .map(|builder| {
-                let tree_id = builder.write_tree();
+                let tree_id = builder.write_tree()?;
                 store.get_tree(RepoPath::root(), &tree_id)
             })
             .try_collect()?;
@@ -1216,7 +1216,7 @@ impl MergedTreeBuilder {
                         }
                     }
                 }
-                let legacy_id = tree_builder.write_tree();
+                let legacy_id = tree_builder.write_tree()?;
                 if store.use_tree_conflict_format() {
                     let legacy_tree = store.get_tree(RepoPath::root(), &legacy_id)?;
                     let merged_tree = MergedTree::from_legacy_tree(legacy_tree)?;
@@ -1272,7 +1272,7 @@ impl MergedTreeBuilder {
         let merge_builder: MergeBuilder<TreeId> = tree_builders
             .into_iter()
             .map(|builder| builder.write_tree())
-            .collect();
+            .try_collect()?;
         Ok(merge_builder.build())
     }
 }

lib/src/tree_builder.rs

@@ -17,8 +17,7 @@
 use std::collections::BTreeMap;
 use std::sync::Arc;
 
-use crate::backend;
-use crate::backend::{TreeId, TreeValue};
+use crate::backend::{self, BackendResult, TreeId, TreeValue};
 use crate::repo_path::{RepoPath, RepoPathBuf};
 use crate::store::Store;
 use crate::tree::Tree;
@@ -69,12 +68,12 @@ impl TreeBuilder {
         }
     }
 
-    pub fn write_tree(self) -> TreeId {
+    pub fn write_tree(self) -> BackendResult<TreeId> {
         if self.overrides.is_empty() {
-            return self.base_tree_id;
+            return Ok(self.base_tree_id);
         }
 
-        let mut trees_to_write = self.get_base_trees();
+        let mut trees_to_write = self.get_base_trees()?;
 
         // Update entries in parent trees for file overrides
         for (path, file_override) in self.overrides {
@@ -103,24 +102,25 @@ impl TreeBuilder {
                         // Entry would have been replaced with file (see above)
                     }
                 } else {
-                    let tree = store.write_tree(&dir, tree).unwrap();
+                    let tree = store.write_tree(&dir, tree)?;
                     parent_tree.set(basename.to_owned(), TreeValue::Tree(tree.id().clone()));
                 }
             } else {
                 // We're writing the root tree. Write it even if empty. Return its id.
                 assert!(trees_to_write.is_empty());
-                return store.write_tree(&dir, tree).unwrap().id().clone();
+                let written_tree = store.write_tree(&dir, tree)?;
+                return Ok(written_tree.id().clone());
             }
         }
 
         unreachable!("trees_to_write must contain the root tree");
     }
 
-    fn get_base_trees(&self) -> BTreeMap<RepoPathBuf, backend::Tree> {
+    fn get_base_trees(&self) -> BackendResult<BTreeMap<RepoPathBuf, backend::Tree>> {
         let store = &self.store;
         let mut tree_cache = {
             let dir = RepoPathBuf::root();
-            let tree = store.get_tree(&dir, &self.base_tree_id).unwrap();
+            let tree = store.get_tree(&dir, &self.base_tree_id)?;
             BTreeMap::from([(dir, tree)])
         };
 
@@ -128,28 +128,26 @@ impl TreeBuilder {
             tree_cache: &'a mut BTreeMap<RepoPathBuf, Tree>,
             store: &Arc<Store>,
             dir: &RepoPath,
-        ) -> &'a Tree {
+        ) -> BackendResult<&'a Tree> {
             // `if let Some(tree) = ...` doesn't pass lifetime check as of Rust 1.76.0
             if tree_cache.contains_key(dir) {
-                return tree_cache.get(dir).unwrap();
+                return Ok(tree_cache.get(dir).unwrap());
             }
             let (parent, basename) = dir.split().expect("root must be populated");
-            // TODO: Propagate errors
-            let tree = populate_trees(tree_cache, store, parent)
-                .sub_tree(basename)
-                .unwrap()
+            let tree = populate_trees(tree_cache, store, parent)?
+                .sub_tree(basename)?
                 .unwrap_or_else(|| Tree::null(store.clone(), dir.to_owned()));
-            tree_cache.entry(dir.to_owned()).or_insert(tree)
+            Ok(tree_cache.entry(dir.to_owned()).or_insert(tree))
         }
 
         for path in self.overrides.keys() {
             let parent = path.parent().unwrap();
-            populate_trees(&mut tree_cache, store, parent);
+            populate_trees(&mut tree_cache, store, parent)?;
         }
 
-        tree_cache
+        Ok(tree_cache
             .into_iter()
             .map(|(dir, tree)| (dir, tree.data().clone()))
-            .collect()
+            .collect())
     }
 }

lib/tests/test_local_working_copy.rs

@@ -348,7 +348,7 @@ fn test_tree_builder_file_directory_transition() {
     // Add file at parent_path
     let mut tree_builder = store.tree_builder(store.empty_tree_id().clone());
     testutils::write_normal_file(&mut tree_builder, parent_path, "");
-    let tree_id = tree_builder.write_tree();
+    let tree_id = tree_builder.write_tree().unwrap();
     check_out_tree(&tree_id);
     assert!(parent_path.to_fs_path(&workspace_root).is_file());
     assert!(!child_path.to_fs_path(&workspace_root).exists());
@@ -357,7 +357,7 @@ fn test_tree_builder_file_directory_transition() {
     let mut tree_builder = store.tree_builder(tree_id);
     tree_builder.remove(parent_path.to_owned());
     testutils::write_normal_file(&mut tree_builder, child_path, "");
-    let tree_id = tree_builder.write_tree();
+    let tree_id = tree_builder.write_tree().unwrap();
     check_out_tree(&tree_id);
     assert!(parent_path.to_fs_path(&workspace_root).is_dir());
     assert!(child_path.to_fs_path(&workspace_root).is_file());
@@ -366,7 +366,7 @@ fn test_tree_builder_file_directory_transition() {
     let mut tree_builder = store.tree_builder(tree_id);
     tree_builder.remove(child_path.to_owned());
     testutils::write_normal_file(&mut tree_builder, parent_path, "");
-    let tree_id = tree_builder.write_tree();
+    let tree_id = tree_builder.write_tree().unwrap();
     check_out_tree(&tree_id);
     assert!(parent_path.to_fs_path(&workspace_root).is_file());
     assert!(!child_path.to_fs_path(&workspace_root).exists());
@@ -799,7 +799,7 @@ fn test_gitignores_ignored_directory_already_tracked() {
                 }
             }
         }
-        let id = tree_builder.write_tree();
+        let id = tree_builder.write_tree().unwrap();
         MergedTree::legacy(store.get_tree(RepoPath::root(), &id).unwrap())
     };
 
@@ -930,7 +930,7 @@ fn test_gitsubmodule() {
         TreeValue::GitSubmodule(submodule_id),
     );
 
-    let tree_id = MergedTreeId::Legacy(tree_builder.write_tree());
+    let tree_id = MergedTreeId::Legacy(tree_builder.write_tree().unwrap());
     let tree = store.get_root_tree(&tree_id).unwrap();
     let commit = commit_with_tree(repo.store(), tree.id());
     let ws = &mut test_workspace.workspace;

lib/tests/test_merge_trees.rs

@@ -60,7 +60,7 @@ fn test_same_type() {
                 );
             }
         }
-        let tree_id = tree_builder.write_tree();
+        let tree_id = tree_builder.write_tree().unwrap();
         store.get_tree(RepoPath::root(), &tree_id).unwrap()
     };
 
@@ -202,7 +202,7 @@ fn test_executable() {
                 testutils::write_normal_file(&mut tree_builder, repo_path, "contents");
             }
         }
-        let tree_id = tree_builder.write_tree();
+        let tree_id = tree_builder.write_tree().unwrap();
         store.get_tree(RepoPath::root(), &tree_id).unwrap()
     };
 
@@ -250,7 +250,7 @@ fn test_subtrees() {
                 &format!("contents of {path:?}"),
             );
         }
-        let tree_id = tree_builder.write_tree();
+        let tree_id = tree_builder.write_tree().unwrap();
         store.get_tree(RepoPath::root(), &tree_id).unwrap()
     };
 
@@ -304,7 +304,7 @@ fn test_subtree_becomes_empty() {
                 &format!("contents of {path:?}"),
             );
         }
-        let tree_id = tree_builder.write_tree();
+        let tree_id = tree_builder.write_tree().unwrap();
         store.get_tree(RepoPath::root(), &tree_id).unwrap()
     };
 
@@ -333,7 +333,7 @@ fn test_subtree_one_missing() {
                 &format!("contents of {path:?}"),
             );
         }
-        let tree_id = tree_builder.write_tree();
+        let tree_id = tree_builder.write_tree().unwrap();
         store.get_tree(RepoPath::root(), &tree_id).unwrap()
     };
 
@@ -400,11 +400,11 @@ fn test_types() {
         RepoPath::from_internal_string("tree_normal_symlink"),
         "contents",
     );
-    let base_tree_id = base_tree_builder.write_tree();
+    let base_tree_id = base_tree_builder.write_tree().unwrap();
     let base_tree = store.get_tree(RepoPath::root(), &base_tree_id).unwrap();
-    let side1_tree_id = side1_tree_builder.write_tree();
+    let side1_tree_id = side1_tree_builder.write_tree().unwrap();
     let side1_tree = store.get_tree(RepoPath::root(), &side1_tree_id).unwrap();
-    let side2_tree_id = side2_tree_builder.write_tree();
+    let side2_tree_id = side2_tree_builder.write_tree().unwrap();
     let side2_tree = store.get_tree(RepoPath::root(), &side2_tree_id).unwrap();
 
     // Created the merged tree

lib/tests/test_merged_tree.rs

@@ -142,7 +142,7 @@ fn test_from_legacy_tree() {
     let dir1_filename_id = write_file(store.as_ref(), dir1_filename, "file5_v2");
     tree_builder.set(dir1_filename.to_owned(), file_value(&dir1_filename_id));
 
-    let tree_id = tree_builder.write_tree();
+    let tree_id = tree_builder.write_tree().unwrap();
     let tree = store.get_tree(RepoPath::root(), &tree_id).unwrap();
 
     let merged_tree = MergedTree::from_legacy_tree(tree.clone()).unwrap();

lib/testutils/src/lib.rs

@@ -314,7 +314,7 @@ pub fn create_single_tree(repo: &Arc<ReadonlyRepo>, path_contents: &[(&RepoPath,
     for (path, contents) in path_contents {
         write_normal_file(&mut tree_builder, path, contents);
     }
-    let id = tree_builder.write_tree();
+    let id = tree_builder.write_tree().unwrap();
     store.get_tree(RepoPath::root(), &id).unwrap()
 }