From 112b754f2660b2a6bea56c028d268e2eb3095987 Mon Sep 17 00:00:00 2001 From: Han Xu Date: Thu, 20 Oct 2022 10:52:49 -0700 Subject: [PATCH] radicle-surf: refactor design: part 4 (WIP) - Update design doc. - Refactor History API. - Add more test cases. Signed-off-by: Han Xu --- radicle-surf/docs/refactor-design.md | 30 ++++++++++-- radicle-surf/src/commit.rs | 2 +- radicle-surf/src/object/tree.rs | 3 +- radicle-surf/src/vcs.rs | 29 +++++++----- radicle-surf/src/vcs/git/reference.rs | 11 +---- radicle-surf/src/vcs/git/repo.rs | 66 +++++++++++++-------------- radicle-surf/t/src/git.rs | 53 +++++++++++++++------ 7 files changed, 116 insertions(+), 78 deletions(-) diff --git a/radicle-surf/docs/refactor-design.md b/radicle-surf/docs/refactor-design.md index 643f50e..37f97dd 100644 --- a/radicle-surf/docs/refactor-design.md +++ b/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; + + /// Returns the diff between a revision and the initial state of the repo. + pub fn initial_diff(&self, rev: &Rev) -> Result { + + /// Returns the diff of a specific revision. + pub fn diff_of_rev(&self, rev: &Rev) -> Result; } ``` -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; + pub fn history(&self, rev: &Rev) -> Result; } ``` +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 diff --git a/radicle-surf/src/commit.rs b/radicle-surf/src/commit.rs index b28aa46..c6b1f28 100644 --- a/radicle-surf/src/commit.rs +++ b/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 }) } diff --git a/radicle-surf/src/object/tree.rs b/radicle-surf/src/object/tree.rs index 8c9e60e..e17b493 100644 --- a/radicle-surf/src/object/tree.rs +++ b/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 }; diff --git a/radicle-surf/src/vcs.rs b/radicle-surf/src/vcs.rs index e4af0bb..3acedc4 100644 --- a/radicle-surf/src/vcs.rs +++ b/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(pub NonEmpty); +pub struct History { + items: NonEmpty, +} impl History { /// 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 { - 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,7 +79,7 @@ impl History { .collect::>(), ); - new_history.map(History) + new_history.map(|x| History { items: x }) } /// Apply a function from `A` to `B` over the `History` @@ -84,7 +87,9 @@ impl History { where F: FnMut(A) -> B, { - History(self.0.map(f)) + History { + items: self.items.map(f), + } } /// Find an artifact in the `History`. diff --git a/radicle-surf/src/vcs/git/reference.rs b/radicle-surf/src/vcs/git/reference.rs index 0c1d423..60d1a53 100644 --- a/radicle-surf/src/vcs/git/reference.rs +++ b/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::Error> { - repo.repo_ref.find_reference(&self.to_string()) - } } impl fmt::Display for Ref { diff --git a/radicle-surf/src/vcs/git/repo.rs b/radicle-surf/src/vcs/git/repo.rs index 228f63f..b4ae476 100644 --- a/radicle-surf/src/vcs/git/repo.rs +++ b/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; +pub type History = crate::vcs::History; /// 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(&self, reference: R) -> Result - where - R: Into, - { - 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 { 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 { + 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 { 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 { 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 { 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, 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 { - 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 { 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 @@ -531,14 +532,9 @@ impl<'a> RepositoryRef<'a> { } /// 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) - }, - } + pub fn history(&self, rev: &Rev) -> Result { + 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 } diff --git a/radicle-surf/t/src/git.rs b/radicle-surf/t/src/git.rs index 268e2ed..f8158f5 100644 --- a/radicle-surf/t/src/git.rs +++ b/radicle-surf/t/src/git.rs @@ -32,9 +32,9 @@ mod namespace { fn switch_to_banana() -> Result<(), Error> { let repo = Repository::new(GIT_PLATINUM)?; let repo = repo.as_ref(); - let history_master = repo.history(Branch::local("master").into())?; + let history_master = repo.history(&Branch::local("master").into())?; repo.switch_namespace("golden")?; - let history_banana = repo.history(Branch::local("banana").into())?; + let history_banana = repo.history(&Branch::local("banana").into())?; assert_ne!(history_master, history_banana); @@ -45,14 +45,14 @@ mod namespace { fn me_namespace() -> Result<(), Error> { let repo = Repository::new(GIT_PLATINUM)?; let repo = repo.as_ref(); - let history = repo.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.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")]; @@ -76,7 +76,7 @@ mod namespace { fn golden_namespace() -> Result<(), Error> { let repo = Repository::new(GIT_PLATINUM)?; let repo = repo.as_ref(); - let history = repo.history(Branch::local("master").into())?; + let history = repo.history(&Branch::local("master").into())?; assert_eq!(repo.which_namespace(), Ok(None)); @@ -87,7 +87,7 @@ mod namespace { Ok(Some(Namespace::try_from("golden")?)) ); - let golden_history = repo.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")]; @@ -115,7 +115,7 @@ mod namespace { fn silver_namespace() -> Result<(), Error> { let repo = Repository::new(GIT_PLATINUM)?; let repo = repo.as_ref(); - let history = repo.history(Branch::local("master").into())?; + let history = repo.history(&Branch::local("master").into())?; assert_eq!(repo.which_namespace(), Ok(None)); @@ -124,7 +124,7 @@ mod namespace { repo.which_namespace(), Ok(Some(Namespace::try_from("golden/silver")?)) ); - let silver_history = repo.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")]; @@ -157,7 +157,7 @@ mod rev { fn _master() -> Result<(), Error> { let repo = Repository::new(GIT_PLATINUM)?; let repo = repo.as_ref(); - let history = repo.history(Branch::remote("master", "origin").into())?; + let history = repo.history(&Branch::remote("master", "origin").into())?; let commit1 = Oid::from_str("3873745c8f6ffb45c990eb23b491d4b4b6182f95")?; assert!( @@ -194,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().history(rev)?; + let history = repo.as_ref().history(&rev)?; let commit1 = Oid::from_str("3873745c8f6ffb45c990eb23b491d4b4b6182f95")?; assert!(history @@ -212,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().history(rev)?; + let history = repo.as_ref().history(&rev)?; let commit = history.first(); assert_eq!( @@ -227,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().history(rev)?; + let history = repo.as_ref().history(&rev)?; let commit1 = Oid::from_str("3873745c8f6ffb45c990eb23b491d4b4b6182f95")?; assert!(history @@ -245,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().history(rev)?; + let history = repo.as_ref().history(&rev)?; let commit1 = Oid::from_str("2429f097664f9af0c5b7b389ab998b2199ffa977")?; assert_eq!(history.first().id, commit1); @@ -373,7 +373,7 @@ mod last_commit { let expected_oid = repo .as_ref() - .history(Branch::local("master").into()) + .history(&Branch::local("master").into()) .unwrap() .first() .id; @@ -389,9 +389,9 @@ mod diff { #[test] fn test_initial_diff() -> Result<(), Error> { - let oid = Oid::from_str("d3464e33d75c75c99bfb90fa2e9d16efc0b7d0e3")?; let repo = Repository::new(GIT_PLATINUM)?; let repo = repo.as_ref(); + let oid = Oid::from_str("d3464e33d75c75c99bfb90fa2e9d16efc0b7d0e3")?; let commit = repo.get_git2_commit(oid).unwrap(); assert!(commit.parents().count() == 0); @@ -424,6 +424,19 @@ mod diff { Ok(()) } + #[test] + fn test_diff_of_rev() -> Result<(), Error> { + let repo = Repository::new(GIT_PLATINUM)?; + let repo = repo.as_ref(); + let oid = Oid::from_str("80bacafba303bf0cdf6142921f430ff265f25095")?; + let diff = repo.diff_of_rev(&oid.into())?; + assert_eq!(diff.created.len(), 0); + assert_eq!(diff.deleted.len(), 0); + assert_eq!(diff.moved.len(), 0); + assert_eq!(diff.modified.len(), 1); + Ok(()) + } + #[test] fn test_diff() -> Result<(), Error> { let repo = Repository::new(GIT_PLATINUM)?; @@ -844,4 +857,14 @@ mod code_browsing { count } } + + #[test] + fn test_tag_snapshot() { + let repo = Repository::new(GIT_PLATINUM).unwrap(); + let repo_ref = repo.as_ref(); + let tags = repo_ref.list_tags(RefScope::Local).unwrap(); + assert_eq!(tags.len(), 6); + let root_dir = repo_ref.snapshot(&tags[0].name().into()).unwrap(); + assert_eq!(root_dir.contents().count(), 1); + } }