From 0b7f169c8bcd51a5e01a83ec2a00fe5aea31207c Mon Sep 17 00:00:00 2001
From: Slack Coder <slackcoder@server.ky>
Date: Sat, 18 Mar 2023 10:53:44 -0500
Subject: [PATCH] cli: Support revision id in `rad merge`

Revision IDs are globally unique.  Users will be able to use the
Revision ID in place of its patch ID.

Make `rad merge` refuse to merge an older revision unless the `--force`
option is given.
---
 .../workflow/5-patching-maintainer.md         | 33 +++++++++
 radicle-cli/src/commands/merge.rs             | 70 +++++++++++--------
 radicle-cli/src/git.rs                        | 10 +--
 radicle-cob/src/history/entry.rs              |  7 ++
 radicle-term/src/io.rs                        |  2 +-
 radicle/src/cob/patch.rs                      | 35 +++++++++-
 6 files changed, 121 insertions(+), 36 deletions(-)

diff --git a/radicle-cli/examples/workflow/5-patching-maintainer.md b/radicle-cli/examples/workflow/5-patching-maintainer.md
index 8322d860..b1c8ec33 100644
--- a/radicle-cli/examples/workflow/5-patching-maintainer.md
+++ b/radicle-cli/examples/workflow/5-patching-maintainer.md
@@ -54,3 +54,36 @@ a07ef77 R1 (27857ec) -> R2 (f6484e0)
 
 ✓ Patch a07ef77 updated to 737dc47d9eba730c5db8a8e33c41c7955f9093de
 ```
+
+Great, all fixed up, lets merge the code.
+
+```
+$ git checkout master
+Your branch is up to date with 'rad/master'.
+$ rad merge 737dc47d9eba730c5db8a8e33c41c7955f9093de
+Merging a07ef77 R2 (f6484e0) by did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk into master (f2de534) via fast-forward...
+Running `git merge --ff-only f6484e0f43e48a8983b9b39bf9bd4cd889f1d520`...
+Updating f2de534..f6484e0
+Fast-forward
+ README.md       | 0
+ REQUIREMENTS.md | 0
+ 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 `rad push` to publish
+```
+
+The patch is now merged and closed :).
+
+```
+$ rad patch
+╭───────────────────────────────────────────────────────────────────────────────────────────────╮
+│ Define power requirements a07ef77 R2 f6484e0 (flux-capacitor-power, master) ahead 3, behind 0 │
+├───────────────────────────────────────────────────────────────────────────────────────────────┤
+│ ● opened by did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk 3 months ago             │
+│ ↑ updated to a07ef7743a32a2e902672ea3526d1db6ee08108a (3e674d1) 3 months ago                  │
+│ ↑ updated to 4f15eb5c994edd0bb6be29cb4801aa74308cc628 (27857ec) 3 months ago                  │
+│ ✓ merged by did:key:z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi (you) 3 months ago       │
+╰───────────────────────────────────────────────────────────────────────────────────────────────╯
+```
diff --git a/radicle-cli/src/commands/merge.rs b/radicle-cli/src/commands/merge.rs
index 1131dd37..af90ff26 100644
--- a/radicle-cli/src/commands/merge.rs
+++ b/radicle-cli/src/commands/merge.rs
@@ -1,14 +1,12 @@
 use std::ffi::OsString;
 use std::fmt;
 use std::fmt::Write;
-use std::str::FromStr;
 
 use anyhow::{anyhow, Context};
 
 use crate::git::Rev;
 use crate::terminal as term;
 use crate::terminal::args::{string, Args, Error, Help};
-use radicle::cob::patch::RevisionIx;
 use radicle::cob::patch::{Patch, PatchId, Patches};
 use radicle::git;
 use radicle::prelude::*;
@@ -21,14 +19,14 @@ pub const HELP: Help = Help {
     usage: r#"
 Usage
 
-    rad merge [<id>] [<option>...]
+    rad merge [<revision-id>] [<option>...]
 
-    To specify a patch to merge, use the fully qualified patch id.
+    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
-    -r, --revision <number>   Revision number to merge, defaults to the latest
         --help                Print help
 "#,
 };
@@ -71,11 +69,12 @@ pub enum State {
     Open,
     Merged,
 }
+
 #[derive(Debug)]
 pub struct Options {
-    pub id: Rev,
+    pub revision_id: Rev,
+    pub force: bool,
     pub interactive: bool,
-    pub revision: Option<RevisionIx>,
 }
 
 impl Args for Options {
@@ -83,8 +82,8 @@ impl Args for Options {
         use lexopt::prelude::*;
 
         let mut parser = lexopt::Parser::from_args(args);
-        let mut id: Option<Rev> = None;
-        let mut revision: Option<RevisionIx> = None;
+        let mut force = false;
+        let mut revision_id = None;
         let mut interactive = false;
 
         while let Some(arg) = parser.next()? {
@@ -92,30 +91,28 @@ impl Args for Options {
                 Long("help") => {
                     return Err(Error::Help.into());
                 }
+                Long("force") | Short('f') => {
+                    force = true;
+                }
                 Long("interactive") | Short('i') => {
                     interactive = true;
                 }
-                Long("revision") | Short('r') => {
-                    let value = parser.value()?;
-                    let id =
-                        RevisionIx::from_str(value.to_str().unwrap_or_default()).map_err(|_| {
-                            anyhow!("invalid revision number `{}`", value.to_string_lossy())
-                        })?;
-                    revision = Some(id);
-                }
                 Value(val) => {
                     let val = string(&val);
-                    id = Some(Rev::from(val));
+                    revision_id = Some(Rev::from(val));
                 }
                 _ => return Err(anyhow::anyhow!(arg.unexpected())),
             }
         }
 
+        let revision_id =
+            revision_id.ok_or_else(|| anyhow!("a revision id to merge must be provided"))?;
+
         Ok((
             Options {
-                id: id.ok_or_else(|| anyhow!("a patch id to merge must be provided"))?,
+                revision_id,
+                force,
                 interactive,
-                revision,
             },
             vec![],
         ))
@@ -144,10 +141,27 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> {
     //
     // Get patch information
     //
-    let patch_id = options.id.resolve(&repository.backend)?;
+    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}", &options.id.clone()))?;
+        .map_err(|e| anyhow!("couldn't find patch {} locally: {e}", &id))?;
 
     let head = repo.head()?;
     let branch = head
@@ -156,11 +170,6 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> {
     let head_oid = head
         .target()
         .ok_or_else(|| anyhow!("cannot merge into detatched head; aborting"))?;
-    let revision_ix = options.revision.unwrap_or_else(|| patch.version());
-    let (revision_id, revision) = patch
-        .revisions()
-        .nth(revision_ix)
-        .ok_or_else(|| anyhow!("revision R{} does not exist", revision_ix))?;
 
     //
     // Analyze merge
@@ -221,6 +230,11 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> {
             .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"),
@@ -261,7 +275,7 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> {
     // Update patch COB
     //
     // TODO: Don't allow merging the same revision twice?
-    patch.merge(*revision_id, head_oid.into(), &signer)?;
+    patch.merge(revision_id, head_oid.into(), &signer)?;
 
     term::success!(
         "Patch state updated, use {} to publish",
diff --git a/radicle-cli/src/git.rs b/radicle-cli/src/git.rs
index 3ad7f2cf..b3c2f84c 100644
--- a/radicle-cli/src/git.rs
+++ b/radicle-cli/src/git.rs
@@ -11,7 +11,6 @@ use std::str::FromStr;
 use anyhow::anyhow;
 use anyhow::Context as _;
 
-use radicle::cob::ObjectId;
 use radicle::crypto::ssh;
 use radicle::git;
 use radicle::git::raw as git2;
@@ -39,10 +38,13 @@ impl Rev {
         &self.0
     }
 
-    /// Resolve the revision to an [`ObjectId`].
-    pub fn resolve(&self, repo: &git2::Repository) -> Result<ObjectId, git2::Error> {
+    /// Resolve the revision to an [`From<git2::Oid>`].
+    pub fn resolve<T>(&self, repo: &git2::Repository) -> Result<T, git2::Error>
+    where
+        T: From<git2::Oid>,
+    {
         let object = repo.revparse_single(self.as_str())?;
-        Ok(ObjectId::from(object.id()))
+        Ok(object.id().into())
     }
 }
 
diff --git a/radicle-cob/src/history/entry.rs b/radicle-cob/src/history/entry.rs
index d761627f..6e4ddeb6 100644
--- a/radicle-cob/src/history/entry.rs
+++ b/radicle-cob/src/history/entry.rs
@@ -10,6 +10,7 @@ use nonempty::NonEmpty;
 use radicle_crypto::PublicKey;
 use serde::{Deserialize, Serialize};
 
+use crate::object;
 use crate::pruning_fold;
 
 /// Entry contents.
@@ -60,6 +61,12 @@ impl From<EntryId> for Oid {
     }
 }
 
+impl From<&EntryId> for object::ObjectId {
+    fn from(EntryId(id): &EntryId) -> Self {
+        id.into()
+    }
+}
+
 /// One entry in the dependency graph for a change
 #[derive(Clone, Debug, PartialEq, Eq, Hash)]
 pub struct Entry {
diff --git a/radicle-term/src/io.rs b/radicle-term/src/io.rs
index aaccdd36..1e107279 100644
--- a/radicle-term/src/io.rs
+++ b/radicle-term/src/io.rs
@@ -124,7 +124,7 @@ pub fn indented(msg: impl fmt::Display) {
 }
 
 pub fn subcommand(msg: impl fmt::Display) {
-    println!("{} {}", style("$").dim(), style(msg).dim());
+    println!("{}", style(format!("Running `{msg}`...")).dim());
 }
 
 pub fn warning(warning: &str) {
diff --git a/radicle/src/cob/patch.rs b/radicle/src/cob/patch.rs
index f48e04cf..a7f01c40 100644
--- a/radicle/src/cob/patch.rs
+++ b/radicle/src/cob/patch.rs
@@ -216,6 +216,13 @@ impl Patch {
             .author
     }
 
+    /// Get the `Revision` by its `RevisionId`.
+    ///
+    /// None is returned if the `Revision` has been redacted (deleted).
+    pub fn revision(&self, id: &RevisionId) -> Option<&Revision> {
+        self.revisions.get(id).and_then(Redactable::get)
+    }
+
     /// List of patch revisions. The initial changeset is part of the
     /// first revision.
     pub fn revisions(&self) -> impl DoubleEndedIterator<Item = (&RevisionId, &Revision)> {
@@ -1010,6 +1017,25 @@ impl<'a> Patches<'a> {
         Ok(state_groups)
     }
 
+    /// Find the `Patch` containing the given `Revision`.
+    pub fn find_by_revision(
+        &self,
+        id: &RevisionId,
+    ) -> Result<Option<(PatchId, Patch, Revision)>, Error> {
+        // Revision may be the patch's first, making it have the same ID.
+        let p_id = ObjectId::from(id);
+        if let Some(p) = self.get(&p_id)? {
+            return Ok(p.revision(id).map(|r| (p_id, p.clone(), r.clone())));
+        }
+
+        let result = self
+            .all()?
+            .into_iter()
+            .filter_map(|result| result.ok())
+            .find_map(|(p_id, p, _)| p.revision(id).map(|r| (p_id, p.clone(), r.clone())));
+        Ok(result)
+    }
+
     /// Get a patch.
     pub fn get(&self, id: &ObjectId) -> Result<Option<Patch>, store::Error> {
         self.raw.get(id).map(|r| r.map(|(p, _)| p))
@@ -1253,8 +1279,8 @@ mod test {
 
         assert_eq!(patch.clock.get(), 1);
 
-        let id = patch.id;
-        let patch = patches.get(&id).unwrap().unwrap();
+        let patch_id = patch.id;
+        let patch = patches.get(&patch_id).unwrap().unwrap();
 
         assert_eq!(patch.title(), "My first patch");
         assert_eq!(patch.description(), "Blah blah blah.");
@@ -1263,13 +1289,16 @@ mod test {
         assert_eq!(patch.target(), target);
         assert_eq!(patch.version(), 0);
 
-        let (_, revision) = patch.latest().unwrap();
+        let (rev_id, revision) = patch.latest().unwrap();
 
         assert_eq!(revision.author.id(), &author);
         assert_eq!(revision.description(), "");
         assert_eq!(revision.discussion.len(), 0);
         assert_eq!(revision.oid, oid);
         assert_eq!(revision.base, base);
+
+        let (id, _, _) = patches.find_by_revision(rev_id).unwrap().unwrap();
+        assert_eq!(id, patch_id);
     }
 
     #[test]