cob: Better distinguish patch revisions

The `Patch` API and semantics are changed a little bit:

* It's now valid again to propose a revision without being the patch
  author.
* To get revisions by the patch author, `Patch::updates` is used.
* `Patch::latest` now only looks at revisions by the patch author.
* Added `Patch::revisions_by`, `Patch::authors` and `Patch::latest_by`.
* The merge behavior on git-push is improved to favor revisions that
  merge more recent commits. We also allow for merging the non-latest
  revision.

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                                          │
+│ Title    Define power requirements                                           │
-│ Patch     69e881c606639691330051d7d8f013854f32fb87                           │
+│ Patch    69e881c606639691330051d7d8f013854f32fb87                            │
-│ Author    bob z6Mkt67…v4N1tRk                                                │
+│ Author   bob z6Mkt67…v4N1tRk                                                 │
-│ Head      f567f695d25b4e8fb63b5f5ad2a584529826e908                           │
+│ Head     27857ec9eb04c69cacab516e8bf4b5fd36090f66                            │
-│ Branches  master, patch/69e881c                                              │
+│ Commits  ahead 0, behind 1                                                   │
-│ Commits   up to date                                                         │
+│ Status   merged                                                              │
-│ 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) [   ...    ] │
+│ * revised by alice (you) in 70dd9a3 (f567f69) [   ...    ]                   │
-│ ✓ merged by alice (you) [   ...    ]                                         │
+│ ✓ merged by alice (you) at revision 70dd9a3 (f567f69) [    ...    ]          │
 ╰──────────────────────────────────────────────────────────────────────────────╯
 ```
 

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((
+            if revision.author() == patch.author() {
-                revision.timestamp(),
+                timeline.push((
-                term::Line::spaced(
+                    revision.timestamp(),
-                    [
+                    term::Line::spaced(
-                        term::format::tertiary("↑").into(),
+                        [
-                        term::format::default("updated to").into(),
+                            term::format::tertiary("↑").into(),
-                        term::format::dim(revision_id).into(),
+                            term::format::default("updated to").into(),
-                        term::format::parens(term::format::secondary(term::format::oid(
+                            term::format::dim(revision_id).into(),
-                            revision.head(),
+                            term::format::parens(term::format::secondary(term::format::oid(
-                        )))
+                                revision.head(),
-                        .into(),
+                            )))
-                    ]
+                            .into(),
-                    .into_iter(),
+                        ]
-                ),
+                        .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("merged by").into(),
-                term::format::default("by").into(),
+                alias,
-            ])
+                nid,
-            .space()
+                term::format::default("at revision").into(),
-            .extend(Author::new(&peer.id, profile).line());
+                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() {

radicle-remote-helper/src/push.rs

@@ -454,9 +454,10 @@ fn patch_merge_all<G: Signer>(
     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::<Result<HashSet<git::Oid>, _>>()?;
+        .collect::<Result<Vec<git::Oid>, _>>()?;
 
     let mut patches = patch::Patches::open(stored)?;
     for patch in patches.all()? {
@@ -464,12 +465,26 @@ fn patch_merge_all<G: Signer>(
             // 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::<Vec<_>>();
 
-        if patch.is_open() && commits.contains(&revision.head()) {
+        // Try to find a revision to merge. Favor revisions that match the more recent commits.
-            let patch = patch::PatchMut::new(id, patch, &mut patches);
+        // 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<G: Signer>(
     working: &git::raw::Repository,
     signer: &G,
 ) -> Result<(), Error> {
+    let (latest, _) = patch.latest();
     let merged = patch.merge(revision, commit, signer)?;
 
-    eprintln!(
+    if revision == latest {
-        "{} Patch {} merged",
+        eprintln!(
-        cli::format::positive("✓"),
+            "{} Patch {} merged",
-        cli::format::tertiary(merged.patch)
+            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

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,
 }

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
+        &self.author
-            .revisions()
+    }
-            .next()
+
-            .map(|(_, r)| r)
+    /// All revision authors.
-            .expect("Patch::author: at least one revision is present")
+    pub fn authors(&self) -> BTreeSet<&Author> {
-            .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<Item = (RevisionId, &Revision)> {
+        self.revisions_by(self.author().public_key())
+    }
+
+    /// List of all patch revisions by all authors.
     pub fn revisions(&self) -> impl DoubleEndedIterator<Item = (RevisionId, &Revision)> {
-        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<Item = (RevisionId, &Revision)> {
+        self.revisions()
+            .filter(move |(_, r)| (r.author.public_key() == author))
+    }
+
     /// List of patch assignees.
     pub fn assignees(&self) -> impl Iterator<Item = Did> + '_ {
         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()
+        self.latest_by(self.author().public_key())
-            .next_back()
             .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.
+            // Anyone can propose revisions.
-            Action::Revision { .. } => Authorization::from(actor == author),
+            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)? {