From de418e811e72014d91e9d17c346b45f60db35cef Mon Sep 17 00:00:00 2001 From: Han Xu Date: Mon, 16 Jan 2023 17:21:19 -0800 Subject: [PATCH] return Result<> instead of Result> for find_entry Signed-off-by: Han Xu --- radicle-surf/src/fs.rs | 93 ++++++++++++++----------------- radicle-surf/src/repo.rs | 6 +- radicle-surf/t/src/file_system.rs | 40 ++++++------- 3 files changed, 63 insertions(+), 76 deletions(-) diff --git a/radicle-surf/src/fs.rs b/radicle-surf/src/fs.rs index 88d6953..1e43017 100644 --- a/radicle-surf/src/fs.rs +++ b/radicle-surf/src/fs.rs @@ -29,7 +29,6 @@ use std::{ use git2::Blob; use radicle_git_ext::{is_not_found_err, Oid}; -use radicle_std_ext::result::ResultExt as _; use crate::{Repository, Revision}; @@ -43,17 +42,15 @@ pub mod error { #[error(transparent)] Git(#[from] git2::Error), #[error(transparent)] - Entry(#[from] Entry), - #[error(transparent)] File(#[from] File), #[error("the path {0} is not valid")] InvalidPath(PathBuf), - } - - #[derive(Debug, Error, PartialEq, Eq)] - pub enum Entry { + #[error("{0}: {1}")] + InvalidType(PathBuf, &'static str), #[error("the entry name was not valid UTF-8")] Utf8Error, + #[error("the path {0} not found")] + PathNotFound(PathBuf), } #[derive(Debug, Error, PartialEq)] @@ -258,15 +255,18 @@ impl Entry { pub(crate) fn from_entry( entry: &git2::TreeEntry, path: PathBuf, - ) -> Result, error::Entry> { - let name = entry.name().ok_or(error::Entry::Utf8Error)?.to_string(); + ) -> Result { + let name = entry.name().ok_or(error::Directory::Utf8Error)?.to_string(); let id = entry.id().into(); - Ok(entry.kind().and_then(|kind| match kind { - git2::ObjectType::Tree => Some(Self::Directory(Directory::new(name, path, id))), - git2::ObjectType::Blob => Some(Self::File(File::new(name, path, id))), - _ => None, - })) + match entry.kind() { + Some(git2::ObjectType::Tree) => Ok(Self::Directory(Directory::new(name, path, id))), + Some(git2::ObjectType::Blob) => Ok(Self::File(File::new(name, path, id))), + _ => Err(error::Directory::InvalidType( + path, + "Entry must be a tree or a blob", + )), + } } } @@ -361,7 +361,7 @@ impl Directory { // empty for the first level. tree.walk(git2::TreeWalkMode::PreOrder, |_entry_path, entry| { match Entry::from_entry(entry, path.clone()) { - Ok(Some(entry)) => match entry { + Ok(entry) => match entry { Entry::File(_) => { entries.insert(entry.name().clone(), entry); git2::TreeWalkResult::Ok @@ -372,7 +372,6 @@ impl Directory { git2::TreeWalkResult::Skip }, }, - Ok(None) => git2::TreeWalkResult::Skip, Err(err) => { error = Some(err); git2::TreeWalkResult::Abort @@ -381,72 +380,66 @@ impl Directory { })?; match error { - Some(err) => Err(err.into()), + Some(err) => Err(err), None => Ok(Entries { listing: entries }), } } - /// Find the [`Entry`] found at `path`, if it exists. - pub fn find_entry

( - &self, - path: &P, - repo: &Repository, - ) -> Result, error::Directory> + /// Find the [`Entry`] found at a non-empty `path`, if it exists. + pub fn find_entry

(&self, path: &P, repo: &Repository) -> Result where P: AsRef, { // Search the path in git2 tree. let path = path.as_ref(); let git2_tree = repo.find_tree(self.id)?; - let entry = git2_tree - .get_path(path) - .map(Some) - .or_matches::(is_not_found_err, || Ok(None))?; + let entry = git2_tree.get_path(path).map_err(|e| { + if is_not_found_err(&e) { + error::Directory::PathNotFound(path.to_path_buf()) + } else { + error::Directory::Git(e) + } + })?; let parent = path .parent() .ok_or_else(|| error::Directory::InvalidPath(path.to_path_buf()))?; let root_path = self.path().join(parent); - Ok(entry - .and_then(|entry| Entry::from_entry(&entry, root_path.to_path_buf()).transpose()) - .transpose() - .unwrap()) + Entry::from_entry(&entry, root_path) } /// Find the `Oid`, for a [`File`], found at `path`, if it exists. - pub fn find_file

( - &self, - path: &P, - repo: &Repository, - ) -> Result, error::Directory> + pub fn find_file

(&self, path: &P, repo: &Repository) -> Result where P: AsRef, { - Ok(match self.find_entry(path, repo)? { - Some(Entry::File(file)) => Some(file), - _ => None, - }) + match self.find_entry(path, repo)? { + Entry::File(file) => Ok(file), + _ => Err(error::Directory::InvalidType( + path.as_ref().to_path_buf(), + "Entry must be a file", + )), + } } /// Find the `Directory` found at `path`, if it exists. /// /// If `path` is `ROOT_DIR` (i.e. an empty path), returns self. - pub fn find_directory

( - &self, - path: &P, - repo: &Repository, - ) -> Result, error::Directory> + pub fn find_directory

(&self, path: &P, repo: &Repository) -> Result where P: AsRef, { if path.as_ref() == Path::new(ROOT_DIR) { - return Ok(Some(self.clone())); + return Ok(self.clone()); } - Ok(match self.find_entry(path, repo)? { - Some(Entry::Directory(d)) => Some(d), - _ => None, - }) + match self.find_entry(path, repo)? { + Entry::Directory(d) => Ok(d), + _ => Err(error::Directory::InvalidType( + path.as_ref().to_path_buf(), + "Entry must be a directory", + )), + } } // TODO(fintan): This is going to be a bit trickier so going to leave it out for diff --git a/radicle-surf/src/repo.rs b/radicle-surf/src/repo.rs index 0315f39..adf90fc 100644 --- a/radicle-surf/src/repo.rs +++ b/radicle-surf/src/repo.rs @@ -258,15 +258,13 @@ impl Repository { path: &P, ) -> Result { let root = self.root_dir(commit)?; - root.find_directory(path, self)? - .ok_or_else(|| Error::PathNotFound(path.as_ref().to_path_buf())) + Ok(root.find_directory(path, self)?) } /// Returns a [`File`] for `path` in `commit`. pub fn file>(&self, commit: C, path: &P) -> Result { let root = self.root_dir(commit)?; - root.find_file(path, self)? - .ok_or_else(|| Error::PathNotFound(path.as_ref().to_path_buf())) + Ok(root.find_file(path, self)?) } /// Returns a [`Tree`] for `path` in `commit`. diff --git a/radicle-surf/t/src/file_system.rs b/radicle-surf/t/src/file_system.rs index d9811f1..d4885d2 100644 --- a/radicle-surf/t/src/file_system.rs +++ b/radicle-surf/t/src/file_system.rs @@ -22,32 +22,37 @@ mod directory { // find_entry for a file. let path = Path::new("src/memory.rs"); let entry = root.find_entry(&path, &repo).unwrap(); - assert!(matches!(entry, Some(fs::Entry::File(_)))); + assert!(matches!(entry, fs::Entry::File(_))); // find_entry for a directory. let path = Path::new("this/is/a/really/deeply/nested/directory/tree"); let entry = root.find_entry(&path, &repo).unwrap(); - assert!(matches!(entry, Some(fs::Entry::Directory(_)))); + assert!(matches!(entry, fs::Entry::Directory(_))); // find_entry for a non-leaf directory and its relative path. let path = Path::new("text"); let entry = root.find_entry(&path, &repo).unwrap(); - assert!(matches!(entry, Some(fs::Entry::Directory(_)))); - if let Some(fs::Entry::Directory(sub_dir)) = entry { + assert!(matches!(entry, fs::Entry::Directory(_))); + if let fs::Entry::Directory(sub_dir) = entry { let inner_path = Path::new("garden.txt"); let inner_entry = sub_dir.find_entry(&inner_path, &repo).unwrap(); - assert!(matches!(inner_entry, Some(fs::Entry::File(_)))); + assert!(matches!(inner_entry, fs::Entry::File(_))); } // find_entry for non-existing file let path = Path::new("this/is/a/really/missing_file"); - let result = root.find_entry(&path, &repo).unwrap(); - assert_eq!(result, None); + let result = root.find_entry(&path, &repo); + assert!(matches!(result, Err(fs::error::Directory::PathNotFound(_)))); // find_entry for absolute path: fail. let path = Path::new("/src/memory.rs"); let result = root.find_entry(&path, &repo); assert!(result.is_err()); + + // find entry for an empty path + let path = Path::new(""); + let result = root.find_entry(&path, &repo); + assert!(result.is_err()); } #[test] @@ -59,10 +64,7 @@ mod directory { .unwrap(); // Assert that we can find the memory.rs file! - assert!(root - .find_file(&Path::new("src/memory.rs"), &repo) - .unwrap() - .is_some()); + assert!(root.find_file(&Path::new("src/memory.rs"), &repo).is_ok()); let root_contents: Vec = root.entries(&repo).unwrap().collect(); assert_eq!(root_contents.len(), 7); @@ -82,10 +84,7 @@ mod directory { assert_eq!(root_contents[5].name(), "text"); assert_eq!(root_contents[6].name(), "this"); - let src = root - .find_directory(&Path::new("src"), &repo) - .unwrap() - .unwrap(); + let src = root.find_directory(&Path::new("src"), &repo).unwrap(); assert_eq!(src.path(), Path::new("src").to_path_buf()); let src_contents: Vec = src.entries(&repo).unwrap().collect(); assert_eq!(src_contents.len(), 3); @@ -109,8 +108,8 @@ mod directory { let path = Path::new("src"); let entry = root.find_entry(&path, &repo).unwrap(); - assert!(matches!(entry, Some(fs::Entry::Directory(_)))); - if let Some(fs::Entry::Directory(d)) = entry { + assert!(matches!(entry, fs::Entry::Directory(_))); + if let fs::Entry::Directory(d) = entry { assert_eq!(16297, d.size(&repo).unwrap()); } } @@ -120,7 +119,7 @@ mod directory { let repo = Repository::open(GIT_PLATINUM).unwrap(); let branch = Branch::local(refname!("dev")); let root = repo.root_dir(&branch).unwrap(); - let dir = root.find_directory(&"this/is", &repo).unwrap().unwrap(); + let dir = root.find_directory(&"this/is", &repo).unwrap(); let last_commit = repo.last_commit(&dir.path(), &branch).unwrap().unwrap(); assert_eq!( last_commit.id.to_string(), @@ -135,10 +134,7 @@ mod directory { let root = repo.root_dir(&branch).unwrap(); // Find a file with "\" in its name. - let f = root - .find_file(&"special/faux\\path", &repo) - .unwrap() - .unwrap(); + let f = root.find_file(&"special/faux\\path", &repo).unwrap(); let last_commit = repo.last_commit(&f.path(), &branch).unwrap().unwrap(); assert_eq!( last_commit.id.to_string(),