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,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/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>
@@ -241,7 +247,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/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.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/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 {