From 152be650539a7d3fbe3c0eff3d87d88fee94a9d4 Mon Sep 17 00:00:00 2001 From: cloudhead Date: Mon, 4 Sep 2023 15:41:56 +0200 Subject: [PATCH] cob: Wrap patch ID types with new types To prevent using eg. a `RevisionId` when a `ReviewId` is required, use compiler types. --- radicle-cli/src/commands/comment.rs | 4 +- radicle-cli/src/commands/patch/edit.rs | 2 +- radicle-cli/src/commands/patch/list.rs | 6 +- radicle-cli/src/commands/patch/redact.rs | 4 +- radicle-cli/src/commands/patch/show.rs | 2 +- radicle-cli/src/commands/review.rs | 13 +- radicle-remote-helper/src/push.rs | 1 - radicle-tools/src/rad-merge.rs | 2 +- radicle/src/cob/patch.rs | 171 +++++++++++++++-------- 9 files changed, 126 insertions(+), 79 deletions(-) diff --git a/radicle-cli/src/commands/comment.rs b/radicle-cli/src/commands/comment.rs index 72e110f1..bb5ca4e7 100644 --- a/radicle-cli/src/commands/comment.rs +++ b/radicle-cli/src/commands/comment.rs @@ -122,8 +122,8 @@ fn comment( let mut patches = Patches::open(repo)?; match patches.get_mut(&id) { Ok(mut patch) => { - let (revision_id, _) = patch.revisions().last().expect("patch has a revision"); - let comment_id = patch.comment(*revision_id, message, options.reply_to, &signer)?; + let (revision_id, _) = patch.latest(); + let comment_id = patch.comment(revision_id, message, options.reply_to, &signer)?; term::print(comment_id); return Ok(()); diff --git a/radicle-cli/src/commands/patch/edit.rs b/radicle-cli/src/commands/patch/edit.rs index a7053062..047b9147 100644 --- a/radicle-cli/src/commands/patch/edit.rs +++ b/radicle-cli/src/commands/patch/edit.rs @@ -36,7 +36,7 @@ pub fn run( return Ok(()); } - let root = *patch.id; + let (root, _) = patch.root(); let target = patch.target(); patch.transaction("Edit", &signer, |tx| { diff --git a/radicle-cli/src/commands/patch/list.rs b/radicle-cli/src/commands/patch/list.rs index 1d0357fe..273e5187 100644 --- a/radicle-cli/src/commands/patch/list.rs +++ b/radicle-cli/src/commands/patch/list.rs @@ -122,7 +122,6 @@ pub fn row( pub fn timeline( profile: &Profile, - patch_id: &PatchId, patch: &Patch, repository: &Repository, ) -> anyhow::Result> { @@ -134,10 +133,11 @@ pub fn timeline( .extend(Author::new(patch.author().id(), profile).line()); let mut timeline = vec![(patch.timestamp(), open)]; + let (root, _) = patch.root(); for (revision_id, revision) in patch.revisions() { // Don't show an "update" line for the first revision. - if *revision_id != **patch_id { + if revision_id != root { timeline.push(( revision.timestamp(), term::Line::spaced( @@ -155,7 +155,7 @@ pub fn timeline( )); } - for (nid, merge) in patch.merges().filter(|(_, m)| m.revision == *revision_id) { + for (nid, merge) in patch.merges().filter(|(_, m)| m.revision == revision_id) { let peer = repository.remote(nid)?; let line = term::Line::spaced([ term::format::primary("✓").bold().into(), diff --git a/radicle-cli/src/commands/patch/redact.rs b/radicle-cli/src/commands/patch/redact.rs index 7d7b5162..9107cb59 100644 --- a/radicle-cli/src/commands/patch/redact.rs +++ b/radicle-cli/src/commands/patch/redact.rs @@ -14,8 +14,8 @@ pub fn run( let signer = &term::signer(profile)?; let mut patches = patch::Patches::open(repository)?; - let revision_id = revision_id.resolve::(&repository.backend)?; - let Some((patch_id, _, _)) = patches.find_by_revision(&revision_id)? else { + let revision_id = revision_id.resolve(&repository.backend)?; + let Some((patch_id, _, revision_id, _)) = patches.find_by_revision(revision_id)? else { anyhow::bail!("patch revision `{revision_id}` not found"); }; let Ok(mut patch) = patches.get_mut(&patch_id) else { diff --git a/radicle-cli/src/commands/patch/show.rs b/radicle-cli/src/commands/patch/show.rs index 98033353..71773e88 100644 --- a/radicle-cli/src/commands/patch/show.rs +++ b/radicle-cli/src/commands/patch/show.rs @@ -138,7 +138,7 @@ pub fn run( .children(commits.into_iter().map(|l| l.boxed())) .divider(); - for line in list::timeline(profile, patch_id, &patch, stored)? { + for line in list::timeline(profile, &patch, stored)? { widget.push(line); } widget.print(); diff --git a/radicle-cli/src/commands/review.rs b/radicle-cli/src/commands/review.rs index 1298c2f8..a73fd230 100644 --- a/radicle-cli/src/commands/review.rs +++ b/radicle-cli/src/commands/review.rs @@ -5,7 +5,7 @@ use std::ffi::OsString; use anyhow::{anyhow, Context}; -use radicle::cob::patch::{PatchId, Patches, RevisionId, Verdict}; +use radicle::cob::patch::{PatchId, Patches, Verdict}; use radicle::prelude::*; use radicle::{git, rad}; @@ -209,11 +209,12 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { let mut patches = Patches::open(&repository)?; let (patch_id, revision) = if options.revision { - let id = options.id.resolve::(&repository.backend)?; - let (patch_id, _, rev) = patches - .find_by_revision(&id)? + let id = options.id.resolve(&repository.backend)?; + let (patch_id, _, rev_id, rev) = patches + .find_by_revision(id)? .ok_or_else(|| anyhow!("revision {} does not exist", id))?; - (patch_id, Some((id, rev))) + + (patch_id, Some((rev_id, rev))) } else { let id = options.id.resolve::(&repository.backend)?; (id, None) @@ -227,7 +228,7 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { v } else { let (id, r) = patch.latest(); - (*id, r.clone()) + (id, r.clone()) }; let patch_id_pretty = term::format::tertiary(term::format::cob(&patch_id)); diff --git a/radicle-remote-helper/src/push.rs b/radicle-remote-helper/src/push.rs index 0c8eeba6..6eeb7914 100644 --- a/radicle-remote-helper/src/push.rs +++ b/radicle-remote-helper/src/push.rs @@ -467,7 +467,6 @@ fn patch_merge_all( let (revision_id, revision) = patch.latest(); if patch.is_open() && commits.contains(&revision.head()) { - let revision_id = *revision_id; let patch = patch::PatchMut::new(id, patch, &mut patches); patch_merge(patch, revision_id, new, working, signer)?; diff --git a/radicle-tools/src/rad-merge.rs b/radicle-tools/src/rad-merge.rs index bb4b24cf..49fcf660 100644 --- a/radicle-tools/src/rad-merge.rs +++ b/radicle-tools/src/rad-merge.rs @@ -37,7 +37,7 @@ fn main() -> anyhow::Result<()> { let signer = term::signer(&profile)?; patch - .merge(*revision, head, &signer)? + .merge(revision, head, &signer)? .cleanup(&working, &signer)?; println!("✓ Patch {pid} merged at commit {head}"); diff --git a/radicle/src/cob/patch.rs b/radicle/src/cob/patch.rs index 23eef35d..3157967b 100644 --- a/radicle/src/cob/patch.rs +++ b/radicle/src/cob/patch.rs @@ -6,6 +6,7 @@ use std::ops::Range; use std::path::PathBuf; use std::str::FromStr; +use amplify::Wrapper; use once_cell::sync::Lazy; use serde::ser::SerializeStruct; use serde::{Deserialize, Serialize}; @@ -38,7 +39,44 @@ pub type Op = cob::Op; pub type PatchId = ObjectId; /// Unique identifier for a patch revision. -pub type RevisionId = EntryId; +#[derive( + Wrapper, + Debug, + Clone, + Copy, + Serialize, + Deserialize, + PartialEq, + Eq, + PartialOrd, + Ord, + Hash, + From, + Display, +)] +#[display(inner)] +#[wrap(Deref)] +pub struct RevisionId(EntryId); + +/// Unique identifier for a patch review. +#[derive( + Wrapper, + Debug, + Clone, + Copy, + Serialize, + Deserialize, + PartialEq, + Eq, + PartialOrd, + Ord, + Hash, + From, + Display, +)] +#[display(inner)] +#[wrapper(Deref)] +pub struct ReviewId(EntryId); /// Index of a revision in the revisions list. pub type RevisionIx = usize; @@ -117,7 +155,7 @@ pub enum Action { }, #[serde(rename = "review.edit")] ReviewEdit { - review: EntryId, + review: ReviewId, #[serde(default, skip_serializing_if = "Option::is_none")] summary: Option, }, @@ -125,7 +163,7 @@ pub enum Action { ReviewRedact { review: EntryId }, #[serde(rename = "review.comment")] ReviewComment { - review: EntryId, + review: ReviewId, body: String, #[serde(default, skip_serializing_if = "Option::is_none")] location: Option, @@ -137,23 +175,23 @@ pub enum Action { }, #[serde(rename = "review.comment.edit")] ReviewCommentEdit { - review: EntryId, + review: ReviewId, comment: EntryId, body: String, }, #[serde(rename = "review.comment.redact")] - ReviewCommentRedact { review: EntryId, comment: EntryId }, + ReviewCommentRedact { review: ReviewId, comment: EntryId }, #[serde(rename = "review.comment.react")] ReviewCommentReact { - review: EntryId, + review: ReviewId, comment: EntryId, reaction: Reaction, active: bool, }, #[serde(rename = "review.comment.resolve")] - ReviewCommentResolve { review: EntryId, comment: EntryId }, + ReviewCommentResolve { review: ReviewId, comment: EntryId }, #[serde(rename = "review.comment.unresolve")] - ReviewCommentUnresolve { review: EntryId, comment: EntryId }, + ReviewCommentUnresolve { review: ReviewId, comment: EntryId }, // // Revision actions @@ -330,7 +368,7 @@ pub struct Patch { /// Timeline of operations. pub(super) timeline: Vec, /// Reviews index. Keeps track of reviews for better performance. - pub(super) reviews: BTreeMap>, + pub(super) reviews: BTreeMap>, } impl Patch { @@ -365,7 +403,8 @@ impl Patch { /// Patch description. pub fn description(&self) -> &str { - self.root().description() + let (_, r) = self.root(); + r.description() } /// Author of the first revision of the patch. @@ -387,12 +426,12 @@ impl Patch { /// List of patch revisions. The initial changeset is part of the /// first revision. - pub fn revisions(&self) -> impl DoubleEndedIterator { + pub fn revisions(&self) -> impl DoubleEndedIterator { self.timeline.iter().filter_map(|id| { self.revisions .get(id) .and_then(|o| o.as_ref()) - .map(|rev| (id, rev)) + .map(|rev| (RevisionId(*id), rev)) }) } @@ -445,15 +484,14 @@ impl Patch { /// Root revision. /// /// This is the revision that was created with the patch. - pub fn root(&self) -> &Revision { + pub fn root(&self) -> (RevisionId, &Revision) { self.revisions() .next() - .map(|(_, r)| r) .expect("Patch::root: there is always a root revision") } /// Latest revision. - pub fn latest(&self) -> (&RevisionId, &Revision) { + pub fn latest(&self) -> (RevisionId, &Revision) { self.revisions() .next_back() .expect("Patch::latest: there is always at least one revision") @@ -544,22 +582,22 @@ impl store::FromHistory for Patch { revision.description = description; } } else { - return Err(Error::Missing(revision)); + return Err(Error::Missing(revision.into_inner())); } } Action::ReviewEdit { review, summary } => { let Some(Some((revision, author))) = self.reviews.get(&review) else { - return Err(Error::Missing(review)); + return Err(Error::Missing(review.into_inner())); }; let Some(rev) = self.revisions.get_mut(revision) else { - return Err(Error::Missing(*revision)); + return Err(Error::Missing(revision.into_inner())); }; // If the revision was redacted concurrently, there's nothing to do. // Likewise, if the review was redacted concurrently, there's nothing to do. if let Some(rev) = rev { let Some(review) = rev.reviews.get_mut(author) else { - return Err(Error::Missing(review)); + return Err(Error::Missing(review.into_inner())); }; if let Some(review) = review { review.summary = summary; @@ -575,7 +613,7 @@ impl store::FromHistory for Patch { debug_assert!(!self.revisions.contains_key(&op.id)); self.revisions.insert( - op.id, + RevisionId(op.id), Some(Revision::new( author.clone(), description, @@ -599,7 +637,7 @@ impl store::FromHistory for Patch { } *r = None; } else { - return Err(Error::Missing(revision)); + return Err(Error::Missing(revision.into_inner())); } } Action::Review { @@ -609,7 +647,7 @@ impl store::FromHistory for Patch { labels, } => { let Some(rev) = self.revisions.get_mut(&revision) else { - return Err(Error::Missing(revision)); + return Err(Error::Missing(revision.into_inner())); }; if let Some(rev) = rev { // Nb. Applying two reviews by the same author is not allowed and @@ -619,7 +657,8 @@ impl store::FromHistory for Patch { Some(Review::new(verdict, summary.to_owned(), labels, timestamp)), ); // Update reviews index. - self.reviews.insert(op.id, Some((revision, op.author))); + self.reviews + .insert(ReviewId(op.id), Some((revision, op.author))); } } Action::ReviewCommentReact { @@ -831,20 +870,20 @@ mod lookup { // Redacted. Some(None) => Ok(None), // Missing. Causal error. - None => Err(Error::Missing(*revision)), + None => Err(Error::Missing(revision.into_inner())), } } pub fn review<'a>( patch: &'a mut Patch, - review: &EntryId, + review: &ReviewId, ) -> Result, Error> { match patch.reviews.get(review) { Some(Some((revision, author))) => { match patch.revisions.get_mut(revision) { Some(Some(r)) => { let Some(review) = r.reviews.get_mut(author) else { - return Err(Error::Missing(*review)); + return Err(Error::Missing(review.into_inner())); }; Ok(review.as_mut()) } @@ -853,14 +892,14 @@ mod lookup { // Likewise, if the review was redacted concurrently, there's nothing to do. Ok(None) } - None => Err(Error::Missing(*revision)), + None => Err(Error::Missing(revision.into_inner())), } } Some(None) => { // Redacted. Ok(None) } - None => Err(Error::Missing(*review)), + None => Err(Error::Missing(review.into_inner())), } } } @@ -1144,7 +1183,7 @@ impl store::Transaction { pub fn edit_review( &mut self, - review: EntryId, + review: ReviewId, summary: Option, ) -> Result<(), store::Error> { self.push(Action::ReviewEdit { review, summary }) @@ -1226,7 +1265,7 @@ impl store::Transaction { /// Comment on a review. pub fn review_comment( &mut self, - review: EntryId, + review: ReviewId, body: S, location: Option, reply_to: Option, @@ -1242,7 +1281,7 @@ impl store::Transaction { /// Edit review comment. pub fn edit_review_comment( &mut self, - review: EntryId, + review: ReviewId, comment: EntryId, body: S, ) -> Result<(), store::Error> { @@ -1256,7 +1295,7 @@ impl store::Transaction { /// React to a review comment. pub fn react_review_comment( &mut self, - review: EntryId, + review: ReviewId, comment: EntryId, reaction: Reaction, active: bool, @@ -1272,7 +1311,7 @@ impl store::Transaction { /// Redact a review comment. pub fn redact_review_comment( &mut self, - review: EntryId, + review: ReviewId, comment: EntryId, ) -> Result<(), store::Error> { self.push(Action::ReviewCommentRedact { review, comment }) @@ -1390,7 +1429,7 @@ where /// Edit review. pub fn edit_review( &mut self, - review: EntryId, + review: ReviewId, summary: Option, signer: &G, ) -> Result { @@ -1403,7 +1442,7 @@ where revision: RevisionId, signer: &G, ) -> Result { - if revision == RevisionId::from(*self.id) { + if revision.0 == *self.id { return Err(Error::RootRevision(revision)); } self.transaction("Redact revision", signer, |tx| tx.redact(revision)) @@ -1472,7 +1511,7 @@ where /// Comment on a line of code as part of a review. pub fn review_comment( &mut self, - review: EntryId, + review: ReviewId, body: S, location: Option, reply_to: Option, @@ -1486,7 +1525,7 @@ where /// Edit review comment. pub fn edit_review_comment( &mut self, - review: EntryId, + review: ReviewId, comment: EntryId, body: S, signer: &G, @@ -1499,7 +1538,7 @@ where /// React to a review comment. pub fn react_review_comment( &mut self, - review: EntryId, + review: ReviewId, comment: EntryId, reaction: Reaction, active: bool, @@ -1513,7 +1552,7 @@ where /// React to a review comment. pub fn redact_review_comment( &mut self, - review: EntryId, + review: ReviewId, comment: EntryId, signer: &G, ) -> Result { @@ -1530,10 +1569,11 @@ where comment: Option, labels: Vec