From 4e6b51378f73a4ec834ea045f78597e67c26a084 Mon Sep 17 00:00:00 2001
From: Martin von Zweigbergk <martinvonz@google.com>
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 <martinvonz@google.com>
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<Tree> {
                 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<Store>,
-        dir: &RepoPath,
+        dir: RepoPathBuf,
         value: Option<&TreeValue>,
     ) -> BackendResult<Tree> {
         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<MergedTree> {
         let trees = if values.is_tree() {
             let builder: MergeBuilder<Tree> = 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<Store>) -> BackendResult<MergedTreeId> {
         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<Self>, dir: &RepoPath, id: &TreeId) -> BackendResult<Tree> {
+    pub fn get_tree(self: &Arc<Self>, dir: RepoPathBuf, id: &TreeId) -> BackendResult<Tree> {
         self.get_tree_async(dir, id).block_on()
     }
 
     pub async fn get_tree_async(
         self: &Arc<Self>,
-        dir: &RepoPath,
+        dir: RepoPathBuf,
         id: &TreeId,
     ) -> BackendResult<Tree> {
-        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<Self>, id: &MergedTreeId) -> BackendResult<MergedTree> {
         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<ReadonlyRepo>, 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<ReadonlyRepo>, path_contents: &[(&RepoPath, &str)]) -> MergedTree {

From 3ca2ef2a0f8370ea9dcb6c8397106790a15b2265 Mon Sep 17 00:00:00 2001
From: Martin von Zweigbergk <martinvonz@google.com>
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();