radicle-surf: refactor design: part 4 (WIP)

- Update design doc.
- Refactor History API.
- Use FromRevision and IntoCommit traits.
- Add more test cases.

Signed-off-by: Han Xu <keepsimple@gmail.com>

Author: keepsimple1

radicle-surf/docs/refactor-design.md

@@ -98,7 +98,7 @@ I think we should use `Rev` in our API as much as possible.
 
 #### How to identify History
 
-TBD
+Please see section [Retrieve the history](#retrieve-the-history) below.
 
 ### Code browsing
 
@@ -143,18 +143,80 @@ The core API is:
 
 ```Rust
 imp RepositoryRef {
+    /// Returns the diff between two revisions.
     pub fn diff(&self, from: &Rev, to: &Rev) -> Result<Diff, Error>;
+
+    /// Returns the diff between a revision and the initial state of the repo.
+    pub fn initial_diff(&self, rev: &Rev) -> Result<Diff, Error> {
+
+    /// Returns the diff of a specific revision.
+    pub fn diff_of_rev(&self, rev: &Rev) -> Result<Diff, Error>;
 }
 ```
 
-To help convert from `Oid` to `Rev`, we provide a helper method:
+### Retrieve the history
+
+The user would be able to get the list of previous commits reachable from a
+particular commit or a ref.
+
 ```Rust
 imp RepositoryRef {
-    /// Returns the Oid of `rev`.
-    pub fn rev_oid(&self, rev: &Rev) -> Result<Oid, Error>;
+    pub fn history(&self, rev: &Rev) -> Result<History, Error>;
 }
 ```
 
+`History` is defined as an iterator to support potentially very long histories.
+
+```Rust
+pub struct History<'a> {
+    repo: RepositoryRef<'a>,
+    head: Commit,
+    revwalk: git2::Revwalk<'a>,
+}
+
+impl<'a> History<'a> {
+    /// `head` is always valid. A history is guaranteed not emnpty.
+    pub fn head(&self) -> &Commit;
+
+    /// Returns an iterator that only iterates commits that has the file `path`.
+    pub fn has_file(self, path: file_system::Path) -> impl Iterator<Item = Commit> + 'a;
+
+    /// Reloads the history so that a caller can iterate it again.
+    pub fn reload(&mut self);
+}
+```
+Note: the only way to create a new `History` to call `repo.history(rev)`.
+
+- Alternative design:
+
+One potential downside of define `History` as an iterator is that:
+`history.next()` takes a mutable history object. A different design is to use
+`History` as immutable object that produces an iterator on-demand:
+
+```Rust
+pub struct History<'a> {
+    repo: RepositoryRef<'a>,
+    head: Commit,
+}
+
+impl<'a> History<'a> {
+    /// This method creats a new `RevWalk` internally and return an
+    /// iterator for all commits in a history.
+    pub fn iter(&self) -> impl Iterator<Item = Commit>;
+}
+```
+
+In this design, `History` does not keep `RevWalk` in its state. It will create
+a new one when `iter()` is called. I like the immutable interface of this design
+but did not implement it in the current code mainly because the libgit2 doc says
+[creating a new `RevWalk` is relatively expensive](https://libgit2.org/libgit2/#HEAD/group/revwalk/git_revwalk_new).
+
+### Retrieve a specific object: all its metadata
+
+Git has four different types of objects: Blob, Tree, Commit and Tag. The `blob`
+and `tree` are low-level to git and so I don't think we should expose them in
+our API.
+
 ## Error handling
 
 TBD

radicle-surf/examples/diff.rs

@@ -32,10 +32,7 @@ fn main() {
     let base_oid = Oid::from_str(&options.base_revision).unwrap();
     let now = Instant::now();
     let elapsed_nanos = now.elapsed().as_nanos();
-    let diff = repo
-        .as_ref()
-        .diff(&base_oid.into(), &head_oid.into())
-        .unwrap();
+    let diff = repo.as_ref().diff(base_oid, head_oid).unwrap();
     print_diff_summary(&diff, elapsed_nanos);
 }
 

radicle-surf/src/commit.rs

@@ -109,6 +109,12 @@ impl From<&git::Commit> for Header {
     }
 }
 
+impl From<git::Commit> for Header {
+    fn from(commit: git::Commit) -> Self {
+        Self::from(&commit)
+    }
+}
+
 #[cfg(feature = "serialize")]
 impl Serialize for Header {
     fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
@@ -143,13 +149,8 @@ pub struct Commits {
 /// fails.
 pub fn commit(repo: &RepositoryRef, rev: &Rev) -> Result<Commit, Error> {
     let sha1 = repo.rev_oid(rev)?;
-    let commit = repo.get_commit(sha1)?;
-    let diff = if let Some(parent) = commit.parents.first() {
-        let parent_rev = (*parent).into();
-        repo.diff(&parent_rev, rev)?
-    } else {
-        repo.initial_diff(rev)?
-    };
+    let commit = repo.commit(sha1)?;
+    let diff = repo.diff_of_commit(&commit)?;
 
     let mut deletions = 0;
     let mut additions = 0;
@@ -216,7 +217,7 @@ pub fn commit(repo: &RepositoryRef, rev: &Rev) -> Result<Commit, Error> {
 /// Will return [`Error`] if the project doesn't exist or the surf interaction
 /// fails.
 pub fn header(repo: &RepositoryRef, sha1: Oid) -> Result<Header, Error> {
-    let commit = repo.get_commit(sha1)?;
+    let commit = repo.commit(sha1)?;
     Ok(Header::from(&commit))
 }
 
@@ -241,7 +242,7 @@ where
     };
 
     let stats = repo.get_stats(&rev)?;
-    let headers = repo.history(rev)?.iter().map(Header::from).collect();
+    let headers = repo.history(&rev)?.map(Header::from).collect();
 
     Ok(Commits { headers, stats })
 }

radicle-surf/src/object/blob.rs

@@ -141,8 +141,8 @@ where
 {
     let maybe_revision = maybe_revision.map(Rev::try_from).transpose()?;
     let revision = maybe_revision.unwrap();
-
-    let root = repo.snapshot(&revision)?;
+    let oid = repo.rev_oid(&revision)?;
+    let root = repo.snapshot(oid)?;
     let p = file_system::Path::from_str(path)?;
 
     let file = root

radicle-surf/src/object/tree.rs

@@ -108,7 +108,8 @@ where
         file_system::Path::from_str(&prefix)?
     };
 
-    let root_dir = repo.snapshot(&rev)?;
+    let oid = repo.rev_oid(&rev)?;
+    let root_dir = repo.snapshot(oid)?;
     let prefix_dir = if path.is_root() {
         &root_dir
     } else {
@@ -155,7 +156,8 @@ where
     entries.sort_by(|a, b| a.info.object_type.cmp(&b.info.object_type));
 
     let last_commit = if path.is_root() {
-        Some(commit::Header::from(repo.history(rev).unwrap().first()))
+        let history = repo.history(&rev)?;
+        Some(commit::Header::from(history.head()))
     } else {
         None
     };

radicle-surf/src/vcs.rs

@@ -15,120 +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/>.
 
-//! A model of a general VCS History.
-
-use nonempty::NonEmpty;
+//! A model of a general VCS.
 
 pub mod git;
-
-/// A non-empty bag of artifacts which are used to
-/// derive a [`crate::file_system::Directory`] view. Examples of artifacts
-/// would be commits in Git or patches in Pijul.
-#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)]
-pub struct History<A>(pub NonEmpty<A>);
-
-impl<A> History<A> {
-    /// Create a new `History` consisting of one artifact.
-    pub fn new(a: A) -> Self {
-        History(NonEmpty::new(a))
-    }
-
-    /// Push an artifact to the end of the `History`.
-    pub fn push(&mut self, a: A) {
-        self.0.push(a)
-    }
-
-    /// Iterator over the artifacts.
-    pub fn iter(&self) -> impl Iterator<Item = &A> {
-        self.0.iter()
-    }
-
-    /// Get the firest artifact in the `History`.
-    pub fn first(&self) -> &A {
-        self.0.first()
-    }
-
-    /// Get the length of `History` (aka the artefacts count)
-    pub fn len(&self) -> usize {
-        self.0.len()
-    }
-
-    /// Check if `History` is empty
-    pub fn is_empty(&self) -> bool {
-        self.0.is_empty()
-    }
-
-    /// Given that the `History` is topological order from most
-    /// recent artifact to least recent, `find_suffix` gets returns
-    /// the history up until the point of the given artifact.
-    ///
-    /// This operation may fail if the artifact does not exist in
-    /// the given `History`.
-    pub fn find_suffix(&self, artifact: &A) -> Option<Self>
-    where
-        A: Clone + PartialEq,
-    {
-        let new_history: Option<NonEmpty<A>> = NonEmpty::from_slice(
-            &self
-                .iter()
-                .cloned()
-                .skip_while(|current| *current != *artifact)
-                .collect::<Vec<_>>(),
-        );
-
-        new_history.map(History)
-    }
-
-    /// Apply a function from `A` to `B` over the `History`
-    pub fn map<F, B>(self, f: F) -> History<B>
-    where
-        F: FnMut(A) -> B,
-    {
-        History(self.0.map(f))
-    }
-
-    /// Find an artifact in the `History`.
-    ///
-    /// The function provided should return `Some` if the item is the desired
-    /// output and `None` otherwise.
-    pub fn find<F, B>(&self, f: F) -> Option<B>
-    where
-        F: Fn(&A) -> Option<B>,
-    {
-        self.iter().find_map(f)
-    }
-
-    /// Find an atrifact in the given `History` using the artifacts ID.
-    ///
-    /// This operation may fail if the artifact does not exist in the history.
-    pub fn find_in_history<Identifier, F>(&self, identifier: &Identifier, id_of: F) -> Option<A>
-    where
-        A: Clone,
-        F: Fn(&A) -> Identifier,
-        Identifier: PartialEq,
-    {
-        self.iter()
-            .find(|artifact| {
-                let current_id = id_of(artifact);
-                *identifier == current_id
-            })
-            .cloned()
-    }
-
-    /// Find all occurences of an artifact in a bag of `History`s.
-    pub fn find_in_histories<Identifier, F>(
-        histories: Vec<Self>,
-        identifier: &Identifier,
-        id_of: F,
-    ) -> Vec<Self>
-    where
-        A: Clone,
-        F: Fn(&A) -> Identifier + Copy,
-        Identifier: PartialEq,
-    {
-        histories
-            .into_iter()
-            .filter(|history| history.find_in_history(identifier, id_of).is_some())
-            .collect()
-    }
-}

radicle-surf/src/vcs/git.rs

@@ -63,6 +63,8 @@
 //! # }
 //! ```
 
+use std::str::FromStr;
+
 // Re-export git2 as sub-module
 pub use git2::{self, Error as Git2Error, Time};
 pub use radicle_git_ext::Oid;
@@ -75,6 +77,7 @@ mod repo;
 pub use repo::{History, Repository, RepositoryRef};
 
 pub mod error;
+pub use error::Error;
 
 pub mod ext;
 
@@ -138,3 +141,35 @@ where
         })
     }
 }
+
+/// Supports various ways to specify a revision used in Git.
+pub trait FromRevision {
+    /// Resolves a revision into an object id in `repo`.
+    fn object_id(&self, repo: &RepositoryRef) -> Result<Oid, Error>;
+}
+
+impl FromRevision for Oid {
+    fn object_id(&self, _repo: &RepositoryRef) -> Result<Oid, Error> {
+        Ok(*self)
+    }
+}
+
+impl FromRevision for &str {
+    fn object_id(&self, _repo: &RepositoryRef) -> Result<Oid, Error> {
+        Oid::from_str(*self).map_err(Error::Git)
+    }
+}
+
+impl FromRevision for &Branch {
+    fn object_id(&self, repo: &RepositoryRef) -> Result<Oid, Error> {
+        let git2_oid = repo.repo_ref.refname_to_id(&self.refname())?;
+        Ok(git2_oid.into())
+    }
+}
+
+impl FromRevision for &Tag {
+    fn object_id(&self, repo: &RepositoryRef) -> Result<Oid, Error> {
+        let git2_oid = repo.repo_ref.refname_to_id(&self.refname())?;
+        Ok(git2_oid.into())
+    }
+}

radicle-surf/src/vcs/git/branch.rs

@@ -16,10 +16,9 @@
 // along with this program. If not, see <https://www.gnu.org/licenses/>.
 
 use crate::vcs::git::{self, error::Error, ext, reference::Ref};
-use std::{cmp::Ordering, convert::TryFrom, fmt, str};
-
 #[cfg(feature = "serialize")]
 use serde::{Deserialize, Serialize};
+use std::{cmp::Ordering, convert::TryFrom, fmt, str};
 
 /// The branch type we want to filter on.
 #[cfg_attr(feature = "serialize", derive(Serialize, Deserialize))]
@@ -147,13 +146,13 @@ impl Branch {
     }
 
     /// Get the name of the `Branch`.
-    pub fn name(&self) -> String {
+    pub fn refname(&self) -> String {
         let branch_name = self.name.0.clone();
         match self.locality {
-            BranchType::Local => branch_name,
+            BranchType::Local => format!("refs/heads/{}", branch_name),
             BranchType::Remote { ref name } => match name {
                 None => branch_name,
-                Some(remote_name) => format!("{}/{}", remote_name, branch_name),
+                Some(remote_name) => format!("refs/remotes/{}/{}", remote_name, branch_name),
             },
         }
     }