diff --git a/radicle-cli/examples/rad-review-by-hunk.md b/radicle-cli/examples/rad-review-by-hunk.md index 2abaaba6..d21b15e1 100644 --- a/radicle-cli/examples/rad-review-by-hunk.md +++ b/radicle-cli/examples/rad-review-by-hunk.md @@ -125,7 +125,7 @@ $ rad patch review --patch --accept --hunk 1 7a2ac7e2841cc1e7394f99f107555a499b1 │ @@ -1,1 +0,0 @@ │ │ 1 - *.draft │ ╰──────────────────────╯ -✓ Updated review tree to a5fccf0e977225ff13c3f74c43faf4cb679bf835 +✓ Updated brain to a5fccf0e977225ff13c3f74c43faf4cb679bf835 ``` ``` $ rad patch review --patch --accept --hunk 1 7a2ac7e2841cc1e7394f99f107555a499b1d3f23 --no-announce @@ -136,7 +136,7 @@ $ rad patch review --patch --accept --hunk 1 7a2ac7e2841cc1e7394f99f107555a499b1 │ @@ -0,0 +1,1 @@ │ │ 1 + All food is served as-is, with no warranty! │ ╰──────────────────────────────────────────────────────────╯ -✓ Updated review tree to 2cdb82ea726e64d3b52847c7699d0d4759198f5c +✓ Updated brain to 2cdb82ea726e64d3b52847c7699d0d4759198f5c ``` ``` $ rad patch review --patch --accept -U3 --hunk 1 7a2ac7e2841cc1e7394f99f107555a499b1d3f23 --no-announce @@ -152,7 +152,7 @@ $ rad patch review --patch --accept -U3 --hunk 1 7a2ac7e2841cc1e7394f99f107555a4 │ 4 5 Mac & Cheese │ │ 5 6 │ ╰─────────────────────────────╯ -✓ Updated review tree to d4aecbb859a802a3215def0b538358bf63593953 +✓ Updated brain to d4aecbb859a802a3215def0b538358bf63593953 ``` ``` $ rad patch review --patch --accept -U3 --hunk 1 7a2ac7e2841cc1e7394f99f107555a499b1d3f23 --no-announce @@ -168,7 +168,7 @@ $ rad patch review --patch --accept -U3 --hunk 1 7a2ac7e2841cc1e7394f99f107555a4 │ 15 + French Fries! │ │ 16 + Garlic Green Beans │ ╰───────────────────────────────────────╯ -✓ Updated review tree to 59cee720b0642b1491b241400912b35926a76c3f +✓ Updated brain to 59cee720b0642b1491b241400912b35926a76c3f ``` ``` @@ -177,7 +177,7 @@ $ rad patch review --patch --accept --hunk 1 7a2ac7e2841cc1e7394f99f107555a499b1 ╭────────────────────────────────────────────────────╮ │ INSTRUCTIONS.txt -> notes/INSTRUCTIONS.txt ❲moved❳ │ ╰────────────────────────────────────────────────────╯ -✓ Updated review tree to 3effc8f6462fa2573697072245e57708c4dcbe62 +✓ Updated brain to 3effc8f6462fa2573697072245e57708c4dcbe62 ``` ``` diff --git a/radicle-cli/src/commands/patch/review.rs b/radicle-cli/src/commands/patch/review.rs index d9e2e003..418d8ed9 100644 --- a/radicle-cli/src/commands/patch/review.rs +++ b/radicle-cli/src/commands/patch/review.rs @@ -89,7 +89,7 @@ pub fn run( .minimal(true) .context_lines(unified as u32); - builder::ReviewBuilder::new(patch_id, *profile.id(), repository) + builder::ReviewBuilder::new(patch_id, signer, repository) .hunk(hunk) .verdict(verdict) .run(revision, &mut opts)?; diff --git a/radicle-cli/src/commands/patch/review/builder.rs b/radicle-cli/src/commands/patch/review/builder.rs index 6fb6a1c4..c589ddb2 100644 --- a/radicle-cli/src/commands/patch/review/builder.rs +++ b/radicle-cli/src/commands/patch/review/builder.rs @@ -18,11 +18,12 @@ use std::path::{Path, PathBuf}; use std::str::FromStr; use std::{fmt, io}; +use radicle::cob; use radicle::cob::patch::{PatchId, Revision, Verdict}; use radicle::cob::{CodeLocation, CodeRange}; use radicle::git; use radicle::prelude::*; -use radicle::storage::git::Repository; +use radicle::storage::git::{cob::DraftStore, Repository}; use radicle_git_ext::Oid; use radicle_surf::diff::*; use radicle_term::{Element, VStack}; @@ -387,6 +388,16 @@ impl ReviewQueue { } } +impl From for ReviewQueue { + fn from(diff: Diff) -> Self { + let mut queue = Self::default(); + for file in diff.into_files() { + queue.add_file(file); + } + queue + } +} + impl std::ops::Deref for ReviewQueue { type Target = VecDeque<(usize, ReviewItem)>; @@ -410,6 +421,7 @@ impl Iterator for ReviewQueue { } /// Builds a review for a single file. +/// Adjusts line deltas when a hunk is ignored. pub struct FileReviewBuilder { header: FileHeader, delta: i32, @@ -438,12 +450,7 @@ impl FileReviewBuilder { } } - fn apply_item<'a>( - &mut self, - item: ReviewItem, - brain: &mut git::raw::Tree<'a>, - repo: &'a git::raw::Repository, - ) -> Result<(), Error> { + fn item_diff(&mut self, item: ReviewItem) -> Result { let mut buf = Vec::new(); let mut writer = unified_diff::Writer::new(&mut buf); writer.encode(&self.header)?; @@ -462,22 +469,111 @@ impl FileReviewBuilder { } drop(writer); - let diff = git::raw::Diff::from_buffer(&buf)?; - let mut index = repo.apply_to_tree(brain, &diff, None)?; - let brain_oid = index.write_tree_to(repo)?; + git::raw::Diff::from_buffer(&buf).map_err(Error::from) + } +} - *brain = repo.find_tree(brain_oid)?; +/// Represents the reviewer's brain, ie. what they have seen or not seen in terms +/// of changes introduced by a patch. +pub struct Brain<'a> { + /// Where the review draft is being stored. + refname: git::Namespaced<'a>, + /// The commit pointed to by the ref. + head: git::raw::Commit<'a>, + /// The tree of accepted changes pointed to by the head commit. + accepted: git::raw::Tree<'a>, +} + +impl<'a> Brain<'a> { + /// Create a new brain in the repository. + fn new( + patch: PatchId, + remote: &NodeId, + base: git::raw::Commit, + repo: &'a git::raw::Repository, + ) -> Result { + let refname = Self::refname(&patch, remote); + let author = repo.signature()?; + let oid = repo.commit( + Some(refname.as_str()), + &author, + &author, + &format!("Review for {patch}"), + &base.tree()?, + // TODO: Verify this is necessary, shouldn't matter. + &[&base], + )?; + let head = repo.find_commit(oid)?; + let tree = head.tree()?; + + Ok(Self { + refname, + head, + accepted: tree, + }) + } + + /// Return the content identifier of this brain. This represents the state of the + /// accepted hunks, ie. the git tree. + fn cid(&self) -> Oid { + self.accepted.id().into() + } + + /// Load an existing brain from the repository. + fn load( + patch: PatchId, + remote: &NodeId, + repo: &'a git::raw::Repository, + ) -> Result { + // TODO: Validate this leads to correct UX for potentially abandoned drafts on + // past revisions. + let refname = Self::refname(&patch, remote); + let head = repo.find_reference(&refname)?.peel_to_commit()?; + let tree = head.tree()?; + + Ok(Self { + refname, + head, + accepted: tree, + }) + } + + /// Accept changes to the brain. + fn accept( + &mut self, + diff: git::raw::Diff, + repo: &'a git::raw::Repository, + ) -> Result<(), git::raw::Error> { + let mut index = repo.apply_to_tree(&self.accepted, &diff, None)?; + let accepted = index.write_tree_to(repo)?; + self.accepted = repo.find_tree(accepted)?; + + // Update review with new brain. + let head = self.head.amend( + Some(&self.refname), + None, + None, + None, + None, + Some(&self.accepted), + )?; + self.head = repo.find_commit(head)?; Ok(()) } + + /// Get the brain's refname given the patch and remote. + fn refname(patch: &PatchId, remote: &NodeId) -> git::Namespaced<'a> { + git::refs::storage::draft::review(remote, patch) + } } /// Builds a patch review interactively, across multiple files. -pub struct ReviewBuilder<'a> { +pub struct ReviewBuilder<'a, G> { /// Patch being reviewed. patch_id: PatchId, - /// Where the review draft is being stored. - refname: git::Namespaced<'a>, + /// Signer. + signer: G, /// Stored copy of repository. repo: &'a Repository, /// Single hunk review. @@ -486,14 +582,12 @@ pub struct ReviewBuilder<'a> { verdict: Option, } -impl<'a> ReviewBuilder<'a> { +impl<'a, G: Signer> ReviewBuilder<'a, G> { /// Create a new review builder. - pub fn new(patch_id: PatchId, nid: NodeId, repo: &'a Repository) -> Self { + pub fn new(patch_id: PatchId, signer: G, repo: &'a Repository) -> Self { Self { patch_id, - // TODO: Validate this leads to correct UX for potentially abandoned drafts on - // past revisions. - refname: git::refs::storage::draft::review(&nid, &patch_id), + signer, repo, hunk: None, verdict: None, @@ -515,8 +609,8 @@ impl<'a> ReviewBuilder<'a> { /// Run the review builder for the given revision. pub fn run(self, revision: &Revision, opts: &mut git::raw::DiffOptions) -> anyhow::Result<()> { let repo = self.repo.raw(); + let signer = &self.signer; let base = repo.find_commit((*revision.base()).into())?; - let author = repo.signature()?; let patch_id = self.patch_id; let tree = { let commit = repo.find_commit(revision.head().into())?; @@ -530,38 +624,46 @@ impl<'a> ReviewBuilder<'a> { } else { Box::new(io::stderr().lock()) }; - let mut review = if let Ok(c) = self.current() { + let mut brain = if let Ok(b) = Brain::load(self.patch_id, signer.public_key(), repo) { term::success!( "Loaded existing review {} for patch {}", - term::format::secondary(term::format::parens(term::format::oid(c.id()))), + term::format::secondary(term::format::parens(term::format::oid(b.head.id()))), term::format::tertiary(&patch_id) ); - c + b } else { - let oid = repo.commit( - Some(self.refname.as_str()), - &author, - &author, - &format!("Review {patch_id}"), - &base.tree()?, - // TODO: Verify this is necessary, shouldn't matter. - &[&base], - )?; - repo.find_commit(oid)? + Brain::new(self.patch_id, signer.public_key(), base, repo)? }; - let mut brain = review.tree()?; - let mut queue = ReviewQueue::default(); - let diff = self.diff(&brain, &tree, repo, opts)?; + let diff = self.diff(&brain.accepted, &tree, repo, opts)?; + let drafts = DraftStore::new(*signer.public_key(), self.repo).with( + signer.public_key(), + &cob::patch::TYPENAME, + &patch_id, + )?; + let mut patches = cob::patch::Cache::no_cache(&drafts)?; + let mut patch = patches.get_mut(&patch_id)?; + let mut queue = ReviewQueue::from(diff); - // Build the review queue. - for file in diff.into_files() { - queue.add_file(file); - } if queue.is_empty() { term::success!("All hunks have been reviewed"); return Ok(()); } + let review = if let Some(r) = revision.review_by(signer.public_key()) { + r.id() + } else { + patch.review( + revision.id(), + // This is amended before the review is finalized, if all hunks are + // accepted. We can't set this to `None`, as that will be invalid without + // a review summary. + Some(Verdict::Reject), + None, + vec![], + signer, + )? + }; + // File review for the current file. Starts out as `None` and is set on the first hunk. // Keeps track of deltas for hunk offsets. let mut file: Option = None; @@ -589,15 +691,12 @@ impl<'a> ReviewBuilder<'a> { // When a hunk is accepted, we convert it to unified diff format, // and apply it to the `brain`. Some(ReviewAction::Accept) => { - // Update brain with accepted hunk. - file.apply_item(item, &mut brain, repo)?; - // Update review with new brain. - let review_oid = - review.amend(Some(&self.refname), None, None, None, None, Some(&brain))?; - review = repo.find_commit(review_oid)?; + // Compute hunk diff and update brain by applying it. + let diff = file.item_diff(item)?; + brain.accept(diff, repo)?; if self.hunk.is_some() { - term::success!("Updated review tree to {}", brain.id()); + term::success!("Updated brain to {}", brain.cid()); } } Some(ReviewAction::Ignore) => { @@ -609,12 +708,21 @@ impl<'a> ReviewBuilder<'a> { let path = old.or(new); if let (Some(hunk), Some((path, _))) = (item.hunk(), path) { - let mut builder = CommentBuilder::new(revision.head(), path.to_path_buf()); - builder.edit(hunk)?; + let builder = CommentBuilder::new(revision.head(), path.to_path_buf()); + let comments = builder.edit(hunk)?; - let _comments = builder.comments(); - - queue.push_front((ix, item)); + patch.transaction("Review comments", signer, |tx| { + for comment in comments { + tx.review_comment( + review, + comment.body, + Some(comment.location), + None, // Not a reply. + vec![], // No embeds. + )?; + } + Ok(()) + })?; } else { eprintln!( "{}", @@ -706,13 +814,6 @@ impl<'a> ReviewBuilder<'a> { Some(ReviewAction::Ignore) } } - - fn current(&self) -> Result { - self.repo - .raw() - .find_reference(&self.refname)? - .peel_to_commit() - } } #[derive(Debug, PartialEq, Eq)] @@ -751,7 +852,7 @@ impl CommentBuilder { } } - fn edit(&mut self, hunk: &Hunk) -> Result<&mut Self, Error> { + fn edit(mut self, hunk: &Hunk) -> Result, Error> { let mut input = String::new(); for line in hunk.to_unified_string()?.lines() { writeln!(&mut input, "> {line}")?; @@ -762,7 +863,7 @@ impl CommentBuilder { let header = HunkHeader::try_from(hunk)?; self.add_hunk(header, &output); } - Ok(self) + Ok(self.comments()) } fn add_hunk(&mut self, hunk: HunkHeader, input: &str) -> &mut Self { diff --git a/radicle/src/cob/patch.rs b/radicle/src/cob/patch.rs index dfb14f6e..d30c47a3 100644 --- a/radicle/src/cob/patch.rs +++ b/radicle/src/cob/patch.rs @@ -2187,12 +2187,12 @@ where &mut self, revision: RevisionId, verdict: Option, - comment: Option, + summary: Option, labels: Vec