Radish alpha
h
Radicle Heartwood Protocol & Stack
Radicle
Git (anonymous pull)
Log in to clone via SSH
cli/patch/review: Remove builder
Fintan Halpenny committed 3 months ago
commit f1a9ad9d0764ce67d035896f2db98be48a7dd9ad
parent 02318f199c6f29a2eede1f282e1f9b99927d27ec
4 files changed +18 -1311
modified crates/radicle-cli/src/commands/patch/review.rs
@@ -1,9 +1,6 @@
-
mod builder;
-

use anyhow::{anyhow, Context};

use radicle::cob::patch::{PatchId, RevisionId, Verdict};
-
use radicle::git;
use radicle::prelude::*;
use radicle::storage::git::Repository;

@@ -74,7 +71,7 @@ pub fn run(
        .get_mut(&patch_id)
        .context(format!("couldn't find patch {patch_id} locally"))?;

-
    let (revision_id, revision) = match revision_id {
+
    let (revision_id, _revision) = match revision_id {
        Some(id) => (
            id,
            patch
@@ -88,21 +85,10 @@ pub fn run(
    match options.op {
        Operation::Review(ReviewOptions {
            by_hunk,
-
            unified,
-
            hunk,
-
            verdict,
-
        }) if by_hunk => {
-
            crate::warning::obsolete("rad patch review --patch");
-
            let mut opts = git::raw::DiffOptions::new();
-
            opts.patience(true)
-
                .minimal(true)
-
                .context_lines(unified as u32);
-

-
            builder::ReviewBuilder::new(patch_id, repository)
-
                .hunk(hunk)
-
                .verdict(verdict)
-
                .run(revision, &mut opts, &signer)?;
-
        }
+
            unified: _,
+
            hunk: _,
+
            verdict: _,
+
        }) if by_hunk => Err(crate::removed("rad patch review --patch")),
        Operation::Review(ReviewOptions { verdict, .. }) => {
            let message = options.message.get(REVIEW_HELP_MSG)?;
            let message = message.replace(REVIEW_HELP_MSG.trim(), "");
@@ -132,19 +118,9 @@ pub fn run(
                    term::success!("Patch {} reviewed", patch_id_pretty);
                }
            }
-
        }
-
        Operation::Delete => {
-
            crate::warning::obsolete("rad patch review --delete");
-
            let name = git::refs::storage::draft::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}");
-
                }
-
            }
+
            Ok(())
        }
+
        Operation::Delete => Err(crate::removed("rad patch review --delete")),
    }
-

-
    Ok(())
}
deleted crates/radicle-cli/src/commands/patch/review/builder.rs
@@ -1,1280 +0,0 @@
-
//! 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::fmt::Write as _;
-
use std::ops::{Deref, Not, Range};
-
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::crypto;
-
use radicle::git;
-
use radicle::git::Oid;
-
use radicle::node::device::Device;
-
use radicle::prelude::*;
-
use radicle::storage::git::{cob::DraftStore, Repository};
-
use radicle_surf::diff::*;
-
use radicle_term::{Element, VStack};
-

-
use crate::git::pretty_diff::ToPretty;
-
use crate::git::pretty_diff::{Blob, Blobs, Repo};
-
use crate::git::unified_diff::{self, FileHeader};
-
use crate::git::unified_diff::{Encode, HunkHeader};
-
use crate::terminal as term;
-
use crate::terminal::highlight::Highlighter;
-

-
/// 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";
-

-
/// A terminal or file where the review UI output can be written to.
-
trait PromptWriter: io::Write {
-
    /// Is the writer a terminal?
-
    fn is_terminal(&self) -> bool;
-
}
-

-
impl PromptWriter for Box<dyn PromptWriter> {
-
    fn is_terminal(&self) -> bool {
-
        self.deref().is_terminal()
-
    }
-
}
-

-
impl<T: io::Write + io::IsTerminal> PromptWriter for T {
-
    fn is_terminal(&self) -> bool {
-
        <Self as io::IsTerminal>::is_terminal(self)
-
    }
-
}
-

-
/// 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.
-
/// Files are usually split into multiple review items.
-
#[derive(Debug)]
-
pub enum ReviewItem {
-
    FileAdded {
-
        path: PathBuf,
-
        header: FileHeader,
-
        new: DiffFile,
-
        hunk: Option<Hunk<Modification>>,
-
    },
-
    FileDeleted {
-
        path: PathBuf,
-
        header: FileHeader,
-
        old: DiffFile,
-
        hunk: Option<Hunk<Modification>>,
-
    },
-
    FileModified {
-
        path: PathBuf,
-
        header: FileHeader,
-
        old: DiffFile,
-
        new: DiffFile,
-
        hunk: Option<Hunk<Modification>>,
-
    },
-
    FileMoved {
-
        moved: Moved,
-
    },
-
    FileCopied {
-
        copied: Copied,
-
    },
-
    FileEofChanged {
-
        path: PathBuf,
-
        header: FileHeader,
-
        old: DiffFile,
-
        new: DiffFile,
-
        eof: EofNewLine,
-
    },
-
    FileModeChanged {
-
        path: PathBuf,
-
        header: FileHeader,
-
        old: DiffFile,
-
        new: DiffFile,
-
    },
-
}
-

-
impl ReviewItem {
-
    fn hunk(&self) -> Option<&Hunk<Modification>> {
-
        match self {
-
            Self::FileAdded { hunk, .. } => hunk.as_ref(),
-
            Self::FileDeleted { hunk, .. } => hunk.as_ref(),
-
            Self::FileModified { hunk, .. } => hunk.as_ref(),
-
            _ => None,
-
        }
-
    }
-

-
    fn hunk_header(&self) -> Option<HunkHeader> {
-
        self.hunk().and_then(|h| HunkHeader::try_from(h).ok())
-
    }
-

-
    fn paths(&self) -> (Option<(&Path, Oid)>, Option<(&Path, Oid)>) {
-
        match self {
-
            Self::FileAdded { path, new, .. } => (None, Some((path, Oid::from(*new.oid)))),
-
            Self::FileDeleted { path, old, .. } => (Some((path, Oid::from(*old.oid))), None),
-
            Self::FileMoved { moved } => (
-
                Some((&moved.old_path, Oid::from(*moved.old.oid))),
-
                Some((&moved.new_path, Oid::from(*moved.new.oid))),
-
            ),
-
            Self::FileCopied { copied } => (
-
                Some((&copied.old_path, Oid::from(*copied.old.oid))),
-
                Some((&copied.new_path, Oid::from(*copied.new.oid))),
-
            ),
-
            Self::FileModified { path, old, new, .. } => (
-
                Some((path, Oid::from(*old.oid))),
-
                Some((path, Oid::from(*new.oid))),
-
            ),
-
            Self::FileEofChanged { path, old, new, .. } => (
-
                Some((path, Oid::from(*old.oid))),
-
                Some((path, Oid::from(*new.oid))),
-
            ),
-
            Self::FileModeChanged { path, old, new, .. } => (
-
                Some((path, Oid::from(*old.oid))),
-
                Some((path, Oid::from(*new.oid))),
-
            ),
-
        }
-
    }
-

-
    fn file_header(&self) -> FileHeader {
-
        match self {
-
            Self::FileAdded { header, .. } => header.clone(),
-
            Self::FileDeleted { header, .. } => header.clone(),
-
            Self::FileMoved { moved } => FileHeader::Moved {
-
                old_path: moved.old_path.clone(),
-
                new_path: moved.new_path.clone(),
-
            },
-
            Self::FileCopied { copied } => FileHeader::Copied {
-
                old_path: copied.old_path.clone(),
-
                new_path: copied.new_path.clone(),
-
            },
-
            Self::FileModified { header, .. } => header.clone(),
-
            Self::FileEofChanged { header, .. } => header.clone(),
-
            Self::FileModeChanged { header, .. } => header.clone(),
-
        }
-
    }
-

-
    fn blobs<R: Repo>(&self, repo: &R) -> Blobs<(PathBuf, Blob)> {
-
        let (old, new) = self.paths();
-
        Blobs::from_paths(old, new, repo)
-
    }
-

-
    fn pretty<R: Repo>(&self, repo: &R) -> Box<dyn Element> {
-
        let mut hi = Highlighter::default();
-
        let blobs = self.blobs(repo);
-
        let highlighted = blobs.highlight(&mut hi);
-
        let header = self.file_header();
-

-
        match self {
-
            Self::FileMoved { moved } => moved.pretty(&mut hi, &header, repo),
-
            Self::FileCopied { copied } => copied.pretty(&mut hi, &header, repo),
-
            Self::FileModified { hunk, .. }
-
            | Self::FileAdded { hunk, .. }
-
            | Self::FileDeleted { hunk, .. } => {
-
                let header = header.pretty(&mut hi, &None, repo);
-
                let vstack = term::VStack::default()
-
                    .border(Some(term::colors::FAINT))
-
                    .padding(1)
-
                    .child(header);
-

-
                if let Some(hunk) = hunk {
-
                    let hunk = hunk.pretty(&mut hi, &highlighted, repo);
-
                    if !hunk.is_empty() {
-
                        return vstack.divider().merge(hunk).boxed();
-
                    }
-
                }
-
                vstack
-
            }
-
            Self::FileEofChanged { eof, .. } => match eof {
-
                EofNewLine::NewMissing => {
-
                    VStack::default().child(term::Label::new("`\\n` missing at end-of-file"))
-
                }
-
                EofNewLine::OldMissing => {
-
                    VStack::default().child(term::Label::new("`\\n` added at end-of-file"))
-
                }
-
                _ => VStack::default(),
-
            },
-
            Self::FileModeChanged { .. } => VStack::default(),
-
        }
-
        .boxed()
-
    }
-
}
-

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

-
impl ReviewQueue {
-
    /// Add a file to the queue.
-
    /// Mostly splits files into individual review items (eg. hunks) to review.
-
    fn add_file(&mut self, file: FileDiff) {
-
        let header = FileHeader::from(&file);
-

-
        match file {
-
            FileDiff::Moved(moved) => {
-
                self.add_item(ReviewItem::FileMoved { moved });
-
            }
-
            FileDiff::Copied(copied) => {
-
                self.add_item(ReviewItem::FileCopied { copied });
-
            }
-
            FileDiff::Added(a) => {
-
                self.add_item(ReviewItem::FileAdded {
-
                    path: a.path,
-
                    header: header.clone(),
-
                    new: a.new,
-
                    hunk: if let DiffContent::Plain {
-
                        hunks: Hunks(mut hs),
-
                        ..
-
                    } = a.diff
-
                    {
-
                        hs.pop()
-
                    } else {
-
                        None
-
                    },
-
                });
-
            }
-
            FileDiff::Deleted(d) => {
-
                self.add_item(ReviewItem::FileDeleted {
-
                    path: d.path,
-
                    header: header.clone(),
-
                    old: d.old,
-
                    hunk: if let DiffContent::Plain {
-
                        hunks: Hunks(mut hs),
-
                        ..
-
                    } = d.diff
-
                    {
-
                        hs.pop()
-
                    } else {
-
                        None
-
                    },
-
                });
-
            }
-
            FileDiff::Modified(m) => {
-
                if m.old.mode != m.new.mode {
-
                    self.add_item(ReviewItem::FileModeChanged {
-
                        path: m.path.clone(),
-
                        header: header.clone(),
-
                        old: m.old.clone(),
-
                        new: m.new.clone(),
-
                    });
-
                }
-
                match m.diff {
-
                    DiffContent::Empty => {
-
                        // Likely a file mode change, which is handled above.
-
                    }
-
                    DiffContent::Binary => {
-
                        self.add_item(ReviewItem::FileModified {
-
                            path: m.path.clone(),
-
                            header: header.clone(),
-
                            old: m.old.clone(),
-
                            new: m.new.clone(),
-
                            hunk: None,
-
                        });
-
                    }
-
                    DiffContent::Plain {
-
                        hunks: Hunks(hunks),
-
                        eof,
-
                        ..
-
                    } => {
-
                        for hunk in hunks {
-
                            self.add_item(ReviewItem::FileModified {
-
                                path: m.path.clone(),
-
                                header: header.clone(),
-
                                old: m.old.clone(),
-
                                new: m.new.clone(),
-
                                hunk: Some(hunk),
-
                            });
-
                        }
-
                        if let EofNewLine::OldMissing | EofNewLine::NewMissing = eof {
-
                            self.add_item(ReviewItem::FileEofChanged {
-
                                path: m.path.clone(),
-
                                header: header.clone(),
-
                                old: m.old.clone(),
-
                                new: m.new.clone(),
-
                                eof,
-
                            })
-
                        }
-
                    }
-
                }
-
            }
-
        }
-
    }
-

-
    fn add_item(&mut self, item: ReviewItem) {
-
        self.queue.push_back((self.queue.len(), item));
-
    }
-
}
-

-
impl From<Diff> 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)>;
-

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

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

-
impl Iterator for ReviewQueue {
-
    type Item = (usize, ReviewItem);
-

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

-
/// Builds a review for a single file.
-
/// Adjusts line deltas when a hunk is ignored.
-
pub struct FileReviewBuilder {
-
    header: FileHeader,
-
    delta: i32,
-
}
-

-
impl FileReviewBuilder {
-
    fn new(item: &ReviewItem) -> Self {
-
        Self {
-
            header: item.file_header(),
-
            delta: 0,
-
        }
-
    }
-

-
    fn set_item(&mut self, item: &ReviewItem) -> &mut Self {
-
        let header = item.file_header();
-
        if self.header != header {
-
            self.header = header;
-
            self.delta = 0;
-
        }
-
        self
-
    }
-

-
    fn ignore_item(&mut self, item: &ReviewItem) {
-
        if let Some(h) = item.hunk_header() {
-
            self.delta += h.new_size as i32 - h.old_size as i32;
-
        }
-
    }
-

-
    fn item_diff(&mut self, item: ReviewItem) -> Result<git::raw::Diff<'_>, Error> {
-
        let mut buf = Vec::new();
-
        let mut writer = unified_diff::Writer::new(&mut buf);
-
        writer.encode(&self.header)?;
-

-
        if let (Some(h), Some(mut header)) = (item.hunk(), item.hunk_header()) {
-
            header.old_line_no -= self.delta as u32;
-
            header.new_line_no -= self.delta as u32;
-

-
            let h = Hunk {
-
                header: header.to_unified_string()?.as_bytes().to_owned().into(),
-
                lines: h.lines.clone(),
-
                old: h.old.clone(),
-
                new: h.new.clone(),
-
            };
-
            writer.encode(&h)?;
-
        }
-
        drop(writer);
-

-
        git::raw::Diff::from_buffer(&buf).map_err(Error::from)
-
    }
-
}
-

-
/// 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::fmt::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<Self, git::raw::Error> {
-
        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<Self, git::raw::Error> {
-
        // 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::fmt::Namespaced<'a> {
-
        git::refs::storage::draft::review(remote, patch)
-
    }
-
}
-

-
/// Builds a patch review interactively, across multiple files.
-
pub struct ReviewBuilder<'a> {
-
    /// Patch being reviewed.
-
    patch_id: PatchId,
-
    /// 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, repo: &'a Repository) -> Self {
-
        Self {
-
            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<G>(
-
        self,
-
        revision: &Revision,
-
        opts: &mut git::raw::DiffOptions,
-
        signer: &Device<G>,
-
    ) -> anyhow::Result<()>
-
    where
-
        G: crypto::signature::Signer<crypto::Signature>,
-
    {
-
        let repo = self.repo.raw();
-
        let base = repo.find_commit((*revision.base()).into())?;
-
        let patch_id = self.patch_id;
-
        let tree = {
-
            let commit = repo.find_commit(revision.head().into())?;
-
            commit.tree()?
-
        };
-

-
        let stdout = io::stdout().lock();
-
        let mut stdin = io::stdin().lock();
-
        let mut writer: Box<dyn PromptWriter> = if self.hunk.is_some() || !stdout.is_terminal() {
-
            Box::new(stdout)
-
        } else {
-
            Box::new(io::stderr().lock())
-
        };
-
        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(b.head.id()))),
-
                term::format::tertiary(&patch_id)
-
            );
-
            b
-
        } else {
-
            Brain::new(self.patch_id, signer.public_key(), base, repo)?
-
        };
-
        let diff = self.diff(&brain.accepted, &tree, repo, 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)?;
-
        let mut queue = ReviewQueue::from(diff);
-

-
        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<FileReviewBuilder> = None;
-
        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 file = match file.as_mut() {
-
                Some(fr) => fr.set_item(&item),
-
                None => file.insert(FileReviewBuilder::new(&item)),
-
            };
-
            term::element::write_to(
-
                &item.pretty(repo),
-
                &mut writer,
-
                term::Constraint::from_env().unwrap_or_default(),
-
            )?;
-

-
            // Prompts the user for action on the above hunk.
-
            match self.prompt(&mut stdin, &mut writer, progress) {
-
                // When a hunk is accepted, we convert it to unified diff format,
-
                // and apply it to the `brain`.
-
                Some(ReviewAction::Accept) => {
-
                    // 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 brain to {}", brain.cid());
-
                    }
-
                }
-
                Some(ReviewAction::Ignore) => {
-
                    // Do nothing. Hunk will be reviewable again next time.
-
                    file.ignore_item(&item);
-
                }
-
                Some(ReviewAction::Comment) => {
-
                    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,
-
                                    comment.body,
-
                                    Some(comment.location),
-
                                    None,   // Not a reply.
-
                                    vec![], // No embeds.
-
                                )?;
-
                            }
-
                            Ok(())
-
                        })?;
-
                    } else {
-
                        eprintln!(
-
                            "{}",
-
                            term::format::tertiary(
-
                                "Commenting on binary blobs 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 diff(
-
        &self,
-
        brain: &git::raw::Tree<'_>,
-
        tree: &git::raw::Tree<'_>,
-
        repo: &'a git::raw::Repository,
-
        opts: &mut git::raw::DiffOptions,
-
    ) -> Result<Diff, Error> {
-
        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))?;
-

-
        let diff = Diff::try_from(diff)?;
-

-
        Ok(diff)
-
    }
-

-
    fn prompt(
-
        &self,
-
        mut input: impl io::BufRead,
-
        output: &mut impl PromptWriter,
-
        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,q,?]").bold();
-

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

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

-
#[derive(thiserror::Error, Debug)]
-
enum Error {
-
    #[error(transparent)]
-
    Diff(#[from] unified_diff::Error),
-
    #[error(transparent)]
-
    Surf(#[from] radicle_surf::diff::git::error::Diff),
-
    #[error(transparent)]
-
    Io(#[from] io::Error),
-
    #[error(transparent)]
-
    Format(#[from] std::fmt::Error),
-
    #[error(transparent)]
-
    Git(#[from] git::raw::Error),
-
}
-

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

-
impl CommentBuilder {
-
    fn new(commit: Oid, path: PathBuf) -> Self {
-
        Self {
-
            commit,
-
            path,
-
            comments: Vec::new(),
-
        }
-
    }
-

-
    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}")?;
-
        }
-
        let output = term::Editor::comment()
-
            .extension("diff")
-
            .initial(input)?
-
            .edit()?;
-

-
        if let Some(output) = output {
-
            let header = HunkHeader::try_from(hunk)?;
-
            self.add_hunk(header, &output);
-
        }
-
        Ok(self.comments())
-
    }
-

-
    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);
-
        let mut comment = String::new();
-

-
        for line in lines {
-
            if line.starts_with('>') {
-
                if !comment.is_empty() {
-
                    self.add_comment(
-
                        &hunk,
-
                        &comment,
-
                        old_start..old_line - 1,
-
                        new_start..new_line - 1,
-
                    );
-

-
                    old_start = old_line - 1;
-
                    new_start = new_line - 1;
-

-
                    comment.clear();
-
                }
-
                match line.trim_start_matches('>').trim_start().chars().next() {
-
                    Some('-') => old_line += 1,
-
                    Some('+') => new_line += 1,
-
                    _ => {
-
                        old_line += 1;
-
                        new_line += 1;
-
                    }
-
                }
-
            } else {
-
                comment.push_str(line);
-
                comment.push('\n');
-
            }
-
        }
-
        if !comment.is_empty() {
-
            self.add_comment(
-
                &hunk,
-
                &comment,
-
                old_start..old_line - 1,
-
                new_start..new_line - 1,
-
            );
-
        }
-
        self
-
    }
-

-
    fn add_comment(
-
        &mut self,
-
        hunk: &HunkHeader,
-
        comment: &str,
-
        mut old_range: Range<usize>,
-
        mut new_range: Range<usize>,
-
    ) {
-
        // Empty lines between quoted text can generate empty comments
-
        // that should be filtered out.
-
        if comment.trim().is_empty() {
-
            return;
-
        }
-
        // Top-level comment, it should apply to the whole hunk.
-
        if old_range.is_empty() && new_range.is_empty() {
-
            old_range = hunk.old_line_no as usize..(hunk.old_line_no + hunk.old_size + 1) as usize;
-
            new_range = hunk.new_line_no as usize..(hunk.new_line_no + hunk.new_size + 1) as usize;
-
        }
-
        let old_range = old_range
-
            .is_empty()
-
            .not()
-
            .then_some(old_range)
-
            .map(|range| CodeRange::Lines { range });
-
        let new_range = (new_range)
-
            .is_empty()
-
            .not()
-
            .then_some(new_range)
-
            .map(|range| CodeRange::Lines { range });
-

-
        self.comments.push(ReviewComment {
-
            location: CodeLocation {
-
                commit: self.commit,
-
                path: self.path.clone(),
-
                old: old_range,
-
                new: new_range,
-
            },
-
            body: comment.trim().to_owned(),
-
        });
-
    }
-

-
    fn comments(self) -> Vec<ReviewComment> {
-
        self.comments
-
    }
-
}
-
#[cfg(test)]
-
mod tests {
-
    use super::*;
-

-
    #[test]
-
    fn test_review_comments_basic() {
-
        let input = r#"
-
> @@ -2559,18 +2560,18 @@ where
-
>                  // Only consider onion addresses if configured.
-
>                  AddressType::Onion => self.config.onion.is_some(),
-
>                  AddressType::Dns | AddressType::Ipv4 | AddressType::Ipv6 => true,
-
> -            })
-
> -            .take(wanted)
-
> -            .collect::<Vec<_>>(); // # -2564
-

-
Comment #1.
-

-
> +            });
-
>
-
> -        if available.len() < target {
-
> -            log::warn!( # -2567
-
> +        // Peers we are going to attempt connections to.
-
> +        let connect = available.take(wanted).collect::<Vec<_>>();
-

-
Comment #2.
-

-
> +        if connect.len() < wanted {
-
> +            log::debug!(
-
>                  target: "service",
-
> -                "Not enough available peers to connect to (available={}, target={target})",
-
> -                available.len()
-

-
Comment #3.
-

-
> +                "Not enough available peers to connect to (available={}, wanted={wanted})",
-

-
Comment #4.
-

-
> +                connect.len()
-
>              );
-
>          }
-
> -        for (id, ka) in available {
-
> +        for (id, ka) in connect {
-
>              self.connect(id, ka.addr.clone());
-
>          }
-
>     }
-

-
Comment #5.
-

-
"#;
-

-
        let commit = Oid::from_str("a32c4b93e2573fd83b15ac1ad6bf1317dc8fd760").unwrap();
-
        let path = PathBuf::from_str("main.rs").unwrap();
-
        let expected = &[
-
            (ReviewComment {
-
                location: CodeLocation {
-
                    commit,
-
                    path: path.clone(),
-
                    old: Some(CodeRange::Lines { range: 2559..2565 }),
-
                    new: Some(CodeRange::Lines { range: 2560..2563 }),
-
                },
-
                body: "Comment #1.".to_owned(),
-
            }),
-
            (ReviewComment {
-
                location: CodeLocation {
-
                    commit,
-
                    path: path.clone(),
-
                    old: Some(CodeRange::Lines { range: 2565..2568 }),
-
                    new: Some(CodeRange::Lines { range: 2563..2567 }),
-
                },
-
                body: "Comment #2.".to_owned(),
-
            }),
-
            (ReviewComment {
-
                location: CodeLocation {
-
                    commit,
-
                    path: path.clone(),
-
                    old: Some(CodeRange::Lines { range: 2568..2571 }),
-
                    new: Some(CodeRange::Lines { range: 2567..2570 }),
-
                },
-
                body: "Comment #3.".to_owned(),
-
            }),
-
            (ReviewComment {
-
                location: CodeLocation {
-
                    commit,
-
                    path: path.clone(),
-
                    old: None,
-
                    new: Some(CodeRange::Lines { range: 2570..2571 }),
-
                },
-
                body: "Comment #4.".to_owned(),
-
            }),
-
            (ReviewComment {
-
                location: CodeLocation {
-
                    commit,
-
                    path: path.clone(),
-
                    old: Some(CodeRange::Lines { range: 2571..2577 }),
-
                    new: Some(CodeRange::Lines { range: 2571..2578 }),
-
                },
-
                body: "Comment #5.".to_owned(),
-
            }),
-
        ];
-

-
        let mut builder = CommentBuilder::new(commit, path.clone());
-
        builder.add_hunk(
-
            HunkHeader {
-
                old_line_no: 2559,
-
                old_size: 18,
-
                new_line_no: 2560,
-
                new_size: 18,
-
                text: vec![],
-
            },
-
            input,
-
        );
-
        let actual = builder.comments();
-

-
        assert_eq!(actual.len(), expected.len(), "{actual:#?}");
-

-
        for (left, right) in actual.iter().zip(expected) {
-
            assert_eq!(left, right);
-
        }
-
    }
-

-
    #[test]
-
    fn test_review_comments_multiline() {
-
        let input = r#"
-
> @@ -2559,9 +2560,7 @@ where
-
>                  // Only consider onion addresses if configured.
-
>                  AddressType::Onion => self.config.onion.is_some(),
-
>                  AddressType::Dns | AddressType::Ipv4 | AddressType::Ipv6 => true,
-
> -            })
-
> -            .take(wanted)
-
> -            .collect::<Vec<_>>(); // # -2564
-

-
Blah blah blah blah blah blah blah.
-
Blah blah blah.
-

-
Blaah blaah blaah blaah blaah blaah blaah.
-
blaah blaah blaah.
-

-
Blaaah blaaah blaaah.
-

-
> +            });
-
>
-
> -        if available.len() < target {
-
> -            log::warn!( # -2567
-
> +        // Peers we are going to attempt connections to.
-
> +        let connect = available.take(wanted).collect::<Vec<_>>();
-

-
Woof woof.
-
Woof.
-
Woof.
-

-
Woof.
-

-
"#;
-

-
        let commit = Oid::from_str("a32c4b93e2573fd83b15ac1ad6bf1317dc8fd760").unwrap();
-
        let path = PathBuf::from_str("main.rs").unwrap();
-
        let expected = &[
-
            (ReviewComment {
-
                location: CodeLocation {
-
                    commit,
-
                    path: path.clone(),
-
                    old: Some(CodeRange::Lines { range: 2559..2565 }),
-
                    new: Some(CodeRange::Lines { range: 2560..2563 }),
-
                },
-
                body: r#"
-
Blah blah blah blah blah blah blah.
-
Blah blah blah.
-

-
Blaah blaah blaah blaah blaah blaah blaah.
-
blaah blaah blaah.
-

-
Blaaah blaaah blaaah.
-
"#
-
                .trim()
-
                .to_owned(),
-
            }),
-
            (ReviewComment {
-
                location: CodeLocation {
-
                    commit,
-
                    path: path.clone(),
-
                    old: Some(CodeRange::Lines { range: 2565..2568 }),
-
                    new: Some(CodeRange::Lines { range: 2563..2567 }),
-
                },
-
                body: r#"
-
Woof woof.
-
Woof.
-
Woof.
-

-
Woof.
-
"#
-
                .trim()
-
                .to_owned(),
-
            }),
-
        ];
-

-
        let mut builder = CommentBuilder::new(commit, path.clone());
-
        builder.add_hunk(
-
            HunkHeader {
-
                old_line_no: 2559,
-
                old_size: 9,
-
                new_line_no: 2560,
-
                new_size: 7,
-
                text: vec![],
-
            },
-
            input,
-
        );
-
        let actual = builder.comments();
-

-
        assert_eq!(actual.len(), expected.len(), "{actual:#?}");
-

-
        for (left, right) in actual.iter().zip(expected) {
-
            assert_eq!(left, right);
-
        }
-
    }
-

-
    #[test]
-
    fn test_review_comments_before() {
-
        let input = r#"
-
This is a top-level comment.
-

-
> @@ -2559,9 +2560,7 @@ where
-
>                  // Only consider onion addresses if configured.
-
>                  AddressType::Onion => self.config.onion.is_some(),
-
>                  AddressType::Dns | AddressType::Ipv4 | AddressType::Ipv6 => true,
-
> -            })
-
> -            .take(wanted)
-
> -            .collect::<Vec<_>>(); // # -2564
-
> +            });
-
>
-
> -        if available.len() < target {
-
> -            log::warn!( # -2567
-
> +        // Peers we are going to attempt connections to.
-
> +        let connect = available.take(wanted).collect::<Vec<_>>();
-
"#;
-

-
        let commit = Oid::from_str("a32c4b93e2573fd83b15ac1ad6bf1317dc8fd760").unwrap();
-
        let path = PathBuf::from_str("main.rs").unwrap();
-
        let expected = &[(ReviewComment {
-
            location: CodeLocation {
-
                commit,
-
                path: path.clone(),
-
                old: Some(CodeRange::Lines { range: 2559..2569 }),
-
                new: Some(CodeRange::Lines { range: 2560..2568 }),
-
            },
-
            body: "This is a top-level comment.".to_owned(),
-
        })];
-

-
        let mut builder = CommentBuilder::new(commit, path.clone());
-
        builder.add_hunk(
-
            HunkHeader {
-
                old_line_no: 2559,
-
                old_size: 9,
-
                new_line_no: 2560,
-
                new_size: 7,
-
                text: vec![],
-
            },
-
            input,
-
        );
-
        let actual = builder.comments();
-

-
        assert_eq!(actual.len(), expected.len(), "{actual:#?}");
-

-
        for (left, right) in actual.iter().zip(expected) {
-
            assert_eq!(left, right);
-
        }
-
    }
-

-
    #[test]
-
    fn test_review_comments_split_hunk() {
-
        let input = r#"
-
> @@ -2559,6 +2560,4 @@ where
-
>                  // Only consider onion addresses if configured.
-
>                  AddressType::Onion => self.config.onion.is_some(),
-
>                  AddressType::Dns | AddressType::Ipv4 | AddressType::Ipv6 => true,
-
> -            })
-
> -            .take(wanted)
-

-
> -            .collect::<Vec<_>>();
-
> +            });
-

-
Comment on a split hunk.
-
"#;
-

-
        let commit = Oid::from_str("a32c4b93e2573fd83b15ac1ad6bf1317dc8fd760").unwrap();
-
        let path = PathBuf::from_str("main.rs").unwrap();
-
        let expected = &[(ReviewComment {
-
            location: CodeLocation {
-
                commit,
-
                path: path.clone(),
-
                old: Some(CodeRange::Lines { range: 2564..2565 }),
-
                new: Some(CodeRange::Lines { range: 2563..2564 }),
-
            },
-
            body: "Comment on a split hunk.".to_owned(),
-
        })];
-

-
        let mut builder = CommentBuilder::new(commit, path.clone());
-
        builder.add_hunk(
-
            HunkHeader {
-
                old_line_no: 2559,
-
                old_size: 6,
-
                new_line_no: 2560,
-
                new_size: 4,
-
                text: vec![],
-
            },
-
            input,
-
        );
-
        let actual = builder.comments();
-

-
        assert_eq!(actual.len(), expected.len(), "{actual:#?}");
-

-
        for (left, right) in actual.iter().zip(expected) {
-
            assert_eq!(left, right);
-
        }
-
    }
-
}
modified crates/radicle-cli/src/lib.rs
@@ -11,3 +11,12 @@ pub mod terminal;
mod warning;

extern crate radicle_localtime as localtime;
+

+
/// Returns an error saying that given command is obsolete and has been removed.
+
/// See also [`warning::obsolete`].
+
fn removed(command: &str) -> anyhow::Error {
+
    anyhow::anyhow!(
+
        "The command `{}` is obsolete and has been removed.",
+
        command
+
    )
+
}
modified crates/radicle-cli/src/warning.rs
@@ -55,6 +55,8 @@ pub(crate) fn deprecated(old: impl std::fmt::Display, new: impl std::fmt::Displa
}

/// Prints an obsoletion warning to standard error.
+
/// For removed commands, use [`crate::removed`].
+
#[allow(dead_code)] // As of 2026-01-09 all obsoletions have turned into removals. Keep for future use.
pub(crate) fn obsolete(command: impl std::fmt::Display) {
    eprintln!(
        "{} {} The command `{command}` is obsolete and will be removed. Please stop using it.",