mirrors/jj
1
0
Fork 1
mirror of https://github.com/martinvonz/jj.git synced 2024-12-24 12:48:55 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 6

merged_tree: propagate errors from TreeEntriesIterator

Browse source 
We shouldn't panic if we fail to read a tree from the backend.
This commit is contained in:
Martin von Zweigbergk 2024-03-19 13:18:54 -07:00 committed by Martin von Zweigbergk
parent 7093d5d359
commit 0d1ff8a150
8 changed files with 46 additions and 42 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
cli/src/commands/cat.rs
View file

@ -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"),

3
cli/src/commands/chmod.rs
View file

@ -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 '{}'.",

12
cli/tests/test_chmod_command.rs
View file

@ -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,

18
cli/tests/test_debug_command.rs
View file

@ -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 })))
"###
);
}

12
lib/src/merged_tree.rs
View file

@ -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();

23
lib/tests/test_local_working_copy.rs
View file

@ -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]

8
lib/tests/test_merged_tree.rs
View file

@ -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()

4
lib/testutils/src/lib.rs
View file

@ -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);