mab122
/
radicle-heartwood-lfs
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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.
This commit is contained in:
Lorenz Leutgeb 2025-09-30 13:13:46 +02:00
parent d9ce078d56
commit 9c8ab7fa62
No known key found for this signature in database
2 changed files with 135 additions and 64 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

196
crates/radicle-fetch/src/git/repository.rs
View File

@ -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> { pub fn ancestry(repo: &Repository, old: Oid, new: Oid) -> Result<Ancestry, error::Ancestry> {
let old = find_and_peel(repo, old)?; let old = find_and_peel(repo, old)?;
let new = find_and_peel(repo, new)?; 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); return Ok(Ancestry::Equal);
} }
let (ahead, behind) = repo let (ahead, behind) = repo
.backend .backend
.graph_ahead_behind(*new, *old) .graph_ahead_behind(*new_commit, *old_commit)
.map_err(|err| error::Ancestry::Check { old, new, err })?; .map_err(|err| error::Ancestry::Check {
old: old_commit,
new: new_commit,
err,
})?;
if ahead > 0 && behind == 0 { if ahead > 0 && behind == 0 {
Ok(Ancestry::Ahead) Ok(Ancestry::Ahead)
@ -128,77 +143,130 @@ fn direct<'a>(
target: Oid, target: Oid,
no_ff: Policy, no_ff: Policy,
) -> Result<Updated<'a>, error::Update> { ) -> Result<Updated<'a>, error::Update> {
let tip = refname_to_id(repo, name.clone())?; let Some(reference) = find(repo, &name)? else {
match tip { repo.backend
Some(prev) => { .reference(name.as_ref(), target.into(), false, "radicle: create")
let ancestry = ancestry(repo, prev, target)?; .map_err(|err| error::Update::Create {
name: name.to_owned(),
target,
err,
})?;
match ancestry { return Ok(RefUpdate::Created {
Ancestry::Equal => Ok(RefUpdate::Skipped { name: name.to_ref_string(),
name: name.to_ref_string(), oid: target,
oid: target, }
} .into());
.into()), };
Ancestry::Ahead => {
// N.b. the update is a fast-forward so we can safely let Some(prev) = reference.target() else {
// pass `force: true`. // This should never happen, as there are no facilities to create
repo.backend // symbolic references in Radicle namespaces. If it does, e.g. because
.reference(name.as_ref(), target.into(), true, "radicle: update") // some external program or the user themselves created it, we better
.map_err(|err| error::Update::Create { // do not touch it.
name: name.to_owned(), return Err(error::Update::Symbolic {
target, name: name.to_owned(),
err, });
})?; };
Ok(RefUpdate::from(name.to_ref_string(), prev, target).into())
} if prev == *target {
Ancestry::Behind | Ancestry::Diverged if matches!(no_ff, Policy::Allow) => { // If the two objects are identical, their ancestry does not matter,
// N.b. the update is a non-fast-forward but // we can always skip the update.
// we allow it, so we pass `force: true`. return Ok(RefUpdate::Skipped {
repo.backend name: name.to_ref_string(),
.reference(name.as_ref(), target.into(), true, "radicle: update") oid: target,
.map_err(|err| error::Update::Create { }
name: name.to_owned(), .into());
target, }
err,
})?; let ancestry = {
Ok(RefUpdate::from(name.to_ref_string(), prev, target).into()) use git::raw::ObjectType::{self, *};
} const ANY_KIND: Option<ObjectType> = Some(Any);
// N.b. if the target is behind, we simply reject the update
Ancestry::Behind => Ok(Update::Direct { let prev = repo.backend.find_object(prev, ANY_KIND).map_err(|err| {
name, error::Update::Ancestry(error::Ancestry::Object {
target, oid: prev.into(),
no_ff, err,
} })
.into()), })?;
Ancestry::Diverged if matches!(no_ff, Policy::Reject) => Ok(Update::Direct {
name, let target = repo
target, .backend
no_ff, .find_object(*target, ANY_KIND)
} .map_err(|err| error::Update::Ancestry(error::Ancestry::Object { oid: target, err }))?;
.into()),
Ancestry::Diverged => Err(error::Update::NonFF { match (prev.kind(), target.kind()) {
name: name.to_owned(), (Some(Commit), Some(Commit)) => {
new: target, // This is the common case, we have two commits to compare.
cur: prev, 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 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 { .map_err(|err| error::Update::Create {
name: name.to_owned(), name: name.to_owned(),
target, target,
err, err,
})?; })?;
Ok(RefUpdate::Created { Ok(RefUpdate::from(name.to_ref_string(), prev, target).into())
name: name.to_ref_string(),
oid: 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(),
}),
} }
} }

3
crates/radicle-fetch/src/git/repository/error.rs
View File

@ -79,4 +79,7 @@ pub enum Update {
Peel(#[source] raw::Error), Peel(#[source] raw::Error),
#[error(transparent)] #[error(transparent)]
Resolve(#[from] Resolve), Resolve(#[from] Resolve),
#[error("refusing to update symbolic ref {name}")]
Symbolic { name: Namespaced<'static> },
} }