From c618c3e44334ea85d725816b2ae5dcb8fcf62687 Mon Sep 17 00:00:00 2001 From: Fintan Halpenny Date: Thu, 30 Mar 2023 16:09:13 +0100 Subject: [PATCH] node: remove Namespaces::One The One variant caused many paint points for fetching logic, where it was not necessary. It was only constructed in one place, which could be replaced by using the variant that holds a set of keys. Remove the Namespaces::One variant and rename Namespaces::Many to Namespaces::Trusted. Signed-off-by: Fintan Halpenny X-Clacks-Overhead: GNU Terry Pratchett --- radicle-node/src/service.rs | 14 +++----------- radicle-node/src/service/tracking.rs | 2 +- radicle-node/src/test/simulator.rs | 3 +-- radicle-node/src/worker/fetch.rs | 12 ++++-------- radicle-node/src/worker/fetch/refspecs.rs | 13 ++----------- radicle/src/storage.rs | 12 +++++------- 6 files changed, 16 insertions(+), 40 deletions(-) diff --git a/radicle-node/src/service.rs b/radicle-node/src/service.rs index 515b7a85..0597b65c 100644 --- a/radicle-node/src/service.rs +++ b/radicle-node/src/service.rs @@ -516,7 +516,7 @@ where resp.send(untracked).ok(); } Command::AnnounceRefs(id) => { - if let Err(err) = self.announce_refs(id, &Namespaces::One(self.node_id())) { + if let Err(err) = self.announce_refs(id, &Namespaces::from_iter([self.node_id()])) { error!("Error announcing refs: {}", err); } } @@ -1004,7 +1004,7 @@ where tracking::Scope::Trusted => { match self.tracking.namespaces_for(&self.storage, &message.rid) { Ok(Namespaces::All) => Ok(true), - Ok(Namespaces::Many(mut trusted)) => { + Ok(Namespaces::Trusted(mut trusted)) => { // Get the set of trusted nodes except self. trusted.remove(&self.node_id()); @@ -1014,9 +1014,6 @@ where .iter() .any(|(pub_key, _refs)| trusted.contains(pub_key))) } - Ok(Namespaces::One(key)) => { - Ok(message.refs.iter().any(|(pub_key, _refs)| pub_key == &key)) - } Err(NamespacesError::NoTrusted { rid }) => { debug!(target: "service", "No trusted nodes to fetch {}", &rid); Ok(false) @@ -1272,12 +1269,7 @@ where } } } - Namespaces::One(pk) => refs - .push((*pk, repo.remote(pk)?.refs.unverified())) - // SAFETY: `REF_REMOTE_LIMIT` is greater than 1, thus the bounded vec can hold at least - // one remote. - .unwrap(), - Namespaces::Many(trusted) => { + Namespaces::Trusted(trusted) => { for remote_id in trusted.iter() { if refs .push((*remote_id, repo.remote(remote_id)?.refs.unverified())) diff --git a/radicle-node/src/service/tracking.rs b/radicle-node/src/service/tracking.rs index a26f4a0f..75f3b42c 100644 --- a/radicle-node/src/service/tracking.rs +++ b/radicle-node/src/service/tracking.rs @@ -131,7 +131,7 @@ impl Config { // trusted and delegate remotes. Ok(Namespaces::All) } else { - Ok(Namespaces::Many(trusted)) + Ok(Namespaces::Trusted(trusted)) } } }, diff --git a/radicle-node/src/test/simulator.rs b/radicle-node/src/test/simulator.rs index c06388d4..e761614b 100644 --- a/radicle-node/src/test/simulator.rs +++ b/radicle-node/src/test/simulator.rs @@ -670,8 +670,7 @@ fn fetch( ) -> Result, radicle::storage::FetchError> { let namespace = match namespaces.into() { Namespaces::All => None, - Namespaces::One(ns) => Some(ns), - Namespaces::Many(trusted) => trusted.into_iter().next(), + Namespaces::Trusted(trusted) => trusted.into_iter().next(), }; let mut updates = Vec::new(); let mut callbacks = git::RemoteCallbacks::new(); diff --git a/radicle-node/src/worker/fetch.rs b/radicle-node/src/worker/fetch.rs index 2f17c0e3..2b496a56 100644 --- a/radicle-node/src/worker/fetch.rs +++ b/radicle-node/src/worker/fetch.rs @@ -132,11 +132,7 @@ impl<'a> StagingPhaseInitial<'a> { let doc = doc.verified()?; let mut trusted = match self.namespaces.clone() { Namespaces::All => HashSet::new(), - // TODO(finto): this is one of those cases where - // having the `One` variant doesn't make any - // sense. - Namespaces::One(pk) => [pk].into_iter().collect(), - Namespaces::Many(trusted) => trusted, + Namespaces::Trusted(trusted) => trusted, }; let delegates = doc.delegates.map(PublicKey::from); trusted.extend(delegates); @@ -148,13 +144,13 @@ impl<'a> StagingPhaseInitial<'a> { // Nb. Namespaces::One is not constructed in // namespaces_for so it's safe to just bundle this // with Namespaces::All - Namespaces::One(_) | Namespaces::All => { + Namespaces::All => { let mut trusted = repo.remote_ids()?.collect::, _>>()?; trusted.extend(repo.delegates()?.map(PublicKey::from)); trusted } - Namespaces::Many(trusted) => trusted, + Namespaces::Trusted(trusted) => trusted, } } }; @@ -202,7 +198,7 @@ impl<'a> StagingPhaseFinal<'a> { /// references. pub fn refspecs(&self) -> Vec> { match self.repo { - StagedRepository::Cloning(_) => Namespaces::Many(self.trusted.clone()).as_refspecs(), + StagedRepository::Cloning(_) => Namespaces::Trusted(self.trusted.clone()).as_refspecs(), StagedRepository::Fetching(_) => { self.remotes().fold(Vec::new(), |mut specs, remote| { specs.extend(remote.as_refspecs()); diff --git a/radicle-node/src/worker/fetch/refspecs.rs b/radicle-node/src/worker/fetch/refspecs.rs index 5d41e043..57518781 100644 --- a/radicle-node/src/worker/fetch/refspecs.rs +++ b/radicle-node/src/worker/fetch/refspecs.rs @@ -54,8 +54,7 @@ impl AsRefspecs for SpecialRefs { }, ] } - Namespaces::One(pk) => rad_refs(pk), - Namespaces::Many(pks) => pks.iter().flat_map(rad_refs).collect(), + Namespaces::Trusted(pks) => pks.iter().flat_map(rad_refs).collect(), } } @@ -106,15 +105,7 @@ impl AsRefspecs for Namespaces { dst: (*storage::git::NAMESPACES_GLOB).clone(), force: false, }], - Namespaces::One(pk) => { - let ns = pk.to_namespace().with_pattern(git::refspec::STAR); - vec![Refspec { - src: ns.clone(), - dst: ns, - force: false, - }] - } - Namespaces::Many(pks) => pks + Namespaces::Trusted(pks) => pks .iter() .map(|pk| { let ns = pk.to_namespace().with_pattern(git::refspec::STAR); diff --git a/radicle/src/storage.rs b/radicle/src/storage.rs index c1180f40..817aa4c5 100644 --- a/radicle/src/storage.rs +++ b/radicle/src/storage.rs @@ -34,15 +34,13 @@ pub enum Namespaces { /// All namespaces. #[default] All, - /// A single namespace, by public key. - One(PublicKey), - /// Many namespaces, by public keys. - Many(HashSet), + /// The trusted set of namespaces. + Trusted(HashSet), } -impl From for Namespaces { - fn from(pk: PublicKey) -> Self { - Self::One(pk) +impl FromIterator for Namespaces { + fn from_iter>(iter: T) -> Self { + Self::Trusted(iter.into_iter().collect()) } }