From 83b95d6928a86b60842bb9ab6571a5e3d3ef6a85 Mon Sep 17 00:00:00 2001 From: Han Xu Date: Thu, 5 Jan 2023 11:22:45 -0800 Subject: [PATCH] remove last_commit() in Directory and File. Users would call Repository::last_commit(). Also renamed "id" to "tree_id" in Directory and "blob_id" in File. Signed-off-by: Han Xu --- radicle-surf/src/fs.rs | 89 +++++++++++++------------------ radicle-surf/src/object/tree.rs | 4 +- radicle-surf/src/repo.rs | 6 +-- radicle-surf/t/src/file_system.rs | 23 +++++++- 4 files changed, 62 insertions(+), 60 deletions(-) diff --git a/radicle-surf/src/fs.rs b/radicle-surf/src/fs.rs index c0020a6..66c46dc 100644 --- a/radicle-surf/src/fs.rs +++ b/radicle-surf/src/fs.rs @@ -31,9 +31,11 @@ use git2::Blob; use radicle_git_ext::{is_not_found_err, Oid}; use radicle_std_ext::result::ResultExt as _; -use crate::{Commit, Repository, Revision}; +use crate::{Repository, Revision}; pub mod error { + use std::path::PathBuf; + use thiserror::Error; #[derive(Debug, Error, PartialEq)] @@ -45,7 +47,7 @@ pub mod error { #[error(transparent)] File(#[from] File), #[error("the path {0} is not valid")] - InvalidPath(String), + InvalidPath(PathBuf), } #[derive(Debug, Error, PartialEq, Eq)] @@ -58,6 +60,8 @@ pub mod error { pub enum File { #[error(transparent)] Git(#[from] git2::Error), + #[error("the path {0} is not valid")] + InvalidPath(PathBuf), } } @@ -78,9 +82,7 @@ pub struct File { /// in respect to the root of the git repository. prefix: PathBuf, /// The object identifier of the git blob of this file. - id: Oid, - /// The commit that created this file version. - last_commit: Commit, + blob_id: Oid, } impl File { @@ -90,7 +92,7 @@ impl File { /// so should not end in `name`. /// /// The `id` must point to a git blob. - pub(crate) fn new(name: String, prefix: PathBuf, id: Oid, last_commit: Commit) -> Self { + pub(crate) fn new(name: String, prefix: PathBuf, id: Oid) -> Self { debug_assert!( !prefix.ends_with(&name), "prefix = {:?}, name = {}", @@ -100,8 +102,7 @@ impl File { Self { name, prefix, - id, - last_commit, + blob_id: id, } } @@ -111,8 +112,8 @@ impl File { } /// The object identifier of this `File`. - pub fn id(&self) -> Oid { - self.id + pub fn blob_id(&self) -> Oid { + self.blob_id } /// Return the exact path for this `File`, including the `name` of @@ -120,7 +121,7 @@ impl File { /// /// The path is relative to the git repository root. pub fn path(&self) -> PathBuf { - self.prefix.join(&self.name) + self.prefix.join(escaped_name(&self.name)) } /// Return the [`Path`] where this `File` is located, relative to the @@ -136,7 +137,7 @@ impl File { /// This function will fail if it could not find the `git` blob /// for the `Oid` of this `File`. pub fn content<'a>(&self, repo: &'a Repository) -> Result, error::File> { - let blob = repo.find_blob(self.id)?; + let blob = repo.find_blob(self.blob_id)?; Ok(FileContent { blob }) } } @@ -263,26 +264,13 @@ impl Entry { pub(crate) fn from_entry( entry: &git2::TreeEntry, path: PathBuf, - repo: &Repository, - parent_commit: Oid, ) -> Result, error::Entry> { let name = entry.name().ok_or(error::Entry::Utf8Error)?.to_string(); let id = entry.id().into(); - let escaped_name = name.replace('\\', r"\\"); - let entry_path = path.join(escaped_name); - // FIXME: I don't like to use FIXME, but here it is. I would - // like to simplify the error definitions and then fix these - // unwrap(s). - let last_commit = repo - .last_commit(&entry_path, parent_commit) - .unwrap() - .unwrap(); Ok(entry.kind().and_then(|kind| match kind { - git2::ObjectType::Tree => { - Some(Self::Directory(Directory::new(name, path, id, last_commit))) - }, - git2::ObjectType::Blob => Some(Self::File(File::new(name, path, id, last_commit))), + git2::ObjectType::Tree => Some(Self::Directory(Directory::new(name, path, id))), + git2::ObjectType::Blob => Some(Self::File(File::new(name, path, id))), _ => None, })) } @@ -305,9 +293,7 @@ pub struct Directory { /// in respect to the root of the git repository. prefix: PathBuf, /// The object identifier of the git tree of this directory. - id: Oid, - /// The commit that created this directory version. - last_commit: Commit, + tree_id: Oid, } const ROOT_DIR: &str = ""; @@ -316,8 +302,8 @@ impl Directory { /// Creates a directory given its `tree_id`. /// /// The `name` and `prefix` are both set to be empty. - pub(crate) fn root(tree_id: Oid, repo_commit: Commit) -> Self { - Self::new(ROOT_DIR.to_string(), PathBuf::new(), tree_id, repo_commit) + pub(crate) fn root(tree_id: Oid) -> Self { + Self::new(ROOT_DIR.to_string(), PathBuf::new(), tree_id) } /// Creates a directory given its `name` and `id`. @@ -326,7 +312,7 @@ impl Directory { /// so should not end in `name`. /// /// The `id` must point to a `git` tree. - pub(crate) fn new(name: String, prefix: PathBuf, id: Oid, last_commit: Commit) -> Self { + pub(crate) fn new(name: String, prefix: PathBuf, tree_id: Oid) -> Self { debug_assert!( name.is_empty() || !prefix.ends_with(&name), "prefix = {:?}, name = {}", @@ -336,8 +322,7 @@ impl Directory { Self { name, prefix, - id, - last_commit, + tree_id, } } @@ -346,9 +331,9 @@ impl Directory { &self.name } - /// The object identifier of this `File`. - pub fn id(&self) -> Oid { - self.id + /// The object identifier of this `[Directory]`. + pub fn tree_id(&self) -> Oid { + self.tree_id } /// Return the exact path for this `Directory`, including the `name` of the @@ -356,7 +341,7 @@ impl Directory { /// /// The path is relative to the git repository root. pub fn path(&self) -> PathBuf { - self.prefix.join(&self.name) + self.prefix.join(escaped_name(&self.name)) } /// Return the [`Path`] where this `Directory` is located, relative to the @@ -376,7 +361,7 @@ impl Directory { /// This function will fail if it could not find the `git` tree /// for the `Oid`. pub fn entries(&self, repo: &Repository) -> Result { - let tree = repo.find_tree(self.id)?; + let tree = repo.find_tree(self.tree_id)?; let mut entries = BTreeMap::new(); let mut error = None; @@ -385,7 +370,7 @@ impl Directory { // Walks only the first level of entries. And `_entry_path` is always // empty for the first level. tree.walk(git2::TreeWalkMode::PreOrder, |_entry_path, entry| { - match Entry::from_entry(entry, path.clone(), repo, self.last_commit.id) { + match Entry::from_entry(entry, path.clone()) { Ok(Some(entry)) => match entry { Entry::File(_) => { entries.insert(entry.name().clone(), entry); @@ -411,11 +396,6 @@ impl Directory { } } - /// Returns the last commit that created or modified this directory. - pub fn last_commit(&self) -> &Commit { - &self.last_commit - } - /// Find the [`Entry`] found at `path`, if it exists. pub fn find_entry

( &self, @@ -427,21 +407,18 @@ impl Directory { { // Search the path in git2 tree. let path = path.as_ref(); - let git2_tree = repo.find_tree(self.id)?; + let git2_tree = repo.find_tree(self.tree_id)?; let entry = git2_tree .get_path(path) .map(Some) .or_matches::(is_not_found_err, || Ok(None))?; let parent = path .parent() - .ok_or_else(|| error::Directory::InvalidPath(path.to_string_lossy().to_string()))?; + .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(), repo, self.last_commit.id) - .transpose() - }) + .and_then(|entry| Entry::from_entry(&entry, root_path.to_path_buf()).transpose()) .transpose() .unwrap()) } @@ -535,6 +512,12 @@ impl Revision for Directory { type Error = Infallible; fn object_id(&self, _repo: &Repository) -> Result { - Ok(self.id) + Ok(self.tree_id) } } + +/// When we need to escape "\" (represented as `\\`) for `PathBuf` +/// so that it can be processed correctly. +fn escaped_name(name: &str) -> String { + name.replace('\\', r"\\") +} diff --git a/radicle-surf/src/object/tree.rs b/radicle-surf/src/object/tree.rs index 3378edd..738089c 100644 --- a/radicle-surf/src/object/tree.rs +++ b/radicle-surf/src/object/tree.rs @@ -210,8 +210,8 @@ impl Eq for TreeEntry {} impl From for Entry { fn from(entry: fs::Entry) -> Self { match entry { - fs::Entry::File(f) => Entry::Blob(f.id()), - fs::Entry::Directory(d) => Entry::Tree(d.id()), + fs::Entry::File(f) => Entry::Blob(f.blob_id()), + fs::Entry::Directory(d) => Entry::Tree(d.tree_id()), } } } diff --git a/radicle-surf/src/repo.rs b/radicle-surf/src/repo.rs index dc70466..c3ba38b 100644 --- a/radicle-surf/src/repo.rs +++ b/radicle-surf/src/repo.rs @@ -248,7 +248,7 @@ impl Repository { .map_err(|err| Error::ToCommit(err.into()))?; let git2_commit = self.inner.find_commit((commit.id).into())?; let tree = git2_commit.as_object().peel_to_tree()?; - Ok(Directory::root(tree.id().into(), commit)) + Ok(Directory::root(tree.id().into())) } /// Returns a [`Directory`] for `path` in `commit`. @@ -293,7 +293,7 @@ impl Repository { .last_commit(path, commit)? .ok_or_else(|| Error::PathNotFound(path.as_ref().to_path_buf()))?; let header = Header::from(last_commit); - Ok(Tree::new(dir.id(), entries, header)) + Ok(Tree::new(dir.tree_id(), entries, header)) } /// Returns a [`Blob`] for `path` in `commit`. @@ -308,7 +308,7 @@ impl Repository { let header = Header::from(last_commit); let content = file.content(self)?; - Ok(Blob::new(file.id(), content.as_bytes(), header)) + Ok(Blob::new(file.blob_id(), content.as_bytes(), header)) } /// Returns the last commit, if exists, for a `path` in the history of diff --git a/radicle-surf/t/src/file_system.rs b/radicle-surf/t/src/file_system.rs index 5a86c52..d9811f1 100644 --- a/radicle-surf/t/src/file_system.rs +++ b/radicle-surf/t/src/file_system.rs @@ -118,12 +118,31 @@ mod directory { #[test] fn directory_last_commit() { let repo = Repository::open(GIT_PLATINUM).unwrap(); - let root = repo.root_dir(Branch::local(refname!("dev"))).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 last_commit = dir.last_commit(); + let last_commit = repo.last_commit(&dir.path(), &branch).unwrap().unwrap(); assert_eq!( last_commit.id.to_string(), "2429f097664f9af0c5b7b389ab998b2199ffa977" ); } + + #[test] + fn file_last_commit() { + let repo = Repository::open(GIT_PLATINUM).unwrap(); + let branch = Branch::local(refname!("master")); + 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 last_commit = repo.last_commit(&f.path(), &branch).unwrap().unwrap(); + assert_eq!( + last_commit.id.to_string(), + "a0dd9122d33dff2a35f564d564db127152c88e02" + ); + } }