From 27e39c0df5f1b624710874fa55ac0b26c8f2b64e Mon Sep 17 00:00:00 2001 From: Alexis Sellier Date: Tue, 27 Sep 2022 14:13:20 +0200 Subject: [PATCH] Make `Id` a `Copy` --- radicle-node/src/control.rs | 2 +- radicle-node/src/service.rs | 18 ++++++++--------- radicle-node/src/service/reactor.rs | 2 +- radicle-node/src/test/tests.rs | 26 ++++++++++++------------- radicle-tools/src/rad-push.rs | 2 +- radicle/src/identity/project.rs | 25 ++++++++++++------------ radicle/src/identity/project/id.rs | 2 +- radicle/src/rad.rs | 30 ++++++++++++++--------------- radicle/src/storage.rs | 12 ++++++------ radicle/src/storage/git.rs | 28 +++++++++++++-------------- radicle/src/test/storage.rs | 8 ++++---- 11 files changed, 78 insertions(+), 77 deletions(-) diff --git a/radicle-node/src/control.rs b/radicle-node/src/control.rs index f687de52..f9856831 100644 --- a/radicle-node/src/control.rs +++ b/radicle-node/src/control.rs @@ -108,7 +108,7 @@ fn drain(stream: &UnixStream, handle: &H) -> Result<(), DrainError> { } fn fetch(id: Id, mut writer: W, handle: &H) -> Result<(), DrainError> { - match handle.fetch(id.clone()) { + match handle.fetch(id) { Err(e) => { return Err(DrainError::Client(e)); } diff --git a/radicle-node/src/service.rs b/radicle-node/src/service.rs index 4d57f5b8..c208629c 100644 --- a/radicle-node/src/service.rs +++ b/radicle-node/src/service.rs @@ -272,7 +272,7 @@ where } /// 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) } @@ -306,12 +306,12 @@ where &mut self.reactor } - pub fn lookup(&self, id: &Id) -> Lookup { + pub fn lookup(&self, id: Id) -> Lookup { Lookup { local: self.storage.get(&self.node_id(), id).unwrap(), remote: self .routing - .get(id) + .get(&id) .map_or(vec![], |r| r.iter().cloned().collect()), } } @@ -391,7 +391,7 @@ where }; log::debug!("Found {} seeds for {}", seeds.len(), id); - let mut repo = match self.storage.repository(&id) { + let mut repo = match self.storage.repository(id) { Ok(repo) => repo, Err(err) => { log::error!("Error opening repo for {}: {}", id, err); @@ -445,7 +445,7 @@ where } Command::AnnounceRefs(id) => { let node = self.node_id(); - let repo = self.storage.repository(&id).unwrap(); + let repo = self.storage.repository(id).unwrap(); let remote = repo.remote(&node).unwrap(); let peers = self.sessions.negotiated().map(|(_, p)| p); let refs = remote.refs.into(); @@ -689,12 +689,12 @@ where // TODO: Check that we're tracking this user as well. if self.config.is_tracking(&message.id) { // TODO: Check refs to see if we should try to fetch or not. - let updated = self.storage.fetch(&message.id, git).unwrap(); + let updated = self.storage.fetch(message.id, git).unwrap(); let is_updated = !updated.is_empty(); self.reactor.event(Event::RefsFetched { from: git.clone(), - project: message.id.clone(), + project: message.id, updated, }); @@ -747,12 +747,12 @@ where for proj_id in inventory { let inventory = self .routing - .entry(proj_id.clone()) + .entry(*proj_id) .or_insert_with(|| HashSet::with_hasher(self.rng.clone().into())); // TODO: Fire an event on routing update. if inventory.insert(from) && self.config.is_tracking(proj_id) { - self.storage.fetch(proj_id, remote).unwrap(); + self.storage.fetch(*proj_id, remote).unwrap(); } } } diff --git a/radicle-node/src/service/reactor.rs b/radicle-node/src/service/reactor.rs index 7274df35..4bf52fab 100644 --- a/radicle-node/src/service/reactor.rs +++ b/radicle-node/src/service/reactor.rs @@ -95,7 +95,7 @@ impl Reactor { /// Relay a message to interested peers. pub fn relay<'a>(&mut self, msg: Message, peers: impl IntoIterator) { if let Message::RefsAnnouncement { message, .. } = &msg { - let id = message.id.clone(); + let id = message.id; let peers = peers.into_iter().filter(|p| { if let Some(subscribe) = &p.subscribe { subscribe.filter.contains(&id) diff --git a/radicle-node/src/test/tests.rs b/radicle-node/src/test/tests.rs index 1d1e0b08..0936b0a4 100644 --- a/radicle-node/src/test/tests.rs +++ b/radicle-node/src/test/tests.rs @@ -168,7 +168,7 @@ fn test_tracking() { let proj_id: identity::Id = test::arbitrary::gen(1); let (sender, receiver) = chan::bounded(1); - alice.command(Command::Track(proj_id.clone(), sender)); + alice.command(Command::Track(proj_id, sender)); let policy_change = receiver .recv() .map_err(client::handle::Error::from) @@ -177,7 +177,7 @@ fn test_tracking() { assert!(alice.config().is_tracking(&proj_id)); let (sender, receiver) = chan::bounded(1); - alice.command(Command::Untrack(proj_id.clone(), sender)); + alice.command(Command::Untrack(proj_id, sender)); let policy_change = receiver .recv() .map_err(client::handle::Error::from) @@ -405,30 +405,30 @@ fn test_push_and_pull() { // Bob tracks Alice's project. let (sender, _) = chan::bounded(1); - bob.command(service::Command::Track(proj_id.clone(), sender)); + bob.command(service::Command::Track(proj_id, sender)); // Eve tracks Alice's project. let (sender, _) = chan::bounded(1); - eve.command(service::Command::Track(proj_id.clone(), sender)); + eve.command(service::Command::Track(proj_id, sender)); // Neither of them have it in the beginning. - assert!(eve.get(&proj_id).unwrap().is_none()); - assert!(bob.get(&proj_id).unwrap().is_none()); + assert!(eve.get(proj_id).unwrap().is_none()); + assert!(bob.get(proj_id).unwrap().is_none()); // Alice announces her refs. // We now expect Eve to fetch Alice's project from Alice. // Then we expect Bob to fetch Alice's project from Eve. - alice.command(service::Command::AnnounceRefs(proj_id.clone())); + alice.command(service::Command::AnnounceRefs(proj_id)); sim.run_while([&mut alice, &mut bob, &mut eve], |s| !s.is_settled()); assert!(eve .storage() - .get(&alice.node_id(), &proj_id) + .get(&alice.node_id(), proj_id) .unwrap() .is_some()); assert!(bob .storage() - .get(&alice.node_id(), &proj_id) + .get(&alice.node_id(), proj_id) .unwrap() .is_some()); assert_matches!( @@ -455,7 +455,7 @@ fn prop_inventory_exchange_dense() { ] { for id in inv.keys() { routing - .entry(id.clone()) + .entry(*id) .or_insert_with(|| HashSet::with_hasher(rng.clone().into())) .insert(*peer); } @@ -481,16 +481,16 @@ fn prop_inventory_exchange_dense() { for (proj_id, remotes) in &routing { for peer in peers.values() { - let lookup = peer.lookup(proj_id); + let lookup = peer.lookup(*proj_id); if lookup.local.is_some() { - peer.get(proj_id) + peer.get(*proj_id) .expect("There are no errors querying storage") .expect("The project is available locally"); } else { for remote in &lookup.remote { peers[remote] - .get(proj_id) + .get(*proj_id) .expect("There are no errors querying storage") .expect("The project is available remotely"); } diff --git a/radicle-tools/src/rad-push.rs b/radicle-tools/src/rad-push.rs index de4cc3ff..3263cbfe 100644 --- a/radicle-tools/src/rad-push.rs +++ b/radicle-tools/src/rad-push.rs @@ -11,7 +11,7 @@ fn main() -> anyhow::Result<()> { let output = radicle::git::run(&cwd, &["push", "rad"])?; println!("{}", output); - let project = profile.storage.repository(&id)?; + let project = profile.storage.repository(id)?; let sigrefs = profile.storage.sign_refs(&project, &profile.signer)?; profile.node()?.announce_refs(&id)?; diff --git a/radicle/src/identity/project.rs b/radicle/src/identity/project.rs index 7bd4da3d..649e42d8 100644 --- a/radicle/src/identity/project.rs +++ b/radicle/src/identity/project.rs @@ -138,7 +138,7 @@ impl Doc { // let (doc_oid, doc) = self.encode()?; let id = Id::from(doc_oid); - let repo = storage.repository(&id).unwrap(); + let repo = storage.repository(id).unwrap(); let tree = git::write_tree(*PATH, doc.as_slice(), repo.raw())?; let oid = Doc::commit(remote, &tree, msg, &[], repo.raw())?; @@ -379,6 +379,8 @@ pub enum IdentityError { InvalidSignature(PublicKey, crypto::Error), #[error("quorum not reached: {0} signatures for a threshold of {1}")] QuorumNotReached(usize, usize), + #[error("the identity branch was not found")] + NotFound, } #[derive(Clone, Debug, PartialEq, Eq)] @@ -421,7 +423,7 @@ impl Identity { pub fn load( remote: &RemoteId, repo: &R, - ) -> Result>, IdentityError> { + ) -> Result, IdentityError> { if let Some(head) = Doc::::head(remote, repo)? { let mut history = repo.revwalk(head)?.collect::>(); @@ -466,16 +468,16 @@ impl Identity { current = blob.id().into(); } - return Ok(Some(Identity { + return Ok(Identity { root, head, current, revision, doc: trusted, signatures: signatures.into_iter().collect(), - })); + }); } - Ok(None) + Err(IdentityError::NotFound) } } @@ -505,14 +507,14 @@ mod test { fixtures::project(tempdir.path().join("copy"), &storage, &alice).unwrap(); // Bob and Eve fork the project from Alice. - rad::fork_remote(&id, alice.public_key(), &bob, &storage).unwrap(); - rad::fork_remote(&id, alice.public_key(), &eve, &storage).unwrap(); + rad::fork_remote(id, alice.public_key(), &bob, &storage).unwrap(); + rad::fork_remote(id, alice.public_key(), &eve, &storage).unwrap(); // TODO: In some cases we want to get the repo and the project, but don't // want to have to create a repository object twice. Perhaps there should // be a way of getting a project from a repo. - let mut proj = storage.get(alice.public_key(), &id).unwrap().unwrap(); - let repo = storage.repository(&id).unwrap(); + let mut proj = storage.get(alice.public_key(), id).unwrap().unwrap(); + let repo = storage.repository(id).unwrap(); // Make a change to the description and sign it. proj.payload.description += "!"; @@ -575,8 +577,7 @@ mod test { let identity: Identity = Identity::load(alice.public_key(), &repo) .unwrap() - .unwrap() - .verified(id.clone()) + .verified(id) .unwrap(); assert_eq!(identity.signatures.len(), 2); @@ -586,7 +587,7 @@ mod test { assert_eq!(identity.head, head); assert_eq!(identity.doc, proj); - let proj = storage.get(alice.public_key(), &id).unwrap().unwrap(); + let proj = storage.get(alice.public_key(), id).unwrap().unwrap(); assert_eq!(proj.description, "Acme's repository!?"); } diff --git a/radicle/src/identity/project/id.rs b/radicle/src/identity/project/id.rs index 56a6aad1..df636886 100644 --- a/radicle/src/identity/project/id.rs +++ b/radicle/src/identity/project/id.rs @@ -17,7 +17,7 @@ pub enum IdError { Multibase(#[from] multibase::Error), } -#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct Id(git::Oid); impl fmt::Display for Id { diff --git a/radicle/src/rad.rs b/radicle/src/rad.rs index 931dc928..0359b69d 100644 --- a/radicle/src/rad.rs +++ b/radicle/src/rad.rs @@ -100,7 +100,7 @@ pub enum ForkError { /// Create a local tree for an existing project, from an existing remote. pub fn fork_remote( - proj: &Id, + proj: Id, remote: &RemoteId, signer: G, storage: S, @@ -117,7 +117,7 @@ pub fn fork_remote( let me = signer.public_key(); let project = storage .get(remote, proj)? - .ok_or_else(|| ForkError::NotFound(proj.clone()))?; + .ok_or(ForkError::NotFound(proj))?; let repository = storage.repository(proj)?; let raw = repository.raw(); @@ -149,7 +149,7 @@ pub fn fork_remote( } pub fn fork( - proj: &Id, + proj: Id, signer: &G, storage: &S, ) -> Result<(), ForkError> { @@ -208,13 +208,13 @@ pub enum CloneError { } pub fn clone, G: Signer, S: storage::WriteStorage, H: node::Handle>( - proj: &Id, + proj: Id, path: P, signer: &G, storage: &S, handle: &H, ) -> Result { - let _ = handle.fetch(proj)?; + let _ = handle.fetch(&proj)?; let _ = fork(proj, signer, storage)?; let working = checkout(proj, signer.public_key(), path, storage)?; @@ -234,7 +234,7 @@ pub enum CloneUrlError { } pub fn clone_url, G: Signer, S: storage::WriteStorage>( - proj: &Id, + proj: Id, url: &git::Url, path: P, signer: &G, @@ -261,7 +261,7 @@ pub enum CheckoutError { /// Checkout a project from storage as a working copy. /// This effectively does a `git-clone` from storage. pub fn checkout, S: storage::ReadStorage>( - proj: &Id, + proj: Id, remote: &RemoteId, path: P, storage: &S, @@ -270,14 +270,14 @@ pub fn checkout, S: storage::ReadStorage>( // TODO: Look into sharing object databases. let project = storage .get(remote, proj)? - .ok_or_else(|| CheckoutError::NotFound(proj.clone()))?; + .ok_or(CheckoutError::NotFound(proj))?; let mut opts = git2::RepositoryInitOptions::new(); opts.no_reinit(true).description(&project.description); let repo = git2::Repository::init_opts(path, &opts)?; let default_branch = project.default_branch.as_str(); - let url = storage.url(proj); + let url = storage.url(&proj); // Configure and fetch all refs from remote. git::configure_remote(&repo, REMOTE_NAME, remote, &url)?.fetch::<&str>(&[], None, None)?; @@ -341,9 +341,9 @@ mod tests { ) .unwrap(); - let project = storage.get(&public_key, &proj).unwrap().unwrap(); + let project = storage.get(&public_key, proj).unwrap().unwrap(); let remotes: HashMap<_, _> = storage - .repository(&proj) + .repository(proj) .unwrap() .remotes() .unwrap() @@ -385,10 +385,10 @@ mod tests { .unwrap(); // Bob forks it and creates a checkout. - fork(&id, &bob, &storage).unwrap(); - checkout(&id, bob_id, tempdir.path().join("copy"), &storage).unwrap(); + fork(id, &bob, &storage).unwrap(); + checkout(id, bob_id, tempdir.path().join("copy"), &storage).unwrap(); - let bob_remote = storage.repository(&id).unwrap().remote(bob_id).unwrap(); + let bob_remote = storage.repository(id).unwrap().remote(bob_id).unwrap(); assert_eq!( bob_remote.refs.get(&refname!("master")), @@ -414,7 +414,7 @@ mod tests { ) .unwrap(); - let copy = checkout(&id, remote_id, tempdir.path().join("copy"), &storage).unwrap(); + let copy = checkout(id, remote_id, tempdir.path().join("copy"), &storage).unwrap(); assert_eq!( copy.head().unwrap().target(), diff --git a/radicle/src/storage.rs b/radicle/src/storage.rs index fc84b8fc..ee7fe65d 100644 --- a/radicle/src/storage.rs +++ b/radicle/src/storage.rs @@ -218,20 +218,20 @@ impl Remote { pub trait ReadStorage { fn path(&self) -> &Path; fn url(&self, proj: &Id) -> Url; - fn get(&self, remote: &RemoteId, proj: &Id) -> Result>, Error>; + fn get(&self, remote: &RemoteId, proj: Id) -> Result>, Error>; fn inventory(&self) -> Result; } pub trait WriteStorage: ReadStorage { type Repository: WriteRepository; - fn repository(&self, proj: &Id) -> Result; + fn repository(&self, proj: Id) -> Result; fn sign_refs( &self, repository: &Self::Repository, signer: G, ) -> Result, Error>; - fn fetch(&self, proj_id: &Id, remote: &Url) -> Result, FetchError>; + fn fetch(&self, proj_id: Id, remote: &Url) -> Result, FetchError>; } pub trait ReadRepository { @@ -280,7 +280,7 @@ where 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) } } @@ -292,7 +292,7 @@ where { type Repository = S::Repository; - fn repository(&self, proj: &Id) -> Result { + fn repository(&self, proj: Id) -> Result { self.deref().repository(proj) } @@ -304,7 +304,7 @@ where self.deref().sign_refs(repository, signer) } - fn fetch(&self, proj_id: &Id, remote: &Url) -> Result, FetchError> { + fn fetch(&self, proj_id: Id, remote: &Url) -> Result, FetchError> { self.deref().fetch(proj_id, remote) } } diff --git a/radicle/src/storage/git.rs b/radicle/src/storage/git.rs index 7cc95eed..7d3be7c6 100644 --- a/radicle/src/storage/git.rs +++ b/radicle/src/storage/git.rs @@ -66,7 +66,7 @@ impl ReadStorage for Storage { } } - 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? self.repository(proj)? @@ -82,8 +82,8 @@ impl ReadStorage for Storage { impl WriteStorage for Storage { type Repository = Repository; - fn repository(&self, proj: &Id) -> Result { - Repository::open(paths::repository(self, proj)) + fn repository(&self, proj: Id) -> Result { + Repository::open(paths::repository(self, &proj)) } fn sign_refs( @@ -94,7 +94,7 @@ impl WriteStorage for Storage { repository.sign_refs(signer) } - fn fetch(&self, proj_id: &Id, remote: &Url) -> Result, FetchError> { + fn fetch(&self, proj_id: Id, remote: &Url) -> Result, FetchError> { let mut repo = self.repository(proj_id).unwrap(); let mut path = remote.path.clone(); @@ -139,7 +139,7 @@ impl Storage { pub fn inspect(&self) -> Result<(), Error> { for proj in self.projects()? { - let repo = self.repository(&proj)?; + let repo = self.repository(proj)?; for r in repo.raw().references()? { let r = r?; @@ -264,7 +264,7 @@ impl Repository { Ok(()) } - pub fn identity(&self, remote: &RemoteId) -> Result>, IdentityError> { + pub fn identity(&self, remote: &RemoteId) -> Result, IdentityError> { Identity::load(remote, self) } @@ -648,7 +648,7 @@ mod tests { }) .unwrap(); - let project = storage.repository(proj).unwrap(); + let project = storage.repository(*proj).unwrap(); let remotes = project.remotes().unwrap(); // Strip the remote refs of sigrefs so we can compare them. @@ -671,7 +671,7 @@ mod tests { let alice = fixtures::storage(tmp.path().join("alice"), alice_signer).unwrap(); let bob = Storage::open(tmp.path().join("bob")).unwrap(); let inventory = alice.inventory().unwrap(); - let proj = inventory.first().unwrap(); + let proj = *inventory.first().unwrap(); let repo = alice.repository(proj).unwrap(); let remotes = repo.remotes().unwrap().collect::>(); let refname = git::refname!("heads/master"); @@ -682,7 +682,7 @@ mod tests { .unwrap() .fetch(&git::Url { scheme: git_url::Scheme::File, - path: paths::repository(&alice, proj) + path: paths::repository(&alice, &proj) .to_string_lossy() .into_owned() .into(), @@ -734,11 +734,11 @@ mod tests { }; // Have Bob fetch Alice's refs. - let updates = bob.repository(&proj_id).unwrap().fetch(&alice_url).unwrap(); + let updates = bob.repository(proj_id).unwrap().fetch(&alice_url).unwrap(); // Three refs are created: the branch, the signature and the id. assert_eq!(updates.len(), 3); - let alice_proj_storage = alice.repository(&proj_id).unwrap(); + let alice_proj_storage = alice.repository(proj_id).unwrap(); let alice_head = proj_repo.find_commit(alice_head).unwrap(); let alice_head = git::commit(&proj_repo, &alice_head, &refname, "Making changes", "Alice") .unwrap() @@ -747,7 +747,7 @@ mod tests { alice.sign_refs(&alice_proj_storage, &alice_signer).unwrap(); // Have Bob fetch Alice's new commit. - let updates = bob.repository(&proj_id).unwrap().fetch(&alice_url).unwrap(); + let updates = bob.repository(proj_id).unwrap().fetch(&alice_url).unwrap(); // The branch and signature refs are updated. assert_matches!( updates.as_slice(), @@ -755,7 +755,7 @@ mod tests { ); // Bob's storage is updated. - let bob_repo = bob.repository(&proj_id).unwrap(); + let bob_repo = bob.repository(proj_id).unwrap(); let bob_master = bob_repo.reference(alice_id, &refname).unwrap().unwrap(); assert_eq!(bob_master.target().unwrap(), alice_head); @@ -769,7 +769,7 @@ mod tests { let storage = Storage::open(tmp.path()).unwrap(); let proj_id = arbitrary::gen::(1); let alice = *signer.public_key(); - let project = storage.repository(&proj_id).unwrap(); + let project = storage.repository(proj_id).unwrap(); let backend = &project.backend; let sig = git2::Signature::now(&alice.to_string(), "anonymous@radicle.xyz").unwrap(); let head = git::initial_commit(backend, &sig).unwrap(); diff --git a/radicle/src/test/storage.rs b/radicle/src/test/storage.rs index 6c223a78..a4b37568 100644 --- a/radicle/src/test/storage.rs +++ b/radicle/src/test/storage.rs @@ -44,8 +44,8 @@ impl ReadStorage for MockStorage { } } - fn get(&self, _remote: &RemoteId, proj: &Id) -> Result>, Error> { - Ok(self.inventory.get(proj).cloned()) + fn get(&self, _remote: &RemoteId, proj: Id) -> Result>, Error> { + Ok(self.inventory.get(&proj).cloned()) } fn inventory(&self) -> Result { @@ -56,7 +56,7 @@ impl ReadStorage for MockStorage { impl WriteStorage for MockStorage { type Repository = MockRepository; - fn repository(&self, _proj: &Id) -> Result { + fn repository(&self, _proj: Id) -> Result { Ok(MockRepository {}) } @@ -68,7 +68,7 @@ impl WriteStorage for MockStorage { todo!() } - fn fetch(&self, _proj_id: &Id, _remote: &Url) -> Result, FetchError> { + fn fetch(&self, _proj_id: Id, _remote: &Url) -> Result, FetchError> { Ok(vec![]) } }