From 4e6b51378f73a4ec834ea045f78597e67c26a084 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Wed, 27 Nov 2024 15:07:00 -0800 Subject: [PATCH 1/3] merged_tree: avoid two unnecessary clones --- lib/src/merged_tree.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/src/merged_tree.rs b/lib/src/merged_tree.rs index 52c06c40a4..b785e3057c 100644 --- a/lib/src/merged_tree.rs +++ b/lib/src/merged_tree.rs @@ -211,7 +211,7 @@ impl MergedTree { } _ => { let subdir = self.dir().join(name); - Ok(Tree::empty(self.store().clone(), subdir.clone())) + Ok(Tree::empty(self.store().clone(), subdir)) } })?; Ok(Some(MergedTree { trees })) @@ -961,7 +961,7 @@ impl<'matcher> TreeDiffStreamImpl<'matcher> { .await?; builder.build() } else { - Merge::resolved(Tree::empty(store, dir.clone())) + Merge::resolved(Tree::empty(store, dir)) }; Ok(MergedTree { trees }) } From ad6b5cf6c9a432ca18005bc69a9ca0690ef3ea3d Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Wed, 27 Nov 2024 14:47:07 -0800 Subject: [PATCH 2/3] store: make `get_tree()` functions take owned repo path The function needs an owned value, so we might as well pass it one and avoid a few clone calls. --- cli/src/commands/debug/tree.rs | 2 +- lib/src/merge.rs | 2 +- lib/src/merged_tree.rs | 14 +++++++------- lib/src/store.rs | 12 ++++++------ lib/src/tree.rs | 12 ++++++------ lib/src/tree_builder.rs | 2 +- lib/tests/test_local_working_copy.rs | 4 ++-- lib/tests/test_merge_trees.rs | 17 +++++++++-------- lib/tests/test_merged_tree.rs | 2 +- lib/testutils/src/lib.rs | 2 +- 10 files changed, 35 insertions(+), 34 deletions(-) diff --git a/cli/src/commands/debug/tree.rs b/cli/src/commands/debug/tree.rs index 7c40d33b57..479276e4ff 100644 --- a/cli/src/commands/debug/tree.rs +++ b/cli/src/commands/debug/tree.rs @@ -54,7 +54,7 @@ pub fn cmd_debug_tree( RepoPathBuf::root() }; let store = workspace_command.repo().store(); - let tree = store.get_tree(&dir, &tree_id)?; + let tree = store.get_tree(dir, &tree_id)?; MergedTree::resolved(tree) } else { let commit = workspace_command diff --git a/lib/src/merge.rs b/lib/src/merge.rs index 3ed104602e..3032b26be6 100644 --- a/lib/src/merge.rs +++ b/lib/src/merge.rs @@ -613,7 +613,7 @@ where if let Some(tree_id_merge) = tree_id_merge { let get_tree = |id: &Option<&TreeId>| -> BackendResult { if let Some(id) = id { - store.get_tree(dir, id) + store.get_tree(dir.to_owned(), id) } else { Ok(Tree::empty(store.clone(), dir.to_owned())) } diff --git a/lib/src/merged_tree.rs b/lib/src/merged_tree.rs index b785e3057c..2f2fc20e32 100644 --- a/lib/src/merged_tree.rs +++ b/lib/src/merged_tree.rs @@ -117,7 +117,7 @@ impl MergedTree { .into_iter() .map(|builder| { let tree_id = builder.write_tree()?; - store.get_tree(RepoPath::root(), &tree_id) + store.get_tree(RepoPathBuf::root(), &tree_id) }) .try_collect()?; Ok(MergedTree { @@ -199,7 +199,7 @@ impl MergedTree { Ok(Some(TreeValue::Tree(sub_tree_id))) => { let subdir = self.dir().join(name); Ok(Some(MergedTree::resolved( - self.store().get_tree(&subdir, sub_tree_id)?, + self.store().get_tree(subdir, sub_tree_id)?, ))) } Ok(_) => Ok(None), @@ -207,7 +207,7 @@ impl MergedTree { let trees = merge.try_map(|value| match value { Some(TreeValue::Tree(sub_tree_id)) => { let subdir = self.dir().join(name); - self.store().get_tree(&subdir, sub_tree_id) + self.store().get_tree(subdir, sub_tree_id) } _ => { let subdir = self.dir().join(name); @@ -939,12 +939,12 @@ impl<'matcher> TreeDiffStreamImpl<'matcher> { async fn single_tree( store: &Arc, - dir: &RepoPath, + dir: RepoPathBuf, value: Option<&TreeValue>, ) -> BackendResult { match value { Some(TreeValue::Tree(tree_id)) => store.get_tree_async(dir, tree_id).await, - _ => Ok(Tree::empty(store.clone(), dir.to_owned())), + _ => Ok(Tree::empty(store.clone(), dir.clone())), } } @@ -956,7 +956,7 @@ impl<'matcher> TreeDiffStreamImpl<'matcher> { ) -> BackendResult { let trees = if values.is_tree() { let builder: MergeBuilder = futures::stream::iter(values.iter()) - .then(|value| Self::single_tree(&store, &dir, value.as_ref())) + .then(|value| Self::single_tree(&store, dir.clone(), value.as_ref())) .try_collect() .await?; builder.build() @@ -1147,7 +1147,7 @@ impl MergedTreeBuilder { pub fn write_tree(self, store: &Arc) -> BackendResult { let base_tree_ids = match self.base_tree_id.clone() { MergedTreeId::Legacy(base_tree_id) => { - let legacy_base_tree = store.get_tree(RepoPath::root(), &base_tree_id)?; + let legacy_base_tree = store.get_tree(RepoPathBuf::root(), &base_tree_id)?; let base_tree = MergedTree::from_legacy_tree(legacy_base_tree)?; base_tree.id().to_merge() } diff --git a/lib/src/store.rs b/lib/src/store.rs index 6d64e66596..e7fb650105 100644 --- a/lib/src/store.rs +++ b/lib/src/store.rs @@ -170,17 +170,17 @@ impl Store { Ok(Commit::new(self.clone(), commit_id, data)) } - pub fn get_tree(self: &Arc, dir: &RepoPath, id: &TreeId) -> BackendResult { + pub fn get_tree(self: &Arc, dir: RepoPathBuf, id: &TreeId) -> BackendResult { self.get_tree_async(dir, id).block_on() } pub async fn get_tree_async( self: &Arc, - dir: &RepoPath, + dir: RepoPathBuf, id: &TreeId, ) -> BackendResult { - let data = self.get_backend_tree(dir, id).await?; - Ok(Tree::new(self.clone(), dir.to_owned(), id.clone(), data)) + let data = self.get_backend_tree(&dir, id).await?; + Ok(Tree::new(self.clone(), dir, id.clone(), data)) } async fn get_backend_tree( @@ -205,11 +205,11 @@ impl Store { pub fn get_root_tree(self: &Arc, id: &MergedTreeId) -> BackendResult { match &id { MergedTreeId::Legacy(id) => { - let tree = self.get_tree(RepoPath::root(), id)?; + let tree = self.get_tree(RepoPathBuf::root(), id)?; MergedTree::from_legacy_tree(tree) } MergedTreeId::Merge(ids) => { - let trees = ids.try_map(|id| self.get_tree(RepoPath::root(), id))?; + let trees = ids.try_map(|id| self.get_tree(RepoPathBuf::root(), id))?; Ok(MergedTree::new(trees)) } } diff --git a/lib/src/tree.rs b/lib/src/tree.rs index 35cd28c98f..453cb6cca4 100644 --- a/lib/src/tree.rs +++ b/lib/src/tree.rs @@ -155,7 +155,7 @@ impl Tree { match sub_tree { TreeValue::Tree(sub_tree_id) => { let subdir = self.dir.join(name); - let sub_tree = self.store.get_tree(&subdir, sub_tree_id)?; + let sub_tree = self.store.get_tree(subdir, sub_tree_id)?; Ok(Some(sub_tree)) } _ => Ok(None), @@ -165,7 +165,7 @@ impl Tree { } } - fn known_sub_tree(&self, subdir: &RepoPath, id: &TreeId) -> Tree { + fn known_sub_tree(&self, subdir: RepoPathBuf, id: &TreeId) -> Tree { self.store.get_tree(subdir, id).unwrap() } @@ -251,7 +251,7 @@ impl Iterator for TreeEntriesIterator<'_> { if self.matcher.visit(&path).is_nothing() { continue; } - let subtree = top.tree.known_sub_tree(&path, &id); + let subtree = top.tree.known_sub_tree(path, &id); self.stack.push(TreeEntriesDirItem::from(subtree)); } value => { @@ -369,9 +369,9 @@ fn merge_tree_value( Ok(match (base_tree_id, side1_tree_id, side2_tree_id) { (Some(base_id), Some(side1_id), Some(side2_id)) => { let subdir = dir.join(basename); - let base_tree = store.get_tree(&subdir, base_id)?; - let side1_tree = store.get_tree(&subdir, side1_id)?; - let side2_tree = store.get_tree(&subdir, side2_id)?; + let base_tree = store.get_tree(subdir.clone(), base_id)?; + let side1_tree = store.get_tree(subdir.clone(), side1_id)?; + let side2_tree = store.get_tree(subdir, side2_id)?; let merged_tree = merge_trees(&side1_tree, &base_tree, &side2_tree)?; if merged_tree.id() == empty_tree_id { None diff --git a/lib/src/tree_builder.rs b/lib/src/tree_builder.rs index 4a32f6dde3..fba2c20bd8 100644 --- a/lib/src/tree_builder.rs +++ b/lib/src/tree_builder.rs @@ -127,7 +127,7 @@ impl TreeBuilder { let store = &self.store; let mut tree_cache = { let dir = RepoPathBuf::root(); - let tree = store.get_tree(&dir, &self.base_tree_id)?; + let tree = store.get_tree(dir.clone(), &self.base_tree_id)?; BTreeMap::from([(dir, tree)]) }; diff --git a/lib/tests/test_local_working_copy.rs b/lib/tests/test_local_working_copy.rs index 2c7f5f4ba2..5bb5d21a57 100644 --- a/lib/tests/test_local_working_copy.rs +++ b/lib/tests/test_local_working_copy.rs @@ -506,7 +506,7 @@ fn test_tree_builder_file_directory_transition() { let mut ws = test_workspace.workspace; let workspace_root = ws.workspace_root().to_owned(); let mut check_out_tree = |tree_id: &TreeId| { - let tree = repo.store().get_tree(RepoPath::root(), tree_id).unwrap(); + let tree = repo.store().get_tree(RepoPathBuf::root(), tree_id).unwrap(); let commit = commit_with_tree(repo.store(), MergedTreeId::Legacy(tree.id().clone())); ws.check_out( repo.op_id().clone(), @@ -1140,7 +1140,7 @@ fn test_gitignores_ignored_directory_already_tracked() { } } let id = tree_builder.write_tree().unwrap(); - MergedTree::resolved(store.get_tree(RepoPath::root(), &id).unwrap()) + MergedTree::resolved(store.get_tree(RepoPathBuf::root(), &id).unwrap()) }; let gitignore_path = RepoPath::from_internal_string(".gitignore"); diff --git a/lib/tests/test_merge_trees.rs b/lib/tests/test_merge_trees.rs index 1b639eaf70..46d45e3467 100644 --- a/lib/tests/test_merge_trees.rs +++ b/lib/tests/test_merge_trees.rs @@ -16,6 +16,7 @@ use itertools::Itertools; use jj_lib::backend::TreeValue; use jj_lib::repo::Repo; use jj_lib::repo_path::RepoPath; +use jj_lib::repo_path::RepoPathBuf; use jj_lib::repo_path::RepoPathComponent; use jj_lib::rewrite::rebase_commit; use jj_lib::tree::merge_trees; @@ -65,7 +66,7 @@ fn test_same_type() { } } let tree_id = tree_builder.write_tree().unwrap(); - store.get_tree(RepoPath::root(), &tree_id).unwrap() + store.get_tree(RepoPathBuf::root(), &tree_id).unwrap() }; let base_tree = write_tree(0); @@ -207,7 +208,7 @@ fn test_executable() { } } let tree_id = tree_builder.write_tree().unwrap(); - store.get_tree(RepoPath::root(), &tree_id).unwrap() + store.get_tree(RepoPathBuf::root(), &tree_id).unwrap() }; fn contents_in_tree<'a>(files: &[&'a str], index: usize) -> Vec<(&'a str, bool)> { @@ -255,7 +256,7 @@ fn test_subtrees() { ); } let tree_id = tree_builder.write_tree().unwrap(); - store.get_tree(RepoPath::root(), &tree_id).unwrap() + store.get_tree(RepoPathBuf::root(), &tree_id).unwrap() }; let base_tree = write_tree(vec!["f1", "d1/f1", "d1/d1/f1", "d1/d1/d1/f1"]); @@ -309,7 +310,7 @@ fn test_subtree_becomes_empty() { ); } let tree_id = tree_builder.write_tree().unwrap(); - store.get_tree(RepoPath::root(), &tree_id).unwrap() + store.get_tree(RepoPathBuf::root(), &tree_id).unwrap() }; let base_tree = write_tree(vec!["f1", "d1/f1", "d1/d1/d1/f1", "d1/d1/d1/f2"]); @@ -338,7 +339,7 @@ fn test_subtree_one_missing() { ); } let tree_id = tree_builder.write_tree().unwrap(); - store.get_tree(RepoPath::root(), &tree_id).unwrap() + store.get_tree(RepoPathBuf::root(), &tree_id).unwrap() }; let tree1 = write_tree(vec![]); @@ -405,11 +406,11 @@ fn test_types() { "contents", ); let base_tree_id = base_tree_builder.write_tree().unwrap(); - let base_tree = store.get_tree(RepoPath::root(), &base_tree_id).unwrap(); + let base_tree = store.get_tree(RepoPathBuf::root(), &base_tree_id).unwrap(); let side1_tree_id = side1_tree_builder.write_tree().unwrap(); - let side1_tree = store.get_tree(RepoPath::root(), &side1_tree_id).unwrap(); + let side1_tree = store.get_tree(RepoPathBuf::root(), &side1_tree_id).unwrap(); let side2_tree_id = side2_tree_builder.write_tree().unwrap(); - let side2_tree = store.get_tree(RepoPath::root(), &side2_tree_id).unwrap(); + let side2_tree = store.get_tree(RepoPathBuf::root(), &side2_tree_id).unwrap(); // Created the merged tree let merged_tree = merge_trees(&side1_tree, &base_tree, &side2_tree).unwrap(); diff --git a/lib/tests/test_merged_tree.rs b/lib/tests/test_merged_tree.rs index e11501cc21..b32e1be3c6 100644 --- a/lib/tests/test_merged_tree.rs +++ b/lib/tests/test_merged_tree.rs @@ -164,7 +164,7 @@ fn test_from_legacy_tree() { tree_builder.set(dir1_filename.to_owned(), file_value(&dir1_filename_id)); let tree_id = tree_builder.write_tree().unwrap(); - let tree = store.get_tree(RepoPath::root(), &tree_id).unwrap(); + let tree = store.get_tree(RepoPathBuf::root(), &tree_id).unwrap(); let merged_tree = MergedTree::from_legacy_tree(tree.clone()).unwrap(); assert_eq!( diff --git a/lib/testutils/src/lib.rs b/lib/testutils/src/lib.rs index eebd199247..a6afeea061 100644 --- a/lib/testutils/src/lib.rs +++ b/lib/testutils/src/lib.rs @@ -384,7 +384,7 @@ pub fn create_single_tree(repo: &Arc, path_contents: &[(&RepoPath, write_normal_file(&mut tree_builder, path, contents); } let id = tree_builder.write_tree().unwrap(); - store.get_tree(RepoPath::root(), &id).unwrap() + store.get_tree(RepoPathBuf::root(), &id).unwrap() } pub fn create_tree(repo: &Arc, path_contents: &[(&RepoPath, &str)]) -> MergedTree { From 3ca2ef2a0f8370ea9dcb6c8397106790a15b2265 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Wed, 27 Nov 2024 14:59:04 -0800 Subject: [PATCH 3/3] test_merged_tree: avoid a temporary lifetime extension I think it's just clearer to assign the owned value to the variable than to assign a reference to a temporary value. --- lib/tests/test_merged_tree.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/tests/test_merged_tree.rs b/lib/tests/test_merged_tree.rs index b32e1be3c6..dc5fabb237 100644 --- a/lib/tests/test_merged_tree.rs +++ b/lib/tests/test_merged_tree.rs @@ -157,11 +157,11 @@ fn test_from_legacy_tree() { // dir1: directory without conflicts let dir1_basename = RepoPathComponent::new("dir1"); - let dir1_filename = &RepoPath::root() + let dir1_filename = RepoPath::root() .join(dir1_basename) .join(RepoPathComponent::new("file")); - let dir1_filename_id = write_file(store.as_ref(), dir1_filename, "file5_v2"); - tree_builder.set(dir1_filename.to_owned(), file_value(&dir1_filename_id)); + let dir1_filename_id = write_file(store.as_ref(), &dir1_filename, "file5_v2"); + tree_builder.set(dir1_filename.clone(), file_value(&dir1_filename_id)); let tree_id = tree_builder.write_tree().unwrap(); let tree = store.get_tree(RepoPathBuf::root(), &tree_id).unwrap(); @@ -269,7 +269,7 @@ fn test_from_legacy_tree() { tree_builder.set_or_remove(file3_path.to_owned(), file3_conflict); tree_builder.set_or_remove(file4_path.to_owned(), file4_conflict); tree_builder.set_or_remove( - dir1_filename.to_owned(), + dir1_filename.clone(), Merge::normal(file_value(&dir1_filename_id)), ); let recreated_merged_id = tree_builder.write_tree(store).unwrap();