From 1970ddef1573132cdee5e53a7f6bc0c3ac083d50 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Tue, 21 May 2024 06:55:33 -0700 Subject: [PATCH] tree: propagate errors from `sub_tree()`/`path_value()` --- cli/src/cli_util.rs | 3 +- cli/src/commands/cat.rs | 2 +- cli/src/commands/git.rs | 2 +- cli/src/merge_tools/builtin.rs | 12 ++--- cli/src/merge_tools/mod.rs | 2 +- lib/src/local_working_copy.rs | 2 +- lib/src/merged_tree.rs | 54 +++++++++++++--------- lib/src/tree.rs | 56 +++++++++++++++-------- lib/src/tree_builder.rs | 2 + lib/tests/test_merge_trees.rs | 6 +-- lib/tests/test_merged_tree.rs | 83 ++++++++++++++++++++-------------- lib/tests/test_rewrite.rs | 15 ++++-- 12 files changed, 146 insertions(+), 93 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 5cd3104df..7d5a78742 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -1967,7 +1967,8 @@ pub fn print_unmatched_explicit_paths<'a>( ) -> io::Result<()> { let mut explicit_paths = expression.explicit_paths().collect_vec(); for tree in trees { - explicit_paths.retain(|&path| tree.path_value(path).is_absent()); + // TODO: propagate errors + explicit_paths.retain(|&path| tree.path_value(path).unwrap().is_absent()); if explicit_paths.is_empty() { return Ok(()); } diff --git a/cli/src/commands/cat.rs b/cli/src/commands/cat.rs index 6c606dd45..ba0ef733b 100644 --- a/cli/src/commands/cat.rs +++ b/cli/src/commands/cat.rs @@ -58,7 +58,7 @@ pub(crate) fn cmd_cat( // Try fast path for single file entry if let Some(path) = get_single_path(&fileset_expression) { - let value = tree.path_value(path); + let value = tree.path_value(path)?; if value.is_absent() { let ui_path = workspace_command.format_file_path(path); return Err(user_error(format!("No such path: {ui_path}"))); diff --git a/cli/src/commands/git.rs b/cli/src/commands/git.rs index 9f22ca7c4..2e1cbe86e 100644 --- a/cli/src/commands/git.rs +++ b/cli/src/commands/git.rs @@ -1262,7 +1262,7 @@ fn cmd_git_submodule_print_gitmodules( let commit = workspace_command.resolve_single_rev(&args.revisions)?; let tree = commit.tree()?; let gitmodules_path = RepoPath::from_internal_string(".gitmodules"); - let mut gitmodules_file = match tree.path_value(gitmodules_path).into_resolved() { + let mut gitmodules_file = match tree.path_value(gitmodules_path)?.into_resolved() { Ok(None) => { writeln!(ui.status(), "No submodules!")?; return Ok(()); diff --git a/cli/src/merge_tools/builtin.rs b/cli/src/merge_tools/builtin.rs index 9e84cfd3c..217d95aef 100644 --- a/cli/src/merge_tools/builtin.rs +++ b/cli/src/merge_tools/builtin.rs @@ -128,7 +128,7 @@ fn read_file_contents( tree: &MergedTree, path: &RepoPath, ) -> Result { - let value = tree.path_value(path); + let value = tree.path_value(path)?; let materialized_value = materialize_tree_value(store, path, value) .map_err(BuiltinToolError::BackendError) .block_on()?; @@ -444,7 +444,7 @@ pub fn apply_diff_builtin( let file_deleted = file_existed_previously && !file_exists_now; if new_empty_file { - let value = right_tree.path_value(&path); + let value = right_tree.path_value(&path)?; tree_builder.set_or_remove(path, value); } else if file_deleted { tree_builder.set_or_remove(path, Merge::absent()); @@ -458,7 +458,7 @@ pub fn apply_diff_builtin( old_description: _, new_description: _, } => { - let value = right_tree.path_value(&path); + let value = right_tree.path_value(&path)?; tree_builder.set_or_remove(path, value); } scm_record::SelectedContents::Present { contents } => { @@ -1028,9 +1028,9 @@ mod tests { } } let merge = Merge::from_vec(vec![ - to_file_id(left_tree.path_value(path)), - to_file_id(base_tree.path_value(path)), - to_file_id(right_tree.path_value(path)), + to_file_id(left_tree.path_value(path).unwrap()), + to_file_id(base_tree.path_value(path).unwrap()), + to_file_id(right_tree.path_value(path).unwrap()), ]); let content = extract_as_single_hunk(&merge, store, path).block_on(); let slices = content.map(|ContentHunk(buf)| buf.as_slice()); diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index 8a45fa7a3..00dee77ba 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -283,7 +283,7 @@ impl MergeEditor { tree: &MergedTree, repo_path: &RepoPath, ) -> Result { - let conflict = match tree.path_value(repo_path).into_resolved() { + let conflict = match tree.path_value(repo_path)?.into_resolved() { Err(conflict) => conflict, Ok(Some(_)) => return Err(ConflictResolveError::NotAConflict(repo_path.to_owned())), Ok(None) => return Err(ConflictResolveError::PathNotFound(repo_path.to_owned())), diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index 16e2f41ea..b72a9168b 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -1081,7 +1081,7 @@ impl TreeState { if clean { Ok(None) } else { - let current_tree_values = current_tree.path_value(repo_path); + let current_tree_values = current_tree.path_value(repo_path)?; let new_file_type = if !self.symlink_support { let mut new_file_type = new_file_state.file_type.clone(); if matches!(new_file_type, FileType::Normal { .. }) diff --git a/lib/src/merged_tree.rs b/lib/src/merged_tree.rs index 9de9a6189..40583ec5a 100644 --- a/lib/src/merged_tree.rs +++ b/lib/src/merged_tree.rs @@ -211,30 +211,30 @@ impl MergedTree { /// Gets the `MergeTree` in a subdirectory of the current tree. If the path /// doesn't correspond to a tree in any of the inputs to the merge, then /// that entry will be replace by an empty tree in the result. - pub fn sub_tree(&self, name: &RepoPathComponent) -> Option { + pub fn sub_tree(&self, name: &RepoPathComponent) -> BackendResult> { if let MergedTree::Legacy(tree) = self { - tree.sub_tree(name).map(MergedTree::Legacy) + Ok(tree.sub_tree(name)?.map(MergedTree::Legacy)) } else { match self.value(name) { MergedTreeVal::Resolved(Some(TreeValue::Tree(sub_tree_id))) => { let subdir = self.dir().join(name); - Some(MergedTree::resolved( - self.store().get_tree(&subdir, sub_tree_id).unwrap(), - )) + Ok(Some(MergedTree::resolved( + self.store().get_tree(&subdir, sub_tree_id)?, + ))) } - MergedTreeVal::Resolved(_) => None, + MergedTreeVal::Resolved(_) => Ok(None), MergedTreeVal::Conflict(merge) => { - let merged_trees = merge.map(|value| match value { + let merged_trees = merge.try_map(|value| match value { Some(TreeValue::Tree(sub_tree_id)) => { let subdir = self.dir().join(name); - self.store().get_tree(&subdir, sub_tree_id).unwrap() + self.store().get_tree(&subdir, sub_tree_id) } _ => { let subdir = self.dir().join(name); - Tree::null(self.store().clone(), subdir.clone()) + Ok(Tree::null(self.store().clone(), subdir.clone())) } - }); - Some(MergedTree::Merge(merged_trees)) + })?; + Ok(Some(MergedTree::Merge(merged_trees))) } } } @@ -243,17 +243,17 @@ impl MergedTree { /// The value at the given path. The value can be `Resolved` even if /// `self` is a `Conflict`, which happens if the value at the path can be /// trivially merged. - pub fn path_value(&self, path: &RepoPath) -> MergedTreeValue { + pub fn path_value(&self, path: &RepoPath) -> BackendResult { assert_eq!(self.dir(), RepoPath::root()); match path.split() { - Some((dir, basename)) => match self.sub_tree_recursive(dir.components()) { - None => Merge::absent(), - Some(tree) => tree.value(basename).to_merge(), + Some((dir, basename)) => match self.sub_tree_recursive(dir.components())? { + None => Ok(Merge::absent()), + Some(tree) => Ok(tree.value(basename).to_merge()), }, None => match self { - MergedTree::Legacy(tree) => Merge::normal(TreeValue::Tree(tree.id().clone())), + MergedTree::Legacy(tree) => Ok(Merge::normal(TreeValue::Tree(tree.id().clone()))), MergedTree::Merge(trees) => { - trees.map(|tree| Some(TreeValue::Tree(tree.id().clone()))) + Ok(trees.map(|tree| Some(TreeValue::Tree(tree.id().clone())))) } }, } @@ -267,12 +267,22 @@ impl MergedTree { } } - fn sub_tree_recursive(&self, mut components: RepoPathComponentsIter) -> Option { - if let Some(first) = components.next() { - components.try_fold(self.sub_tree(first)?, |tree, name| tree.sub_tree(name)) - } else { - Some(self.clone()) + fn sub_tree_recursive( + &self, + components: RepoPathComponentsIter, + ) -> BackendResult> { + let mut current_tree = self.clone(); + for name in components { + match current_tree.sub_tree(name)? { + None => { + return Ok(None); + } + Some(sub_tree) => { + current_tree = sub_tree; + } + } } + Ok(Some(current_tree)) } /// Iterator over the entries matching the given matcher. Subtrees are diff --git a/lib/src/tree.rs b/lib/src/tree.rs index a7088c645..039a28d84 100644 --- a/lib/src/tree.rs +++ b/lib/src/tree.rs @@ -124,39 +124,55 @@ impl Tree { self.data.value(basename) } - pub fn path_value(&self, path: &RepoPath) -> Option { + pub fn path_value(&self, path: &RepoPath) -> BackendResult> { assert_eq!(self.dir(), RepoPath::root()); match path.split() { - Some((dir, basename)) => self - .sub_tree_recursive(dir.components()) - .and_then(|tree| tree.data.value(basename).cloned()), - None => Some(TreeValue::Tree(self.id.clone())), + Some((dir, basename)) => { + let tree = self.sub_tree_recursive(dir.components())?; + Ok(tree.and_then(|tree| tree.data.value(basename).cloned())) + } + None => Ok(Some(TreeValue::Tree(self.id.clone()))), } } - pub fn sub_tree(&self, name: &RepoPathComponent) -> Option { - self.data.value(name).and_then(|sub_tree| match sub_tree { - TreeValue::Tree(sub_tree_id) => { - let subdir = self.dir.join(name); - Some(self.store.get_tree(&subdir, sub_tree_id).unwrap()) + pub fn sub_tree(&self, name: &RepoPathComponent) -> BackendResult> { + if let Some(sub_tree) = self.data.value(name) { + match sub_tree { + TreeValue::Tree(sub_tree_id) => { + let subdir = self.dir.join(name); + let sub_tree = self.store.get_tree(&subdir, sub_tree_id)?; + Ok(Some(sub_tree)) + } + _ => Ok(None), } - _ => None, - }) + } else { + Ok(None) + } } fn known_sub_tree(&self, subdir: &RepoPath, id: &TreeId) -> Tree { self.store.get_tree(subdir, id).unwrap() } - fn sub_tree_recursive(&self, mut components: RepoPathComponentsIter) -> Option { - if let Some(first) = components.next() { - components.try_fold(self.sub_tree(first)?, |tree, name| tree.sub_tree(name)) - } else { - // TODO: It would be nice to be able to return a reference here, but - // then we would have to figure out how to share Tree instances - // across threads. - Some(self.clone()) + fn sub_tree_recursive( + &self, + components: RepoPathComponentsIter, + ) -> BackendResult> { + let mut current_tree = self.clone(); + for name in components { + match current_tree.sub_tree(name)? { + None => { + return Ok(None); + } + Some(sub_tree) => { + current_tree = sub_tree; + } + } } + // TODO: It would be nice to be able to return a reference here, but + // then we would have to figure out how to share Tree instances + // across threads. + Ok(Some(current_tree)) } pub fn conflicts_matching(&self, matcher: &dyn Matcher) -> Vec<(RepoPathBuf, ConflictId)> { diff --git a/lib/src/tree_builder.rs b/lib/src/tree_builder.rs index 71ec1c077..b7dc42b15 100644 --- a/lib/src/tree_builder.rs +++ b/lib/src/tree_builder.rs @@ -134,8 +134,10 @@ impl TreeBuilder { return 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() .unwrap_or_else(|| Tree::null(store.clone(), dir.to_owned())); tree_cache.entry(dir.to_owned()).or_insert(tree) } diff --git a/lib/tests/test_merge_trees.rs b/lib/tests/test_merge_trees.rs index 35ba95601..0b11f6828 100644 --- a/lib/tests/test_merge_trees.rs +++ b/lib/tests/test_merge_trees.rs @@ -599,8 +599,8 @@ fn test_simplify_conflict_after_resolving_parent() { // Test the setup: Both B and C should have conflicts. let tree_b2 = commit_b2.tree().unwrap(); let tree_c2 = commit_b2.tree().unwrap(); - assert!(!tree_b2.path_value(path).is_resolved()); - assert!(!tree_c2.path_value(path).is_resolved()); + assert!(!tree_b2.path_value(path).unwrap().is_resolved()); + assert!(!tree_c2.path_value(path).unwrap().is_resolved()); // Create the resolved B and rebase C on top. let tree_b3 = create_tree(repo, &[(path, "AbC\ndef\nghi\n")]); @@ -622,7 +622,7 @@ fn test_simplify_conflict_after_resolving_parent() { // The conflict should now be resolved. let tree_c2 = commit_c3.tree().unwrap(); - let resolved_value = tree_c2.path_value(path); + let resolved_value = tree_c2.path_value(path).unwrap(); match resolved_value.into_resolved() { Ok(Some(TreeValue::File { id, diff --git a/lib/tests/test_merged_tree.rs b/lib/tests/test_merged_tree.rs index 01e05fb39..6d91188cb 100644 --- a/lib/tests/test_merged_tree.rs +++ b/lib/tests/test_merged_tree.rs @@ -301,7 +301,7 @@ fn test_path_value_and_entries() { // Get the root tree assert_eq!( - merged_tree.path_value(RepoPath::root()), + merged_tree.path_value(RepoPath::root()).unwrap(), Merge::from_removes_adds( vec![Some(TreeValue::Tree(tree1.id().clone()))], vec![ @@ -312,40 +312,43 @@ fn test_path_value_and_entries() { ); // Get file path without conflict assert_eq!( - merged_tree.path_value(resolved_file_path), - Merge::resolved(tree1.path_value(resolved_file_path)), + merged_tree.path_value(resolved_file_path).unwrap(), + Merge::resolved(tree1.path_value(resolved_file_path).unwrap()), ); // Get directory path without conflict assert_eq!( - merged_tree.path_value(resolved_dir_path), - Merge::resolved(tree1.path_value(resolved_dir_path)), + merged_tree.path_value(resolved_dir_path).unwrap(), + Merge::resolved(tree1.path_value(resolved_dir_path).unwrap()), ); // Get missing path - assert_eq!(merged_tree.path_value(missing_path), Merge::absent()); + assert_eq!( + merged_tree.path_value(missing_path).unwrap(), + Merge::absent() + ); // Get modify/delete conflict (some None values) assert_eq!( - merged_tree.path_value(modify_delete_path), + merged_tree.path_value(modify_delete_path).unwrap(), Merge::from_removes_adds( - vec![tree1.path_value(modify_delete_path)], - vec![tree2.path_value(modify_delete_path), None] + vec![tree1.path_value(modify_delete_path).unwrap()], + vec![tree2.path_value(modify_delete_path).unwrap(), None] ), ); // Get file/dir conflict path assert_eq!( - merged_tree.path_value(file_dir_conflict_path), + merged_tree.path_value(file_dir_conflict_path).unwrap(), Merge::from_removes_adds( - vec![tree1.path_value(file_dir_conflict_path)], + vec![tree1.path_value(file_dir_conflict_path).unwrap()], vec![ - tree2.path_value(file_dir_conflict_path), - tree3.path_value(file_dir_conflict_path) + tree2.path_value(file_dir_conflict_path).unwrap(), + tree3.path_value(file_dir_conflict_path).unwrap() ] ), ); // Get file inside file/dir conflict // There is a conflict in the parent directory, but this file is still resolved assert_eq!( - merged_tree.path_value(file_dir_conflict_sub_path), - Merge::resolved(tree3.path_value(file_dir_conflict_sub_path)), + merged_tree.path_value(file_dir_conflict_sub_path).unwrap(), + Merge::resolved(tree3.path_value(file_dir_conflict_sub_path).unwrap()), ); // Test entries() @@ -363,7 +366,7 @@ fn test_path_value_and_entries() { ] .iter() .sorted() - .map(|&path| (path.to_owned(), merged_tree.path_value(path))) + .map(|&path| (path.to_owned(), merged_tree.path_value(path).unwrap())) .collect_vec(); assert_eq!(actual_entries, expected_entries); @@ -378,7 +381,7 @@ fn test_path_value_and_entries() { let expected_entries = [resolved_file_path, modify_delete_path] .iter() .sorted() - .map(|&path| (path.to_owned(), merged_tree.path_value(path))) + .map(|&path| (path.to_owned(), merged_tree.path_value(path).unwrap())) .collect_vec(); assert_eq!(actual_entries, expected_entries); } @@ -592,8 +595,11 @@ fn test_conflict_iterator() { let conflicts = tree.conflicts().collect_vec(); let conflict_at = |path: &RepoPath| { Merge::from_removes_adds( - vec![base1.path_value(path)], - vec![side1.path_value(path), side2.path_value(path)], + vec![base1.path_value(path).unwrap()], + vec![ + side1.path_value(path).unwrap(), + side2.path_value(path).unwrap(), + ], ) }; // We initially also get a conflict in trivial_hunk_path because we had @@ -681,11 +687,14 @@ fn test_conflict_iterator_higher_arity() { let conflicts = tree.conflicts().collect_vec(); let conflict_at = |path: &RepoPath| { Merge::from_removes_adds( - vec![base1.path_value(path), base2.path_value(path)], vec![ - side1.path_value(path), - side2.path_value(path), - side3.path_value(path), + base1.path_value(path).unwrap(), + base2.path_value(path).unwrap(), + ], + vec![ + side1.path_value(path).unwrap(), + side2.path_value(path).unwrap(), + side3.path_value(path).unwrap(), ], ) }; @@ -710,10 +719,10 @@ fn test_conflict_iterator_higher_arity() { ( two_sided_path.to_owned(), Merge::from_removes_adds( - vec![base2.path_value(two_sided_path)], + vec![base2.path_value(two_sided_path).unwrap()], vec![ - side1.path_value(two_sided_path), - side3.path_value(two_sided_path), + side1.path_value(two_sided_path).unwrap(), + side3.path_value(two_sided_path).unwrap(), ], ) ), @@ -761,8 +770,8 @@ fn test_diff_resolved() { ( modified_path.to_owned(), ( - Merge::resolved(before.path_value(modified_path)), - Merge::resolved(after.path_value(modified_path)) + Merge::resolved(before.path_value(modified_path).unwrap()), + Merge::resolved(after.path_value(modified_path).unwrap()) ), ) ); @@ -771,7 +780,7 @@ fn test_diff_resolved() { ( removed_path.to_owned(), ( - Merge::resolved(before.path_value(removed_path)), + Merge::resolved(before.path_value(removed_path).unwrap()), Merge::absent() ), ) @@ -782,7 +791,7 @@ fn test_diff_resolved() { added_path.to_owned(), ( Merge::absent(), - Merge::resolved(after.path_value(added_path)) + Merge::resolved(after.path_value(added_path).unwrap()) ), ) ); @@ -869,7 +878,10 @@ fn test_diff_conflicted() { .map(|&path| { ( path.to_owned(), - (left_merged.path_value(path), right_merged.path_value(path)), + ( + left_merged.path_value(path).unwrap(), + right_merged.path_value(path).unwrap(), + ), ) }) .collect_vec(); @@ -885,7 +897,10 @@ fn test_diff_conflicted() { .map(|&path| { ( path.to_owned(), - (right_merged.path_value(path), left_merged.path_value(path)), + ( + right_merged.path_value(path).unwrap(), + left_merged.path_value(path).unwrap(), + ), ) }) .collect_vec(); @@ -985,8 +1000,8 @@ fn test_diff_dir_file() { vec![right_base], vec![right_side1, right_side2], )); - let left_value = |path: &RepoPath| left_merged.path_value(path); - let right_value = |path: &RepoPath| right_merged.path_value(path); + let left_value = |path: &RepoPath| left_merged.path_value(path).unwrap(); + let right_value = |path: &RepoPath| right_merged.path_value(path).unwrap(); // Test the forwards diff { diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index 7ff9ed0f9..682df0017 100644 --- a/lib/tests/test_rewrite.rs +++ b/lib/tests/test_rewrite.rs @@ -947,9 +947,18 @@ fn test_rebase_descendants_contents() { let tree_c = commit_c.tree().unwrap(); let tree_d = commit_d.tree().unwrap(); let new_tree_c = new_commit_c.tree().unwrap(); - assert_eq!(new_tree_c.path_value(path3), tree_c.path_value(path3)); - assert_eq!(new_tree_c.path_value(path4), tree_d.path_value(path4)); - assert_ne!(new_tree_c.path_value(path2), tree_b.path_value(path2)); + assert_eq!( + new_tree_c.path_value(path3).unwrap(), + tree_c.path_value(path3).unwrap() + ); + assert_eq!( + new_tree_c.path_value(path4).unwrap(), + tree_d.path_value(path4).unwrap() + ); + assert_ne!( + new_tree_c.path_value(path2).unwrap(), + tree_b.path_value(path2).unwrap() + ); } #[test]