From a100f1c683c05349866c301b4e243d80bf4d578b Mon Sep 17 00:00:00 2001 From: Alexis Sellier Date: Wed, 7 Dec 2022 15:12:18 +0100 Subject: [PATCH] cli: Implement tracking correctly The tracking command was tracking a project when it should have been tracking peers. We also add the ability to pass an alias when tracking from the CLI. Signed-off-by: Alexis Sellier --- radicle-cli/src/commands/clone.rs | 2 +- radicle-cli/src/commands/track.rs | 70 +++++++++++++++-------- radicle-cli/src/commands/untrack.rs | 2 +- radicle-node/src/client/handle.rs | 32 +++++++++-- radicle-node/src/control.rs | 65 ++++++++++++++++++--- radicle-node/src/service.rs | 44 ++++++++++---- radicle-node/src/test/handle.rs | 20 +++++-- radicle-node/src/tests.rs | 16 +++--- radicle/src/node.rs | 89 +++++++++++++++++++++++------ radicle/src/rad.rs | 2 +- 10 files changed, 257 insertions(+), 85 deletions(-) diff --git a/radicle-cli/src/commands/clone.rs b/radicle-cli/src/commands/clone.rs index 5c4f5dfd..91be6466 100644 --- a/radicle-cli/src/commands/clone.rs +++ b/radicle-cli/src/commands/clone.rs @@ -84,7 +84,7 @@ pub fn clone(id: Id, _interactive: Interactive, ctx: impl term::Context) -> anyh let signer = term::signer(&profile)?; // Track & fetch project. - node.track(&id).context("track")?; + node.track_repo(&id).context("track")?; node.fetch(&id).context("fetch")?; // Create a local fork of the project, under our own id. diff --git a/radicle-cli/src/commands/track.rs b/radicle-cli/src/commands/track.rs index e14c4c7a..22d43e55 100644 --- a/radicle-cli/src/commands/track.rs +++ b/radicle-cli/src/commands/track.rs @@ -1,8 +1,8 @@ use std::ffi::OsString; +use std::str::FromStr; use anyhow::{anyhow, Context as _}; -use radicle::identity::project::Id; use radicle::node::Handle; use radicle::prelude::*; use radicle::storage::WriteStorage; @@ -12,15 +12,16 @@ use crate::terminal::args::{Args, Error, Help}; pub const HELP: Help = Help { name: "track", - description: "Manage project tracking relationships", + description: "Manage project tracking policy", version: env!("CARGO_PKG_VERSION"), usage: r#" Usage - rad track [] [--fetch] + rad track [--fetch] [--alias ] Options + --alias Add an alias to this peer identifier --fetch Fetch the peer's refs into the working copy --verbose, -v Verbose output --help Print help @@ -29,7 +30,8 @@ Options #[derive(Debug)] pub struct Options { - pub id: Option, + pub peer: NodeId, + pub alias: Option, pub fetch: bool, pub verbose: bool, } @@ -39,21 +41,31 @@ impl Args for Options { use lexopt::prelude::*; let mut parser = lexopt::Parser::from_args(args); - let mut id: Option = None; + let mut peer: Option = None; + let mut alias: Option = None; let mut fetch = true; let mut verbose = false; while let Some(arg) = parser.next()? { match arg { + Long("alias") => { + let name = parser.value()?; + let name = name + .to_str() + .to_owned() + .ok_or_else(|| anyhow!("alias specified is not UTF-8"))?; + + alias = Some(name.to_owned()); + } Long("no-fetch") => fetch = false, Long("verbose") | Short('v') => verbose = true, - Value(val) if id.is_none() => { + Value(val) if peer.is_none() => { let val = val.to_string_lossy(); - if let Ok(val) = Id::from_human(&val) { - id = Some(val); + if let Ok(val) = NodeId::from_str(&val) { + peer = Some(val); } else { - return Err(anyhow!("invalid ID '{}'", val)); + return Err(anyhow!("invalid Node ID '{}'", val)); } } Long("help") => { @@ -65,18 +77,24 @@ impl Args for Options { } } - Ok((Options { id, fetch, verbose }, vec![])) + Ok(( + Options { + peer: peer.ok_or_else(|| anyhow!("a peer to track must be supplied"))?, + alias, + fetch, + verbose, + }, + vec![], + )) } } pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { - let id = options - .id - .or_else(|| radicle::rad::cwd().ok().map(|(_, id)| id)) - .context("current directory is not a git repository; please supply an ``")?; + let peer = options.peer; let profile = ctx.profile()?; let storage = &profile.storage; - let Doc { payload, .. } = storage.repository(id)?.project_of(profile.id())?; + let (_, rid) = radicle::rad::cwd().context("this command must be run within a project")?; + let Doc { payload, .. } = storage.repository(rid)?.project_of(profile.id())?; let node = radicle::node::connect(&profile.node())?; term::info!( @@ -85,16 +103,22 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { ); term::blank(); - let tracked = node.track(&id)?; - term::success!( - "Tracking relationship for {} ({}) {}", - term::format::tertiary(&payload.name), - &id.to_human(), - if tracked { "established" } else { "exists" } - ); + let tracked = node.track_node(&peer, options.alias.as_deref())?; + let outcome = if tracked { "established" } else { "exists" }; + + if let Some(alias) = options.alias { + term::success!( + "Tracking relationship with {} ({}) {}", + term::format::tertiary(alias), + peer, + outcome + ); + } else { + term::success!("Tracking relationship with {} {}", peer, outcome); + } if options.fetch { - node.fetch(&id)?; + node.fetch(&rid)?; } Ok(()) diff --git a/radicle-cli/src/commands/untrack.rs b/radicle-cli/src/commands/untrack.rs index b03d3079..cd76ced1 100644 --- a/radicle-cli/src/commands/untrack.rs +++ b/radicle-cli/src/commands/untrack.rs @@ -89,5 +89,5 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { pub fn untrack(id: Id, profile: &Profile) -> anyhow::Result { let node = radicle::node::connect(profile.node())?; - node.untrack(&id).map_err(|e| anyhow!(e)) + node.untrack_repo(&id).map_err(|e| anyhow!(e)) } diff --git a/radicle-node/src/client/handle.rs b/radicle-node/src/client/handle.rs index de6e1670..c8a4acfe 100644 --- a/radicle-node/src/client/handle.rs +++ b/radicle-node/src/client/handle.rs @@ -66,15 +66,31 @@ impl traits::Handle for Handle { receiver.recv().map_err(Error::from) } - fn track(&mut self, id: Id) -> Result { + fn track_node(&mut self, id: NodeId, alias: Option) -> Result { let (sender, receiver) = chan::bounded(1); - self.commands.send(service::Command::Track(id, sender))?; + self.commands + .send(service::Command::TrackNode(id, alias, sender))?; receiver.recv().map_err(Error::from) } - fn untrack(&mut self, id: Id) -> Result { + fn untrack_node(&mut self, id: NodeId) -> Result { let (sender, receiver) = chan::bounded(1); - self.commands.send(service::Command::Untrack(id, sender))?; + self.commands + .send(service::Command::UntrackNode(id, sender))?; + receiver.recv().map_err(Error::from) + } + + fn track_repo(&mut self, id: Id) -> Result { + let (sender, receiver) = chan::bounded(1); + self.commands + .send(service::Command::TrackRepo(id, sender))?; + receiver.recv().map_err(Error::from) + } + + fn untrack_repo(&mut self, id: Id) -> Result { + let (sender, receiver) = chan::bounded(1); + self.commands + .send(service::Command::UntrackRepo(id, sender))?; receiver.recv().map_err(Error::from) } @@ -146,9 +162,13 @@ pub mod traits { fn fetch(&mut self, id: Id) -> Result; /// Start tracking the given project. Doesn't do anything if the project is already /// tracked. - fn track(&mut self, id: Id) -> Result; + fn track_repo(&mut self, id: Id) -> Result; + /// Start tracking the given node. + fn track_node(&mut self, id: NodeId, alias: Option) -> Result; /// Untrack the given project and delete it from storage. - fn untrack(&mut self, id: Id) -> Result; + fn untrack_repo(&mut self, id: Id) -> Result; + /// Untrack the given node. + fn untrack_node(&mut self, id: NodeId) -> Result; /// Notify the client that a project has been updated. fn announce_refs(&mut self, id: Id) -> Result<(), Error>; /// Send a command to the command channel, and wake up the event loop. diff --git a/radicle-node/src/control.rs b/radicle-node/src/control.rs index 3a5c9437..7b79c36b 100644 --- a/radicle-node/src/control.rs +++ b/radicle-node/src/control.rs @@ -83,9 +83,9 @@ fn drain(stream: &UnixStream, handle: &mut H) -> Result<(), DrainErro return Err(DrainError::InvalidCommandArg(arg.to_owned())); } } - Some(("track", arg)) => { + Some(("track-repo", arg)) => { if let Ok(id) = arg.parse() { - match handle.track(id) { + match handle.track_repo(id) { Ok(updated) => { if updated { writeln!(writer, "{}", node::RESPONSE_OK)?; @@ -101,9 +101,50 @@ fn drain(stream: &UnixStream, handle: &mut H) -> Result<(), DrainErro return Err(DrainError::InvalidCommandArg(arg.to_owned())); } } - Some(("untrack", arg)) => { + Some(("untrack-repo", arg)) => { if let Ok(id) = arg.parse() { - match handle.untrack(id) { + match handle.untrack_repo(id) { + Ok(updated) => { + if updated { + writeln!(writer, "{}", node::RESPONSE_OK)?; + } else { + writeln!(writer, "{}", node::RESPONSE_NOOP)?; + } + } + Err(e) => { + return Err(DrainError::Client(e)); + } + } + } else { + return Err(DrainError::InvalidCommandArg(arg.to_owned())); + } + } + Some(("track-node", args)) => { + let (peer, alias) = if let Some((peer, alias)) = args.split_once(' ') { + (peer, Some(alias.to_owned())) + } else { + (args, None) + }; + if let Ok(id) = peer.parse() { + match handle.track_node(id, alias) { + Ok(updated) => { + if updated { + writeln!(writer, "{}", node::RESPONSE_OK)?; + } else { + writeln!(writer, "{}", node::RESPONSE_NOOP)?; + } + } + Err(e) => { + return Err(DrainError::Client(e)); + } + } + } else { + return Err(DrainError::InvalidCommandArg(args.to_owned())); + } + } + Some(("untrack-node", arg)) => { + if let Ok(id) = arg.parse() { + match handle.untrack_node(id) { Ok(updated) => { if updated { writeln!(writer, "{}", node::RESPONSE_OK)?; @@ -214,7 +255,7 @@ mod tests { use super::*; use crate::identity::Id; use crate::node::Handle; - use crate::node::Node; + use crate::node::{Node, NodeId}; use crate::test; #[test] @@ -255,6 +296,7 @@ mod tests { let tmp = tempfile::tempdir().unwrap(); let socket = tmp.path().join("node.sock"); let proj = test::arbitrary::gen::(1); + let peer = test::arbitrary::gen::(1); thread::spawn({ let socket = socket.clone(); @@ -269,9 +311,14 @@ mod tests { } }; - assert!(handle.track(&proj).unwrap()); - assert!(!handle.track(&proj).unwrap()); - assert!(handle.untrack(&proj).unwrap()); - assert!(!handle.untrack(&proj).unwrap()); + assert!(handle.track_repo(&proj).unwrap()); + assert!(!handle.track_repo(&proj).unwrap()); + assert!(handle.untrack_repo(&proj).unwrap()); + assert!(!handle.untrack_repo(&proj).unwrap()); + + assert!(handle.track_node(&peer, Some("alice")).unwrap()); + assert!(!handle.track_node(&peer, Some("alice")).unwrap()); + assert!(handle.untrack_node(&peer).unwrap()); + assert!(!handle.untrack_node(&peer).unwrap()); } } diff --git a/radicle-node/src/service.rs b/radicle-node/src/service.rs index d2e64b67..f86fd33e 100644 --- a/radicle-node/src/service.rs +++ b/radicle-node/src/service.rs @@ -150,9 +150,13 @@ pub enum Command { /// Fetch the given project from the network. Fetch(Id, chan::Sender), /// Track the given project. - Track(Id, chan::Sender), + TrackRepo(Id, chan::Sender), /// Untrack the given project. - Untrack(Id, chan::Sender), + UntrackRepo(Id, chan::Sender), + /// Track the given node. + TrackNode(NodeId, Option, chan::Sender), + /// Untrack the given node. + UntrackNode(NodeId, chan::Sender), /// Query the internal service state. QueryState(Arc, chan::Sender>), } @@ -163,8 +167,10 @@ impl fmt::Debug for Command { Self::AnnounceRefs(id) => write!(f, "AnnounceRefs({})", id), Self::Connect(addr) => write!(f, "Connect({})", addr), Self::Fetch(id, _) => write!(f, "Fetch({})", id), - Self::Track(id, _) => write!(f, "Track({})", id), - Self::Untrack(id, _) => write!(f, "Untrack({})", id), + Self::TrackRepo(id, _) => write!(f, "TrackRepo({})", id), + Self::UntrackRepo(id, _) => write!(f, "UntrackRepo({})", id), + Self::TrackNode(id, _, _) => write!(f, "TrackNode({})", id), + Self::UntrackNode(id, _) => write!(f, "UntrackNode({})", id), Self::QueryState { .. } => write!(f, "QueryState(..)"), } } @@ -290,20 +296,20 @@ where } } - /// Track a project. + /// Track a repository. /// Returns whether or not the tracking policy was updated. - pub fn track(&mut self, id: &Id, scope: tracking::Scope) -> Result { + pub fn track_repo(&mut self, id: &Id, scope: tracking::Scope) -> Result { self.out_of_sync = self.tracking.track_repo(id, scope)?; self.filter.insert(id); Ok(self.out_of_sync) } - /// Untrack a project. + /// Untrack a repository. /// Returns whether or not the tracking policy was updated. /// Note that when untracking, we don't announce anything to the network. This is because by /// simply not announcing it anymore, it will eventually be pruned by nodes. - pub fn untrack(&mut self, id: &Id) -> Result { + pub fn untrack_repo(&mut self, id: &Id) -> Result { // Nb. This is potentially slow if we have lots of projects. We should probably // only re-compute the filter when we've untracked a certain amount of projects // and the filter is really out of date. @@ -491,18 +497,32 @@ where } } } - Command::Track(id, resp) => { + Command::TrackRepo(id, resp) => { let tracked = self - .track(&id, tracking::Scope::All) + .track_repo(&id, tracking::Scope::All) .expect("Service::command: error tracking repository"); resp.send(tracked).ok(); } - Command::Untrack(id, resp) => { + Command::UntrackRepo(id, resp) => { let untracked = self - .untrack(&id) + .untrack_repo(&id) .expect("Service::command: error untracking repository"); resp.send(untracked).ok(); } + Command::TrackNode(id, alias, resp) => { + let tracked = self + .tracking + .track_node(&id, alias.as_deref()) + .expect("Service::command: error tracking node"); + resp.send(tracked).ok(); + } + Command::UntrackNode(id, resp) => { + let untracked = self + .tracking + .untrack_node(&id) + .expect("Service::command: error untracking node"); + resp.send(untracked).ok(); + } Command::AnnounceRefs(id) => { if let Err(err) = self.announce_refs(id) { error!("Error announcing refs: {}", err); diff --git a/radicle-node/src/test/handle.rs b/radicle-node/src/test/handle.rs index fcc8688b..5103e354 100644 --- a/radicle-node/src/test/handle.rs +++ b/radicle-node/src/test/handle.rs @@ -8,11 +8,13 @@ use crate::client::handle::Error; use crate::identity::Id; use crate::service; use crate::service::FetchLookup; +use crate::service::NodeId; #[derive(Default, Clone)] pub struct Handle { pub updates: Arc>>, - pub tracking: HashSet, + pub tracking_repos: HashSet, + pub tracking_nodes: HashSet, } impl traits::Handle for Handle { @@ -24,12 +26,20 @@ impl traits::Handle for Handle { Ok(FetchLookup::NotFound) } - fn track(&mut self, id: Id) -> Result { - Ok(self.tracking.insert(id)) + fn track_repo(&mut self, id: Id) -> Result { + Ok(self.tracking_repos.insert(id)) } - fn untrack(&mut self, id: Id) -> Result { - Ok(self.tracking.remove(&id)) + fn untrack_repo(&mut self, id: Id) -> Result { + Ok(self.tracking_repos.remove(&id)) + } + + fn track_node(&mut self, id: NodeId, _alias: Option) -> Result { + Ok(self.tracking_nodes.insert(id)) + } + + fn untrack_node(&mut self, id: NodeId) -> Result { + Ok(self.tracking_nodes.remove(&id)) } fn announce_refs(&mut self, id: Id) -> Result<(), Error> { diff --git a/radicle-node/src/tests.rs b/radicle-node/src/tests.rs index 77ae300b..1e39eb31 100644 --- a/radicle-node/src/tests.rs +++ b/radicle-node/src/tests.rs @@ -353,7 +353,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, sender)); + alice.command(Command::TrackRepo(proj_id, sender)); let policy_change = receiver .recv() .map_err(client::handle::Error::from) @@ -362,7 +362,7 @@ fn test_tracking() { assert!(alice.tracking().is_repo_tracked(&proj_id).unwrap()); let (sender, receiver) = chan::bounded(1); - alice.command(Command::Untrack(proj_id, sender)); + alice.command(Command::UntrackRepo(proj_id, sender)); let policy_change = receiver .recv() .map_err(client::handle::Error::from) @@ -556,9 +556,9 @@ fn test_refs_announcement_relay() { }; let bob_inv = bob.inventory().unwrap(); - alice.track(&bob_inv[0], tracking::Scope::All).unwrap(); - alice.track(&bob_inv[1], tracking::Scope::All).unwrap(); - alice.track(&bob_inv[2], tracking::Scope::All).unwrap(); + alice.track_repo(&bob_inv[0], tracking::Scope::All).unwrap(); + alice.track_repo(&bob_inv[1], tracking::Scope::All).unwrap(); + alice.track_repo(&bob_inv[2], tracking::Scope::All).unwrap(); alice.connect_to(&bob); alice.connect_to(&eve); alice.receive(&eve.addr(), Message::Subscribe(Subscribe::all())); @@ -598,7 +598,7 @@ fn test_refs_announcement_no_subscribe() { let eve = Peer::new("eve", [9, 9, 9, 9], MockStorage::empty()); let id = arbitrary::gen(1); - alice.track(&id, tracking::Scope::All).unwrap(); + alice.track_repo(&id, tracking::Scope::All).unwrap(); alice.connect_to(&bob); alice.connect_to(&eve); alice.receive(&bob.addr(), bob.refs_announcement(id)); @@ -857,11 +857,11 @@ fn test_push_and_pull() { // Bob tracks Alice's project. let (sender, _) = chan::bounded(1); - bob.command(service::Command::Track(proj_id, sender)); + bob.command(service::Command::TrackRepo(proj_id, sender)); // Eve tracks Alice's project. let (sender, _) = chan::bounded(1); - eve.command(service::Command::Track(proj_id, sender)); + eve.command(service::Command::TrackRepo(proj_id, sender)); let mut sim = Simulation::new( LocalTime::now(), diff --git a/radicle/src/node.rs b/radicle/src/node.rs index 5fefbac8..5818754f 100644 --- a/radicle/src/node.rs +++ b/radicle/src/node.rs @@ -1,6 +1,5 @@ mod features; -use std::fmt; use std::io; use std::io::{BufRead, BufReader, Write}; use std::os::unix::net::UnixStream; @@ -31,11 +30,15 @@ pub enum Error { pub trait Handle { /// Fetch a project from the network. Fails if the project isn't tracked. fn fetch(&self, id: &Id) -> Result<(), Error>; - /// Start tracking the given project. Doesn't do anything if the project is already - /// tracked. - fn track(&self, id: &Id) -> Result; - /// Untrack the given project and delete it from storage. - fn untrack(&self, id: &Id) -> Result; + /// Start tracking the given node. If the node is already tracked, + /// updates the alias if necessary. + fn track_node(&self, id: &NodeId, alias: Option<&str>) -> Result; + /// Start tracking the given repository. + fn track_repo(&self, id: &Id) -> Result; + /// Untrack the given node. + fn untrack_node(&self, id: &NodeId) -> Result; + /// Untrack the given repository and delete it from storage. + fn untrack_repo(&self, id: &Id) -> Result; /// Notify the network that we have new refs. fn announce_refs(&self, id: &Id) -> Result<(), Error>; /// Ask the node to shutdown. @@ -60,12 +63,17 @@ impl Node { } /// Call a command on the node. - pub fn call( + pub fn call( &self, cmd: &str, - arg: &A, + args: &[A], ) -> Result> + '_, io::Error> { - writeln!(&self.stream, "{cmd} {arg}")?; + let args = args + .iter() + .map(ToString::to_string) + .collect::>() + .join(" "); + writeln!(&self.stream, "{cmd} {args}")?; Ok(BufReader::new(&self.stream).lines()) } @@ -73,16 +81,23 @@ impl Node { impl Handle for Node { fn fetch(&self, id: &Id) -> Result<(), Error> { - for line in self.call("fetch", id)? { + for line in self.call("fetch", &[id])? { let line = line?; log::debug!("node: {}", line); } Ok(()) } - fn track(&self, id: &Id) -> Result { - let mut line = self.call("track", id)?; - let line = line.next().ok_or(Error::EmptyResponse { cmd: "track" })??; + fn track_node(&self, id: &NodeId, alias: Option<&str>) -> Result { + let id = id.to_human(); + let mut line = if let Some(alias) = alias { + self.call("track-node", &[id.as_str(), alias]) + } else { + self.call("track-node", &[id.as_str()]) + }?; + let line = line + .next() + .ok_or(Error::EmptyResponse { cmd: "track-node" })??; log::debug!("node: {}", line); @@ -90,17 +105,17 @@ impl Handle for Node { RESPONSE_OK => Ok(true), RESPONSE_NOOP => Ok(false), _ => Err(Error::InvalidResponse { - cmd: "track", + cmd: "track-node", response: line, }), } } - fn untrack(&self, id: &Id) -> Result { - let mut line = self.call("untrack", id)?; + fn track_repo(&self, id: &Id) -> Result { + let mut line = self.call("track-repo", &[id])?; let line = line .next() - .ok_or(Error::EmptyResponse { cmd: "untrack" })??; + .ok_or(Error::EmptyResponse { cmd: "track-repo" })??; log::debug!("node: {}", line); @@ -108,14 +123,50 @@ impl Handle for Node { RESPONSE_OK => Ok(true), RESPONSE_NOOP => Ok(false), _ => Err(Error::InvalidResponse { - cmd: "untrack", + cmd: "track-repo", + response: line, + }), + } + } + + fn untrack_node(&self, id: &NodeId) -> Result { + let mut line = self.call("untrack-node", &[id])?; + let line = line.next().ok_or(Error::EmptyResponse { + cmd: "untrack-node", + })??; + + log::debug!("node: {}", line); + + match line.as_str() { + RESPONSE_OK => Ok(true), + RESPONSE_NOOP => Ok(false), + _ => Err(Error::InvalidResponse { + cmd: "untrack-node", + response: line, + }), + } + } + + fn untrack_repo(&self, id: &Id) -> Result { + let mut line = self.call("untrack-repo", &[id])?; + let line = line.next().ok_or(Error::EmptyResponse { + cmd: "untrack-repo", + })??; + + log::debug!("node: {}", line); + + match line.as_str() { + RESPONSE_OK => Ok(true), + RESPONSE_NOOP => Ok(false), + _ => Err(Error::InvalidResponse { + cmd: "untrack-repo", response: line, }), } } fn announce_refs(&self, id: &Id) -> Result<(), Error> { - for line in self.call("announce-refs", id)? { + for line in self.call("announce-refs", &[id])? { let line = line?; log::debug!("node: {}", line); } diff --git a/radicle/src/rad.rs b/radicle/src/rad.rs index b33248fb..4351f298 100644 --- a/radicle/src/rad.rs +++ b/radicle/src/rad.rs @@ -199,7 +199,7 @@ pub fn clone, G: Signer, S: storage::WriteStorage, H: node::Handl storage: &S, handle: &H, ) -> Result { - let _ = handle.track(&proj)?; + let _ = handle.track_repo(&proj)?; let _ = handle.fetch(&proj)?; let _ = fork(proj, signer, storage)?; let working = checkout(proj, signer.public_key(), path, storage)?;