From 99732c4fefd668c03efcccbe83c37034c8ea655e Mon Sep 17 00:00:00 2001 From: Han Xu Date: Mon, 6 Feb 2023 13:22:00 -0800 Subject: [PATCH] radicle-surf: fix EofNewLine - added `NoneMissing` case. - moved `EofNewLine` into `DiffContent::Plain`. Signed-off-by: Han Xu --- radicle-surf/src/diff.rs | 42 ++++---- radicle-surf/src/diff/git.rs | 180 ++++++++--------------------------- radicle-surf/t/src/diff.rs | 27 +++--- 3 files changed, 77 insertions(+), 172 deletions(-) diff --git a/radicle-surf/src/diff.rs b/radicle-surf/src/diff.rs index 9ad615f..98c9315 100644 --- a/radicle-surf/src/diff.rs +++ b/radicle-surf/src/diff.rs @@ -90,16 +90,8 @@ impl Diff { &self.stats } - fn insert_modified( - &mut self, - path: PathBuf, - hunks: impl Into>, - eof: Option, - ) { - let diff = DiffContent::Plain { - hunks: hunks.into(), - }; - let diff = FileDiff::Modified(Modified { path, eof, diff }); + fn insert_modified(&mut self, path: PathBuf, diff: DiffContent) { + let diff = FileDiff::Modified(Modified { path, diff }); self.files.push(diff) } @@ -121,15 +113,6 @@ impl Diff { self.files.push(diff); } - fn insert_modified_binary(&mut self, path: PathBuf) { - let diff = FileDiff::Modified(Modified { - path, - eof: None, - diff: DiffContent::Binary, - }); - self.files.push(diff) - } - fn insert_added(&mut self, path: PathBuf, diff: DiffContent) { let diff = FileDiff::Added(Added { path, diff }); self.files.push(diff); @@ -201,17 +184,21 @@ pub enum EofNewLine { OldMissing, NewMissing, BothMissing, + NoneMissing, +} + +impl Default for EofNewLine { + fn default() -> Self { + Self::NoneMissing + } } /// 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 Modified { - /// The path to this file, relative to the repository root. pub path: PathBuf, pub diff: DiffContent, - /// Was there an EOF newline present. - pub eof: Option, } /// The set of changes for a given file. @@ -228,10 +215,20 @@ pub enum DiffContent { #[cfg_attr(feature = "serde", serde(rename_all = "camelCase"))] Plain { hunks: Hunks, + eof: EofNewLine, }, Empty, } +impl DiffContent { + pub fn eof(&self) -> Option { + match self { + Self::Plain { hunks: _, eof } => Some(eof.clone()), + _ => None, + } + } +} + #[derive(Clone, Debug, PartialEq, Eq)] pub enum FileDiff { Added(Added), @@ -260,7 +257,6 @@ impl Serialize for FileDiff { FileDiff::Modified(x) => { state.serialize_field("path", &x.path)?; state.serialize_field("diff", &x.diff)?; - state.serialize_field("eof", &x.eof)? }, FileDiff::Moved(x) => { state.serialize_field("oldPath", &x.old_path)?; diff --git a/radicle-surf/src/diff/git.rs b/radicle-surf/src/diff/git.rs index a572db1..432fe3a 100644 --- a/radicle-surf/src/diff/git.rs +++ b/radicle-surf/src/diff/git.rs @@ -17,7 +17,7 @@ use std::convert::TryFrom; -use crate::diff::{self, Addition, Deletion, Diff, EofNewLine, Hunk, Hunks, Line, Modification}; +use super::{Diff, DiffContent, EofNewLine, Hunk, Hunks, Line, Modification, Stats}; pub mod error { use std::path::PathBuf; @@ -87,11 +87,14 @@ pub mod error { } } -impl TryFrom> for Hunks { +impl TryFrom> for DiffContent { type Error = error::Hunk; fn try_from(patch: git2::Patch) -> Result { let mut hunks = 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()); @@ -99,52 +102,37 @@ impl TryFrom> for Hunks { 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 }); } - 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 = error::Deletion; - - 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)?); - } - hunks.push(Hunk { header, lines }) - } - Ok(hunks.into()) + let eof = match (old_missing_eof, new_missing_eof) { + (true, true) => EofNewLine::BothMissing, + (true, false) => EofNewLine::OldMissing, + (false, true) => EofNewLine::NewMissing, + (false, false) => EofNewLine::NoneMissing, + }; + Ok(DiffContent::Plain { + hunks: Hunks(hunks), + eof, + }) } } @@ -161,37 +149,7 @@ impl<'a> TryFrom> for Modification { } } -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 { +impl From for Stats { fn from(stats: git2::DiffStats) -> Self { Self { files_changed: stats.files_changed(), @@ -241,19 +199,11 @@ fn created( let patch = git2::Patch::from_diff(git_diff, idx)?; if let Some(patch) = patch { - diff.insert_added( - path, - diff::DiffContent::Plain { - hunks: Hunks::try_from(patch)?, - }, - ); + diff.insert_added(path, DiffContent::try_from(patch)?); + } else if diff_file.is_binary() { + diff.insert_added(path, DiffContent::Binary); } else { - diff.insert_added( - path, - diff::DiffContent::Plain { - hunks: Hunks::default(), - }, - ); + return Err(error::Diff::PatchUnavailable(path)); } Ok(()) } @@ -271,19 +221,11 @@ fn deleted( .to_path_buf(); let patch = git2::Patch::from_diff(git_diff, idx)?; if let Some(patch) = patch { - diff.insert_deleted( - path, - diff::DiffContent::Plain { - hunks: Hunks::try_from(patch)?, - }, - ); + diff.insert_deleted(path, DiffContent::try_from(patch)?); + } else if diff_file.is_binary() { + diff.insert_deleted(path, DiffContent::Binary); } else { - diff.insert_deleted( - path, - diff::DiffContent::Plain { - hunks: Hunks::default(), - }, - ); + return Err(error::Diff::PatchUnavailable(path)); } Ok(()) } @@ -302,48 +244,10 @@ fn modified( 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.insert_modified(path, hunks, eof); + diff.insert_modified(path, DiffContent::try_from(patch)?); Ok(()) } else if diff_file.is_binary() { - diff.insert_modified_binary(path); + diff.insert_modified(path, DiffContent::Binary); Ok(()) } else { Err(error::Diff::PatchUnavailable(path)) diff --git a/radicle-surf/t/src/diff.rs b/radicle-surf/t/src/diff.rs index e82ef1e..8b3823f 100644 --- a/radicle-surf/t/src/diff.rs +++ b/radicle-surf/t/src/diff.rs @@ -45,6 +45,7 @@ fn test_initial_diff() -> Result<(), Error> { )], }] .into(), + eof: EofNewLine::default(), }, })]; @@ -79,7 +80,6 @@ fn test_diff_file() -> Result<(), Error> { )?; let expected_diff = FileDiff::Modified(Modified { path: path_buf, - eof: None, diff: DiffContent::Plain { hunks: vec![Hunk { header: Line::from(b"@@ -1 +1,2 @@\n".to_vec()), @@ -88,7 +88,8 @@ fn test_diff_file() -> Result<(), Error> { 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() + }].into(), + eof: EofNewLine::default(), }, }); assert_eq!(expected_diff, diff); @@ -106,7 +107,6 @@ fn test_diff() -> Result<(), Error> { let expected_files = vec![FileDiff::Modified(Modified { path: Path::new("README.md").to_path_buf(), - eof: None, diff: DiffContent::Plain { hunks: vec![Hunk { header: Line::from(b"@@ -1 +1,2 @@\n".to_vec()), @@ -115,7 +115,8 @@ fn test_diff() -> Result<(), Error> { 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() + }].into(), + eof: EofNewLine::default(), }, })]; let expected_stats = Stats { @@ -188,7 +189,6 @@ fn test_diff_serde() -> Result<(), Error> { let rev_to = Branch::local(refname!("diff-test")); let diff = repo.diff(rev_from, rev_to)?; - let eof: Option = None; let json = serde_json::json!({ "added": [{ "path": "LICENSE", @@ -206,7 +206,8 @@ fn test_diff_serde() -> Result<(), Error> { "lineNo": 2, "type": "addition", }] - }] + }], + "eof": "noneMissing", }, }], "deleted": [{ @@ -252,7 +253,8 @@ fn test_diff_serde() -> Result<(), Error> { "type": "deletion", }, ] - }] + }], + "eof": "noneMissing", }, }], "moved": [{ @@ -284,9 +286,9 @@ fn test_diff_serde() -> Result<(), Error> { "type": "addition" }, ] - }] + }], + "eof": "noneMissing", }, - "eof": eof, }], "stats": { "deletions": 9, @@ -315,7 +317,7 @@ index f89e4c0..7c56eb7 100644 let diff = git2::Diff::from_buffer(buf.as_bytes()).unwrap(); let diff = Diff::try_from(diff).unwrap(); assert_eq!( - diff.modified().next().unwrap().eof, + diff.modified().next().unwrap().diff.eof(), Some(EofNewLine::BothMissing) ); } @@ -333,7 +335,10 @@ index f89e4c0..7c56eb7 100644 "#; let diff = git2::Diff::from_buffer(buf.as_bytes()).unwrap(); let diff = Diff::try_from(diff).unwrap(); - assert_eq!(diff.modified().next().unwrap().eof, None); + assert_eq!( + diff.modified().next().unwrap().diff.eof(), + Some(EofNewLine::NoneMissing) + ); } // TODO(xphoniex): uncomment once libgit2 has fixed the bug