diff --git a/crates/radicle-cli/examples/git/git-push-canonical-symbolic-ref.md b/crates/radicle-cli/examples/git/git-push-canonical-symbolic-ref.md index 1e783c4d6..49fa4e623 100644 --- a/crates/radicle-cli/examples/git/git-push-canonical-symbolic-ref.md +++ b/crates/radicle-cli/examples/git/git-push-canonical-symbolic-ref.md @@ -1,17 +1,15 @@ -Bob overhears that the new name for the default branch is "main", not "master" -as it used to be. To make tooling that expect "main" work, without the hassle -of having to push to such branch manually, he opts to create a symbolic -reference. +Bob has a weird name for a particular release in mind, and also wants to test +out symbolic references. ``` ~bob $ cd heartwood -$ rad id update --title "Add canonical symbolic ref" --payload xyz.radicle.crefs symbolic '{ "refs/heads/main": "refs/heads/master" }' +$ rad id update --title "Add canonical symbolic ref" --payload xyz.radicle.crefs symbolic '{ "refs/heads/foobar": "refs/heads/releases/foobaz" }' ✓ Identity revision [..] created ╭────────────────────────────────────────────────────────────────────────╮ │ Title Add canonical symbolic ref │ -│ Revision 62e2cb60c6df9ad9908b6697b5d126760a855484 │ +│ Revision b7597e3eeaba64467db4417602518169b76b43d5 │ │ Parent 37a1aad231100cd206c49aed79e405ea2da9204b │ -│ Blob b20de7b184673eb0d9227be17640c923d8ef3f3e │ +│ Blob 49a21614cf358b131fd6b590d0d01396f98906ce │ │ Author did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk │ │ State active │ │ Quorum no │ @@ -41,7 +39,7 @@ $ rad id update --title "Add canonical symbolic ref" --payload xyz.radicle.crefs } + }, + "symbolic": { -+ "refs/heads/main": "refs/heads/master" ++ "refs/heads/foobar": "refs/heads/releases/foobaz" } }, "xyz.radicle.project": { @@ -58,14 +56,22 @@ $ rad id update --title "Add canonical symbolic ref" --payload xyz.radicle.crefs } ``` -Alice is happy with the new revision and accepts it. +Bob proceeds to push some commit to the branch that is targeted by the +symbolic reference. + +``` ~bob +$ git push -q rad master:releases/foobaz +``` + +Alice decides to play along with Bob's strange ideas and accepts the new +revision. ``` ~alice $ rad sync -f Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from the network, found 1 potential seed(s). ✓ Target met: 1 seed(s) 🌱 Fetched from z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk -$ rad id accept 62e2cb60c6df9ad9908b6697b5d126760a855484 -q +$ rad id accept b7597e3eeaba64467db4417602518169b76b43d5 -q ``` As usual, alice works on "master". @@ -91,9 +97,10 @@ She inspects the remote and sees that indeed a new branch named "main" now exist ``` ~alice $ git ls-remote rad 4dc510ddea5fd66499d1d2e996b8a97c8d57be54 HEAD -4dc510ddea5fd66499d1d2e996b8a97c8d57be54 refs/heads/main +f2de534b5e81d7c6e2dcaf58c3dd91573c0a0354 refs/heads/foobar 4dc510ddea5fd66499d1d2e996b8a97c8d57be54 refs/heads/master afec366785ed3651cdc66975c0fec41866c9ce62 refs/heads/releases/2 +f2de534b5e81d7c6e2dcaf58c3dd91573c0a0354 refs/heads/releases/foobaz f2de534b5e81d7c6e2dcaf58c3dd91573c0a0354 refs/tags/qa/v2.1 ac51a0746a5e8311829bc481202909a1e3acc0c2 refs/tags/v1.0-hotfix 89f935f27a16f8ed97915ade4accab8fe48057aa refs/tags/v2.0 @@ -104,10 +111,11 @@ Of course, she can also fetch it to her working copy as usual. ``` ~alice (stderr) $ git fetch rad From rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji - * [new branch] main -> rad/main - * [new branch] releases/2 -> rad/releases/2 - * [new tag] qa/v2.1 -> rad/tags/qa/v2.1 - * [new tag] qa/v2.1 -> qa/v2.1 + * [new branch] foobar -> rad/foobar + * [new branch] releases/2 -> rad/releases/2 + * [new branch] releases/foobaz -> rad/releases/foobaz + * [new tag] qa/v2.1 -> rad/tags/qa/v2.1 + * [new tag] qa/v2.1 -> qa/v2.1 ``` Bob fetches Alice's changes. @@ -124,8 +132,8 @@ And, sure enough, there is the new branch just as he wanted it. ``` ~bob (stderr) $ git fetch rad From rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji - * [new branch] main -> rad/main + * [new branch] foobar -> rad/foobar f2de534..4dc510d master -> rad/master ``` -Note that neither Alice nor Bob pushed directly to "main". +Note that neither Alice nor Bob pushed directly to "foobar". diff --git a/crates/radicle/src/git/canonical/rules.rs b/crates/radicle/src/git/canonical/rules.rs index 1df5e4e03..2455ce0f3 100644 --- a/crates/radicle/src/git/canonical/rules.rs +++ b/crates/radicle/src/git/canonical/rules.rs @@ -32,9 +32,9 @@ use super::protect::Unprotected; const ASTERISK: char = '*'; -/// Private trait to ensure that not any `Rule` can be deserialized. -/// Implementations are provided for `Allowed` and `usize` so that `RawRule`s -/// can be deserialized, while `ValidRule`s cannot – preventing deserialization +/// Private trait to ensure that not any [`Rule`] can be deserialized. +/// Implementations are provided for [`Allowed`] and [`usize`] so that [`RawRule`]s +/// can be deserialized, while [`ValidRule`]s cannot – preventing deserialization /// bugs for that type. trait Sealed {} impl Sealed for Allowed {} @@ -246,7 +246,13 @@ impl RawRule { } } -/// A set of `RawRule`s that can be serialized and deserialized. +/// A set of [`RawRule`]s that can be serialized and deserialized. +/// +/// Note that matching on the [`RawPattern`]s is deliberately not implemented, +/// as the [`RawRule`]s are not validated and thus should not be used to +/// calculate canonical references. +/// If you want to match on a set of rules, obtain [`Rules`] via +/// [`Rules::from_raw`], and use [`Rules::matches`] on the result. #[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] pub struct RawRules { /// The reference pattern that this rule applies to. @@ -285,18 +291,6 @@ impl RawRules { .iter() .any(|(pattern, _)| pattern.as_str() == refname) } - - /// Check if the `refname` matches any existing rules, including glob - /// matches. - pub fn matches<'a, 'b>( - &self, - refname: &Qualified<'b>, - ) -> impl Iterator + use<'a, '_, 'b> { - let refname = refname.clone(); - self.rules - .iter() - .filter(move |(pattern, _)| matches(pattern, &refname)) - } } impl Extend<(RawPattern, RawRule)> for RawRules { diff --git a/crates/radicle/src/git/canonical/rules/test.rs b/crates/radicle/src/git/canonical/rules/test.rs index 9931d1fac..51550ba86 100644 --- a/crates/radicle/src/git/canonical/rules/test.rs +++ b/crates/radicle/src/git/canonical/rules/test.rs @@ -563,3 +563,45 @@ fn matches_exactly_curly_braces() { assert!(!matches(&exact, &git::fmt::qualified!("refs/heads/foo"))); assert!(!matches(&exact, &git::fmt::qualified!("refs/heads/bar"))); } + +#[test] +fn most_specific_match_ignores_lexicographic_order() { + use helper::*; + + let glob = raw_pattern(qualified_pattern!("refs/heads/*")); + let nested = raw_pattern(qualified_pattern!("refs/heads/feature/*")); + let exact = raw_pattern(qualified_pattern!("refs/heads/main")); + let rules = RawRules::from_iter([ + (glob.clone(), Rule::new(Allowed::Delegates, 1)), + (nested.clone(), Rule::new(Allowed::Delegates, 1)), + (exact.clone(), Rule::new(Allowed::Delegates, 1)), + ]); + + let resolve = &mut || { + doc::Delegates::from( + Did::decode("did:key:z6MkpQTLwr8QyADGmBGAMsGttvWzP4PojUMs4hREZW5T5E3K").unwrap(), + ) + }; + + let rules = Rules::from_raw(rules, resolve).unwrap(); + + let main = git::fmt::qualified!("refs/heads/main"); + assert_eq!( + rules.matches(&main).next().map(|(pattern, _)| pattern), + Some(&exact) + ); + + let feature = git::fmt::qualified!("refs/heads/feature/x"); + assert_eq!( + rules.matches(&feature).next().map(|(pattern, _)| pattern), + Some(&nested), + "more components beat the glob" + ); + + assert!( + rules + .matches(&git::fmt::qualified!("refs/tags/v1.0")) + .next() + .is_none() + ); +} diff --git a/crates/radicle/src/identity/crefs.rs b/crates/radicle/src/identity/crefs.rs index cd7cbb5ef..ba67fbb62 100644 --- a/crates/radicle/src/identity/crefs.rs +++ b/crates/radicle/src/identity/crefs.rs @@ -132,6 +132,10 @@ impl CanonicalRefs { pub fn symbolic(&self) -> &SymbolicRefs { &self.symbolic } + + pub fn into_raw(self) -> (RawRules, SymbolicRefs) { + (self.rules.into(), self.symbolic) + } } impl Extend<(rules::RawPattern, rules::RawRule)> for RawCanonicalRefs { diff --git a/crates/radicle/src/identity/doc.rs b/crates/radicle/src/identity/doc.rs index 11cdabbd9..a2316ae5e 100644 --- a/crates/radicle/src/identity/doc.rs +++ b/crates/radicle/src/identity/doc.rs @@ -19,7 +19,7 @@ use crate::cob::identity; use crate::crypto; use crate::crypto::Signature; use crate::git; -use crate::git::canonical::rules; +use crate::git::canonical::rules::{self, ResolvedDelegates}; use crate::git::canonical::symbolic; use crate::git::fmt::Qualified; use crate::git::fmt::RefString; @@ -808,17 +808,20 @@ impl Doc { /// /// [`RawCanonicalRefs`]: super::crefs::RawCanonicalRefs pub fn canonical_refs(&self) -> Result { - let mut raw_crefs = self + let resolve = &mut || self.delegates.clone(); + let crefs = self .raw_canonical_refs() .transpose() .map_err(CanonicalRefsError::Payload)? + .map(|crefs| crefs.try_into_canonical_refs(resolve)) + .transpose() + .map_err(CanonicalRefsError::CanonicalRefs)? .unwrap_or_default(); - let resolve = &mut || self.delegates.clone(); // Determine where `HEAD` comes from. The `resolve_head()` result // borrows `raw_crefs`, so clone to allow mutation in the synthesis // path. - let head: Option> = raw_crefs.symbolic().resolve_head().cloned(); + let head: Option> = crefs.symbolic().resolve_head().cloned(); match (head, self.project()) { (Some(ref default_branch), Some(Ok(project))) => { @@ -831,13 +834,14 @@ impl Doc { }, )); } - self.validate_head_rule(&raw_crefs, default_branch)?; + self.validate_head_rule(&crefs, default_branch)?; } (Some(ref default_branch), None) => { - self.validate_head_rule(&raw_crefs, default_branch)?; + self.validate_head_rule(&crefs, default_branch)?; } (None, Some(Ok(project))) => { - self.synthesize_head(&mut raw_crefs, &project)?; + let raw_crefs = self.synthesize_head(crefs, &project)?; + return Ok(raw_crefs.try_into_canonical_refs(resolve)?); } (None, None) => { return Err(CanonicalRefsError::SynthesisPayloadMissing); @@ -847,7 +851,7 @@ impl Doc { } } - Ok(raw_crefs.try_into_canonical_refs(resolve)?) + Ok(crefs) } /// Validate that the rule matching `HEAD`'s target branch uses @@ -857,23 +861,32 @@ impl Doc { /// later by [`RawCanonicalRefs::try_into_canonical_refs`] validation. fn validate_head_rule( &self, - raw_crefs: &RawCanonicalRefs, - default_branch: &Qualified, + raw_crefs: &CanonicalRefs, + default_branch: &Qualified<'static>, ) -> Result<(), CanonicalRefsError> { - let Some((pattern, rule)) = raw_crefs.raw_rules().matches(default_branch).next() else { + let Some((pattern, rule)) = raw_crefs.rules().matches(default_branch).next() else { return Ok(()); }; - let allowed = rule.allowed(); - if *allowed != rules::Allowed::Delegates { - return Err(CanonicalRefsError::DefaultBranchRuleError( - DefaultBranchRuleError::Allowed { - pattern: pattern.to_string(), - actual: allowed.to_string(), - }, - )); + let _allowed = rule.allowed(); + match rule.allowed() { + ResolvedDelegates::Delegates(_) => { + // This is the expected case, so we continue to validate the threshold. + } + ResolvedDelegates::Set(actual) => { + return Err(CanonicalRefsError::DefaultBranchRuleError( + DefaultBranchRuleError::Allowed { + pattern: pattern.to_string(), + actual: actual + .iter() + .map(|s| s.to_string()) + .collect::>() + .join(", "), + }, + )); + } } - let actual = *rule.threshold(); + let actual = (*rule.threshold()).into(); let expected = self.threshold(); if actual != expected { return Err(CanonicalRefsError::DefaultBranchRuleError( @@ -891,30 +904,28 @@ impl Doc { /// from the project payload. fn synthesize_head( &self, - raw_crefs: &mut RawCanonicalRefs, + crefs: CanonicalRefs, project: &Project, - ) -> Result<(), CanonicalRefsError> { + ) -> Result { let default_branch = project.default_branch_qualified(); - if raw_crefs - .raw_rules() - .matches(&default_branch) - .next() - .is_none() - { - raw_crefs.raw_rules_mut().insert( + let add_rule = crefs.rules().matches(&default_branch).next().is_none(); + + let (mut raw_rules, mut symbolic) = crefs.into_raw(); + + if add_rule { + raw_rules.insert( git::fmt::refspec::QualifiedPattern::from(default_branch.to_owned()), rules::Rule::new(rules::Allowed::Delegates, self.threshold()), ); } #[allow(deprecated)] - raw_crefs - .symbolic_mut() + symbolic .combine(symbolic::SymbolicRefs::head(project.default_branch())) .map_err(|source| CanonicalRefsError::SynthesisCycle { source })?; - Ok(()) + Ok(RawCanonicalRefs::new(raw_rules, symbolic)) } /// Return the associated [`Visibility`] of this document. diff --git a/crates/radicle/src/identity/doc/update.rs b/crates/radicle/src/identity/doc/update.rs index 6e896a7f0..26d090cf2 100644 --- a/crates/radicle/src/identity/doc/update.rs +++ b/crates/radicle/src/identity/doc/update.rs @@ -208,21 +208,38 @@ pub fn verify(raw: RawDoc) -> Result { } }; + let resolve = &mut || proposal.delegates.clone(); + + let crefs = match proposal.raw_canonical_refs() { + None => None, + Some(Ok(crefs)) => match crefs.try_into_canonical_refs(resolve) { + Ok(crefs) => Some(crefs), + Err(err) => { + return Err(error::DocVerification::PayloadError { + id: PayloadId::canonical_refs().clone(), + err: err.to_string(), + }); + } + }, + Some(Err(PayloadError::Json(e))) => { + return Err(error::DocVerification::PayloadError { + id: PayloadId::canonical_refs().clone(), + err: e.to_string(), + }); + } + }; + // If we have both payloads `xyz.radicle.{project,crefs}` ensure that, // in the `crefs` payload there is no … // 1. … rule that matches the default branch from the `project` payload. // (This rule must be synthesized!) // 2. … symbolic reference with the name `HEAD`. // (This reference must be synthesized!) - match raw - .raw_canonical_refs() - .transpose() - .map(|rcrefs| rcrefs.zip(project)) - { - Ok(Some((crefs, project))) => { + match crefs.zip(project) { + Some((crefs, project)) => { let default = project.default_branch_qualified().to_owned(); let matches = crefs - .raw_rules() + .rules() .matches(&default) .map(|(pattern, _)| pattern.to_string()) .collect::>(); @@ -240,13 +257,6 @@ pub fn verify(raw: RawDoc) -> Result { _ => { /* we validate below */ } } - // Verify that the canonical references payload is valid - if let Err(e) = proposal.canonical_refs() { - return Err(error::DocVerification::PayloadError { - id: PayloadId::canonical_refs().clone(), - err: e.to_string(), - }); - } Ok(proposal) }