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

cli: Verify merge, remove `rad merge` command 

When applying a `Merge` action, we verify that the commit is in the
history of the default branch of that user.

Since this is not the case when issuing a `rad merge`, and since that
command is no longer needed with the push-based merge, we remove the
`rad merge` command completely.
This commit is contained in:
Alexis Sellier 2023-05-21 20:14:17 +02:00
parent 376adde1f9
commit 8e89976937
No known key found for this signature in database
12 changed files with 53 additions and 428 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
radicle-cli/examples/workflow/5-patching-maintainer.md
View File

@ -75,9 +75,7 @@ Great, all fixed up, lets merge the code.
```
$ git checkout master
Your branch is up to date with 'rad/master'.
$ rad merge 4486280d0dec743d6a1a0c05754f8e40426c681d
Merging 5f0a547 R2 (f6484e0) by z6Mkt67…v4N1tRk into master (f2de534) via fast-forward...
Running `git merge --ff-only f6484e0f43e48a8983b9b39bf9bd4cd889f1d520`...
$ git merge flux-capacitor-power
Updating f2de534..f6484e0
Fast-forward
README.md | 0
@ -85,8 +83,7 @@ Fast-forward
2 files changed, 0 insertions(+), 0 deletions(-)
create mode 100644 README.md
create mode 100644 REQUIREMENTS.md
✓ Updated master f2de534 -> f6484e0 via fast-forward
✓ Patch state updated, use `git push rad` to publish
$ git push rad master
```
The patch is now merged and closed :).
@ -99,7 +96,7 @@ $ rad patch show 5f0a547
│ Author did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk │
│ Head f6484e0f43e48a8983b9b39bf9bd4cd889f1d520 │
│ Branches flux-capacitor-power, master │
│ Commits ahead 3, behind 0 │
│ Commits up to date │
│ Status merged │
│ │
│ See details. │

2
radicle-cli/src/commands.rs
View File

@ -26,8 +26,6 @@ pub mod rad_inspect;
pub mod rad_issue;
#[path = "commands/ls.rs"]
pub mod rad_ls;
#[path = "commands/merge.rs"]
pub mod rad_merge;
#[path = "commands/node.rs"]
pub mod rad_node;
#[path = "commands/patch.rs"]

1
radicle-cli/src/commands/help.rs
View File

@ -25,7 +25,6 @@ const COMMANDS: &[Help] = &[
rad_inspect::HELP,
rad_issue::HELP,
rad_ls::HELP,
rad_merge::HELP,
rad_node::HELP,
rad_patch::HELP,
rad_path::HELP,

402
radicle-cli/src/commands/merge.rs
View File

@ -1,402 +0,0 @@
use std::ffi::OsString;
use std::fmt;
use std::fmt::Write;
use anyhow::{anyhow, Context};
use radicle::cob::patch::{Patch, PatchId, Patches};
use radicle::git;
use radicle::prelude::*;
use radicle::rad;
use crate::git::Rev;
use crate::terminal as term;
use crate::terminal::args::{string, Args, Error, Help};
pub const HELP: Help = Help {
name: "merge",
description: "Merge a patch",
version: env!("CARGO_PKG_VERSION"),
usage: r#"
Usage
rad merge [<revision-id>] [<option>...]
To specify a patch revision to merge, use the fully qualified revision id.
Options
-f, --force Force merging an older patch revision
-i, --interactive Ask for confirmations
--help Print help
"#,
};
/// Merge commit help message.
const MERGE_HELP_MSG: &str = r#"
# Check the commit message for this merge and make sure everything looks good,
# or make any necessary change.
#
# Lines starting with '#' will be ignored, and an empty message aborts the commit.
#
# vim: ft=gitcommit
#
"#;
/// A patch merge style.
#[derive(Debug, PartialEq, Eq)]
pub enum MergeStyle {
/// A merge commit is created.
Commit,
/// The branch is fast-forwarded to the patch's commit.
FastForward,
}
impl fmt::Display for MergeStyle {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Self::Commit => {
write!(f, "merge-commit")
}
Self::FastForward => {
write!(f, "fast-forward")
}
}
}
}
#[derive(PartialEq, Eq)]
pub enum State {
Open,
Merged,
}
#[derive(Debug)]
pub struct Options {
pub revision_id: Rev,
pub force: bool,
pub interactive: bool,
}
impl Args for Options {
fn from_args(args: Vec<OsString>) -> anyhow::Result<(Self, Vec<OsString>)> {
use lexopt::prelude::*;
let mut parser = lexopt::Parser::from_args(args);
let mut force = false;
let mut revision_id = None;
let mut interactive = false;
while let Some(arg) = parser.next()? {
match arg {
Long("help") => {
return Err(Error::Help.into());
}
Long("force") | Short('f') => {
force = true;
}
Long("interactive") | Short('i') => {
interactive = true;
}
Value(val) => {
let val = string(&val);
revision_id = Some(Rev::from(val));
}
_ => return Err(anyhow::anyhow!(arg.unexpected())),
}
}
let revision_id =
revision_id.ok_or_else(|| anyhow!("a revision to merge must be provided"))?;
Ok((
Options {
revision_id,
force,
interactive,
},
vec![],
))
}
}
pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> {
//
// Setup
//
let (repo, id) =
rad::cwd().map_err(|_| anyhow!("this command must be run in the context of a project"))?;
let profile = ctx.profile()?;
let signer = term::signer(&profile)?;
let repository = profile.storage.repository(id)?;
let _project = repository
.identity_doc_of(profile.id())
.context(format!("couldn't load project {id} from local state"))?;
let repository = profile.storage.repository(id)?;
let mut patches = Patches::open(&repository)?;
if repo.head_detached()? {
anyhow::bail!("HEAD is in a detached state; can't merge");
}
//
// Get patch information
//
let revision_id = options.revision_id.resolve(&repository.backend)?;
let (patch_id, patch, revision) = patches.find_by_revision(&revision_id)?.ok_or(anyhow!(
"no open patch with revision `{}` could be found",
&revision_id
))?;
if !patch.is_open() {
anyhow::bail!(
"revision's patch must be open for merging, but it is `{}`",
patch.state()
);
}
let (last_revision_id, _) = patch
.latest()
.ok_or(anyhow!("patch must have atleast one unredacted revision"))?;
if !options.force && revision_id != *last_revision_id {
anyhow::bail!("refusing to merge old patch revision");
}
let mut patch = patches
.get_mut(&patch_id)
.map_err(|e| anyhow!("couldn't find patch {} locally: {e}", &id))?;
let head = repo.head()?;
let branch = head
.shorthand()
.ok_or_else(|| anyhow!("invalid head branch"))?;
let head_oid = head
.target()
.ok_or_else(|| anyhow!("cannot merge into detatched head; aborting"))?;
//
// Analyze merge
//
let patch_commit = repo
.find_annotated_commit(revision.head().into())
.or_else(|e| match e.code() {
git::raw::ErrorCode::NotFound => {
// Avoid using git2 until 'rad://' supports fetching using an Oid as a refspec.
crate::git::git(
repo.path(),
[
"fetch",
&git::Url::from(id)
.with_namespace(**patch.author().id())
.to_string(),
&revision.head().to_string(),
],
)?;
let c = repo.find_annotated_commit(revision.head().into())?;
Ok(c)
}
_ => Err(anyhow::Error::from(e)),
})
.context("patch head not found in local repository")?;
let (merge, _merge_pref) = repo.merge_analysis(&[&patch_commit])?;
let merge_style = if merge.is_fast_forward() {
// The given merge input is a fast-forward from HEAD and no merge needs to be performed.
// Instead, the client can apply the input commits to its HEAD.
MergeStyle::FastForward
} else if merge.is_normal() {
// A “normal” merge; both HEAD and the given merge input have diverged from their common
// ancestor. The divergent commits must be merged.
//
// Let's check if there are potential merge conflicts.
let our_commit = head.peel_to_commit()?;
let their_commit = repo.find_commit(revision.head().into())?;
let index = repo
.merge_commits(&our_commit, &their_commit, None)
.context("failed to perform merge analysis")?;
if index.has_conflicts() {
return Err(Error::WithHint {
err: anyhow!("patch conflicts with {}", branch),
hint: "Patch must be rebased before it can be merged.",
}
.into());
}
MergeStyle::Commit
} else if merge.is_up_to_date() {
term::success!(
"Patch {} is already part of current branch {}",
term::format::tertiary(patch_id),
term::format::parens(term::format::yellow(branch))
);
return Ok(());
} else if merge.is_unborn() {
anyhow::bail!("HEAD does not point to a valid commit");
} else {
anyhow::bail!(
"no merge is possible between {} and {}",
head_oid,
revision.head()
);
};
let merge_style_pretty = match merge_style {
MergeStyle::FastForward => term::format::style(merge_style.to_string())
.dim()
.italic()
.to_string(),
MergeStyle::Commit => term::format::yellow(merge_style.to_string())
.italic()
.to_string(),
};
// SAFETY: The patch has already been fetched by its revision_id.
let revision_ix = patch
.revisions()
.position(|(id_, _)| id_ == &revision_id)
.unwrap();
term::info!(
"{} {} {} {} by {} into {} {} via {}...",
term::format::bold("Merging"),
term::format::tertiary(term::format::cob(&patch_id)),
term::format::dim(format!("R{revision_ix}")),
term::format::parens(term::format::secondary(term::format::oid(revision.head()))),
term::format::tertiary(term::format::node(&patch.author().id)),
term::format::highlight(branch),
term::format::parens(term::format::secondary(term::format::oid(head_oid))),
merge_style_pretty
);
if options.interactive && !term::confirm("Confirm?") {
anyhow::bail!("merge aborted by user");
}
//
// Perform merge
//
let head = match merge_style {
MergeStyle::Commit => {
merge_commit(&repo, patch_id, &patch_commit, &patch, signer.public_key())
}
MergeStyle::FastForward => fast_forward(&repo, &revision.head()),
}?;
term::success!(
"Updated {} {} -> {} via {}",
term::format::highlight(branch),
term::format::secondary(term::format::oid(head_oid)),
term::format::secondary(term::format::oid(head)),
merge_style_pretty
);
//
// Update patch COB
//
// TODO: Don't allow merging the same revision twice?
patch.merge(revision_id, head, &signer)?;
term::success!(
"Patch state updated, use {} to publish",
term::format::secondary("`git push rad`")
);
Ok(())
}
// Perform git merge.
//
// This does not touch the COB state.
//
// Nb. Merge can fail even though conflicts were not detected if there are some
// files in the repo that are not checked in. This is because the merge conflict
// simulation only operates on the commits, not the checkout.
fn merge_commit(
repo: &git::raw::Repository,
patch_id: PatchId,
patch_commit: &git::raw::AnnotatedCommit,
patch: &Patch,
whoami: &PublicKey,
) -> anyhow::Result<git::Oid> {
let description = patch.description().trim();
let mut merge_opts = git::raw::MergeOptions::new();
let mut merge_msg = format!(
"Merge patch '{}' from {}",
term::format::cob(&patch_id),
patch.author().id()
);
write!(&mut merge_msg, "\n\n")?;
if !description.is_empty() {
write!(&mut merge_msg, "{description}")?;
write!(&mut merge_msg, "\n\n")?;
}
writeln!(&mut merge_msg, "Rad-Patch: {patch_id}")?;
writeln!(&mut merge_msg, "Rad-Author: {}", patch.author().id())?;
writeln!(&mut merge_msg, "Rad-Committer: {whoami}")?;
writeln!(&mut merge_msg)?;
writeln!(&mut merge_msg, "{}", MERGE_HELP_MSG.trim())?;
// Offer user the chance to edit the message before committing.
let merge_msg = match term::Editor::new().extension("git-commit").edit(merge_msg) {
Ok(Some(s)) => s
.lines()
.filter(|l| !l.starts_with('#'))
.collect::<Vec<_>>()
.join("\n"),
_ => anyhow::bail!("user aborted merge"),
};
// Empty message aborts merge.
if merge_msg.trim().is_empty() {
anyhow::bail!("user aborted merge");
}
// Perform merge (nb. this does not commit).
repo.merge(&[patch_commit], Some(merge_opts.patience(true)), None)
.context("merge failed")?;
// Commit staged changes.
let commit = repo.find_commit(patch_commit.id())?;
let author = commit.author();
let committer = repo
.signature()
.context("git user name or email not configured")?;
let tree = repo.index()?.write_tree()?;
let tree = repo.find_tree(tree)?;
let parents = &[&repo.head()?.peel_to_commit()?, &commit];
let oid = repo
.commit(
Some("HEAD"),
&author,
&committer,
&merge_msg,
&tree,
parents,
)
.context("merge commit failed")?;
// Cleanup merge state.
repo.cleanup_state().context("merge state cleanup failed")?;
Ok(oid.into())
}
/// Perform fast-forward merge of patch.
fn fast_forward(repo: &git::raw::Repository, patch_oid: &git::Oid) -> anyhow::Result<git::Oid> {
let oid = patch_oid.to_string();
let args = ["merge", "--ff-only", &oid];
term::subcommand(format!("git {}", args.join(" ")));
let output = git::run::<_, _, &str, &str>(
repo.workdir()
.ok_or_else(|| anyhow!("cannot fast-forward in bare repo"))?,
args,
[],
)
.context("fast-forward failed")?;
term::blob(output);
Ok(*patch_oid)
}

8
radicle-cli/src/main.rs
View File

@ -219,14 +219,6 @@ fn run_other(exe: &str, args: &[OsString]) -> Result<(), Option<anyhow::Error>>
args.to_vec(),
);
}
"merge" => {
term::run_command_args::<rad_merge::Options, _>(
rad_merge::HELP,
"Merge",
rad_merge::run,
args.to_vec(),
);
}
"node" => {
term::run_command_args::<rad_node::Options, _>(
rad_node::HELP,

2
radicle-remote-helper/src/push.rs
View File

@ -198,7 +198,7 @@ pub fn run(
let rid = stored.id.to_string();
let stderr = io::stderr().as_raw_fd();
// Nb. allow this to fail. The push to local storage was still successful.
execute("rad", ["sync", &rid, "--verbose"], unsafe {
execute("rad", ["sync", &rid, "--announce", "--verbose"], unsafe {
process::Stdio::from_raw_fd(stderr)
})
.ok();

26
radicle/src/cob/patch.rs
View File

@ -25,6 +25,7 @@ use crate::cob::{store, ActorId, EntryId, ObjectId, TypeName};
use crate::crypto::{PublicKey, Signer};
use crate::git;
use crate::identity::doc::DocError;
use crate::identity::PayloadError;
use crate::prelude::*;
use crate::storage::git as storage;
@ -65,9 +66,15 @@ pub enum ApplyError {
/// Error loading the identity document committed to by an operation.
#[error("identity doc failed to load: {0}")]
Doc(#[from] DocError),
/// Error loading the document payload.
#[error("payload failed to load: {0}")]
Payload(#[from] PayloadError),
/// The merge operation is invalid.
#[error("invalid merge operation in {0}")]
InvalidMerge(EntryId),
/// Git error.
#[error("git: {0}")]
Git(#[from] git::ext::Error),
}
/// Error updating or creating patches.
@ -420,9 +427,22 @@ impl store::FromHistory for Patch {
}
Action::Merge { revision, commit } => {
if let Some(Redactable::Present(_)) = self.revisions.get_mut(&revision) {
let doc = repo.identity_doc_at(op.identity)?;
if !doc.is_delegate(&op.author) {
return Err(ApplyError::InvalidMerge(op.id));
let doc = repo.identity_doc_at(op.identity)?.verified()?;
match self.target() {
MergeTarget::Delegates => {
if !doc.is_delegate(&op.author) {
return Err(ApplyError::InvalidMerge(op.id));
}
let proj = doc.project()?;
let branch = git::refs::branch(proj.default_branch());
let Ok(head) = repo.reference_oid(&op.author, &branch) else {
return Err(ApplyError::InvalidMerge(op.id));
};
if commit != head && !repo.is_ancestor_of(commit, head)? {
return Err(ApplyError::InvalidMerge(op.id));
}
}
}
self.merges.insert(
op.author,

10
radicle/src/git.rs
View File

@ -151,6 +151,14 @@ pub mod refs {
Ok((qualified, target.into()))
}
/// Create a qualified branch reference.
///
/// `refs/heads/<branch>`
///
pub fn branch<'a>(branch: &RefStr) -> Qualified<'a> {
Qualified::from(lit::refs_heads(branch))
}
pub mod storage {
use format::{
lit,
@ -180,7 +188,7 @@ pub mod refs {
///
/// `refs/namespaces/<remote>/refs/heads/<branch>`
///
pub fn branch<'a>(remote: &RemoteId, branch: &RefStr) -> Namespaced<'a> {
pub fn branch_of<'a>(remote: &RemoteId, branch: &RefStr) -> Namespaced<'a> {
Qualified::from(lit::refs_heads(branch)).with_namespace(remote.into())
}

4
radicle/src/rad.rs
View File

@ -152,12 +152,12 @@ pub fn fork_remote<G: Signer, S: storage::WriteStorage>(
let repository = storage.repository_mut(proj)?;
let raw = repository.raw();
let remote_head = raw.refname_to_id(&git::refs::storage::branch(
let remote_head = raw.refname_to_id(&git::refs::storage::branch_of(
remote,
project.default_branch(),
))?;
raw.reference(
&git::refs::storage::branch(me, project.default_branch()),
&git::refs::storage::branch_of(me, project.default_branch()),
remote_head,
false,
&format!("creating default branch for {me}"),

3
radicle/src/storage.rs
View File

@ -390,6 +390,9 @@ pub trait ReadRepository {
/// Perform a revision walk of a commit history starting from the given head.
fn revwalk(&self, head: Oid) -> Result<git2::Revwalk, git2::Error>;
/// Check whether the given commit is an ancestor of another commit.
fn is_ancestor_of(&self, ancestor: Oid, head: Oid) -> Result<bool, git::ext::Error>;
/// Get the object id of a reference under the given remote.
fn reference_oid(
&self,

6
radicle/src/storage/git.rs
View File

@ -434,6 +434,12 @@ impl ReadRepository for Repository {
Ok(revwalk)
}
fn is_ancestor_of(&self, ancestor: Oid, head: Oid) -> Result<bool, git::Error> {
self.backend
.graph_descendant_of(head.into(), ancestor.into())
.map_err(git::Error::from)
}
fn remote(&self, remote: &RemoteId) -> Result<Remote<Verified>, refs::Error> {
let refs = SignedRefs::load(*remote, self)?;
Ok(Remote::<Verified>::new(refs))

8
radicle/src/test/storage.rs
View File

@ -174,6 +174,10 @@ impl ReadRepository for MockRepository {
todo!()
}
fn is_ancestor_of(&self, _ancestor: Oid, _head: Oid) -> Result<bool, git_ext::Error> {
Ok(true)
}
fn blob_at<'a>(
&'a self,
_oid: git_ext::Oid,
@ -195,7 +199,7 @@ impl ReadRepository for MockRepository {
_remote: &RemoteId,
_reference: &git::Qualified,
) -> Result<git_ext::Oid, git_ext::Error> {
todo!()
Ok(Oid::from_str("ffffffffffffffffffffffffffffffffffffffff").unwrap())
}
fn references_of(&self, _remote: &RemoteId) -> Result<crate::storage::refs::Refs, Error> {
@ -227,7 +231,7 @@ impl ReadRepository for MockRepository {
}
fn canonical_identity_head(&self) -> Result<Oid, IdentityError> {
Ok(Oid::from_str("cb18e95ada2bb38aadd8e6cef0963ce37a87add3").unwrap())
Ok(Oid::from_str("cccccccccccccccccccccccccccccccccccccccc").unwrap())
}
}