Merge pull request #26 from keepsimple1/new-design

radicle-surf: refactoring the design - part 1

Author: keepsimple1

radicle-surf/benches/last_commit.rs

@@ -18,14 +18,13 @@
 use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion};
 use radicle_surf::{
     file_system::{unsound, Path},
-    vcs::git::{Branch, Browser, Repository},
+    vcs::git::{Branch, Repository, Rev},
 };
 
 fn last_commit_comparison(c: &mut Criterion) {
     let repo = Repository::new("./data/git-platinum")
         .expect("Could not retrieve ./data/git-platinum as git repository");
-    let browser =
-        Browser::new(&repo, Branch::local("master")).expect("Could not initialise Browser");
+    let rev: Rev = Branch::local("master").into();
 
     let mut group = c.benchmark_group("Last Commit");
     for path in [
@@ -36,7 +35,7 @@ fn last_commit_comparison(c: &mut Criterion) {
     .iter()
     {
         group.bench_with_input(BenchmarkId::new("", path), path, |b, path| {
-            b.iter(|| browser.last_commit(path.clone()))
+            b.iter(|| repo.as_ref().last_commit(path.clone(), &rev))
         });
     }
 }

radicle-surf/docs/refactor-design.md

@@ -0,0 +1,47 @@
+# An updated design for radicle-surf
+
+Now we have ported the `radicle-surf` crate from its own github repo to be part of the `radicle-git` repo. We are taking this opportunity to refactor its design as well. Intuitively, `radicle-surf` provides an API so that one can use it to create a github-like UI for a git repo:
+
+- Given a commit (or other types of ref), list the content: i.e. files and directories.
+- Generate a diff between two commits.
+- List the history of the commits.
+- List refs: Branches, Tags.
+
+The main goals of the changes are:
+
+- make API simpler whenever possible.
+- address open issues in the original `radicle-surf` repo as much as possible.
+- not to shy away from being `git` specific. (i.e. not to consider supporting other VCS systems)
+
+## Make API simpler
+
+The current API has quite a bit accidental complexity that is not inherent with the requirements, especially when we can be git specific and don't care about other VCS systems.
+
+### Remove the `Browser`
+
+The type `Browser` is awkward as of today:
+
+- it is not a source of truth of any information. For example, `list_branches` method is just a wrapper of `Repository::list_branches`.
+- it takes in `History`, but really works at the `Snapshot` level.
+- it is mutable but its state does not help much.
+
+Can we just remove `Browser` and implement its functionalities using other types?
+
+- For iteratoring the history, use `History`.
+- For generating `Directory`, use `Repository` directly given a `Rev`.
+- For accessing `Branch`, `Tag` or `Commit`, use `Repository`.
+
+## Remove the `Snapshot`
+
+A `Snapshot` should be really just a tree (or `Directory`) of a `Commit` in git. Currently it is a function that returns a `Directory`. Because it is OK to be git specific, we don't need to have this generic function to create a snapshot across different VCS systems.
+
+The only `snapshot` function defined currently:
+
+```Rust
+let snapshot = Box::new(|repository: &RepositoryRef<'a>, history: &History| {
+            let tree = Self::get_tree(repository.repo_ref, history.0.first())?;
+            Ok(directory::Directory::from_hash_map(tree))
+        });
+```
+
+The above function can be easily implement as a method of `Repository`.

radicle-surf/examples/diff.rs

@@ -19,37 +19,20 @@ extern crate radicle_surf;
 
 use std::{env::Args, str::FromStr, time::Instant};
 
-use nonempty::NonEmpty;
-
 use radicle_git_ext::Oid;
-use radicle_surf::{
-    diff::Diff,
-    file_system::Directory,
-    vcs::{git, History},
-};
+use radicle_surf::{diff::Diff, vcs::git};
 
 fn main() {
     let options = get_options_or_exit();
     let repo = init_repository_or_exit(&options.path_to_repo);
-    let mut browser =
-        git::Browser::new(&repo, git::Branch::local("master")).expect("failed to create browser:");
-
-    match options.head_revision {
-        HeadRevision::Head => {
-            reset_browser_to_head_or_exit(&mut browser);
-        },
-        HeadRevision::Commit(id) => {
-            set_browser_history_or_exit(&mut browser, &id);
-        },
-    }
-    let head_directory = get_directory_or_exit(&browser);
-
-    set_browser_history_or_exit(&mut browser, &options.base_revision);
-    let base_directory = get_directory_or_exit(&browser);
-
+    let head_oid = match options.head_revision {
+        HeadRevision::Head => repo.as_ref().head_oid().unwrap(),
+        HeadRevision::Commit(id) => Oid::from_str(&id).unwrap(),
+    };
+    let base_oid = Oid::from_str(&options.base_revision).unwrap();
     let now = Instant::now();
     let elapsed_nanos = now.elapsed().as_nanos();
-    let diff = Diff::diff(base_directory, head_directory);
+    let diff = repo.as_ref().diff(base_oid, head_oid).unwrap();
     print_diff_summary(&diff, elapsed_nanos);
 }
 
@@ -73,46 +56,6 @@ fn init_repository_or_exit(path_to_repo: &str) -> git::Repository {
     }
 }
 
-fn reset_browser_to_head_or_exit(browser: &mut git::Browser) {
-    if let Err(e) = browser.head() {
-        println!("Failed to set browser to HEAD: {:?}", e);
-        std::process::exit(1);
-    }
-}
-
-fn set_browser_history_or_exit(browser: &mut git::Browser, commit_id: &str) {
-    // TODO: Might consider to not require resetting to HEAD when history is not at
-    // HEAD
-    reset_browser_to_head_or_exit(browser);
-    if let Err(e) = set_browser_history(browser, commit_id) {
-        println!("Failed to set browser history: {:?}", e);
-        std::process::exit(1);
-    }
-}
-
-fn set_browser_history(browser: &mut git::Browser, commit_id: &str) -> Result<(), String> {
-    let oid = match Oid::from_str(commit_id) {
-        Ok(oid) => oid,
-        Err(e) => return Err(format!("{}", e)),
-    };
-    let commit = match browser.get().find_in_history(&oid, |artifact| artifact.id) {
-        Some(commit) => commit,
-        None => return Err(format!("Git commit not found: {}", commit_id)),
-    };
-    browser.set(History(NonEmpty::new(commit)));
-    Ok(())
-}
-
-fn get_directory_or_exit(browser: &git::Browser) -> Directory {
-    match browser.get_directory() {
-        Ok(dir) => dir,
-        Err(e) => {
-            println!("Failed to get directory: {:?}", e);
-            std::process::exit(1)
-        },
-    }
-}
-
 fn print_diff_summary(diff: &Diff, elapsed_nanos: u128) {
     diff.created.iter().for_each(|created| {
         println!("+++ {}", created.path);
@@ -131,7 +74,7 @@ fn print_diff_summary(diff: &Diff, elapsed_nanos: u128) {
         diff.modified.len(),
         diff.created.len() + diff.deleted.len() + diff.modified.len()
     );
-    println!("diff took {} micros ", elapsed_nanos / 1000);
+    println!("diff took {} nanos ", elapsed_nanos);
 }
 
 struct Options {

radicle-surf/src/commit.rs

@@ -30,7 +30,10 @@ use crate::{
     file_system,
     person::Person,
     revision::Revision,
-    vcs::git::{self, BranchName, Browser, Rev},
+    vcs::{
+        git::{self, BranchName, RepositoryRef, Rev},
+        Vcs,
+    },
 };
 
 use radicle_git_ext::Oid;
@@ -141,16 +144,12 @@ pub struct Commits {
 ///
 /// Will return [`Error`] if the project doesn't exist or the surf interaction
 /// fails.
-pub fn commit(browser: &mut Browser<'_>, sha1: Oid) -> Result<Commit, Error> {
-    browser.commit(sha1)?;
-
-    let history = browser.get();
-    let commit = history.first();
-
+pub fn commit(repo: &RepositoryRef, sha1: Oid) -> Result<Commit, Error> {
+    let commit = repo.get_commit(sha1)?;
     let diff = if let Some(parent) = commit.parents.first() {
-        browser.diff(*parent, sha1)?
+        repo.diff(*parent, sha1)?
     } else {
-        browser.initial_diff(sha1)?
+        repo.initial_diff(sha1)?
     };
 
     let mut deletions = 0;
@@ -194,14 +193,14 @@ pub fn commit(browser: &mut Browser<'_>, sha1: Oid) -> Result<Commit, Error> {
         }
     }
 
-    let branches = browser
-        .revision_branches(sha1)?
+    let branches = repo
+        .revision_branches(&sha1)?
         .into_iter()
         .map(|b| b.name)
         .collect();
 
     Ok(Commit {
-        header: Header::from(commit),
+        header: Header::from(&commit),
         stats: Stats {
             additions,
             deletions,
@@ -217,13 +216,9 @@ pub fn commit(browser: &mut Browser<'_>, sha1: Oid) -> Result<Commit, Error> {
 ///
 /// Will return [`Error`] if the project doesn't exist or the surf interaction
 /// fails.
-pub fn header(browser: &mut Browser<'_>, sha1: Oid) -> Result<Header, Error> {
-    browser.commit(sha1)?;
-
-    let history = browser.get();
-    let commit = history.first();
-
-    Ok(Header::from(commit))
+pub fn header(repo: &RepositoryRef, sha1: Oid) -> Result<Header, Error> {
+    let commit = repo.get_commit(sha1)?;
+    Ok(Header::from(&commit))
 }
 
 /// Retrieves the [`Commit`] history for the given `revision`.
@@ -233,20 +228,21 @@ pub fn header(browser: &mut Browser<'_>, sha1: Oid) -> Result<Header, Error> {
 /// Will return [`Error`] if the project doesn't exist or the surf interaction
 /// fails.
 pub fn commits<P>(
-    browser: &mut Browser<'_>,
+    repo: &RepositoryRef,
     maybe_revision: Option<Revision<P>>,
 ) -> Result<Commits, Error>
 where
     P: ToString,
 {
     let maybe_revision = maybe_revision.map(Rev::try_from).transpose()?;
 
-    if let Some(revision) = maybe_revision {
-        browser.rev(revision)?;
-    }
+    let rev: Rev = match maybe_revision {
+        Some(revision) => revision,
+        None => repo.head_oid()?.into(),
+    };
 
-    let headers = browser.get().iter().map(Header::from).collect();
-    let stats = browser.get_stats()?;
+    let stats = repo.get_stats(&rev)?;
+    let headers = repo.get_history(rev)?.iter().map(Header::from).collect();
 
     Ok(Commits { headers, stats })
 }

radicle-surf/src/object.rs

@@ -25,7 +25,7 @@ use serde::{
 };
 
 pub mod blob;
-pub use blob::{blob, Blob, BlobContent};
+pub use blob::{Blob, BlobContent};
 
 pub mod tree;
 pub use tree::{tree, Tree, TreeEntry};

radicle-surf/src/object/blob.rs

@@ -32,9 +32,10 @@ use serde::{
 use crate::{
     commit,
     file_system,
+    git::RepositoryRef,
     object::{Error, Info, ObjectType},
     revision::Revision,
-    vcs::git::{Browser, Rev},
+    vcs::git::Rev,
 };
 
 #[cfg(feature = "syntax")]
@@ -118,18 +119,18 @@ impl Serialize for BlobContent {
 /// Will return [`Error`] if the project doesn't exist or a surf interaction
 /// fails.
 pub fn blob<P>(
-    browser: &mut Browser,
+    repo: &RepositoryRef,
     maybe_revision: Option<Revision<P>>,
     path: &str,
 ) -> Result<Blob, Error>
 where
     P: ToString,
 {
-    make_blob(browser, maybe_revision, path, content)
+    make_blob(repo, maybe_revision, path, content)
 }
 
 fn make_blob<P, C>(
-    browser: &mut Browser,
+    repo: &RepositoryRef,
     maybe_revision: Option<Revision<P>>,
     path: &str,
     content: C,
@@ -139,11 +140,9 @@ where
     C: FnOnce(&[u8]) -> BlobContent,
 {
     let maybe_revision = maybe_revision.map(Rev::try_from).transpose()?;
-    if let Some(revision) = maybe_revision {
-        browser.rev(revision)?;
-    }
+    let revision = maybe_revision.unwrap();
 
-    let root = browser.get_directory()?;
+    let root = repo.snapshot(&revision)?;
     let p = file_system::Path::from_str(path)?;
 
     let file = root
@@ -153,8 +152,8 @@ where
     let mut commit_path = file_system::Path::root();
     commit_path.append(p.clone());
 
-    let last_commit = browser
-        .last_commit(commit_path)?
+    let last_commit = repo
+        .last_commit(commit_path, &revision)?
         .map(|c| commit::Header::from(&c));
     let (_rest, last) = p.split_last();
 

radicle-surf/src/object/tree.rs

@@ -29,9 +29,13 @@ use serde::{
 use crate::{
     commit,
     file_system,
+    git::RepositoryRef,
     object::{Error, Info, ObjectType},
     revision::Revision,
-    vcs::git::{Browser, Rev},
+    vcs::{
+        git::{Branch, Rev},
+        Vcs,
+    },
 };
 
 /// Result of a directory listing, carries other trees and blobs.
@@ -86,7 +90,7 @@ impl Serialize for TreeEntry {
 ///
 /// Will return [`Error`] if any of the surf interactions fail.
 pub fn tree<P>(
-    browser: &mut Browser<'_>,
+    repo: &RepositoryRef,
     maybe_revision: Option<Revision<P>>,
     maybe_prefix: Option<String>,
 ) -> Result<Tree, Error>
@@ -96,17 +100,18 @@ where
     let maybe_revision = maybe_revision.map(Rev::try_from).transpose()?;
     let prefix = maybe_prefix.unwrap_or_default();
 
-    if let Some(revision) = maybe_revision {
-        browser.rev(revision)?;
-    }
+    let rev = match maybe_revision {
+        Some(r) => r,
+        None => Branch::local("main").into(),
+    };
 
     let path = if prefix == "/" || prefix.is_empty() {
         file_system::Path::root()
     } else {
         file_system::Path::from_str(&prefix)?
     };
 
-    let root_dir = browser.get_directory()?;
+    let root_dir = repo.snapshot(&rev)?;
     let prefix_dir = if path.is_root() {
         root_dir
     } else {
@@ -155,7 +160,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(browser.get().first()))
+        Some(commit::Header::from(repo.get_history(rev).unwrap().first()))
     } else {
         None
     };