remote-helper: improve canonical handling

This set of changes refactors the canonical handling by isolating the business
logic into another type, `Canonical`.

The functionality remains the same, but certain error scenarios are better
handled. Primarily, the commits of the inner `Canonical` type are checked to see
if they exist in the working copy, and if not, provide a more useful message
about which commit was missing or invalid for which DID.

crates/radicle-cli/examples/git/git-push-diverge.md

@@ -45,7 +45,7 @@ integrate Bob's changes before pushing ours:
 $ git push rad
 hint: you are attempting to push a commit that would cause your upstream to diverge from the canonical reference refs/heads/master
 hint: to integrate the remote changes, run `git pull --rebase` and try again
-error: refusing to update branch to commit that is not a descendant of canonical head
+error: refusing to update canonical reference to commit that is not a descendant of current canonical head
 error: failed to push some refs to 'rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi'
 ```
 

crates/radicle-remote-helper/src/push.rs

@@ -1,5 +1,6 @@
 #![allow(clippy::too_many_arguments)]
 
+mod canonical;
 mod error;
 
 use std::collections::HashMap;
@@ -19,7 +20,6 @@ use radicle::cob::patch;
 use radicle::cob::patch::cache::Patches as _;
 use radicle::crypto;
 use radicle::explorer::ExplorerResource;
-use radicle::git::canonical;
 use radicle::identity::{CanonicalRefs, Did};
 use radicle::node;
 use radicle::node::{Handle, NodeId};
@@ -31,7 +31,7 @@ use radicle::{git, rad};
 use radicle_cli as cli;
 use radicle_cli::terminal as term;
 
-use crate::{hint, read_line, warn, Options};
+use crate::{hint, read_line, Options};
 
 #[derive(Debug, Error)]
 pub enum Error {
@@ -45,9 +45,6 @@ pub enum Error {
     /// No public key is given
     #[error("no public key given as a remote namespace, perhaps you are attempting to push to restricted refs")]
     NoKey,
-    /// Head being pushed diverges from canonical head.
-    #[error("refusing to update branch to commit that is not a descendant of canonical head")]
-    HeadsDiverge(git::Oid, git::Oid),
     /// User tried to delete the canonical branch.
     #[error("refusing to delete default branch ref '{0}'")]
     DeleteForbidden(git::RefString),
@@ -121,6 +118,8 @@ pub enum Error {
     CanonicalRefs(#[from] radicle::identity::doc::CanonicalRefsError),
     #[error(transparent)]
     PushAction(#[from] error::PushAction),
+    #[error(transparent)]
+    Canonical(#[from] error::CanonicalUnrecoverable),
 }
 
 /// Push command.
@@ -224,7 +223,7 @@ impl PushAction {
                 let patch = git::Oid::from_str(oid)
                     .map_err(|err| error::PushAction::InvalidPatchId {
                         suffix: oid.to_string(),
-                        err,
+                        source: err,
                     })
                     .map(patch::PatchId::from)?;
                 Ok(Self::UpdatePatch { dst, patch })
@@ -348,46 +347,11 @@ pub fn run(
                         //
                         // Note that we *do* allow rolling back to a previous commit on the
                         // canonical branch.
-                        if let Some(mut canonical) = rules.canonical(dst.clone(), stored)? {
+                        if let Some(canonical) = rules.canonical(dst.clone(), stored)? {
-                            if canonical.is_allowed(&me) {
+                            let canonical = canonical::Canonical::new(me, *src, canonical);
-                                let head = *src;
-                                let converges = canonical.converges(&working, (&me, &head))?;
-
-                                // If `canonical` is empty then we're creating a new reference.
-                                // If we're the only delegate then we need to modify our vote.
-                                if converges || canonical.has_no_tips() || canonical.is_only(&me) {
-                                    canonical.modify_vote(me, head);
-                                }
-
                             match canonical.quorum(&working) {
-                                    Ok((dst, canonical_oid)) => {
+                                Ok(quorum) => set_canonical_refs.push(quorum),
-                                        // Canonical head is an ancestor of head.
+                                Err(e) => canonical::io::handle_error(e, &dst, hints)?,
-                                        let is_ff = head == canonical_oid
-                                            || working
-                                                .graph_descendant_of(*head, *canonical_oid)?;
-
-                                        if !is_ff && !converges {
-                                            if hints {
-                                                hint(
-                                                    format!("you are attempting to push a commit that would cause \
-                                                    your upstream to diverge from the canonical reference {dst}"),
-                                                );
-                                                hint(
-                                                    "to integrate the remote changes, run `git pull --rebase` \
-                                                    and try again",
-                                                );
-                                            }
-                                            return Err(Error::HeadsDiverge(head, canonical_oid));
-                                        }
-                                        set_canonical_refs
-                                            .push((dst.clone().to_owned(), canonical_oid));
-                                    }
-                                    Err(canonical::QuorumError::Git(e)) => return Err(e.into()),
-                                    Err(e) => {
-                                        warn(e.to_string());
-                                        warn("it is recommended to find a commit to agree upon");
-                                    }
-                                }
                             }
                         }
                         push(src, &dst, *force, &nid, &working, stored, patches, &signer)

crates/radicle-remote-helper/src/push/canonical.rs

@@ -0,0 +1,129 @@
+use radicle::git;
+use radicle::git::raw::Repository;
+use radicle::prelude::Did;
+
+use super::error;
+
+pub(crate) struct Vote {
+    did: Did,
+    commit: git::Oid,
+}
+
+/// Validates a vote to update a canonical reference during push.
+pub(crate) struct Canonical<'a, 'b> {
+    vote: Vote,
+    canonical: git::canonical::Canonical<'a, 'b>,
+}
+
+impl<'a, 'b> Canonical<'a, 'b> {
+    pub fn new(me: Did, head: git::Oid, canonical: git::canonical::Canonical<'a, 'b>) -> Self {
+        Self {
+            vote: Vote {
+                did: me,
+                commit: head,
+            },
+            canonical,
+        }
+    }
+
+    /// Calculates the quorum of the [`git::canonical::Canonical`] provided.
+    ///
+    /// In some cases, it ensures that the [`head`] is attempting to converge
+    /// with the set of commits of the other [`Did`]s.
+    ///
+    /// If a quorum is found, then it is also ensured that the new [`head`] is a
+    /// descendant of the current canonical commit, otherwise the commits are
+    /// considered diverging.
+    ///
+    /// # Errors
+    ///
+    /// Ensures that the commits of the other [`Did`]s are in the working
+    /// copy, and that checks that any two commits are related in the graph.
+    ///
+    /// Ensures that the new head and the canonical commit do not diverge.
+    ///
+    /// [`head`]: crate::push::canonical::Canonical::head
+    pub fn quorum(
+        mut self,
+        working: &Repository,
+    ) -> Result<(git::Qualified<'a>, git::Oid), error::Canonical> {
+        let converges = self
+            .canonical
+            .converges(working, (&self.vote.did, &self.vote.commit))?;
+        if converges || self.canonical.has_no_tips() || self.canonical.is_only(&self.vote.did) {
+            self.canonical.modify_vote(self.vote.did, self.vote.commit);
+        }
+
+        match self.canonical.quorum(working) {
+            Ok((cref, quorum_head)) => {
+                // Canonical head is an ancestor of head.
+                let is_ff = self.vote.commit == quorum_head
+                    || working
+                        .graph_descendant_of(*self.vote.commit, *quorum_head)
+                        .map_err(|err| {
+                            error::Canonical::graph_descendant(self.vote.commit, quorum_head, err)
+                        })?;
+
+                if !is_ff && !converges {
+                    Err(error::Canonical::heads_diverge(
+                        self.vote.commit,
+                        quorum_head,
+                    ))
+                } else {
+                    Ok((cref, quorum_head))
+                }
+            }
+            Err(err) => Err(err.into()),
+        }
+    }
+}
+
+pub(crate) mod io {
+    use radicle::git;
+    use radicle::git::canonical;
+
+    use crate::push::error;
+    use crate::{hint, warn};
+
+    /// Handle recoverable errors, printing relevant information to the
+    /// terminal. Otherwise, convert the error into an unrecoverable error
+    /// [`error::CanonicalUnrecoverable`].
+    pub fn handle_error(
+        e: error::Canonical,
+        canonical: &git::Qualified,
+        hints: bool,
+    ) -> Result<(), error::CanonicalUnrecoverable> {
+        match e {
+            error::Canonical::GraphDescendant(e) => Err(e.into()),
+            error::Canonical::Converges(e) => Err(e.into()),
+            error::Canonical::HeadsDiverge(e) => {
+                if hints {
+                    hint(
+                        format!("you are attempting to push a commit that would cause \
+                                                 your upstream to diverge from the canonical reference {canonical}"),
+                    );
+                    hint(
+                        "to integrate the remote changes, run `git pull --rebase` \
+                                                 and try again",
+                    );
+                }
+                Err(e.into())
+            }
+            error::Canonical::Quorum(e) => match e {
+                e @ canonical::QuorumError::Diverging { .. } => {
+                    warn(e.to_string());
+                    warn("it is recommended to find a commit to agree upon");
+                    Ok(())
+                }
+                e @ canonical::QuorumError::NoCandidates { .. } => {
+                    warn(e.to_string());
+                    warn("it is recommended to find a commit to agree upon");
+                    Ok(())
+                }
+                canonical::QuorumError::Git(err) => {
+                    Err(error::CanonicalUnrecoverable::Git { source: err })
+                }
+            },
+        }
+    }
+}

crates/radicle-remote-helper/src/push/error.rs

@@ -1,6 +1,61 @@
-use radicle::storage::git;
+use radicle::git;
+use radicle::git::canonical;
 use thiserror::Error;
 
+#[derive(Debug, Error)]
+pub enum CanonicalUnrecoverable {
+    #[error(transparent)]
+    GraphDescendant(#[from] GraphDescendant),
+    #[error(transparent)]
+    Converges(#[from] canonical::ConvergesError),
+    #[error(transparent)]
+    HeadsDiverge(#[from] HeadsDiverge),
+    #[error("failure while computing canonical reference: {source}")]
+    Git { source: git::raw::Error },
+}
+
+#[derive(Debug, Error)]
+pub enum Canonical {
+    #[error(transparent)]
+    GraphDescendant(GraphDescendant),
+    #[error(transparent)]
+    Converges(#[from] canonical::ConvergesError),
+    #[error(transparent)]
+    HeadsDiverge(HeadsDiverge),
+    #[error(transparent)]
+    Quorum(#[from] canonical::QuorumError),
+}
+
+impl Canonical {
+    pub fn graph_descendant(head: git::Oid, canonical: git::Oid, source: git::raw::Error) -> Self {
+        Self::GraphDescendant(GraphDescendant {
+            head,
+            canonical,
+            source,
+        })
+    }
+
+    pub fn heads_diverge(head: git::Oid, canonical: git::Oid) -> Self {
+        Self::HeadsDiverge(HeadsDiverge { head, canonical })
+    }
+}
+
+#[derive(Debug, Error)]
+#[error("failed to check if {head} is an ancestor of {canonical} due to: {source}")]
+pub struct GraphDescendant {
+    head: git::Oid,
+    canonical: git::Oid,
+    source: git::raw::Error,
+}
+
+#[derive(Debug, Error)]
+/// Head being pushed diverges from canonical head.
+#[error("refusing to update canonical reference to commit that is not a descendant of current canonical head")]
+pub struct HeadsDiverge {
+    head: git::Oid,
+    canonical: git::Oid,
+}
+
 #[derive(Debug, Error)]
 pub enum PushAction {
     #[error("invalid reference {refname}, expected qualified reference starting with `refs/`")]
@@ -8,7 +63,6 @@ pub enum PushAction {
     #[error("found refs/heads/patches/{suffix} where {suffix} was an invalid Patch ID")]
     InvalidPatchId {
         suffix: String,
-        #[source]
+        source: git::raw::Error,
-        err: git::raw::Error,
     },
 }

crates/radicle/src/git/canonical.rs

@@ -2,6 +2,7 @@ pub mod rules;
 pub use rules::{MatchedRule, RawRule, Rules, ValidRule};
 
 use std::collections::BTreeMap;
+use std::path::PathBuf;
 
 use raw::Repository;
 use thiserror::Error;
@@ -50,6 +51,70 @@ pub enum QuorumError {
     Git(#[from] git2::Error),
 }
 
+#[derive(Debug, Error)]
+#[error("failed to check if {head} is an ancestor of {canonical} due to: {source}")]
+pub struct GraphDescendant {
+    head: Oid,
+    canonical: Oid,
+    source: raw::Error,
+}
+
+#[derive(Debug, Error)]
+#[error("the commit {commit} for {did} is missing from the repository {repo:?}")]
+pub struct MissingCommit {
+    repo: PathBuf,
+    did: Did,
+    commit: Oid,
+    source: raw::Error,
+}
+
+#[derive(Debug, Error)]
+#[error("could not determine whether the commit {commit} for {did} is part of the repository {repo:?} due to: {source}")]
+pub struct InvalidCommit {
+    repo: PathBuf,
+    did: Did,
+    commit: Oid,
+    source: raw::Error,
+}
+
+#[derive(Debug, Error)]
+pub enum ConvergesError {
+    #[error(transparent)]
+    GraphDescendant(#[from] GraphDescendant),
+    #[error(transparent)]
+    MissingCommit(#[from] MissingCommit),
+    #[error(transparent)]
+    InvalidCommit(#[from] InvalidCommit),
+}
+
+impl ConvergesError {
+    pub fn graph_descendant(head: Oid, canonical: Oid, source: raw::Error) -> Self {
+        Self::GraphDescendant(GraphDescendant {
+            head,
+            canonical,
+            source,
+        })
+    }
+
+    pub fn missing_commit(repo: PathBuf, did: Did, commit: Oid, err: raw::Error) -> Self {
+        Self::MissingCommit(MissingCommit {
+            repo,
+            did,
+            commit,
+            source: err,
+        })
+    }
+
+    pub fn invalid_commit(repo: PathBuf, did: Did, commit: Oid, err: raw::Error) -> Self {
+        Self::InvalidCommit(InvalidCommit {
+            repo,
+            did,
+            commit,
+            source: err,
+        })
+    }
+}
+
 impl<'a, 'b> Canonical<'a, 'b> {
     /// Construct the set of canonical tips given for the given `rule` and
     /// the reference `refname`.
@@ -124,14 +189,25 @@ impl<'a, 'b> Canonical<'a, 'b> {
     pub fn converges(
         &self,
         repo: &Repository,
-        candidate: (&Did, &Oid),
+        (candidate, commit): (&Did, &Oid),
-    ) -> Result<bool, raw::Error> {
+    ) -> Result<bool, ConvergesError> {
-        for tip in self
+        let heads = {
+            let mut heads = self
                 .tips
                 .iter()
-            .filter_map(|(did, tip)| (did != candidate.0).then_some(tip))
+                .filter_map(|(did, tip)| (did != candidate).then_some((did, tip)));
-        {
+            heads.try_fold(
-            let (ahead, behind) = repo.graph_ahead_behind(**candidate.1, **tip)?;
+                Vec::with_capacity(heads.size_hint().0),
+                |mut heads, (did, head)| {
+                    heads.push(Self::ensure_commit(*did, *head, repo)?);
+                    Ok::<_, ConvergesError>(heads)
+                },
+            )?
+        };
+        for head in heads {
+            let (ahead, behind) = repo
+                .graph_ahead_behind(**commit, *head)
+                .map_err(|err| ConvergesError::graph_descendant(*commit, head, err))?;
             if ahead * behind == 0 {
                 return Ok(true);
             }
@@ -225,6 +301,21 @@ impl<'a, 'b> Canonical<'a, 'b> {
     fn threshold(&self) -> usize {
         (*self.rule.threshold()).into()
     }
+
+    fn ensure_commit(from: Did, commit: Oid, working: &Repository) -> Result<Oid, ConvergesError> {
+        match working.find_commit(*commit).map(|_| commit) {
+            Ok(oid) => Ok(oid),
+            Err(err) if err.code() == raw::ErrorCode::NotFound => Err(
+                ConvergesError::missing_commit(working.path().to_path_buf(), from, commit, err),
+            ),
+            Err(err) => Err(ConvergesError::invalid_commit(
+                working.path().to_path_buf(),
+                from,
+                commit,
+                err,
+            )),
+        }
+    }
 }
 
 #[cfg(test)]