diff --git a/radicle-surf/docs/refactor-design.md b/radicle-surf/docs/refactor-design.md index b5449c9..583b0da 100644 --- a/radicle-surf/docs/refactor-design.md +++ b/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//<...>. -- 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. +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: +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 + +### Code browsing + +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; +} ``` -The above function can be easily implement as a method of `Repository`. +- Browse a Directory: +```Rust +impl Directory { + pub fn contents(&self) -> impl Iterator; +} +``` +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; +} +``` + +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; +} +``` + +## Error handling + +TBD diff --git a/radicle-surf/examples/diff.rs b/radicle-surf/examples/diff.rs index 6f721ae..ca8058c 100644 --- a/radicle-surf/examples/diff.rs +++ b/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); } diff --git a/radicle-surf/src/commit.rs b/radicle-surf/src/commit.rs index 983c4cb..55bc400 100644 --- a/radicle-surf/src/commit.rs +++ b/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 { +pub fn commit(repo: &RepositoryRef, rev: &Rev) -> Result { + 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; diff --git a/radicle-surf/src/diff.rs b/radicle-surf/src/diff.rs index b6d3217..55418ee 100644 --- a/radicle-surf/src/diff.rs +++ b/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>, 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)); }, } diff --git a/radicle-surf/src/file_system/directory.rs b/radicle-surf/src/file_system/directory.rs index a8d3d4d..e332a52 100644 --- a/radicle-surf/src/file_system/directory.rs +++ b/radicle-surf/src/file_system/directory.rs @@ -21,42 +21,15 @@ //! See [`Directory`] for more information. //! //! As well as this, this module contains [`DirectoryContents`] which is the -//! output of iterating over a [`Directory`], and also [`SystemType`] which is -//! an identifier of what type of [`DirectoryContents`] one is viewing when -//! [listing](#method.list_directory) a directory. +//! output of iterating over a [`Directory`]. -use crate::{file_system::path::*, tree::*}; +use crate::file_system::path::*; use nonempty::NonEmpty; use std::{ - collections::{hash_map::DefaultHasher, HashMap}, + collections::{hash_map::DefaultHasher, BTreeMap}, hash::{Hash, Hasher}, }; -/// `SystemType` is an enumeration over what can be found in a [`Directory`] so -/// we can report back to the caller a [`Label`] and its type. -/// -/// See [`SystemType::file`](#method.file) and -/// [`SystemType::directory`](#method.directory). -#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] -pub enum SystemType { - /// The `File` type in a directory system. - File, - /// The `Directory` type in a directory system. - Directory, -} - -impl SystemType { - /// A file name and [`SystemType::File`]. - pub fn file(label: Label) -> (Label, Self) { - (label, SystemType::File) - } - - /// A directory name and [`SystemType::Directory`]. - pub fn directory(label: Label) -> (Label, Self) { - (label, SystemType::Directory) - } -} - /// A `File` consists of its file contents (a [`Vec`] of bytes). /// /// The `Debug` instance of `File` will show the first few bytes of the file and @@ -126,29 +99,16 @@ impl File { } } -#[derive(Debug, Clone, PartialEq, Eq)] -enum Location { - Root, - SubDirectory(Label), -} - -/// A `Directory` can be thought of as a non-empty set of entries of -/// sub-directories and files. The reason for the non-empty property is that a -/// VCS directory would have at least one artifact as a sub-directory which -/// tracks the VCS work, e.g. git using the `.git` folder. -/// -/// On top of that, some VCSes, such as git, will not track an empty directory, -/// and so when creating a new directory to track it will have to contain at -/// least one file. +/// A `Directory` is a set of entries of sub-directories and files, ordered +/// by their unique names in the alphabetical order. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Directory { - current: Location, - sub_directories: Forest, + name: Label, + contents: BTreeMap, } /// `DirectoryContents` is an enumeration of what a [`Directory`] can contain -/// and is used for when we are [`iter`](struct.Directory.html#method.iter)ating -/// through a [`Directory`]. +/// and is used for when we are iterating through a [`Directory`]. #[derive(Debug, Clone, PartialEq, Eq)] pub enum DirectoryContents { /// The `File` variant contains the file's name and the [`File`] itself. @@ -165,168 +125,53 @@ pub enum DirectoryContents { impl DirectoryContents { /// Get a label for the `DirectoryContents`, either the name of the [`File`] /// or the name of the [`Directory`]. - pub fn label(&self) -> Label { + pub fn label(&self) -> &Label { match self { - DirectoryContents::File { name, .. } => name.clone(), + DirectoryContents::File { name, .. } => name, DirectoryContents::Directory(directory) => directory.current(), } } } -impl From> for DirectoryContents { - fn from(sub_tree: SubTree) -> Self { - match sub_tree { - SubTree::Node { key, value } => DirectoryContents::File { - name: key, - file: value, - }, - SubTree::Branch { key, forest } => DirectoryContents::Directory(Directory { - current: Location::SubDirectory(key), - sub_directories: (*forest).into(), - }), - } - } -} - impl Directory { /// Create a root directory. /// /// This function is usually used for testing and demonstation purposes. pub fn root() -> Self { Directory { - current: Location::Root, - sub_directories: Forest::root(), + name: Label::root(), + contents: BTreeMap::new(), } } /// Create a directory, similar to `root`, except with a given name. /// /// This function is usually used for testing and demonstation purposes. - pub fn new(label: Label) -> Self { + pub fn new(name: Label) -> Self { Directory { - current: Location::SubDirectory(label), - sub_directories: Forest::root(), + name, + contents: BTreeMap::new(), } } - /// List the current `Directory`'s files and sub-directories. - /// - /// The listings are a pair of [`Label`] and [`SystemType`], where the - /// [`Label`] represents the name of the file or directory. - /// - /// ``` - /// use nonempty::NonEmpty; - /// use radicle_surf::file_system::{Directory, File, SystemType}; - /// use radicle_surf::file_system::unsound; - /// - /// let mut directory = Directory::root(); - /// - /// // Root files set up - /// let root_files = NonEmpty::from(( - /// (unsound::label::new("foo.rs"), File::new(b"use crate::bar")), - /// vec![( - /// unsound::label::new("bar.rs"), - /// File::new(b"fn hello_world()"), - /// )], - /// )); - /// directory.insert_files(&[], root_files); - /// - /// // Haskell files set up - /// let haskell_files = NonEmpty::from(( - /// ( - /// unsound::label::new("foo.hs"), - /// File::new(b"module Foo where"), - /// ), - /// vec![( - /// unsound::label::new("bar.hs"), - /// File::new(b"module Bar where"), - /// )], - /// )); - /// - /// directory.insert_files(&[unsound::label::new("haskell")], haskell_files); - /// - /// let mut directory_contents = directory.list_directory(); - /// directory_contents.sort(); - /// - /// assert_eq!( - /// directory_contents, - /// vec![ - /// SystemType::file(unsound::label::new("bar.rs")), - /// SystemType::file(unsound::label::new("foo.rs")), - /// SystemType::directory(unsound::label::new("haskell")), - /// ] - /// ); - /// ``` - pub fn list_directory(&self) -> Vec<(Label, SystemType)> { - let forest = &self.sub_directories; - match &forest.0 { - None => vec![], - Some(trees) => trees - .0 - .iter() - .map(|tree| match tree { - SubTree::Node { key: name, .. } => SystemType::file(name.clone()), - SubTree::Branch { key: name, .. } => SystemType::directory(name.clone()), - }) - .collect(), - } + /// Get the name of the current `Directory`. + pub fn name(&self) -> &Label { + &self.name } - /// Get the [`Label`] of the current directory. - /// - /// # Examples - /// - /// ``` - /// use radicle_surf::file_system::{Directory, DirectoryContents, File, Label}; - /// use radicle_surf::file_system::unsound; - /// - /// let mut root = Directory::root(); - /// - /// let main = File::new(b"println!(\"Hello, world!\")"); - /// root.insert_file(unsound::path::new("main.rs"), main.clone()); - /// - /// let lib = File::new(b"struct Hello(String)"); - /// root.insert_file(unsound::path::new("lib.rs"), lib.clone()); - /// - /// let test_mod = File::new(b"assert_eq!(1 + 1, 2);"); - /// root.insert_file(unsound::path::new("test/mod.rs"), test_mod.clone()); - /// - /// let mut root_iter = root.iter(); - /// - /// assert_eq!(root_iter.next(), Some(DirectoryContents::File { - /// name: unsound::label::new("lib.rs"), - /// file: lib - /// })); - /// - /// assert_eq!(root_iter.next(), Some(DirectoryContents::File { - /// name: unsound::label::new("main.rs"), - /// file: main - /// })); - /// - /// let mut test_dir = Directory::new(unsound::label::new("test")); - /// test_dir.insert_file(unsound::path::new("mod.rs"), test_mod); - /// - /// assert_eq!(root_iter.next(), Some(DirectoryContents::Directory(test_dir))); - /// ``` - pub fn iter(&self) -> impl Iterator + '_ { - let mut empty_iter = None; - let mut trees_iter = None; - match &self.sub_directories.0 { - None => empty_iter = Some(std::iter::empty()), - Some(trees) => { - trees_iter = Some( - trees - .iter_subtrees() - .cloned() - .map(|sub_tree| sub_tree.into()), - ) - }, - } + /// Add the `content` under `name` to the current `Directory`. + /// If `name` already exists in this directory, then the previous contents + /// are replaced. + pub fn insert(&mut self, name: Label, content: DirectoryContents) { + self.contents.insert(name, content); + } - empty_iter - .into_iter() - .flatten() - .chain(trees_iter.into_iter().flatten()) + /// Returns an iterator for the contents of the current directory. + /// + /// Note that the returned iterator only iterates the current level, + /// without going recursively into sub-directories. + pub fn contents(&self) -> impl Iterator { + self.contents.values() } /// Find a [`File`] in the directory given the [`Path`] to the [`File`]. @@ -361,8 +206,23 @@ impl Directory { /// // We shouldn't be able to find a file that doesn't exist /// assert_eq!(directory.find_file(unsound::path::new("foo/bar/qux.rs")), None); /// ``` - pub fn find_file(&self, path: Path) -> Option { - self.sub_directories.find_node(path.0).cloned() + pub fn find_file(&self, path: Path) -> Option<&File> { + let mut contents = &self.contents; + let path_depth = path.0.len(); + for (idx, label) in path.iter().enumerate() { + match contents.get(label) { + Some(DirectoryContents::Directory(d)) => contents = &d.contents, + Some(DirectoryContents::File { name: _, file }) => { + if idx + 1 == path_depth { + return Some(file); + } else { + break; // Abort: finding a file before the last label. + } + }, + None => break, // Abort: a label not found. + } + } + None } /// Find a `Directory` in the directory given the [`Path`] to the @@ -400,17 +260,22 @@ impl Directory { /// // 'baz.rs' is a file and not a directory /// assert!(directory.find_directory(unsound::path::new("foo/bar/baz.rs")).is_none()); /// ``` - pub fn find_directory(&self, path: Path) -> Option { - self.sub_directories - .find_branch(path.0.clone()) - .cloned() - .map(|tree| { - let (_, current) = path.split_last(); - Directory { - current: Location::SubDirectory(current), - sub_directories: tree.into(), - } - }) + pub fn find_directory(&self, path: Path) -> Option<&Self> { + let mut found = None; + let mut contents = &self.contents; + + for label in path.iter() { + match contents.get(label) { + Some(DirectoryContents::Directory(d)) => { + found = Some(d); + contents = &d.contents; + }, + Some(DirectoryContents::File { .. }) => break, // Abort: should not be a file. + None => break, // Abort: a label not found. + } + } + + found } /// Get the [`Label`] of the current directory. @@ -433,11 +298,8 @@ impl Directory { /// ).expect("Missing test directory"); /// assert_eq!(test.current(), unsound::label::new("test")); /// ``` - pub fn current(&self) -> Label { - match &self.current { - Location::Root => Label::root(), - Location::SubDirectory(label) => label.clone(), - } + pub fn current(&self) -> &Label { + &self.name } // TODO(fintan): This is going to be a bit trickier so going to leave it out for @@ -464,9 +326,13 @@ impl Directory { /// assert_eq!(root.size(), 66); /// ``` pub fn size(&self) -> usize { - self.sub_directories - .iter() - .fold(0, |size, file| size + file.size()) + self.contents().fold(0, |size, item| { + if let DirectoryContents::File { name: _, file } = item { + size + file.size() + } else { + size + } + }) } /// Insert a file into a directory, given the full path to file (file name @@ -474,7 +340,39 @@ impl Directory { /// /// This function is usually used for testing and demonstation purposes. pub fn insert_file(&mut self, path: Path, file: File) { - self.sub_directories.insert(path.0, file) + let name = path.0.last().clone(); + let f = DirectoryContents::File { + name: name.clone(), + file, + }; + + let mut contents = &mut self.contents; + let path_depth = path.0.len() - 1; // exclude the last label: file name + + if path_depth == 0 { + contents.insert(name, f); + } else { + for (idx, label) in path.iter().enumerate() { + // if label does not exist, create a sub directory. + if contents.get(label).is_none() { + let new_dir = Directory::new(label.clone()); + contents.insert(label.clone(), DirectoryContents::Directory(new_dir)); + } + + match contents.get_mut(label) { + Some(DirectoryContents::Directory(d)) => { + contents = &mut d.contents; + if idx + 1 == path_depth { + // We are in the last directory level, insert the file. + contents.insert(name, f); + return; + } + }, + Some(DirectoryContents::File { .. }) => return, // Abort: should not be a file. + None => return, + } + } + } } /// Insert files into a shared directory path. @@ -503,25 +401,4 @@ impl Directory { }, } } - - /// Creates a `Directory` from a HashMap `files`. - pub fn from_hash_map(files: HashMap>) -> Self { - let mut directory: Self = Directory::root(); - - for (path, files) in files.into_iter() { - for (file_name, file) in files.into_iter() { - let file_path = if path.is_root() { - Path::new(file_name) - } else { - let mut new_path = path.clone(); - new_path.push(file_name); - new_path - }; - - directory.insert_file(file_path, file) - } - } - - directory - } } diff --git a/radicle-surf/src/file_system/path.rs b/radicle-surf/src/file_system/path.rs index f1d97ba..971d6ef 100644 --- a/radicle-surf/src/file_system/path.rs +++ b/radicle-surf/src/file_system/path.rs @@ -48,6 +48,9 @@ impl Deref for Label { } } +/// The label for the root directory. +pub const ROOT_LABEL: &str = "~"; + impl Label { /// The root label for the root directory, i.e. `"~"`. /// @@ -64,7 +67,7 @@ impl Label { /// ``` pub fn root() -> Self { Label { - label: "~".into(), + label: ROOT_LABEL.into(), hidden: false, } } diff --git a/radicle-surf/src/lib.rs b/radicle-surf/src/lib.rs index c7d6016..34dcbbe 100644 --- a/radicle-surf/src/lib.rs +++ b/radicle-surf/src/lib.rs @@ -103,8 +103,6 @@ pub mod syntax; #[cfg(feature = "syntax")] pub use syntax::SYNTAX_SET; -pub mod tree; - // Private modules mod nonempty; diff --git a/radicle-surf/src/object/tree.rs b/radicle-surf/src/object/tree.rs index 608cdf5..1e60f44 100644 --- a/radicle-surf/src/object/tree.rs +++ b/radicle-surf/src/object/tree.rs @@ -28,7 +28,7 @@ use serde::{ use crate::{ commit, - file_system, + file_system::{self, DirectoryContents}, git::RepositoryRef, object::{Error, Info, ObjectType}, revision::Revision, @@ -113,33 +113,31 @@ where let root_dir = repo.snapshot(&rev)?; let prefix_dir = if path.is_root() { - root_dir + &root_dir } else { root_dir .find_directory(path.clone()) .ok_or_else(|| Error::PathNotFound(path.clone()))? }; - let mut prefix_contents = prefix_dir.list_directory(); - prefix_contents.sort(); - let entries_results: Result, Error> = prefix_contents - .iter() - .map(|(label, system_type)| { + let entries_results: Result, Error> = prefix_dir + .contents() + .map(|entry| { let entry_path = if path.is_root() { - file_system::Path::new(label.clone()) + file_system::Path::new(entry.label().clone()) } else { let mut p = path.clone(); - p.push(label.clone()); + p.push(entry.label().clone()); p }; let mut commit_path = file_system::Path::root(); commit_path.append(entry_path.clone()); let info = Info { - name: label.to_string(), - object_type: match system_type { - file_system::SystemType::Directory => ObjectType::Tree, - file_system::SystemType::File => ObjectType::Blob, + name: entry.label().to_string(), + object_type: match entry { + DirectoryContents::Directory(_) => ObjectType::Tree, + DirectoryContents::File { .. } => ObjectType::Blob, }, last_commit: None, }; diff --git a/radicle-surf/src/tree.rs b/radicle-surf/src/tree.rs deleted file mode 100644 index 541e755..0000000 --- a/radicle-surf/src/tree.rs +++ /dev/null @@ -1,503 +0,0 @@ -// This file is part of radicle-surf -// -// -// Copyright (C) 2019-2020 The Radicle Team -// -// This program is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License version 3 or -// later as published by the Free Software Foundation. -// -// This program is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. -// -// You should have received a copy of the GNU General Public License -// along with this program. If not, see . - -#![allow(missing_docs)] - -use crate::nonempty::split_last; -use nonempty::NonEmpty; -use std::cmp::Ordering; - -#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] -pub enum SubTree { - Node { key: K, value: A }, - Branch { key: K, forest: Box> }, -} - -impl SubTree { - /// Create a new `Branch` from a key and sub-tree. - /// - /// This function is a convenience for now having to - /// remember to use `Box::new`. - fn branch(key: K, tree: Tree) -> Self { - SubTree::Branch { - key, - forest: Box::new(tree), - } - } - - fn key(&self) -> &K { - match self { - SubTree::Node { key, .. } => key, - SubTree::Branch { key, .. } => key, - } - } - - pub fn find(&self, keys: NonEmpty) -> Option<&Self> - where - K: Ord, - { - let (head, tail) = keys.into(); - let tail = NonEmpty::from_vec(tail); - match self { - SubTree::Node { key, .. } => match tail { - None if *key == head => Some(self), - _ => None, - }, - SubTree::Branch { key, ref forest } => match tail { - None if *key == head => Some(self), - None => None, - Some(keys) => forest.find(keys), - }, - } - } - - pub fn to_nonempty(&self) -> NonEmpty - where - A: Clone, - K: Clone, - { - match self { - Self::Node { value, .. } => NonEmpty::new(value.clone()), - Self::Branch { forest, .. } => forest.to_nonempty(), - } - } - - pub(crate) fn iter<'a>(&'a self) -> Box + 'a> { - match self { - SubTree::Node { value, .. } => Box::new(std::iter::once(value)), - SubTree::Branch { ref forest, .. } => Box::new(forest.iter()), - } - } - - fn iter_keys<'a>(&'a self) -> Box + 'a> { - match self { - SubTree::Node { key, .. } => Box::new(std::iter::once(key)), - SubTree::Branch { - ref key, - ref forest, - } => Box::new(std::iter::once(key).chain(forest.iter_keys())), - } - } - - fn compare_by(&self, other: &Self, f: &F) -> Ordering - where - F: Fn(&A, &A) -> Ordering, - { - match (self, other) { - ( - SubTree::Node { value, .. }, - SubTree::Node { - value: other_value, .. - }, - ) => f(value, other_value), - (SubTree::Branch { forest, .. }, SubTree::Node { value, .. }) => { - let max_forest = forest.maximum_by(f); - f(max_forest, value) - }, - (SubTree::Node { value, .. }, SubTree::Branch { forest, .. }) => { - let max_forest = &forest.maximum_by(f); - f(value, max_forest) - }, - ( - SubTree::Branch { forest, .. }, - SubTree::Branch { - forest: other_forest, - .. - }, - ) => { - let max_forest = forest.maximum_by(f); - let max_other_forest = other_forest.maximum_by(f); - f(max_forest, max_other_forest) - }, - } - } - - pub fn maximum_by(&self, f: &F) -> &A - where - F: Fn(&A, &A) -> Ordering, - { - match self { - SubTree::Node { value, .. } => value, - SubTree::Branch { forest, .. } => forest.maximum_by(f), - } - } - - pub fn map(self, f: &mut F) -> SubTree - where - F: FnMut(A) -> B, - { - match self { - SubTree::Node { key, value } => SubTree::Node { - key, - value: f(value), - }, - SubTree::Branch { key, forest } => SubTree::Branch { - key, - forest: Box::new(forest.map(f)), - }, - } - } -} - -#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] -pub struct Tree(pub NonEmpty>); - -impl From> for Forest { - fn from(tree: Tree) -> Self { - Forest(Some(tree)) - } -} - -#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] -pub struct Forest(pub Option>); - -impl Tree { - /// Create a new `Tree` containing a single `Branch` given - /// the key and sub-tree. - pub fn branch(key: K, forest: Self) -> Self { - Tree(NonEmpty::new(SubTree::branch(key, forest))) - } - - /// Create a new `Tree` containing a single `Node`. - pub fn node(key: K, value: A) -> Self { - Tree(NonEmpty::new(SubTree::Node { key, value })) - } - - /// Create a new `Tree` that creates a series of - /// `Branch`es built using the `keys`. The final `Branch` - /// will contain the `node`. - fn new(keys: NonEmpty, node: A) -> Self - where - K: Ord, - { - let (start, mut middle) = keys.into(); - let last = middle.pop(); - - match last { - None => Tree::node(start, node), - Some(last) => { - let mut branch = Tree::node(last, node); - - for key in middle.into_iter().rev() { - branch = Tree(NonEmpty::new(SubTree::branch(key, branch))) - } - - Tree::branch(start, branch) - }, - } - } - - /// Perform a binary search in the sub-trees, based on comparing - /// each of the sub-trees' key to the provided `key`. - fn search(&self, key: &K) -> Result - where - K: Ord, - { - self.0.binary_search_by(|tree| tree.key().cmp(key)) - } - - pub fn map(self, mut f: F) -> Tree - where - F: FnMut(A) -> B, - { - Tree(self.0.map(|tree| tree.map(&mut f))) - } - - /// Insert a `node` into the list of sub-trees. - /// - /// The node's position will be based on the `Ord` instance - /// of `K`. - fn insert_node_with(&mut self, key: K, value: A, f: F) - where - F: FnOnce(&mut A), - K: Ord, - { - let result = self.search(&key); - - match result { - Ok(index) => { - let old_node = self.0.get_mut(index).unwrap(); - match old_node { - SubTree::Node { value: old, .. } => f(old), - SubTree::Branch { .. } => *old_node = SubTree::Node { key, value }, - } - }, - Err(index) => self.0.insert(index, SubTree::Node { key, value }), - } - } - - /// Insert the `node` in the position given by `keys`. - /// - /// If the same path to a node is provided the `node` will replace the old - /// one, i.e. if `a/b/c` exists in the tree and `a/b/c` is the full path - /// to the node, then `c` will be replaced. - /// - /// If the path points to a branch, then the `node` will be inserted in this - /// branch. - /// - /// If a portion of the path points to a node then a branch will be created - /// in its place, i.e. if `a/b/c` exists in the tree and the provided - /// path is `a/b/c/d`, then the node `c` will be replaced by a branch - /// `c/d`. - /// - /// If the path does not exist it will be inserted into the set of - /// sub-trees. - fn insert_with(&mut self, keys: NonEmpty, value: A, f: F) - where - F: FnOnce(&mut A), - K: Ord, - { - let (head, tail) = keys.into(); - let maybe_keys = NonEmpty::from_vec(tail); - match self.search(&head) { - // Found the label in our set of sub-trees - Ok(index) => match maybe_keys { - // The keys have been exhausted and so its time to insert the node - None => { - let sub_tree = self.0.get_mut(index).unwrap(); - match sub_tree { - // Our sub-tree was a node. - SubTree::Node { key, value } => { - let _ = std::mem::replace(key, head); - f(value); - }, - SubTree::Branch { .. } => *sub_tree = SubTree::Node { key: head, value }, - } - }, - Some(keys) => { - let sub_tree = self.0.get_mut(index).unwrap(); - match sub_tree { - // We have reached a node, but still have keys left to get through. - SubTree::Node { .. } => { - let new_tree = SubTree::branch(head, Tree::new(keys, value)); - *sub_tree = new_tree - }, - // We keep moving down the set of keys to find where to insert this node. - SubTree::Branch { forest, .. } => forest.insert_with(keys, value, f), - } - }, - }, - // The label was not found and we have an index for insertion. - Err(index) => match maybe_keys { - // We create the branch with the head label and node, since there are - // no more labels left. - None => self.0.insert(index, SubTree::Node { key: head, value }), - // We insert an entirely new branch with the full list of keys. - Some(tail) => self - .0 - .insert(index, SubTree::branch(head, Tree::new(tail, value))), - }, - } - } - - pub fn insert(&mut self, keys: NonEmpty, value: A) - where - A: Clone, - K: Ord, - { - self.insert_with(keys, value.clone(), |old| *old = value) - } - - pub fn to_nonempty(&self) -> NonEmpty - where - A: Clone, - K: Clone, - { - self.0.clone().flat_map(|sub_tree| sub_tree.to_nonempty()) - } - - pub fn iter(&self) -> impl Iterator { - self.0.iter().flat_map(|tree| tree.iter()) - } - - pub fn iter_keys(&self) -> impl Iterator { - self.0.iter().flat_map(|tree| tree.iter_keys()) - } - - pub fn iter_subtrees(&self) -> impl Iterator> { - self.0.iter() - } - - pub fn find_node(&self, keys: NonEmpty) -> Option<&A> - where - K: Ord, - { - self.find(keys).and_then(|tree| match tree { - SubTree::Node { value, .. } => Some(value), - SubTree::Branch { .. } => None, - }) - } - - pub fn find_branch(&self, keys: NonEmpty) -> Option<&Self> - where - K: Ord, - { - self.find(keys).and_then(|tree| match tree { - SubTree::Node { .. } => None, - SubTree::Branch { ref forest, .. } => Some(&**forest), - }) - } - - /// Find a `SubTree` given a search path. If the path does not match - /// it will return `None`. - pub fn find(&self, keys: NonEmpty) -> Option<&SubTree> - where - K: Ord, - { - let (head, tail) = keys.into(); - let tail = NonEmpty::from_vec(tail); - match self.search(&head) { - Err(_) => None, - Ok(index) => { - let sub_tree = self.0.get(index).unwrap(); - match tail { - None => match sub_tree { - SubTree::Node { .. } => Some(sub_tree), - SubTree::Branch { .. } => Some(sub_tree), - }, - Some(mut tail) => { - tail.insert(0, head); - sub_tree.find(tail) - }, - } - }, - } - } - - pub fn maximum_by(&self, f: &F) -> &A - where - F: Fn(&A, &A) -> Ordering, - { - self.0.maximum_by(|s, t| s.compare_by(t, f)).maximum_by(f) - } - - #[allow(dead_code)] - pub fn maximum(&self) -> &A - where - A: Ord, - { - self.maximum_by(&|a, b| a.cmp(b)) - } -} - -impl Forest { - pub fn root() -> Self { - Forest(None) - } - - #[allow(dead_code)] - pub fn is_empty(&self) -> bool { - self.0.is_none() - } - - fn insert_forest(&mut self, forest: Tree) { - self.0 = Some(forest) - } - - /// Insert the `node` in the position given by `keys`. - /// - /// If the same path to a node is provided the `node` will replace the old - /// one, i.e. if `a/b/c` exists in the tree and `a/b/c` is the full path - /// to the node, then `c` will be replaced. - /// - /// If the path points to a branch, then the `node` will be inserted in this - /// branch. - /// - /// If a portion of the path points to a node then a branch will be created - /// in its place, i.e. if `a/b/c` exists in the tree and the provided - /// path is `a/b/c/d`, then the node `c` will be replaced by a branch - /// `c/d`. - /// - /// If the path does not exist it will be inserted into the set of - /// sub-trees. - #[allow(dead_code)] - pub fn insert(&mut self, keys: NonEmpty, node: A) - where - A: Clone, - K: Ord, - { - self.insert_with(keys, node.clone(), |old| *old = node) - } - - pub fn insert_with(&mut self, keys: NonEmpty, node: A, f: F) - where - F: FnOnce(&mut A), - K: Ord, - { - let (prefix, node_key) = split_last(keys); - match self.0.as_mut() { - Some(forest) => match NonEmpty::from_vec(prefix) { - None => { - // Insert the node at the root - forest.insert_node_with(node_key, node, f) - }, - Some(mut keys) => { - keys.push(node_key); - forest.insert_with(keys, node, f) - }, - }, - None => match NonEmpty::from_vec(prefix) { - None => self.insert_forest(Tree::node(node_key, node)), - Some(mut keys) => { - keys.push(node_key); - self.insert_forest(Tree::new(keys, node)) - }, - }, - } - } - - pub fn find_node(&self, keys: NonEmpty) -> Option<&A> - where - K: Ord, - { - self.0.as_ref().and_then(|trees| trees.find_node(keys)) - } - - pub fn find_branch(&self, keys: NonEmpty) -> Option<&Tree> - where - K: Ord, - { - self.0.as_ref().and_then(|trees| trees.find_branch(keys)) - } - - #[allow(dead_code)] - /// Find a `SubTree` given a search path. If the path does not match - /// it will return `None`. - pub fn find(&self, keys: NonEmpty) -> Option<&SubTree> - where - K: Ord, - { - self.0.as_ref().and_then(|trees| trees.find(keys)) - } - - #[allow(dead_code)] - pub fn maximum_by(&self, f: F) -> Option<&A> - where - F: Fn(&A, &A) -> Ordering, - { - self.0.as_ref().map(|trees| trees.maximum_by(&f)) - } - - pub fn iter(&self) -> impl Iterator { - self.0.iter().flat_map(|trees| trees.iter()) - } - - #[allow(dead_code)] - pub fn iter_keys(&self) -> impl Iterator { - self.0.iter().flat_map(|trees| trees.iter_keys()) - } -} diff --git a/radicle-surf/src/vcs/git/error.rs b/radicle-surf/src/vcs/git/error.rs index 5e4c23d..474b09c 100644 --- a/radicle-surf/src/vcs/git/error.rs +++ b/radicle-surf/src/vcs/git/error.rs @@ -89,38 +89,3 @@ pub enum Error { #[error(transparent)] Git(#[from] git2::Error), } - -/// A private enum that captures a recoverable and -/// non-recoverable error when walking the git tree. -/// -/// In the case of `NotBlob` we abort the the computation but do -/// a check for it and recover. -/// -/// In the of `Git` we abort both computations. -#[derive(Debug, Error)] -pub(crate) enum TreeWalkError { - #[error("entry is not a blob")] - NotBlob, - #[error("git object is a commit")] - Commit, - #[error(transparent)] - Git(#[from] Error), -} - -impl From for TreeWalkError { - fn from(err: git2::Error) -> Self { - TreeWalkError::Git(err.into()) - } -} - -impl From for TreeWalkError { - fn from(err: file_system::Error) -> Self { - err.into() - } -} - -impl From for TreeWalkError { - fn from(err: str::Utf8Error) -> Self { - err.into() - } -} diff --git a/radicle-surf/src/vcs/git/repo.rs b/radicle-surf/src/vcs/git/repo.rs index 9c4c64b..51240b9 100644 --- a/radicle-surf/src/vcs/git/repo.rs +++ b/radicle-surf/src/vcs/git/repo.rs @@ -18,7 +18,7 @@ use crate::{ diff::*, file_system, - file_system::directory, + file_system::{directory, DirectoryContents, Label}, vcs, vcs::{ git::{ @@ -37,10 +37,11 @@ use crate::{ Vcs, }, }; +use directory::Directory; use nonempty::NonEmpty; use radicle_git_ext::Oid; use std::{ - collections::{BTreeSet, HashMap, HashSet}, + collections::{BTreeSet, HashSet}, convert::TryFrom, str, }; @@ -163,14 +164,17 @@ impl<'a> RepositoryRef<'a> { } /// Get the [`Diff`] between two commits. - pub fn diff(&self, from: Oid, to: Oid) -> Result { - self.diff_commits(None, Some(from.into()), to.into()) + pub fn diff(&self, from: &Rev, to: &Rev) -> Result { + let from_commit = self.rev_to_commit(from)?; + let to_commit = self.rev_to_commit(to)?; + self.diff_commits(None, Some(&from_commit), &to_commit) .and_then(|diff| Diff::try_from(diff).map_err(Error::from)) } /// Get the [`Diff`] of a commit with no parents. - pub fn initial_diff(&self, oid: Oid) -> Result { - self.diff_commits(None, None, oid.into()) + pub fn initial_diff(&self, rev: &Rev) -> Result { + let commit = self.rev_to_commit(rev)?; + self.diff_commits(None, None, &commit) .and_then(|diff| Diff::try_from(diff).map_err(Error::from)) } @@ -180,11 +184,9 @@ impl<'a> RepositoryRef<'a> { } /// Gets a snapshot of the repo as a Directory. - pub fn snapshot(&self, rev: &Rev) -> Result { + pub fn snapshot(&self, rev: &Rev) -> Result { let commit = self.rev_to_commit(rev)?; - let oid: Oid = commit.id().into(); - let tree = self.get_tree(&oid)?; - Ok(directory::Directory::from_hash_map(tree)) + self.directory_of_commit(&commit) } /// Returns the last commit, if exists, for a `path` in the history of @@ -254,6 +256,12 @@ impl<'a> RepositoryRef<'a> { Ok(head_commit.id().into()) } + /// Returns the Oid of `rev`. + pub fn rev_oid(&self, rev: &Rev) -> Result { + let commit = self.rev_to_commit(rev)?; + Ok(commit.id().into()) + } + pub(super) fn rev_to_commit(&self, rev: &Rev) -> Result { match rev { Rev::Oid(oid) => Ok(self.repo_ref.find_commit((*oid).into())?), @@ -406,9 +414,8 @@ impl<'a> RepositoryRef<'a> { commit: &git2::Commit, ) -> Result, Error> { let mut parents = commit.parents(); - let parent = parents.next().map(|c| c.id()); - let diff = self.diff_commits(Some(path), parent, commit.id())?; + let diff = self.diff_commits(Some(path), parents.next().as_ref(), commit)?; if let Some(_delta) = diff.deltas().next() { Ok(Some(path.clone())) } else { @@ -419,13 +426,11 @@ impl<'a> RepositoryRef<'a> { fn diff_commits( &self, path: Option<&file_system::Path>, - from: Option, - to: git2::Oid, + from: Option<&git2::Commit>, + to: &git2::Commit, ) -> Result { - let new_tree = self.repo_ref.find_commit(to)?.tree()?; - let old_tree = from.map_or(Ok(None), |oid| { - self.repo_ref.find_commit(oid)?.tree().map(Some) - })?; + let new_tree = to.tree()?; + let old_tree = from.map_or(Ok(None), |c| c.tree().map(Some))?; let mut opts = git2::DiffOptions::new(); if let Some(path) = path { @@ -441,97 +446,98 @@ impl<'a> RepositoryRef<'a> { Ok(diff) } - fn update_file_map( - path: file_system::Path, - name: file_system::Label, - file: directory::File, - files: &mut HashMap>, - ) { - files - .entry(path) - .and_modify(|entries| entries.push((name.clone(), file.clone()))) - .or_insert_with(|| NonEmpty::new((name, file))); - } - - /// Do a pre-order TreeWalk of the given commit. This turns a Tree - /// into a HashMap of Paths and a list of Files. We can then turn that - /// into a Directory. - fn get_tree( - &self, - oid: &Oid, - ) -> Result>, Error> - { - let mut file_paths_or_error: Result< - HashMap>, - Error, - > = Ok(HashMap::new()); - - let commit = self.repo_ref.find_commit((*oid).into())?; + /// Generates a Directory for the commit. + fn directory_of_commit(&self, commit: &git2::Commit) -> Result { + let mut parent_dirs = vec![Directory::root()]; let tree = commit.as_object().peel_to_tree()?; tree.walk(git2::TreeWalkMode::PreOrder, |s, entry| { - match self.tree_entry_to_file_and_path(s, entry) { - Ok((path, name, file)) => { - match file_paths_or_error.as_mut() { - Ok(files) => Self::update_file_map(path, name, file, files), + let tree_level = s.split('/').count(); + if tree_level < parent_dirs.len() { + // As it is PreOrder, the last directory A was visited + // completely and we are back to the level. Now insert A + // into its parent directory. + if let Some(last_dir) = parent_dirs.pop() { + if let Some(parent) = parent_dirs.last_mut() { + let name = last_dir.name().clone(); + let content = DirectoryContents::Directory(last_dir); + parent.insert(name, content); + } + } + } - // We don't need to update, we want to keep the error. - Err(_err) => {}, + match entry.kind() { + Some(git2::ObjectType::Tree) => { + if let Some(name) = entry.name() { + // Add a new level of directory. + match name.parse() { + Ok(label) => parent_dirs.push(Directory::new(label)), + Err(_) => { + return git2::TreeWalkResult::Abort; + }, + } } - git2::TreeWalkResult::Ok }, - Err(err) => match err { - // We want to continue if the entry was not a Blob. - TreeWalkError::NotBlob => git2::TreeWalkResult::Ok, - - // We found a ObjectType::Commit (likely a submodule) and - // so we can skip it. - TreeWalkError::Commit => git2::TreeWalkResult::Ok, - - // But we want to keep the error and abort otherwise. - TreeWalkError::Git(err) => { - file_paths_or_error = Err(err); - git2::TreeWalkResult::Abort - }, + Some(git2::ObjectType::Blob) => { + // Construct a File to insert into its parent directory. + let object = match entry.to_object(self.repo_ref) { + Ok(obj) => obj, + Err(_) => { + return git2::TreeWalkResult::Abort; + }, + }; + let blob = match object.as_blob() { + Some(b) => b, + None => return git2::TreeWalkResult::Abort, + }; + let f = directory::File::new(blob.content()); + let label = match entry.name() { + Some(name) => match name.parse::