fetch: Rewrite `git::repository::direct` — heartwood

rad:z3gqcJUoA1n9HaHKufZs5FCSGazv5

Radicle Heartwood Protocol & Stack

fetch: Rewrite `git::repository::direct`

Merged lorenz opened 7 months ago

This function (and the helpers ancestry and find_and_peel) are eagerly peeling to commits, leading to updates of tags that target the same commit to be missed.

For example, there could be tag two tag objects with different OIDs A and B, from different authors, using different tag names, signed with different secret keys, and both pointing to the same commit C.

The implementation would consider A and B to be the same, just becuase A and B both peel to C, skipping the update.

This is counterintuitive, and when combined with canonical references can be quite confusing.

Change this to only reason about an ancestry if the two objects in question really both are commits directly. Otherwise, treat cases where no structure can be used as ancestry similarly to non-fast-forward updates.

Activity Changes +136 -65

2 files changed +136 -65 d9ce078d → 9c8ab7fa

modified crates/radicle-fetch/src/git/repository.rs

@@ -58,18 +58,33 @@ fn find_and_peel(repo: &Repository, oid: Oid) -> Result<Oid, error::Ancestry> {

     }
 }
 /// Peels the two objects to commits (see [`find_and_peel`]) and determines
 /// their ancestry relationship (see [`ahead_behind`]).
 pub fn ancestry(repo: &Repository, old: Oid, new: Oid) -> Result<Ancestry, error::Ancestry> {
     let old = find_and_peel(repo, old)?;
     let new = find_and_peel(repo, new)?;
     if old == new {
     ahead_behind(repo, old, new)
 }
 /// Determine the ancestry relationship between two commits.
 pub fn ahead_behind(
     repo: &Repository,
     old_commit: Oid,
     new_commit: Oid,
 ) -> Result<Ancestry, error::Ancestry> {
     if old_commit == new_commit {
         return Ok(Ancestry::Equal);
     }
     let (ahead, behind) = repo
         .backend
         .graph_ahead_behind(*new, *old)
         .map_err(|err| error::Ancestry::Check { old, new, err })?;
         .graph_ahead_behind(*new_commit, *old_commit)
         .map_err(|err| error::Ancestry::Check {
             old: old_commit,
             new: new_commit,
             err,
         })?;
     if ahead > 0 && behind == 0 {
         Ok(Ancestry::Ahead)

@@ -128,77 +143,130 @@ fn direct<'a>(

     target: Oid,
     no_ff: Policy,
 ) -> Result<Updated<'a>, error::Update> {
     let tip = refname_to_id(repo, name.clone())?;
     match tip {
         Some(prev) => {
             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(),
                             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,
                 }
                 .into()),
                 Ancestry::Diverged if matches!(no_ff, Policy::Reject) => Ok(Update::Direct {
                     name,
                     target,
                     no_ff,
                 }
                 .into()),
                 Ancestry::Diverged => Err(error::Update::NonFF {
                     name: name.to_owned(),
                     new: target,
                     cur: prev,
                 }),
     let Some(reference) = find(repo, &name)? else {
         repo.backend
             .reference(name.as_ref(), target.into(), false, "radicle: create")
             .map_err(|err| error::Update::Create {
                 name: name.to_owned(),
                 target,
                 err,
             })?;
         return Ok(RefUpdate::Created {
             name: name.to_ref_string(),
             oid: target,
         }
         .into());
     };
     let Some(prev) = reference.target() else {
         // This should never happen, as there are no facilities to create
         // symbolic references in Radicle namespaces. If it does, e.g. because
         // some external program or the user themselves created it, we better
         // do not touch it.
         return Err(error::Update::Symbolic {
             name: name.to_owned(),
         });
     };
     if prev == *target {
         // If the two objects are identical, their ancestry does not matter,
         // we can always skip the update.
         return Ok(RefUpdate::Skipped {
             name: name.to_ref_string(),
             oid: target,
         }
         .into());
     }
     let ancestry = {
         use git::raw::ObjectType::{self, *};
         const ANY_KIND: Option<ObjectType> = Some(Any);
         let prev = repo.backend.find_object(prev, ANY_KIND).map_err(|err| {
             error::Update::Ancestry(error::Ancestry::Object {
                 oid: prev.into(),
                 err,
             })
         })?;
         let target = repo
             .backend
             .find_object(*target, ANY_KIND)
             .map_err(|err| error::Update::Ancestry(error::Ancestry::Object { oid: target, err }))?;
         match (prev.kind(), target.kind()) {
             (Some(Commit), Some(Commit)) => {
                 // This is the common case, we have two commits to compare.
                 let prev = prev.id().into();
                 let target = target.id().into();
                 Some(ahead_behind(repo, prev, target)?)
             }
             (Some(Tag), Some(Tag)) => {
                 // Even though these tags might point to the same commit,
                 // refuse to peel, because that tag itself has changed
                 // (e.g. its name or signature).
                 None
             }
             (Some(Commit | Tag), Some(Commit | Tag)) => {
                 // The reference changes from a commit to a tag or vice versa.
                 None
             }
             _ => {
                 // One of the objects is not a commit or a tag, we're clueless.
                 None
             }
         }
         None => {
             // N.b. the reference didn't exist so we pass `force:
             // false`.
     };
     match ancestry {
         Some(Ancestry::Equal) => Ok(RefUpdate::Skipped {
             name: name.to_ref_string(),
             oid: target,
         }
         .into()),
         Some(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(), false, "radicle: create")
                 .reference(name.as_ref(), target.into(), true, "radicle: update")
                 .map_err(|err| error::Update::Create {
                     name: name.to_owned(),
                     target,
                     err,
                 })?;
             Ok(RefUpdate::Created {
                 name: name.to_ref_string(),
                 oid: target,
             }
             .into())
             Ok(RefUpdate::from(name.to_ref_string(), prev, target).into())
         }
         Some(Ancestry::Behind | Ancestry::Diverged) | None 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(),
                     target,
                     err,
                 })?;
             Ok(RefUpdate::from(name.to_ref_string(), prev, target).into())
         }
         // N.b. if the target is behind, we simply reject the update
         Some(Ancestry::Behind) => Ok(Update::Direct {
             name,
             target,
             no_ff,
         }
         .into()),
         Some(Ancestry::Diverged) | None if matches!(no_ff, Policy::Reject) => Ok(Update::Direct {
             name,
             target,
             no_ff,
         }
         .into()),
         Some(Ancestry::Diverged) | None => Err(error::Update::NonFF {
             name: name.to_owned(),
             new: target,
             cur: prev.into(),
         }),
     }
 }

modified crates/radicle-fetch/src/git/repository/error.rs

@@ -79,4 +79,7 @@ pub enum Update {

     Peel(#[source] raw::Error),
     #[error(transparent)]
     Resolve(#[from] Resolve),
     #[error("refusing to update symbolic ref {name}")]
     Symbolic { name: Namespaced<'static> },
 }