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 <keepsimple@gmail.com>

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<FileContent<'a>, 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<Option<Self>, 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<Entries, error::Directory> {
-        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<P>(
         &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::<git2::Error, _, _>(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<Oid, Self::Error> {
-        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"\\")
+}

radicle-surf/src/object/tree.rs

@@ -210,8 +210,8 @@ impl Eq for TreeEntry {}
 impl From<fs::Entry> 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()),
         }
     }
 }

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

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"
+        );
+    }
 }