Radish alpha
h
Radicle Heartwood Protocol & Stack
Radicle
Git (anonymous pull)
Log in to clone via SSH
cli: Implement hunk-based code review
Alexis Sellier committed 2 years ago
commit 63ff4717dc19f874a8beaefbd2c5dbfe8b992008
parent 22360f8c25f4d02ca2a72fdf2425a780a4c2ebbd
17 files changed +941 -51
modified Cargo.lock
@@ -1944,6 +1944,7 @@ dependencies = [
 "radicle-crypto",
 "radicle-git-ext",
 "radicle-node",
+
 "radicle-surf",
 "radicle-term",
 "serde",
 "serde_json",
@@ -2031,9 +2032,9 @@ dependencies = [

[[package]]
name = "radicle-git-ext"
-
version = "0.5.0"
+
version = "0.5.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
-
checksum = "7d6531d5c9b029df83b8887c27d56e05de59f2821f70fc5e8fce0646437ecda5"
+
checksum = "85d13d7488924b07c0d541760520b1e4224c6ffa2d9765e05d1de9591e248970"
dependencies = [
 "git-ref-format",
 "git2",
modified radicle-cli/Cargo.toml
@@ -21,6 +21,7 @@ nonempty = { version = "0.8" }
# N.b. this is required to use macros, even though it's re-exported
# through radicle
radicle-git-ext = { version = "0.5.0", features = ["serde"] }
+
radicle-surf = { version = "0.12.0" }
serde = { version = "1.0" }
serde_json = { version = "1" }
serde_yaml = { version = "0.8" }
added radicle-cli/examples/rad-review-by-hunk.md
@@ -0,0 +1,184 @@
+
Let's start by creating some files we will patch:
+

+
``` ./MENU.txt
+
Classics
+
--------
+
Salmon Tartare
+
Mac & Cheese
+

+
Comfort Food
+
------------
+
Reuben Sandwich
+
Club Sandwich
+
Fried Shrimp Basket
+

+
Sides
+
-----
+
French Fries
+
```
+

+
``` ./INSTRUCTIONS.txt
+
Notes on how to prepare food go here.
+
```
+

+
``` ./.gitignore
+
*.draft
+
```
+

+
Now let's commit and push them:
+

+
```
+
$ git add MENU.txt INSTRUCTIONS.txt .gitignore
+
$ git commit -q -a -m "Add files"
+
$ git push rad master
+
```
+

+
We can now make some changes and create a patch:
+

+
```
+
$ sed -i '$a Garlic Green Beans' MENU.txt
+
$ sed -i '3i\Baked Brie' MENU.txt
+
$ sed -i 's/French Fries/French Fries!/' MENU.txt
+
$ rm .gitignore
+
$ mkdir notes
+
$ mv INSTRUCTIONS.txt notes/
+
```
+

+
``` ./DISCLAIMER.txt
+
All food is served as-is, with no warranty!
+
```
+

+
```
+
$ git checkout -q -b patch/1
+
$ git add .
+
$ git status --short
+
D  .gitignore
+
A  DISCLAIMER.txt
+
M  MENU.txt
+
R  INSTRUCTIONS.txt -> notes/INSTRUCTIONS.txt
+
$ git commit -q -m "Update files"
+
```
+

+
``` (stderr)
+
$ git push rad HEAD:refs/patches
+
✓ Patch a05c57911e6ec9eba4e5539a095a6a86e2bf7c2c opened
+
To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi
+
 * [new reference]   HEAD -> refs/patches
+
```
+

+
Finally, we do a review of the patch by hunk. The output of this command should
+
match `git diff master -W100% -U5 --patience`:
+

+
```
+
$ rad review --no-sync --patch -U5 a05c57911e6ec9eba4e5539a095a6a86e2bf7c2c
+
diff --git a/.gitignore b/.gitignore
+
deleted file mode 100644
+
index 7937fb3..0000000
+
--- a/.gitignore
+
+++ /dev/null
+
@@ -1 +0,0 @@
+
-*.draft
+
diff --git a/DISCLAIMER.txt b/DISCLAIMER.txt
+
new file mode 100644
+
index 0000000..2b5bd86
+
--- /dev/null
+
+++ b/DISCLAIMER.txt
+
@@ -0,0 +1 @@
+
+All food is served as-is, with no warranty!
+
diff --git a/MENU.txt b/MENU.txt
+
index 867958c..3af9741 100644
+
--- a/MENU.txt
+
+++ b/MENU.txt
+
@@ -1,7 +1,8 @@
+
 Classics
+
 --------
+
+Baked Brie
+
 Salmon Tartare
+
 Mac & Cheese
+
[..]
+
 Comfort Food
+
 ------------
+
@@ -9,6 +10,7 @@ Reuben Sandwich
+
 Club Sandwich
+
 Fried Shrimp Basket
+
[..]
+
 Sides
+
 -----
+
-French Fries
+
+French Fries!
+
+Garlic Green Beans
+
diff --git a/INSTRUCTIONS.txt b/notes/INSTRUCTIONS.txt
+
similarity index 100%
+
rename from INSTRUCTIONS.txt
+
rename to notes/INSTRUCTIONS.txt
+
```
+

+
Now let's accept these hunks one by one..
+

+
```
+
$ rad review --no-sync --patch --accept --hunk 1 a05c57911e6ec9eba4e5539a095a6a86e2bf7c2c
+
✓ Loaded existing review ([..]) for patch a05c57911e6ec9eba4e5539a095a6a86e2bf7c2c
+
diff --git a/.gitignore b/.gitignore
+
deleted file mode 100644
+
index 7937fb3..0000000
+
--- a/.gitignore
+
+++ /dev/null
+
@@ -1 +0,0 @@
+
-*.draft
+
```
+
```
+
$ rad review --no-sync --patch --accept --hunk 1 a05c57911e6ec9eba4e5539a095a6a86e2bf7c2c
+
✓ Loaded existing review ([..]) for patch a05c57911e6ec9eba4e5539a095a6a86e2bf7c2c
+
diff --git a/DISCLAIMER.txt b/DISCLAIMER.txt
+
new file mode 100644
+
index 0000000..2b5bd86
+
--- /dev/null
+
+++ b/DISCLAIMER.txt
+
@@ -0,0 +1 @@
+
+All food is served as-is, with no warranty!
+
```
+
```
+
$ rad review --no-sync --patch --accept -U3 --hunk 1 a05c57911e6ec9eba4e5539a095a6a86e2bf7c2c
+
✓ Loaded existing review ([..]) for patch a05c57911e6ec9eba4e5539a095a6a86e2bf7c2c
+
diff --git a/MENU.txt b/MENU.txt
+
index 867958c..3af9741 100644
+
--- a/MENU.txt
+
+++ b/MENU.txt
+
@@ -1,5 +1,6 @@
+
 Classics
+
 --------
+
+Baked Brie
+
 Salmon Tartare
+
 Mac & Cheese
+
[..]
+
```
+
```
+
$ rad review --no-sync --patch --accept -U3 --hunk 1 a05c57911e6ec9eba4e5539a095a6a86e2bf7c2c
+
✓ Loaded existing review ([..]) for patch a05c57911e6ec9eba4e5539a095a6a86e2bf7c2c
+
diff --git a/MENU.txt b/MENU.txt
+
index 4e2e828..3af9741 100644
+
--- a/MENU.txt
+
+++ b/MENU.txt
+
@@ -12,4 +12,5 @@ Fried Shrimp Basket
+
[..]
+
 Sides
+
 -----
+
-French Fries
+
+French Fries!
+
+Garlic Green Beans
+
```
+

+
```
+
$ rad review --no-sync --patch --accept --hunk 1 a05c57911e6ec9eba4e5539a095a6a86e2bf7c2c
+
✓ Loaded existing review ([..]) for patch a05c57911e6ec9eba4e5539a095a6a86e2bf7c2c
+
diff --git a/INSTRUCTIONS.txt b/notes/INSTRUCTIONS.txt
+
similarity index 100%
+
rename from INSTRUCTIONS.txt
+
rename to notes/INSTRUCTIONS.txt
+
```
+

+
```
+
$ rad review --no-sync --patch --accept --hunk 1 a05c57911e6ec9eba4e5539a095a6a86e2bf7c2c
+
✓ Loaded existing review ([..]) for patch a05c57911e6ec9eba4e5539a095a6a86e2bf7c2c
+
✓ All hunks have been reviewed
+
```
modified radicle-cli/src/commands/ls.rs
@@ -81,7 +81,7 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> {
                continue;
            }
        };
-
        let head = term::format::oid(head);
+
        let head = term::format::oid(head).into();
        table.push([
            term::format::bold(proj.name().to_owned()),
            term::format::tertiary(id.urn()),
modified radicle-cli/src/commands/patch/show.rs
@@ -41,7 +41,9 @@ fn patch_commits(patch: &patch::Patch, stored: &Repository) -> anyhow::Result<Ve
        let commit = stored.raw().find_commit(commit)?;

        lines.push(term::Line::spaced([
-
            term::label(term::format::secondary(term::format::oid(commit.id()))),
+
            term::label(term::format::secondary::<String>(
+
                term::format::oid(commit.id()).into(),
+
            )),
            term::label(term::format::default(
                commit.summary().unwrap_or_default().to_owned(),
            )),
modified radicle-cli/src/commands/review.rs
@@ -1,3 +1,8 @@
+
#[path = "review/builder.rs"]
+
mod builder;
+
#[path = "review/diff.rs"]
+
mod diff;
+

use std::ffi::OsString;
use std::str::FromStr;

@@ -5,7 +10,7 @@ use anyhow::{anyhow, Context};

use radicle::cob::patch::{Patches, RevisionIx, Verdict};
use radicle::prelude::*;
-
use radicle::rad;
+
use radicle::{git, rad};

use crate::git::Rev;
use crate::terminal as term;
@@ -19,13 +24,23 @@ pub const HELP: Help = Help {
    usage: r#"
Usage

-
    rad review [<patch-id>] [--accept|--reject] [-m [<string>]] [<option>...]
+
    rad review [<patch-id>] [--accept | --reject] [-m [<string>]] [<option>...]
+
    rad review [<patch-id>] [-d | --delete]

    To specify a patch to review, use the fully qualified patch id
    or an unambiguous prefix of it.

+
    In scripting contexts, patch mode can be used non-interactively,
+
    by passing eg. the `--hunk` and `--accept` options.
+

Options

+
    -p, --patch               Review by patch hunks
+
        --hunk <index>        Only review a specific hunk
+
        --accept              Accept a patch or set of hunks
+
        --reject              Reject a patch or set of hunks
+
    -U, --unified <n>         Generate diffs with <n> lines of context instead of the usual three
+
    -d, --delete              Delete a review draft
    -r, --revision <number>   Revision number to review, defaults to the latest
        --[no-]sync           Sync review to seed (default: sync)
    -m, --message [<string>]  Provide a comment with the review (default: prompt)
@@ -43,6 +58,28 @@ Markdown supported.
-->
"#;

+
#[derive(Debug, PartialEq, Eq)]
+
pub enum Operation {
+
    Delete,
+
    Review {
+
        by_hunk: bool,
+
        unified: usize,
+
        hunk: Option<usize>,
+
        verdict: Option<Verdict>,
+
    },
+
}
+

+
impl Default for Operation {
+
    fn default() -> Self {
+
        Self::Review {
+
            by_hunk: false,
+
            unified: 3,
+
            hunk: None,
+
            verdict: None,
+
        }
+
    }
+
}
+

#[derive(Debug)]
pub struct Options {
    pub id: Rev,
@@ -50,7 +87,7 @@ pub struct Options {
    pub message: Message,
    pub sync: bool,
    pub verbose: bool,
-
    pub verdict: Option<Verdict>,
+
    pub op: Operation,
}

impl Args for Options {
@@ -63,7 +100,7 @@ impl Args for Options {
        let mut message = Message::default();
        let mut sync = true;
        let mut verbose = false;
-
        let mut verdict = None;
+
        let mut op = Operation::default();

        while let Some(arg) = parser.next()? {
            match arg {
@@ -93,14 +130,59 @@ impl Args for Options {
                Long("no-message") => {
                    message = Message::Blank;
                }
+
                Long("patch") | Short('p') => {
+
                    if let Operation::Review { by_hunk, .. } = &mut op {
+
                        *by_hunk = true;
+
                    } else {
+
                        return Err(arg.unexpected().into());
+
                    }
+
                }
+
                Long("unified") | Short('U') => {
+
                    if let Operation::Review { unified, .. } = &mut op {
+
                        let val = parser.value()?;
+
                        *unified = term::args::number(&val)?;
+
                    } else {
+
                        return Err(arg.unexpected().into());
+
                    }
+
                }
+
                Long("hunk") => {
+
                    if let Operation::Review { hunk, .. } = &mut op {
+
                        let val = parser.value()?;
+
                        let val = term::args::number(&val)
+
                            .map_err(|e| anyhow!("invalid hunk value: {e}"))?;
+

+
                        *hunk = Some(val);
+
                    } else {
+
                        return Err(arg.unexpected().into());
+
                    }
+
                }
+
                Long("delete") | Short('d') => {
+
                    op = Operation::Delete;
+
                }
                Long("verbose") | Short('v') => {
                    verbose = true;
                }
-
                Long("accept") if verdict.is_none() => {
-
                    verdict = Some(Verdict::Accept);
+
                Long("accept") => {
+
                    if let Operation::Review {
+
                        verdict: verdict @ None,
+
                        ..
+
                    } = &mut op
+
                    {
+
                        *verdict = Some(Verdict::Accept);
+
                    } else {
+
                        return Err(arg.unexpected().into());
+
                    }
                }
-
                Long("reject") if verdict.is_none() => {
-
                    verdict = Some(Verdict::Reject);
+
                Long("reject") => {
+
                    if let Operation::Review {
+
                        verdict: verdict @ None,
+
                        ..
+
                    } = &mut op
+
                    {
+
                        *verdict = Some(Verdict::Reject);
+
                    } else {
+
                        return Err(arg.unexpected().into());
+
                    }
                }
                Value(val) => {
                    id = Some(Rev::from(string(&val)));
@@ -116,7 +198,7 @@ impl Args for Options {
                sync,
                revision,
                verbose,
-
                verdict,
+
                op,
            },
            vec![],
        ))
@@ -140,37 +222,68 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> {
        .context(format!("couldn't find patch {patch_id} locally"))?;
    let patch_id_pretty = term::format::tertiary(term::format::cob(&patch_id));
    let revision_ix = options.revision.unwrap_or_else(|| patch.version());
-
    let (revision_id, _) = patch
+
    let (revision_id, revision) = patch
        .revisions()
        .nth(revision_ix)
        .ok_or_else(|| anyhow!("revision R{} does not exist", revision_ix))?;
-
    let message = options.message.get(REVIEW_HELP_MSG)?;
-
    let message = message.replace(REVIEW_HELP_MSG.trim(), "");
-
    let message = if message.is_empty() {
-
        None
-
    } else {
-
        Some(message)
-
    };
-

-
    patch.review(*revision_id, options.verdict, message, vec![], &signer)?;
-

-
    match options.verdict {
-
        Some(Verdict::Accept) => {
-
            term::success!(
-
                "Patch {} {}",
-
                patch_id_pretty,
-
                term::format::highlight("accepted")
-
            );
-
        }
-
        Some(Verdict::Reject) => {
-
            term::success!(
-
                "Patch {} {}",
-
                patch_id_pretty,
-
                term::format::negative("rejected")
-
            );
+

+
    match options.op {
+
        Operation::Review {
+
            verdict,
+
            by_hunk,
+
            unified,
+
            hunk,
+
        } => {
+
            if by_hunk {
+
                let mut opts = git::raw::DiffOptions::new();
+
                opts.patience(true)
+
                    .minimal(true)
+
                    .context_lines(unified as u32);
+

+
                builder::ReviewBuilder::new(patch_id, *profile.id(), &repository)
+
                    .hunk(hunk)
+
                    .verdict(verdict)
+
                    .run(revision, &mut opts)?;
+
            } else {
+
                let message = options.message.get(REVIEW_HELP_MSG)?;
+
                let message = message.replace(REVIEW_HELP_MSG.trim(), "");
+
                let message = if message.is_empty() {
+
                    None
+
                } else {
+
                    Some(message)
+
                };
+
                patch.review(*revision_id, verdict, message, vec![], &signer)?;
+

+
                match verdict {
+
                    Some(Verdict::Accept) => {
+
                        term::success!(
+
                            "Patch {} {}",
+
                            patch_id_pretty,
+
                            term::format::highlight("accepted")
+
                        );
+
                    }
+
                    Some(Verdict::Reject) => {
+
                        term::success!(
+
                            "Patch {} {}",
+
                            patch_id_pretty,
+
                            term::format::negative("rejected")
+
                        );
+
                    }
+
                    None => {
+
                        term::success!("Patch {} reviewed", patch_id_pretty);
+
                    }
+
                }
+
            }
        }
-
        None => {
-
            term::success!("Patch {} reviewed", patch_id_pretty);
+
        Operation::Delete => {
+
            let name = git::refs::storage::review(profile.id(), &patch_id);
+

+
            match repository.backend.find_reference(&name) {
+
                Ok(mut r) => r.delete()?,
+
                Err(e) => {
+
                    anyhow::bail!("Couldn't delete review reference '{name}': {e}");
+
                }
+
            }
        }
    }

added radicle-cli/src/commands/review/builder.rs
@@ -0,0 +1,406 @@
+
//! Review builder.
+
//!
+
//! This module enables a user to review a patch by interactively viewing and accepting diff hunks.
+
//! The interaction and output is modeled around `git add -p`.
+
//!
+
//! To implement this behavior, we keep a hidden Git tree object that tracks the state of the
+
//! repository including the accepted hunks. Thus, every time a diff hunk is accepted, it is applied
+
//! to that tree. We call that tree the "brain", as it tracks what the code reviewer has reviewed.
+
//!
+
//! The brain starts out equalling the tree of the base branch, and eventually, when the brain
+
//! matches the tree of the patch being reviewed (by accepting hunks), we can say that the patch has
+
//! been fully reviewed.
+
//!
+
use std::collections::VecDeque;
+
use std::io::IsTerminal as _;
+
use std::str::FromStr;
+
use std::{fmt, io};
+

+
use radicle::cob::patch::{PatchId, Revision, Verdict};
+
use radicle::git;
+
use radicle::prelude::*;
+
use radicle::storage::git::Repository;
+
use radicle_surf::diff::*;
+

+
use crate::terminal as term;
+

+
use super::diff::DiffWriter;
+

+
/// Help message shown to user.
+
const HELP: &str = "\
+
y - accept this hunk
+
n - ignore this hunk
+
c - comment on this hunk
+
j - leave this hunk undecided, see next hunk
+
k - leave this hunk undecided, see previous hunk
+
s - split the current hunk into smaller hunks
+
q - quit; do not accept this hunk nor any of the remaining ones
+
? - print help";
+

+
/// The actions that a user can carry out on a review item.
+
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+
pub enum ReviewAction {
+
    Accept,
+
    Ignore,
+
    Comment,
+
    Split,
+
    Next,
+
    Previous,
+
    Help,
+
    Quit,
+
}
+

+
impl ReviewAction {
+
    /// Ask the user what action to take.
+
    fn prompt(
+
        mut input: impl io::BufRead,
+
        mut output: impl io::Write,
+
        prompt: impl fmt::Display,
+
    ) -> io::Result<Option<Self>> {
+
        write!(&mut output, "{prompt} ")?;
+

+
        let mut s = String::new();
+
        input.read_line(&mut s)?;
+

+
        if s.trim().is_empty() {
+
            return Ok(None);
+
        }
+
        Self::from_str(s.trim())
+
            .map(Some)
+
            .map_err(|e| io::Error::new(io::ErrorKind::InvalidInput, e))
+
    }
+
}
+

+
impl std::fmt::Display for ReviewAction {
+
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+
        match self {
+
            Self::Accept => write!(f, "y"),
+
            Self::Ignore => write!(f, "n"),
+
            Self::Comment => write!(f, "c"),
+
            Self::Split => write!(f, "s"),
+
            Self::Next => write!(f, "j"),
+
            Self::Previous => write!(f, "k"),
+
            Self::Help => write!(f, "?"),
+
            Self::Quit => write!(f, "q"),
+
        }
+
    }
+
}
+

+
impl FromStr for ReviewAction {
+
    type Err = io::Error;
+

+
    fn from_str(s: &str) -> Result<Self, Self::Err> {
+
        match s {
+
            "y" => Ok(Self::Accept),
+
            "n" => Ok(Self::Ignore),
+
            "c" => Ok(Self::Comment),
+
            "s" => Ok(Self::Split),
+
            "j" => Ok(Self::Next),
+
            "k" => Ok(Self::Previous),
+
            "?" => Ok(Self::Help),
+
            "q" => Ok(Self::Quit),
+
            _ => Err(io::Error::new(
+
                io::ErrorKind::InvalidInput,
+
                format!("invalid action '{s}'"),
+
            )),
+
        }
+
    }
+
}
+

+
/// A single review item. Can be a hunk or eg. a file move.
+
pub struct ReviewItem<'a> {
+
    file: &'a FileDiff,
+
    hunk: Option<&'a Hunk<Modification>>,
+
}
+

+
/// Queue of items (usually hunks) left to review.
+
#[derive(Default)]
+
pub struct ReviewQueue<'a> {
+
    queue: VecDeque<(usize, ReviewItem<'a>)>,
+
}
+

+
impl<'a> ReviewQueue<'a> {
+
    /// Push an item to the queue.
+
    fn push(&mut self, file: &'a FileDiff, hunks: Option<&'a Hunks<Modification>>) {
+
        if let Some(hunks) = hunks {
+
            for hunk in hunks.iter() {
+
                let ix = self.queue.len();
+

+
                self.queue.push_back((
+
                    ix,
+
                    ReviewItem {
+
                        file,
+
                        hunk: Some(hunk),
+
                    },
+
                ));
+
            }
+
        } else {
+
            self.queue
+
                .push_back((self.queue.len(), ReviewItem { file, hunk: None }));
+
        }
+
    }
+
}
+

+
impl<'a> std::ops::Deref for ReviewQueue<'a> {
+
    type Target = VecDeque<(usize, ReviewItem<'a>)>;
+

+
    fn deref(&self) -> &Self::Target {
+
        &self.queue
+
    }
+
}
+

+
impl<'a> std::ops::DerefMut for ReviewQueue<'a> {
+
    fn deref_mut(&mut self) -> &mut Self::Target {
+
        &mut self.queue
+
    }
+
}
+

+
impl<'a> Iterator for ReviewQueue<'a> {
+
    type Item = (usize, ReviewItem<'a>);
+

+
    fn next(&mut self) -> Option<Self::Item> {
+
        self.queue.pop_front()
+
    }
+
}
+

+
/// Builds a patch review interactively.
+
pub struct ReviewBuilder<'a> {
+
    /// Patch being reviewed.
+
    patch_id: PatchId,
+
    /// Where the review draft is being stored.
+
    refname: git::Namespaced<'a>,
+
    /// Stored copy of repository.
+
    repo: &'a Repository,
+
    /// Single hunk review.
+
    hunk: Option<usize>,
+
    /// Verdict for review items.
+
    verdict: Option<Verdict>,
+
}
+

+
impl<'a> ReviewBuilder<'a> {
+
    /// Create a new review builder.
+
    pub fn new(patch_id: PatchId, nid: NodeId, repo: &'a Repository) -> Self {
+
        Self {
+
            patch_id,
+
            refname: git::refs::storage::review(&nid, &patch_id),
+
            repo,
+
            hunk: None,
+
            verdict: None,
+
        }
+
    }
+

+
    /// Review a single hunk. Set to `None` to review all hunks.
+
    pub fn hunk(mut self, hunk: Option<usize>) -> Self {
+
        self.hunk = hunk;
+
        self
+
    }
+

+
    /// Give this verdict to all review items. Set to `None` to not give a verdict.
+
    pub fn verdict(mut self, verdict: Option<Verdict>) -> Self {
+
        self.verdict = verdict;
+
        self
+
    }
+

+
    /// 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 base = repo.find_commit((*revision.base()).into())?;
+
        let author = repo.signature()?;
+
        let patch_id = self.patch_id;
+
        let review = if let Ok(c) = self.current() {
+
            term::success!(
+
                "Loaded existing review {} for patch {}",
+
                term::format::secondary(term::format::parens(term::format::oid(c.id()))),
+
                term::format::tertiary(&patch_id)
+
            );
+
            c
+
        } else {
+
            let oid = repo.commit(
+
                Some(self.refname.as_str()),
+
                &author,
+
                &author,
+
                &format!("Review {patch_id}"),
+
                &base.tree()?,
+
                &[&base],
+
            )?;
+
            repo.find_commit(oid)?
+
        };
+

+
        let mut writer = DiffWriter::new(io::stdout()).styled(true);
+
        let mut queue = ReviewQueue::default(); // Queue of hunks to review.
+
        let mut current = None; // File of the current hunk.
+
        let mut stdin = io::stdin().lock();
+
        let mut stderr = io::stderr().lock();
+

+
        let commit = repo.find_commit(revision.head().into())?;
+
        let tree = commit.tree()?;
+
        let brain = review.tree()?;
+

+
        let mut find_opts = git::raw::DiffFindOptions::new();
+
        find_opts.exact_match_only(true);
+
        find_opts.all(true);
+
        find_opts.copies(false); // We don't support finding copies at the moment.
+

+
        let mut diff = repo.diff_tree_to_tree(Some(&brain), Some(&tree), Some(opts))?;
+
        diff.find_similar(Some(&mut find_opts))?;
+

+
        if diff.deltas().next().is_none() {
+
            term::success!("All hunks have been reviewed");
+
            return Ok(());
+
        }
+
        let diff = Diff::try_from(diff)?;
+

+
        for file in diff.files() {
+
            match file {
+
                FileDiff::Modified(f) => match &f.diff {
+
                    DiffContent::Plain { hunks, .. } => {
+
                        queue.push(file, Some(hunks));
+
                    }
+
                    DiffContent::Binary => {
+
                        queue.push(file, None);
+
                    }
+
                    DiffContent::Empty => {}
+
                },
+
                FileDiff::Added(f) => match &f.diff {
+
                    DiffContent::Plain { hunks, .. } => {
+
                        queue.push(file, Some(hunks));
+
                    }
+
                    DiffContent::Binary => {
+
                        queue.push(file, None);
+
                    }
+
                    DiffContent::Empty => {}
+
                },
+
                FileDiff::Deleted(f) => match &f.diff {
+
                    DiffContent::Plain { hunks, .. } => {
+
                        queue.push(file, Some(hunks));
+
                    }
+
                    DiffContent::Binary => {
+
                        queue.push(file, None);
+
                    }
+
                    DiffContent::Empty => {}
+
                },
+
                FileDiff::Moved(_) => {
+
                    queue.push(file, None);
+
                }
+
                FileDiff::Copied(_) => {
+
                    // Copies are not supported and should never be generated due to the diff
+
                    // options we pass.
+
                    panic!("ReviewBuilder::by_hunk: copy diffs are not supported");
+
                }
+
            }
+
        }
+
        let total = queue.len();
+

+
        while let Some((ix, item)) = queue.next() {
+
            if let Some(hunk) = self.hunk {
+
                if hunk != ix + 1 {
+
                    continue;
+
                }
+
            }
+
            let progress = term::format::secondary(format!("({}/{total})", ix + 1));
+
            let ReviewItem { file, hunk } = item;
+

+
            if current.map_or(true, |c| c != file) {
+
                writer.file_header(file)?;
+
                current = Some(file);
+
            }
+
            if let Some(h) = hunk {
+
                writer.hunk(h)?;
+
            }
+

+
            match self.prompt(&mut stdin, &mut stderr, progress) {
+
                Some(ReviewAction::Accept) => {
+
                    let mut writer = DiffWriter::new(Vec::new());
+

+
                    writer.file_header(file)?;
+
                    if let Some(h) = hunk {
+
                        writer.hunk(h)?;
+
                    }
+
                    let diff = writer.into_inner();
+
                    let diff = git::raw::Diff::from_buffer(&diff)?;
+

+
                    let mut index = repo.apply_to_tree(&brain, &diff, None)?;
+
                    let brain = index.write_tree_to(repo)?;
+
                    let brain = repo.find_tree(brain)?;
+

+
                    let _oid =
+
                        review.amend(Some(&self.refname), None, None, None, None, Some(&brain))?;
+
                }
+
                Some(ReviewAction::Ignore) => {
+
                    // Do nothing. Hunk will be reviewable again next time.
+
                }
+
                Some(ReviewAction::Comment) => {
+
                    eprintln!(
+
                        "{}",
+
                        term::format::tertiary("Commenting is not yet implemented").bold()
+
                    );
+
                    queue.push_front((ix, item));
+
                }
+
                Some(ReviewAction::Split) => {
+
                    eprintln!(
+
                        "{}",
+
                        term::format::tertiary("Splitting is not yet implemented").bold()
+
                    );
+
                    queue.push_front((ix, item));
+
                }
+
                Some(ReviewAction::Next) => {
+
                    queue.push_back((ix, item));
+
                }
+
                Some(ReviewAction::Previous) => {
+
                    queue.push_front((ix, item));
+

+
                    if let Some(e) = queue.pop_back() {
+
                        queue.push_front(e);
+
                    }
+
                }
+
                Some(ReviewAction::Quit) => {
+
                    break;
+
                }
+
                Some(ReviewAction::Help) => {
+
                    eprintln!("{}", term::format::tertiary(HELP).bold());
+
                    queue.push_front((ix, item));
+
                }
+
                None => {
+
                    eprintln!(
+
                        "{}",
+
                        term::format::secondary(format!(
+
                            "{} hunk(s) remaining to review",
+
                            queue.len() + 1
+
                        ))
+
                    );
+
                    queue.push_front((ix, item));
+
                }
+
            }
+
        }
+

+
        Ok(())
+
    }
+

+
    fn prompt(
+
        &self,
+
        mut input: impl io::BufRead,
+
        mut output: &mut io::StderrLock,
+
        progress: impl fmt::Display,
+
    ) -> Option<ReviewAction> {
+
        if let Some(v) = self.verdict {
+
            match v {
+
                Verdict::Accept => Some(ReviewAction::Accept),
+
                Verdict::Reject => Some(ReviewAction::Ignore),
+
            }
+
        } else if output.is_terminal() {
+
            let prompt = term::format::secondary("Accept this hunk? [y,n,c,j,k,?]").bold();
+

+
            ReviewAction::prompt(&mut input, &mut output, format!("{progress} {prompt}"))
+
                .unwrap_or(Some(ReviewAction::Help))
+
        } else {
+
            Some(ReviewAction::Ignore)
+
        }
+
    }
+

+
    fn current(&self) -> Result<git::raw::Commit, git::raw::Error> {
+
        self.repo
+
            .raw()
+
            .find_reference(&self.refname)?
+
            .peel_to_commit()
+
    }
+
}
added radicle-cli/src/commands/review/diff.rs
@@ -0,0 +1,143 @@
+
use std::path::Path;
+
use std::{fmt, io};
+

+
use radicle::git;
+
use radicle_surf::diff::{FileDiff, Hunk, Modification};
+
use radicle_term::ansi::Style;
+

+
use crate::terminal as term;
+

+
/// Writes git-compatible diffs to the given stream.
+
#[derive(Default)]
+
pub struct DiffWriter<W: io::Write> {
+
    styled: bool,
+
    stream: W,
+
}
+

+
impl<W: io::Write> DiffWriter<W> {
+
    /// Create a new writer from a writable stream.
+
    pub fn new(stream: W) -> Self {
+
        Self {
+
            stream,
+
            styled: false,
+
        }
+
    }
+

+
    /// Consume writer and return the underlying stream.
+
    pub fn into_inner(self) -> W {
+
        self.stream
+
    }
+

+
    /// Use styled output or not.
+
    pub fn styled(mut self, choice: bool) -> Self {
+
        self.styled = choice;
+
        self
+
    }
+

+
    /// Write a diff file header.
+
    pub fn file_header(&mut self, file: &FileDiff) -> io::Result<()> {
+
        fn diff(old: &Path, new: &Path) -> String {
+
            format!("diff --git a/{} b/{}", old.display(), new.display())
+
        }
+

+
        match file {
+
            FileDiff::Modified(f) => {
+
                self.meta(diff(&f.path, &f.path))?;
+
                self.meta(format!(
+
                    "index {}..{} {:o}",
+
                    term::format::oid(f.old.oid),
+
                    term::format::oid(f.new.oid),
+
                    u32::from(f.new.mode.clone())
+
                ))?;
+
                self.meta(format!("--- a/{}", f.path.display()))?;
+
                self.meta(format!("+++ b/{}", f.path.display()))?;
+
            }
+
            FileDiff::Added(f) => {
+
                self.meta(diff(&f.path, &f.path))?;
+
                self.meta(format!("new file mode {:o}", u32::from(f.new.mode.clone())))?;
+
                self.meta(format!(
+
                    "index {}..{}",
+
                    term::format::oid(git::raw::Oid::zero()),
+
                    term::format::oid(*f.new.oid),
+
                ))?;
+
                self.meta("--- /dev/null")?;
+
                self.meta(format!("+++ b/{}", f.path.display()))?;
+
            }
+
            FileDiff::Copied(_) => todo!(),
+
            FileDiff::Deleted(f) => {
+
                self.meta(diff(&f.path, &f.path))?;
+
                self.meta(format!(
+
                    "deleted file mode {:o}",
+
                    u32::from(f.old.mode.clone())
+
                ))?;
+
                self.meta(format!(
+
                    "index {}..{}",
+
                    term::format::oid(*f.old.oid),
+
                    term::format::oid(git::raw::Oid::zero()),
+
                ))?;
+
                self.meta(format!("--- a/{}", f.path.display()))?;
+
                self.meta("+++ /dev/null")?;
+
            }
+
            FileDiff::Moved(f) => {
+
                self.meta(diff(&f.old_path, &f.new_path))?;
+
                // Nb. We only display diffs as moves when the file was not changed.
+
                self.meta("similarity index 100%")?;
+
                self.meta(format!("rename from {}", f.old_path.display()))?;
+
                self.meta(format!("rename to {}", f.new_path.display()))?;
+
            }
+
        };
+

+
        Ok(())
+
    }
+

+
    /// Write a diff hunk.
+
    pub fn hunk(&mut self, hunk: &Hunk<Modification>) -> io::Result<()> {
+
        self.magenta(hunk.header.from_utf8_lossy().trim_end())?;
+

+
        for modification in &hunk.lines {
+
            match modification {
+
                Modification::Deletion(radicle_surf::diff::Deletion { line, .. }) => {
+
                    self.deleted(format!(
+
                        "-{}",
+
                        String::from_utf8_lossy(line.as_bytes()).trim_end()
+
                    ))?;
+
                }
+
                Modification::Addition(radicle_surf::diff::Addition { line, .. }) => {
+
                    self.added(format!("+{}", line.from_utf8_lossy()).trim_end())?;
+
                }
+
                Modification::Context { line, .. } => {
+
                    self.context(format!(" {}", line.from_utf8_lossy().trim_end()))?;
+
                }
+
            }
+
        }
+
        Ok(())
+
    }
+

+
    fn write(&mut self, s: impl fmt::Display, style: Style) -> io::Result<()> {
+
        if self.styled {
+
            writeln!(self.stream, "{}", term::Paint::new(s).with_style(style))
+
        } else {
+
            writeln!(self.stream, "{s}")
+
        }
+
    }
+

+
    fn meta(&mut self, s: impl fmt::Display) -> io::Result<()> {
+
        self.write(s, term::Style::new(term::Color::Yellow))
+
    }
+

+
    fn magenta(&mut self, s: impl fmt::Display) -> io::Result<()> {
+
        self.write(s, term::Style::new(term::Color::Magenta))
+
    }
+

+
    fn deleted(&mut self, s: impl fmt::Display) -> io::Result<()> {
+
        self.write(s, term::Style::new(term::Color::Red))
+
    }
+

+
    fn added(&mut self, s: impl fmt::Display) -> io::Result<()> {
+
        self.write(s, term::Style::new(term::Color::Green))
+
    }
+

+
    fn context(&mut self, s: impl fmt::Display) -> io::Result<()> {
+
        self.write(s, term::Style::default().dim())
+
    }
+
}
modified radicle-cli/src/terminal/args.rs
@@ -127,6 +127,11 @@ pub fn addr(val: &OsString) -> anyhow::Result<Address> {
    Address::from_str(&val).map_err(|_| anyhow!("invalid address '{}'", val))
}

+
pub fn number(val: &OsString) -> anyhow::Result<usize> {
+
    let val = val.to_string_lossy();
+
    usize::from_str(&val).map_err(|_| anyhow!("invalid number '{}'", val))
+
}
+

pub fn string(val: &OsString) -> String {
    val.to_string_lossy().to_string()
}
modified radicle-cli/src/terminal/format.rs
@@ -20,8 +20,8 @@ pub fn node(node: &NodeId) -> String {
}

/// Format a git Oid.
-
pub fn oid(oid: impl Into<radicle::git::Oid>) -> String {
-
    format!("{:.7}", oid.into())
+
pub fn oid(oid: impl Into<radicle::git::Oid>) -> Paint<String> {
+
    Paint::new(format!("{:.7}", oid.into()))
}

/// Wrap parenthesis around styled input, eg. `"input"` -> `"(input)"`.
modified radicle-cli/src/terminal/patch.rs
@@ -273,7 +273,7 @@ pub fn list_commits(commits: &[git::raw::Commit]) -> anyhow::Result<()> {
            .summary_bytes()
            .unwrap_or_else(|| commit.message_bytes());
        table.push([
-
            term::format::secondary(term::format::oid(commit.id())),
+
            term::format::secondary(term::format::oid(commit.id()).into()),
            term::format::italic(String::from_utf8_lossy(message).to_string()),
        ]);
    }
modified radicle-cli/tests/commands.rs
@@ -1,7 +1,6 @@
use std::path::Path;
use std::str::FromStr;
-
use std::{env, fs};
-
use std::{thread, time};
+
use std::{env, fs, thread, time};

use radicle::git;
use radicle::node::Handle as _;
@@ -361,6 +360,28 @@ fn rad_patch_via_push() {
}

#[test]
+
fn rad_review_by_hunk() {
+
    logger::init(log::Level::Debug);
+

+
    let mut environment = Environment::new();
+
    let profile = environment.profile("alice");
+
    let working = tempfile::tempdir().unwrap();
+
    let home = &profile.home;
+

+
    // Setup a test repository.
+
    fixtures::repository(working.path());
+

+
    test("examples/rad-init.md", working.path(), Some(home), []).unwrap();
+
    test(
+
        "examples/rad-review-by-hunk.md",
+
        working.path(),
+
        Some(home),
+
        [],
+
    )
+
    .unwrap();
+
}
+

+
#[test]
fn rad_rm() {
    let mut environment = Environment::new();
    let profile = environment.profile("alice");
modified radicle-term/src/ansi/paint.rs
@@ -55,6 +55,12 @@ impl From<Paint<&str>> for Paint<String> {
    }
}

+
impl From<Paint<String>> for String {
+
    fn from(value: Paint<String>) -> Self {
+
        value.item
+
    }
+
}
+

impl<T> Paint<T> {
    /// Constructs a new `Paint` structure encapsulating `item` with no set
    /// styling.
modified radicle-term/src/editor.rs
@@ -45,16 +45,16 @@ impl Editor {
    ///
    /// If the text hasn't changed from the initial contents of the editor,
    /// return `None`.
-
    pub fn edit(&mut self, initial: impl ToString) -> io::Result<Option<String>> {
-
        let initial = initial.to_string();
+
    pub fn edit(&mut self, initial: impl AsRef<[u8]>) -> io::Result<Option<String>> {
+
        let initial = initial.as_ref();
        let mut file = fs::OpenOptions::new()
            .write(true)
            .create(true)
            .open(&self.path)?;

        if file.metadata()?.len() == 0 {
-
            file.write_all(initial.as_bytes())?;
-
            if !initial.ends_with('\n') {
+
            file.write_all(initial)?;
+
            if !initial.ends_with(&[b'\n']) {
                file.write_all(b"\n")?;
            }
            file.flush()?;
modified radicle-term/src/lib.rs
@@ -14,7 +14,7 @@ pub mod textarea;
pub mod vstack;

pub use ansi::Color;
-
pub use ansi::{paint, Paint};
+
pub use ansi::{paint, Paint, Style};
pub use editor::Editor;
pub use element::{Element, Line, Max, Size};
pub use hstack::HStack;
modified radicle-tui/src/ui/cob.rs
@@ -142,7 +142,7 @@ impl TableItem<8> for PatchItem {
        let author = Cell::from(format_author(&self.author.did, self.author.is_you))
            .style(Style::default().fg(theme.colors.browser_list_author));

-
        let head = Cell::from(format::oid(self.head))
+
        let head = Cell::from(format::oid(self.head).item)
            .style(Style::default().fg(theme.colors.browser_patch_list_head));

        let added = Cell::from(format!("{}", self.added))
modified radicle/src/git.rs
@@ -241,6 +241,14 @@ pub mod refs {
            .with_namespace(remote.into())
        }

+
        /// Review draft reference.
+
        ///
+
        /// When building a patch review, we store the intermediate state in this ref.
+
        pub fn review<'a>(remote: &RemoteId, patch: &cob::ObjectId) -> Namespaced<'a> {
+
            Qualified::from_components(component!("reviews"), Component::from(patch), None)
+
                .with_namespace(remote.into())
+
        }
+

        /// Staging/temporary references.
        pub mod staging {
            use super::*;