From f4a41f38803244b2accdbb46782faf0765e0577e Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Wed, 7 Apr 2021 11:27:40 -0700 Subject: [PATCH] trees: make tree diff return an iterator instead of taking a callback This is yet another step towards making it easy to propagate `BrokenPipe` errors. The `jj diff` code (naturally) diffs two trees and prints the diffs. If the printing fails, we shouldn't just crash like we do today. The new code is probably slower since it does more copying (the callback got references to the `FileRepoPath` and `TreeValue`). I hope that won't make a noticeable difference. At least `jj diff -r 334afbc76fbd --summary` didn't seem to get measurably slower. --- lib/src/tree.rs | 19 ++-- lib/src/trees.rs | 190 ++++++++++++++++++------------- lib/src/working_copy.rs | 30 ++--- src/commands.rs | 246 ++++++++++++++++++++-------------------- src/diff_edit.rs | 4 +- 5 files changed, 260 insertions(+), 229 deletions(-) diff --git a/lib/src/tree.rs b/lib/src/tree.rs index 599fbf448..ac89b9451 100644 --- a/lib/src/tree.rs +++ b/lib/src/tree.rs @@ -17,14 +17,13 @@ use std::fmt::{Debug, Error, Formatter}; use std::pin::Pin; use std::sync::Arc; -use crate::matchers::AlwaysMatcher; use crate::repo_path::{ DirRepoPath, DirRepoPathComponent, FileRepoPath, RepoPath, RepoPathComponent, RepoPathJoin, }; use crate::store; use crate::store::{ConflictId, TreeEntriesNonRecursiveIter, TreeEntry, TreeId, TreeValue}; use crate::store_wrapper::StoreWrapper; -use crate::trees::{recursive_tree_diff, TreeValueDiff}; +use crate::trees::{recursive_tree_diff, Diff, TreeDiffIterator}; #[derive(Clone)] pub struct Tree { @@ -165,19 +164,21 @@ impl Tree { } } - pub fn diff(&self, other: &Tree, callback: &mut impl FnMut(&FileRepoPath, TreeValueDiff)) { - recursive_tree_diff(self.clone(), other.clone(), &AlwaysMatcher {}, callback); + pub fn diff<'a>(&'a self, other: &'a Tree) -> TreeDiffIterator { + recursive_tree_diff(self.clone(), other.clone()) } pub fn diff_summary(&self, other: &Tree) -> DiffSummary { let mut modified = vec![]; let mut added = vec![]; let mut removed = vec![]; - self.diff(other, &mut |file, diff| match diff { - TreeValueDiff::Modified(_, _) => modified.push(file.clone()), - TreeValueDiff::Added(_) => added.push(file.clone()), - TreeValueDiff::Removed(_) => removed.push(file.clone()), - }); + for (file, diff) in self.diff(other) { + match diff { + Diff::Modified(_, _) => modified.push(file.clone()), + Diff::Added(_) => added.push(file.clone()), + Diff::Removed(_) => removed.push(file.clone()), + } + } modified.sort(); added.sort(); removed.sort(); diff --git a/lib/src/trees.rs b/lib/src/trees.rs index 3ff2cf0ec..69a70c744 100644 --- a/lib/src/trees.rs +++ b/lib/src/trees.rs @@ -14,10 +14,10 @@ use std::cmp::Ordering; use std::iter::Peekable; +use std::pin::Pin; use crate::files; use crate::files::MergeResult; -use crate::matchers::Matcher; use crate::repo_path::{ DirRepoPath, DirRepoPathComponent, FileRepoPath, FileRepoPathComponent, RepoPathJoin, }; @@ -124,91 +124,119 @@ fn diff_entries<'a>(tree1: &'a Tree, tree2: &'a Tree) -> TreeEntryDiffIterator<' TreeEntryDiffIterator::new(tree1, tree2) } -pub fn recursive_tree_diff( - root1: Tree, - root2: Tree, - matcher: &M, - callback: &mut impl FnMut(&FileRepoPath, TreeValueDiff), -) where - M: Matcher, -{ - internal_recursive_tree_diff(vec![(DirRepoPath::root(), root1, root2)], matcher, callback) +pub fn recursive_tree_diff(root1: Tree, root2: Tree) -> TreeDiffIterator { + TreeDiffIterator::new(DirRepoPath::root(), root1, root2) } -fn internal_recursive_tree_diff( - work: Vec<(DirRepoPath, Tree, Tree)>, - _matcher: &M, - callback: &mut impl FnMut(&FileRepoPath, TreeValueDiff), -) where - M: Matcher, -{ - let mut new_work = Vec::new(); - // Diffs for which to invoke the callback after having visited subtrees. This is - // used for making sure that when a directory gets replaced by a file, we - // call the callback for the addition of the file after we call the callback +pub struct TreeDiffIterator { + dir: DirRepoPath, + tree1: Pin>, + tree2: Pin>, + // Iterator over the diffs between tree1 and tree2 + entry_iterator: TreeEntryDiffIterator<'static>, + // This is used for making sure that when a directory gets replaced by a file, we + // yield the value for the addition of the file after we yield the values // for removing files in the directory. - let mut late_file_diffs = Vec::new(); - for (dir, tree1, tree2) in &work { - for (name, diff) in diff_entries(tree1, tree2) { - let file_path = dir.join(&FileRepoPathComponent::from(name.as_str())); - let subdir = DirRepoPathComponent::from(name.as_str()); - let subdir_path = dir.join(&subdir); - // TODO: simplify this mess - match diff { - TreeValueDiff::Modified(TreeValue::Tree(id_before), TreeValue::Tree(id_after)) => { - new_work.push(( - subdir_path, - tree1.known_sub_tree(&subdir, &id_before), - tree2.known_sub_tree(&subdir, &id_after), - )); - } - TreeValueDiff::Modified(TreeValue::Tree(id_before), file_after) => { - new_work.push(( - subdir_path.clone(), - tree1.known_sub_tree(&subdir, &id_before), - Tree::null(tree2.store().clone(), subdir_path), - )); - late_file_diffs.push((file_path, TreeValueDiff::Added(file_after))); - } - TreeValueDiff::Modified(file_before, TreeValue::Tree(id_after)) => { - new_work.push(( - subdir_path.clone(), - Tree::null(tree1.store().clone(), subdir_path), - tree2.known_sub_tree(&subdir, &id_after), - )); - callback(&file_path, TreeValueDiff::Removed(file_before)); - } - TreeValueDiff::Modified(_, _) => { - callback(&file_path, diff); - } - TreeValueDiff::Added(TreeValue::Tree(id_after)) => { - new_work.push(( - subdir_path.clone(), - Tree::null(tree1.store().clone(), subdir_path), - tree2.known_sub_tree(&subdir, &id_after), - )); - } - TreeValueDiff::Added(_) => { - callback(&file_path, diff); - } - TreeValueDiff::Removed(TreeValue::Tree(id_before)) => { - new_work.push(( - subdir_path.clone(), - tree1.known_sub_tree(&subdir, &id_before), - Tree::null(tree2.store().clone(), subdir_path), - )); - } - TreeValueDiff::Removed(_) => { - callback(&file_path, diff); - } - } + added_file: Option<(FileRepoPath, TreeValue)>, + // Iterator over the diffs of a subdirectory, if we're currently visiting one. + subdir_iterator: Option>, +} + +impl TreeDiffIterator { + fn new(dir: DirRepoPath, tree1: Tree, tree2: Tree) -> TreeDiffIterator { + let tree1 = Box::pin(tree1); + let tree2 = Box::pin(tree2); + let root_entry_iterator: TreeEntryDiffIterator = diff_entries(&tree1, &tree2); + let root_entry_iterator: TreeEntryDiffIterator<'static> = + unsafe { std::mem::transmute(root_entry_iterator) }; + Self { + dir, + tree1, + tree2, + entry_iterator: root_entry_iterator, + added_file: None, + subdir_iterator: None, } } - if !new_work.is_empty() { - internal_recursive_tree_diff(new_work, _matcher, callback) - } - for (file_path, diff) in late_file_diffs { - callback(&file_path, diff); +} + +impl Iterator for TreeDiffIterator { + type Item = (FileRepoPath, Diff); + + fn next(&mut self) -> Option { + loop { + // First return results from any subdirectory we're currently visiting. + if let Some(subdir_iterator) = &mut self.subdir_iterator { + if let Some(element) = subdir_iterator.next() { + return Some(element); + } + } + + if let Some((name, value)) = self.added_file.take() { + return Some((name, Diff::Added(value))); + } + + // Note: whenever we say "file" below, it may also be a symlink or a conflict. + if let Some((name, diff)) = self.entry_iterator.next() { + let file_path = self.dir.join(&FileRepoPathComponent::from(name.as_str())); + let subdir = DirRepoPathComponent::from(name.as_str()); + let subdir_path = self.dir.join(&subdir); + // TODO: simplify this mess + match diff { + Diff::Modified(TreeValue::Tree(id_before), TreeValue::Tree(id_after)) => { + self.subdir_iterator = Some(Box::new(TreeDiffIterator::new( + subdir_path, + self.tree1.known_sub_tree(&subdir, &id_before), + self.tree2.known_sub_tree(&subdir, &id_after), + ))); + } + Diff::Modified(TreeValue::Tree(id_before), file_after) => { + self.subdir_iterator = Some(Box::new(TreeDiffIterator::new( + subdir_path.clone(), + self.tree1.known_sub_tree(&subdir, &id_before), + Tree::null(self.tree2.store().clone(), subdir_path), + ))); + self.added_file = Some((file_path, file_after.clone())); + } + Diff::Modified(file_before, TreeValue::Tree(id_after)) => { + self.subdir_iterator = Some(Box::new(TreeDiffIterator::new( + subdir_path.clone(), + Tree::null(self.tree1.store().clone(), subdir_path), + self.tree2.known_sub_tree(&subdir, &id_after), + ))); + return Some((file_path, Diff::Removed(file_before.clone()))); + } + Diff::Modified(file_before, file_after) => { + return Some(( + file_path, + Diff::Modified(file_before.clone(), file_after.clone()), + )); + } + Diff::Added(TreeValue::Tree(id_after)) => { + self.subdir_iterator = Some(Box::new(TreeDiffIterator::new( + subdir_path.clone(), + Tree::null(self.tree1.store().clone(), subdir_path), + self.tree2.known_sub_tree(&subdir, &id_after), + ))); + } + Diff::Added(value_after) => { + return Some((file_path, Diff::Added(value_after.clone()))); + } + Diff::Removed(TreeValue::Tree(id_before)) => { + self.subdir_iterator = Some(Box::new(TreeDiffIterator::new( + subdir_path.clone(), + self.tree1.known_sub_tree(&subdir, &id_before), + Tree::null(self.tree2.store().clone(), subdir_path), + ))); + } + Diff::Removed(value_before) => { + return Some((file_path, Diff::Removed(value_before.clone()))); + } + } + } else { + return None; + } + } } } diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs index fb1e7eeac..ea2b988eb 100644 --- a/lib/src/working_copy.rs +++ b/lib/src/working_copy.rs @@ -42,7 +42,7 @@ use crate::repo_path::{ use crate::settings::UserSettings; use crate::store::{CommitId, FileId, MillisSinceEpoch, StoreError, SymlinkId, TreeId, TreeValue}; use crate::store_wrapper::StoreWrapper; -use crate::trees::TreeValueDiff; +use crate::trees::Diff; #[derive(Debug, PartialEq, Eq, Clone)] pub enum FileType { @@ -470,14 +470,14 @@ impl TreeState { removed_files: 0, }; - old_tree.diff(&new_tree, &mut |path, diff| { + for (path, diff) in old_tree.diff(&new_tree) { let disk_path = self .working_copy_path .join(PathBuf::from(path.to_internal_string())); // TODO: Check that the file has not changed before overwriting/removing it. match diff { - TreeValueDiff::Removed(_before) => { + Diff::Removed(_before) => { fs::remove_file(&disk_path).ok(); let mut parent_dir = disk_path.parent().unwrap(); loop { @@ -489,15 +489,15 @@ impl TreeState { self.file_states.remove(&path); stats.removed_files += 1; } - TreeValueDiff::Added(after) => { + Diff::Added(after) => { let file_state = match after { TreeValue::Normal { id, executable } => { - self.write_file(&disk_path, path, id, *executable) + self.write_file(&disk_path, &path, &id, executable) } - TreeValue::Symlink(id) => self.write_symlink(&disk_path, path, id), + TreeValue::Symlink(id) => self.write_symlink(&disk_path, &path, &id), TreeValue::GitSubmodule(_id) => { println!("ignoring git submodule at {:?}", path); - return; + continue; } TreeValue::Tree(_id) => { panic!("unexpected tree entry in diff at {:?}", path); @@ -512,7 +512,7 @@ impl TreeState { self.file_states.insert(path.clone(), file_state); stats.added_files += 1; } - TreeValueDiff::Modified(before, after) => { + Diff::Modified(before, after) => { fs::remove_file(&disk_path).ok(); let file_state = match (before, after) { ( @@ -524,19 +524,19 @@ impl TreeState { ) if id == old_id => { // Optimization for when only the executable bit changed assert_ne!(executable, old_executable); - self.set_executable(&disk_path, *executable); + self.set_executable(&disk_path, executable); let mut file_state = self.file_states.get(&path).unwrap().clone(); - file_state.mark_executable(*executable); + file_state.mark_executable(executable); file_state } (_, TreeValue::Normal { id, executable }) => { - self.write_file(&disk_path, path, id, *executable) + self.write_file(&disk_path, &path, &id, executable) } - (_, TreeValue::Symlink(id)) => self.write_symlink(&disk_path, path, id), + (_, TreeValue::Symlink(id)) => self.write_symlink(&disk_path, &path, &id), (_, TreeValue::GitSubmodule(_id)) => { println!("ignoring git submodule at {:?}", path); - self.file_states.remove(path); - return; + self.file_states.remove(&path); + continue; } (_, TreeValue::Tree(_id)) => { panic!("unexpected tree entry in diff at {:?}", path); @@ -553,7 +553,7 @@ impl TreeState { stats.updated_files += 1; } } - }); + } self.tree_id = tree_id; self.save(); Ok(stats) diff --git a/src/commands.rs b/src/commands.rs index df13f0d41..7771fc7cd 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -44,7 +44,7 @@ use jujube_lib::settings::UserSettings; use jujube_lib::store::{CommitId, StoreError, Timestamp, TreeValue}; use jujube_lib::store_wrapper::StoreWrapper; use jujube_lib::tree::Tree; -use jujube_lib::trees::TreeValueDiff; +use jujube_lib::trees::Diff; use jujube_lib::working_copy::{CheckoutStats, WorkingCopy}; use jujube_lib::{conflicts, files, git}; use pest::Parser; @@ -777,7 +777,6 @@ fn cmd_diff( } let mut repo = get_repo(ui, &matches)?; let mut_repo = Arc::get_mut(&mut repo).unwrap(); - if sub_matches.is_present("from") || sub_matches.is_present("to") {} let from_tree; let to_tree; if sub_matches.is_present("from") || sub_matches.is_present("to") { @@ -800,136 +799,139 @@ fn cmd_diff( } else { let mut styler = ui.styler(); styler.add_label(String::from("diff")); - from_tree.diff(&to_tree, &mut |path, diff| match diff { - TreeValueDiff::Added(TreeValue::Normal { - id, - executable: false, - }) => { - styler.add_label(String::from("header")); - styler.write_str(&format!("added file {}:\n", path.to_internal_string())); - styler.remove_label(); + for (path, diff) in from_tree.diff(&to_tree) { + match diff { + Diff::Added(TreeValue::Normal { + id, + executable: false, + }) => { + styler.add_label(String::from("header")); + styler.write_str(&format!("added file {}:\n", path.to_internal_string())); + styler.remove_label(); - let mut file_reader = repo.store().read_file(path, id).unwrap(); - styler.write_from_reader(&mut file_reader); - } - TreeValueDiff::Modified( - TreeValue::Normal { - id: id_left, - executable: left_executable, - }, - TreeValue::Normal { - id: id_right, - executable: right_executable, - }, - ) if left_executable == right_executable => { - styler.add_label(String::from("header")); - if *left_executable { + let mut file_reader = repo.store().read_file(&path, &id).unwrap(); + styler.write_from_reader(&mut file_reader); + } + Diff::Modified( + TreeValue::Normal { + id: id_left, + executable: left_executable, + }, + TreeValue::Normal { + id: id_right, + executable: right_executable, + }, + ) if left_executable == right_executable => { + styler.add_label(String::from("header")); + if left_executable { + styler.write_str(&format!( + "modified executable file {}:\n", + path.to_internal_string() + )); + } else { + styler + .write_str(&format!("modified file {}:\n", path.to_internal_string())); + } + styler.remove_label(); + + let mut file_reader_left = repo.store().read_file(&path, &id_left).unwrap(); + let mut buffer_left = vec![]; + file_reader_left.read_to_end(&mut buffer_left).unwrap(); + let mut file_reader_right = repo.store().read_file(&path, &id_right).unwrap(); + let mut buffer_right = vec![]; + file_reader_right.read_to_end(&mut buffer_right).unwrap(); + + print_diff( + buffer_left.as_slice(), + buffer_right.as_slice(), + styler.as_mut(), + ); + } + Diff::Modified( + TreeValue::Conflict(id_left), + TreeValue::Normal { + id: id_right, + executable: false, + }, + ) => { + styler.add_label(String::from("header")); styler.write_str(&format!( - "modified executable file {}:\n", + "resolved conflict in file {}:\n", path.to_internal_string() )); - } else { - styler.write_str(&format!("modified file {}:\n", path.to_internal_string())); + styler.remove_label(); + + let conflict_left = repo.store().read_conflict(&id_left).unwrap(); + let mut buffer_left = vec![]; + conflicts::materialize_conflict( + repo.store(), + &path.to_repo_path(), + &conflict_left, + &mut buffer_left, + ); + let mut file_reader_right = repo.store().read_file(&path, &id_right).unwrap(); + let mut buffer_right = vec![]; + file_reader_right.read_to_end(&mut buffer_right).unwrap(); + + print_diff( + buffer_left.as_slice(), + buffer_right.as_slice(), + styler.as_mut(), + ); } - styler.remove_label(); + Diff::Modified( + TreeValue::Normal { + id: id_left, + executable: false, + }, + TreeValue::Conflict(id_right), + ) => { + styler.add_label(String::from("header")); + styler.write_str(&format!( + "new conflict in file {}:\n", + path.to_internal_string() + )); + styler.remove_label(); - let mut file_reader_left = repo.store().read_file(path, id_left).unwrap(); - let mut buffer_left = vec![]; - file_reader_left.read_to_end(&mut buffer_left).unwrap(); - let mut file_reader_right = repo.store().read_file(path, id_right).unwrap(); - let mut buffer_right = vec![]; - file_reader_right.read_to_end(&mut buffer_right).unwrap(); + let mut file_reader_left = repo.store().read_file(&path, &id_left).unwrap(); + let mut buffer_left = vec![]; + file_reader_left.read_to_end(&mut buffer_left).unwrap(); + let conflict_right = repo.store().read_conflict(&id_right).unwrap(); + let mut buffer_right = vec![]; + conflicts::materialize_conflict( + repo.store(), + &path.to_repo_path(), + &conflict_right, + &mut buffer_right, + ); - print_diff( - buffer_left.as_slice(), - buffer_right.as_slice(), - styler.as_mut(), - ); - } - TreeValueDiff::Modified( - TreeValue::Conflict(id_left), - TreeValue::Normal { - id: id_right, + print_diff( + buffer_left.as_slice(), + buffer_right.as_slice(), + styler.as_mut(), + ); + } + Diff::Removed(TreeValue::Normal { + id, executable: false, - }, - ) => { - styler.add_label(String::from("header")); - styler.write_str(&format!( - "resolved conflict in file {}:\n", - path.to_internal_string() - )); - styler.remove_label(); + }) => { + styler.add_label(String::from("header")); + styler.write_str(&format!("removed file {}:\n", path.to_internal_string())); + styler.remove_label(); - let conflict_left = repo.store().read_conflict(id_left).unwrap(); - let mut buffer_left = vec![]; - conflicts::materialize_conflict( - repo.store(), - &path.to_repo_path(), - &conflict_left, - &mut buffer_left, - ); - let mut file_reader_right = repo.store().read_file(path, id_right).unwrap(); - let mut buffer_right = vec![]; - file_reader_right.read_to_end(&mut buffer_right).unwrap(); - - print_diff( - buffer_left.as_slice(), - buffer_right.as_slice(), - styler.as_mut(), - ); + let mut file_reader = repo.store().read_file(&path, &id).unwrap(); + styler.write_from_reader(&mut file_reader); + } + other => { + writeln!( + styler, + "unhandled diff case in path {:?}: {:?}", + path, other + ) + .unwrap(); + } } - TreeValueDiff::Modified( - TreeValue::Normal { - id: id_left, - executable: false, - }, - TreeValue::Conflict(id_right), - ) => { - styler.add_label(String::from("header")); - styler.write_str(&format!( - "new conflict in file {}:\n", - path.to_internal_string() - )); - styler.remove_label(); - - let mut file_reader_left = repo.store().read_file(path, id_left).unwrap(); - let mut buffer_left = vec![]; - file_reader_left.read_to_end(&mut buffer_left).unwrap(); - let conflict_right = repo.store().read_conflict(id_right).unwrap(); - let mut buffer_right = vec![]; - conflicts::materialize_conflict( - repo.store(), - &path.to_repo_path(), - &conflict_right, - &mut buffer_right, - ); - - print_diff( - buffer_left.as_slice(), - buffer_right.as_slice(), - styler.as_mut(), - ); - } - TreeValueDiff::Removed(TreeValue::Normal { - id, - executable: false, - }) => { - styler.add_label(String::from("header")); - styler.write_str(&format!("removed file {}:\n", path.to_internal_string())); - styler.remove_label(); - - let mut file_reader = repo.store().read_file(path, id).unwrap(); - styler.write_from_reader(&mut file_reader); - } - other => { - writeln!( - styler, - "unhandled diff case in path {:?}: {:?}", - path, other - ) - .unwrap(); - } - }); + } styler.remove_label(); } Ok(()) diff --git a/src/diff_edit.rs b/src/diff_edit.rs index 8b136c8d1..c5f9d9015 100644 --- a/src/diff_edit.rs +++ b/src/diff_edit.rs @@ -98,7 +98,7 @@ pub fn edit_diff(left_tree: &Tree, right_tree: &Tree) -> Result Result