From 87f47cdc67658ae3d5b274b3c20cc24ee3292c90 Mon Sep 17 00:00:00 2001 From: Sebastian Martinez Date: Thu, 30 Mar 2023 17:45:00 +0200 Subject: [PATCH] Reduce pub accessors in patch and issue cob structs Replaces pub accessors to struct fields with methods Also creates some methods to compute necessary information Signed-off-by: Sebastian Martinez --- radicle-cli/src/commands/merge.rs | 12 +-- radicle-cli/src/commands/patch/checkout.rs | 4 +- radicle-cli/src/commands/patch/common.rs | 2 +- radicle-cli/src/commands/patch/list.rs | 12 +-- radicle-cli/src/commands/patch/update.rs | 8 +- radicle-httpd/src/api/json.rs | 13 ++- radicle/src/cob/patch.rs | 106 ++++++++++++++++----- 7 files changed, 109 insertions(+), 48 deletions(-) diff --git a/radicle-cli/src/commands/merge.rs b/radicle-cli/src/commands/merge.rs index c70262ee..1131dd37 100644 --- a/radicle-cli/src/commands/merge.rs +++ b/radicle-cli/src/commands/merge.rs @@ -166,7 +166,7 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { // Analyze merge // let patch_commit = repo - .find_annotated_commit(revision.oid.into()) + .find_annotated_commit(revision.head().into()) .context("patch head not found in local repository")?; let (merge, _merge_pref) = repo.merge_analysis(&[&patch_commit])?; @@ -180,7 +180,7 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { // // Let's check if there are potential merge conflicts. let our_commit = head.peel_to_commit()?; - let their_commit = repo.find_commit(revision.oid.into())?; + let their_commit = repo.find_commit(revision.head().into())?; let index = repo .merge_commits(&our_commit, &their_commit, None) @@ -207,7 +207,7 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { anyhow::bail!( "no merge is possible between {} and {}", head_oid, - revision.oid + revision.head() ); }; @@ -226,7 +226,7 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { 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.oid))), + term::format::parens(term::format::secondary(term::format::oid(revision.head()))), term::format::tertiary(patch.author().id), term::format::highlight(branch), term::format::parens(term::format::secondary(term::format::oid(head_oid))), @@ -245,7 +245,7 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { merge_commit(&repo, patch_id, &patch_commit, &patch, signer.public_key())?; } MergeStyle::FastForward => { - fast_forward(&repo, &revision.oid)?; + fast_forward(&repo, &revision.head())?; } } @@ -253,7 +253,7 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { "Updated {} {} -> {} via {}", term::format::highlight(branch), term::format::secondary(term::format::oid(head_oid)), - term::format::secondary(term::format::oid(revision.oid)), + term::format::secondary(term::format::oid(revision.head())), merge_style_pretty ); diff --git a/radicle-cli/src/commands/patch/checkout.rs b/radicle-cli/src/commands/patch/checkout.rs index f54184fb..6148a6dc 100644 --- a/radicle-cli/src/commands/patch/checkout.rs +++ b/radicle-cli/src/commands/patch/checkout.rs @@ -56,7 +56,7 @@ fn find_patch_commit<'a>( let (_, rev) = patch .latest() .ok_or(anyhow!("patch does not have any revisions"))?; - let author = **rev.author.id(); + let author = *rev.author().id(); let remote = stored.remote(&author)?; // Find a ref in storage that points to our patch, so that we can fetch the patch @@ -70,7 +70,7 @@ fn find_patch_commit<'a>( &RefString::try_from(author.to_string())?, patch_branch, ); - let url = git::Url::from(stored.id).with_namespace(author); + let url = git::Url::from(stored.id).with_namespace(*author); // Fetch only the ref pointing to the patch revision. working.remote_anonymous(url.to_string().as_str())?.fetch( diff --git a/radicle-cli/src/commands/patch/common.rs b/radicle-cli/src/commands/patch/common.rs index e28ccad1..8de35791 100644 --- a/radicle-cli/src/commands/patch/common.rs +++ b/radicle-cli/src/commands/patch/common.rs @@ -185,7 +185,7 @@ pub fn find_unmerged_with_base( for (id, patch, clock) in proposed { let (_, rev) = patch.latest().unwrap(); - if !rev.merges.is_empty() { + if !rev.merges().count() == 0 { continue; } if **patch.head() == patch_head { diff --git a/radicle-cli/src/commands/patch/list.rs b/radicle-cli/src/commands/patch/list.rs index 5a0f6f02..025ff5c5 100644 --- a/radicle-cli/src/commands/patch/list.rs +++ b/radicle-cli/src/commands/patch/list.rs @@ -86,11 +86,11 @@ fn print( term::format::dim(format!("R{}", patch.version())).into(), ]) .space() - .extend(common::pretty_commit_version(&revision.oid, workdir)?) + .extend(common::pretty_commit_version(&revision.head(), workdir)?) .space() .extend(common::pretty_sync_status( repository.raw(), - *revision.oid, + revision.head().into(), target_head, )?), ) @@ -103,14 +103,14 @@ fn print( // Don't show an "update" line for the first revision. if revision_id != latest { timeline.push(( - revision.timestamp, + 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.oid, + revision.head(), ))) .into(), ] @@ -119,7 +119,7 @@ fn print( )); } - for merge in revision.merges.iter() { + for merge in revision.merges() { let peer = repository.remote(&merge.node)?; let mut badges = Vec::new(); @@ -144,7 +144,7 @@ fn print( ), )); } - for (reviewer, review) in revision.reviews.iter() { + for (reviewer, review) in revision.reviews() { let verdict = match review.verdict() { Some(Verdict::Accept) => term::format::positive(term::format::dim("✓ accepted")), Some(Verdict::Reject) => term::format::negative(term::format::dim("✗ rejected")), diff --git a/radicle-cli/src/commands/patch/update.rs b/radicle-cli/src/commands/patch/update.rs index 63a5f3c4..277d11d4 100644 --- a/radicle-cli/src/commands/patch/update.rs +++ b/radicle-cli/src/commands/patch/update.rs @@ -57,15 +57,15 @@ fn show_update_commit_info( term::format::tertiary(term::format::cob(patch_id)), term::format::dim(format!("R{current_version}")), term::format::parens(term::format::secondary(term::format::oid( - current_revision.oid + current_revision.head() ))), term::format::dim(format!("R{}", current_version + 1)), - term::format::parens(term::format::secondary(term::format::oid(*head_oid))), + term::format::parens(term::format::secondary(term::format::oid(head_oid))), ); // Difference between the two revisions. let head_oid = branch_oid(head_branch)?; - term::patch::print_commits_ahead_behind(workdir, *head_oid, *current_revision.oid)?; + term::patch::print_commits_ahead_behind(workdir, *head_oid, *current_revision.head())?; Ok(()) } @@ -97,7 +97,7 @@ pub fn run( // TODO(cloudhead): Handle error. let (_, current_revision) = patch.latest().unwrap(); - if *current_revision.oid == *branch_oid(&head_branch)? { + if current_revision.head() == branch_oid(&head_branch)? { term::info!("Nothing to do, patch is already up to date."); return Ok(()); } diff --git a/radicle-httpd/src/api/json.rs b/radicle-httpd/src/api/json.rs index 91482622..996db6ca 100644 --- a/radicle-httpd/src/api/json.rs +++ b/radicle-httpd/src/api/json.rs @@ -121,15 +121,14 @@ pub(crate) fn patch(id: PatchId, patch: Patch, repo: &git::Repository) -> Value json!({ "id": id, "description": rev.description(), - "base": rev.base, - "oid": rev.oid, - "refs": get_refs(repo, patch.author().id(), &rev.oid).unwrap_or(vec![]), + "base": rev.base(), + "oid": rev.head(), + "refs": get_refs(repo, patch.author().id(), &rev.head()).unwrap_or(vec![]), "merges": rev.merges().collect::>(), - "discussions": rev.discussion - .comments() - .map(|(id, comment)| Comment::new(id, comment, &rev.discussion)) + "discussions": rev.discussion().comments() + .map(|(id, comment)| Comment::new(id, comment, rev.discussion())) .collect::>(), - "timestamp": rev.timestamp, + "timestamp": rev.timestamp(), "reviews": rev.reviews().collect::>(), }) }).collect::>(), diff --git a/radicle/src/cob/patch.rs b/radicle/src/cob/patch.rs index b7c92511..5b218e2d 100644 --- a/radicle/src/cob/patch.rs +++ b/radicle/src/cob/patch.rs @@ -130,20 +130,20 @@ pub enum MergeTarget { #[derive(Debug, Clone, PartialEq, Eq)] pub struct Patch { /// Title of the patch. - pub title: LWWReg>, + title: LWWReg>, /// Patch description. - pub description: LWWReg>, + description: LWWReg>, /// Current state of the patch. - pub state: LWWReg>, + state: LWWReg>, /// Target this patch is meant to be merged in. - pub target: LWWReg>, + target: LWWReg>, /// Associated tags. - pub tags: LWWSet, + tags: LWWSet, /// List of patch revisions. The initial changeset is part of the /// first revision. - pub revisions: GMap>, + revisions: GMap>, /// Timeline of operations. - pub timeline: GSet<(Lamport, EntryId)>, + timeline: GSet<(Lamport, EntryId)>, } impl Semilattice for Patch { @@ -172,18 +172,22 @@ impl Default for Patch { } impl Patch { + /// Title of the patch. pub fn title(&self) -> &str { self.title.get().get() } + /// Current state of the patch. pub fn state(&self) -> State { *self.state.get().get() } + /// Target this patch is meant to be merged in. pub fn target(&self) -> MergeTarget { *self.target.get().get() } + /// Timestamp of the first revision of the patch. pub fn timestamp(&self) -> Timestamp { self.revisions() .next() @@ -192,14 +196,17 @@ impl Patch { .timestamp } + /// Associated tags. pub fn tags(&self) -> impl Iterator { self.tags.iter() } + /// Patch description. pub fn description(&self) -> &str { self.description.get().get() } + /// Author of the first revision of the patch. pub fn author(&self) -> &Author { &self .revisions() @@ -209,6 +216,8 @@ impl Patch { .author } + /// List of patch revisions. The initial changeset is part of the + /// first revision. pub fn revisions(&self) -> impl DoubleEndedIterator { self.timeline.iter().filter_map(|(_, id)| { self.revisions @@ -218,6 +227,7 @@ impl Patch { }) } + /// Reference to the Git object containing the code on the latest revision. pub fn head(&self) -> &git::Oid { &self .latest() @@ -226,6 +236,7 @@ impl Patch { .oid } + /// Index of latest revision in the revisions list. pub fn version(&self) -> RevisionIx { self.revisions .len() @@ -233,14 +244,17 @@ impl Patch { .expect("Patch::version: at least one revision is present") } + /// Latest revision. pub fn latest(&self) -> Option<(&RevisionId, &Revision)> { self.revisions().next_back() } + /// Check if the patch is open. pub fn is_open(&self) -> bool { matches!(self.state.get().get(), State::Open) } + /// Check if the patch is archived. pub fn is_archived(&self) -> bool { matches!(self.state.get().get(), &State::Archived) } @@ -379,21 +393,21 @@ impl store::FromHistory for Patch { #[derive(Debug, Clone, PartialEq, Eq)] pub struct Revision { /// Author of the revision. - pub author: Author, + author: Author, /// Revision description. - pub description: LWWReg>, + description: LWWReg>, /// Base branch commit, used as a merge base. - pub base: git::Oid, + base: git::Oid, /// Reference to the Git object containing the code (revision head). - pub oid: git::Oid, + oid: git::Oid, /// Discussion around this revision. - pub discussion: Thread, + discussion: Thread, /// Merges of this revision into other repositories. - pub merges: LWWSet>, + merges: LWWSet>, /// Reviews of this revision's changes (one per actor). - pub reviews: GMap, + reviews: GMap, /// When this revision was created. - pub timestamp: Timestamp, + timestamp: Timestamp, } impl Revision { @@ -420,10 +434,37 @@ impl Revision { self.description.get() } + /// Author of the revision. + pub fn author(&self) -> &Author { + &self.author + } + + /// Base branch commit, used as a merge base. + pub fn base(&self) -> &git::Oid { + &self.base + } + + /// Reference to the Git object containing the code (revision head). + pub fn head(&self) -> git::Oid { + self.oid + } + + /// When this revision was created. + pub fn timestamp(&self) -> Timestamp { + self.timestamp + } + + /// Discussion around this revision. + pub fn discussion(&self) -> &Thread { + &self.discussion + } + + /// Merges of this revision into other repositories. pub fn merges(&self) -> impl Iterator { self.merges.iter().map(|m| m.get()) } + /// Reviews of this revision's changes (one per actor). pub fn reviews(&self) -> impl DoubleEndedIterator { self.reviews.iter() } @@ -532,24 +573,41 @@ impl Ord for CodeLocation { #[serde(rename_all = "camelCase")] pub struct CodeComment { /// Code location of the comment. - pub location: CodeLocation, + location: CodeLocation, /// Comment. - pub comment: String, + comment: String, /// Timestamp. - pub timestamp: Timestamp, + timestamp: Timestamp, +} + +impl CodeComment { + /// Code location of the comment. + pub fn location(&self) -> &CodeLocation { + &self.location + } + + /// Comment. + pub fn comment(&self) -> &str { + &self.comment + } + + /// Timestamp. + pub fn timestamp(&self) -> &Timestamp { + &self.timestamp + } } /// A patch review on a revision. #[derive(Debug, Default, Clone, PartialEq, Eq)] pub struct Review { /// Review verdict. - pub verdict: LWWReg>, + verdict: LWWReg>, /// Review general comment. - pub comment: LWWReg>>, + comment: LWWReg>>, /// Review inline code comments. - pub inline: LWWSet>, + inline: LWWSet>, /// Review timestamp. - pub timestamp: Max, + timestamp: Max, } impl Serialize for Review { @@ -596,18 +654,22 @@ impl Review { } } + /// Review verdict. pub fn verdict(&self) -> Option { self.verdict.get().as_ref().copied() } + /// Review inline code comments. pub fn inline(&self) -> impl Iterator { self.inline.iter().map(|m| m.get()) } + /// Review general comment. pub fn comment(&self) -> Option<&str> { self.comment.get().as_ref().map(|m| m.get().as_str()) } + /// Review timestamp. pub fn timestamp(&self) -> Timestamp { *self.timestamp.get() }