From 9c8ab7fa627f7da7235c6f0b04e5c607640f0a32 Mon Sep 17 00:00:00 2001 From: Lorenz Leutgeb Date: Tue, 30 Sep 2025 13:13:46 +0200 Subject: [PATCH] fetch: Rewrite `git::repository::direct` 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. --- crates/radicle-fetch/src/git/repository.rs | 196 ++++++++++++------ .../radicle-fetch/src/git/repository/error.rs | 3 + 2 files changed, 135 insertions(+), 64 deletions(-) diff --git a/crates/radicle-fetch/src/git/repository.rs b/crates/radicle-fetch/src/git/repository.rs index 806e3e34..8c32f5e0 100644 --- a/crates/radicle-fetch/src/git/repository.rs +++ b/crates/radicle-fetch/src/git/repository.rs @@ -58,18 +58,33 @@ fn find_and_peel(repo: &Repository, oid: Oid) -> Result { } } +/// 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 { 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 { + 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, error::Update> { - let tip = refname_to_id(repo, name.clone())?; - match tip { - Some(prev) => { - let ancestry = ancestry(repo, prev, target)?; + 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, + })?; - 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, - }), + 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 = 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(), + }), } } diff --git a/crates/radicle-fetch/src/git/repository/error.rs b/crates/radicle-fetch/src/git/repository/error.rs index 6c2d546c..268e8f6f 100644 --- a/crates/radicle-fetch/src/git/repository/error.rs +++ b/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> }, }