From 1f06f5340f81ea8815dae06cb9828da649518cd8 Mon Sep 17 00:00:00 2001 From: Alexis Sellier Date: Sun, 22 Jan 2023 17:24:36 +0100 Subject: [PATCH] Make `Id` to string conversion more explicit Two types of conversion: `canonical`, which is just the base58 encoding, and `urn` which is like canonical, but includes `rad:`. --- radicle-cli/src/commands/init.rs | 2 +- radicle-cli/src/commands/inspect.rs | 4 +- radicle-cli/src/commands/ls.rs | 2 +- radicle-cli/src/commands/rm.rs | 4 +- radicle-cli/src/commands/untrack.rs | 6 +- radicle-tools/src/rad-clone.rs | 3 +- radicle/src/identity.rs | 2 +- radicle/src/identity/doc.rs | 5 +- radicle/src/identity/doc/id.rs | 37 ++++-- radicle/src/node.rs | 8 +- radicle/src/sql.rs | 4 +- radicle/src/storage/git.rs | 117 +----------------- .../src/storage/git/transport/local/url.rs | 31 +++-- .../src/storage/git/transport/remote/mock.rs | 2 +- .../src/storage/git/transport/remote/url.rs | 33 +++-- 15 files changed, 88 insertions(+), 172 deletions(-) diff --git a/radicle-cli/src/commands/init.rs b/radicle-cli/src/commands/init.rs index 420dcd39..55e1e554 100644 --- a/radicle-cli/src/commands/init.rs +++ b/radicle-cli/src/commands/init.rs @@ -232,7 +232,7 @@ pub fn init(options: Options, profile: &profile::Profile) -> anyhow::Result<()> term::blank(); term::info!( "Your project id is {}. You can show it any time by running:", - term::format::highlight(id) + term::format::highlight(id.urn()) ); term::indented(term::format::secondary("rad .")); diff --git a/radicle-cli/src/commands/inspect.rs b/radicle-cli/src/commands/inspect.rs index 23506b29..af8f7fbf 100644 --- a/radicle-cli/src/commands/inspect.rs +++ b/radicle-cli/src/commands/inspect.rs @@ -130,7 +130,7 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { let path = profile .paths() .storage() - .join(id.to_human()) + .join(id.urn()) .join("refs") .join("namespaces"); @@ -195,7 +195,7 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { println!(); } } else if options.id_only { - term::info!("{}", term::format::highlight(id.to_human())); + term::info!("{}", term::format::highlight(id.urn())); } else { term::info!("{}", term::format::highlight(id)); } diff --git a/radicle-cli/src/commands/ls.rs b/radicle-cli/src/commands/ls.rs index 0bc5b21d..df88d143 100644 --- a/radicle-cli/src/commands/ls.rs +++ b/radicle-cli/src/commands/ls.rs @@ -53,7 +53,7 @@ pub fn run(_options: Options, ctx: impl term::Context) -> anyhow::Result<()> { let head = term::format::oid(head); table.push([ term::format::bold(proj.name()), - term::format::tertiary(id), + term::format::tertiary(id.urn()), term::format::secondary(head), term::format::italic(proj.description()), ]); diff --git a/radicle-cli/src/commands/rm.rs b/radicle-cli/src/commands/rm.rs index 96e4691d..d5d0fd89 100644 --- a/radicle-cli/src/commands/rm.rs +++ b/radicle-cli/src/commands/rm.rs @@ -81,12 +81,12 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { let id = options.id; if let Ok(Some(_)) = storage.get(signer.public_key(), id.to_owned()) { - let namespace = profile.paths().storage().join(id.to_human()); + let namespace = profile.paths().storage().join(id.urn()); if !options.confirm || term::confirm(format!( "Are you sure you would like to delete {}?", - term::format::dim(id.to_human()) + term::format::dim(id.urn()) )) { rad_untrack::untrack(id.to_owned(), &profile)?; diff --git a/radicle-cli/src/commands/untrack.rs b/radicle-cli/src/commands/untrack.rs index 83dd80a6..5a855876 100644 --- a/radicle-cli/src/commands/untrack.rs +++ b/radicle-cli/src/commands/untrack.rs @@ -42,7 +42,7 @@ impl Args for Options { Value(val) if id.is_none() => { let val = val.to_string_lossy(); - if let Ok(val) = Id::from_human(&val) { + if let Ok(val) = Id::from_urn(&val) { id = Some(val); } else { return Err(anyhow!("invalid ID '{}'", val)); @@ -74,13 +74,13 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { term::success!( "Tracking relationships for {} ({}) removed", term::format::highlight(project.name()), - &id.to_human() + &id.urn() ); } else { term::info!( "Tracking relationships for {} ({}) doesn't exist", term::format::highlight(project.name()), - &id.to_human() + &id.urn() ); } diff --git a/radicle-tools/src/rad-clone.rs b/radicle-tools/src/rad-clone.rs index f2e31b63..39393b2f 100644 --- a/radicle-tools/src/rad-clone.rs +++ b/radicle-tools/src/rad-clone.rs @@ -1,6 +1,5 @@ use std::env; use std::path::Path; -use std::str::FromStr; use radicle::identity::Id; @@ -10,7 +9,7 @@ fn main() -> anyhow::Result<()> { let signer = profile.signer()?; if let Some(id) = env::args().nth(1) { - let id = Id::from_str(&id)?; + let id = Id::from_urn(&id)?; let mut node = radicle::node::connect(profile.socket())?; let repo = radicle::rad::clone(id, &cwd, &signer, &profile.storage, &mut node)?; diff --git a/radicle/src/identity.rs b/radicle/src/identity.rs index 1888a690..03aef869 100644 --- a/radicle/src/identity.rs +++ b/radicle/src/identity.rs @@ -167,7 +167,7 @@ mod test { assert_eq!(format!("\"{}\"", pk), json); let json = serde_json::to_string(&proj).unwrap(); - assert_eq!(format!("\"{}\"", proj), json); + assert_eq!(format!("\"{}\"", proj.urn()), json); let json = serde_json::to_string(&did).unwrap(); assert_eq!(format!("\"{}\"", did), json); diff --git a/radicle/src/identity/doc.rs b/radicle/src/identity/doc.rs index 23f6c9b6..e52d607c 100644 --- a/radicle/src/identity/doc.rs +++ b/radicle/src/identity/doc.rs @@ -394,10 +394,7 @@ mod test { (*id).to_string(), "d96f425412c9f8ad5d9a9a05c9831d0728e2338d" ); - assert_eq!( - id.to_human(), - String::from("rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji") - ); + assert_eq!(id.urn(), String::from("rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji")); } #[test] diff --git a/radicle/src/identity/doc/id.rs b/radicle/src/identity/doc/id.rs index 6c73d076..ec95cc20 100644 --- a/radicle/src/identity/doc/id.rs +++ b/radicle/src/identity/doc/id.rs @@ -27,7 +27,7 @@ pub struct Id(git::Oid); impl fmt::Display for Id { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str(&self.to_human()) + f.write_str(self.urn().as_str()) } } @@ -38,23 +38,34 @@ impl fmt::Debug for Id { } impl Id { - /// Format the identifier in a human-readable way. + /// Format the identifier as a human-readable URN. /// /// Eg. `rad:z3XncAdkZjeK9mQS5Sdc4qhw98BUX`. /// - pub fn to_human(&self) -> String { - format!( - "{RAD_PREFIX}{}", - multibase::encode(multibase::Base::Base58Btc, self.0.as_bytes()) - ) + pub fn urn(&self) -> String { + format!("{RAD_PREFIX}{}", self.canonical()) } - /// Parse an identifier from the human-readable format. + /// Parse an identifier from the human-readable URN format. /// Accepts strings without the radicle prefix as well, /// for convenience. - pub fn from_human(s: &str) -> Result { + pub fn from_urn(s: &str) -> Result { let s = s.strip_prefix(RAD_PREFIX).unwrap_or(s); - let (_, bytes) = multibase::decode(s)?; + let id = Self::from_canonical(s)?; + + Ok(id) + } + + /// Format the identifier as a multibase string. + /// + /// Eg. `z3XncAdkZjeK9mQS5Sdc4qhw98BUX`. + /// + pub fn canonical(&self) -> String { + multibase::encode(multibase::Base::Base58Btc, self.0.as_bytes()) + } + + pub fn from_canonical(input: &str) -> Result { + let (_, bytes) = multibase::decode(input)?; let array: git::Oid = bytes.as_slice().try_into()?; Ok(Self(array)) @@ -65,7 +76,7 @@ impl FromStr for Id { type Err = IdError; fn from_str(s: &str) -> Result { - Self::from_human(s) + Self::from_urn(s) } } @@ -74,7 +85,7 @@ impl TryFrom for Id { fn try_from(value: OsString) -> Result { let string = value.to_string_lossy(); - Self::from_str(&string) + Self::from_canonical(&string) } } @@ -103,7 +114,7 @@ impl serde::Serialize for Id { where S: serde::Serializer, { - serde_ext::string::serialize(self, serializer) + serde_ext::string::serialize(&self.urn(), serializer) } } diff --git a/radicle/src/node.rs b/radicle/src/node.rs index 059df758..16c0f92a 100644 --- a/radicle/src/node.rs +++ b/radicle/src/node.rs @@ -138,7 +138,7 @@ impl Handle for Node { } fn fetch(&mut self, id: Id) -> Result<(), Error> { - for line in self.call("fetch", &[id])? { + for line in self.call("fetch", &[id.urn()])? { let line = line?; log::debug!("node: {}", line); } @@ -169,7 +169,7 @@ impl Handle for Node { } fn track_repo(&mut self, id: Id) -> Result { - let mut line = self.call("track-repo", &[id])?; + let mut line = self.call("track-repo", &[id.urn()])?; let line = line .next() .ok_or(Error::EmptyResponse { cmd: "track-repo" })??; @@ -205,7 +205,7 @@ impl Handle for Node { } fn untrack_repo(&mut self, id: Id) -> Result { - let mut line = self.call("untrack-repo", &[id])?; + let mut line = self.call("untrack-repo", &[id.urn()])?; let line = line.next().ok_or(Error::EmptyResponse { cmd: "untrack-repo", })??; @@ -223,7 +223,7 @@ impl Handle for Node { } fn announce_refs(&mut self, id: Id) -> Result<(), Error> { - for line in self.call("announce-refs", &[id])? { + for line in self.call("announce-refs", &[id.urn()])? { let line = line?; log::debug!("node: {}", line); } diff --git a/radicle/src/sql.rs b/radicle/src/sql.rs index fea223f2..147c95bc 100644 --- a/radicle/src/sql.rs +++ b/radicle/src/sql.rs @@ -13,7 +13,7 @@ impl TryFrom<&Value> for Id { fn try_from(value: &Value) -> Result { match value { - Value::String(id) => Id::from_str(id).map_err(|e| sql::Error { + Value::String(id) => Id::from_urn(id).map_err(|e| sql::Error { code: None, message: Some(e.to_string()), }), @@ -27,7 +27,7 @@ impl TryFrom<&Value> for Id { impl sqlite::BindableWithIndex for &Id { fn bind(self, stmt: &mut sql::Statement<'_>, i: I) -> sql::Result<()> { - self.to_human().as_str().bind(stmt, i) + self.urn().as_str().bind(stmt, i) } } diff --git a/radicle/src/storage/git.rs b/radicle/src/storage/git.rs index 4d3019e4..0ab700e4 100644 --- a/radicle/src/storage/git.rs +++ b/radicle/src/storage/git.rs @@ -134,7 +134,7 @@ impl Storage { let name = r.name().ok_or(Error::InvalidRef)?; let oid = r.target().ok_or(Error::InvalidRef)?; - println!("{} {} {}", proj, oid, name); + println!("{} {} {}", proj.urn(), oid, name); } } Ok(()) @@ -726,21 +726,17 @@ pub mod paths { use super::ReadStorage; pub fn repository(storage: &S, proj: &Id) -> PathBuf { - storage.path().join(proj.to_string()) + storage.path().join(proj.canonical()) } } #[cfg(test)] mod tests { - use std::io::{Read, Write}; - use std::{io, net, process, thread}; - use crypto::test::signer::MockSigner; use super::*; use crate::assert_matches; use crate::git; - use crate::rad; use crate::storage::refs::SIGREFS_BRANCH; use crate::storage::{ReadRepository, ReadStorage, RefUpdate, WriteRepository}; use crate::test::arbitrary; @@ -903,115 +899,6 @@ mod tests { assert_eq!(refs, vec!["refs/heads/master", "refs/rad/id"]); } - #[test] - #[ignore] - // Test the remote transport using `git-upload-pack` and TCP streams. - // Must be run on its own, since it tries to register the remote transport, which - // will fail if the mock transport was already registered. - fn test_upload_pack() { - let tmp = tempfile::tempdir().unwrap(); - let signer = MockSigner::default(); - let remote = *signer.public_key(); - let storage = Storage::open(tmp.path().join("storage")).unwrap(); - let socket = net::TcpListener::bind(net::SocketAddr::from(([0, 0, 0, 0], 0))).unwrap(); - let addr = socket.local_addr().unwrap(); - let source_path = tmp.path().join("source"); - let target_path = tmp.path().join("target"); - let (source, _) = fixtures::repository(source_path); - - transport::local::register(storage.clone()); - - let (proj, _, _) = rad::init( - &source, - "radicle", - "radicle", - git::refname!("master"), - &signer, - &storage, - ) - .unwrap(); - - let t = thread::spawn(move || { - let (stream, _) = socket.accept().unwrap(); - let repo = storage.repository(proj).unwrap(); - // NOTE: `GIT_PROTOCOL=version=2` doesn't work. - let mut child = process::Command::new("git") - .current_dir(repo.path()) - .arg("upload-pack") - .arg("--strict") // The path to the git repo must be exact. - .arg(".") - .stdout(process::Stdio::piped()) - .stdin(process::Stdio::piped()) - .spawn() - .unwrap(); - - let mut stdin = child.stdin.take().unwrap(); - let mut stdout = child.stdout.take().unwrap(); - - let mut stream_r = stream.try_clone().unwrap(); - let mut stream_w = stream; - - let t = thread::spawn(move || { - let mut buf = [0u8; 1024]; - - while let Ok(n) = stream_r.read(&mut buf) { - if n == 0 { - break; - } - if stdin.write_all(&buf[..n]).is_err() { - break; - } - } - }); - io::copy(&mut stdout, &mut stream_w).unwrap(); - - t.join().unwrap(); - child.wait().unwrap(); - }); - - let mut updates = Vec::new(); - { - let mut callbacks = git2::RemoteCallbacks::new(); - let mut opts = git2::FetchOptions::default(); - - callbacks.update_tips(|name, _, _| { - updates.push(name.to_owned()); - true - }); - opts.remote_callbacks(callbacks); - - let target = git2::Repository::init_bare(target_path).unwrap(); - let stream = net::TcpStream::connect(addr).unwrap(); - - // Register the `heartwood://` transport for this stream. - transport::remote::register(remote, stream.try_clone().unwrap()); - - // Fetch with the `heartwood://` transport. - target - .remote_anonymous(&format!("heartwood://{remote}/{proj}")) - .unwrap() - .fetch( - &["refs/namespaces/*:refs/namespaces/*"], - Some(&mut opts), - None, - ) - .unwrap(); - - stream.shutdown(net::Shutdown::Both).unwrap(); - - t.join().unwrap(); - } - - assert_eq!( - updates, - vec![ - format!("refs/namespaces/{remote}/refs/heads/master"), - format!("refs/namespaces/{remote}/refs/rad/id"), - format!("refs/namespaces/{remote}/refs/rad/sigrefs") - ] - ); - } - #[test] fn test_sign_refs() { let tmp = tempfile::tempdir().unwrap(); diff --git a/radicle/src/storage/git/transport/local/url.rs b/radicle/src/storage/git/transport/local/url.rs index 4f7a107a..2d82f841 100644 --- a/radicle/src/storage/git/transport/local/url.rs +++ b/radicle/src/storage/git/transport/local/url.rs @@ -66,9 +66,9 @@ impl From for Url { impl fmt::Display for Url { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { if let Some(ns) = self.namespace { - write!(f, "{}://{}/{}", Self::SCHEME, self.repo, ns) + write!(f, "{}://{}/{}", Self::SCHEME, self.repo.canonical(), ns) } else { - write!(f, "{}://{}", Self::SCHEME, self.repo) + write!(f, "{}://{}", Self::SCHEME, self.repo.canonical()) } } } @@ -88,7 +88,7 @@ impl FromStr for Url { _ => return Err(UrlError::InvalidFormat), }; - let resource = Id::from_str(resource).map_err(UrlError::InvalidRepository)?; + let resource = Id::from_canonical(resource).map_err(UrlError::InvalidRepository)?; let namespace = namespace .map(|pk| Namespace::from_str(pk).map_err(UrlError::InvalidNamespace)) .transpose()?; @@ -106,33 +106,37 @@ mod test { #[test] fn test_url_parse() { - let repo = Id::from_str("z2w8RArM3gaBXZxXhQUswE3hhLcss").unwrap(); + let repo = Id::from_canonical("z2w8RArM3gaBXZxXhQUswE3hhLcss").unwrap(); let namespace = Namespace::from_str("z6Mkifeb5NPS6j7JP72kEQEeuqMTpCAVcHsJi1C86jGTzHRi").unwrap(); - let url = format!("rad://{repo}"); + let url = format!("rad://{}", repo.canonical()); let url = Url::from_str(&url).unwrap(); assert_eq!(url.repo, repo); assert_eq!(url.namespace, None); - let url = format!("rad://{repo}/{namespace}"); + let url = format!("rad://{}/{namespace}", repo.canonical()); let url = Url::from_str(&url).unwrap(); assert_eq!(url.repo, repo); assert_eq!(url.namespace, Some(namespace)); - assert!(format!("heartwood://{repo}").parse::().is_err()); - assert!(format!("git://{repo}").parse::().is_err()); + assert!(format!("heartwood://{}", repo.canonical()) + .parse::() + .is_err()); + assert!(format!("git://{}", repo.canonical()) + .parse::() + .is_err()); assert!(format!("rad://{namespace}").parse::().is_err()); - assert!(format!("rad://{repo}/{namespace}/fnord") + assert!(format!("rad://{}/{namespace}/fnord", repo.canonical()) .parse::() .is_err()); } #[test] fn test_url_to_string() { - let repo = Id::from_str("z2w8RArM3gaBXZxXhQUswE3hhLcss").unwrap(); + let repo = Id::from_canonical("z2w8RArM3gaBXZxXhQUswE3hhLcss").unwrap(); let namespace = Namespace::from_str("z6Mkifeb5NPS6j7JP72kEQEeuqMTpCAVcHsJi1C86jGTzHRi").unwrap(); @@ -140,12 +144,15 @@ mod test { repo, namespace: None, }; - assert_eq!(url.to_string(), format!("rad://{repo}")); + assert_eq!(url.to_string(), format!("rad://{}", repo.canonical())); let url = Url { repo, namespace: Some(namespace), }; - assert_eq!(url.to_string(), format!("rad://{repo}/{namespace}")); + assert_eq!( + url.to_string(), + format!("rad://{}/{namespace}", repo.canonical()) + ); } } diff --git a/radicle/src/storage/git/transport/remote/mock.rs b/radicle/src/storage/git/transport/remote/mock.rs index 7c5c3b17..b8b4d306 100644 --- a/radicle/src/storage/git/transport/remote/mock.rs +++ b/radicle/src/storage/git/transport/remote/mock.rs @@ -42,7 +42,7 @@ impl git2::transport::SmartSubtransport for MockTransport { url.node ))); }; - let git_dir = storage.join(url.repo.to_human()); + let git_dir = storage.join(url.repo.canonical()); let mut cmd = process::Command::new("git"); let mut child = cmd .arg("upload-pack") diff --git a/radicle/src/storage/git/transport/remote/url.rs b/radicle/src/storage/git/transport/remote/url.rs index 2d160b79..638a84f2 100644 --- a/radicle/src/storage/git/transport/remote/url.rs +++ b/radicle/src/storage/git/transport/remote/url.rs @@ -53,9 +53,22 @@ impl Url { impl fmt::Display for Url { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { if let Some(ns) = self.namespace { - write!(f, "{}://{}/{}/{}", Self::SCHEME, self.node, self.repo, ns) + write!( + f, + "{}://{}/{}/{}", + Self::SCHEME, + self.node, + self.repo.canonical(), + ns + ) } else { - write!(f, "{}://{}/{}", Self::SCHEME, self.node, self.repo) + write!( + f, + "{}://{}/{}", + Self::SCHEME, + self.node, + self.repo.canonical() + ) } } } @@ -76,7 +89,7 @@ impl FromStr for Url { }; let node = NodeId::from_str(node).map_err(UrlError::InvalidNode)?; - let resource = Id::from_str(resource).map_err(UrlError::InvalidRepository)?; + let resource = Id::from_canonical(resource).map_err(UrlError::InvalidRepository)?; let namespace = namespace .map(|pk| Namespace::from_str(pk).map_err(UrlError::InvalidNamespace)) .transpose()?; @@ -96,18 +109,18 @@ mod test { #[test] fn test_url_parse() { let node = NodeId::from_str("z6MkhaXgBZDvotDkL5257faiztiGiC2QtKLGpbnnEGta2doK").unwrap(); - let repo = Id::from_str("z2w8RArM3gaBXZxXhQUswE3hhLcss").unwrap(); + let repo = Id::from_canonical("z2w8RArM3gaBXZxXhQUswE3hhLcss").unwrap(); let namespace = Namespace::from_str("z6Mkifeb5NPS6j7JP72kEQEeuqMTpCAVcHsJi1C86jGTzHRi").unwrap(); - let url = format!("heartwood://{node}/{repo}"); + let url = format!("heartwood://{node}/{}", repo.canonical()); let url = Url::from_str(&url).unwrap(); assert_eq!(url.node, node); assert_eq!(url.repo, repo); assert_eq!(url.namespace, None); - let url = format!("heartwood://{node}/{repo}/{namespace}"); + let url = format!("heartwood://{node}/{}/{namespace}", repo.canonical()); let url = Url::from_str(&url).unwrap(); assert_eq!(url.node, node); @@ -119,8 +132,10 @@ mod test { assert!(format!("heartwood://{node}/{namespace}") .parse::() .is_err()); - assert!(format!("heartwood://{node}/{repo}/{namespace}/fnord") - .parse::() - .is_err()); + assert!( + format!("heartwood://{node}/{}/{namespace}/fnord", repo.canonical()) + .parse::() + .is_err() + ); } }