Merge remote-tracking branch 'han/new-design-2'

* han/new-design-2:
  refactor design: part 2

Author: FintanH

radicle-surf/docs/refactor-design.md

@@ -1,47 +1,153 @@
 # 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:
+## Introduction
 
-- 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.
+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:
 
-The main goals of the changes are:
+1. Code browsing: given a specific commit/ref, browse files and directories.
+2. Diff between two revisions that resolve into two commits.
+3. Retrieve the history of the commits.
+4. Retrieve a specific object: all its metadata.
+5. Retrieve the refs: Branches, Tags, Remotes, Notes and user-defined
+"categories", where a category is: refs/<category>/<...>.
 
-- 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)
+## Motivation
 
-## Make API simpler
+The `radicle-surf` crate aims to provide a safe and easy-to-use API that
+supports the features listed in [Introduction]. Based on the existing API,
+the main goals of the refactoring are:
 
-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.
+- API review: 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)
+- Hide away `git2` from the API. The use of `git2` should be an implementation
+detail.
+
+## API review
+
+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 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?
+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`
+## Remove the `Snapshot` type
+
+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 snapshot function can be easily implement as a method of `RepositoryRef`.
+
+## Simplify `Directory` and remove the `Tree` and `Forest` types
+
+The `Directory` type represents the file system view of a snapshot. Its field
+`sub_directories` is defined a `Forest` based on `Tree`. The types are
+over-engineered from such a simple concept. We could refactor `Directory` to
+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.
+
+## The new API
+
+With the changes proposed in the previous section, we describe what the new API
+would look like and how they meet the requirements.
+
+### Common principles
+
+#### How to identify things that resolve into a commit
+
+In our API, it will be good to have a single way to identify all refs and
+objects that resolve into commits. In other words, we try to avoid using
+different ways at different places. Currently there are multiple types in the
+API for this purpose:
+
+- Commit
+- Oid
+- Rev
+
+Because `Rev` is the most high level among those and supports refs already,
+I think we should use `Rev` in our API as much as possible.
+
+#### How to identify History
+
+TBD
 
-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.
+### Code browsing
 
-The only `snapshot` function defined currently:
+The user should be able to browse the files and directories for any given
+commits or references. The core API is:
 
+- Create a root Directory:
 ```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))
-        });
+imp RepositoryRef {
+    pub fn snapshot(&self, rev: &Rev) -> Result<Directory, Error>;
+}
 ```
 
-The above function can be easily implement as a method of `Repository`.
+- Browse a Directory:
+```Rust
+impl Directory {
+    pub fn contents(&self) -> impl Iterator<Item = &DirectoryContents>;
+}
+```
+where `DirectoryContents` supports both files and sub-directories:
+```Rust
+pub enum DirectoryContents {
+    /// The `File` variant contains the file's name and the [`File`] itself.
+    File {
+        /// The name of the file.
+        name: Label,
+        /// The file data.
+        file: File,
+    },
+    /// The `Directory` variant contains a sub-directory to the current one.
+    Directory(Directory),
+}
+```
+
+### Diffs
+
+The user would be able to create a diff between any two revisions that resolve
+into two commits.
+
+The main change is to use `Rev` instead of `Oid` to identify `from` and `to`.
+The core API is:
+
+```Rust
+imp RepositoryRef {
+    pub fn diff(&self, from: &Rev, to: &Rev) -> Result<Diff, Error>;
+}
+```
+
+To help convert from `Oid` to `Rev`, we provide a helper method:
+```Rust
+imp RepositoryRef {
+    /// Returns the Oid of `rev`.
+    pub fn rev_oid(&self, rev: &Rev) -> Result<Oid, Error>;
+}
+```
+
+## Error handling
+
+TBD

radicle-surf/examples/diff.rs

@@ -32,7 +32,10 @@ fn main() {
     let base_oid = Oid::from_str(&options.base_revision).unwrap();
     let now = Instant::now();
     let elapsed_nanos = now.elapsed().as_nanos();
-    let diff = repo.as_ref().diff(base_oid, head_oid).unwrap();
+    let diff = repo
+        .as_ref()
+        .diff(&base_oid.into(), &head_oid.into())
+        .unwrap();
     print_diff_summary(&diff, elapsed_nanos);
 }
 

radicle-surf/src/commit.rs

@@ -144,12 +144,14 @@ pub struct Commits {
 ///
 /// Will return [`Error`] if the project doesn't exist or the surf interaction
 /// fails.
-pub fn commit(repo: &RepositoryRef, sha1: Oid) -> Result<Commit, Error> {
+pub fn commit(repo: &RepositoryRef, rev: &Rev) -> Result<Commit, Error> {
+    let sha1 = repo.rev_oid(rev)?;
     let commit = repo.get_commit(sha1)?;
     let diff = if let Some(parent) = commit.parents.first() {
-        repo.diff(*parent, sha1)?
+        let parent_rev = (*parent).into();
+        repo.diff(&parent_rev, rev)?
     } else {
-        repo.initial_diff(sha1)?
+        repo.initial_diff(rev)?
     };
 
     let mut deletions = 0;

radicle-surf/src/diff.rs

@@ -295,7 +295,10 @@ impl Diff {
     #[allow(clippy::self_named_constructors)]
     pub fn diff(left: Directory, right: Directory) -> Self {
         let mut diff = Diff::new();
-        let path = Rc::new(RefCell::new(Path::from_labels(right.current(), &[])));
+        let path = Rc::new(RefCell::new(Path::from_labels(
+            right.current().clone(),
+            &[],
+        )));
         Diff::collect_diff(&left, &right, &path, &mut diff);
 
         // TODO: Some of the deleted files may actually be moved (renamed) to one of the
@@ -315,15 +318,15 @@ impl Diff {
         parent_path: &Rc<RefCell<Path>>,
         diff: &mut Diff,
     ) {
-        let mut old_iter = old.iter();
-        let mut new_iter = new.iter();
+        let mut old_iter = old.contents();
+        let mut new_iter = new.contents();
         let mut old_entry_opt = old_iter.next();
         let mut new_entry_opt = new_iter.next();
 
         while old_entry_opt.is_some() || new_entry_opt.is_some() {
             match (&old_entry_opt, &new_entry_opt) {
                 (Some(ref old_entry), Some(ref new_entry)) => {
-                    match new_entry.label().cmp(&old_entry.label()) {
+                    match new_entry.label().cmp(old_entry.label()) {
                         Ordering::Greater => {
                             diff.add_deleted_files(old_entry, parent_path);
                             old_entry_opt = old_iter.next();
@@ -414,11 +417,11 @@ impl Diff {
                         },
                     }
                 },
-                (Some(ref old_entry), None) => {
+                (Some(old_entry), None) => {
                     diff.add_deleted_files(old_entry, parent_path);
                     old_entry_opt = old_iter.next();
                 },
-                (None, Some(ref new_entry)) => {
+                (None, Some(new_entry)) => {
                     diff.add_created_files(new_entry, parent_path);
                     new_entry_opt = new_iter.next();
                 },
@@ -465,15 +468,15 @@ impl Diff {
     ) where
         F: Fn(Path) -> T + Copy,
     {
-        parent_path.borrow_mut().push(dir.current());
-        for entry in dir.iter() {
+        parent_path.borrow_mut().push(dir.current().clone());
+        for entry in dir.contents() {
             match entry {
                 DirectoryContents::Directory(subdir) => {
-                    Diff::collect_files_inner(&subdir, parent_path, mapper, files);
+                    Diff::collect_files_inner(subdir, parent_path, mapper, files);
                 },
                 DirectoryContents::File { name, .. } => {
                     let mut path = parent_path.borrow().clone();
-                    path.push(name);
+                    path.push(name.clone());
                     files.push(mapper(path));
                 },
             }