diff --git a/radicle-cli/examples/workflow/5-patching-maintainer.md b/radicle-cli/examples/workflow/5-patching-maintainer.md index e652f563..be788d44 100644 --- a/radicle-cli/examples/workflow/5-patching-maintainer.md +++ b/radicle-cli/examples/workflow/5-patching-maintainer.md @@ -87,7 +87,13 @@ Fast-forward 2 files changed, 0 insertions(+), 0 deletions(-) create mode 100644 README.md create mode 100644 REQUIREMENTS.md +``` +``` (stderr) $ git push rad master +✓ Patch 69e881c606639691330051d7d8f013854f32fb87 merged at revision 70dd9a3 +✓ Synced with 1 node(s) +To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi + f2de534..f567f69 master -> master ``` The patch is now merged and closed :). @@ -95,24 +101,22 @@ The patch is now merged and closed :). ``` $ rad patch show 69e881c ╭──────────────────────────────────────────────────────────────────────────────╮ -│ Title Define power requirements │ -│ Patch 69e881c606639691330051d7d8f013854f32fb87 │ -│ Author bob z6Mkt67…v4N1tRk │ -│ Head f567f695d25b4e8fb63b5f5ad2a584529826e908 │ -│ Branches master, patch/69e881c │ -│ Commits up to date │ -│ Status merged │ +│ Title Define power requirements │ +│ Patch 69e881c606639691330051d7d8f013854f32fb87 │ +│ Author bob z6Mkt67…v4N1tRk │ +│ Head 27857ec9eb04c69cacab516e8bf4b5fd36090f66 │ +│ Commits ahead 0, behind 1 │ +│ Status merged │ │ │ │ See details. │ ├──────────────────────────────────────────────────────────────────────────────┤ -│ f567f69 Use markdown for requirements │ │ 27857ec Add README, just for the fun │ │ 3e674d1 Define power requirements │ ├──────────────────────────────────────────────────────────────────────────────┤ -│ ● opened by bob z6Mkt67…v4N1tRk [ ... ] │ +│ ● opened by bob z6Mkt67…v4N1tRk [ ... ] │ │ ↑ updated to dcf3e6dd97c95cf8653cbb8ce47df20d28eb1821 (27857ec) [ ... ] │ -│ ↑ updated to 70dd9a31882d184a9fe8f1f590471f5543c4d85b (f567f69) [ ... ] │ -│ ✓ merged by alice (you) [ ... ] │ +│ * revised by alice (you) in 70dd9a3 (f567f69) [ ... ] │ +│ ✓ merged by alice (you) at revision 70dd9a3 (f567f69) [ ... ] │ ╰──────────────────────────────────────────────────────────────────────────────╯ ``` diff --git a/radicle-cli/src/commands/patch/list.rs b/radicle-cli/src/commands/patch/list.rs index 273e5187..08f6966e 100644 --- a/radicle-cli/src/commands/patch/list.rs +++ b/radicle-cli/src/commands/patch/list.rs @@ -138,33 +138,59 @@ pub fn timeline( for (revision_id, revision) in patch.revisions() { // Don't show an "update" line for the first revision. if revision_id != root { - timeline.push(( - revision.timestamp(), - term::Line::spaced( - [ - term::format::tertiary("↑").into(), - term::format::default("updated to").into(), - term::format::dim(revision_id).into(), - term::format::parens(term::format::secondary(term::format::oid( - revision.head(), - ))) - .into(), - ] - .into_iter(), - ), - )); + if revision.author() == patch.author() { + timeline.push(( + revision.timestamp(), + term::Line::spaced( + [ + term::format::tertiary("↑").into(), + term::format::default("updated to").into(), + term::format::dim(revision_id).into(), + term::format::parens(term::format::secondary(term::format::oid( + revision.head(), + ))) + .into(), + ] + .into_iter(), + ), + )); + } else { + let (alias, nid) = Author::new(revision.author().id(), profile).labels(); + + timeline.push(( + revision.timestamp(), + term::Line::spaced( + [ + term::format::tertiary("*").into(), + term::format::default("revised by").into(), + alias.clone(), + nid.clone(), + term::format::default("in").into(), + term::format::dim(term::format::oid(revision_id)).into(), + term::format::parens(term::format::secondary(term::format::oid( + revision.head(), + ))) + .into(), + ] + .into_iter(), + ), + )); + } } for (nid, merge) in patch.merges().filter(|(_, m)| m.revision == revision_id) { let peer = repository.remote(nid)?; + let (alias, nid) = Author::new(&peer.id, profile).labels(); let line = term::Line::spaced([ term::format::primary("✓").bold().into(), - term::format::default("merged").into(), - term::format::default("by").into(), - ]) - .space() - .extend(Author::new(&peer.id, profile).line()); - + term::format::default("merged by").into(), + alias, + nid, + term::format::default("at revision").into(), + term::format::dim(term::format::oid(merge.revision)).into(), + term::format::parens(term::format::secondary(term::format::oid(merge.commit))) + .into(), + ]); timeline.push((merge.timestamp, line)); } for (reviewer, review) in revision.reviews() { diff --git a/radicle-remote-helper/src/push.rs b/radicle-remote-helper/src/push.rs index 6d89db61..558c097d 100644 --- a/radicle-remote-helper/src/push.rs +++ b/radicle-remote-helper/src/push.rs @@ -454,9 +454,10 @@ fn patch_merge_all( let mut revwalk = working.revwalk()?; revwalk.push_range(&format!("{old}..{new}"))?; + // These commits are ordered by children first and then parents. let commits = revwalk .map(|r| r.map(git::Oid::from)) - .collect::, _>>()?; + .collect::, _>>()?; let mut patches = patch::Patches::open(stored)?; for patch in patches.all()? { @@ -464,12 +465,26 @@ fn patch_merge_all( // Skip patches that failed to load. continue; }; - let (revision_id, revision) = patch.latest(); + if !patch.is_open() { + continue; + } + // Later revisions are more likely to be merged, so we build the list backwards. + let revisions = patch + .revisions() + .rev() + .map(|(id, r)| (id, r.head())) + .collect::>(); - if patch.is_open() && commits.contains(&revision.head()) { - let patch = patch::PatchMut::new(id, patch, &mut patches); + // Try to find a revision to merge. Favor revisions that match the more recent commits. + // It's possible for more than one revision to be merged by this push, so we pick the + // revision that is closest to the tip of the commit chain we're pushing. + for commit in &commits { + if let Some((revision_id, _)) = revisions.iter().find(|(_, head)| commit == head) { + let patch = patch::PatchMut::new(id, patch, &mut patches); + patch_merge(patch, *revision_id, new, working, signer)?; - patch_merge(patch, revision_id, new, working, signer)?; + break; + } } } Ok(()) @@ -482,13 +497,23 @@ fn patch_merge( working: &git::raw::Repository, signer: &G, ) -> Result<(), Error> { + let (latest, _) = patch.latest(); let merged = patch.merge(revision, commit, signer)?; - eprintln!( - "{} Patch {} merged", - cli::format::positive("✓"), - cli::format::tertiary(merged.patch) - ); + if revision == latest { + eprintln!( + "{} Patch {} merged", + cli::format::positive("✓"), + cli::format::tertiary(merged.patch) + ); + } else { + eprintln!( + "{} Patch {} merged at revision {}", + cli::format::positive("✓"), + cli::format::tertiary(merged.patch), + cli::format::oid(revision), + ); + } // Delete patch references that were created when the patch was opened. // Note that we don't return an error if we can't delete the refs, since it's diff --git a/radicle/src/cob/common.rs b/radicle/src/cob/common.rs index 5ce0e04b..a2a1e95c 100644 --- a/radicle/src/cob/common.rs +++ b/radicle/src/cob/common.rs @@ -11,7 +11,7 @@ use crate::prelude::*; pub type Timestamp = LocalTime; /// Author. -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)] pub struct Author { pub id: Did, } diff --git a/radicle/src/cob/patch.rs b/radicle/src/cob/patch.rs index b0e0d82d..7526275f 100644 --- a/radicle/src/cob/patch.rs +++ b/radicle/src/cob/patch.rs @@ -347,6 +347,8 @@ impl MergeTarget { pub struct Patch { /// Title of the patch. pub(super) title: String, + /// Patch author. + pub(super) author: Author, /// Current state of the patch. pub(super) state: State, /// Target this patch is meant to be merged in. @@ -380,6 +382,7 @@ impl Patch { pub fn new(title: String, target: MergeTarget, (id, revision): (RevisionId, Revision)) -> Self { Self { title, + author: revision.author.clone(), state: State::default(), target, labels: BTreeSet::default(), @@ -408,7 +411,7 @@ impl Patch { /// Timestamp of the first revision of the patch. pub fn timestamp(&self) -> Timestamp { - self.revisions() + self.updates() .next() .map(|(_, r)| r) .expect("Patch::timestamp: at least one revision is present") @@ -428,12 +431,16 @@ impl Patch { /// Author of the first revision of the patch. pub fn author(&self) -> &Author { - &self - .revisions() - .next() - .map(|(_, r)| r) - .expect("Patch::author: at least one revision is present") - .author + &self.author + } + + /// All revision authors. + pub fn authors(&self) -> BTreeSet<&Author> { + self.revisions + .values() + .filter_map(|r| r.as_ref()) + .map(|r| &r.author) + .collect() } /// Get the `Revision` by its `RevisionId`. @@ -443,10 +450,15 @@ impl Patch { self.revisions.get(id).and_then(|o| o.as_ref()) } - /// List of patch revisions. The initial changeset is part of the + /// List of patch revisions by the patch author. The initial changeset is part of the /// first revision. + pub fn updates(&self) -> impl DoubleEndedIterator { + self.revisions_by(self.author().public_key()) + } + + /// List of all patch revisions by all authors. pub fn revisions(&self) -> impl DoubleEndedIterator { - self.timeline.iter().filter_map(|id| { + self.timeline.iter().filter_map(move |id| { self.revisions .get(id) .and_then(|o| o.as_ref()) @@ -454,6 +466,15 @@ impl Patch { }) } + /// List of patch revisions by the given author. + pub fn revisions_by<'a>( + &'a self, + author: &'a PublicKey, + ) -> impl DoubleEndedIterator { + self.revisions() + .filter(move |(_, r)| (r.author.public_key() == author)) + } + /// List of patch assignees. pub fn assignees(&self) -> impl Iterator + '_ { self.assignees.iter().map(Did::from) @@ -504,18 +525,22 @@ impl Patch { /// /// This is the revision that was created with the patch. pub fn root(&self) -> (RevisionId, &Revision) { - self.revisions() + self.updates() .next() .expect("Patch::root: there is always a root revision") } - /// Latest revision. + /// Latest revision by the patch author. pub fn latest(&self) -> (RevisionId, &Revision) { - self.revisions() - .next_back() + self.latest_by(self.author().public_key()) .expect("Patch::latest: there is always at least one revision") } + /// Latest revision by the given author. + pub fn latest_by<'a>(&'a self, author: &'a PublicKey) -> Option<(RevisionId, &Revision)> { + self.revisions_by(author).next_back() + } + /// Time of last update. pub fn updated_at(&self) -> Timestamp { self.latest().1.timestamp() @@ -618,8 +643,8 @@ impl Patch { // Redacted. Authorization::Unknown } - // Only patch authors can propose revisions. - Action::Revision { .. } => Authorization::from(actor == author), + // Anyone can propose revisions. + Action::Revision { .. } => Authorization::Allow, // Only the revision author can edit or redact their revision. Action::RevisionEdit { revision, .. } | Action::RevisionRedact { revision, .. } => { if let Some(revision) = lookup::revision(self, revision)? {