Radish alpha
r
Radicle terminal user interface
Radicle
Git (anonymous pull)
Log in to clone via SSH
bin: Implement commenting in `patch review`
Erik Kundt committed 1 year ago
commit f562059b04b7e73ce8ca4d1c2265cf94b9c6bf72
parent 41c2e506bc5161987b0928a6c9e6ac256b6734bf
3 files changed +113 -30
modified bin/commands/patch.rs
@@ -9,20 +9,23 @@ use std::ffi::OsString;

use anyhow::anyhow;

+
use radicle::cob::{self, CodeLocation};
use radicle::crypto::Signer;
use radicle::identity::RepoId;
-
use radicle::patch::Status;
+
use radicle::patch::{Status, Verdict};
+
use radicle::storage::git::cob::DraftStore;
use radicle::storage::WriteRepository;

use radicle_cli::git::Rev;
use radicle_cli::terminal;
use radicle_cli::terminal::args::{string, Args, Error, Help};
+
use radicle_cli::terminal::*;

use crate::cob::patch;
use crate::cob::patch::Filter;
use crate::commands::tui_patch::review::ReviewAction;

-
use crate::tui_patch::review::builder::{Brain, ReviewBuilder};
+
use crate::tui_patch::review::builder::{Brain, CommentBuilder, ReviewBuilder, ReviewComment};

pub const HELP: Help = Help {
    name: "patch",
@@ -233,7 +236,7 @@ pub async fn run(options: Options, ctx: impl terminal::Context) -> anyhow::Resul
                .map(|rev| rev.resolve::<radicle::git::Oid>(&repo.backend))
                .transpose()?
                .map(radicle::cob::patch::RevisionId::from);
-
            let (_revision_id, revision) = match revision_id {
+
            let (_, revision) = match revision_id {
                Some(id) => (
                    id,
                    patch
@@ -245,7 +248,7 @@ pub async fn run(options: Options, ctx: impl terminal::Context) -> anyhow::Resul

            let brain = if let Ok(b) = Brain::load(patch_id, signer.public_key(), repo.raw()) {
                log::info!(
-
                    "Loaded existing review {} for patch {}",
+
                    "Loaded existing brain {} for patch {}",
                    b.head().id(),
                    &patch_id
                );
@@ -255,9 +258,61 @@ pub async fn run(options: Options, ctx: impl terminal::Context) -> anyhow::Resul
                Brain::new(patch_id, signer.public_key(), base, repo.raw())?
            };

-
            let queue = ReviewBuilder::new(patch_id, signer, &repo).queue(&brain, &revision)?;
+
            let builder = ReviewBuilder::new(patch_id, &signer, &repo);
+
            let queue = builder.queue(&brain, &revision)?;
+

+
            let drafts = DraftStore::new(&repo, *signer.public_key());
+
            let mut patches = cob::patch::Cache::no_cache(&drafts)?;
+
            let mut patch = patches.get_mut(&patch_id)?;
+

+
            if let Some(review) = revision.review_by(signer.public_key()) {
+
                // Review already finalized. Do nothing and warn.
+
                terminal::warning(format!(
+
                    "Review ({}) already finalized. Exiting.",
+
                    review.id()
+
                ));
+

+
                return Ok(());
+
            };
+

+
            let (review_id, review) = if let Some((id, review)) = patch
+
                .clone()
+
                .reviews_of(revision.id())
+
                .find(|(_, review)| review.author().public_key() == signer.public_key())
+
            {
+
                // Review already started, resume.
+
                log::info!("Resuming review {id}..");
+
                (id.clone(), review.clone())
+
            } else {
+
                // No review to resume, start a new one.
+
                let id = 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,
+
                )?;
+
                log::info!("Starting new review {id}..");
+

+
                // Load newly created review.
+
                match patch
+
                    .reviews_of(revision.id())
+
                    .find(|(_, review)| review.author().public_key() == signer.public_key())
+
                {
+
                    Some((id, review)) => (id.clone(), review.clone()),
+
                    None => anyhow::bail!("Could not find review {}", id),
+
                }
+
            };
+

+
            loop {
+
                log::info!(
+
                    "Found comments for {review_id}: {:?}",
+
                    review.comments().collect::<Vec<_>>()
+
                );

-
            while !queue.is_empty() {
                let selection = review::Tui::new(profile.clone(), rid, queue.clone())
                    .run()
                    .await?;
@@ -272,9 +327,41 @@ pub async fn run(options: Options, ctx: impl terminal::Context) -> anyhow::Resul
                            // next hunk
                        }
                        ReviewAction::Comment => {
-
                            radicle_cli::terminal::Editor::new()
-
                                .extension("diff")
-
                                .edit(String::new())?;
+
                            if let Some(hunk) = selection.hunk {
+
                                if let Some((_, item)) = queue.get(hunk) {
+
                                    let (old, new) = item.paths();
+
                                    let path = old.or(new);
+

+
                                    if let (Some(hunk), Some((path, _))) = (item.hunk(), path) {
+
                                        let builder = CommentBuilder::new(
+
                                            revision.head(),
+
                                            path.to_path_buf(),
+
                                        );
+
                                        let comments = builder.edit(hunk)?;
+

+
                                        patch.transaction("Review comments", &signer, |tx| {
+
                                            for comment in comments {
+
                                                tx.review_comment(
+
                                                    review_id,
+
                                                    comment.body,
+
                                                    Some(comment.location),
+
                                                    None,   // Not a reply.
+
                                                    vec![], // No embeds.
+
                                                )?;
+
                                            }
+
                                            Ok(())
+
                                        })?;
+
                                    } else {
+
                                        log::warn!(
+
                                            "Commenting on binary blobs is not yet implemented"
+
                                        );
+
                                    }
+
                                } else {
+
                                    log::error!("Expected a hunk to comment on.")
+
                                }
+
                            } else {
+
                                log::error!("Expected a selected hunk.")
+
                            }
                        }
                    }
                } else {
modified bin/commands/patch/review.rs
@@ -77,7 +77,7 @@ pub struct Args(String);
#[derive(Clone, Debug, Eq, PartialEq)]
pub struct Selection {
    pub action: ReviewAction,
-
    pub hunk: usize,
+
    pub hunk: Option<usize>,
    pub args: Option<Args>,
}

@@ -492,14 +492,14 @@ impl<'a> store::Update<Message> for App<'a> {
            Message::Accept => Some(Exit {
                value: Some(Selection {
                    action: ReviewAction::Accept,
-
                    hunk: 0,
+
                    hunk: self.queue.1.selected(),
                    args: None,
                }),
            }),
            Message::Comment => Some(Exit {
                value: Some(Selection {
                    action: ReviewAction::Comment,
-
                    hunk: 0,
+
                    hunk: self.queue.1.selected(),
                    args: None,
                }),
            }),
modified bin/commands/patch/review/builder.rs
@@ -19,10 +19,12 @@ use std::str::FromStr;
use std::{fmt, io};

use radicle::cob;
+
use radicle::cob::cache::NoCache;
use radicle::cob::patch::{PatchId, Revision, Verdict};
use radicle::cob::{CodeLocation, CodeRange};
use radicle::git;
use radicle::git::Oid;
+
use radicle::patch::PatchMut;
use radicle::prelude::*;
use radicle::storage::git::{cob::DraftStore, Repository};
use radicle_surf::diff::*;
@@ -441,7 +443,7 @@ pub struct ReviewBuilder<'a, G> {
    /// Patch being reviewed.
    patch_id: PatchId,
    /// Signer.
-
    signer: G,
+
    signer: &'a G,
    /// Stored copy of repository.
    repo: &'a Repository,
    /// Single hunk review.
@@ -452,7 +454,7 @@ pub struct ReviewBuilder<'a, G> {

impl<'a, G: Signer> ReviewBuilder<'a, G> {
    /// Create a new review builder.
-
    pub fn new(patch_id: PatchId, signer: G, repo: &'a Repository) -> Self {
+
    pub fn new(patch_id: PatchId, signer: &'a G, repo: &'a Repository) -> Self {
        Self {
            patch_id,
            signer,
@@ -477,9 +479,6 @@ impl<'a, G: Signer> ReviewBuilder<'a, G> {
    /// Assemble the review for the given revision.
    pub fn queue(&self, brain: &'a Brain<'a>, revision: &Revision) -> anyhow::Result<ReviewQueue> {
        let repo = self.repo.raw();
-
        let signer = &self.signer;
-
        let patch_id = self.patch_id;
-
        let base = repo.find_commit((*revision.base()).into())?;
        let tree = {
            let commit = repo.find_commit(revision.head().into())?;
            commit.tree()?
@@ -489,9 +488,6 @@ impl<'a, G: Signer> ReviewBuilder<'a, G> {
        opts.patience(true).minimal(true).context_lines(3_u32);

        let diff = self.diff(&brain.accepted(), &tree, repo, &mut opts)?;
-
        let drafts = DraftStore::new(self.repo, *signer.public_key());
-
        let mut patches = cob::patch::Cache::no_cache(&drafts)?;
-
        let mut patch = patches.get_mut(&patch_id)?;

        Ok(ReviewQueue::from(diff))
    }
@@ -546,7 +542,7 @@ impl<'a, G: Signer> ReviewBuilder<'a, G> {
                Some(Verdict::Reject),
                None,
                vec![],
-
                signer,
+
                *signer,
            )?
        };

@@ -597,7 +593,7 @@ impl<'a, G: Signer> ReviewBuilder<'a, G> {
                        let builder = CommentBuilder::new(revision.head(), path.to_path_buf());
                        let comments = builder.edit(hunk)?;

-
                        patch.transaction("Review comments", signer, |tx| {
+
                        patch.transaction("Review comments", *signer, |tx| {
                            for comment in comments {
                                tx.review_comment(
                                    review,
@@ -703,13 +699,13 @@ impl<'a, G: Signer> ReviewBuilder<'a, G> {
}

#[derive(Debug, PartialEq, Eq)]
-
struct ReviewComment {
-
    location: CodeLocation,
-
    body: String,
+
pub struct ReviewComment {
+
    pub location: CodeLocation,
+
    pub body: String,
}

#[derive(thiserror::Error, Debug)]
-
enum Error {
+
pub enum Error {
    #[error(transparent)]
    Diff(#[from] unified_diff::Error),
    #[error(transparent)]
@@ -723,14 +719,14 @@ enum Error {
}

#[derive(Debug)]
-
struct CommentBuilder {
+
pub struct CommentBuilder {
    commit: Oid,
    path: PathBuf,
    comments: Vec<ReviewComment>,
}

impl CommentBuilder {
-
    fn new(commit: Oid, path: PathBuf) -> Self {
+
    pub fn new(commit: Oid, path: PathBuf) -> Self {
        Self {
            commit,
            path,
@@ -738,7 +734,7 @@ impl CommentBuilder {
        }
    }

-
    fn edit(mut self, hunk: &Hunk<Modification>) -> Result<Vec<ReviewComment>, Error> {
+
    pub fn edit(mut self, hunk: &Hunk<Modification>) -> Result<Vec<ReviewComment>, Error> {
        let mut input = String::new();
        for line in hunk.to_unified_string()?.lines() {
            writeln!(&mut input, "> {line}")?;
@@ -752,7 +748,7 @@ impl CommentBuilder {
        Ok(self.comments())
    }

-
    fn add_hunk(&mut self, hunk: HunkHeader, input: &str) -> &mut Self {
+
    pub fn add_hunk(&mut self, hunk: HunkHeader, input: &str) -> &mut Self {
        let lines = input.trim().lines().map(|l| l.trim());
        let (mut old_line, mut new_line) = (hunk.old_line_no as usize, hunk.new_line_no as usize);
        let (mut old_start, mut new_start) = (old_line, new_line);