From 91914d9345e75fbcc1922e5dcbe1e0da1f2c21c5 Mon Sep 17 00:00:00 2001 From: Fintan Halpenny Date: Wed, 6 Nov 2024 10:31:50 +0000 Subject: [PATCH] radicle: improve quorum copy The `no quorum found` error can be opaque and lead to a lot of confusion. It is better to give more information for this error so that it leads to easier debugging, since it can be quite a common warning/error. Instead of a single error, there are two cases provided: `NoCandidates` and `Diverging`, to capture the two different variants of errors that can occur. `Display` implementations are provided for both to provide more information. --- radicle-cli/examples/git/git-push-converge.md | 3 +- radicle-node/src/worker/fetch.rs | 10 +- radicle-remote-helper/src/push.rs | 14 +- radicle/src/git/canonical.rs | 185 ++++++++++++++---- 4 files changed, 166 insertions(+), 46 deletions(-) diff --git a/radicle-cli/examples/git/git-push-converge.md b/radicle-cli/examples/git/git-push-converge.md index 171baa1f..ef9f54aa 100644 --- a/radicle-cli/examples/git/git-push-converge.md +++ b/radicle-cli/examples/git/git-push-converge.md @@ -89,7 +89,8 @@ commit: ``` ~alice (stderr) $ git push rad -f -warn: no quorum was found for `refs/heads/master` +warn: could not determine canonical tip for `refs/heads/master` +warn: no commit found with at least 3 vote(s) (threshold not met) warn: it is recommended to find a commit to agree upon ✓ Synced with 2 node(s) To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi diff --git a/radicle-node/src/worker/fetch.rs b/radicle-node/src/worker/fetch.rs index c180efed..7e1f615e 100644 --- a/radicle-node/src/worker/fetch.rs +++ b/radicle-node/src/worker/fetch.rs @@ -7,7 +7,6 @@ use localtime::LocalTime; use radicle::cob::TypedId; use radicle::crypto::PublicKey; -use radicle::git::canonical::QuorumError; use radicle::identity::DocAt; use radicle::prelude::RepoId; use radicle::storage::refs::RefsAt; @@ -89,6 +88,8 @@ impl Handle { remote: PublicKey, refs_at: Option>, ) -> Result { + use git::canonical::QuorumError::{Diverging, NoCandidates}; + let (result, clone, notifs) = match self { Self::Clone { mut handle, tmp } => { log::debug!(target: "worker", "{} cloning from {remote}", handle.local()); @@ -143,8 +144,11 @@ impl Handle { log::trace!(target: "worker", "Set HEAD to {}", head.new); } } - Err(RepositoryError::Quorum(QuorumError::NoQuorum)) => { - log::warn!(target: "worker", "Fetch could not set HEAD: no quorum found") + Err(RepositoryError::Quorum(Diverging(e))) => { + log::warn!(target: "worker", "Fetch could not set HEAD: {e}") + } + Err(RepositoryError::Quorum(NoCandidates(e))) => { + log::warn!(target: "worker", "Fetch could not set HEAD: {e}") } Err(e) => return Err(e.into()), } diff --git a/radicle-remote-helper/src/push.rs b/radicle-remote-helper/src/push.rs index a9e8574d..12c8c09f 100644 --- a/radicle-remote-helper/src/push.rs +++ b/radicle-remote-helper/src/push.rs @@ -306,8 +306,18 @@ pub fn run( )); } } - Err(canonical::QuorumError::NoQuorum) => { - warn(format!("no quorum was found for `{canonical_ref}`")); + Err(canonical::QuorumError::Diverging(e)) => { + warn(format!( + "could not determine canonical tip for `{canonical_ref}`" + )); + warn(e.to_string()); + warn("it is recommended to find a commit to agree upon"); + } + Err(canonical::QuorumError::NoCandidates(e)) => { + warn(format!( + "could not determine canonical tip for `{canonical_ref}`" + )); + warn(e.to_string()); warn("it is recommended to find a commit to agree upon"); } Err(e) => return Err(e.into()), diff --git a/radicle/src/git/canonical.rs b/radicle/src/git/canonical.rs index 67b2cd18..c8637b82 100644 --- a/radicle/src/git/canonical.rs +++ b/radicle/src/git/canonical.rs @@ -1,4 +1,5 @@ use std::collections::BTreeMap; +use std::fmt; use nonempty::NonEmpty; use raw::Repository; @@ -27,14 +28,61 @@ pub struct Canonical { /// Error that can occur when calculation the [`Canonical::quorum`]. #[derive(Debug, Error)] pub enum QuorumError { - /// Could not determine a quorum [`Oid`]. - #[error("no quorum was found")] - NoQuorum, + /// Could not determine a quorum [`Oid`], due to diverging tips. + #[error("could not determine canonical reference tip, {0}")] + Diverging(Diverging), + /// Could not determine a base candidate from the given set of delegates. + #[error("could not determine canonical reference tip, {0}")] + NoCandidates(NoCandidates), /// An error occurred from [`git2`]. #[error(transparent)] Git(#[from] git2::Error), } +/// No candidates were found for the [`Canonical::quorum`] calculation. +/// +/// The [`fmt::Display`] is used in [`QuorumError`], to provide information on +/// the threshold and delegates in the calculation. +#[derive(Debug)] +pub struct NoCandidates { + threshold: usize, +} + +impl fmt::Display for NoCandidates { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let NoCandidates { threshold } = self; + write!( + f, + "no commit found with at least {threshold} vote(s) (threshold not met)" + ) + } +} + +/// Diverging commits were found during the [`Canonical::quorum`] calculation. +/// +/// The [`fmt::Display`] is used in [`QuorumError`], to provide information on +/// the threshold, base commit, and the two diverging commits, in the +/// calculation. +#[derive(Debug)] +pub struct Diverging { + threshold: usize, + base: Oid, + longest: Oid, + head: Oid, +} + +impl fmt::Display for Diverging { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let Diverging { + threshold, + base, + longest, + head, + } = self; + write!(f, "found diverging commits {longest} and {head}, with base commit {base} and threshold {threshold}") + } +} + impl Canonical { /// Construct the set of canonical tips of the `Project::default_branch` for /// the given `delegates`. @@ -150,8 +198,9 @@ impl Canonical { // Keep commits which pass the threshold. candidates.retain(|_, votes| *votes >= threshold); - // Keep track of the longest identity branch. - let (mut longest, _) = candidates.pop_first().ok_or(QuorumError::NoQuorum)?; + let (mut longest, _) = candidates + .pop_first() + .ok_or_else(|| QuorumError::NoCandidates(NoCandidates { threshold }))?; // Now that all scores are calculated, figure out what is the longest branch // that passes the threshold. In case of divergence, return an error. @@ -185,7 +234,12 @@ impl Canonical { // o (base) // | // - return Err(QuorumError::NoQuorum); + return Err(QuorumError::Diverging(Diverging { + threshold, + base: base.into(), + longest, + head: *head, + })); } } Ok((*longest).into()) @@ -281,8 +335,8 @@ mod tests { assert_eq!(quorum(&[*c1], 1, &repo).unwrap(), c1); assert_eq!(quorum(&[*c2], 1, &repo).unwrap(), c2); assert_eq!(quorum(&[*c0], 0, &repo).unwrap(), c0); - assert_matches!(quorum(&[], 0, &repo), Err(QuorumError::NoQuorum)); - assert_matches!(quorum(&[*c0], 2, &repo), Err(QuorumError::NoQuorum)); + assert_matches!(quorum(&[], 0, &repo), Err(QuorumError::NoCandidates(_))); + assert_matches!(quorum(&[*c0], 2, &repo), Err(QuorumError::NoCandidates(_))); // C1 // | @@ -308,19 +362,31 @@ mod tests { // C0 assert_matches!( quorum(&[*c1, *c2, *b2], 1, &repo), - Err(QuorumError::NoQuorum) + Err(QuorumError::Diverging(_)) + ); + assert_matches!( + quorum(&[*c2, *b2], 1, &repo), + Err(QuorumError::Diverging(_)) + ); + assert_matches!( + quorum(&[*b2, *c2], 1, &repo), + Err(QuorumError::Diverging(_)) + ); + assert_matches!( + quorum(&[*c2, *b2], 2, &repo), + Err(QuorumError::NoCandidates(_)) + ); + assert_matches!( + quorum(&[*b2, *c2], 2, &repo), + Err(QuorumError::NoCandidates(_)) ); - assert_matches!(quorum(&[*c2, *b2], 1, &repo), Err(QuorumError::NoQuorum)); - assert_matches!(quorum(&[*b2, *c2], 1, &repo), Err(QuorumError::NoQuorum)); - assert_matches!(quorum(&[*c2, *b2], 2, &repo), Err(QuorumError::NoQuorum)); - assert_matches!(quorum(&[*b2, *c2], 2, &repo), Err(QuorumError::NoQuorum)); assert_eq!(quorum(&[*c1, *c2, *b2], 2, &repo).unwrap(), c1); assert_eq!(quorum(&[*c1, *c2, *b2], 3, &repo).unwrap(), c1); assert_eq!(quorum(&[*b2, *b2, *c2], 2, &repo).unwrap(), b2); assert_eq!(quorum(&[*b2, *c2, *c2], 2, &repo).unwrap(), c2); assert_matches!( quorum(&[*b2, *b2, *c2, *c2], 2, &repo), - Err(QuorumError::NoQuorum) + Err(QuorumError::Diverging(_)) ); // B2 C2 C3 @@ -331,15 +397,15 @@ mod tests { assert_eq!(quorum(&[*b2, *c2, *c2], 2, &repo).unwrap(), c2); assert_matches!( quorum(&[*b2, *c2, *c2], 3, &repo), - Err(QuorumError::NoQuorum) + Err(QuorumError::NoCandidates(_)) ); assert_matches!( quorum(&[*b2, *c2, *b2, *c2], 3, &repo), - Err(QuorumError::NoQuorum) + Err(QuorumError::NoCandidates(_)) ); assert_matches!( quorum(&[*c3, *b2, *c2, *b2, *c2, *c3], 3, &repo), - Err(QuorumError::NoQuorum) + Err(QuorumError::NoCandidates(_)) ); // B2 C2 @@ -349,19 +415,19 @@ mod tests { // C0 assert_matches!( quorum(&[*c2, *b2, *a1], 1, &repo), - Err(QuorumError::NoQuorum) + Err(QuorumError::Diverging(_)) ); assert_matches!( quorum(&[*c2, *b2, *a1], 2, &repo), - Err(QuorumError::NoQuorum) + Err(QuorumError::NoCandidates(_)) ); assert_matches!( quorum(&[*c2, *b2, *a1], 3, &repo), - Err(QuorumError::NoQuorum) + Err(QuorumError::NoCandidates(_)) ); assert_matches!( quorum(&[*c1, *c2, *b2, *a1], 4, &repo), - Err(QuorumError::NoQuorum) + Err(QuorumError::NoCandidates(_)) ); assert_eq!(quorum(&[*c0, *c1, *c2, *b2, *a1], 2, &repo).unwrap(), c1,); assert_eq!(quorum(&[*c0, *c1, *c2, *b2, *a1], 3, &repo).unwrap(), c1,); @@ -369,23 +435,23 @@ mod tests { assert_eq!(quorum(&[*c0, *c1, *c2, *b2, *a1], 4, &repo).unwrap(), c0,); assert_matches!( quorum(&[*a1, *a1, *c2, *c2, *c1], 2, &repo), - Err(QuorumError::NoQuorum) + Err(QuorumError::Diverging(_)) ); assert_matches!( quorum(&[*a1, *a1, *c2, *c2, *c1], 1, &repo), - Err(QuorumError::NoQuorum) + Err(QuorumError::Diverging(_)) ); assert_matches!( quorum(&[*a1, *a1, *c2], 1, &repo), - Err(QuorumError::NoQuorum) + Err(QuorumError::Diverging(_)) ); assert_matches!( quorum(&[*b2, *b2, *c2, *c2], 1, &repo), - Err(QuorumError::NoQuorum) + Err(QuorumError::Diverging(_)) ); assert_matches!( quorum(&[*b2, *b2, *c2, *c2, *a1], 1, &repo), - Err(QuorumError::NoQuorum) + Err(QuorumError::Diverging(_)) ); // M2 M1 @@ -396,15 +462,30 @@ mod tests { // \| // C0 assert_eq!(quorum(&[*m1], 1, &repo).unwrap(), m1); - assert_matches!(quorum(&[*m1, *m2], 1, &repo), Err(QuorumError::NoQuorum)); - assert_matches!(quorum(&[*m2, *m1], 1, &repo), Err(QuorumError::NoQuorum)); - assert_matches!(quorum(&[*m1, *m2], 2, &repo), Err(QuorumError::NoQuorum)); + assert_matches!( + quorum(&[*m1, *m2], 1, &repo), + Err(QuorumError::Diverging(_)) + ); + assert_matches!( + quorum(&[*m2, *m1], 1, &repo), + Err(QuorumError::Diverging(_)) + ); + assert_matches!( + quorum(&[*m1, *m2], 2, &repo), + Err(QuorumError::NoCandidates(_)) + ); assert_matches!( quorum(&[*m1, *m2, *c2], 1, &repo), - Err(QuorumError::NoQuorum) + Err(QuorumError::Diverging(_)) + ); + assert_matches!( + quorum(&[*m1, *a1], 1, &repo), + Err(QuorumError::Diverging(_)) + ); + assert_matches!( + quorum(&[*m1, *a1], 2, &repo), + Err(QuorumError::NoCandidates(_)) ); - assert_matches!(quorum(&[*m1, *a1], 1, &repo), Err(QuorumError::NoQuorum)); - assert_matches!(quorum(&[*m1, *a1], 2, &repo), Err(QuorumError::NoQuorum)); assert_eq!(quorum(&[*m1, *m2, *b2, *c1], 4, &repo).unwrap(), c1); assert_eq!(quorum(&[*m1, *m1, *b2], 2, &repo).unwrap(), m1); assert_eq!(quorum(&[*m1, *m1, *c2], 2, &repo).unwrap(), m1); @@ -440,8 +521,14 @@ mod tests { // C1 C2 C3 // \| / // C0 - assert_matches!(quorum(&[*m1, *m2], 1, &repo), Err(QuorumError::NoQuorum)); - assert_matches!(quorum(&[*m1, *m2], 2, &repo), Err(QuorumError::NoQuorum)); + assert_matches!( + quorum(&[*m1, *m2], 1, &repo), + Err(QuorumError::Diverging(_)) + ); + assert_matches!( + quorum(&[*m1, *m2], 2, &repo), + Err(QuorumError::NoCandidates(_)) + ); let m3 = fixtures::commit("M3", &[*c2, *c1], &repo); @@ -450,11 +537,29 @@ mod tests { // C1 C2 C3 // \| / // C0 - assert_matches!(quorum(&[*m1, *m3], 1, &repo), Err(QuorumError::NoQuorum)); - assert_matches!(quorum(&[*m1, *m3], 2, &repo), Err(QuorumError::NoQuorum)); - assert_matches!(quorum(&[*m3, *m1], 1, &repo), Err(QuorumError::NoQuorum)); - assert_matches!(quorum(&[*m3, *m1], 2, &repo), Err(QuorumError::NoQuorum)); - assert_matches!(quorum(&[*m3, *m2], 1, &repo), Err(QuorumError::NoQuorum)); - assert_matches!(quorum(&[*m3, *m2], 2, &repo), Err(QuorumError::NoQuorum)); + assert_matches!( + quorum(&[*m1, *m3], 1, &repo), + Err(QuorumError::Diverging(_)) + ); + assert_matches!( + quorum(&[*m1, *m3], 2, &repo), + Err(QuorumError::NoCandidates(_)) + ); + assert_matches!( + quorum(&[*m3, *m1], 1, &repo), + Err(QuorumError::Diverging(_)) + ); + assert_matches!( + quorum(&[*m3, *m1], 2, &repo), + Err(QuorumError::NoCandidates(_)) + ); + assert_matches!( + quorum(&[*m3, *m2], 1, &repo), + Err(QuorumError::Diverging(_)) + ); + assert_matches!( + quorum(&[*m3, *m2], 2, &repo), + Err(QuorumError::NoCandidates(_)) + ); } }