radicle-surf: refactor the design: part 3

- Detect renames in Diff by default.
- Remove Vcs trait.
- Move `history()` to be a regular method of RepositoryRef.
- Update design doc.

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

radicle-surf/docs/refactor-design.md

@@ -68,6 +68,13 @@ use `DirectoryContents` for its items and not to use `Tree` or `Forest` at all.
 We also found the `list_directory()` method duplicates with `iter()` method.
 Hence `list_directory()` is removed, together with `SystemType` type.
 
+## Remove `Vcs` trait
+
+The `Vcs` trait was introduced to support different version control backends,
+for example both Git and Pijul, and potentially others. However, since this
+port is part of `radicle-git` repo, we are only supporting Git going forward.
+We no longer need another layer of indirection defined by `Vcs` trait.
+
 ## The new API
 
 With the changes proposed in the previous section, we describe what the new API

radicle-surf/src/commit.rs

@@ -30,10 +30,7 @@ use crate::{
     file_system,
     person::Person,
     revision::Revision,
-    vcs::{
-        git::{self, BranchName, RepositoryRef, Rev},
-        Vcs,
-    },
+    vcs::git::{self, BranchName, RepositoryRef, Rev},
 };
 
 use radicle_git_ext::Oid;
@@ -244,7 +241,7 @@ where
     };
 
     let stats = repo.get_stats(&rev)?;
-    let headers = repo.get_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

@@ -32,10 +32,7 @@ use crate::{
     git::RepositoryRef,
     object::{Error, Info, ObjectType},
     revision::Revision,
-    vcs::{
-        git::{Branch, Rev},
-        Vcs,
-    },
+    vcs::git::{Branch, Rev},
 };
 
 /// Result of a directory listing, carries other trees and blobs.
@@ -158,7 +155,7 @@ 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.get_history(rev).unwrap().first()))
+        Some(commit::Header::from(repo.history(rev).unwrap().first()))
     } else {
         None
     };

radicle-surf/src/vcs.rs

@@ -15,10 +15,8 @@
 // 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. The components consist of a [`History`]
-//! and a [`Vcs`] trait.
+//! A model of a general VCS History.
 
-// use crate::file_system::directory::Directory;
 use nonempty::NonEmpty;
 
 pub mod git;
@@ -134,34 +132,3 @@ impl<A> History<A> {
             .collect()
     }
 }
-
-pub(crate) trait GetVcs<Error>
-where
-    Self: Sized,
-{
-    /// The way to identify a Repository.
-    type RepoId;
-
-    /// Find a Repository
-    fn get_repo(identifier: Self::RepoId) -> Result<Self, Error>;
-}
-
-/// The `VCS` trait encapsulates the minimal amount of information for
-/// interacting with some notion of `History` from a given
-/// Version-Control-System.
-pub trait Vcs<A, Error> {
-    /// The way to identify a History.
-    type HistoryId;
-
-    /// The way to identify an artefact.
-    type ArtefactId;
-
-    /// Find a History in a Repo given a way to identify it
-    fn get_history(&self, identifier: Self::HistoryId) -> Result<History<A>, Error>;
-
-    /// Find all histories in a Repo
-    fn get_histories(&self) -> Result<Vec<History<A>>, Error>;
-
-    /// Identify artefacts of a Repository
-    fn get_identifier(artefact: &A) -> Self::ArtefactId;
-}

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

@@ -20,21 +20,18 @@ use crate::{
     file_system,
     file_system::{directory, DirectoryContents, Label},
     vcs,
-    vcs::{
-        git::{
-            error::*,
-            reference::{glob::RefGlob, Ref, Rev},
-            Branch,
-            BranchName,
-            Commit,
-            Namespace,
-            RefScope,
-            Signature,
-            Stats,
-            Tag,
-            TagName,
-        },
-        Vcs,
+    vcs::git::{
+        error::*,
+        reference::{glob::RefGlob, Ref, Rev},
+        Branch,
+        BranchName,
+        Commit,
+        Namespace,
+        RefScope,
+        Signature,
+        Stats,
+        Tag,
+        TagName,
     },
 };
 use directory::Directory;
@@ -145,21 +142,15 @@ impl<'a> RepositoryRef<'a> {
         Ok(namespaces?.into_iter().collect())
     }
 
-    pub(super) fn reference<R, P>(&self, reference: R, check: P) -> Result<History, Error>
+    pub(super) fn ref_history<R>(&self, reference: R) -> Result<History, Error>
     where
         R: Into<Ref>,
-        P: FnOnce(&git2::Reference) -> Option<Error>,
     {
         let reference = match self.which_namespace()? {
             None => reference.into(),
             Some(namespace) => reference.into().namespaced(namespace),
         }
         .find_ref(self)?;
-
-        if let Some(err) = check(&reference) {
-            return Err(err);
-        }
-
         self.to_history(&reference)
     }
 
@@ -207,7 +198,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.get_history(rev.clone())?;
+        let history = self.history(rev.clone())?;
         let commits = history.len();
 
         let contributors = history
@@ -280,12 +271,6 @@ impl<'a> RepositoryRef<'a> {
         Ok(commit)
     }
 
-    /// Build a [`History`] using the `head` reference.
-    pub fn head_history(&self) -> Result<History, Error> {
-        let head = self.repo_ref.head()?;
-        self.to_history(&head)
-    }
-
     /// 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> {
@@ -439,10 +424,15 @@ impl<'a> RepositoryRef<'a> {
             opts.skip_binary_check(true);
         }
 
-        let diff =
+        let mut diff =
             self.repo_ref
                 .diff_tree_to_tree(old_tree.as_ref(), Some(&new_tree), Some(&mut opts))?;
 
+        // Detect renames by default.
+        let mut find_opts = git2::DiffFindOptions::new();
+        find_opts.renames(true);
+        diff.find_similar(Some(&mut find_opts))?;
+
         Ok(diff)
     }
 
@@ -539,40 +529,17 @@ impl<'a> RepositoryRef<'a> {
             rev: commit.id().to_string(),
         })
     }
-}
 
-impl<'a> Vcs<Commit, Error> for RepositoryRef<'a> {
-    type HistoryId = Rev;
-    type ArtefactId = Oid;
-
-    fn get_history(&self, history_id: Self::HistoryId) -> Result<History, Error> {
-        match history_id {
-            Rev::Ref(reference) => self.reference(reference, |_| None),
+    /// 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)
             },
         }
     }
-
-    fn get_histories(&self) -> Result<Vec<History>, Error> {
-        self.repo_ref
-            .references()
-            .map_err(Error::from)
-            .and_then(|mut references| {
-                references.try_fold(vec![], |mut acc, reference| {
-                    reference.map_err(Error::from).and_then(|r| {
-                        let history = self.to_history(&r)?;
-                        acc.push(history);
-                        Ok(acc)
-                    })
-                })
-            })
-    }
-
-    fn get_identifier(artifact: &Commit) -> Self::ArtefactId {
-        artifact.id
-    }
 }
 
 impl<'a> std::fmt::Debug for RepositoryRef<'a> {
@@ -608,16 +575,6 @@ impl<'a> From<&'a Repository> for RepositoryRef<'a> {
     }
 }
 
-impl vcs::GetVcs<Error> for Repository {
-    type RepoId = String;
-
-    fn get_repo(repo_id: Self::RepoId) -> Result<Self, Error> {
-        git2::Repository::open(&repo_id)
-            .map(Repository)
-            .map_err(Error::from)
-    }
-}
-
 impl From<git2::Repository> for Repository {
     fn from(repo: git2::Repository) -> Self {
         Repository(repo)

radicle-surf/t/src/git.rs

@@ -27,15 +27,14 @@ fn test_submodule_failure() {
 mod namespace {
     use super::*;
     use pretty_assertions::{assert_eq, assert_ne};
-    use radicle_surf::vcs::Vcs;
 
     #[test]
     fn switch_to_banana() -> Result<(), Error> {
         let repo = Repository::new(GIT_PLATINUM)?;
         let repo = repo.as_ref();
-        let history_master = repo.get_history(Branch::local("master").into())?;
+        let history_master = repo.history(Branch::local("master").into())?;
         repo.switch_namespace("golden")?;
-        let history_banana = repo.get_history(Branch::local("banana").into())?;
+        let history_banana = repo.history(Branch::local("banana").into())?;
 
         assert_ne!(history_master, history_banana);
 
@@ -46,14 +45,14 @@ mod namespace {
     fn me_namespace() -> Result<(), Error> {
         let repo = Repository::new(GIT_PLATINUM)?;
         let repo = repo.as_ref();
-        let history = repo.get_history(Branch::local("master").into())?;
+        let history = repo.history(Branch::local("master").into())?;
 
         assert_eq!(repo.which_namespace(), Ok(None));
 
         repo.switch_namespace("me")?;
         assert_eq!(repo.which_namespace(), Ok(Some(Namespace::try_from("me")?)));
 
-        let history_feature = repo.get_history(Branch::local("feature/#1194").into())?;
+        let history_feature = repo.history(Branch::local("feature/#1194").into())?;
         assert_eq!(history, history_feature);
 
         let expected_branches: Vec<Branch> = vec![Branch::local("feature/#1194")];
@@ -77,7 +76,7 @@ mod namespace {
     fn golden_namespace() -> Result<(), Error> {
         let repo = Repository::new(GIT_PLATINUM)?;
         let repo = repo.as_ref();
-        let history = repo.get_history(Branch::local("master").into())?;
+        let history = repo.history(Branch::local("master").into())?;
 
         assert_eq!(repo.which_namespace(), Ok(None));
 
@@ -88,7 +87,7 @@ mod namespace {
             Ok(Some(Namespace::try_from("golden")?))
         );
 
-        let golden_history = repo.get_history(Branch::local("master").into())?;
+        let golden_history = repo.history(Branch::local("master").into())?;
         assert_eq!(history, golden_history);
 
         let expected_branches: Vec<Branch> = vec![Branch::local("banana"), Branch::local("master")];
@@ -116,7 +115,7 @@ mod namespace {
     fn silver_namespace() -> Result<(), Error> {
         let repo = Repository::new(GIT_PLATINUM)?;
         let repo = repo.as_ref();
-        let history = repo.get_history(Branch::local("master").into())?;
+        let history = repo.history(Branch::local("master").into())?;
 
         assert_eq!(repo.which_namespace(), Ok(None));
 
@@ -125,7 +124,7 @@ mod namespace {
             repo.which_namespace(),
             Ok(Some(Namespace::try_from("golden/silver")?))
         );
-        let silver_history = repo.get_history(Branch::local("master").into())?;
+        let silver_history = repo.history(Branch::local("master").into())?;
         assert_ne!(history, silver_history);
 
         let expected_branches: Vec<Branch> = vec![Branch::local("master")];
@@ -140,8 +139,6 @@ mod namespace {
 
 #[cfg(test)]
 mod rev {
-    use radicle_surf::vcs::Vcs;
-
     use super::*;
     use std::str::FromStr;
 
@@ -160,7 +157,7 @@ mod rev {
     fn _master() -> Result<(), Error> {
         let repo = Repository::new(GIT_PLATINUM)?;
         let repo = repo.as_ref();
-        let history = repo.get_history(Branch::remote("master", "origin").into())?;
+        let history = repo.history(Branch::remote("master", "origin").into())?;
 
         let commit1 = Oid::from_str("3873745c8f6ffb45c990eb23b491d4b4b6182f95")?;
         assert!(
@@ -197,7 +194,7 @@ mod rev {
     fn commit() -> Result<(), Error> {
         let repo = Repository::new(GIT_PLATINUM)?;
         let rev: Rev = Oid::from_str("3873745c8f6ffb45c990eb23b491d4b4b6182f95")?.into();
-        let history = repo.as_ref().get_history(rev)?;
+        let history = repo.as_ref().history(rev)?;
 
         let commit1 = Oid::from_str("3873745c8f6ffb45c990eb23b491d4b4b6182f95")?;
         assert!(history
@@ -215,7 +212,7 @@ mod rev {
     fn commit_parents() -> Result<(), Error> {
         let repo = Repository::new(GIT_PLATINUM)?;
         let rev: Rev = Oid::from_str("3873745c8f6ffb45c990eb23b491d4b4b6182f95")?.into();
-        let history = repo.as_ref().get_history(rev)?;
+        let history = repo.as_ref().history(rev)?;
         let commit = history.first();
 
         assert_eq!(
@@ -230,7 +227,7 @@ mod rev {
     fn commit_short() -> Result<(), Error> {
         let repo = Repository::new(GIT_PLATINUM)?;
         let rev: Rev = repo.as_ref().oid("3873745c8")?.into();
-        let history = repo.as_ref().get_history(rev)?;
+        let history = repo.as_ref().history(rev)?;
 
         let commit1 = Oid::from_str("3873745c8f6ffb45c990eb23b491d4b4b6182f95")?;
         assert!(history
@@ -248,7 +245,7 @@ mod rev {
     fn tag() -> Result<(), Error> {
         let repo = Repository::new(GIT_PLATINUM)?;
         let rev: Rev = TagName::new("v0.2.0").into();
-        let history = repo.as_ref().get_history(rev)?;
+        let history = repo.as_ref().history(rev)?;
 
         let commit1 = Oid::from_str("2429f097664f9af0c5b7b389ab998b2199ffa977")?;
         assert_eq!(history.first().id, commit1);
@@ -259,8 +256,6 @@ mod rev {
 
 #[cfg(test)]
 mod last_commit {
-    use radicle_surf::vcs::Vcs;
-
     use super::*;
     use std::str::FromStr;
 
@@ -378,7 +373,7 @@ mod last_commit {
 
         let expected_oid = repo
             .as_ref()
-            .get_history(Branch::local("master").into())
+            .history(Branch::local("master").into())
             .unwrap()
             .first()
             .id;
@@ -464,6 +459,42 @@ mod diff {
         Ok(())
     }
 
+    #[test]
+    fn test_branch_diff() -> Result<(), Error> {
+        let repo = Repository::new(GIT_PLATINUM)?;
+        let repo = repo.as_ref();
+        let diff = repo.diff(
+            &Branch::local("master").into(),
+            &Branch::local("dev").into(),
+        )?;
+
+        println!("Diff two branches: master -> dev");
+        println!(
+            "result: created {} deleted {} moved {} modified {}",
+            diff.created.len(),
+            diff.deleted.len(),
+            diff.moved.len(),
+            diff.modified.len()
+        );
+        assert_eq!(diff.created.len(), 1);
+        assert_eq!(diff.deleted.len(), 11);
+        assert_eq!(diff.moved.len(), 1);
+        assert_eq!(diff.modified.len(), 2);
+        for c in diff.created.iter() {
+            println!("created: {}", &c.path);
+        }
+        for d in diff.deleted.iter() {
+            println!("deleted: {}", &d.path);
+        }
+        for m in diff.moved.iter() {
+            println!("moved: {} -> {}", &m.old_path, &m.new_path);
+        }
+        for m in diff.modified.iter() {
+            println!("modified: {}", &m.path);
+        }
+        Ok(())
+    }
+
     #[cfg(feature = "serialize")]
     #[test]
     fn test_diff_serde() -> Result<(), Error> {