From 0d1ff8a150ec85b3e66cc4ea00cf4ab43a021dc7 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Tue, 19 Mar 2024 13:18:54 -0700 Subject: [PATCH] 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 | 8 +++++--- cli/src/commands/chmod.rs | 3 ++- cli/tests/test_chmod_command.rs | 12 ++++++------ cli/tests/test_debug_command.rs | 18 +++++++++--------- lib/src/merged_tree.rs | 12 ++++++------ lib/tests/test_local_working_copy.rs | 23 ++++++++++------------- lib/tests/test_merged_tree.rs | 8 ++++++-- lib/testutils/src/lib.rs | 4 ++-- 8 files changed, 46 insertions(+), 42 deletions(-) diff --git a/cli/src/commands/cat.rs b/cli/src/commands/cat.rs index f5f20084b..6c606dd45 100644 --- a/cli/src/commands/cat.rs +++ b/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>( ui: &Ui, workspace_command: &WorkspaceCommandHelper, - entries: impl IntoIterator, + entries: impl IntoIterator)>, ) -> 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"), diff --git a/cli/src/commands/chmod.rs b/cli/src/commands/chmod.rs index 4d8078d4e..1bc29aa05 100644 --- a/cli/src/commands/chmod.rs +++ b/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 '{}'.", diff --git a/cli/tests/test_chmod_command.rs b/cli/tests/test_chmod_command.rs index 4ae1ed63f..4a6db40af 100644 --- a/cli/tests/test_chmod_command.rs +++ b/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, diff --git a/cli/tests/test_debug_command.rs b/cli/tests/test_debug_command.rs index 4a126c315..741ba5ff3 100644 --- a/cli/tests/test_debug_command.rs +++ b/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 }))) "### ); } diff --git a/lib/src/merged_tree.rs b/lib/src/merged_tree.rs index 2bdab0d8a..2593bf05c 100644 --- a/lib/src/merged_tree.rs +++ b/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); fn next(&mut self) -> Option { 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(); diff --git a/lib/tests/test_local_working_copy.rs b/lib/tests/test_local_working_copy.rs index 1b058c920..e676ecf48 100644 --- a/lib/tests/test_local_working_copy.rs +++ b/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 { paths.iter().map(|&path| path.to_owned()).collect() } +fn tree_entries(tree: &MergedTree) -> Vec<(RepoPathBuf, Option)> { + 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] diff --git a/lib/tests/test_merged_tree.rs b/lib/tests/test_merged_tree.rs index b19893dda..fcbe0ae09 100644 --- a/lib/tests/test_merged_tree.rs +++ b/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() diff --git a/lib/testutils/src/lib.rs b/lib/testutils/src/lib.rs index fdd871aa5..53a9fd348 100644 --- a/lib/testutils/src/lib.rs +++ b/lib/testutils/src/lib.rs @@ -380,8 +380,8 @@ pub fn dump_tree(store: &Arc, 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);