From 07bb1d81b76e9f27481076ce39f92f666f58eb5f Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Tue, 21 May 2024 20:42:28 -0700 Subject: [PATCH] tree_builder: propagate errors from `write_tree()` --- lib/src/merged_tree.rs | 6 ++--- lib/src/tree_builder.rs | 36 +++++++++++++--------------- lib/tests/test_local_working_copy.rs | 10 ++++---- lib/tests/test_merge_trees.rs | 16 ++++++------- lib/tests/test_merged_tree.rs | 2 +- lib/testutils/src/lib.rs | 2 +- 6 files changed, 35 insertions(+), 37 deletions(-) diff --git a/lib/src/merged_tree.rs b/lib/src/merged_tree.rs index 40583ec5a..e9cb5f674 100644 --- a/lib/src/merged_tree.rs +++ b/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 = tree_builders .into_iter() .map(|builder| builder.write_tree()) - .collect(); + .try_collect()?; Ok(merge_builder.build()) } } diff --git a/lib/src/tree_builder.rs b/lib/src/tree_builder.rs index b7dc42b15..88fc0d335 100644 --- a/lib/src/tree_builder.rs +++ b/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 { 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 { + fn get_base_trees(&self) -> BackendResult> { 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, store: &Arc, 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()) } } diff --git a/lib/tests/test_local_working_copy.rs b/lib/tests/test_local_working_copy.rs index ea2f1be90..8c06660e2 100644 --- a/lib/tests/test_local_working_copy.rs +++ b/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; diff --git a/lib/tests/test_merge_trees.rs b/lib/tests/test_merge_trees.rs index 0b11f6828..de4d37971 100644 --- a/lib/tests/test_merge_trees.rs +++ b/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 diff --git a/lib/tests/test_merged_tree.rs b/lib/tests/test_merged_tree.rs index 6d91188cb..49f225b7e 100644 --- a/lib/tests/test_merged_tree.rs +++ b/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(); diff --git a/lib/testutils/src/lib.rs b/lib/testutils/src/lib.rs index 53a9fd348..25016d911 100644 --- a/lib/testutils/src/lib.rs +++ b/lib/testutils/src/lib.rs @@ -314,7 +314,7 @@ pub fn create_single_tree(repo: &Arc, 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() }