diff --git a/radicle-surf/docs/refactor-design.md b/radicle-surf/docs/refactor-design.md index 583b0da..643f50e 100644 --- a/radicle-surf/docs/refactor-design.md +++ b/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 diff --git a/radicle-surf/src/commit.rs b/radicle-surf/src/commit.rs index 55bc400..b28aa46 100644 --- a/radicle-surf/src/commit.rs +++ b/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 }) } diff --git a/radicle-surf/src/object/tree.rs b/radicle-surf/src/object/tree.rs index 1e60f44..8c9e60e 100644 --- a/radicle-surf/src/object/tree.rs +++ b/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 }; diff --git a/radicle-surf/src/vcs.rs b/radicle-surf/src/vcs.rs index 3b3d6e6..e4af0bb 100644 --- a/radicle-surf/src/vcs.rs +++ b/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 . -//! 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 History { .collect() } } - -pub(crate) trait GetVcs -where - Self: Sized, -{ - /// The way to identify a Repository. - type RepoId; - - /// Find a Repository - fn get_repo(identifier: Self::RepoId) -> Result; -} - -/// 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 { - /// 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, Error>; - - /// Find all histories in a Repo - fn get_histories(&self) -> Result>, Error>; - - /// Identify artefacts of a Repository - fn get_identifier(artefact: &A) -> Self::ArtefactId; -} diff --git a/radicle-surf/src/vcs/git/repo.rs b/radicle-surf/src/vcs/git/repo.rs index 51240b9..228f63f 100644 --- a/radicle-surf/src/vcs/git/repo.rs +++ b/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(&self, reference: R, check: P) -> Result + pub(super) fn ref_history(&self, reference: R) -> Result where R: Into, - P: FnOnce(&git2::Reference) -> Option, { 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 { 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 { - 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 { @@ -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 for RepositoryRef<'a> { - type HistoryId = Rev; - type ArtefactId = Oid; - - fn get_history(&self, history_id: Self::HistoryId) -> Result { - match history_id { - Rev::Ref(reference) => self.reference(reference, |_| None), + /// Returns the history of `rev`. + pub fn history(&self, rev: Rev) -> Result { + 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, 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 for Repository { - type RepoId = String; - - fn get_repo(repo_id: Self::RepoId) -> Result { - git2::Repository::open(&repo_id) - .map(Repository) - .map_err(Error::from) - } -} - impl From for Repository { fn from(repo: git2::Repository) -> Self { Repository(repo) diff --git a/radicle-surf/t/src/git.rs b/radicle-surf/t/src/git.rs index da02d69..268e2ed 100644 --- a/radicle-surf/t/src/git.rs +++ b/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 = 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 = 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 = 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> {