From dd4c47f37339b8dc86c429c002a8a22e5cd90537 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sat, 5 Jun 2021 14:29:40 -0700 Subject: [PATCH] tree: support filtering diff by matcher This change teaches `Tree::diff()` to filter by a matcher. It only filters the result so far; it does not restrict the tree walk to what `Matcher::visit()` says is necessary yet. It also doesn't teach the CLI to create a matcher and pass it in. --- lib/src/matchers.rs | 16 ++-- lib/src/tree.rs | 137 +++++++++++++++++------------ lib/src/working_copy.rs | 3 +- lib/tests/test_commit_builder.rs | 12 ++- lib/tests/test_diff_summary.rs | 142 +++++++++++++++++++++++++++++-- lib/tests/test_working_copy.rs | 5 +- src/commands.rs | 9 +- src/diff_edit.rs | 3 +- 8 files changed, 252 insertions(+), 75 deletions(-) diff --git a/lib/src/matchers.rs b/lib/src/matchers.rs index 9110cbbb1..4da62ea40 100644 --- a/lib/src/matchers.rs +++ b/lib/src/matchers.rs @@ -19,21 +19,21 @@ use std::collections::{HashMap, HashSet}; use crate::repo_path::{RepoPath, RepoPathComponent}; #[derive(PartialEq, Eq, Debug)] -pub struct Visit<'a> { - dirs: VisitDirs<'a>, - files: VisitFiles<'a>, +pub struct Visit<'matcher> { + pub dirs: VisitDirs<'matcher>, + pub files: VisitFiles<'matcher>, } #[derive(PartialEq, Eq, Debug)] -pub enum VisitDirs<'a> { +pub enum VisitDirs<'matcher> { All, - Set(&'a HashSet), + Set(&'matcher HashSet), } #[derive(PartialEq, Eq, Debug)] -pub enum VisitFiles<'a> { +pub enum VisitFiles<'matcher> { All, - Set(&'a HashSet), + Set(&'matcher HashSet), } pub trait Matcher { @@ -64,7 +64,7 @@ pub struct FilesMatcher { } impl FilesMatcher { - fn new(files: HashSet) -> Self { + pub fn new(files: HashSet) -> Self { let mut dirs = Dirs::new(); for f in &files { dirs.add_file(f); diff --git a/lib/src/tree.rs b/lib/src/tree.rs index 3f45d09da..0a60295db 100644 --- a/lib/src/tree.rs +++ b/lib/src/tree.rs @@ -20,6 +20,7 @@ use std::pin::Pin; use std::sync::Arc; use crate::files::MergeResult; +use crate::matchers::{EverythingMatcher, Matcher}; use crate::repo_path::{RepoPath, RepoPathComponent, RepoPathJoin}; use crate::store::{ Conflict, ConflictId, ConflictPart, StoreError, TreeEntriesNonRecursiveIter, TreeEntry, TreeId, @@ -164,15 +165,19 @@ impl Tree { } } - pub fn diff<'a>(&'a self, other: &'a Tree) -> TreeDiffIterator { - recursive_tree_diff(self.clone(), other.clone()) + pub fn diff<'matcher>( + &self, + other: &Tree, + matcher: &'matcher dyn Matcher, + ) -> TreeDiffIterator<'matcher> { + recursive_tree_diff(self.clone(), other.clone(), matcher) } - pub fn diff_summary(&self, other: &Tree) -> DiffSummary { + pub fn diff_summary(&self, other: &Tree, matcher: &dyn Matcher) -> DiffSummary { let mut modified = vec![]; let mut added = vec![]; let mut removed = vec![]; - for (file, diff) in self.diff(other) { + for (file, diff) in self.diff(other, matcher) { match diff { Diff::Modified(_, _) => modified.push(file.clone()), Diff::Added(_) => added.push(file.clone()), @@ -269,24 +274,30 @@ impl Diff { } } -struct TreeEntryDiffIterator<'a> { - it1: Peekable>, - it2: Peekable>, +struct TreeEntryDiffIterator<'trees, 'matcher> { + it1: Peekable>, + it2: Peekable>, + // TODO: Restrict walk according to Matcher::visit() + _matcher: &'matcher dyn Matcher, } -impl<'a> TreeEntryDiffIterator<'a> { - fn new(tree1: &'a Tree, tree2: &'a Tree) -> Self { +impl<'trees, 'matcher> TreeEntryDiffIterator<'trees, 'matcher> { + fn new(tree1: &'trees Tree, tree2: &'trees Tree, matcher: &'matcher dyn Matcher) -> Self { let it1 = tree1.entries_non_recursive().peekable(); let it2 = tree2.entries_non_recursive().peekable(); - TreeEntryDiffIterator { it1, it2 } + TreeEntryDiffIterator { + it1, + it2, + _matcher: matcher, + } } } -impl<'a> Iterator for TreeEntryDiffIterator<'a> { +impl<'trees, 'matcher> Iterator for TreeEntryDiffIterator<'trees, 'matcher> { type Item = ( RepoPathComponent, - Option<&'a TreeValue>, - Option<&'a TreeValue>, + Option<&'trees TreeValue>, + Option<&'trees TreeValue>, ); fn next(&mut self) -> Option { @@ -339,39 +350,54 @@ impl<'a> Iterator for TreeEntryDiffIterator<'a> { } } -fn diff_entries<'a>(tree1: &'a Tree, tree2: &'a Tree) -> TreeEntryDiffIterator<'a> { - TreeEntryDiffIterator::new(tree1, tree2) +fn diff_entries<'trees, 'matcher>( + tree1: &'trees Tree, + tree2: &'trees Tree, + matcher: &'matcher dyn Matcher, +) -> TreeEntryDiffIterator<'trees, 'matcher> { + // TODO: make TreeEntryDiffIterator an enum with one variant that iterates over + // the tree entries and filters by the matcher (i.e. what + // TreeEntryDiffIterator does now) and another variant that iterates over + // what the matcher says to visit + TreeEntryDiffIterator::new(tree1, tree2, matcher) } -pub fn recursive_tree_diff(root1: Tree, root2: Tree) -> TreeDiffIterator { - TreeDiffIterator::new(RepoPath::root(), root1, root2) +pub fn recursive_tree_diff(root1: Tree, root2: Tree, matcher: &dyn Matcher) -> TreeDiffIterator { + TreeDiffIterator::new(RepoPath::root(), root1, root2, matcher) } -pub struct TreeDiffIterator { +pub struct TreeDiffIterator<'matcher> { dir: RepoPath, tree1: Pin>, tree2: Pin>, + matcher: &'matcher dyn Matcher, // Iterator over the diffs between tree1 and tree2 - entry_iterator: TreeEntryDiffIterator<'static>, + entry_iterator: TreeEntryDiffIterator<'static, 'matcher>, // 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. added_file: Option<(RepoPath, TreeValue)>, // Iterator over the diffs of a subdirectory, if we're currently visiting one. - subdir_iterator: Option>, + subdir_iterator: Option>>, } -impl TreeDiffIterator { - fn new(dir: RepoPath, tree1: Tree, tree2: Tree) -> TreeDiffIterator { +impl<'matcher> TreeDiffIterator<'matcher> { + fn new( + dir: RepoPath, + tree1: Tree, + tree2: Tree, + matcher: &'matcher dyn Matcher, + ) -> 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> = + let root_entry_iterator: TreeEntryDiffIterator = diff_entries(&tree1, &tree2, matcher); + let root_entry_iterator: TreeEntryDiffIterator<'static, 'matcher> = unsafe { std::mem::transmute(root_entry_iterator) }; Self { dir, tree1, tree2, + matcher, entry_iterator: root_entry_iterator, added_file: None, subdir_iterator: None, @@ -379,7 +405,7 @@ impl TreeDiffIterator { } } -impl Iterator for TreeDiffIterator { +impl Iterator for TreeDiffIterator<'_> { type Item = (RepoPath, Diff); fn next(&mut self) -> Option { @@ -400,17 +426,17 @@ impl Iterator for TreeDiffIterator { let tree_before = matches!(before, Some(TreeValue::Tree(_))); let tree_after = matches!(after, Some(TreeValue::Tree(_))); if tree_before || tree_after { - let subdir = RepoPathComponent::from(name.as_str()); - let subdir_path = self.dir.join(&subdir); + let subdir = &name; + let subdir_path = self.dir.join(subdir); let before_tree = match before { Some(TreeValue::Tree(id_before)) => { - self.tree1.known_sub_tree(&subdir, &id_before) + self.tree1.known_sub_tree(subdir, &id_before) } _ => Tree::null(self.tree1.store().clone(), subdir_path.clone()), }; let after_tree = match after { Some(TreeValue::Tree(id_after)) => { - self.tree2.known_sub_tree(&subdir, &id_after) + self.tree2.known_sub_tree(subdir, &id_after) } _ => Tree::null(self.tree2.store().clone(), subdir_path.clone()), }; @@ -418,33 +444,36 @@ impl Iterator for TreeDiffIterator { subdir_path, before_tree, after_tree, + self.matcher, ))); } - let file_path = self.dir.join(&RepoPathComponent::from(name.as_str())); - if !tree_before && tree_after { - if let Some(file_before) = before { - return Some((file_path, Diff::Removed(file_before.clone()))); - } - } else if tree_before && !tree_after { - if let Some(file_after) = after { - self.added_file = Some((file_path, file_after.clone())); - } - } else if !tree_before && !tree_after { - match (before, after) { - (Some(file_before), Some(file_after)) => { - return Some(( - file_path, - Diff::Modified(file_before.clone(), file_after.clone()), - )); - } - (None, Some(file_after)) => { - return Some((file_path, Diff::Added(file_after.clone()))); - } - (Some(file_before), None) => { + let file_path = self.dir.join(&name); + if self.matcher.matches(&file_path) { + if !tree_before && tree_after { + if let Some(file_before) = before { return Some((file_path, Diff::Removed(file_before.clone()))); } - (None, None) => { - panic!("unexpected diff") + } else if tree_before && !tree_after { + if let Some(file_after) = after { + self.added_file = Some((file_path, file_after.clone())); + } + } else if !tree_before && !tree_after { + match (before, after) { + (Some(file_before), Some(file_after)) => { + return Some(( + file_path, + Diff::Modified(file_before.clone(), file_after.clone()), + )); + } + (None, Some(file_after)) => { + return Some((file_path, Diff::Added(file_after.clone()))); + } + (Some(file_before), None) => { + return Some((file_path, Diff::Removed(file_before.clone()))); + } + (None, None) => { + panic!("unexpected diff") + } } } } @@ -475,7 +504,9 @@ pub fn merge_trees( // Start with a tree identical to side 1 and modify based on changes from base // to side 2. let mut new_tree = side1_tree.data().clone(); - for (basename, maybe_base, maybe_side2) in diff_entries(base_tree, side2_tree) { + for (basename, maybe_base, maybe_side2) in + diff_entries(base_tree, side2_tree, &EverythingMatcher) + { let maybe_side1 = side1_tree.value(&basename); if maybe_side1 == maybe_base { // side 1 is unchanged: use the value from side 2 diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs index e0142f7de..3dc36afb1 100644 --- a/lib/src/working_copy.rs +++ b/lib/src/working_copy.rs @@ -37,6 +37,7 @@ use crate::commit::Commit; use crate::commit_builder::CommitBuilder; use crate::gitignore::GitIgnoreFile; use crate::lock::FileLock; +use crate::matchers::EverythingMatcher; use crate::repo::ReadonlyRepo; use crate::repo_path::{RepoPath, RepoPathComponent, RepoPathJoin}; use crate::settings::UserSettings; @@ -501,7 +502,7 @@ impl TreeState { removed_files: 0, }; - for (path, diff) in old_tree.diff(&new_tree) { + for (path, diff) in old_tree.diff(&new_tree, &EverythingMatcher) { let disk_path = path.to_fs_path(&self.working_copy_path); // TODO: Check that the file has not changed before overwriting/removing it. diff --git a/lib/tests/test_commit_builder.rs b/lib/tests/test_commit_builder.rs index 507d056aa..53b48cf96 100644 --- a/lib/tests/test_commit_builder.rs +++ b/lib/tests/test_commit_builder.rs @@ -13,6 +13,7 @@ // limitations under the License. use jujutsu_lib::commit_builder::CommitBuilder; +use jujutsu_lib::matchers::EverythingMatcher; use jujutsu_lib::repo_path::RepoPath; use jujutsu_lib::settings::UserSettings; use jujutsu_lib::testutils; @@ -49,7 +50,10 @@ fn test_initial(use_git: bool) { assert_eq!(commit.committer().name, settings.user_name()); assert_eq!(commit.committer().email, settings.user_email()); assert_eq!( - store.root_commit().tree().diff_summary(&commit.tree()), + store + .root_commit() + .tree() + .diff_summary(&commit.tree(), &EverythingMatcher), DiffSummary { modified: vec![], added: vec![dir_file_path, root_file_path], @@ -119,7 +123,7 @@ fn test_rewrite(use_git: bool) { store .root_commit() .tree() - .diff_summary(&rewritten_commit.tree()), + .diff_summary(&rewritten_commit.tree(), &EverythingMatcher), DiffSummary { modified: vec![], added: vec![dir_file_path.clone(), root_file_path], @@ -127,7 +131,9 @@ fn test_rewrite(use_git: bool) { } ); assert_eq!( - initial_commit.tree().diff_summary(&rewritten_commit.tree()), + initial_commit + .tree() + .diff_summary(&rewritten_commit.tree(), &EverythingMatcher), 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 215aad0e6..1ea6f808c 100644 --- a/lib/tests/test_diff_summary.rs +++ b/lib/tests/test_diff_summary.rs @@ -12,9 +12,11 @@ // See the License for the specific language governing permissions and // limitations under the License. +use jujutsu_lib::matchers::{EverythingMatcher, FilesMatcher}; use jujutsu_lib::repo_path::RepoPath; use jujutsu_lib::testutils; use jujutsu_lib::tree::DiffSummary; +use maplit::hashset; use test_case::test_case; #[test_case(false ; "local store")] @@ -47,7 +49,7 @@ fn test_types(use_git: bool) { ); assert_eq!( - tree1.diff_summary(&tree2), + tree1.diff_summary(&tree2, &EverythingMatcher), DiffSummary { modified: vec![modified_path], added: vec![added_path], @@ -69,7 +71,7 @@ fn test_tree_file_transition(use_git: bool) { let tree2 = testutils::create_tree(&repo, &[(&dir_path, "contents")]); assert_eq!( - tree1.diff_summary(&tree2), + tree1.diff_summary(&tree2, &EverythingMatcher), DiffSummary { modified: vec![], added: vec![dir_path.clone()], @@ -77,7 +79,7 @@ fn test_tree_file_transition(use_git: bool) { } ); assert_eq!( - tree2.diff_summary(&tree1), + tree2.diff_summary(&tree1, &EverythingMatcher), DiffSummary { modified: vec![], added: vec![dir_file_path], @@ -127,7 +129,7 @@ fn test_sorting(use_git: bool) { ); assert_eq!( - tree1.diff_summary(&tree2), + tree1.diff_summary(&tree2, &EverythingMatcher), DiffSummary { modified: vec![a_path.clone(), f_a_path.clone(), f_f_a_path.clone()], added: vec![ @@ -142,7 +144,7 @@ fn test_sorting(use_git: bool) { } ); assert_eq!( - tree2.diff_summary(&tree1), + tree2.diff_summary(&tree1, &EverythingMatcher), DiffSummary { modified: vec![a_path, f_a_path, f_f_a_path], added: vec![], @@ -150,3 +152,133 @@ fn test_sorting(use_git: bool) { } ); } + +#[test_case(false ; "local store")] +#[test_case(true ; "git store")] +fn test_matcher_dir_file_transition(use_git: bool) { + let settings = testutils::user_settings(); + let (_temp_dir, repo) = testutils::init_repo(&settings, use_git); + + let a_path = RepoPath::from_internal_string("a"); + let a_a_path = RepoPath::from_internal_string("a/a"); + + let tree1 = testutils::create_tree(&repo, &[(&a_path, "before")]); + let tree2 = testutils::create_tree(&repo, &[(&a_a_path, "after")]); + + let matcher = FilesMatcher::new(hashset! {a_path.clone()}); + assert_eq!( + tree1.diff_summary(&tree2, &matcher), + DiffSummary { + modified: vec![], + added: vec![], + removed: vec![a_path.clone()] + } + ); + assert_eq!( + tree2.diff_summary(&tree1, &matcher), + DiffSummary { + modified: vec![], + added: vec![a_path.clone()], + removed: vec![] + } + ); + + let matcher = FilesMatcher::new(hashset! {a_a_path.clone()}); + assert_eq!( + tree1.diff_summary(&tree2, &matcher), + DiffSummary { + modified: vec![], + added: vec![a_a_path.clone()], + removed: vec![] + } + ); + assert_eq!( + tree2.diff_summary(&tree1, &matcher), + DiffSummary { + modified: vec![], + added: vec![], + removed: vec![a_a_path.clone()] + } + ); + + let matcher = FilesMatcher::new(hashset! {a_path.clone(), a_a_path.clone()}); + assert_eq!( + tree1.diff_summary(&tree2, &matcher), + DiffSummary { + modified: vec![], + added: vec![a_a_path.clone()], + removed: vec![a_path.clone()] + } + ); + assert_eq!( + tree2.diff_summary(&tree1, &matcher), + DiffSummary { + modified: vec![], + added: vec![a_path.clone()], + removed: vec![a_a_path.clone()] + } + ); +} + +#[test_case(false ; "local store")] +#[test_case(true ; "git store")] +fn test_matcher_normal_cases(use_git: bool) { + let settings = testutils::user_settings(); + let (_temp_dir, repo) = testutils::init_repo(&settings, use_git); + + let a_path = RepoPath::from_internal_string("a"); + let dir1_a_path = RepoPath::from_internal_string("dir1/a"); + let dir2_b_path = RepoPath::from_internal_string("dir2/b"); + let z_path = RepoPath::from_internal_string("z"); + + let tree1 = testutils::create_tree(&repo, &[(&a_path, "before"), (&dir1_a_path, "before")]); + // File "a" gets modified + // File "dir1/a" gets modified + // File "dir2/b" gets created + // File "z" gets created + let tree2 = testutils::create_tree( + &repo, + &[ + (&a_path, "after"), + (&dir1_a_path, "after"), + (&dir2_b_path, "after"), + (&z_path, "after"), + ], + ); + + let matcher = FilesMatcher::new(hashset! {a_path.clone(), z_path.clone()}); + assert_eq!( + tree1.diff_summary(&tree2, &matcher), + DiffSummary { + modified: vec![a_path.clone()], + added: vec![z_path.clone()], + removed: vec![] + } + ); + assert_eq!( + tree2.diff_summary(&tree1, &matcher), + DiffSummary { + modified: vec![a_path.clone()], + added: vec![], + removed: vec![z_path.clone()] + } + ); + + let matcher = FilesMatcher::new(hashset! {dir1_a_path.clone(), dir2_b_path.clone()}); + assert_eq!( + tree1.diff_summary(&tree2, &matcher), + DiffSummary { + modified: vec![dir1_a_path.clone()], + added: vec![dir2_b_path.clone()], + removed: vec![] + } + ); + assert_eq!( + tree2.diff_summary(&tree1, &matcher), + DiffSummary { + modified: vec![dir1_a_path.clone()], + added: vec![], + removed: vec![dir2_b_path.clone()] + } + ); +} diff --git a/lib/tests/test_working_copy.rs b/lib/tests/test_working_copy.rs index ab31c4684..71832eab0 100644 --- a/lib/tests/test_working_copy.rs +++ b/lib/tests/test_working_copy.rs @@ -20,6 +20,7 @@ use std::sync::Arc; use itertools::Itertools; use jujutsu_lib::commit_builder::CommitBuilder; +use jujutsu_lib::matchers::EverythingMatcher; use jujutsu_lib::repo::ReadonlyRepo; use jujutsu_lib::repo_path::{RepoPath, RepoPathComponent}; use jujutsu_lib::settings::UserSettings; @@ -185,7 +186,9 @@ fn test_checkout_file_transitions(use_git: bool) { // Check that the working copy is clean. let (reloaded_repo, after_commit) = wc.commit(&settings, repo); repo = reloaded_repo; - let diff_summary = right_commit.tree().diff_summary(&after_commit.tree()); + let diff_summary = right_commit + .tree() + .diff_summary(&after_commit.tree(), &EverythingMatcher); assert_eq!(diff_summary.modified, vec![]); assert_eq!(diff_summary.added, vec![]); assert_eq!(diff_summary.removed, vec![]); diff --git a/src/commands.rs b/src/commands.rs index 8633f3635..85d499386 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -39,6 +39,7 @@ use jujutsu_lib::evolution::{ use jujutsu_lib::files::DiffLine; use jujutsu_lib::git::GitFetchError; use jujutsu_lib::index::HexPrefix; +use jujutsu_lib::matchers::EverythingMatcher; use jujutsu_lib::op_heads_store::OpHeadsStore; use jujutsu_lib::op_store::{OpStore, OpStoreError, OperationId}; use jujutsu_lib::operation::Operation; @@ -1052,12 +1053,12 @@ fn cmd_diff( } let repo = repo_command.repo(); if sub_matches.is_present("summary") { - let summary = from_tree.diff_summary(&to_tree); + let summary = from_tree.diff_summary(&to_tree, &EverythingMatcher); show_diff_summary(ui, repo.working_copy_path(), &summary)?; } else { let mut formatter = ui.stdout_formatter(); formatter.add_label(String::from("diff"))?; - for (path, diff) in from_tree.diff(&to_tree) { + for (path, diff) in from_tree.diff(&to_tree, &EverythingMatcher) { let ui_path = ui.format_file_path(repo.working_copy_path(), &path); match diff { Diff::Added(TreeValue::Normal { @@ -1211,7 +1212,9 @@ fn cmd_status( ui.write("Working copy : ")?; ui.write_commit_summary(repo.as_repo_ref(), &commit)?; ui.write("\n")?; - let summary = commit.parents()[0].tree().diff_summary(&commit.tree()); + let summary = commit.parents()[0] + .tree() + .diff_summary(&commit.tree(), &EverythingMatcher); if summary.is_empty() { ui.write("The working copy is clean\n")?; } else { diff --git a/src/diff_edit.rs b/src/diff_edit.rs index d475181bf..bed4d1a9f 100644 --- a/src/diff_edit.rs +++ b/src/diff_edit.rs @@ -18,6 +18,7 @@ use std::path::{Path, PathBuf}; use std::process::Command; use std::sync::Arc; +use jujutsu_lib::matchers::EverythingMatcher; use jujutsu_lib::repo_path::RepoPath; use jujutsu_lib::store::{StoreError, TreeId, TreeValue}; use jujutsu_lib::store_wrapper::StoreWrapper; @@ -106,7 +107,7 @@ pub fn edit_diff( let store = left_tree.store(); let mut left_tree_builder = store.tree_builder(store.empty_tree_id().clone()); let mut right_tree_builder = store.tree_builder(store.empty_tree_id().clone()); - for (file_path, diff) in left_tree.diff(&right_tree) { + for (file_path, diff) in left_tree.diff(&right_tree, &EverythingMatcher) { let (left_value, right_value) = diff.as_options(); if let Some(value) = left_value { add_to_tree(store, &mut left_tree_builder, &file_path, value).unwrap();