//! Review builder.
//!
//! This module enables a user to review a patch by interactively viewing and accepting diff hunks.
//!
//! 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::fmt::Write as _;
use std::io;
use std::ops::{Not, Range};
use std::path::PathBuf;
use radicle::cob::patch::Revision;
use radicle::cob::{CodeLocation, CodeRange};
use radicle::git;
use radicle::git::Oid;
use radicle::prelude::*;
use radicle::storage::git::Repository;
use radicle_surf::diff::*;
use radicle_cli::git::unified_diff::{self, FileHeader};
use radicle_cli::git::unified_diff::{Encode, HunkHeader};
use radicle_cli::terminal as term;
use crate::git::HunkDiff;
/// Queue of items (usually hunks) left to review.
#[derive(Clone, Default, Debug)]
pub struct Hunks {
hunks: Vec<HunkDiff>,
}
impl Hunks {
pub fn new(base: Diff) -> Self {
let base_files = base.into_files();
let mut queue = Self::default();
for file in base_files {
queue.add_file(file);
}
queue
}
/// 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(HunkDiff::Moved { moved });
}
FileDiff::Copied(copied) => {
self.add_item(HunkDiff::Copied {
copied: copied.clone(),
});
}
FileDiff::Added(a) => {
self.add_item(HunkDiff::Added {
path: a.path.clone(),
header: header.clone(),
new: a.new.clone(),
hunk: if let DiffContent::Plain {
hunks: Hunks(mut hs),
..
} = a.diff.clone()
{
hs.pop()
} else {
None
},
_stats: a.diff.stats().cloned(),
});
}
FileDiff::Deleted(d) => {
self.add_item(HunkDiff::Deleted {
path: d.path.clone(),
header: header.clone(),
old: d.old.clone(),
hunk: if let DiffContent::Plain {
hunks: Hunks(mut hs),
..
} = d.diff.clone()
{
hs.pop()
} else {
None
},
_stats: d.diff.stats().cloned(),
});
}
FileDiff::Modified(m) => {
if m.old.mode != m.new.mode {
self.add_item(HunkDiff::ModeChanged {
path: m.path.clone(),
header: header.clone(),
old: m.old.clone(),
new: m.new.clone(),
});
}
match m.diff.clone() {
DiffContent::Empty => {
// Likely a file mode change, which is handled above.
}
DiffContent::Binary => {
self.add_item(HunkDiff::Modified {
path: m.path.clone(),
header: header.clone(),
old: m.old.clone(),
new: m.new.clone(),
hunk: None,
_stats: m.diff.stats().cloned(),
});
}
DiffContent::Plain {
hunks: Hunks(hunks),
eof,
stats,
} => {
let base_hunks = hunks.clone();
for hunk in base_hunks {
self.add_item(HunkDiff::Modified {
path: m.path.clone(),
header: header.clone(),
old: m.old.clone(),
new: m.new.clone(),
hunk: Some(hunk),
_stats: Some(stats),
});
}
if let EofNewLine::OldMissing | EofNewLine::NewMissing = eof {
self.add_item(HunkDiff::EofChanged {
path: m.path.clone(),
header: header.clone(),
old: m.old.clone(),
new: m.new.clone(),
_eof: eof,
})
}
}
}
}
}
}
fn add_item(&mut self, item: HunkDiff) {
self.hunks.push(item);
}
}
impl std::ops::Deref for Hunks {
type Target = Vec<HunkDiff>;
fn deref(&self) -> &Self::Target {
&self.hunks
}
}
impl std::ops::DerefMut for Hunks {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.hunks
}
}
pub struct DiffUtil<'a> {
repo: &'a Repository,
}
impl<'a> DiffUtil<'a> {
pub fn new(repo: &'a Repository) -> Self {
Self { repo }
}
pub fn all_diffs(&self, revision: &Revision) -> anyhow::Result<Diff> {
let repo = self.repo.raw();
let base = repo.find_commit((*revision.base()).into())?.tree()?;
let revision = {
let commit = repo.find_commit(revision.head().into())?;
commit.tree()?
};
let mut opts = git::raw::DiffOptions::new();
opts.patience(true).minimal(true).context_lines(3_u32);
let base_diff = self.diff(&base, &revision, repo, &mut opts)?;
Ok(base_diff)
}
pub 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)
}
}
/// Builds a patch review interactively, across multiple files.
pub struct ReviewBuilder<'a> {
/// Stored copy of repository.
repo: &'a Repository,
}
impl<'a> ReviewBuilder<'a> {
/// Create a new review builder.
pub fn new(repo: &'a Repository) -> Self {
Self { repo }
}
pub fn hunks(&self, revision: &Revision) -> anyhow::Result<Hunks> {
let diff = DiffUtil::new(self.repo).all_diffs(revision)?;
Ok(Hunks::new(diff))
}
}
#[derive(Debug, PartialEq, Eq)]
pub struct ReviewComment {
pub location: CodeLocation,
pub body: String,
}
#[derive(thiserror::Error, Debug)]
pub 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)]
pub struct CommentBuilder {
commit: Oid,
path: PathBuf,
comments: Vec<ReviewComment>,
}
impl CommentBuilder {
pub fn new(commit: Oid, path: PathBuf) -> Self {
Self {
commit,
path,
comments: Vec::new(),
}
}
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}")?;
}
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())
}
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);
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::*;
use std::str::FromStr;
#[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);
}
}
}