diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 842a256fe0..310f383127 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -1567,7 +1567,8 @@ impl WorkspaceCommandTransaction<'_> { Ok(right_tree.id().clone()) } else { let mut tree_builder = MergedTreeBuilder::new(left_tree.id().clone()); - for (repo_path, _left, right) in left_tree.diff(right_tree, matcher) { + for (repo_path, diff) in left_tree.diff(right_tree, matcher) { + let (_left, right) = diff?; tree_builder.set_or_remove(repo_path, right); } Ok(tree_builder.write_tree(self.repo().store())?) diff --git a/cli/src/commands/mod.rs b/cli/src/commands/mod.rs index b41a810237..a9f1810972 100644 --- a/cli/src/commands/mod.rs +++ b/cli/src/commands/mod.rs @@ -3175,7 +3175,8 @@ fn cmd_restore( let matcher = workspace_command.matcher_from_values(&args.paths)?; let mut tree_builder = MergedTreeBuilder::new(to_commit.tree_id().clone()); let to_tree = to_commit.tree()?; - for (repo_path, before, _after) in from_tree.diff(&to_tree, matcher.as_ref()) { + for (repo_path, diff) in from_tree.diff(&to_tree, matcher.as_ref()) { + let (before, _after) = diff?; tree_builder.set_or_remove(repo_path, before); } tree_builder.write_tree(workspace_command.repo().store())? diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index 71d0eef533..23c39ca166 100644 --- a/cli/src/diff_util.rs +++ b/cli/src/diff_util.rs @@ -408,8 +408,9 @@ pub fn show_color_words_diff( ) -> Result<(), CommandError> { let repo = workspace_command.repo(); formatter.push_label("diff")?; - for (path, left_value, right_value) in tree_diff { + for (path, diff) in tree_diff { let ui_path = workspace_command.format_file_path(&path); + let (left_value, right_value) = diff?; if left_value.is_absent() { let right_content = diff_content(repo, &path, &right_value)?; let description = basic_diff_file_type(&right_value); @@ -686,8 +687,9 @@ pub fn show_git_diff( ) -> Result<(), CommandError> { let repo = workspace_command.repo(); formatter.push_label("diff")?; - for (path, left_value, right_value) in tree_diff { + for (path, diff) in tree_diff { let path_string = path.to_internal_file_string(); + let (left_value, right_value) = diff?; if left_value.is_absent() { let right_part = git_diff_part(repo, &path, &right_value)?; formatter.with_label("file_header", |formatter| { @@ -746,7 +748,8 @@ pub fn show_diff_summary( tree_diff: TreeDiffIterator, ) -> io::Result<()> { formatter.with_label("diff", |formatter| { - for (repo_path, before, after) in tree_diff { + for (repo_path, diff) in tree_diff { + let (before, after) = diff.unwrap(); if before.is_present() && after.is_present() { writeln!( formatter.labeled("modified"), @@ -806,7 +809,8 @@ pub fn show_diff_stat( let mut stats: Vec = vec![]; let mut max_path_width = 0; let mut max_diffs = 0; - for (repo_path, left, right) in tree_diff { + for (repo_path, diff) in tree_diff { + let (left, right) = diff?; let path = workspace_command.format_file_path(&repo_path); let left_content = diff_content(workspace_command.repo(), &repo_path, &left)?; let right_content = diff_content(workspace_command.repo(), &repo_path, &right)?; @@ -873,7 +877,8 @@ pub fn show_types( tree_diff: TreeDiffIterator, ) -> io::Result<()> { formatter.with_label("diff", |formatter| { - for (repo_path, before, after) in tree_diff { + for (repo_path, diff) in tree_diff { + let (before, after) = diff.unwrap(); writeln!( formatter.labeled("modified"), "{}{} {}", diff --git a/cli/src/merge_tools/builtin.rs b/cli/src/merge_tools/builtin.rs index 2fa31ff27d..68bfee7955 100644 --- a/cli/src/merge_tools/builtin.rs +++ b/cli/src/merge_tools/builtin.rs @@ -408,10 +408,10 @@ pub fn edit_diff_builtin( matcher: &dyn Matcher, ) -> Result { let store = left_tree.store().clone(); - let changed_files = left_tree + let changed_files: Vec<_> = left_tree .diff(right_tree, matcher) - .map(|(path, _left, _right)| path) - .collect_vec(); + .map(|(path, diff)| diff.map(|_| path)) + .try_collect()?; let files = make_diff_files(&store, left_tree, right_tree, &changed_files)?; let recorder = scm_record::Recorder::new( scm_record::RecordState { diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index f0ef9b7ad6..de9e2bdaaf 100644 --- a/cli/src/merge_tools/external.rs +++ b/cli/src/merge_tools/external.rs @@ -240,7 +240,7 @@ fn check_out_trees( ) -> Result { let changed_files = left_tree .diff(right_tree, matcher) - .map(|(path, _left, _right)| path) + .map(|(path, _diff)| path) .collect_vec(); let temp_dir = new_utf8_temp_dir("jj-diff-").map_err(DiffCheckoutError::SetUpDir)?; diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index 16d67835a1..c979f86536 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -1152,7 +1152,8 @@ impl TreeState { removed_files: 0, skipped_files: 0, }; - for (path, before, after) in old_tree.diff(new_tree, matcher) { + for (path, diff) in old_tree.diff(new_tree, matcher) { + let (before, after) = diff?; if after.is_absent() { stats.removed_files += 1; } else if before.is_absent() { @@ -1226,7 +1227,8 @@ impl TreeState { other => ResetError::InternalBackendError(other), })?; - for (path, _before, after) in old_tree.diff(new_tree, self.sparse_matcher().as_ref()) { + for (path, diff) in old_tree.diff(new_tree, self.sparse_matcher().as_ref()) { + let (_before, after) = diff?; if after.is_absent() { self.file_states.remove(&path); } else { diff --git a/lib/src/merged_tree.rs b/lib/src/merged_tree.rs index 8b701ff383..6e77212999 100644 --- a/lib/src/merged_tree.rs +++ b/lib/src/merged_tree.rs @@ -22,6 +22,7 @@ use std::{iter, vec}; use futures::executor::block_on; use futures::stream::StreamExt; +use futures::TryStreamExt; use itertools::Itertools; use crate::backend::{BackendError, BackendResult, ConflictId, MergedTreeId, TreeId, TreeValue}; @@ -337,11 +338,16 @@ impl MergedTree { /// Collects lists of modified, added, and removed files between this tree /// and another tree. - pub fn diff_summary(&self, other: &MergedTree, matcher: &dyn Matcher) -> DiffSummary { + pub fn diff_summary( + &self, + other: &MergedTree, + matcher: &dyn Matcher, + ) -> BackendResult { let mut modified = vec![]; let mut added = vec![]; let mut removed = vec![]; - for (file, before, after) in self.diff(other, matcher) { + for (file, diff) in self.diff(other, matcher) { + let (before, after) = diff?; if before.is_absent() { added.push(file); } else if after.is_absent() { @@ -353,11 +359,11 @@ impl MergedTree { modified.sort(); added.sort(); removed.sort(); - DiffSummary { + Ok(DiffSummary { modified, added, removed, - } + }) } /// Merges this tree with `other`, using `base` as base. @@ -814,20 +820,28 @@ impl<'matcher> TreeDiffIterator<'matcher> { Self { stack, matcher } } - async fn single_tree(store: &Arc, dir: &RepoPath, value: Option<&TreeValue>) -> Tree { + async fn single_tree( + store: &Arc, + dir: &RepoPath, + value: Option<&TreeValue>, + ) -> BackendResult { match value { - Some(TreeValue::Tree(tree_id)) => store.get_tree_async(dir, tree_id).await.unwrap(), - _ => Tree::null(store.clone(), dir.clone()), + Some(TreeValue::Tree(tree_id)) => store.get_tree_async(dir, tree_id).await, + _ => Ok(Tree::null(store.clone(), dir.clone())), } } /// Gets the given tree if `value` is a tree, otherwise an empty tree. - async fn tree(tree: &MergedTree, dir: &RepoPath, values: &MergedTreeValue) -> MergedTree { + async fn tree( + tree: &MergedTree, + dir: &RepoPath, + values: &MergedTreeValue, + ) -> BackendResult { let trees = if values.is_tree() { let builder: MergeBuilder = futures::stream::iter(values.iter()) .then(|value| Self::single_tree(tree.store(), dir, value.as_ref())) - .collect() - .await; + .try_collect() + .await?; builder.build() } else { Merge::resolved(Tree::null(tree.store().clone(), dir.clone())) @@ -835,8 +849,8 @@ impl<'matcher> TreeDiffIterator<'matcher> { // Maintain the type of tree, so we resolve `TreeValue::Conflict` as necessary // in the subtree match tree { - MergedTree::Legacy(_) => MergedTree::Legacy(trees.into_resolved().unwrap()), - MergedTree::Merge(_) => MergedTree::Merge(trees), + MergedTree::Legacy(_) => Ok(MergedTree::Legacy(trees.into_resolved().unwrap())), + MergedTree::Merge(_) => Ok(MergedTree::Merge(trees)), } } } @@ -857,7 +871,7 @@ impl TreeDiffDirItem { } impl Iterator for TreeDiffIterator<'_> { - type Item = (RepoPath, MergedTreeValue, MergedTreeValue); + type Item = (RepoPath, BackendResult<(MergedTreeValue, MergedTreeValue)>); fn next(&mut self) -> Option { while let Some(top) = self.stack.last_mut() { @@ -872,7 +886,7 @@ impl Iterator for TreeDiffIterator<'_> { } TreeDiffItem::File(..) => { if let TreeDiffItem::File(name, before, after) = self.stack.pop().unwrap() { - return Some((name, before, after)); + return Some((name, Ok((before, after)))); } else { unreachable!(); } @@ -889,6 +903,18 @@ impl Iterator for TreeDiffIterator<'_> { let after_tree = Self::tree(dir.tree2.as_ref(), &path, &after); futures::join!(before_tree, after_tree) }); + let before_tree = match before_tree { + Ok(tree) => tree, + Err(err) => { + return Some((path, Err(err))); + } + }; + let after_tree = match after_tree { + Ok(tree) => tree, + Err(err) => { + return Some((path, Err(err))); + } + }; let subdir = TreeDiffDirItem::new(path.clone(), before_tree, after_tree); self.stack.push(TreeDiffItem::Dir(subdir)); self.stack.len() - 1 @@ -898,7 +924,7 @@ impl Iterator for TreeDiffIterator<'_> { if self.matcher.matches(&path) { if !tree_before && tree_after { if before.is_present() { - return Some((path, before, Merge::absent())); + return Some((path, Ok((before, Merge::absent())))); } } else if tree_before && !tree_after { if after.is_present() { @@ -908,7 +934,7 @@ impl Iterator for TreeDiffIterator<'_> { ); } } else if !tree_before && !tree_after { - return Some((path, before, after)); + return Some((path, Ok((before, after)))); } } } diff --git a/lib/tests/test_commit_builder.rs b/lib/tests/test_commit_builder.rs index 506c3874b2..eef2d099c4 100644 --- a/lib/tests/test_commit_builder.rs +++ b/lib/tests/test_commit_builder.rs @@ -83,7 +83,8 @@ fn test_initial(backend: TestRepoBackend) { .root_commit() .tree() .unwrap() - .diff_summary(&commit.tree().unwrap(), &EverythingMatcher), + .diff_summary(&commit.tree().unwrap(), &EverythingMatcher) + .unwrap(), DiffSummary { modified: vec![], added: vec![dir_file_path, root_file_path], @@ -167,7 +168,8 @@ fn test_rewrite(backend: TestRepoBackend) { .root_commit() .tree() .unwrap() - .diff_summary(&rewritten_commit.tree().unwrap(), &EverythingMatcher), + .diff_summary(&rewritten_commit.tree().unwrap(), &EverythingMatcher) + .unwrap(), DiffSummary { modified: vec![], added: vec![dir_file_path.clone(), root_file_path], @@ -178,7 +180,8 @@ fn test_rewrite(backend: TestRepoBackend) { initial_commit .tree() .unwrap() - .diff_summary(&rewritten_commit.tree().unwrap(), &EverythingMatcher), + .diff_summary(&rewritten_commit.tree().unwrap(), &EverythingMatcher) + .unwrap(), DiffSummary { modified: vec![dir_file_path], added: vec![], diff --git a/lib/tests/test_diff_summary.rs b/lib/tests/test_diff_summary.rs index 1fe2636000..83265b20b3 100644 --- a/lib/tests/test_diff_summary.rs +++ b/lib/tests/test_diff_summary.rs @@ -46,7 +46,7 @@ fn test_types() { ); assert_eq!( - tree1.diff_summary(&tree2, &EverythingMatcher), + tree1.diff_summary(&tree2, &EverythingMatcher).unwrap(), DiffSummary { modified: vec![modified_path], added: vec![added_path], @@ -67,7 +67,7 @@ fn test_tree_file_transition() { let tree2 = create_tree(repo, &[(&dir_path, "contents")]); assert_eq!( - tree1.diff_summary(&tree2, &EverythingMatcher), + tree1.diff_summary(&tree2, &EverythingMatcher).unwrap(), DiffSummary { modified: vec![], added: vec![dir_path.clone()], @@ -75,7 +75,7 @@ fn test_tree_file_transition() { } ); assert_eq!( - tree2.diff_summary(&tree1, &EverythingMatcher), + tree2.diff_summary(&tree1, &EverythingMatcher).unwrap(), DiffSummary { modified: vec![], added: vec![dir_file_path], @@ -124,7 +124,7 @@ fn test_sorting() { ); assert_eq!( - tree1.diff_summary(&tree2, &EverythingMatcher), + tree1.diff_summary(&tree2, &EverythingMatcher).unwrap(), DiffSummary { modified: vec![a_path.clone(), f_a_path.clone(), f_f_a_path.clone()], added: vec![ @@ -139,7 +139,7 @@ fn test_sorting() { } ); assert_eq!( - tree2.diff_summary(&tree1, &EverythingMatcher), + tree2.diff_summary(&tree1, &EverythingMatcher).unwrap(), DiffSummary { modified: vec![a_path, f_a_path, f_f_a_path], added: vec![], @@ -161,7 +161,7 @@ fn test_matcher_dir_file_transition() { let matcher = FilesMatcher::new(&[a_path.clone()]); assert_eq!( - tree1.diff_summary(&tree2, &matcher), + tree1.diff_summary(&tree2, &matcher).unwrap(), DiffSummary { modified: vec![], added: vec![], @@ -169,7 +169,7 @@ fn test_matcher_dir_file_transition() { } ); assert_eq!( - tree2.diff_summary(&tree1, &matcher), + tree2.diff_summary(&tree1, &matcher).unwrap(), DiffSummary { modified: vec![], added: vec![a_path.clone()], @@ -179,7 +179,7 @@ fn test_matcher_dir_file_transition() { let matcher = FilesMatcher::new(&[a_a_path.clone()]); assert_eq!( - tree1.diff_summary(&tree2, &matcher), + tree1.diff_summary(&tree2, &matcher).unwrap(), DiffSummary { modified: vec![], added: vec![a_a_path.clone()], @@ -187,7 +187,7 @@ fn test_matcher_dir_file_transition() { } ); assert_eq!( - tree2.diff_summary(&tree1, &matcher), + tree2.diff_summary(&tree1, &matcher).unwrap(), DiffSummary { modified: vec![], added: vec![], @@ -197,7 +197,7 @@ fn test_matcher_dir_file_transition() { let matcher = FilesMatcher::new(&[a_path.clone(), a_a_path.clone()]); assert_eq!( - tree1.diff_summary(&tree2, &matcher), + tree1.diff_summary(&tree2, &matcher).unwrap(), DiffSummary { modified: vec![], added: vec![a_a_path.clone()], @@ -205,7 +205,7 @@ fn test_matcher_dir_file_transition() { } ); assert_eq!( - tree2.diff_summary(&tree1, &matcher), + tree2.diff_summary(&tree1, &matcher).unwrap(), DiffSummary { modified: vec![], added: vec![a_path], @@ -241,7 +241,7 @@ fn test_matcher_normal_cases() { let matcher = FilesMatcher::new(&[a_path.clone(), z_path.clone()]); assert_eq!( - tree1.diff_summary(&tree2, &matcher), + tree1.diff_summary(&tree2, &matcher).unwrap(), DiffSummary { modified: vec![a_path.clone()], added: vec![z_path.clone()], @@ -249,7 +249,7 @@ fn test_matcher_normal_cases() { } ); assert_eq!( - tree2.diff_summary(&tree1, &matcher), + tree2.diff_summary(&tree1, &matcher).unwrap(), DiffSummary { modified: vec![a_path], added: vec![], @@ -259,7 +259,7 @@ fn test_matcher_normal_cases() { let matcher = FilesMatcher::new(&[dir1_a_path.clone(), dir2_b_path.clone()]); assert_eq!( - tree1.diff_summary(&tree2, &matcher), + tree1.diff_summary(&tree2, &matcher).unwrap(), DiffSummary { modified: vec![dir1_a_path.clone()], added: vec![dir2_b_path.clone()], @@ -267,7 +267,7 @@ fn test_matcher_normal_cases() { } ); assert_eq!( - tree2.diff_summary(&tree1, &matcher), + tree2.diff_summary(&tree1, &matcher).unwrap(), DiffSummary { modified: vec![dir1_a_path], added: vec![], diff --git a/lib/tests/test_merged_tree.rs b/lib/tests/test_merged_tree.rs index 0390e5666b..c559f4abdd 100644 --- a/lib/tests/test_merged_tree.rs +++ b/lib/tests/test_merged_tree.rs @@ -681,30 +681,37 @@ fn test_diff_resolved() { let diff = before_merged .diff(&after_merged, &EverythingMatcher) + .map(|(path, diff)| (path, diff.unwrap())) .collect_vec(); assert_eq!(diff.len(), 3); assert_eq!( diff[0].clone(), ( modified_path.clone(), - Merge::resolved(before.path_value(&modified_path)), - Merge::resolved(after.path_value(&modified_path)), + ( + Merge::resolved(before.path_value(&modified_path)), + Merge::resolved(after.path_value(&modified_path)) + ), ) ); assert_eq!( diff[1].clone(), ( removed_path.clone(), - Merge::resolved(before.path_value(&removed_path)), - Merge::absent(), + ( + Merge::resolved(before.path_value(&removed_path)), + Merge::absent() + ), ) ); assert_eq!( diff[2].clone(), ( added_path.clone(), - Merge::absent(), - Merge::resolved(after.path_value(&added_path)), + ( + Merge::absent(), + Merge::resolved(after.path_value(&added_path)) + ), ) ); } @@ -786,14 +793,14 @@ fn test_diff_conflicted() { // Test the forwards diff let actual_diff = left_merged .diff(&right_merged, &EverythingMatcher) + .map(|(path, diff)| (path, diff.unwrap())) .collect_vec(); let expected_diff = [&path2, &path3, &path4] .iter() .map(|path| { ( (*path).clone(), - left_merged.path_value(path), - right_merged.path_value(path), + (left_merged.path_value(path), right_merged.path_value(path)), ) }) .collect_vec(); @@ -801,14 +808,14 @@ fn test_diff_conflicted() { // Test the reverse diff let actual_diff = right_merged .diff(&left_merged, &EverythingMatcher) + .map(|(path, diff)| (path, diff.unwrap())) .collect_vec(); let expected_diff = [&path2, &path3, &path4] .iter() .map(|path| { ( (*path).clone(), - right_merged.path_value(path), - left_merged.path_value(path), + (right_merged.path_value(path), left_merged.path_value(path)), ) }) .collect_vec(); @@ -911,136 +918,122 @@ fn test_diff_dir_file() { // Test the forwards diff let actual_diff = left_merged .diff(&right_merged, &EverythingMatcher) + .map(|(path, diff)| (path, diff.unwrap())) .collect_vec(); let expected_diff = vec![ // path1: file1 -> directory1 ( path1.clone(), - left_merged.path_value(&path1), - Merge::absent(), + (left_merged.path_value(&path1), Merge::absent()), ), ( path1.join(&file), - Merge::absent(), - right_merged.path_value(&path1.join(&file)), + (Merge::absent(), right_merged.path_value(&path1.join(&file))), ), // path2: file1 -> directory1+(directory2-absent) ( path2.clone(), - left_merged.path_value(&path2), - Merge::absent(), + (left_merged.path_value(&path2), Merge::absent()), ), ( path2.join(&file), - Merge::absent(), - right_merged.path_value(&path2.join(&file)), + (Merge::absent(), right_merged.path_value(&path2.join(&file))), ), // path3: file1 -> directory1+(file1-absent) ( path3.clone(), - left_merged.path_value(&path3), - right_merged.path_value(&path3), + ( + left_merged.path_value(&path3), + right_merged.path_value(&path3), + ), ), // path4: file1+(file2-file3) -> directory1+(directory2-directory3) ( path4.clone(), - left_merged.path_value(&path4), - Merge::absent(), + (left_merged.path_value(&path4), Merge::absent()), ), ( path4.join(&file), - Merge::absent(), - right_merged.path_value(&path4.join(&file)), + (Merge::absent(), right_merged.path_value(&path4.join(&file))), ), // path5: directory1 -> file1+(file2-absent) ( path5.join(&file), - left_merged.path_value(&path5.join(&file)), - Merge::absent(), + (left_merged.path_value(&path5.join(&file)), Merge::absent()), ), ( path5.clone(), - Merge::absent(), - right_merged.path_value(&path5), + (Merge::absent(), right_merged.path_value(&path5)), ), // path6: directory1 -> file1+(directory1-absent) ( path6.join(&file), - left_merged.path_value(&path6.join(&file)), - Merge::absent(), + (left_merged.path_value(&path6.join(&file)), Merge::absent()), ), ( path6.clone(), - Merge::absent(), - right_merged.path_value(&path6), + (Merge::absent(), right_merged.path_value(&path6)), ), ]; assert_eq!(actual_diff, expected_diff); // Test the reverse diff let actual_diff = right_merged .diff(&left_merged, &EverythingMatcher) + .map(|(path, diff)| (path, diff.unwrap())) .collect_vec(); let expected_diff = vec![ // path1: file1 -> directory1 ( path1.join(&file), - right_merged.path_value(&path1.join(&file)), - Merge::absent(), + (right_merged.path_value(&path1.join(&file)), Merge::absent()), ), ( path1.clone(), - Merge::absent(), - left_merged.path_value(&path1), + (Merge::absent(), left_merged.path_value(&path1)), ), // path2: file1 -> directory1+(directory2-absent) ( path2.join(&file), - right_merged.path_value(&path2.join(&file)), - Merge::absent(), + (right_merged.path_value(&path2.join(&file)), Merge::absent()), ), ( path2.clone(), - Merge::absent(), - left_merged.path_value(&path2), + (Merge::absent(), left_merged.path_value(&path2)), ), // path3: file1 -> directory1+(file1-absent) ( path3.clone(), - right_merged.path_value(&path3), - left_merged.path_value(&path3), + ( + right_merged.path_value(&path3), + left_merged.path_value(&path3), + ), ), // path4: file1+(file2-file3) -> directory1+(directory2-directory3) ( path4.join(&file), - right_merged.path_value(&path4.join(&file)), - Merge::absent(), + (right_merged.path_value(&path4.join(&file)), Merge::absent()), ), ( path4.clone(), - Merge::absent(), - left_merged.path_value(&path4), + (Merge::absent(), left_merged.path_value(&path4)), ), // path5: directory1 -> file1+(file2-absent) ( path5.clone(), - right_merged.path_value(&path5), - Merge::absent(), + (right_merged.path_value(&path5), Merge::absent()), ), ( path5.join(&file), - Merge::absent(), - left_merged.path_value(&path5.join(&file)), + (Merge::absent(), left_merged.path_value(&path5.join(&file))), ), // path6: directory1 -> file1+(directory1-absent) ( path6.clone(), - right_merged.path_value(&path6), - Merge::absent(), + (right_merged.path_value(&path6), Merge::absent()), ), ( path6.join(&file), - Merge::absent(), - left_merged.path_value(&path6.join(&file)), + (Merge::absent(), left_merged.path_value(&path6.join(&file))), ), ]; assert_eq!(actual_diff, expected_diff);