From f9ad3ac155e4428986b1cd4689807fd8c3288ae3 Mon Sep 17 00:00:00 2001 From: Slack Coder Date: Fri, 23 Sep 2022 06:04:45 -0500 Subject: [PATCH] git: Use RefString types for git::refs Lean on rust's type system to improve safety. Signed-off-by: Slack Coder --- Cargo.lock | 2 ++ radicle-node/Cargo.toml | 9 +++++---- radicle-node/src/test/tests.rs | 4 ++-- radicle-tools/Cargo.toml | 1 + radicle-tools/src/rad-init.rs | 4 +++- radicle/src/git.rs | 36 ++++++++++++++++++++++----------- radicle/src/identity/project.rs | 4 ++-- radicle/src/rad.rs | 35 ++++++++++++++++---------------- radicle/src/storage.rs | 2 +- radicle/src/storage/git.rs | 2 +- radicle/src/test/arbitrary.rs | 8 +++++--- radicle/src/test/fixtures.rs | 6 +++--- 12 files changed, 67 insertions(+), 46 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9d66374b..f96fab44 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -815,6 +815,7 @@ dependencies = [ "colored", "crossbeam-channel", "fastrand", + "git-ref-format", "lexopt", "log", "nakamoto-net", @@ -839,6 +840,7 @@ name = "radicle-tools" version = "0.2.0" dependencies = [ "anyhow", + "git-ref-format", "radicle", ] diff --git a/radicle-node/Cargo.toml b/radicle-node/Cargo.toml index 8226cf2d..dfa9aaa1 100644 --- a/radicle-node/Cargo.toml +++ b/radicle-node/Cargo.toml @@ -7,19 +7,20 @@ edition = "2021" [dependencies] anyhow = { version = "1" } -byteorder = { version = "1" } bloomy = { version = "1.2" } +byteorder = { version = "1" } chrono = { version = "0.4.0" } colored = { version = "1.9.0" } crossbeam-channel = { version = "0.5.6" } fastrand = { version = "1.8.0" } +git-ref-format = { version = "0", features = ["serde", "macro"] } lexopt = { version = "0.2.1" } log = { version = "0.4.17", features = ["std"] } -serde = { version = "1", features = ["derive"] } -serde_json = { version = "1", features = ["preserve_order"] } -nonempty = { version = "0.8.0", features = ["serialize"] } nakamoto-net = { version = "0.3.0" } nakamoto-net-poll = { version = "0.3.0" } +nonempty = { version = "0.8.0", features = ["serialize"] } +serde = { version = "1", features = ["derive"] } +serde_json = { version = "1", features = ["preserve_order"] } tempfile = { version = "3.3.0" } thiserror = { version = "1" } diff --git a/radicle-node/src/test/tests.rs b/radicle-node/src/test/tests.rs index 0936b0a4..8974236d 100644 --- a/radicle-node/src/test/tests.rs +++ b/radicle-node/src/test/tests.rs @@ -22,7 +22,7 @@ use crate::test::simulator; use crate::test::simulator::{Peer as _, Simulation}; use crate::test::storage::MockStorage; use crate::LocalTime; -use crate::{client, identity, rad, service, storage, test}; +use crate::{client, git, identity, rad, service, test}; // NOTE // @@ -397,7 +397,7 @@ fn test_push_and_pull() { &repo, "alice", "alice's repo", - storage::BranchName::from("master"), + git::refname!("master"), alice.signer(), alice.storage(), ) diff --git a/radicle-tools/Cargo.toml b/radicle-tools/Cargo.toml index 35c31b65..523ededa 100644 --- a/radicle-tools/Cargo.toml +++ b/radicle-tools/Cargo.toml @@ -7,6 +7,7 @@ edition = "2021" [dependencies] anyhow = { version = "1" } +git-ref-format = { version = "0", features = ["serde", "macro"] } [dependencies.radicle] version = "0" diff --git a/radicle-tools/src/rad-init.rs b/radicle-tools/src/rad-init.rs index 522aaea6..5b4e7b88 100644 --- a/radicle-tools/src/rad-init.rs +++ b/radicle-tools/src/rad-init.rs @@ -1,5 +1,7 @@ use std::path::Path; +use radicle::git; + fn main() -> anyhow::Result<()> { let cwd = Path::new(".").canonicalize()?; let name = cwd.file_name().unwrap().to_string_lossy().to_string(); @@ -9,7 +11,7 @@ fn main() -> anyhow::Result<()> { &repo, &name, "", - radicle::git::BranchName::from("master"), + git::refname!("master"), &profile.signer, &profile.storage, )?; diff --git a/radicle/src/git.rs b/radicle/src/git.rs index ff5f42e6..b5405d15 100644 --- a/radicle/src/git.rs +++ b/radicle/src/git.rs @@ -16,7 +16,7 @@ pub use ext::Error; pub use ext::Oid; pub use git2 as raw; pub use git_ref_format as fmt; -pub use git_ref_format::{refname, RefStr, RefString}; +pub use git_ref_format::{refname, Component, Qualified, RefStr, RefString}; pub use git_url as url; pub use git_url::Url; pub use radicle_git_ext as ext; @@ -41,6 +41,13 @@ pub enum ListRefsError { InvalidRef(#[from] RefError), } +impl From<&RemoteId> for RefString { + fn from(id: &RemoteId) -> Self { + // PublicKey strings contain only legal characters. + RefString::try_from(id.to_string()).unwrap() + } +} + pub mod refs { use super::*; @@ -50,31 +57,36 @@ pub mod refs { pub mod storage { use super::*; - pub fn branch(remote: &RemoteId, branch: &str) -> String { - format!("refs/remotes/{remote}/heads/{branch}") + pub fn branch(remote: &RemoteId, branch: &RefStr) -> RefString { + refname!("refs/remotes") + .and::(remote.into()) + .and(refname!("heads")) + .and(branch) } /// Get the branch used to track project information. - pub fn id(remote: &RemoteId) -> String { + pub fn id(remote: &RemoteId) -> RefString { branch(remote, &IDENTITY_BRANCH) } } pub mod workdir { - pub fn branch(branch: &str) -> String { - format!("refs/heads/{branch}") + use super::*; + + pub fn branch(branch: &RefStr) -> RefString { + refname!("refs/heads").join(branch) } - pub fn note(name: &str) -> String { - format!("refs/notes/{name}") + pub fn note(name: &RefStr) -> RefString { + refname!("refs/notes").join(name) } - pub fn remote_branch(remote: &str, branch: &str) -> String { - format!("refs/remotes/{remote}/{branch}") + pub fn remote_branch(remote: &RefStr, branch: &RefStr) -> RefString { + refname!("refs/remotes").and(remote).and(branch) } - pub fn tag(name: &str) -> String { - format!("refs/tags/{name}") + pub fn tag(name: &RefStr) -> RefString { + refname!("refs/tags").join(name) } } } diff --git a/radicle/src/identity/project.rs b/radicle/src/identity/project.rs index 649e42d8..f666e435 100644 --- a/radicle/src/identity/project.rs +++ b/radicle/src/identity/project.rs @@ -71,8 +71,8 @@ impl From for PublicKey { #[serde(rename_all = "kebab-case")] pub struct Payload { pub name: String, - pub description: String, // TODO: Make optional. - pub default_branch: String, // TODO: Make optional. + pub description: String, // TODO: Make optional. + pub default_branch: git::RefString, // TODO: Make optional. } #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] diff --git a/radicle/src/rad.rs b/radicle/src/rad.rs index 0359b69d..a7ef9dbf 100644 --- a/radicle/src/rad.rs +++ b/radicle/src/rad.rs @@ -3,6 +3,7 @@ use std::io; use std::path::Path; use std::str::FromStr; +use once_cell::sync::Lazy; use thiserror::Error; use crate::crypto::{Signer, Verified}; @@ -13,7 +14,7 @@ use crate::storage::refs::SignedRefs; use crate::storage::{BranchName, ReadRepository as _, RemoteId, WriteRepository as _}; use crate::{identity, storage}; -pub const REMOTE_NAME: &str = "rad"; +pub static REMOTE_NAME: Lazy = Lazy::new(|| git::refname!("rad")); #[derive(Error, Debug)] pub enum InitError { @@ -63,7 +64,7 @@ pub fn init( git::set_upstream( repo, - REMOTE_NAME, + &REMOTE_NAME, &default_branch, &git::refs::storage::branch(pk, &default_branch), )?; @@ -71,7 +72,7 @@ pub fn init( // 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, &url)?.push::<&str>( + git::configure_remote(repo, &REMOTE_NAME, pk, &url)?.push::<&str>( &[&format!( "{}:{}", &git::refs::workdir::branch(&default_branch), @@ -276,24 +277,24 @@ pub fn checkout, S: storage::ReadStorage>( 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); // Configure and fetch all refs from remote. - git::configure_remote(&repo, REMOTE_NAME, remote, &url)?.fetch::<&str>(&[], None, None)?; + git::configure_remote(&repo, &REMOTE_NAME, remote, &url)?.fetch::<&str>(&[], None, None)?; { // Setup default branch. - let remote_head_ref = git::refs::workdir::remote_branch(REMOTE_NAME, default_branch); + let remote_head_ref = + git::refs::workdir::remote_branch(&REMOTE_NAME, &project.default_branch); let remote_head_commit = repo.find_reference(&remote_head_ref)?.peel_to_commit()?; - let _ = repo.branch(default_branch, &remote_head_commit, true)?; + let _ = repo.branch(&project.default_branch, &remote_head_commit, true)?; // Setup remote tracking for default branch. git::set_upstream( &repo, - REMOTE_NAME, - default_branch, - &git::refs::storage::branch(remote, default_branch), + &REMOTE_NAME, + &project.default_branch, + &git::refs::storage::branch(remote, &project.default_branch), )?; } @@ -302,7 +303,7 @@ pub fn checkout, S: storage::ReadStorage>( /// Get the radicle ("rad") remote of a repository, and return the associated project id. pub fn remote(repo: &git2::Repository) -> Result<(git2::Remote<'_>, Id), git2::Error> { - let remote = repo.find_remote(REMOTE_NAME)?; + let remote = repo.find_remote(&REMOTE_NAME)?; let url = remote.url_bytes(); let url = git::Url::from_bytes(url).unwrap(); let path = url.path.to_string(); @@ -335,7 +336,7 @@ mod tests { &repo, "acme", "Acme's repo", - BranchName::from("master"), + git::refname!("master"), &signer, &storage, ) @@ -353,7 +354,7 @@ mod tests { 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.default_branch, git::refname!("master")); assert_eq!( project.delegates.first(), &Delegate { @@ -378,7 +379,7 @@ mod tests { &original, "acme", "Acme's repo", - BranchName::from("master"), + git::refname!("master"), &alice, &storage, ) @@ -408,7 +409,7 @@ mod tests { &original, "acme", "Acme's repo", - BranchName::from("master"), + git::refname!("master"), &signer, &storage, ) @@ -430,14 +431,14 @@ mod tests { .to_vec() ); assert_eq!( - copy.find_remote(REMOTE_NAME) + copy.find_remote(&REMOTE_NAME) .unwrap() .refspecs() .into_iter() .map(|r| r.bytes().to_vec()) .collect::>(), original - .find_remote(REMOTE_NAME) + .find_remote(&REMOTE_NAME) .unwrap() .refspecs() .into_iter() diff --git a/radicle/src/storage.rs b/radicle/src/storage.rs index ee7fe65d..e3ea2dfc 100644 --- a/radicle/src/storage.rs +++ b/radicle/src/storage.rs @@ -24,7 +24,7 @@ use crate::storage::refs::Refs; use self::refs::SignedRefs; -pub type BranchName = String; +pub type BranchName = git::RefString; pub type Inventory = Vec; /// Storage error. diff --git a/radicle/src/storage/git.rs b/radicle/src/storage/git.rs index 583f1c5a..76b48b38 100644 --- a/radicle/src/storage/git.rs +++ b/radicle/src/storage/git.rs @@ -810,7 +810,7 @@ mod tests { &source, "radicle", "radicle", - BranchName::from("master"), + git::refname!("master"), signer, &storage, ) diff --git a/radicle/src/test/arbitrary.rs b/radicle/src/test/arbitrary.rs index 3943c6a1..e272bb54 100644 --- a/radicle/src/test/arbitrary.rs +++ b/radicle/src/test/arbitrary.rs @@ -85,7 +85,7 @@ impl Arbitrary for Doc { fn arbitrary(g: &mut quickcheck::Gen) -> Self { let name = String::arbitrary(g); let description = String::arbitrary(g); - let default_branch = String::arbitrary(g); + let default_branch = git::RefString::try_from(String::arbitrary(g)).unwrap(); let delegate = Delegate::arbitrary(g); Self::initial(name, description, default_branch, delegate) @@ -101,9 +101,11 @@ impl Arbitrary for Doc { let description = iter::repeat_with(|| rng.alphanumeric()) .take(rng.usize(0..32)) .collect(); - let default_branch = iter::repeat_with(|| rng.alphanumeric()) + let default_branch: git::RefString = iter::repeat_with(|| rng.alphanumeric()) .take(rng.usize(1..16)) - .collect(); + .collect::() + .try_into() + .unwrap(); let delegates: NonEmpty<_> = iter::repeat_with(|| Delegate { name: iter::repeat_with(|| rng.alphanumeric()) .take(rng.usize(1..16)) diff --git a/radicle/src/test/fixtures.rs b/radicle/src/test/fixtures.rs index 2ef83275..5c2f1a6b 100644 --- a/radicle/src/test/fixtures.rs +++ b/radicle/src/test/fixtures.rs @@ -6,7 +6,7 @@ use crate::identity::Id; use crate::rad; use crate::storage::git::Storage; use crate::storage::refs::SignedRefs; -use crate::storage::{BranchName, WriteStorage}; +use crate::storage::WriteStorage; /// Create a new storage with a project. pub fn storage, G: Signer>(path: P, signer: G) -> Result { @@ -23,7 +23,7 @@ pub fn storage, G: Signer>(path: P, signer: G) -> Result, S: WriteStorage, G: Signer>( &repo, "acme", "Acme's repository", - BranchName::from("master"), + git::refname!("master"), signer, storage, )?;