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 <me@sebastinez.dev>

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
     );
 

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(

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 {

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")),

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(());
     }

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,
+                "base": rev.base(),
-                "oid": rev.oid,
+                "oid": rev.head(),
-                "refs": get_refs(repo, patch.author().id(), &rev.oid).unwrap_or(vec![]),
+                "refs": get_refs(repo, patch.author().id(), &rev.head()).unwrap_or(vec![]),
                 "merges": rev.merges().collect::<Vec<_>>(),
-                "discussions": rev.discussion
+                "discussions": rev.discussion().comments()
-                  .comments()
+                  .map(|(id, comment)| Comment::new(id, comment, rev.discussion()))
-                  .map(|(id, comment)| Comment::new(id, comment, &rev.discussion))
                   .collect::<Vec<_>>(),
-                "timestamp": rev.timestamp,
+                "timestamp": rev.timestamp(),
                 "reviews": rev.reviews().collect::<Vec<_>>(),
             })
         }).collect::<Vec<_>>(),

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<Max<String>>,
+    title: LWWReg<Max<String>>,
     /// Patch description.
-    pub description: LWWReg<Max<String>>,
+    description: LWWReg<Max<String>>,
     /// Current state of the patch.
-    pub state: LWWReg<Max<State>>,
+    state: LWWReg<Max<State>>,
     /// Target this patch is meant to be merged in.
-    pub target: LWWReg<Max<MergeTarget>>,
+    target: LWWReg<Max<MergeTarget>>,
     /// Associated tags.
-    pub tags: LWWSet<Tag>,
+    tags: LWWSet<Tag>,
     /// List of patch revisions. The initial changeset is part of the
     /// first revision.
-    pub revisions: GMap<RevisionId, Redactable<Revision>>,
+    revisions: GMap<RevisionId, Redactable<Revision>>,
     /// 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<Item = &Tag> {
         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<Item = (&RevisionId, &Revision)> {
         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<Max<String>>,
+    description: LWWReg<Max<String>>,
     /// 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<Max<Merge>>,
+    merges: LWWSet<Max<Merge>>,
     /// Reviews of this revision's changes (one per actor).
-    pub reviews: GMap<ActorId, Review>,
+    reviews: GMap<ActorId, Review>,
     /// 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<Item = &Merge> {
         self.merges.iter().map(|m| m.get())
     }
 
+    /// Reviews of this revision's changes (one per actor).
     pub fn reviews(&self) -> impl DoubleEndedIterator<Item = (&PublicKey, &Review)> {
         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<Option<Verdict>>,
+    verdict: LWWReg<Option<Verdict>>,
     /// Review general comment.
-    pub comment: LWWReg<Option<Max<String>>>,
+    comment: LWWReg<Option<Max<String>>>,
     /// Review inline code comments.
-    pub inline: LWWSet<Max<CodeComment>>,
+    inline: LWWSet<Max<CodeComment>>,
     /// Review timestamp.
-    pub timestamp: Max<Timestamp>,
+    timestamp: Max<Timestamp>,
 }
 
 impl Serialize for Review {
@@ -596,18 +654,22 @@ impl Review {
         }
     }
 
+    /// Review verdict.
     pub fn verdict(&self) -> Option<Verdict> {
         self.verdict.get().as_ref().copied()
     }
 
+    /// Review inline code comments.
     pub fn inline(&self) -> impl Iterator<Item = &CodeComment> {
         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()
     }