radicle-surf: reorganise errors

The current `error::Error` type aggregated a lot of different error
scenarios that were not for the actual repo actions, instead they
would be for the domains that the repo actions would return.

This is a reorganisation of those by localising error variants in
their modules:
* namespace::Error
* commit::Error
* glob::Error
* adding `type Error` to ToCommit
* adding `type Error` to Revision

The main Error type now consists of these other Error types and lives
in the `repo` module. It is re-exported at the top-level since
RepositoryRef is the main interaction with the library.

During this refactor, some reorganisation of functions was done as
well:
* Move `get_file` from `Commit` to `RepositoryRef` as
`get_commit_file`.
* Have a blanket implementation of `ToCommit` for any `Revision`,
instead of repeating the implementations.

Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
X-Clacks-Overhead: GNU Terry Pratchett

radicle-surf/src/commit.rs

@@ -27,7 +27,7 @@ use serde::{
 use crate::{
     diff,
     file_system,
-    git::{self, Glob, RepositoryRef},
+    git::{self, glob, Glob, RepositoryRef},
     person::Person,
     revision::Revision,
 };
@@ -251,7 +251,10 @@ pub enum Error {
 
     /// An error occurred during a git operation.
     #[error(transparent)]
-    Git(#[from] git::error::Error),
+    Git(#[from] git::Error),
+
+    #[error(transparent)]
+    Glob(#[from] glob::Error),
 
     /// Trying to find a file path which could not be found.
     #[error("the path '{0}' was not found")]

radicle-surf/src/file_system/directory.rs

@@ -26,7 +26,11 @@ use crate::{
 };
 use git2::Blob;
 use radicle_git_ext::Oid;
-use std::{collections::BTreeMap, convert::TryFrom, path};
+use std::{
+    collections::BTreeMap,
+    convert::{Infallible, TryFrom},
+    path,
+};
 
 /// Represents a `file` in a git repo.
 #[derive(Clone, PartialEq, Eq, Debug)]
@@ -289,7 +293,9 @@ impl Directory {
 }
 
 impl Revision for Directory {
-    fn object_id(&self, _repo: &RepositoryRef) -> Result<Oid, git::Error> {
+    type Error = Infallible;
+
+    fn object_id(&self, _repo: &RepositoryRef) -> Result<Oid, Self::Error> {
         Ok(self.oid)
     }
 }

radicle-surf/src/git.rs

@@ -63,7 +63,7 @@
 //! # }
 //! ```
 
-use std::str::FromStr;
+use std::{convert::Infallible, str::FromStr};
 
 // Re-export git2 as sub-module
 pub use git2::{self, Error as Git2Error, Time};
@@ -71,17 +71,14 @@ use git_ref_format::{name::Components, Component, Qualified, RefString};
 pub use radicle_git_ext::Oid;
 
 mod repo;
-pub use repo::{Repository, RepositoryRef};
+pub use repo::{Error, Repository, RepositoryRef};
 
-mod glob;
+pub mod glob;
 pub use glob::Glob;
 
 mod history;
 pub use history::History;
 
-pub mod error;
-pub use error::Error;
-
 /// Provides the data for talking about branches.
 pub mod branch;
 pub use branch::{Branch, Local, Remote};
@@ -116,57 +113,100 @@ impl From<git2::Buf> for Signature {
 
 /// Supports various ways to specify a revision used in Git.
 pub trait Revision {
+    type Error: std::error::Error + Send + Sync + 'static;
+
     /// Returns the object id of this revision in `repo`.
-    fn object_id(&self, repo: &RepositoryRef) -> Result<Oid, Error>;
+    fn object_id(&self, repo: &RepositoryRef) -> Result<Oid, Self::Error>;
 }
 
 impl Revision for RefString {
-    fn object_id(&self, repo: &RepositoryRef) -> Result<Oid, Error> {
-        repo.refname_to_oid(self.as_str())
+    type Error = git2::Error;
+
+    fn object_id(&self, repo: &RepositoryRef) -> Result<Oid, Self::Error> {
+        repo.repo_ref.refname_to_id(self.as_str()).map(Oid::from)
     }
 }
 
 impl Revision for &RefString {
-    fn object_id(&self, repo: &RepositoryRef) -> Result<Oid, Error> {
-        repo.refname_to_oid(self.as_str())
+    type Error = git2::Error;
+
+    fn object_id(&self, repo: &RepositoryRef) -> Result<Oid, Self::Error> {
+        repo.repo_ref.refname_to_id(self.as_str()).map(Oid::from)
     }
 }
 
 impl Revision for Qualified<'_> {
-    fn object_id(&self, repo: &RepositoryRef) -> Result<Oid, Error> {
-        repo.refname_to_oid(self.as_str())
+    type Error = git2::Error;
+
+    fn object_id(&self, repo: &RepositoryRef) -> Result<Oid, Self::Error> {
+        repo.repo_ref.refname_to_id(self.as_str()).map(Oid::from)
     }
 }
 
 impl Revision for &Qualified<'_> {
-    fn object_id(&self, repo: &RepositoryRef) -> Result<Oid, Error> {
-        repo.refname_to_oid(self.as_str())
+    type Error = git2::Error;
+
+    fn object_id(&self, repo: &RepositoryRef) -> Result<Oid, Self::Error> {
+        repo.repo_ref.refname_to_id(self.as_str()).map(Oid::from)
     }
 }
 
 impl Revision for Oid {
-    fn object_id(&self, _repo: &RepositoryRef) -> Result<Oid, Error> {
+    type Error = Infallible;
+
+    fn object_id(&self, _repo: &RepositoryRef) -> Result<Oid, Self::Error> {
         Ok(*self)
     }
 }
 
 impl Revision for &str {
-    fn object_id(&self, _repo: &RepositoryRef) -> Result<Oid, Error> {
-        Oid::from_str(self).map_err(Error::Git)
+    type Error = git2::Error;
+
+    fn object_id(&self, _repo: &RepositoryRef) -> Result<Oid, Self::Error> {
+        Oid::from_str(self).map(Oid::from)
     }
 }
 
 impl Revision for &Branch {
-    fn object_id(&self, repo: &RepositoryRef) -> Result<Oid, Error> {
+    type Error = Error;
+
+    fn object_id(&self, repo: &RepositoryRef) -> Result<Oid, Self::Error> {
         let refname = repo.namespaced_refname(&self.refname())?;
         Ok(repo.repo_ref.refname_to_id(&refname).map(Oid::from)?)
     }
 }
 
 impl Revision for &Tag {
-    fn object_id(&self, repo: &RepositoryRef) -> Result<Oid, Error> {
-        let refname = repo.namespaced_refname(&self.refname())?;
-        Ok(repo.repo_ref.refname_to_id(&refname).map(Oid::from)?)
+    type Error = Infallible;
+
+    fn object_id(&self, _repo: &RepositoryRef) -> Result<Oid, Self::Error> {
+        Ok(self.id())
+    }
+}
+
+/// A common trait for anything that can convert to a `Commit`.
+pub trait ToCommit {
+    type Error: std::error::Error + Send + Sync + 'static;
+
+    /// Converts to a commit in `repo`.
+    fn to_commit(self, repo: &RepositoryRef) -> Result<Commit, Self::Error>;
+}
+
+impl ToCommit for Commit {
+    type Error = Infallible;
+
+    fn to_commit(self, _repo: &RepositoryRef) -> Result<Commit, Self::Error> {
+        Ok(self)
+    }
+}
+
+impl<R: Revision> ToCommit for R {
+    type Error = Error;
+
+    fn to_commit(self, repo: &RepositoryRef) -> Result<Commit, Self::Error> {
+        let oid = repo.object_id(&self)?;
+        let commit = repo.repo_ref.find_commit(oid.into())?;
+        Ok(Commit::try_from(commit)?)
     }
 }
 

radicle-surf/src/git/commit.rs

@@ -15,17 +15,24 @@
 // You should have received a copy of the GNU General Public License
 // along with this program. If not, see <https://www.gnu.org/licenses/>.
 
-use crate::{
-    file_system::{self, directory},
-    git::{error::Error, Branch, RepositoryRef, Tag},
-};
-use git_ref_format::{Qualified, RefString};
-use radicle_git_ext::Oid;
 use std::{convert::TryFrom, str};
 
+use radicle_git_ext::Oid;
+use thiserror::Error;
+
 #[cfg(feature = "serialize")]
 use serde::{ser::SerializeStruct, Deserialize, Deserializer, Serialize, Serializer};
 
+#[derive(Debug, Error)]
+pub enum Error {
+    /// When trying to get the summary for a [`git2::Commit`] some action
+    /// failed.
+    #[error("an error occurred trying to get a commit's summary")]
+    MissingSummary,
+    #[error(transparent)]
+    Utf8Error(#[from] str::Utf8Error),
+}
+
 /// `Author` is the static information of a [`git2::Signature`].
 #[cfg_attr(feature = "serialize", derive(Deserialize, Serialize))]
 #[derive(Clone, PartialEq, Eq, PartialOrd, Ord)]
@@ -120,16 +127,6 @@ impl Commit {
             .unwrap_or(&self.message)
             .trim()
     }
-
-    /// Retrieves the file with `path` in this commit.
-    pub fn get_file<'a>(
-        &'a self,
-        repo: &'a RepositoryRef,
-        path: file_system::Path,
-    ) -> Result<directory::FileContent, Error> {
-        let git2_commit = repo.get_git2_commit(self.id)?;
-        repo.get_commit_file(&git2_commit, path)
-    }
 }
 
 #[cfg(feature = "serialize")]
@@ -180,51 +177,3 @@ impl<'repo> TryFrom<git2::Commit<'repo>> for Commit {
         })
     }
 }
-
-/// A common trait for anything that can convert to a `Commit`.
-pub trait ToCommit {
-    /// Converts to a commit in `repo`.
-    fn to_commit(self, repo: &RepositoryRef) -> Result<Commit, Error>;
-}
-
-impl ToCommit for Commit {
-    fn to_commit(self, _repo: &RepositoryRef) -> Result<Commit, Error> {
-        Ok(self)
-    }
-}
-
-impl ToCommit for &str {
-    fn to_commit(self, repo: &RepositoryRef) -> Result<Commit, Error> {
-        repo.commit(self)
-    }
-}
-
-impl ToCommit for Oid {
-    fn to_commit(self, repo: &RepositoryRef) -> Result<Commit, Error> {
-        repo.commit(self)
-    }
-}
-
-impl ToCommit for &Branch {
-    fn to_commit(self, repo: &RepositoryRef) -> Result<Commit, Error> {
-        repo.commit(self)
-    }
-}
-
-impl ToCommit for &Tag {
-    fn to_commit(self, repo: &RepositoryRef) -> Result<Commit, Error> {
-        repo.commit(self)
-    }
-}
-
-impl ToCommit for &RefString {
-    fn to_commit(self, repo: &RepositoryRef) -> Result<Commit, Error> {
-        repo.commit(self)
-    }
-}
-
-impl ToCommit for &Qualified<'_> {
-    fn to_commit(self, repo: &RepositoryRef) -> Result<Commit, Error> {
-        repo.commit(self)
-    }
-}

radicle-surf/src/git/error.rs

@@ -1,81 +0,0 @@
-// This file is part of radicle-surf
-// <https://github.com/radicle-dev/radicle-surf>
-//
-// Copyright (C) 2019-2020 The Radicle Team <dev@radicle.xyz>
-//
-// This program is free software: you can redistribute it and/or modify
-// it under the terms of the GNU General Public License version 3 or
-// later as published by the Free Software Foundation.
-//
-// This program is distributed in the hope that it will be useful,
-// but WITHOUT ANY WARRANTY; without even the implied warranty of
-// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-// GNU General Public License for more details.
-//
-// You should have received a copy of the GNU General Public License
-// along with this program. If not, see <https://www.gnu.org/licenses/>.
-
-//! Collection of errors and helper instances that can occur when performing
-//! operations from [`crate::git`].
-
-use crate::{diff, file_system, git, git::Namespace};
-use std::str;
-use thiserror::Error;
-
-/// Enumeration of errors that can occur in operations from [`crate::git`].
-#[derive(Debug, Error)]
-#[non_exhaustive]
-pub enum Error {
-    #[error(transparent)]
-    BranchIter(#[from] git::repo::iter::error::Branch),
-    #[error(transparent)]
-    TagIter(#[from] git::repo::iter::error::Tag),
-    /// A `revspec` was provided that could not be parsed into a branch, tag, or
-    /// commit object.
-    #[error("provided revspec '{rev}' could not be parsed into a git object")]
-    RevParseFailure {
-        /// The provided revspec that failed to parse.
-        rev: String,
-    },
-    /// A `revspec` was provided that could not be found in the given
-    /// `namespace`.
-    #[error("provided revspec '{rev}' could not be parsed into a git object in the namespace '{namespace}'")]
-    NamespaceRevParseFailure {
-        /// The namespace we are in when attempting to fetch the `rev`.
-        namespace: Namespace,
-        /// The provided revspec that failed to parse.
-        rev: String,
-    },
-    /// When parsing a namespace we may come across one that was an empty
-    /// string.
-    #[error("tried parsing the namespace but it was empty")]
-    EmptyNamespace,
-    /// A [`str::Utf8Error`] error, which usually occurs when a git object's
-    /// name is not in UTF-8 form and parsing of it as such fails.
-    #[error(transparent)]
-    Utf8Error(#[from] str::Utf8Error),
-    /// When trying to get the summary for a [`git2::Commit`] some action
-    /// failed.
-    #[error("an error occurred trying to get a commit's summary")]
-    MissingSummary,
-    /// An error that comes from performing a [`crate::file_system`] operation.
-    #[error(transparent)]
-    FileSystem(#[from] file_system::Error),
-    /// While attempting to calculate a diff for retrieving the
-    /// [`crate::vcs::git::Browser.last_commit()`], the file path was returned
-    /// as an `Option::None`.
-    #[error("last commit has an invalid file path")]
-    LastCommitException,
-    /// The requested file was not found.
-    #[error("path not found for: {0}")]
-    PathNotFound(file_system::Path),
-    /// An error that comes from performing a *diff* operations.
-    #[error(transparent)]
-    Diff(#[from] diff::git::error::Diff),
-    /// A wrapper around the generic [`git2::Error`].
-    #[error(transparent)]
-    Git(#[from] git2::Error),
-    /// A wrapper around git-ref-format::Error
-    #[error(transparent)]
-    RefFormat(#[from] git_ref_format::Error),
-}

radicle-surf/src/git/glob.rs

@@ -15,10 +15,17 @@
 // You should have received a copy of the GNU General Public License
 // along with this program. If not, see <https://www.gnu.org/licenses/>.
 
-use crate::git::Error;
-use git_ref_format::refspec::PatternString;
 use std::{convert::TryFrom, marker::PhantomData, str};
 
+use git_ref_format::refspec::PatternString;
+use thiserror::Error;
+
+#[derive(Debug, Error)]
+pub enum Error {
+    #[error(transparent)]
+    RefFormat(#[from] git_ref_format::Error),
+}
+
 /// A collection of globs for T (a git reference type).
 pub struct Glob<T> {
     globs: Vec<PatternString>,

radicle-surf/src/git/history.rs

@@ -17,7 +17,7 @@
 
 use crate::{
     file_system,
-    git::{commit::ToCommit, Commit, Error, RepositoryRef},
+    git::{Commit, Error, RepositoryRef, ToCommit},
 };
 use std::convert::TryFrom;
 
@@ -38,7 +38,9 @@ enum FilterBy {
 impl<'a> History<'a> {
     /// Creates a new history starting from `head`, in `repo`.
     pub fn new<C: ToCommit>(repo: RepositoryRef<'a>, head: C) -> Result<Self, Error> {
-        let head = head.to_commit(&repo)?;
+        let head = head
+            .to_commit(&repo)
+            .map_err(|err| Error::ToCommit(err.into()))?;
         let mut revwalk = repo.repo_ref.revwalk()?;
         revwalk.push(head.id.into())?;
         let history = Self {

radicle-surf/src/git/namespace.rs

@@ -15,10 +15,21 @@
 // You should have received a copy of the GNU General Public License
 // along with this program. If not, see <https://www.gnu.org/licenses/>.
 
-use crate::git::error::Error;
+use std::{convert::TryFrom, fmt, str};
+
 use nonempty::NonEmpty;
 pub use radicle_git_ext::Oid;
-use std::{convert::TryFrom, fmt, str};
+use thiserror::Error;
+
+#[derive(Debug, Error)]
+pub enum Error {
+    /// When parsing a namespace we may come across one that was an empty
+    /// string.
+    #[error("tried parsing the namespace but it was empty")]
+    EmptyNamespace,
+    #[error(transparent)]
+    Utf8(#[from] str::Utf8Error),
+}
 
 /// A `Namespace` value allows us to switch the git namespace of
 /// a repo.

radicle-surf/src/git/repo.rs

@@ -15,17 +15,6 @@
 // You should have received a copy of the GNU General Public License
 // along with this program. If not, see <https://www.gnu.org/licenses/>.
 
-use crate::{
-    diff::*,
-    file_system,
-    file_system::{
-        directory::{self, Directory, DirectoryEntry, FileContent},
-        Label,
-    },
-    git::{error::*, Branch, Commit, Glob, History, Namespace, Revision, Signature, Stats, Tag},
-};
-use git_ref_format::RefString;
-use radicle_git_ext::Oid;
 use std::{
     collections::{BTreeMap, BTreeSet},
     convert::TryFrom,
@@ -33,11 +22,74 @@ use std::{
     str,
 };
 
-use super::commit::ToCommit;
+use directory::{Directory, FileContent};
+use git_ref_format::RefString;
+use radicle_git_ext::Oid;
+use thiserror::Error;
+
+use crate::{
+    diff::{self, *},
+    file_system,
+    file_system::{directory, DirectoryEntry, Label},
+    git::{
+        commit,
+        glob,
+        namespace,
+        Branch,
+        Commit,
+        Glob,
+        History,
+        Namespace,
+        Revision,
+        Signature,
+        Stats,
+        Tag,
+        ToCommit,
+    },
+};
 
 pub mod iter;
 pub use iter::{Branches, Namespaces, Tags};
 
+/// Enumeration of errors that can occur in operations from [`crate::git`].
+#[derive(Debug, Error)]
+#[non_exhaustive]
+pub enum Error {
+    #[error(transparent)]
+    Branches(#[from] iter::error::Branch),
+    #[error(transparent)]
+    Commit(#[from] commit::Error),
+    /// An error that comes from performing a *diff* operations.
+    #[error(transparent)]
+    Diff(#[from] diff::git::error::Diff),
+    /// An error that comes from performing a [`crate::file_system`] operation.
+    #[error(transparent)]
+    FileSystem(#[from] file_system::Error),
+    /// A wrapper around the generic [`git2::Error`].
+    #[error(transparent)]
+    Git(#[from] git2::Error),
+    #[error(transparent)]
+    Glob(#[from] glob::Error),
+    #[error(transparent)]
+    Namespace(#[from] namespace::Error),
+    /// The requested file was not found.
+    #[error("path not found for: {0}")]
+    PathNotFound(file_system::Path),
+    #[error(transparent)]
+    Revision(Box<dyn std::error::Error + Send + Sync + 'static>),
+    /// A `revspec` was provided that could not be parsed into a branch, tag, or
+    /// commit object.
+    #[error("provided revspec '{rev}' could not be parsed into a git object")]
+    RevParseFailure {
+        /// The provided revspec that failed to parse.
+        rev: String,
+    },
+    #[error(transparent)]
+    ToCommit(Box<dyn std::error::Error + Send + Sync + 'static>),
+    #[error(transparent)]
+    Tags(#[from] iter::error::Tag),
+}
+
 /// Wrapper around the `git2`'s `git2::Repository` type.
 /// This is to to limit the functionality that we can do
 /// on the underlying object.
@@ -72,7 +124,7 @@ impl<'a> RepositoryRef<'a> {
     pub fn which_namespace(&self) -> Result<Option<Namespace>, Error> {
         self.repo_ref
             .namespace_bytes()
-            .map(Namespace::try_from)
+            .map(|ns| Namespace::try_from(ns).map_err(Error::from))
             .transpose()
     }
 
@@ -108,7 +160,7 @@ impl<'a> RepositoryRef<'a> {
                 .map(|reference| {
                     reference
                         .map_err(Error::Git)
-                        .and_then(|r| Namespace::try_from(r).map_err(|_| Error::EmptyNamespace))
+                        .and_then(|r| Namespace::try_from(r).map_err(Error::from))
                 })
                 .collect::<Result<BTreeSet<Namespace>, Error>>()?;
             set.extend(new_set);
@@ -118,22 +170,24 @@ impl<'a> RepositoryRef<'a> {
 
     /// Get the [`Diff`] between two commits.
     pub fn diff(&self, from: impl Revision, to: impl Revision) -> Result<Diff, Error> {
-        let from_commit = self.get_git2_commit(from.object_id(self)?)?;
-        let to_commit = self.get_git2_commit(to.object_id(self)?)?;
+        let from_commit = self.get_git2_commit(self.object_id(&from)?)?;
+        let to_commit = self.get_git2_commit(self.object_id(&to)?)?;
         self.diff_commits(None, Some(&from_commit), &to_commit)
             .and_then(|diff| Diff::try_from(diff).map_err(Error::from))
     }
 
     /// Get the [`Diff`] of a commit with no parents.
     pub fn initial_diff<R: Revision>(&self, rev: R) -> Result<Diff, Error> {
-        let commit = self.get_git2_commit(rev.object_id(self)?)?;
+        let commit = self.get_git2_commit(self.object_id(&rev)?)?;
         self.diff_commits(None, None, &commit)
             .and_then(|diff| Diff::try_from(diff).map_err(Error::from))
     }
 
     /// Get the diff introduced by a particlar rev.
     pub fn diff_from_parent<C: ToCommit>(&self, commit: C) -> Result<Diff, Error> {
-        let commit = commit.to_commit(self)?;
+        let commit = commit
+            .to_commit(self)
+            .map_err(|err| Error::ToCommit(err.into()))?;
         match commit.parents.first() {
             Some(parent) => self.diff(*parent, commit.id),
             None => self.initial_diff(commit.id),
@@ -150,7 +204,9 @@ impl<'a> RepositoryRef<'a> {
     /// To visit inside any nested sub-directories, call `directory.get(&repo)`
     /// on the sub-directory.
     pub fn root_dir<C: ToCommit>(&self, commit: C) -> Result<Directory, Error> {
-        let commit = commit.to_commit(self)?;
+        let commit = commit
+            .to_commit(self)
+            .map_err(|err| Error::ToCommit(err.into()))?;
         let git2_commit = self.repo_ref.find_commit((commit.id).into())?;
         let tree = git2_commit.as_object().peel_to_tree()?;
         Ok(Directory {
@@ -222,11 +278,7 @@ impl<'a> RepositoryRef<'a> {
 
     /// Returns a commit for `rev` if exists.
     pub fn commit<R: Revision>(&self, rev: R) -> Result<Commit, Error> {
-        let oid = rev.object_id(self)?;
-        match self.repo_ref.find_commit(oid.into()) {
-            Ok(commit) => Commit::try_from(commit),
-            Err(e) => Err(Error::Git(e)),
-        }
+        rev.to_commit(self)
     }
 
     /// Gets stats of `commit`.
@@ -264,6 +316,21 @@ impl<'a> RepositoryRef<'a> {
         Ok(blob.size())
     }
 
+    /// Retrieves the file with `path` in this commit.
+    pub fn get_commit_file<R: Revision>(
+        &self,
+        rev: &R,
+        path: file_system::Path,
+    ) -> Result<FileContent, crate::git::Error> {
+        let id = self.object_id(rev)?;
+        let commit = self.get_git2_commit(id)?;
+        let tree = commit.tree()?;
+        let entry = tree.get_path(PathBuf::from(&path).as_ref())?;
+        let object = entry.to_object(self.repo_ref)?;
+        let blob = object.into_blob().map_err(|_| Error::PathNotFound(path))?;
+        Ok(FileContent::new(blob))
+    }
+
     /// Lists branch names with `filter`.
     pub fn branch_names(&self, filter: &Glob<Branch>) -> Result<Vec<RefString>, Error> {
         let mut branches = self
@@ -307,11 +374,6 @@ impl<'a> RepositoryRef<'a> {
         Ok(fullname)
     }
 
-    pub(crate) fn refname_to_oid(&self, refname: &str) -> Result<Oid, Error> {
-        let oid = self.repo_ref.refname_to_id(refname)?;
-        Ok(oid.into())
-    }
-
     /// Get a particular `git2::Commit` of `oid`.
     pub(crate) fn get_git2_commit(&self, oid: Oid) -> Result<git2::Commit, Error> {
         self.repo_ref.find_commit(oid.into()).map_err(Error::Git)
@@ -368,18 +430,6 @@ impl<'a> RepositoryRef<'a> {
         Ok(other == git2_oid || is_descendant)
     }
 
-    pub(crate) fn get_commit_file(
-        &self,
-        git2_commit: &git2::Commit,
-        path: file_system::Path,
-    ) -> Result<FileContent, Error> {
-        let git2_tree = git2_commit.tree()?;
-        let entry = git2_tree.get_path(PathBuf::from(&path).as_ref())?;
-        let object = entry.to_object(self.repo_ref)?;
-        let blob = object.into_blob().map_err(|_| Error::PathNotFound(path))?;
-        Ok(FileContent::new(blob))
-    }
-
     pub(crate) fn diff_commit_and_parents(
         &self,
         path: &file_system::Path,
@@ -427,6 +477,10 @@ impl<'a> RepositoryRef<'a> {
     pub fn history<C: ToCommit>(&self, head: C) -> Result<History, Error> {
         History::new(*self, head)
     }
+
+    pub(super) fn object_id<R: Revision>(&self, r: &R) -> Result<Oid, Error> {
+        r.object_id(self).map_err(|err| Error::Revision(err.into()))
+    }
 }
 
 impl<'a> std::fmt::Debug for RepositoryRef<'a> {

radicle-surf/src/object.rs

@@ -90,7 +90,7 @@ pub enum Error {
 
     /// An error occurred during a git operation.
     #[error(transparent)]
-    Git(#[from] git::error::Error),
+    Git(#[from] git::Error),
 
     /// Trying to find a file path which could not be found.
     #[error("the path '{0}' was not found")]

radicle-surf/src/revision.rs

@@ -25,7 +25,7 @@ use serde::{Deserialize, Serialize};
 
 use radicle_git_ext::Oid;
 
-use crate::git::{self, commit::ToCommit, error::Error, Glob, RepositoryRef};
+use crate::git::{self, Error, Glob, RepositoryRef};
 
 /// Types of a peer.
 pub enum Category<P, U> {
@@ -76,7 +76,9 @@ pub enum Revision {
 }
 
 impl git::Revision for &Revision {
-    fn object_id(&self, repo: &RepositoryRef) -> Result<Oid, Error> {
+    type Error = git2::Error;
+
+    fn object_id(&self, repo: &RepositoryRef) -> Result<Oid, Self::Error> {
         match self {
             Revision::Tag { name } => match name.qualified() {
                 None => Qualified::from(lit::refs_tags(name)).object_id(repo),
@@ -93,12 +95,6 @@ impl git::Revision for &Revision {
     }
 }
 
-impl ToCommit for &Revision {
-    fn to_commit(self, repo: &RepositoryRef) -> Result<git::Commit, Error> {
-        repo.commit(self)
-    }
-}
-
 /// Bundled response to retrieve both branches and tags for
 /// a user's repo.
 #[derive(Clone, Debug, PartialEq, Eq)]

radicle-surf/t/src/git.rs

@@ -8,7 +8,7 @@ use radicle_surf::git::{Author, Commit};
 use radicle_surf::{
     diff::*,
     file_system::{unsound, DirectoryEntry, Path},
-    git::{error::Error, Branch, Glob, Namespace, Oid, Repository},
+    git::{Branch, Error, Glob, Namespace, Oid, Repository},
 };
 
 const GIT_PLATINUM: &str = "../data/git-platinum";
@@ -813,8 +813,8 @@ mod code_browsing {
         let path = unsound::path::new("README.md");
         let mut file_history = history.by_path(path);
         let commit = file_history.next().unwrap().unwrap();
-        let file = commit
-            .get_file(&repo, unsound::path::new("README.md"))
+        let file = repo
+            .get_commit_file(&commit.id, unsound::path::new("README.md"))
             .unwrap();
         assert_eq!(file.size(), 67);
     }