Radish alpha
h
heartwood
Radicle Heartwood Protocol & Stack
Radicle
Git (anonymous pull)
Log in to clone via SSH
Commits ›
cli: Implement comment parsing out of diff hunks
Alexis Sellier committed 1 year ago
commit 605388f850d54ff4472ae326199d803514e691ff
parent 13707fee32d2c47ea60f6903d871e041429fee6e
1 file changed +476 -7
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);
+ 
        }
+ 
    }
+ 
}