From c316ce8d706e34e44d080e062792d2c8a29a6941 Mon Sep 17 00:00:00 2001 From: Fintan Halpenny Date: Fri, 25 Nov 2022 12:07:54 +0000 Subject: [PATCH] radicle-surf: refactor diff This is an overhaul of the diff module. The major changes are as follows. It shouldn't be possible for an added or deleted file to have any other kind line changes other than additions or deletions. So there are now 3 different kinds of changes, Addition, Deletion, or Modification. Hunk and Hunks now take a generic type parameter to allow for the three types of changes. As a consequence a FileDiff does the same. For the actual file representations, they fill in their parameters as Addition, Deletion, or Modification. During this refactor some renaming was also made: * CreateFile -> Added -- as well as the created field being renamed to added. * DeleteFile -> Deleted * MoveFile -> Moved * CopyFile -> Copied * ModifiedFile -> Modified The Stats are also now carried in the Diff instead of being calculated. They can be retrieved from the `git2::Diff`. The final change was to remove the `source::Commit::stats` field in favour of accessing the `Diff::stats` instead. The above changes mean that the serialization of `Diff` and `Commit` are not backwards compatible. Signed-off-by: Fintan Halpenny X-Clacks-Overhead: GNU Terry Pratchett --- radicle-surf/examples/diff.rs | 6 +- radicle-surf/src/diff.rs | 344 ++++++++++-------------- radicle-surf/src/diff/git.rs | 427 ++++++++++++++++++++---------- radicle-surf/src/git/repo.rs | 22 +- radicle-surf/src/source/commit.rs | 59 +---- radicle-surf/t/src/diff.rs | 129 --------- radicle-surf/t/src/git/diff.rs | 203 ++++++++++---- radicle-surf/t/src/lib.rs | 3 - 8 files changed, 607 insertions(+), 586 deletions(-) delete mode 100644 radicle-surf/t/src/diff.rs diff --git a/radicle-surf/examples/diff.rs b/radicle-surf/examples/diff.rs index 8c7fb33..ebbe573 100644 --- a/radicle-surf/examples/diff.rs +++ b/radicle-surf/examples/diff.rs @@ -57,7 +57,7 @@ fn init_repository_or_exit(path_to_repo: &str) -> git::Repository { } fn print_diff_summary(diff: &Diff, elapsed_nanos: u128) { - diff.created.iter().for_each(|created| { + diff.added.iter().for_each(|created| { println!("+++ {:?}", created.path); }); diff.deleted.iter().for_each(|deleted| { @@ -69,10 +69,10 @@ fn print_diff_summary(diff: &Diff, elapsed_nanos: u128) { println!( "created {} / deleted {} / modified {} / total {}", - diff.created.len(), + diff.added.len(), diff.deleted.len(), diff.modified.len(), - diff.created.len() + diff.deleted.len() + diff.modified.len() + diff.added.len() + diff.deleted.len() + diff.modified.len() ); println!("diff took {} nanos ", elapsed_nanos); } diff --git a/radicle-surf/src/diff.rs b/radicle-surf/src/diff.rs index 8742734..e7b0676 100644 --- a/radicle-surf/src/diff.rs +++ b/radicle-surf/src/diff.rs @@ -17,54 +17,111 @@ #![allow(dead_code, unused_variables, missing_docs)] -use std::{convert::TryFrom, path::PathBuf, slice}; +use std::path::PathBuf; #[cfg(feature = "serde")] use serde::{ser, Serialize, Serializer}; pub mod git; +/// The serializable representation of a `git diff`. +/// +/// A [`Diff`] can be retrieved by the following functions: +/// * [`crate::git::Repository::diff`] +/// * [`crate::git::Repository::diff_commit`] #[cfg_attr(feature = "serde", derive(Serialize), serde(rename_all = "camelCase"))] -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, Default, PartialEq, Eq)] pub struct Diff { - pub created: Vec, - pub deleted: Vec, - pub moved: Vec, - pub copied: Vec, - pub modified: Vec, + pub added: Vec, + pub deleted: Vec, + pub moved: Vec, + pub copied: Vec, + pub modified: Vec, + pub stats: Stats, } -impl Default for Diff { - fn default() -> Self { - Self::new() +impl Diff { + pub fn new() -> Self { + Diff::default() + } + + fn modified( + &mut self, + path: PathBuf, + hunks: impl Into>, + eof: Option, + ) { + self.modified.push(Modified { + path, + diff: FileDiff::Plain { + hunks: hunks.into(), + }, + eof, + }) + } + + fn moved(&mut self, old_path: PathBuf, new_path: PathBuf) { + self.moved.push(Moved { old_path, new_path }); + } + + fn copied(&mut self, old_path: PathBuf, new_path: PathBuf) { + self.copied.push(Copied { old_path, new_path }); + } + + fn modified_binary(&mut self, path: PathBuf) { + self.modified.push(Modified { + path, + diff: FileDiff::Binary, + eof: None, + }) + } + + fn added(&mut self, path: PathBuf, diff: FileDiff) { + self.added.push(Added { path, diff }) + } + + fn deleted(&mut self, path: PathBuf, diff: FileDiff) { + self.deleted.push(Deleted { path, diff }) } } +/// A file that was added within a [`Diff`]. #[cfg_attr(feature = "serde", derive(Serialize))] #[derive(Clone, Debug, PartialEq, Eq)] -pub struct CreateFile { +pub struct Added { + /// The path to this file, relative to the repository root. pub path: PathBuf, - pub diff: FileDiff, + /// The set of [`Addition`]s to this file. + pub diff: FileDiff, } +/// A file that was deleted within a [`Diff`]. #[cfg_attr(feature = "serde", derive(Serialize))] #[derive(Clone, Debug, PartialEq, Eq)] -pub struct DeleteFile { +pub struct Deleted { + /// The path to this file, relative to the repository root. pub path: PathBuf, - pub diff: FileDiff, + /// The set of [`Deletion`]s to this file. + pub diff: FileDiff, } +/// A file that was moved within a [`Diff`]. #[cfg_attr(feature = "serde", derive(Serialize), serde(rename_all = "camelCase"))] #[derive(Clone, Debug, PartialEq, Eq)] -pub struct MoveFile { +pub struct Moved { + /// The old path to this file, relative to the repository root. pub old_path: PathBuf, + /// The new path to this file, relative to the repository root. pub new_path: PathBuf, } +/// A file that was copied within a [`Diff`]. #[cfg_attr(feature = "serde", derive(Serialize), serde(rename_all = "camelCase"))] #[derive(Clone, Debug, PartialEq, Eq)] -pub struct CopyFile { +pub struct Copied { + /// The old path to this file, relative to the repository root. pub old_path: PathBuf, + /// The new path to this file, relative to the repository root. pub new_path: PathBuf, } @@ -76,101 +133,79 @@ pub enum EofNewLine { BothMissing, } +/// A file that was modified within a [`Diff`]. #[cfg_attr(feature = "serde", derive(Serialize), serde(rename_all = "camelCase"))] #[derive(Clone, Debug, PartialEq, Eq)] -pub struct ModifiedFile { +pub struct Modified { + /// The path to this file, relative to the repository root. pub path: PathBuf, - pub diff: FileDiff, + /// The set of [`Modification`]s to this file. + pub diff: FileDiff, + /// Was there an EOF newline present. pub eof: Option, } -/// A set of changes belonging to one file. +/// The set of changes for a given file. #[cfg_attr( feature = "serde", derive(Serialize), serde(tag = "type", rename_all = "camelCase") )] #[derive(Clone, Debug, PartialEq, Eq)] -pub enum FileDiff { +pub enum FileDiff { + /// The file is a binary file and so no set of changes can be provided. Binary, + /// The set of changes, as [`Hunks`] for a plaintext file. #[cfg_attr(feature = "serde", serde(rename_all = "camelCase"))] - Plain { - hunks: Hunks, - }, + Plain { hunks: Hunks }, } /// Statistics describing a particular [`Diff`]. #[cfg_attr(feature = "serde", derive(Serialize), serde(rename_all = "camelCase"))] -#[derive(Clone, Debug, Eq, PartialEq)] +#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] pub struct Stats { - /// Get the total number of files changed in a diff. + /// Get the total number of files changed in a [`Diff`] pub files_changed: usize, - /// Get the total number of insertions in a diff. + /// Get the total number of insertions in a [`Diff`]. pub insertions: usize, - /// Get the total number of deletions in a diff. + /// Get the total number of deletions in a [`Diff`]. pub deletions: usize, } -/// A set of line changes. +/// A set of changes across multiple lines. +/// +/// The parameter `T` can be an [`Addition`], [`Deletion`], or +/// [`Modification`]. #[cfg_attr(feature = "serde", derive(Serialize), serde(rename_all = "camelCase"))] #[derive(Clone, Debug, PartialEq, Eq)] -pub struct Hunk { +pub struct Hunk { pub header: Line, - pub lines: Vec, + pub lines: Vec, } -/// A set of [`Hunk`]s. +/// A set of [`Hunk`] changes. #[cfg_attr(feature = "serde", derive(Serialize))] -#[derive(Clone, Debug, Default, PartialEq, Eq)] -pub struct Hunks(pub Vec); +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct Hunks(pub Vec>); -pub struct IterHunks<'a> { - inner: slice::Iter<'a, Hunk>, -} - -impl Hunks { - pub fn iter(&self) -> IterHunks<'_> { - IterHunks { - inner: self.0.iter(), - } +impl Default for Hunks { + fn default() -> Self { + Self(Default::default()) } } -impl From> for Hunks { - fn from(hunks: Vec) -> Self { +impl Hunks { + pub fn iter(&self) -> impl Iterator> { + self.0.iter() + } +} + +impl From>> for Hunks { + fn from(hunks: Vec>) -> Self { Self(hunks) } } -impl<'a> Iterator for IterHunks<'a> { - type Item = &'a Hunk; - - fn next(&mut self) -> Option { - self.inner.next() - } -} - -impl TryFrom> for Hunks { - type Error = git::error::Hunk; - - fn try_from(patch: git2::Patch) -> Result { - let mut hunks = Vec::new(); - for h in 0..patch.num_hunks() { - let (hunk, hunk_lines) = patch.hunk(h)?; - let header = Line(hunk.header().to_owned()); - let mut lines: Vec = Vec::new(); - - for l in 0..hunk_lines { - let line = patch.line_in_hunk(h, l)?; - let line = LineDiff::try_from(line)?; - lines.push(line); - } - hunks.push(Hunk { header, lines }); - } - Ok(Hunks(hunks)) - } -} - /// The content of a single line. #[derive(Clone, Debug, PartialEq, Eq)] pub struct Line(pub(crate) Vec); @@ -199,155 +234,68 @@ impl Serialize for Line { } } -/// Single line delta. Two of these are need to represented a modified line: one -/// addition and one deletion. Context is also represented with this type. +/// Either the modification of a single [`Line`], or just contextual +/// information. #[cfg_attr( feature = "serde", derive(Serialize), serde(tag = "type", rename_all = "camelCase") )] #[derive(Clone, Debug, PartialEq, Eq)] -pub enum LineDiff { - /// Line added. +pub enum Modification { + /// A lines is an addition in a file. #[cfg_attr(feature = "serde", serde(rename_all = "camelCase"))] - Addition { line: Line, line_num: u32 }, + Addition(Addition), - /// Line deleted. + /// A lines is a deletion in a file. #[cfg_attr(feature = "serde", serde(rename_all = "camelCase"))] - Deletion { line: Line, line_num: u32 }, + Deletion(Deletion), - /// Line context. + /// A contextual line in a file, i.e. there were no changes to the line. #[cfg_attr(feature = "serde", serde(rename_all = "camelCase"))] Context { line: Line, - line_num_old: u32, - line_num_new: u32, + line_no_old: u32, + line_no_new: u32, }, } -impl LineDiff { - pub fn addition(line: impl Into, line_num: u32) -> Self { - Self::Addition { +/// A addition of a [`Line`] at the `line_no`. +#[cfg_attr(feature = "serde", derive(Serialize), serde(rename_all = "camelCase"))] +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct Addition { + pub line: Line, + pub line_no: u32, +} + +/// A deletion of a [`Line`] at the `line_no`. +#[cfg_attr(feature = "serde", derive(Serialize), serde(rename_all = "camelCase"))] +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct Deletion { + pub line: Line, + pub line_no: u32, +} + +impl Modification { + pub fn addition(line: impl Into, line_no: u32) -> Self { + Self::Addition(Addition { line: line.into(), - line_num, - } + line_no, + }) } - pub fn deletion(line: impl Into, line_num: u32) -> Self { - Self::Deletion { + pub fn deletion(line: impl Into, line_no: u32) -> Self { + Self::Deletion(Deletion { line: line.into(), - line_num, - } + line_no, + }) } - pub fn context(line: impl Into, line_num_old: u32, line_num_new: u32) -> Self { + pub fn context(line: impl Into, line_no_old: u32, line_no_new: u32) -> Self { Self::Context { line: line.into(), - line_num_old, - line_num_new, - } - } -} - -impl Diff { - pub fn new() -> Self { - Diff { - created: Vec::new(), - deleted: Vec::new(), - moved: Vec::new(), - copied: Vec::new(), - modified: Vec::new(), - } - } - - pub(crate) fn add_modified_file( - &mut self, - path: PathBuf, - hunks: impl Into, - eof: Option, - ) { - // TODO: file diff can be calculated at this point - // Use pijul's transaction diff as an inspiration? - // https://nest.pijul.com/pijul_org/pijul:master/1468b7281a6f3785e9#anesp4Qdq3V - self.modified.push(ModifiedFile { - path, - diff: FileDiff::Plain { - hunks: hunks.into(), - }, - eof, - }); - } - - pub(crate) fn add_moved_file(&mut self, old_path: PathBuf, new_path: PathBuf) { - self.moved.push(MoveFile { old_path, new_path }); - } - - pub(crate) fn add_copied_file(&mut self, old_path: PathBuf, new_path: PathBuf) { - self.copied.push(CopyFile { old_path, new_path }); - } - - pub(crate) fn add_modified_binary_file(&mut self, path: PathBuf) { - self.modified.push(ModifiedFile { - path, - diff: FileDiff::Binary, - eof: None, - }); - } - - pub(crate) fn add_created_file(&mut self, path: PathBuf, diff: FileDiff) { - self.created.push(CreateFile { path, diff }); - } - - pub(crate) fn add_deleted_file(&mut self, path: PathBuf, diff: FileDiff) { - self.deleted.push(DeleteFile { path, diff }); - } - - pub fn stats(&self) -> Stats { - let mut deletions = 0; - let mut insertions = 0; - - for file in &self.modified { - if let self::FileDiff::Plain { ref hunks } = file.diff { - for hunk in hunks.iter() { - for line in &hunk.lines { - match line { - self::LineDiff::Addition { .. } => insertions += 1, - self::LineDiff::Deletion { .. } => deletions += 1, - _ => {}, - } - } - } - } - } - - for file in &self.created { - if let self::FileDiff::Plain { ref hunks } = file.diff { - for hunk in hunks.iter() { - for line in &hunk.lines { - if let self::LineDiff::Addition { .. } = line { - insertions += 1 - } - } - } - } - } - - for file in &self.deleted { - if let self::FileDiff::Plain { ref hunks } = file.diff { - for hunk in hunks.iter() { - for line in &hunk.lines { - if let self::LineDiff::Deletion { .. } = line { - deletions += 1 - } - } - } - } - } - - Stats { - files_changed: self.modified.len() + self.created.len() + self.deleted.len(), - insertions, - deletions, + line_no_old, + line_no_new, } } } diff --git a/radicle-surf/src/diff/git.rs b/radicle-surf/src/diff/git.rs index 1c00507..180498e 100644 --- a/radicle-surf/src/diff/git.rs +++ b/radicle-surf/src/diff/git.rs @@ -17,16 +17,34 @@ use std::convert::TryFrom; -use crate::diff::{self, Diff, EofNewLine, Hunk, Hunks, Line, LineDiff}; +use crate::diff::{self, Addition, Deletion, Diff, EofNewLine, Hunk, Hunks, Line, Modification}; pub mod error { use std::path::PathBuf; use thiserror::Error; - #[derive(Debug, Error, PartialEq, Eq)] + #[derive(Debug, Error)] #[non_exhaustive] - pub enum LineDiff { + pub enum Addition { + #[error(transparent)] + Git(#[from] git2::Error), + #[error("the new line number was missing for an added line")] + MissingNewLineNo, + } + + #[derive(Debug, Error)] + #[non_exhaustive] + pub enum Deletion { + #[error(transparent)] + Git(#[from] git2::Error), + #[error("the new line number was missing for an deleted line")] + MissingOldLineNo, + } + + #[derive(Debug, Error)] + #[non_exhaustive] + pub enum Modification { /// A Git `DiffLine` is invalid. #[error( "invalid `git2::DiffLine` which contains no line numbers for either side of the diff" @@ -34,19 +52,23 @@ pub mod error { Invalid, } - #[derive(Debug, Error, PartialEq)] + #[derive(Debug, Error)] #[non_exhaustive] pub enum Hunk { #[error(transparent)] Git(#[from] git2::Error), #[error(transparent)] - Line(#[from] LineDiff), + Line(#[from] Modification), } /// A Git diff error. - #[derive(Debug, PartialEq, Error)] + #[derive(Debug, Error)] #[non_exhaustive] pub enum Diff { + #[error(transparent)] + Addition(#[from] Addition), + #[error(transparent)] + Deletion(#[from] Deletion), /// A Git delta type isn't currently handled. #[error("git delta type is not handled")] DeltaUnhandled(git2::Delta), @@ -55,7 +77,7 @@ pub mod error { #[error(transparent)] Hunk(#[from] Hunk), #[error(transparent)] - Line(#[from] LineDiff), + Line(#[from] Modification), /// A patch is unavailable. #[error("couldn't retrieve patch for {0}")] PatchUnavailable(PathBuf), @@ -65,15 +87,116 @@ pub mod error { } } -impl<'a> TryFrom> for LineDiff { - type Error = error::LineDiff; +impl TryFrom> for Hunks { + type Error = error::Hunk; + + fn try_from(patch: git2::Patch) -> Result { + let mut hunks = Vec::new(); + for h in 0..patch.num_hunks() { + let (hunk, hunk_lines) = patch.hunk(h)?; + let header = Line(hunk.header().to_owned()); + let mut lines: Vec = Vec::new(); + + for l in 0..hunk_lines { + let line = patch.line_in_hunk(h, l)?; + let line = Modification::try_from(line)?; + lines.push(line); + } + hunks.push(Hunk { header, lines }); + } + Ok(Hunks(hunks)) + } +} + +impl TryFrom> for Hunks { + type Error = error::Addition; + + fn try_from(patch: git2::Patch) -> Result { + let mut hunks = Vec::with_capacity(patch.num_hunks()); + for h in 0..patch.num_hunks() { + let (hunk, hunk_lines) = patch.hunk(h)?; + let header = Line(hunk.header().to_owned()); + let mut lines: Vec = Vec::with_capacity(hunk_lines); + + for l in 0..hunk_lines { + let line = patch.line_in_hunk(h, l)?; + lines.push(Addition::try_from(line)?); + } + hunks.push(Hunk { header, lines }) + } + Ok(hunks.into()) + } +} + +impl TryFrom> for Hunks { + type Error = git2::Error; + + fn try_from(patch: git2::Patch) -> Result { + let mut hunks = Vec::with_capacity(patch.num_hunks()); + for h in 0..patch.num_hunks() { + let (hunk, hunk_lines) = patch.hunk(h)?; + let header = Line(hunk.header().to_owned()); + let mut lines: Vec = Vec::with_capacity(hunk_lines); + + for l in 0..hunk_lines { + let line = patch.line_in_hunk(h, l)?; + lines.push(Deletion::try_from(line).expect("TODO")); + } + hunks.push(Hunk { header, lines }) + } + Ok(hunks.into()) + } +} + +impl<'a> TryFrom> for Modification { + type Error = error::Modification; fn try_from(line: git2::DiffLine) -> Result { match (line.old_lineno(), line.new_lineno()) { (None, Some(n)) => Ok(Self::addition(line.content().to_owned(), n)), (Some(n), None) => Ok(Self::deletion(line.content().to_owned(), n)), (Some(l), Some(r)) => Ok(Self::context(line.content().to_owned(), l, r)), - (None, None) => Err(error::LineDiff::Invalid), + (None, None) => Err(error::Modification::Invalid), + } + } +} + +impl TryFrom> for Addition { + type Error = error::Addition; + + fn try_from(line: git2::DiffLine) -> Result { + debug_assert!( + line.old_lineno().is_none(), + "trying to build Addition with a modified line" + ); + Ok(Self { + line: line.content().to_owned().into(), + line_no: line.new_lineno().ok_or(error::Addition::MissingNewLineNo)?, + }) + } +} + +impl TryFrom> for Deletion { + type Error = error::Deletion; + + fn try_from(line: git2::DiffLine) -> Result { + debug_assert!( + line.new_lineno().is_none(), + "trying to build Deletion with a modified line" + ); + Ok(Self { + line: line.content().to_owned().into(), + line_no: line.old_lineno().ok_or(error::Deletion::MissingOldLineNo)?, + }) + } +} + +impl From for diff::Stats { + fn from(stats: git2::DiffStats) -> Self { + Self { + files_changed: stats.files_changed(), + insertions: stats.insertions(), + deletions: stats.deletions(), } } } @@ -82,137 +205,18 @@ impl<'a> TryFrom> for Diff { type Error = error::Diff; fn try_from(git_diff: git2::Diff) -> Result { - use git2::{Delta, Patch}; + use git2::Delta; let mut diff = Diff::new(); + diff.stats = git_diff.stats()?.into(); for (idx, delta) in git_diff.deltas().enumerate() { match delta.status() { - Delta::Added => { - let diff_file = delta.new_file(); - let path = diff_file - .path() - .ok_or(error::Diff::PathUnavailable)? - .to_path_buf(); - - let patch = Patch::from_diff(&git_diff, idx)?; - if let Some(patch) = patch { - diff.add_created_file( - path, - diff::FileDiff::Plain { - hunks: Hunks::try_from(patch)?, - }, - ); - } else { - diff.add_created_file( - path, - diff::FileDiff::Plain { - hunks: Hunks::default(), - }, - ); - } - }, - Delta::Deleted => { - let diff_file = delta.old_file(); - let path = diff_file - .path() - .ok_or(error::Diff::PathUnavailable)? - .to_path_buf(); - let patch = Patch::from_diff(&git_diff, idx)?; - if let Some(patch) = patch { - diff.add_deleted_file( - path, - diff::FileDiff::Plain { - hunks: Hunks::try_from(patch)?, - }, - ); - } else { - diff.add_deleted_file( - path, - diff::FileDiff::Plain { - hunks: Hunks::default(), - }, - ); - } - }, - Delta::Modified => { - let diff_file = delta.new_file(); - let path = diff_file - .path() - .ok_or(error::Diff::PathUnavailable)? - .to_path_buf(); - let patch = Patch::from_diff(&git_diff, idx)?; - - if let Some(patch) = patch { - let mut hunks: Vec = Vec::new(); - let mut old_missing_eof = false; - let mut new_missing_eof = false; - - for h in 0..patch.num_hunks() { - let (hunk, hunk_lines) = patch.hunk(h)?; - let header = Line(hunk.header().to_owned()); - let mut lines: Vec = Vec::new(); - - for l in 0..hunk_lines { - let line = patch.line_in_hunk(h, l)?; - match line.origin_value() { - git2::DiffLineType::ContextEOFNL => { - new_missing_eof = true; - old_missing_eof = true; - continue; - }, - git2::DiffLineType::AddEOFNL => { - old_missing_eof = true; - continue; - }, - git2::DiffLineType::DeleteEOFNL => { - new_missing_eof = true; - continue; - }, - _ => {}, - } - let line = LineDiff::try_from(line)?; - lines.push(line); - } - hunks.push(Hunk { header, lines }); - } - let eof = match (old_missing_eof, new_missing_eof) { - (true, true) => Some(EofNewLine::BothMissing), - (true, false) => Some(EofNewLine::OldMissing), - (false, true) => Some(EofNewLine::NewMissing), - (false, false) => None, - }; - diff.add_modified_file(path, hunks, eof); - } else if diff_file.is_binary() { - diff.add_modified_binary_file(path); - } else { - return Err(error::Diff::PatchUnavailable(path)); - } - }, - Delta::Renamed => { - let old = delta - .old_file() - .path() - .ok_or(error::Diff::PathUnavailable)?; - let new = delta - .new_file() - .path() - .ok_or(error::Diff::PathUnavailable)?; - - diff.add_moved_file(old.to_path_buf(), new.to_path_buf()); - }, - Delta::Copied => { - let old = delta - .old_file() - .path() - .ok_or(error::Diff::PathUnavailable)?; - let new = delta - .new_file() - .path() - .ok_or(error::Diff::PathUnavailable)?; - - diff.add_copied_file(old.to_path_buf(), new.to_path_buf()); - }, + Delta::Added => created(&mut diff, &git_diff, idx, &delta)?, + Delta::Deleted => deleted(&mut diff, &git_diff, idx, &delta)?, + Delta::Modified => modified(&mut diff, &git_diff, idx, &delta)?, + Delta::Renamed => renamed(&mut diff, &delta)?, + Delta::Copied => copied(&mut diff, &delta)?, status => { return Err(error::Diff::DeltaUnhandled(status)); }, @@ -222,3 +226,154 @@ impl<'a> TryFrom> for Diff { Ok(diff) } } + +fn created( + diff: &mut Diff, + git_diff: &git2::Diff<'_>, + idx: usize, + delta: &git2::DiffDelta<'_>, +) -> Result<(), error::Diff> { + let diff_file = delta.new_file(); + let path = diff_file + .path() + .ok_or(error::Diff::PathUnavailable)? + .to_path_buf(); + + let patch = git2::Patch::from_diff(git_diff, idx)?; + if let Some(patch) = patch { + diff.added( + path, + diff::FileDiff::Plain { + hunks: Hunks::try_from(patch)?, + }, + ); + } else { + diff.added( + path, + diff::FileDiff::Plain { + hunks: Hunks::default(), + }, + ); + } + Ok(()) +} + +fn deleted( + diff: &mut Diff, + git_diff: &git2::Diff<'_>, + idx: usize, + delta: &git2::DiffDelta<'_>, +) -> Result<(), error::Diff> { + let diff_file = delta.old_file(); + let path = diff_file + .path() + .ok_or(error::Diff::PathUnavailable)? + .to_path_buf(); + let patch = git2::Patch::from_diff(git_diff, idx)?; + if let Some(patch) = patch { + diff.deleted( + path, + diff::FileDiff::Plain { + hunks: Hunks::try_from(patch)?, + }, + ); + } else { + diff.deleted( + path, + diff::FileDiff::Plain { + hunks: Hunks::default(), + }, + ); + } + Ok(()) +} + +fn modified( + diff: &mut Diff, + git_diff: &git2::Diff<'_>, + idx: usize, + delta: &git2::DiffDelta<'_>, +) -> Result<(), error::Diff> { + let diff_file = delta.new_file(); + let path = diff_file + .path() + .ok_or(error::Diff::PathUnavailable)? + .to_path_buf(); + let patch = git2::Patch::from_diff(git_diff, idx)?; + + if let Some(patch) = patch { + let mut hunks: Vec> = Vec::new(); + let mut old_missing_eof = false; + let mut new_missing_eof = false; + + for h in 0..patch.num_hunks() { + let (hunk, hunk_lines) = patch.hunk(h)?; + let header = Line(hunk.header().to_owned()); + let mut lines: Vec = Vec::new(); + + for l in 0..hunk_lines { + let line = patch.line_in_hunk(h, l)?; + match line.origin_value() { + git2::DiffLineType::ContextEOFNL => { + new_missing_eof = true; + old_missing_eof = true; + continue; + }, + git2::DiffLineType::AddEOFNL => { + old_missing_eof = true; + continue; + }, + git2::DiffLineType::DeleteEOFNL => { + new_missing_eof = true; + continue; + }, + _ => {}, + } + let line = Modification::try_from(line)?; + lines.push(line); + } + hunks.push(Hunk { header, lines }); + } + let eof = match (old_missing_eof, new_missing_eof) { + (true, true) => Some(EofNewLine::BothMissing), + (true, false) => Some(EofNewLine::OldMissing), + (false, true) => Some(EofNewLine::NewMissing), + (false, false) => None, + }; + diff.modified(path, hunks, eof); + Ok(()) + } else if diff_file.is_binary() { + diff.modified_binary(path); + Ok(()) + } else { + Err(error::Diff::PatchUnavailable(path)) + } +} + +fn renamed(diff: &mut Diff, delta: &git2::DiffDelta<'_>) -> Result<(), error::Diff> { + let old = delta + .old_file() + .path() + .ok_or(error::Diff::PathUnavailable)?; + let new = delta + .new_file() + .path() + .ok_or(error::Diff::PathUnavailable)?; + + diff.moved(old.to_path_buf(), new.to_path_buf()); + Ok(()) +} + +fn copied(diff: &mut Diff, delta: &git2::DiffDelta<'_>) -> Result<(), error::Diff> { + let old = delta + .old_file() + .path() + .ok_or(error::Diff::PathUnavailable)?; + let new = delta + .new_file() + .path() + .ok_or(error::Diff::PathUnavailable)?; + + diff.copied(old.to_path_buf(), new.to_path_buf()); + Ok(()) +} diff --git a/radicle-surf/src/git/repo.rs b/radicle-surf/src/git/repo.rs index 04bb7b6..0bd455b 100644 --- a/radicle-surf/src/git/repo.rs +++ b/radicle-surf/src/git/repo.rs @@ -171,15 +171,12 @@ impl Repository { .and_then(|diff| Diff::try_from(diff).map_err(Error::from)) } - /// Get the [`Diff`] of a commit with no parents. - pub fn initial_diff(&self, rev: R) -> Result { - let commit = self.get_git2_commit(self.object_id(&rev)?)?; - self.diff_commits(None, None, &commit) - .and_then(|diff| Diff::try_from(diff).map_err(Error::from)) - } - - /// Get the diff introduced by a particlar rev. - pub fn diff_from_parent(&self, commit: C) -> Result { + /// Get the [`Diff`] of a `commit`. + /// + /// If the `commit` has a parent, then it the diff will be a + /// comparison between itself and that parent. Otherwise, the left + /// hand side of the diff will pass nothing. + pub fn diff_commit(&self, commit: impl ToCommit) -> Result { let commit = commit .to_commit(self) .map_err(|err| Error::ToCommit(err.into()))?; @@ -371,6 +368,13 @@ impl Repository { Ok(contained_branches) } + /// Get the [`Diff`] of a commit with no parents. + fn initial_diff(&self, rev: R) -> Result { + let commit = self.get_git2_commit(self.object_id(&rev)?)?; + self.diff_commits(None, None, &commit) + .and_then(|diff| Diff::try_from(diff).map_err(Error::from)) + } + fn reachable_from(&self, reference: &git2::Reference, oid: &Oid) -> Result { let git2_oid = (*oid).into(); let other = reference.peel_to_commit()?.id(); diff --git a/radicle-surf/src/source/commit.rs b/radicle-surf/src/source/commit.rs index 3bb3ce6..88285d1 100644 --- a/radicle-surf/src/source/commit.rs +++ b/radicle-surf/src/source/commit.rs @@ -34,24 +34,12 @@ use crate::{ use radicle_git_ext::Oid; -/// Commit statistics. -#[cfg_attr(feature = "serde", derive(Serialize))] -#[derive(Clone)] -pub struct Stats { - /// Additions. - pub additions: u64, - /// Deletions. - pub deletions: u64, -} - /// Representation of a changeset between two revs. #[cfg_attr(feature = "serde", derive(Serialize))] #[derive(Clone)] pub struct Commit { /// The commit header. pub header: Header, - /// The change statistics for this commit. - pub stats: Stats, /// The changeset introduced by this commit. pub diff: diff::Diff, /// The list of branches this commit belongs to. @@ -150,48 +138,7 @@ pub fn commit(repo: &Repository, rev: R) -> Result additions += 1, - diff::LineDiff::Deletion { .. } => deletions += 1, - _ => {}, - } - } - } - } - } - - for file in &diff.created { - if let diff::FileDiff::Plain { ref hunks } = file.diff { - for hunk in hunks.iter() { - for line in &hunk.lines { - if let diff::LineDiff::Addition { .. } = line { - additions += 1 - } - } - } - } - } - - for file in &diff.deleted { - if let diff::FileDiff::Plain { ref hunks } = file.diff { - for hunk in hunks.iter() { - for line in &hunk.lines { - if let diff::LineDiff::Deletion { .. } = line { - deletions += 1 - } - } - } - } - } + let diff = repo.diff_commit(commit)?; let branches = repo .revision_branches(&sha1, Glob::all_heads().branches().and(Glob::all_remotes()))? @@ -201,10 +148,6 @@ pub fn commit(repo: &Repository, rev: R) -> Result Result<(), Error> { let commit = repo.commit(oid).unwrap(); assert!(commit.parents.is_empty()); - let diff = repo.initial_diff(oid)?; + let diff = repo.diff_commit(oid)?; let expected_diff = Diff { - created: vec![CreateFile { + added: vec![Added { path: Path::new("README.md").to_path_buf(), diff: FileDiff::Plain { hunks: vec![Hunk { header: Line::from(b"@@ -0,0 +1 @@\n".to_vec()), - lines: vec![LineDiff::addition( - b"This repository is a data source for the Upstream front-end tests.\n" - .to_vec(), - 1, - )], + lines: vec![Addition { + line: + b"This repository is a data source for the Upstream front-end tests.\n" + .to_vec() + .into(), + line_no: 1, + }], }] .into(), }, @@ -37,6 +52,11 @@ fn test_initial_diff() -> Result<(), Error> { moved: vec![], copied: vec![], modified: vec![], + stats: Stats { + files_changed: 1, + insertions: 1, + deletions: 0, + }, }; assert_eq!(expected_diff, diff); @@ -46,8 +66,8 @@ fn test_initial_diff() -> Result<(), Error> { #[test] fn test_diff_of_rev() -> Result<(), Error> { let repo = Repository::open(GIT_PLATINUM)?; - let diff = repo.diff_from_parent("80bacafba303bf0cdf6142921f430ff265f25095")?; - assert_eq!(diff.created.len(), 0); + let diff = repo.diff_commit("80bacafba303bf0cdf6142921f430ff265f25095")?; + assert_eq!(diff.added.len(), 0); assert_eq!(diff.deleted.len(), 0); assert_eq!(diff.moved.len(), 0); assert_eq!(diff.modified.len(), 1); @@ -63,25 +83,30 @@ fn test_diff() -> Result<(), Error> { let diff = repo.diff(*parent_oid, oid)?; let expected_diff = Diff { - created: vec![], - deleted: vec![], - moved: vec![], - copied: vec![], - modified: vec![ModifiedFile { - path: Path::new("README.md").to_path_buf(), - diff: FileDiff::Plain { - hunks: vec![Hunk { - header: Line::from(b"@@ -1 +1,2 @@\n".to_vec()), - lines: vec![ - LineDiff::deletion(b"This repository is a data source for the Upstream front-end tests.\n".to_vec(), 1), - LineDiff::addition(b"This repository is a data source for the Upstream front-end tests and the\n".to_vec(), 1), - LineDiff::addition(b"[`radicle-surf`](https://github.com/radicle-dev/git-platinum) unit tests.\n".to_vec(), 2), - ] - }].into() - }, - eof: None, - }] - }; + added: vec![], + deleted: vec![], + moved: vec![], + copied: vec![], + modified: vec![Modified { + path: Path::new("README.md").to_path_buf(), + diff: FileDiff::Plain { + hunks: vec![Hunk { + header: Line::from(b"@@ -1 +1,2 @@\n".to_vec()), + lines: vec![ + Modification::deletion(b"This repository is a data source for the Upstream front-end tests.\n".to_vec(), 1), + Modification::addition(b"This repository is a data source for the Upstream front-end tests and the\n".to_vec(), 1), + Modification::addition(b"[`radicle-surf`](https://github.com/radicle-dev/git-platinum) unit tests.\n".to_vec(), 2), + ] + }].into() + }, + eof: None, + }], + stats: Stats { + files_changed: 1, + insertions: 2, + deletions: 1, + }, + }; assert_eq!(expected_diff, diff); Ok(()) @@ -97,18 +122,18 @@ fn test_branch_diff() -> Result<(), Error> { println!("Diff two branches: master -> dev"); println!( - "result: created {} deleted {} moved {} modified {}", - diff.created.len(), + "result: added {} deleted {} moved {} modified {}", + diff.added.len(), diff.deleted.len(), diff.moved.len(), diff.modified.len() ); - assert_eq!(diff.created.len(), 1); + assert_eq!(diff.added.len(), 1); assert_eq!(diff.deleted.len(), 11); assert_eq!(diff.moved.len(), 1); assert_eq!(diff.modified.len(), 2); - for c in diff.created.iter() { - println!("created: {:?}", &c.path); + for c in diff.added.iter() { + println!("added: {:?}", &c.path); } for d in diff.deleted.iter() { println!("deleted: {:?}", &d.path); @@ -124,39 +149,42 @@ fn test_branch_diff() -> Result<(), Error> { #[test] fn test_diff_serde() { - use radicle_surf::diff::{Hunks, MoveFile}; - let diff = Diff { - created: vec![ CreateFile { + added: vec![ Added { path: Path::new("LICENSE").to_path_buf(), diff: FileDiff::Plain { hunks: Hunks::default() } }], deleted: vec![], - moved: vec![ MoveFile { + moved: vec![ Moved { old_path: Path::new("CONTRIBUTING").to_path_buf(), new_path: Path::new("CONTRIBUTING.md").to_path_buf(), }], copied: vec![], - modified: vec![ ModifiedFile { + modified: vec![ Modified { path: Path::new("README.md").to_path_buf(), diff: FileDiff::Plain { hunks: vec![Hunk { header: Line::from(b"@@ -1 +1,2 @@\n".to_vec()), lines: vec![ - LineDiff::deletion(b"This repository is a data source for the Upstream front-end tests.\n".to_vec(), 1), - LineDiff::addition(b"This repository is a data source for the Upstream front-end tests and the\n".to_vec(), 1), - LineDiff::addition(b"[`radicle-surf`](https://github.com/radicle-dev/git-platinum) unit tests.\n".to_vec(), 2), - LineDiff::context(b"\n".to_vec(), 3, 4), + Modification::deletion(b"This repository is a data source for the Upstream front-end tests.\n".to_vec(), 1), + Modification::addition(b"This repository is a data source for the Upstream front-end tests and the\n".to_vec(), 1), + Modification::addition(b"[`radicle-surf`](https://github.com/radicle-dev/git-platinum) unit tests.\n".to_vec(), 2), + Modification::context(b"\n".to_vec(), 3, 4), ] }].into() }, eof: None, - }] + }], + stats: Stats { + files_changed: 3, + insertions: 2, + deletions: 1, + }, }; let eof: Option = None; let json = serde_json::json!({ - "created": [{"path": "LICENSE", "diff": { + "added": [{"path": "LICENSE", "diff": { "type": "plain", "hunks": [] }, @@ -171,19 +199,19 @@ fn test_diff_serde() { "hunks": [{ "header": "@@ -1 +1,2 @@\n", "lines": [ - { "lineNum": 1, + { "lineNo": 1, "line": "This repository is a data source for the Upstream front-end tests.\n", "type": "deletion" }, - { "lineNum": 1, + { "lineNo": 1, "line": "This repository is a data source for the Upstream front-end tests and the\n", "type": "addition" }, - { "lineNum": 2, + { "lineNo": 2, "line": "[`radicle-surf`](https://github.com/radicle-dev/git-platinum) unit tests.\n", "type": "addition" }, - { "lineNumOld": 3, "lineNumNew": 4, + { "lineNoOld": 3, "lineNoNew": 4, "line": "\n", "type": "context" } @@ -191,7 +219,82 @@ fn test_diff_serde() { }] }, "eof" : eof, - }] + }], + "stats": { + "deletions": 1, + "filesChanged": 3, + "insertions": 2, + } }); assert_eq!(serde_json::to_value(&diff).unwrap(), json); } + +#[test] +fn test_both_missing_eof_newline() { + let buf = r#" +diff --git a/.env b/.env +index f89e4c0..7c56eb7 100644 +--- a/.env ++++ b/.env +@@ -1 +1 @@ +-hello=123 +\ No newline at end of file ++hello=1234 +\ No newline at end of file +"#; + let diff = git2::Diff::from_buffer(buf.as_bytes()).unwrap(); + let diff = Diff::try_from(diff).unwrap(); + assert_eq!(diff.modified[0].eof, Some(EofNewLine::BothMissing)); +} + +#[test] +fn test_none_missing_eof_newline() { + let buf = r#" +diff --git a/.env b/.env +index f89e4c0..7c56eb7 100644 +--- a/.env ++++ b/.env +@@ -1 +1 @@ +-hello=123 ++hello=1234 +"#; + let diff = git2::Diff::from_buffer(buf.as_bytes()).unwrap(); + let diff = Diff::try_from(diff).unwrap(); + assert_eq!(diff.modified[0].eof, None); +} + +// TODO(xphoniex): uncomment once libgit2 has fixed the bug +//#[test] +// fn test_old_missing_eof_newline() { +// let buf = r#" +// diff --git a/.env b/.env +// index f89e4c0..7c56eb7 100644 +// --- a/.env +// +++ b/.env +// @@ -1 +1 @@ +// -hello=123 +// \ No newline at end of file +// +hello=1234 +// "#; +// let diff = git2::Diff::from_buffer(buf.as_bytes()).unwrap(); +// let diff = Diff::try_from(diff).unwrap(); +// assert_eq!(diff.modified[0].eof, Some(EofNewLine::OldMissing)); +// } + +// TODO(xphoniex): uncomment once libgit2 has fixed the bug +//#[test] +// fn test_new_missing_eof_newline() { +// let buf = r#" +// diff --git a/.env b/.env +// index f89e4c0..7c56eb7 100644 +// --- a/.env +// +++ b/.env +// @@ -1 +1 @@ +// -hello=123 +// +hello=1234 +// \ No newline at end of file +// "#; +// let diff = git2::Diff::from_buffer(buf.as_bytes()).unwrap(); +// let diff = Diff::try_from(diff).unwrap(); +// assert_eq!(diff.modified[0].eof, Some(EofNewLine::NewMissing)); +// } diff --git a/radicle-surf/t/src/lib.rs b/radicle-surf/t/src/lib.rs index b1277eb..7d021db 100644 --- a/radicle-surf/t/src/lib.rs +++ b/radicle-surf/t/src/lib.rs @@ -3,6 +3,3 @@ #[cfg(test)] mod git; - -#[cfg(test)] -mod diff;