diff --git a/radicle-cli/src/commands/patch/review/builder.rs b/radicle-cli/src/commands/patch/review/builder.rs index 340efb09..63b25754 100644 --- a/radicle-cli/src/commands/patch/review/builder.rs +++ b/radicle-cli/src/commands/patch/review/builder.rs @@ -12,18 +12,23 @@ //! been fully reviewed. //! use std::collections::VecDeque; +use std::fmt::Write as _; use std::io::IsTerminal as _; +use std::ops::{Not, Range}; +use std::path::PathBuf; use std::str::FromStr; use std::{fmt, io}; use radicle::cob::patch::{PatchId, Revision, Verdict}; +use radicle::cob::{CodeLocation, CodeRange}; use radicle::git; use radicle::prelude::*; use radicle::storage::git::Repository; +use radicle_git_ext::Oid; use radicle_surf::diff::*; use crate::git::unified_diff; -use crate::git::unified_diff::Encode; +use crate::git::unified_diff::{Encode, HunkHeader}; use crate::terminal as term; /// Help message shown to user. @@ -338,11 +343,24 @@ impl<'a> ReviewBuilder<'a> { } } Some(ReviewAction::Comment) => { - eprintln!( - "{}", - term::format::tertiary("Commenting is not yet implemented").bold() - ); - queue.push_front((ix, item)); + if let Some(hunk) = hunk { + let mut builder = + CommentBuilder::new(revision.head(), item.file.path().to_path_buf()); + builder.edit(hunk)?; + + let _comments = builder.comments(); + + queue.push_front((ix, item)); + } else { + eprintln!( + "{}", + term::format::tertiary( + "Commenting on binary blobs is not yet implemented" + ) + .bold() + ); + queue.push_front((ix, item)); + } } Some(ReviewAction::Split) => { eprintln!( @@ -396,7 +414,7 @@ impl<'a> ReviewBuilder<'a> { Verdict::Reject => Some(ReviewAction::Ignore), } } else if output.is_terminal() { - let prompt = term::format::secondary("Accept this hunk? [y,n,c,j,k,?]").bold(); + let prompt = term::format::secondary("Accept this hunk? [y,n,c,j,k,q,?]").bold(); ReviewAction::prompt(&mut input, &mut output, format!("{progress} {prompt}")) .unwrap_or(Some(ReviewAction::Help)) @@ -412,3 +430,454 @@ impl<'a> ReviewBuilder<'a> { .peel_to_commit() } } + +#[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)] + Io(#[from] io::Error), + #[error(transparent)] + Format(#[from] std::fmt::Error), +} + +#[derive(Debug)] +struct CommentBuilder { + commit: Oid, + path: PathBuf, + comments: Vec, +} + +impl CommentBuilder { + fn new(commit: Oid, path: PathBuf) -> Self { + Self { + commit, + path, + comments: Vec::new(), + } + } + + fn edit(&mut self, hunk: &Hunk) -> Result<&mut Self, Error> { + let mut input = String::new(); + for line in hunk.to_unified_string()?.lines() { + writeln!(&mut input, "> {line}")?; + } + let output = term::Editor::new().extension("diff").edit(input)?; + + if let Some(output) = output { + let header = HunkHeader::try_from(hunk)?; + self.add_hunk(header, &output); + } + Ok(self) + } + + 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, + mut new_range: Range, + ) { + // 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 { + 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::>(); // # -2564 + +Comment #1. + +> + }); +> +> - if available.len() < target { +> - log::warn!( # -2567 +> + // Peers we are going to attempt connections to. +> + let connect = available.take(wanted).collect::>(); + +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::>(); // # -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::>(); + +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::>(); // # -2564 +> + }); +> +> - if available.len() < target { +> - log::warn!( # -2567 +> + // Peers we are going to attempt connections to. +> + let connect = available.take(wanted).collect::>(); +"#; + + 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::>(); +> + }); + +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); + } + } +}