Radish alpha
h
heartwood
rad:z3gqcJUoA1n9HaHKufZs5FCSGazv5
Radicle Heartwood Protocol & Stack
Radicle
Git
Patches › ab7d8d77
cli: Code review in the CLI (Part 1.)
Merged did:key:z6MktaNv...ZRZW opened 1 year ago

Can be reviewed by commit. More detail there.

Activity Changes +491 -15
6 files changed +491 -15 a32c4b93 → 1388601c
modified radicle-cli/src/commands/diff.rs
@@ -27,6 +27,7 @@ Usage


Options
+ 
    --unified, -U   Context lines to show (default: 5)

    --staged        View staged changes

    --color         Force color output

    --help          Print help
modified 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<ReviewComment>,
+ 
}
+
+ 
impl CommentBuilder {
+ 
    fn new(commit: Oid, path: PathBuf) -> Self {
+ 
        Self {
+ 
            commit,
+ 
            path,
+ 
            comments: Vec::new(),
+ 
        }
+ 
    }
+
+ 
    fn edit(&mut self, hunk: &Hunk<Modification>) -> 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<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 radicle-cli/src/git/unified_diff.rs
@@ -132,13 +132,13 @@ impl HunkHeader {
    pub fn old_line_range(&self) -> std::ops::Range<u32> {

        let start: u32 = self.old_line_no;

        let end: u32 = self.old_line_no + self.old_size;
- 
        start..end
+ 
        start..end + 1

    }



    pub fn new_line_range(&self) -> std::ops::Range<u32> {

        let start: u32 = self.new_line_no;

        let end: u32 = self.new_line_no + self.new_size;
- 
        start..end
+ 
        start..end + 1

    }

}
modified radicle-cob/src/object/collaboration/error.rs
@@ -36,7 +36,7 @@ pub struct Remove {


#[derive(Debug, Error)]

pub enum Retrieve {
- 
    #[error("object failed to evaluate: {0}")]
+ 
    #[error(transparent)]

    Evaluate(Box<dyn std::error::Error + Send + Sync + 'static>),

    #[error(transparent)]

    Git(#[from] git2::Error),
@@ -57,7 +57,7 @@ impl Retrieve {


#[derive(Debug, Error)]

pub enum Update {
- 
    #[error("object failed to evaluate: {0}")]
+ 
    #[error(transparent)]

    Evaluate(Box<dyn std::error::Error + Send + Sync + 'static>),

    #[error("no object found")]

    NoSuchObject,
modified radicle-term/src/pager.rs
@@ -78,7 +78,7 @@ fn main<E: Element>(
            recv(events_rx) -> event => {

                let event = event??;

                let page = height as usize - 1; // Don't count the status bar.
- 
                let end = lines.len() - page;
+ 
                let end = if page > lines.len() { 0 } else { lines.len() - page };

                let prev = line;



                match event {
modified radicle/src/cob/patch.rs
@@ -103,6 +103,9 @@ pub enum Error {
    /// Identity document is missing.

    #[error("missing identity document")]

    MissingIdentity,
+ 
    /// Review is empty.
+ 
    #[error("empty review; verdict or summary not provided")]
+ 
    EmptyReview,

    /// Error loading the document payload.

    #[error("payload failed to load: {0}")]

    Payload(#[from] PayloadError),
@@ -132,7 +135,7 @@ pub enum Error {
        #[source]

        err: Box<dyn std::error::Error + Send + Sync + 'static>,

    },
- 
    #[error("failed to remove patch {id} from cache : {err}")]
+ 
    #[error("failed to remove patch {id} from cache: {err}")]

    CacheRemove {

        id: PatchId,

        #[source]
@@ -879,6 +882,9 @@ impl Patch {
                let Some(rev) = self.revisions.get_mut(&revision) else {

                    return Err(Error::Missing(revision.into_inner()));

                };
+ 
                if summary.is_none() && verdict.is_none() {
+ 
                    return Err(Error::EmptyReview);
+ 
                }

                if let Some(rev) = rev {

                    // Nb. Applying two reviews by the same author is not allowed and

                    // results in the review being redacted.
@@ -2928,7 +2934,7 @@ mod test {


        // It's fine to redact a review from a redacted revision.

        let review = patch
- 
            .review(updated, None, None, vec![], &alice.signer)
+ 
            .review(updated, Some(Verdict::Accept), None, vec![], &alice.signer)

            .unwrap();

        patch.redact(updated, &alice.signer).unwrap();

        patch.redact_review(review, &alice.signer).unwrap();
@@ -3160,7 +3166,7 @@ mod test {
            new: Some(CodeRange::Lines { range: 5..8 }),

        };

        let review = patch
- 
            .review(rid, None, None, vec![], &alice.signer)
+ 
            .review(rid, Some(Verdict::Accept), None, vec![], &alice.signer)

            .unwrap();

        patch

            .review_comment(