From a585faf2dbbb4082fc61c33e4326ee4edf401378 Mon Sep 17 00:00:00 2001 From: Stefan Filip Date: Thu, 1 Aug 2019 10:30:45 -0700 Subject: [PATCH] manifest: update `get` to return FileMetadata copy Summary: There is no good reason to return a reference for FileMetadata. It is a relatively small object that implements the `Copy` trait so returning a copy is approapriate. Reviewed By: quark-zju Differential Revision: D16571838 fbshipit-source-id: 0c315c9f405e425832d39da5c67809dd15b4ab5e --- .../mercurial/rust/bindings/src/manifest.rs | 2 +- lib/manifest/src/lib.rs | 2 +- lib/manifest/src/tree/mod.rs | 40 +++++++++---------- 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/edenscm/mercurial/rust/bindings/src/manifest.rs b/edenscm/mercurial/rust/bindings/src/manifest.rs index 718bfd6ce7..46a6200d58 100644 --- a/edenscm/mercurial/rust/bindings/src/manifest.rs +++ b/edenscm/mercurial/rust/bindings/src/manifest.rs @@ -78,7 +78,7 @@ py_class!(class treemanifest |py| { let tree = self.underlying(py).borrow(); let result = match tree.get(&repo_path).map_pyerr::(py)? { None => None, - Some(file_metadata) => Some(file_metadata_to_py_tuple(py, file_metadata)?), + Some(file_metadata) => Some(file_metadata_to_py_tuple(py, &file_metadata)?), }; Ok(result) } diff --git a/lib/manifest/src/lib.rs b/lib/manifest/src/lib.rs index cd32dbd8c0..d9124b670a 100644 --- a/lib/manifest/src/lib.rs +++ b/lib/manifest/src/lib.rs @@ -27,7 +27,7 @@ use types::{Node, RepoPath, RepoPathBuf}; pub trait Manifest { /// Retrieve the FileMetadata that is associated with a path. /// Paths that were not set will return None. - fn get(&self, file_path: &RepoPath) -> Fallible>; + fn get(&self, file_path: &RepoPath) -> Fallible>; /// Associates a file path with specific file metadata. /// A call with a file path that already exists results in an override or the old metadata. diff --git a/lib/manifest/src/tree/mod.rs b/lib/manifest/src/tree/mod.rs index 39ce075f41..5dca7e1ff7 100644 --- a/lib/manifest/src/tree/mod.rs +++ b/lib/manifest/src/tree/mod.rs @@ -69,11 +69,11 @@ impl Tree { } impl Manifest for Tree { - fn get(&self, path: &RepoPath) -> Fallible> { + fn get(&self, path: &RepoPath) -> Fallible> { match self.get_link(path)? { None => Ok(None), Some(link) => { - if let Leaf(file_metadata) = link { + if let &Leaf(file_metadata) = link { Ok(Some(file_metadata)) } else { Err(format_err!("Encountered directory where file was expected")) @@ -622,17 +622,17 @@ mod tests { fn test_insert() { let mut tree = Tree::ephemeral(Arc::new(TestStore::new())); tree.insert(repo_path_buf("foo/bar"), meta("10")).unwrap(); - assert_eq!(tree.get(repo_path("foo/bar")).unwrap(), Some(&meta("10"))); + assert_eq!(tree.get(repo_path("foo/bar")).unwrap(), Some(meta("10"))); assert_eq!(tree.get(repo_path("baz")).unwrap(), None); tree.insert(repo_path_buf("baz"), meta("20")).unwrap(); - assert_eq!(tree.get(repo_path("foo/bar")).unwrap(), Some(&meta("10"))); - assert_eq!(tree.get(repo_path("baz")).unwrap(), Some(&meta("20"))); + assert_eq!(tree.get(repo_path("foo/bar")).unwrap(), Some(meta("10"))); + assert_eq!(tree.get(repo_path("baz")).unwrap(), Some(meta("20"))); tree.insert(repo_path_buf("foo/bat"), meta("30")).unwrap(); - assert_eq!(tree.get(repo_path("foo/bat")).unwrap(), Some(&meta("30"))); - assert_eq!(tree.get(repo_path("foo/bar")).unwrap(), Some(&meta("10"))); - assert_eq!(tree.get(repo_path("baz")).unwrap(), Some(&meta("20"))); + assert_eq!(tree.get(repo_path("foo/bat")).unwrap(), Some(meta("30"))); + assert_eq!(tree.get(repo_path("foo/bar")).unwrap(), Some(meta("10"))); + assert_eq!(tree.get(repo_path("baz")).unwrap(), Some(meta("20"))); } #[test] @@ -657,13 +657,13 @@ mod tests { .unwrap(); let mut tree = Tree::durable(Arc::new(store), node("1")); - assert_eq!(tree.get(repo_path("foo/bar")).unwrap(), Some(&meta("11"))); - assert_eq!(tree.get(repo_path("baz")).unwrap(), Some(&meta("20"))); + assert_eq!(tree.get(repo_path("foo/bar")).unwrap(), Some(meta("11"))); + assert_eq!(tree.get(repo_path("baz")).unwrap(), Some(meta("20"))); tree.insert(repo_path_buf("foo/bat"), meta("12")).unwrap(); - assert_eq!(tree.get(repo_path("foo/bat")).unwrap(), Some(&meta("12"))); - assert_eq!(tree.get(repo_path("foo/bar")).unwrap(), Some(&meta("11"))); - assert_eq!(tree.get(repo_path("baz")).unwrap(), Some(&meta("20"))); + assert_eq!(tree.get(repo_path("foo/bat")).unwrap(), Some(meta("12"))); + assert_eq!(tree.get(repo_path("foo/bar")).unwrap(), Some(meta("11"))); + assert_eq!(tree.get(repo_path("baz")).unwrap(), Some(meta("20"))); } #[test] @@ -721,11 +721,11 @@ mod tests { assert!(tree.remove(RepoPath::empty()).is_err()); assert_eq!(tree.get(repo_path("a1/b1/c1/d1")).unwrap(), None); assert_eq!(tree.get(repo_path("a1/b1/c1")).unwrap(), None); - assert_eq!(tree.get(repo_path("a1/b2")).unwrap(), Some(&meta("20"))); + assert_eq!(tree.get(repo_path("a1/b2")).unwrap(), Some(meta("20"))); tree.remove(repo_path("a1/b2")).unwrap(); assert_eq!(tree.get_link(repo_path("a1")).unwrap(), None); - assert_eq!(tree.get(repo_path("a2/b2/c2")).unwrap(), Some(&meta("30"))); + assert_eq!(tree.get(repo_path("a2/b2/c2")).unwrap(), Some(meta("30"))); tree.remove(repo_path("a2/b2/c2")).unwrap(); assert_eq!(tree.get(repo_path("a2")).unwrap(), None); @@ -756,13 +756,13 @@ mod tests { assert!(tree.remove(repo_path("a1")).is_err()); tree.remove(repo_path("a1/b1")).unwrap(); assert_eq!(tree.get(repo_path("a1/b1")).unwrap(), None); - assert_eq!(tree.get(repo_path("a1/b2")).unwrap(), Some(&meta("12"))); + assert_eq!(tree.get(repo_path("a1/b2")).unwrap(), Some(meta("12"))); tree.remove(repo_path("a1/b2")).unwrap(); assert_eq!(tree.get(repo_path("a1/b2")).unwrap(), None); assert_eq!(tree.get(repo_path("a1")).unwrap(), None); assert_eq!(tree.get_link(repo_path("a1")).unwrap(), None); - assert_eq!(tree.get(repo_path("a2")).unwrap(), Some(&meta("20"))); + assert_eq!(tree.get(repo_path("a2")).unwrap(), Some(meta("20"))); tree.remove(repo_path("a2")).unwrap(); assert_eq!(tree.get(repo_path("a2")).unwrap(), None); @@ -783,10 +783,10 @@ mod tests { let tree = Tree::durable(store.clone(), node); assert_eq!( tree.get(repo_path("a1/b1/c1/d1")).unwrap(), - Some(&meta("10")) + Some(meta("10")) ); - assert_eq!(tree.get(repo_path("a1/b2")).unwrap(), Some(&meta("20"))); - assert_eq!(tree.get(repo_path("a2/b2/c2")).unwrap(), Some(&meta("30"))); + assert_eq!(tree.get(repo_path("a1/b2")).unwrap(), Some(meta("20"))); + assert_eq!(tree.get(repo_path("a2/b2/c2")).unwrap(), Some(meta("30"))); assert_eq!(tree.get(repo_path("a2/b1")).unwrap(), None); }