Radish alpha
h
Radicle Heartwood Protocol & Stack
Radicle
Git (anonymous pull)
Log in to clone via SSH
fetch: Ignore sigrefs that are behind
Fintan Halpenny committed 2 years ago
commit 703a6d80250d7cac6584811fc4553746b4e46bd7
parent 9b7c48ab401cd4310f8cdc8bea4ed6b4235cccd2
4 files changed +236 -59
modified radicle-fetch/src/git/repository.rs
@@ -1,15 +1,36 @@
pub mod error;

-
use either::{
-
    Either,
-
    Either::{Left, Right},
-
};
+
use either::Either;
use radicle::git::{Namespaced, Oid, Qualified};
use radicle::storage::git::Repository;
-
use radicle::storage::ReadRepository;

use super::refs::{Applied, Policy, RefUpdate, Update};

+
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+
pub enum Ancestry {
+
    Equal,
+
    Ahead,
+
    Behind,
+
    Diverged,
+
}
+

+
pub enum Updated<'a> {
+
    Accepted(RefUpdate),
+
    Rejected(Update<'a>),
+
}
+

+
impl<'a> From<RefUpdate> for Updated<'a> {
+
    fn from(up: RefUpdate) -> Self {
+
        Updated::Accepted(up)
+
    }
+
}
+

+
impl<'a> From<Update<'a>> for Updated<'a> {
+
    fn from(up: Update<'a>) -> Self {
+
        Updated::Rejected(up)
+
    }
+
}
+

pub fn contains(repo: &Repository, oid: Oid) -> Result<bool, error::Contains> {
    repo.backend
        .odb()
@@ -17,17 +38,27 @@ pub fn contains(repo: &Repository, oid: Oid) -> Result<bool, error::Contains> {
        .map_err(error::Contains)
}

-
pub fn is_in_ancestry_path(repo: &Repository, old: Oid, new: Oid) -> Result<bool, error::Ancestry> {
+
pub fn ancestry(repo: &Repository, old: Oid, new: Oid) -> Result<Ancestry, error::Ancestry> {
    if !contains(repo, old)? || !contains(repo, new)? {
-
        return Ok(false);
+
        return Err(error::Ancestry::Missing { a: old, b: new });
    }

    if old == new {
-
        return Ok(true);
+
        return Ok(Ancestry::Equal);
    }

-
    repo.is_ancestor_of(old, new)
-
        .map_err(|err| error::Ancestry::Check { old, new, err })
+
    let (ahead, behind) = repo
+
        .backend
+
        .graph_ahead_behind(new.into(), old.into())
+
        .map_err(|err| error::Ancestry::Check { old, new, err })?;
+

+
    if ahead > 0 && behind == 0 {
+
        Ok(Ancestry::Ahead)
+
    } else if ahead == 0 && behind > 0 {
+
        Ok(Ancestry::Behind)
+
    } else {
+
        Ok(Ancestry::Diverged)
+
    }
}

pub fn refname_to_id<'a, N>(repo: &Repository, refname: N) -> Result<Option<Oid>, error::Resolve>
@@ -59,12 +90,12 @@ where
                target,
                no_ff,
            } => match direct(repo, name, target, no_ff)? {
-
                Left(r) => applied.rejected.push(r),
-
                Right(u) => applied.updated.push(u),
+
                Updated::Rejected(r) => applied.rejected.push(r),
+
                Updated::Accepted(u) => applied.updated.push(u),
            },
            Update::Prune { name, prev } => match prune(repo, name, prev)? {
-
                Left(r) => applied.rejected.push(r),
-
                Right(u) => applied.updated.push(u),
+
                Updated::Rejected(r) => applied.rejected.push(r),
+
                Updated::Accepted(u) => applied.updated.push(u),
            },
        }
    }
@@ -77,49 +108,62 @@ fn direct<'a>(
    name: Namespaced<'a>,
    target: Oid,
    no_ff: Policy,
-
) -> Result<Either<Update<'a>, RefUpdate>, error::Update> {
+
) -> Result<Updated<'a>, error::Update> {
    let tip = refname_to_id(repo, name.clone())?;
    match tip {
        Some(prev) => {
-
            let is_ff = is_in_ancestry_path(repo, prev, target)?;
-
            if !is_ff {
-
                match no_ff {
-
                    Policy::Abort => {
-
                        return Err(error::Update::NonFF {
+
            let ancestry = ancestry(repo, prev, target)?;
+

+
            match ancestry {
+
                Ancestry::Equal => Ok(RefUpdate::Skipped {
+
                    name: name.to_ref_string(),
+
                    oid: target,
+
                }
+
                .into()),
+
                Ancestry::Ahead => {
+
                    // N.b. the update is a fast-forward so we can safely
+
                    // pass `force: true`.
+
                    repo.backend
+
                        .reference(name.as_ref(), target.into(), true, "radicle: update")
+
                        .map_err(|err| error::Update::Create {
+
                            name: name.to_owned(),
+
                            target,
+
                            err,
+
                        })?;
+
                    Ok(RefUpdate::from(name.to_ref_string(), prev, target).into())
+
                }
+
                Ancestry::Behind | Ancestry::Diverged if matches!(no_ff, Policy::Allow) => {
+
                    // N.b. the update is a non-fast-forward but
+
                    // we allow it, so we pass `force: true`.
+
                    repo.backend
+
                        .reference(name.as_ref(), target.into(), true, "radicle: update")
+
                        .map_err(|err| error::Update::Create {
                            name: name.to_owned(),
-
                            new: target,
-
                            cur: prev,
-
                        })
-
                    }
-
                    Policy::Reject => Ok(Left(Update::Direct {
-
                        name,
-
                        target,
-
                        no_ff,
-
                    })),
-
                    Policy::Allow => {
-
                        // N.b. the update is a non-fast-forward but
-
                        // we allow it, so we pass `force: true`.
-
                        repo.backend
-
                            .reference(name.as_ref(), target.into(), true, "radicle: update")
-
                            .map_err(|err| error::Update::Create {
-
                                name: name.to_owned(),
-
                                target,
-
                                err,
-
                            })?;
-
                        Ok(Right(RefUpdate::from(name.to_ref_string(), prev, target)))
-
                    }
+
                            target,
+
                            err,
+
                        })?;
+
                    Ok(RefUpdate::from(name.to_ref_string(), prev, target).into())
+
                }
+
                // N.b. if the target is behind, we simply reject the update
+
                Ancestry::Behind => Ok(Update::Direct {
+
                    name,
+
                    target,
+
                    no_ff,
                }
-
            } else {
-
                // N.b. the update is a fast-forward so we can safely
-
                // pass `force: true`.
-
                repo.backend
-
                    .reference(name.as_ref(), target.into(), true, "radicle: update")
-
                    .map_err(|err| error::Update::Create {
+
                .into()),
+
                Ancestry::Diverged if matches!(no_ff, Policy::Reject) => Ok(Update::Direct {
+
                    name,
+
                    target,
+
                    no_ff,
+
                }
+
                .into()),
+
                Ancestry::Diverged => {
+
                    return Err(error::Update::NonFF {
                        name: name.to_owned(),
-
                        target,
-
                        err,
-
                    })?;
-
                Ok(Right(RefUpdate::from(name.to_ref_string(), prev, target)))
+
                        new: target,
+
                        cur: prev,
+
                    })
+
                }
            }
        }
        None => {
@@ -132,10 +176,11 @@ fn direct<'a>(
                    target,
                    err,
                })?;
-
            Ok(Right(RefUpdate::Created {
+
            Ok(RefUpdate::Created {
                name: name.to_ref_string(),
                oid: target,
-
            }))
+
            }
+
            .into())
        }
    }
}
@@ -144,7 +189,7 @@ fn prune<'a>(
    repo: &Repository,
    name: Namespaced<'a>,
    prev: Either<Oid, Qualified<'a>>,
-
) -> Result<Either<Update<'a>, RefUpdate>, error::Update> {
+
) -> Result<Updated<'a>, error::Update> {
    use radicle::git::raw::ObjectType;

    match find(repo, &name)? {
@@ -160,12 +205,13 @@ fn prune<'a>(
                name: name.to_owned(),
                err,
            })?;
-
            Ok(Right(RefUpdate::Deleted {
+
            Ok(RefUpdate::Deleted {
                name: name.to_ref_string(),
                oid: prev,
-
            }))
+
            }
+
            .into())
        }
-
        None => Ok(Left(Update::Prune { name, prev })),
+
        None => Ok(Update::Prune { name, prev }.into()),
    }
}

modified radicle-fetch/src/git/repository/error.rs
@@ -7,6 +7,8 @@ pub struct Contains(#[source] pub raw::Error);

#[derive(Debug, Error)]
pub enum Ancestry {
+
    #[error("missing one of {a} or {b} while checking ancestry")]
+
    Missing { a: Oid, b: Oid },
    #[error(transparent)]
    Contains(#[from] Contains),
    #[error("failed to check ancestry for {old} and {new}")]
@@ -14,7 +16,7 @@ pub enum Ancestry {
        old: Oid,
        new: Oid,
        #[source]
-
        err: ext::Error,
+
        err: raw::Error,
    },
}

modified radicle-fetch/src/state.rs
@@ -29,6 +29,8 @@ pub const DEFAULT_FETCH_DATA_REFS_LIMIT: u64 = 1024 * 1024 * 1024 * 5;
pub mod error {
    use std::io;

+
    use radicle::git::Oid;
+
    use radicle::prelude::PublicKey;
    use thiserror::Error;

    use crate::{git, git::repository, handle, sigrefs, stage};
@@ -48,7 +50,15 @@ pub mod error {
    #[derive(Debug, Error)]
    pub enum Protocol {
        #[error(transparent)]
+
        Ancestry(#[from] repository::error::Ancestry),
+
        #[error(transparent)]
        Canonical(#[from] Canonical),
+
        #[error("delegate '{remote}' has diverged 'rad/sigrefs': {current} -> {received}")]
+
        Diverged {
+
            remote: PublicKey,
+
            current: Oid,
+
            received: Oid,
+
        },
        #[error(transparent)]
        Io(#[from] io::Error),
        #[error("canonical 'refs/rad/id' is missing")]
@@ -371,6 +381,20 @@ impl FetchState {
                    remote,
                    data: Some(sigrefs),
                } => {
+
                    if let Some(SignedRefsAt { at, .. }) = SignedRefsAt::load(remote, &handle.repo)?
+
                    {
+
                        // Prune non-delegates if they're behind or
+
                        // diverged. A diverged case is non-fatal for
+
                        // delegates.
+
                        if matches!(
+
                            repository::ancestry(&handle.repo, at, sigrefs.at)?,
+
                            repository::Ancestry::Behind | repository::Ancestry::Diverged
+
                        ) {
+
                            self.prune(&remote);
+
                            continue;
+
                        }
+
                    }
+

                    let cache = self.as_cached(handle);
                    if let Some(warns) = sigrefs::validate(&cache, sigrefs)?.as_mut() {
                        log::debug!(
@@ -385,6 +409,21 @@ impl FetchState {
                    remote,
                    data: Some(sigrefs),
                } => {
+
                    if let Some(SignedRefsAt { at, .. }) = SignedRefsAt::load(remote, &handle.repo)?
+
                    {
+
                        let ancestry = repository::ancestry(&handle.repo, at, sigrefs.at)?;
+
                        if matches!(ancestry, repository::Ancestry::Behind) {
+
                            self.prune(&remote);
+
                            continue;
+
                        } else if matches!(ancestry, repository::Ancestry::Diverged) {
+
                            return Err(error::Protocol::Diverged {
+
                                remote,
+
                                current: at,
+
                                received: sigrefs.at,
+
                            });
+
                        }
+
                    }
+

                    let cache = self.as_cached(handle);
                    if let Some(fails) = sigrefs::validate(&cache, sigrefs)?.as_mut() {
                        log::warn!(target: "fetch", "Pruning delegate {remote} tips, due to validation failures");
modified radicle-node/src/tests/e2e.rs
@@ -855,7 +855,7 @@ fn test_non_fastforward_sigrefs() {
    // Eve has old refs.
    assert_matches!(
        alice.handle.fetch(rid, eve.id, DEFAULT_TIMEOUT).unwrap(),
-
        FetchResult::Failed { .. }
+
        FetchResult::Success { updated, .. } if updated.is_empty()
    );
}

@@ -949,3 +949,93 @@ fn test_outdated_sigrefs() {
    assert_ne!(eves_refs, old_refs);
    assert_eq!(eves_refs_expected, eves_refs);
}
+

+
#[test]
+
fn test_outdated_delegate_sigrefs() {
+
    logger::init(log::Level::Debug);
+

+
    let tmp = tempfile::tempdir().unwrap();
+

+
    let mut alice = Node::init(tmp.path(), Config::test(Alias::new("alice")));
+
    let bob = Node::init(tmp.path(), Config::test(Alias::new("bob")));
+
    let eve = Node::init(tmp.path(), Config::test(Alias::new("eve")));
+

+
    let rid = alice.project("acme", "");
+

+
    let mut alice = alice.spawn();
+
    let mut bob = bob.spawn();
+
    let mut eve = eve.spawn();
+

+
    bob.handle.track_repo(rid, Scope::All).unwrap();
+
    eve.handle.track_repo(rid, Scope::All).unwrap();
+
    alice.connect(&bob);
+
    bob.connect(&eve);
+
    eve.connect(&alice);
+
    converge([&alice, &bob, &eve]);
+

+
    bob.handle.fetch(rid, alice.id, DEFAULT_TIMEOUT).unwrap();
+
    assert!(bob.storage.contains(&rid).unwrap());
+
    rad::fork(rid, &bob.signer, &bob.storage).unwrap();
+

+
    eve.handle.fetch(rid, alice.id, DEFAULT_TIMEOUT).unwrap();
+
    assert!(eve.storage.contains(&rid).unwrap());
+
    rad::fork(rid, &eve.signer, &eve.storage).unwrap();
+

+
    alice
+
        .handle
+
        .track_node(eve.id, Some(Alias::new("eve")))
+
        .unwrap();
+
    alice.handle.fetch(rid, eve.id, DEFAULT_TIMEOUT).unwrap();
+
    let repo = alice.storage.repository(rid).unwrap();
+
    assert!(repo.remote(&eve.id).is_ok());
+

+
    log::debug!(target: "test", "Bob fetches from Eve..");
+
    assert_matches!(
+
        bob.handle.fetch(rid, eve.id, DEFAULT_TIMEOUT).unwrap(),
+
        FetchResult::Success { .. }
+
    );
+
    let repo = bob.storage.repository(rid).unwrap();
+
    let alice_remote = repo.remote(&alice.id).unwrap();
+
    let old_refs = alice_remote.refs;
+

+
    // At this stage, Alice and Bob have Eve's fork and Eve does not
+
    // have Bob's fork
+

+
    alice.issue(
+
        rid,
+
        "Outdated Sigrefs",
+
        "Outdated sigrefs are harshing my vibes",
+
    );
+
    let repo = alice.storage.repository(rid).unwrap();
+
    let alice_refs = repo.remote(&alice.id).unwrap().refs;
+

+
    // Get the current state of eve's refs in alice's storage
+
    log::debug!(target: "test", "Alice fetches from Eve..");
+
    assert_matches!(
+
        eve.handle.fetch(rid, alice.id, DEFAULT_TIMEOUT).unwrap(),
+
        FetchResult::Success { .. }
+
    );
+
    let repo = eve.storage.repository(rid).unwrap();
+
    let alice_remote = repo.remote(&alice.id).unwrap();
+
    let alice_refs_expected = alice_remote.refs;
+
    assert_ne!(alice_refs_expected, old_refs);
+
    assert_eq!(alice_refs_expected, alice_refs);
+

+
    log::debug!(target: "test", "Alice fetches from Bob..");
+

+
    eve.handle
+
        .track_node(bob.id, Some(Alias::new("bob")))
+
        .unwrap();
+
    assert_matches!(
+
        eve.handle.fetch(rid, bob.id, DEFAULT_TIMEOUT).unwrap(),
+
        FetchResult::Success { .. }
+
    );
+

+
    // Ensure that Eve's refs have not changed after fetching the old refs from Bob.
+
    let repo = eve.storage.repository(rid).unwrap();
+
    let alice_remote = repo.remote(&alice.id).unwrap();
+
    let alice_refs = alice_remote.refs;
+

+
    assert_ne!(alice_refs, old_refs);
+
    assert_eq!(alice_refs_expected, alice_refs);
+
}