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

- Update design doc.
- Refactor History API.
- Add more test cases.

Signed-off-by: Han Xu <[email protected]>

Author: keepsimple1

radicle-surf/docs/refactor-design.md

@@ -98,7 +98,9 @@ I think we should use `Rev` in our API as much as possible.
 
 #### How to identify History
 
-TBD
+Currently we are keeping the basic `History` type, and only change it from
+a TupleStruct to a regular Struct. The goal is to make it easier to refactor
+later.
 
 ### Code browsing
 
@@ -143,17 +145,37 @@ 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.
+
+The main change is to move `get_history` in `Vcs` trait to `history()` in
+`RepositoryRef`.
+
 ```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>;
 }
 ```
+TBD: how to retrieve the history of a particular file? Is it a valid use case?
+
+### 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
 

radicle-surf/src/commit.rs

@@ -241,7 +241,7 @@ where
     };
 
     let stats = repo.get_stats(&rev)?;
-    let headers = repo.history(rev)?.iter().map(Header::from).collect();
+    let headers = repo.history(&rev)?.iter().map(Header::from).collect();
 
     Ok(Commits { headers, stats })
 }

radicle-surf/src/object/tree.rs

@@ -155,7 +155,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.first()))
     } else {
         None
     };

radicle-surf/src/vcs.rs

@@ -21,41 +21,44 @@ use nonempty::NonEmpty;
 
 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.
+/// A non-empty bag of artifacts. Examples of artifacts
+/// would be commits in Git.
 #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)]
-pub struct History<A>(pub NonEmpty<A>);
+pub struct History<A> {
+    items: NonEmpty<A>,
+}
 
 impl<A> History<A> {
     /// Create a new `History` consisting of one artifact.
     pub fn new(a: A) -> Self {
-        History(NonEmpty::new(a))
+        History {
+            items: NonEmpty::new(a),
+        }
     }
 
     /// Push an artifact to the end of the `History`.
     pub fn push(&mut self, a: A) {
-        self.0.push(a)
+        self.items.push(a)
     }
 
     /// Iterator over the artifacts.
     pub fn iter(&self) -> impl Iterator<Item = &A> {
-        self.0.iter()
+        self.items.iter()
     }
 
     /// Get the firest artifact in the `History`.
     pub fn first(&self) -> &A {
-        self.0.first()
+        self.items.first()
     }
 
     /// Get the length of `History` (aka the artefacts count)
     pub fn len(&self) -> usize {
-        self.0.len()
+        self.items.len()
     }
 
     /// Check if `History` is empty
     pub fn is_empty(&self) -> bool {
-        self.0.is_empty()
+        self.items.is_empty()
     }
 
     /// Given that the `History` is topological order from most
@@ -76,15 +79,17 @@ impl<A> History<A> {
                 .collect::<Vec<_>>(),
         );
 
-        new_history.map(History)
+        new_history.map(|x| History { items: x })
     }
 
     /// 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))
+        History {
+            items: self.items.map(f),
+        }
     }
 
     /// Find an artifact in the `History`.

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

@@ -19,7 +19,7 @@ use std::{fmt, str};
 
 use thiserror::Error;
 
-use crate::vcs::git::{repo::RepositoryRef, BranchName, Namespace, TagName};
+use crate::vcs::git::{BranchName, Namespace, TagName};
 use radicle_git_ext::Oid;
 pub(super) mod glob;
 
@@ -93,15 +93,6 @@ impl Ref {
 
         ref_namespace
     }
-
-    /// We try to find a [`git2::Reference`] based off of a `Ref` by turning the
-    /// ref into a fully qualified ref (e.g. refs/remotes/**/master).
-    pub fn find_ref<'a>(
-        &self,
-        repo: &RepositoryRef<'a>,
-    ) -> Result<git2::Reference<'a>, git2::Error> {
-        repo.repo_ref.find_reference(&self.to_string())
-    }
 }
 
 impl fmt::Display for Ref {

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

@@ -19,7 +19,6 @@ use crate::{
     diff::*,
     file_system,
     file_system::{directory, DirectoryContents, Label},
-    vcs,
     vcs::git::{
         error::*,
         reference::{glob::RefGlob, Ref, Rev},
@@ -35,7 +34,6 @@ use crate::{
     },
 };
 use directory::Directory;
-use nonempty::NonEmpty;
 use radicle_git_ext::Oid;
 use std::{
     collections::{BTreeSet, HashSet},
@@ -51,7 +49,7 @@ pub(super) enum CommitHistory {
 }
 
 /// A `History` that uses `git2::Commit` as the underlying artifact.
-pub type History = vcs::History<Commit>;
+pub type History = crate::vcs::History<Commit>;
 
 /// Wrapper around the `git2`'s `git2::Repository` type.
 /// This is to to limit the functionality that we can do
@@ -142,18 +140,6 @@ impl<'a> RepositoryRef<'a> {
         Ok(namespaces?.into_iter().collect())
     }
 
-    pub(super) fn ref_history<R>(&self, reference: R) -> Result<History, Error>
-    where
-        R: Into<Ref>,
-    {
-        let reference = match self.which_namespace()? {
-            None => reference.into(),
-            Some(namespace) => reference.into().namespaced(namespace),
-        }
-        .find_ref(self)?;
-        self.to_history(&reference)
-    }
-
     /// Get the [`Diff`] between two commits.
     pub fn diff(&self, from: &Rev, to: &Rev) -> Result<Diff, Error> {
         let from_commit = self.rev_to_commit(from)?;
@@ -169,6 +155,19 @@ impl<'a> RepositoryRef<'a> {
             .and_then(|diff| Diff::try_from(diff).map_err(Error::from))
     }
 
+    /// Get the diff introduced by a particlar rev.
+    pub fn diff_of_rev(&self, rev: &Rev) -> Result<Diff, Error> {
+        let oid = self.rev_oid(rev)?;
+        let commit = self.get_commit(oid)?;
+        match commit.parents.first() {
+            Some(parent) => {
+                let parent_rev = (*parent).into();
+                self.diff(&parent_rev, rev)
+            },
+            None => self.initial_diff(rev),
+        }
+    }
+
     /// Parse an [`Oid`] from the given string.
     pub fn oid(&self, oid: &str) -> Result<Oid, Error> {
         Ok(self.repo_ref.revparse_single(oid)?.id().into())
@@ -198,7 +197,7 @@ impl<'a> RepositoryRef<'a> {
     /// Gets stats of `rev`.
     pub fn get_stats(&self, rev: &Rev) -> Result<Stats, Error> {
         let branches = self.list_branches(RefScope::Local)?.len();
-        let history = self.history(rev.clone())?;
+        let history = self.history(rev)?;
         let commits = history.len();
 
         let contributors = history
@@ -256,10 +255,19 @@ impl<'a> RepositoryRef<'a> {
     pub(super) fn rev_to_commit(&self, rev: &Rev) -> Result<git2::Commit, Error> {
         match rev {
             Rev::Oid(oid) => Ok(self.repo_ref.find_commit((*oid).into())?),
-            Rev::Ref(reference) => Ok(reference.find_ref(self)?.peel_to_commit()?),
+            Rev::Ref(reference) => Ok(self.find_git2_ref(reference)?.peel_to_commit()?),
         }
     }
 
+    fn find_git2_ref(&self, reference: &Ref) -> Result<git2::Reference<'_>, Error> {
+        let reference = match self.which_namespace()? {
+            None => reference.clone(),
+            Some(namespace) => reference.clone().namespaced(namespace),
+        };
+        let git2_ref = self.repo_ref.find_reference(&reference.to_string())?;
+        Ok(git2_ref)
+    }
+
     /// Switch to a `namespace`
     pub fn switch_namespace(&self, namespace: &str) -> Result<(), Error> {
         Ok(self.repo_ref.set_namespace(namespace)?)
@@ -271,18 +279,11 @@ impl<'a> RepositoryRef<'a> {
         Ok(commit)
     }
 
-    /// Turn a [`git2::Reference`] into a [`History`] by completing
-    /// a revwalk over the first commit in the reference.
-    pub(super) fn to_history(&self, history: &git2::Reference<'a>) -> Result<History, Error> {
-        let head = history.peel_to_commit()?;
-        self.commit_to_history(head)
-    }
-
     /// Turn a [`git2::Reference`] into a [`History`] by completing
     /// a revwalk over the first commit in the reference.
     pub(super) fn commit_to_history(&self, head: git2::Commit) -> Result<History, Error> {
         let head_id = head.id();
-        let mut commits = NonEmpty::new(Commit::try_from(head)?);
+        let mut commits = History::new(Commit::try_from(head)?);
         let mut revwalk = self.repo_ref.revwalk()?;
 
         // Set the revwalk to the head commit
@@ -302,7 +303,7 @@ impl<'a> RepositoryRef<'a> {
             commits.push(commit);
         }
 
-        Ok(vcs::History(commits))
+        Ok(commits)
     }
 
     /// Extract the signature from a commit
@@ -363,7 +364,7 @@ impl<'a> RepositoryRef<'a> {
         Ok(other == git2_oid || is_descendant)
     }
 
-    /// Get the history of the file system where the head of the [`NonEmpty`] is
+    /// Get the history of the file system where the head of the `commit` is
     /// the latest commit.
     pub(super) fn file_history(
         &self,
@@ -531,14 +532,9 @@ impl<'a> RepositoryRef<'a> {
     }
 
     /// Returns the history of `rev`.
-    pub fn history(&self, rev: Rev) -> Result<History, Error> {
-        match rev {
-            Rev::Ref(reference) => self.ref_history(reference),
-            Rev::Oid(oid) => {
-                let commit = self.get_git2_commit(oid)?;
-                self.commit_to_history(commit)
-            },
-        }
+    pub fn history(&self, rev: &Rev) -> Result<History, Error> {
+        let commit = self.rev_to_commit(rev)?;
+        self.commit_to_history(commit)
     }
 }
 
@@ -562,7 +558,7 @@ impl Repository {
 
     /// Since our operations are read-only when it comes to surfing a repository
     /// we have a separate struct called [`RepositoryRef`]. This turns an owned
-    /// [`Repository`], the one returend by [`Repository::new`], into a
+    /// [`Repository`], the one returned by [`Repository::new`], into a
     /// [`RepositoryRef`].
     pub fn as_ref(&'_ self) -> RepositoryRef<'_> {
         RepositoryRef { repo_ref: &self.0 }