From e382bc4eb9a2423c2be022025e2d5571a14102f0 Mon Sep 17 00:00:00 2001 From: Alexis Sellier Date: Thu, 22 Sep 2022 15:59:20 +0200 Subject: [PATCH] Remove `Project` type Signed-off-by: Alexis Sellier --- radicle-node/src/service.rs | 7 +++--- radicle-node/src/test/peer.rs | 10 +++++++- radicle-node/src/test/tests.rs | 4 ++-- radicle/src/git.rs | 10 ++------ radicle/src/identity.rs | 33 -------------------------- radicle/src/identity/doc.rs | 15 ++++++++---- radicle/src/rad.rs | 41 ++++++++++++++++++--------------- radicle/src/storage.rs | 19 +++++++++------ radicle/src/storage/git.rs | 42 +++++++++++++--------------------- radicle/src/test/arbitrary.rs | 20 +--------------- radicle/src/test/storage.rs | 42 +++++++++++++++++++--------------- 11 files changed, 101 insertions(+), 142 deletions(-) diff --git a/radicle-node/src/service.rs b/radicle-node/src/service.rs index 5254c36c..31117836 100644 --- a/radicle-node/src/service.rs +++ b/radicle-node/src/service.rs @@ -21,9 +21,10 @@ use crate::address_manager::AddressManager; use crate::clock::RefClock; use crate::collections::{HashMap, HashSet}; use crate::crypto; +use crate::crypto::Verified; use crate::git; use crate::git::Url; -use crate::identity::{Id, Project}; +use crate::identity::{Doc, Id}; use crate::service::config::ProjectTracking; use crate::service::message::{NodeAnnouncement, RefsAnnouncement}; use crate::service::peer::{Peer, PeerError, PeerState}; @@ -255,7 +256,7 @@ impl<'r, T: WriteStorage<'r>, S: address_book::Store, G: crypto::Signer> Service } /// Get a project from storage, using the local node's key. - pub fn get(&self, proj: &Id) -> Result, storage::Error> { + pub fn get(&self, proj: &Id) -> Result>, storage::Error> { self.storage.get(&self.node_id(), proj) } @@ -645,7 +646,7 @@ impl Iterator for Service { #[derive(Debug)] pub struct Lookup { /// Whether the project was found locally or not. - pub local: Option, + pub local: Option>, /// A list of remote peers on which the project is known to exist. pub remote: Vec, } diff --git a/radicle-node/src/test/peer.rs b/radicle-node/src/test/peer.rs index bc0590a8..84467664 100644 --- a/radicle-node/src/test/peer.rs +++ b/radicle-node/src/test/peer.rs @@ -6,6 +6,7 @@ use log::*; use crate::address_book::{KnownAddress, Source}; use crate::clock::RefClock; use crate::collections::HashMap; +use crate::git; use crate::git::Url; use crate::service; use crate::service::config::*; @@ -63,10 +64,17 @@ where S: WriteStorage<'r> + 'static, { pub fn new(name: &'static str, ip: impl Into, storage: S) -> Self { + let git_url = Url { + scheme: git::url::Scheme::File, + path: storage.path().to_string_lossy().to_string().into(), + + ..git::Url::default() + }; + Self::config( name, Config { - git_url: storage.url(), + git_url, ..Config::default() }, ip, diff --git a/radicle-node/src/test/tests.rs b/radicle-node/src/test/tests.rs index a8e1e22d..865c8f91 100644 --- a/radicle-node/src/test/tests.rs +++ b/radicle-node/src/test/tests.rs @@ -473,9 +473,9 @@ fn prop_inventory_exchange_dense() { (bob_inv.inventory, bob.node_id()), (eve_inv.inventory, eve.node_id()), ] { - for proj in inv { + for id in inv.keys() { routing - .entry(proj.id.clone()) + .entry(id.clone()) .or_insert_with(|| HashSet::with_hasher(rng.clone().into())) .insert(*peer); } diff --git a/radicle/src/git.rs b/radicle/src/git.rs index 50961d71..11d38338 100644 --- a/radicle/src/git.rs +++ b/radicle/src/git.rs @@ -187,17 +187,11 @@ pub fn configure_remote<'r>( repo: &'r git2::Repository, remote_name: &str, remote_id: &RemoteId, - remote_path: &Path, + remote_url: &Url, ) -> Result, git2::Error> { - let url = Url { - scheme: git_url::Scheme::File, - path: remote_path.to_string_lossy().to_string().into(), - - ..Url::default() - }; let fetch = format!("+refs/remotes/{remote_id}/heads/*:refs/remotes/rad/*"); let push = format!("refs/heads/*:refs/remotes/{remote_id}/heads/*"); - let remote = repo.remote_with_fetch(remote_name, url.to_string().as_str(), &fetch)?; + let remote = repo.remote_with_fetch(remote_name, remote_url.to_string().as_str(), &fetch)?; repo.remote_add_push(remote_name, &push)?; Ok(remote) diff --git a/radicle/src/identity.rs b/radicle/src/identity.rs index 6d9f0bb4..c3a683d1 100644 --- a/radicle/src/identity.rs +++ b/radicle/src/identity.rs @@ -1,17 +1,14 @@ pub mod doc; use std::ops::Deref; -use std::path::PathBuf; use std::{ffi::OsString, fmt, str::FromStr}; use serde::{Deserialize, Serialize}; use thiserror::Error; use crate::crypto; -use crate::crypto::Verified; use crate::git; use crate::serde_ext; -use crate::storage::Remotes; pub use crypto::PublicKey; pub use doc::{Delegate, Doc}; @@ -175,36 +172,6 @@ impl Deref for Did { } } -/// A stored and verified project. -#[derive(Debug, Clone)] -pub struct Project { - /// The project identifier. - pub id: Id, - /// The latest project identity document. - pub doc: Doc, - /// The project remotes. - pub remotes: Remotes, - /// On-disk file path for this project's repository. - pub path: PathBuf, -} - -impl Project { - pub fn delegate(&mut self, name: String, key: crypto::PublicKey) -> bool { - self.doc.delegate(Delegate { - name, - id: Did::from(key), - }) - } -} - -impl Deref for Project { - type Target = Doc; - - fn deref(&self) -> &Self::Target { - &self.doc - } -} - #[cfg(test)] mod test { use super::*; diff --git a/radicle/src/identity/doc.rs b/radicle/src/identity/doc.rs index ed34f137..1bef65ec 100644 --- a/radicle/src/identity/doc.rs +++ b/radicle/src/identity/doc.rs @@ -102,7 +102,12 @@ impl Doc { } /// Attempt to add a new delegate to the document. Returns `true` if it wasn't there before. - pub fn delegate(&mut self, delegate: Delegate) -> bool { + pub fn delegate(&mut self, name: String, key: crypto::PublicKey) -> bool { + let delegate = Delegate { + name, + id: Did::from(key), + }; + if self.delegates.iter().all(|d| d.id != delegate.id) { self.delegates.push(delegate); return true; @@ -510,7 +515,7 @@ mod test { let repo = storage.repository(&id).unwrap(); // Make a change to the description and sign it. - proj.doc.payload.description += "!"; + proj.payload.description += "!"; proj.sign(&alice) .and_then(|(_, sig)| { proj.update( @@ -524,7 +529,7 @@ mod test { // Add Bob as a delegate, and sign it. proj.delegate("bob".to_owned(), *bob.public_key()); - proj.doc.threshold = 2; + proj.threshold = 2; proj.sign(&alice) .and_then(|(_, sig)| { proj.update( @@ -552,7 +557,7 @@ mod test { .unwrap(); // Update description again with signatures by Eve and Bob. - proj.doc.payload.description += "?"; + proj.payload.description += "?"; let (current, head) = proj .sign(&bob) .and_then(|(_, bob_sig)| { @@ -579,7 +584,7 @@ mod test { assert_eq!(identity.root, id); assert_eq!(identity.current, current); assert_eq!(identity.head, head); - assert_eq!(identity.doc, proj.doc); + assert_eq!(identity.doc, proj); let proj = storage.get(alice.public_key(), &id).unwrap().unwrap(); assert_eq!(proj.description, "Acme's repository!?"); diff --git a/radicle/src/rad.rs b/radicle/src/rad.rs index 2fc84a39..a9ea1139 100644 --- a/radicle/src/rad.rs +++ b/radicle/src/rad.rs @@ -58,6 +58,7 @@ pub fn init<'r, G: Signer, S: storage::WriteStorage<'r>>( .verified()?; let (id, _, project) = doc.create(pk, "Initialize Radicle", storage)?; + let url = storage.url(&id); git::set_upstream( repo, @@ -69,7 +70,7 @@ pub fn init<'r, G: Signer, S: storage::WriteStorage<'r>>( // TODO: Note that you'll likely want to use `RemoteCallbacks` and set // `push_update_reference` to test whether all the references were pushed // successfully. - git::configure_remote(repo, REMOTE_NAME, pk, project.path())?.push::<&str>( + git::configure_remote(repo, REMOTE_NAME, pk, &url)?.push::<&str>( &[&format!( "{}:{}", &git::refs::workdir::branch(&default_branch), @@ -120,14 +121,11 @@ pub fn fork_remote<'r, G: Signer, S: storage::WriteStorage<'r>>( let raw = repository.raw(); let remote_head = raw - .find_reference(&git::refs::storage::branch( - remote, - &project.doc.default_branch, - ))? + .find_reference(&git::refs::storage::branch(remote, &project.default_branch))? .target() .ok_or(ForkError::InvalidReference)?; raw.reference( - &git::refs::storage::branch(me, &project.doc.default_branch), + &git::refs::storage::branch(me, &project.default_branch), remote_head, false, &format!("creating default branch for {me}"), @@ -274,17 +272,14 @@ pub fn checkout, S: storage::ReadStorage>( .ok_or_else(|| CheckoutError::NotFound(proj.clone()))?; let mut opts = git2::RepositoryInitOptions::new(); - opts.no_reinit(true).description(&project.doc.description); + opts.no_reinit(true).description(&project.description); let repo = git2::Repository::init_opts(path, &opts)?; - let default_branch = project.doc.default_branch.as_str(); + let default_branch = project.default_branch.as_str(); + let url = storage.url(proj); // Configure and fetch all refs from remote. - git::configure_remote(&repo, REMOTE_NAME, remote, &project.path)?.fetch::<&str>( - &[], - None, - None, - )?; + git::configure_remote(&repo, REMOTE_NAME, remote, &url)?.fetch::<&str>(&[], None, None)?; { // Setup default branch. @@ -306,6 +301,8 @@ pub fn checkout, S: storage::ReadStorage>( #[cfg(test)] mod tests { + use std::collections::HashMap; + use super::*; use crate::git::fmt::refname; use crate::identity::{Delegate, Did}; @@ -332,14 +329,20 @@ mod tests { .unwrap(); let project = storage.get(&public_key, &proj).unwrap().unwrap(); + let remotes: HashMap<_, _> = storage + .repository(&proj) + .unwrap() + .remotes() + .unwrap() + .collect::>() + .unwrap(); - assert_eq!(project.remotes[&public_key].refs, refs); - assert_eq!(project.id, proj); - assert_eq!(project.doc.name, "acme"); - assert_eq!(project.doc.description, "Acme's repo"); - assert_eq!(project.doc.default_branch, BranchName::from("master")); + assert_eq!(remotes[&public_key].refs, refs); + assert_eq!(project.name, "acme"); + assert_eq!(project.description, "Acme's repo"); + assert_eq!(project.default_branch, BranchName::from("master")); assert_eq!( - project.doc.delegates.first(), + project.delegates.first(), &Delegate { name: String::from("anonymous"), id: Did::from(public_key), diff --git a/radicle/src/storage.rs b/radicle/src/storage.rs index 189a372b..b8b968ce 100644 --- a/radicle/src/storage.rs +++ b/radicle/src/storage.rs @@ -18,7 +18,7 @@ use crate::git::ext as git_ext; use crate::git::Url; use crate::git::{RefError, RefStr, RefString}; use crate::identity; -use crate::identity::{Id, IdError, Project}; +use crate::identity::{Id, IdError}; use crate::storage::git::IdentityError; use crate::storage::refs::Refs; @@ -216,8 +216,9 @@ impl Remote { } pub trait ReadStorage { - fn url(&self) -> Url; - fn get(&self, remote: &RemoteId, proj: &Id) -> Result, Error>; + fn path(&self) -> &Path; + fn url(&self, proj: &Id) -> Url; + fn get(&self, remote: &RemoteId, proj: &Id) -> Result>, Error>; fn inventory(&self) -> Result; } @@ -254,7 +255,7 @@ pub trait ReadRepository<'r> { fn remote(&self, remote: &RemoteId) -> Result, refs::Error>; fn remotes(&'r self) -> Result; /// Return the project associated with this repository. - fn project(&self) -> Result; + fn project(&self) -> Result, Error>; fn project_identity(&self) -> Result<(Oid, identity::Doc), IdentityError>; } @@ -268,15 +269,19 @@ where T: Deref, S: ReadStorage + 'static, { - fn url(&self) -> Url { - self.deref().url() + fn path(&self) -> &Path { + self.deref().path() + } + + fn url(&self, proj: &Id) -> Url { + self.deref().url(proj) } fn inventory(&self) -> Result { self.deref().inventory() } - fn get(&self, remote: &RemoteId, proj: &Id) -> Result, Error> { + fn get(&self, remote: &RemoteId, proj: &Id) -> Result>, Error> { self.deref().get(remote, proj) } } diff --git a/radicle/src/storage/git.rs b/radicle/src/storage/git.rs index eeab3691..0cc3383f 100644 --- a/radicle/src/storage/git.rs +++ b/radicle/src/storage/git.rs @@ -8,7 +8,7 @@ use once_cell::sync::Lazy; use crate::crypto::{Signer, Unverified, Verified}; use crate::git; use crate::identity; -use crate::identity::{Doc, Id, Project}; +use crate::identity::{Doc, Id}; use crate::storage::refs; use crate::storage::refs::{Refs, SignedRefs}; use crate::storage::{ @@ -50,37 +50,27 @@ impl fmt::Debug for Storage { } impl ReadStorage for Storage { - fn url(&self) -> Url { + fn path(&self) -> &Path { + self.path.as_path() + } + + fn url(&self, proj: &Id) -> Url { + let path = self.path().join(proj.to_string()); + Url { scheme: git_url::Scheme::File, - host: None, - path: self.path.to_string_lossy().to_string().into(), - ..Url::default() + path: path.to_string_lossy().to_string().into(), + + ..git::Url::default() } } - fn get(&self, remote: &RemoteId, proj: &Id) -> Result, Error> { + fn get(&self, remote: &RemoteId, proj: &Id) -> Result>, Error> { // TODO: Don't create a repo here if it doesn't exist? // Perhaps for checking we could have a `contains` method? - let repo = self.repository(proj)?; - - if let Some(doc) = repo.identity_of(remote)? { - let remotes = repo.remotes()?.collect::>()?; - let path = repo.path().to_path_buf(); - - // TODO: We should check that there is at least one remote, which is - // the one of the local user, otherwise it means the project is in - // an corrupted state. - - Ok(Some(Project { - id: proj.clone(), - doc, - remotes, - path, - })) - } else { - Ok(None) - } + self.repository(proj)? + .identity_of(remote) + .map_err(Error::from) } fn inventory(&self) -> Result { @@ -445,7 +435,7 @@ impl<'r> ReadRepository<'r> for Repository { Ok(Box::new(iter)) } - fn project(&self) -> Result { + fn project(&self) -> Result, Error> { todo!() } diff --git a/radicle/src/test/arbitrary.rs b/radicle/src/test/arbitrary.rs index 2e24d9d6..779ffc03 100644 --- a/radicle/src/test/arbitrary.rs +++ b/radicle/src/test/arbitrary.rs @@ -2,7 +2,6 @@ use std::collections::{BTreeMap, HashSet}; use std::hash::Hash; use std::iter; use std::ops::RangeBounds; -use std::path::PathBuf; use nonempty::NonEmpty; use quickcheck::Arbitrary; @@ -12,7 +11,7 @@ use crate::crypto; use crate::crypto::{KeyPair, PublicKey, Seed, Signer, Unverified, Verified}; use crate::git; use crate::hash; -use crate::identity::{doc::Delegate, doc::Doc, Did, Id, Project}; +use crate::identity::{doc::Delegate, doc::Doc, Did, Id}; use crate::storage; use crate::storage::refs::{Refs, SignedRefs}; use crate::test::signer::MockSigner; @@ -67,23 +66,6 @@ impl Arbitrary for storage::Remotes { } } -impl Arbitrary for Project { - fn arbitrary(g: &mut quickcheck::Gen) -> Self { - let doc = Doc::::arbitrary(g); - let (oid, _) = doc.encode().unwrap(); - let id = Id::from(oid); - let remotes = storage::Remotes::arbitrary(g); - let path = PathBuf::arbitrary(g); - - Self { - id, - doc, - remotes, - path, - } - } -} - impl Arbitrary for Did { fn arbitrary(g: &mut quickcheck::Gen) -> Self { Self::from(PublicKey::arbitrary(g)) diff --git a/radicle/src/test/storage.rs b/radicle/src/test/storage.rs index a1bd6975..7afe7898 100644 --- a/radicle/src/test/storage.rs +++ b/radicle/src/test/storage.rs @@ -1,29 +1,42 @@ +use std::collections::HashMap; +use std::path::{Path, PathBuf}; + use git_url::Url; use crate::crypto::{Signer, Verified}; -use crate::identity::{Id, Project}; +use crate::identity::doc::Doc; +use crate::identity::Id; pub use crate::storage::*; #[derive(Clone, Debug)] pub struct MockStorage { - pub inventory: Vec, + pub path: PathBuf, + pub inventory: HashMap>, } impl MockStorage { - pub fn new(inventory: Vec) -> Self { - Self { inventory } + pub fn new(inventory: Vec<(Id, Doc)>) -> Self { + Self { + path: PathBuf::default(), + inventory: inventory.into_iter().collect(), + } } pub fn empty() -> Self { Self { - inventory: Vec::new(), + path: PathBuf::default(), + inventory: HashMap::new(), } } } impl ReadStorage for MockStorage { - fn url(&self) -> Url { + fn path(&self) -> &Path { + self.path.as_path() + } + + fn url(&self, _proj: &Id) -> Url { Url { scheme: git_url::Scheme::Radicle, host: Some("mock".to_string()), @@ -31,21 +44,12 @@ impl ReadStorage for MockStorage { } } - fn get(&self, _remote: &RemoteId, proj: &Id) -> Result, Error> { - if let Some(proj) = self.inventory.iter().find(|p| p.id == *proj) { - return Ok(Some(proj.clone())); - } - Ok(None) + fn get(&self, _remote: &RemoteId, proj: &Id) -> Result>, Error> { + Ok(self.inventory.get(proj).cloned()) } fn inventory(&self) -> Result { - let inventory = self - .inventory - .iter() - .map(|proj| proj.id.clone()) - .collect::>(); - - Ok(inventory) + Ok(self.inventory.keys().cloned().collect::>()) } } @@ -122,7 +126,7 @@ impl ReadRepository<'_> for MockRepository { todo!() } - fn project(&self) -> Result { + fn project(&self) -> Result, Error> { todo!() }