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

remote-helper: Add 'patch.target' push option 

The remote helper now supports the push option `patch.target`. This
allows users to explicitly specify a target canonical reference when
opening or updating a patch. For example, to open a patch that targets
the branch "backport", use:
```
git push -o patch.target=refs/heads/backport
```

Furthermore, strict merge and revert isolation is now enforced:
patches are only marked as merged or reverted if the commits are
pushed to the target branch of the patch explicitly.
This commit is contained in:
Adrian Duke 2026-05-12 13:36:06 +01:00 committed by Fintan Halpenny
parent f0c6abc6ea
commit 06e201c927
3 changed files with 83 additions and 47 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
CHANGELOG.md
View File

@ -69,6 +69,13 @@ COB type names and payload IDs remain unchanged for backwards compatibility.
## New Features ## New Features
- The remote helper (`git-remote-rad`) now supports the push option
`patch.target`. This allows users to explicitly specify a target canonical
reference when opening or updating a patch. For example, to open a patch that
targets the branch "backport", use `git push -o patch.target=refs/heads/backport`.
Furthermore, strict merge and revert isolation is now enforced: patches are
only marked as merged or reverted if the commits are pushed to the target
branch of the patch explicitly.
- Teach `rad patch show` to show the full commit range for each revision. - Teach `rad patch show` to show the full commit range for each revision.
Previously, it would only show the head of the range, but not the base. Previously, it would only show the head of the range, but not the base.
It now shows `<base>..<head>`, where the shortened OID is used when not It now shows `<base>..<head>`, where the shortened OID is used when not

8
crates/radicle-remote-helper/src/main.rs
View File

@ -207,6 +207,8 @@ struct Options {
message: cli::patch::Message, message: cli::patch::Message,
/// Create a branch and set its upstream when opening a patch. /// Create a branch and set its upstream when opening a patch.
branch: Branch, branch: Branch,
/// Patch target to use, when opening or updating a patch.
target: cob::patch::MergeTarget,
verbosity: Verbosity, verbosity: Verbosity,
} }
@ -439,6 +441,12 @@ fn push_option(args: &[&str], opts: &mut Options) -> Result<(), Error> {
"patch.branch" => { "patch.branch" => {
opts.branch = Branch::Provided(git::fmt::RefString::try_from(val)?) opts.branch = Branch::Provided(git::fmt::RefString::try_from(val)?)
} }
"patch.target" => {
let target = val.parse::<cob::patch::TargetBranch>().map_err(|e| {
Error::UnsupportedPushOption(format!("invalid patch.target '{val}': {e}"))
})?;
opts.target = cob::patch::MergeTarget::Branch(target);
}
other => { other => {
return Err(Error::UnsupportedPushOption(other.to_owned())); return Err(Error::UnsupportedPushOption(other.to_owned()));
} }

115
crates/radicle-remote-helper/src/push.rs
View File

@ -11,7 +11,6 @@ use std::{assert_eq, io};
use radicle::cob::store::access::WriteAs; use radicle::cob::store::access::WriteAs;
use thiserror::Error; use thiserror::Error;
use radicle::Profile;
use radicle::cob; use radicle::cob;
use radicle::cob::object::ParseObjectId; use radicle::cob::object::ParseObjectId;
use radicle::cob::patch; use radicle::cob::patch;
@ -24,6 +23,7 @@ use radicle::node::NodeId;
use radicle::storage; use radicle::storage;
use radicle::storage::git::transport::local::Url; use radicle::storage::git::transport::local::Url;
use radicle::storage::{ReadRepository, SignRepository as _, WriteRepository}; use radicle::storage::{ReadRepository, SignRepository as _, WriteRepository};
use radicle::{Profile, identity};
use radicle::{git, rad}; use radicle::{git, rad};
use radicle_cli::terminal as term; use radicle_cli::terminal as term;
@ -114,6 +114,9 @@ pub(super) enum Error {
UnknownObjectType { oid: git::Oid }, UnknownObjectType { oid: git::Oid },
#[error(transparent)] #[error(transparent)]
FindObjects(#[from] git::canonical::error::FindObjectsError), FindObjects(#[from] git::canonical::error::FindObjectsError),
/// Default branch error.
#[error(transparent)]
DefaultBranch(#[from] radicle::identity::doc::DefaultBranchError),
/// Error sending pack from the working copy to storage. /// Error sending pack from the working copy to storage.
#[error( #[error(
@ -579,23 +582,9 @@ where
term::patch::get_create_message(opts.message, &stored.backend, &base.into(), &head.into())?; term::patch::get_create_message(opts.message, &stored.backend, &base.into(), &head.into())?;
let patch = if opts.draft { let patch = if opts.draft {
patches.draft( patches.draft(title, &description, opts.target.clone(), base, *head, &[])
title,
&description,
patch::MergeTarget::default(),
base,
*head,
&[],
)
} else { } else {
patches.create( patches.create(title, &description, opts.target.clone(), base, *head, &[])
title,
&description,
patch::MergeTarget::default(),
base,
*head,
&[],
)
}?; }?;
let action = if patch.is_draft() { let action = if patch.is_draft() {
@ -799,42 +788,47 @@ where
Signer: crypto::signature::Verifier<crypto::Signature>, Signer: crypto::signature::Verifier<crypto::Signature>,
{ {
let head = *src; let head = *src;
let dst = dst.with_namespace(nid.into()); let namespaced_dst = dst.with_namespace(nid.into());
// In some rare cases, pushing to the default branch is a new action for a delegate. let old = {
// If this is the case, use the canonical head before the push to determine the old side. // In some rare cases, pushing to the default branch is a new action for a delegate.
// // If this is the case, use the canonical head before the push to determine the old side.
// Note that this is only important for reverting and merging, but must happen before the push. //
let old = stored // Note that this is only important for reverting and merging, but must happen before the push.
.backend let old = stored
.find_reference(dst.as_str()) .backend
.ok() .find_reference(namespaced_dst.as_str())
.map(|old| old.peel_to_commit().map(|c| c.id().into())) .ok()
.transpose()? .map(|old| old.peel_to_commit().map(|c| c.id().into()))
.or_else(|| stored.canonical_head().map(|(_, oid)| oid).ok()); .transpose()?
.or_else(|| stored.canonical_head().map(|(_, oid)| oid).ok());
push_ref( push_ref(
src, src,
&dst, &namespaced_dst,
force, force,
stored.raw(), stored.raw(),
verbosity, verbosity,
expected_refs, expected_refs,
git, git,
)?; )?;
old
};
if let Some(old) = old { if let Some(old) = old {
let proj = stored.project()?; let identity = stored.identity()?;
let master = &*git::fmt::Qualified::from(git::fmt::lit::refs_heads(proj.default_branch())); let crefs = identity.doc().canonical_refs()?;
let rules = crefs.rules();
let me = Did::from(nid);
// If we're pushing to the project's default branch, we want to see if any patches got // If we're pushing to a valid canonical branch, we want to see if any patches got
// merged or reverted, and if so, update the patch COB. // merged or reverted, and if so, update the patch COB.
if &*dst.strip_namespace() == master { if let Some((_, rule)) = rules.matches(dst).next()
// Only delegates affect the merge state of the COB. && rule.allowed().contains(&me)
if stored.delegates()?.contains(&nid.into()) { {
patch_revert_all(old, head, &stored.backend, &mut patches)?; patch_revert_all(old, head, dst, &stored.backend, &mut patches, &identity)?;
patch_merge_all(old, head, working, &mut patches, signer)?; patch_merge_all(old, head, dst, working, &mut patches, signer, &identity)?;
}
} }
} }
Ok(Some(ExplorerResource::Tree { oid: head })) Ok(Some(ExplorerResource::Tree { oid: head }))
@ -844,6 +838,7 @@ where
fn patch_revert_all<Signer>( fn patch_revert_all<Signer>(
old: git::Oid, old: git::Oid,
new: git::Oid, new: git::Oid,
pushed_dst: &git::fmt::Qualified,
stored: &git::raw::Repository, stored: &git::raw::Repository,
patches: &mut patch::Cache< patches: &mut patch::Cache<
'_, '_,
@ -851,6 +846,7 @@ fn patch_revert_all<Signer>(
WriteAs<'_, Signer>, WriteAs<'_, Signer>,
cob::cache::StoreWriter, cob::cache::StoreWriter,
>, >,
identity: &radicle::identity::Identity,
) -> Result<(), Error> ) -> Result<(), Error>
where where
Signer: crypto::signature::Keypair<VerifyingKey = crypto::PublicKey>, Signer: crypto::signature::Keypair<VerifyingKey = crypto::PublicKey>,
@ -869,11 +865,15 @@ where
return Ok(()); return Ok(());
} }
let identity_doc = identity.doc();
// Get the list of merged patches. // Get the list of merged patches.
let merged = patches let merged = patches
.merged()? .merged()?
// Skip patches that failed to load. // Skip patches that failed to load.
.filter_map(|patch| patch.ok()) .filter_map(|patch| patch.ok())
// Only consider patches that target the destination branch
.filter(|(id, patch)| merge_targets_match(pushed_dst, identity_doc, id, patch))
.collect::<Vec<_>>(); .collect::<Vec<_>>();
for (id, patch) in merged { for (id, patch) in merged {
@ -907,10 +907,25 @@ where
Ok(()) Ok(())
} }
fn merge_targets_match(
pushed_target: &git::fmt::Qualified,
doc: &identity::Doc,
id: &patch::PatchId,
patch: &patch::Patch,
) -> bool {
patch
.is_targeted_by(doc, pushed_target)
.unwrap_or_else(|e| {
log::warn!("Failed to resolve merge target for patch {}: {}", id, e);
false
})
}
/// Merge all patches that have been included in the base branch. /// Merge all patches that have been included in the base branch.
fn patch_merge_all<Signer>( fn patch_merge_all<Signer>(
old: git::Oid, old: git::Oid,
new: git::Oid, new: git::Oid,
pushed_dst: &git::fmt::Qualified,
working: &git::raw::Repository, working: &git::raw::Repository,
patches: &mut patch::Cache< patches: &mut patch::Cache<
'_, '_,
@ -919,6 +934,7 @@ fn patch_merge_all<Signer>(
cob::cache::StoreWriter, cob::cache::StoreWriter,
>, >,
signer: &Signer, signer: &Signer,
identity: &radicle::identity::Identity,
) -> Result<(), Error> ) -> Result<(), Error>
where where
Signer: crypto::signature::Keypair<VerifyingKey = crypto::PublicKey>, Signer: crypto::signature::Keypair<VerifyingKey = crypto::PublicKey>,
@ -937,12 +953,17 @@ where
return Ok(()); return Ok(());
} }
let identity_doc = identity.doc();
let open = patches let open = patches
.opened()? .opened()?
.chain(patches.drafted()?) .chain(patches.drafted()?)
// Skip patches that failed to load. // Skip patches that failed to load.
.filter_map(|patch| patch.ok()) .filter_map(|patch| patch.ok())
// Only consider patches that target the destination branch
.filter(|(id, patch)| merge_targets_match(pushed_dst, identity_doc, id, patch))
.collect::<Vec<_>>(); .collect::<Vec<_>>();
for (id, patch) in open { for (id, patch) in open {
// Later revisions are more likely to be merged, so we build the list backwards. // Later revisions are more likely to be merged, so we build the list backwards.
let revisions = patch let revisions = patch