diff --git a/Cargo.toml b/Cargo.toml index e578989f9..ab16e913e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -214,6 +214,7 @@ redundant_closure_for_method_calls = "allow" redundant_pub_crate = "allow" ref_patterns = "allow" self_named_module_files = "allow" +semicolon_outside_block = "allow" single_call_fn = "allow" std_instead_of_alloc = "allow" std_instead_of_core = "allow" diff --git a/src/application.rs b/src/application.rs index 66be018b1..7791b7d0c 100644 --- a/src/application.rs +++ b/src/application.rs @@ -9,9 +9,10 @@ pub(crate) use crate::application::app_data::AppData; use crate::{ Args, Exit, - config::{Config, ConfigLoader}, + config::{Config, ConfigLoader, DiffIgnoreWhitespaceSetting}, + diff::{self, CommitDiffLoader, CommitDiffLoaderOptions}, display::Display, - git::{Repository, open_repository_from_env}, + git::open_repository_from_env, help::build_help, input::{Event, EventHandler, EventReaderFn, KeyBindings, StandardEvent}, module::{self, ExitStatus, ModuleHandler, State}, @@ -73,20 +74,35 @@ where ModuleProvider: module::ModuleProvider + Send + 'static let search_state = search_threads.state(); threads.push(Box::new(search_threads)); + let commit_diff_loader_options = CommitDiffLoaderOptions::new() + .context_lines(config.git.diff_context) + .copies(config.git.diff_copies) + .ignore_whitespace(config.diff_ignore_whitespace == DiffIgnoreWhitespaceSetting::All) + .ignore_whitespace_change(config.diff_ignore_whitespace == DiffIgnoreWhitespaceSetting::Change) + .ignore_blank_lines(config.diff_ignore_blank_lines) + .interhunk_context(config.git.diff_interhunk_lines) + .renames(config.git.diff_renames, config.git.diff_rename_limit); + let commit_diff_loader = CommitDiffLoader::new(config_loader.eject_repository(), commit_diff_loader_options); + + let diff_update_handler = Self::create_diff_update_handler(input_state.clone()); + let diff_thread = diff::thread::Thread::new(commit_diff_loader, diff_update_handler); + let diff_state = diff_thread.state(); + threads.push(Box::new(diff_thread)); + let keybindings = KeyBindings::new(&config.key_bindings); + let app_data = AppData::new( config, State::WindowSizeError, Arc::clone(&todo_file), + diff_state.clone(), view_state.clone(), input_state.clone(), search_state.clone(), ); - let module_handler = ModuleHandler::new( - EventHandler::new(keybindings), - ModuleProvider::new(Repository::from(config_loader.eject_repository()), &app_data), - ); + let module_handler = ModuleHandler::new(EventHandler::new(keybindings), ModuleProvider::new(&app_data)); + let process = Process::new(&app_data, initial_display_size, module_handler, thread_statuses.clone()); let process_threads = process::Thread::new(process.clone()); threads.push(Box::new(process_threads)); @@ -184,6 +200,10 @@ where ModuleProvider: module::ModuleProvider + Send + 'static fn create_search_update_handler(input_state: crate::input::State) -> impl Fn() + Send + Sync { move || input_state.push_event(Event::Standard(StandardEvent::SearchUpdate)) } + + fn create_diff_update_handler(input_state: crate::input::State) -> impl Fn() + Send + Sync { + move || input_state.push_event(Event::Standard(StandardEvent::DiffUpdate)) + } } #[cfg(all(unix, test))] diff --git a/src/application/app_data.rs b/src/application/app_data.rs index d2239811c..97c35c786 100644 --- a/src/application/app_data.rs +++ b/src/application/app_data.rs @@ -2,13 +2,14 @@ use std::sync::Arc; use parking_lot::Mutex; -use crate::{config::Config, input, module, search, todo_file::TodoFile, view}; +use crate::{config::Config, diff, input, module, search, todo_file::TodoFile, view}; #[derive(Clone, Debug)] pub(crate) struct AppData { config: Arc, active_module: Arc>, todo_file: Arc>, + diff_state: diff::thread::State, view_state: view::State, input_state: input::State, search_state: search::State, @@ -19,6 +20,7 @@ impl AppData { config: Config, active_module: module::State, todo_file: Arc>, + diff_state: diff::thread::State, view_state: view::State, input_state: input::State, search_state: search::State, @@ -27,6 +29,7 @@ impl AppData { config: Arc::new(config), active_module: Arc::new(Mutex::new(active_module)), todo_file, + diff_state, view_state, input_state, search_state, @@ -45,6 +48,10 @@ impl AppData { Arc::clone(&self.todo_file) } + pub(crate) fn diff_state(&self) -> diff::thread::State { + self.diff_state.clone() + } + pub(crate) fn view_state(&self) -> view::State { self.view_state.clone() } diff --git a/src/config.rs b/src/config.rs index c93b36117..327485f90 100644 --- a/src/config.rs +++ b/src/config.rs @@ -97,7 +97,7 @@ impl Config { impl TryFrom<&ConfigLoader> for Config { type Error = ConfigError; - /// Creates a new Config instance loading the Git Config using [`crate::git::Repository`]. + /// Creates a new Config instance loading the Git Config. /// /// # Errors /// diff --git a/src/diff.rs b/src/diff.rs index 578384528..d1b86918c 100644 --- a/src/diff.rs +++ b/src/diff.rs @@ -13,6 +13,8 @@ mod reference_kind; mod status; mod user; +pub(crate) mod thread; + pub(crate) use self::{ commit::Commit, commit_diff::CommitDiff, diff --git a/src/diff/commit.rs b/src/diff/commit.rs index 63f4c9b05..27b910ac1 100644 --- a/src/diff/commit.rs +++ b/src/diff/commit.rs @@ -1,3 +1,5 @@ +use std::time::UNIX_EPOCH; + use chrono::{DateTime, Local, TimeZone as _}; use crate::{ @@ -6,7 +8,7 @@ use crate::{ }; /// Represents a commit. -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct Commit { pub(crate) hash: String, pub(crate) reference: Option, @@ -19,6 +21,20 @@ pub(crate) struct Commit { } impl Commit { + #[must_use] + pub(crate) fn empty() -> Self { + Self { + hash: String::from("0000000000000000000000000000000000000000"), + reference: None, + author: User::new(None, None), + authored_date: None, + message: None, + committer: None, + committed_date: DateTime::from(UNIX_EPOCH), + summary: None, + } + } + /// Get the hash of the commit #[must_use] pub(crate) fn hash(&self) -> &str { @@ -131,6 +147,34 @@ mod tests { with_temp_repository, }; + impl Commit { + pub(crate) fn new_with_hash(hash: &str) -> Self { + Self { + hash: String::from(hash), + reference: None, + author: User::new(None, None), + authored_date: None, + message: None, + committer: None, + committed_date: DateTime::from(UNIX_EPOCH), + summary: None, + } + } + } + + #[test] + fn empty() { + let commit = Commit::empty(); + assert_eq!(commit.hash(), "0000000000000000000000000000000000000000"); + assert_none!(commit.reference()); + assert_eq!(commit.author(), &User::new(None, None)); + assert_none!(commit.authored_date()); + assert_none!(commit.message()); + assert_none!(commit.committer()); + assert_eq!(commit.committed_date().timestamp(), 0); + assert_none!(commit.summary()); + } + #[test] fn hash() { let commit = CommitBuilder::new("0123456789ABCDEF").build(); @@ -180,9 +224,10 @@ mod tests { #[test] fn new_authored_date_same_committed_date() { with_temp_repository(|repository| { - let repo = crate::git::Repository::from(repository); - create_commit(&repo, Some(CreateCommitOptions::new().author_time(JAN_2021_EPOCH))); - let commit = repo.find_commit("refs/heads/main").unwrap(); + let commit = create_commit( + &repository, + Some(CreateCommitOptions::new().author_time(JAN_2021_EPOCH)), + ); assert_none!(commit.authored_date()); }); } @@ -190,16 +235,14 @@ mod tests { #[test] fn new_authored_date_different_than_committed() { with_temp_repository(|repository| { - let repo = crate::git::Repository::from(repository); - create_commit( - &repo, + let commit = create_commit( + &repository, Some( CreateCommitOptions::new() .commit_time(JAN_2021_EPOCH) .author_time(JAN_2021_EPOCH + 1), ), ); - let commit = repo.find_commit("refs/heads/main").unwrap(); assert_some_eq!( commit.authored_date(), &DateTime::parse_from_rfc3339("2021-01-01T00:00:01Z").unwrap() @@ -210,9 +253,7 @@ mod tests { #[test] fn new_committer_different_than_author() { with_temp_repository(|repository| { - let repo = crate::git::Repository::from(repository); - create_commit(&repo, Some(CreateCommitOptions::new().committer("Committer"))); - let commit = repo.find_commit("refs/heads/main").unwrap(); + let commit = create_commit(&repository, Some(CreateCommitOptions::new().committer("Committer"))); assert_some_eq!( commit.committer(), &User::new(Some("Committer"), Some("committer@example.com")) @@ -223,8 +264,7 @@ mod tests { #[test] fn new_committer_same_as_author() { with_temp_repository(|repository| { - let repo = crate::git::Repository::from(repository); - let commit = repo.find_commit("refs/heads/main").unwrap(); + let commit = create_commit(&repository, None); assert_none!(commit.committer()); }); } diff --git a/src/diff/commit_diff.rs b/src/diff/commit_diff.rs index dc20ed2d4..f51c84d67 100644 --- a/src/diff/commit_diff.rs +++ b/src/diff/commit_diff.rs @@ -1,7 +1,7 @@ use crate::diff::{Commit, FileStatus}; /// Represents a commit with a diff -#[derive(Debug)] +#[derive(Debug, Clone)] pub(crate) struct CommitDiff { commit: Commit, parent: Option, @@ -12,21 +12,14 @@ pub(crate) struct CommitDiff { } impl CommitDiff { - pub(crate) fn new( - commit: Commit, - parent: Option, - file_statuses: Vec, - number_files_changed: usize, - number_insertions: usize, - number_deletions: usize, - ) -> Self { + pub(crate) fn new() -> Self { CommitDiff { - commit, - parent, - file_statuses, - number_files_changed, - number_insertions, - number_deletions, + commit: Commit::empty(), + parent: None, + file_statuses: vec![], + number_files_changed: 0, + number_insertions: 0, + number_deletions: 0, } } @@ -66,73 +59,112 @@ impl CommitDiff { pub(crate) const fn number_deletions(&self) -> usize { self.number_deletions } + + /// Update the details of the diff + pub(crate) fn update( + &mut self, + file_statuses: Vec, + number_files_changed: usize, + number_insertions: usize, + number_deletions: usize, + ) { + self.file_statuses = file_statuses; + self.number_files_changed = number_files_changed; + self.number_insertions = number_insertions; + self.number_deletions = number_deletions; + } + + /// Reset the diff back to an empty state + pub(crate) fn reset(&mut self, commit: Commit, parent: Option) { + self.commit = commit; + self.parent = parent; + self.file_statuses.clear(); + self.number_files_changed = 0; + self.number_insertions = 0; + self.number_deletions = 0; + } + + pub(crate) fn clear(&mut self) { + self.commit = Commit::empty(); + self.parent = None; + self.file_statuses.clear(); + self.number_files_changed = 0; + self.number_insertions = 0; + self.number_deletions = 0; + } } #[cfg(test)] mod tests { - use claims::assert_some_eq; + use claims::{assert_none, assert_some_eq}; use crate::{ - diff::{Delta, DiffLine, FileMode, FileStatus, FileStatusBuilder, Origin, Status}, - test_helpers::builders::{CommitBuilder, CommitDiffBuilder}, + assert_empty, + assert_not_empty, + diff::{Commit, CommitDiff, FileMode, FileStatus, Status}, }; #[test] - fn commit() { - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789ABCDEF").build()).build(); - assert_eq!(diff.commit(), &CommitBuilder::new("0123456789ABCDEF").build()); - } - - #[test] - fn parent() { - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789ABCDEF").build()) - .parent(CommitBuilder::new("ABCDEF0123456789").build()) - .build(); - assert_some_eq!(diff.parent(), &CommitBuilder::new("ABCDEF0123456789").build()); - } - - #[test] - fn file_statuses() { - let mut builder = FileStatusBuilder::new(); - builder.add_file_stat(FileStatus::new( - "foo", - FileMode::Normal, - false, - "foo", - FileMode::Normal, - false, - Status::Modified, - )); - builder.add_delta(Delta::new("name", 0, 0, 0, 1)); - builder.add_diff_line(DiffLine::new(Origin::Addition, "line", None, Some(1), false)); - let file_statuses = builder.build(); - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789ABCDEF").build()) - .file_statuses(file_statuses) - .build(); - assert_eq!(diff.file_statuses()[0].source_path().to_string_lossy(), "foo"); - } - - #[test] - fn number_files_changed() { - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789ABCDEF").build()) - .number_files_changed(1) - .build(); - assert_eq!(diff.number_files_changed(), 1); + fn new() { + let diff = CommitDiff::new(); + assert_eq!(diff.commit(), &Commit::empty()); + assert_none!(diff.parent()); + assert_empty!(diff.file_statuses()); + assert_eq!(diff.number_files_changed(), 0); + assert_eq!(diff.number_insertions(), 0); + assert_eq!(diff.number_deletions(), 0); } #[test] - fn number_insertions() { - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789ABCDEF").build()) - .number_insertions(2) - .build(); - assert_eq!(diff.number_insertions(), 2); + fn reset_update() { + let mut diff = CommitDiff::new(); + diff.reset(Commit::new_with_hash("abc123"), Some(Commit::new_with_hash("def456"))); + diff.update( + vec![FileStatus::new( + "foo", + FileMode::Normal, + false, + "foo", + FileMode::Normal, + false, + Status::Modified, + )], + 11, + 22, + 33, + ); + assert_eq!(diff.commit().hash(), "abc123"); + assert_some_eq!(diff.parent().map(|d| d.hash()), "def456"); + assert_not_empty!(diff.file_statuses()); + assert_eq!(diff.number_files_changed(), 11); + assert_eq!(diff.number_insertions(), 22); + assert_eq!(diff.number_deletions(), 33); } #[test] - fn number_deletions() { - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789ABCDEF").build()) - .number_deletions(3) - .build(); - assert_eq!(diff.number_deletions(), 3); + fn reset_clear() { + let mut diff = CommitDiff::new(); + diff.reset(Commit::new_with_hash("abc123"), Some(Commit::new_with_hash("def456"))); + diff.update( + vec![FileStatus::new( + "foo", + FileMode::Normal, + false, + "foo", + FileMode::Normal, + false, + Status::Modified, + )], + 11, + 22, + 33, + ); + diff.clear(); + assert_eq!(diff.commit(), &Commit::empty()); + assert_none!(diff.parent()); + assert_empty!(diff.file_statuses()); + assert_eq!(diff.number_files_changed(), 0); + assert_eq!(diff.number_insertions(), 0); + assert_eq!(diff.number_deletions(), 0); } } diff --git a/src/diff/commit_diff_loader.rs b/src/diff/commit_diff_loader.rs index 74c7b6348..8da541611 100644 --- a/src/diff/commit_diff_loader.rs +++ b/src/diff/commit_diff_loader.rs @@ -1,65 +1,139 @@ -use std::{path::PathBuf, sync::LazyLock}; - -use git2::{DiffFindOptions, DiffOptions, Oid, Repository}; -use parking_lot::Mutex; - -use crate::diff::{ - Commit, - CommitDiff, - CommitDiffLoaderOptions, - Delta, - DiffLine, - FileMode, - FileStatus, - FileStatusBuilder, - Status, +use std::{ + fmt::{Debug, Formatter}, + path::PathBuf, + sync::{Arc, LazyLock}, + time::{Duration, Instant}, +}; + +use git2::{Diff, DiffFindOptions, DiffOptions, Repository}; +use parking_lot::{Mutex, RwLock}; + +use crate::{ + diff::{ + Commit, + CommitDiff, + CommitDiffLoaderOptions, + Delta, + DiffLine, + FileMode, + FileStatus, + FileStatusBuilder, + Status, + thread::LoadStatus, + }, + git::GitError, }; static UNKNOWN_PATH: LazyLock = LazyLock::new(|| PathBuf::from("unknown")); -pub(crate) struct CommitDiffLoader<'options, 'repo> { - config: &'options CommitDiffLoaderOptions, - repository: &'repo Repository, +pub(crate) trait DiffUpdateHandlerFn: Fn(LoadStatus) -> bool + Sync + Send {} + +impl bool + Sync + Send> DiffUpdateHandlerFn for FN {} + +fn create_status_update(quick: bool, processed_files: usize, total_files: usize) -> LoadStatus { + if quick { + LoadStatus::QuickDiff(processed_files, total_files) + } + else { + LoadStatus::Diff(processed_files, total_files) + } +} + +pub(crate) struct CommitDiffLoader { + config: CommitDiffLoaderOptions, + repository: Repository, + commit_diff: Arc>, } -impl<'options, 'repo> CommitDiffLoader<'options, 'repo> { - pub(crate) const fn new(repository: &'repo Repository, config: &'options CommitDiffLoaderOptions) -> Self { - Self { config, repository } +impl CommitDiffLoader { + pub(crate) fn new(repository: Repository, config: CommitDiffLoaderOptions) -> Self { + Self { + repository, + config, + commit_diff: Arc::new(RwLock::new(CommitDiff::new())), + } } - pub(crate) fn load_from_hash(&self, oid: Oid) -> Result { - let commit = self.repository.find_commit(oid)?; - // only the first parent matter for the diff, the second parent, if it exists, was only used - // for conflict resolution - let parent = commit.parents().next(); + pub(crate) fn reset(&mut self) { + self.commit_diff.write().clear(); + } - self.load_diff(parent, &commit) + pub(crate) fn commit_diff(&self) -> Arc> { + Arc::clone(&self.commit_diff) } - #[expect(clippy::as_conversions, reason = "Mostly safe difference between APIs.")] - #[expect(clippy::unwrap_in_result, reason = "Unwrap usage failure considered a bug.")] - fn load_diff( - &self, - parent: Option>, + fn diff<'repo>( + repository: &'repo Repository, + config: &CommitDiffLoaderOptions, commit: &git2::Commit<'_>, - ) -> Result { - let parent_commit = parent.as_ref().map(Commit::from); - - let mut diff_options = DiffOptions::new(); - // include_unmodified added to find copies from unmodified files + diff_options: &mut DiffOptions, + ) -> Result, GitError> { _ = diff_options - .context_lines(self.config.context_lines) + .context_lines(config.context_lines) .ignore_filemode(false) - .ignore_whitespace(self.config.ignore_whitespace) - .ignore_whitespace_change(self.config.ignore_whitespace_change) - .ignore_blank_lines(self.config.ignore_blank_lines) + .ignore_whitespace(config.ignore_whitespace) + .ignore_whitespace_change(config.ignore_whitespace_change) + .ignore_blank_lines(config.ignore_blank_lines) .include_typechange(true) .include_typechange_trees(true) - .include_unmodified(self.config.copies) .indent_heuristic(true) - .interhunk_lines(self.config.interhunk_context) + .interhunk_lines(config.interhunk_context) .minimal(true); + let commit_tree = commit.tree().map_err(|e| GitError::DiffLoad { cause: e })?; + + if let Some(p) = commit.parents().next() { + let parent_tree = p.tree().map_err(|e| GitError::DiffLoad { cause: e })?; + repository.diff_tree_to_tree(Some(&parent_tree), Some(&commit_tree), Some(diff_options)) + } + else { + repository.diff_tree_to_tree(None, Some(&commit_tree), Some(diff_options)) + } + .map_err(|e| GitError::DiffLoad { cause: e }) + } + + pub(crate) fn load_diff(&mut self, hash: &str, update_notifier: impl DiffUpdateHandlerFn) -> Result<(), GitError> { + let oid = self + .repository + .revparse_single(hash) + .map_err(|e| GitError::DiffLoad { cause: e })? + .id(); + let commit = self + .repository + .find_commit(oid) + .map_err(|e| GitError::DiffLoad { cause: e })?; + + { + // only the first parent matter for things like diffs, the second parent, if it exists, + // is only used for conflict resolution, and has no use + let parent = commit.parents().next().map(|c| Commit::from(&c)); + let mut commit_diff = self.commit_diff.write(); + commit_diff.reset(Commit::from(&commit), parent); + if update_notifier(LoadStatus::New) { + return Ok(()); + } + } + + // when a diff contains a lot of untracked files, collecting the diff information can take + // upwards of a minute. This performs a quicker diff, that does not detect copies and + // renames against unmodified files. + if self.config.copies { + let should_continue = self.collect( + &Self::diff(&self.repository, &self.config, &commit, &mut DiffOptions::new())?, + &update_notifier, + true, + )?; + + if !should_continue || update_notifier(LoadStatus::CompleteQuickDiff) { + return Ok(()); + } + } + + let mut diff_options = DiffOptions::new(); + // include_unmodified added to find copies from unmodified files + _ = diff_options.include_unmodified(self.config.copies); + let mut diff = Self::diff(&self.repository, &self.config, &commit, &mut diff_options)?; + let mut diff_find_options = DiffFindOptions::new(); _ = diff_find_options .rename_limit(self.config.rename_limit as usize) @@ -69,31 +143,65 @@ impl<'options, 'repo> CommitDiffLoader<'options, 'repo> { .copies(self.config.copies) .copies_from_unmodified(self.config.copies); - let mut diff = if let Some(p) = parent { - self.repository - .diff_tree_to_tree(Some(&p.tree()?), Some(&commit.tree()?), Some(&mut diff_options))? - } - else { - self.repository - .diff_tree_to_tree(None, Some(&commit.tree()?), Some(&mut diff_options))? - }; + diff.find_similar(Some(&mut diff_find_options)) + .map_err(|e| GitError::DiffLoad { cause: e })?; + let should_continue = self.collect(&diff, &update_notifier, false)?; - diff.find_similar(Some(&mut diff_find_options))?; + if should_continue { + _ = update_notifier(LoadStatus::DiffComplete); + return Ok(()); + } + Ok(()) + } + pub(crate) fn collect( + &self, + diff: &Diff<'_>, + update_handler: &impl DiffUpdateHandlerFn, + quick: bool, + ) -> Result { + let file_stats_builder = Mutex::new(FileStatusBuilder::new()); let mut unmodified_file_count: usize = 0; + let mut change_count: usize = 0; - let file_stats_builder = Mutex::new(FileStatusBuilder::new()); + let stats = diff.stats().map_err(|e| GitError::DiffLoad { cause: e })?; + let total_files_changed = stats.files_changed(); - diff.foreach( + if update_handler(create_status_update(quick, 0, total_files_changed)) { + return Ok(false); + } + let mut time = Instant::now(); + + let collect_result = diff.foreach( &mut |diff_delta, _| { + change_count += 1; + + #[cfg(test)] + { + // this is needed to test timing in tests, the other option would be to mock + // Instant, but that's more effort than is worth the value. + // Since this adds 10ms of delay for each delta, each file added to the diff + // will add ~10ms of delay. + // + // this may be flaky, due to the Diff progress being based on time. However, + // the added thread sleep during tests should make the diff progress very + // stable, as the diff processing can process the files much faster than a + // fraction of a millisecond. + std::thread::sleep(Duration::from_millis(10)); + } + if time.elapsed() > Duration::from_millis(25) { + if update_handler(create_status_update(quick, change_count, total_files_changed)) { + return false; + } + time = Instant::now(); + } + // unmodified files are included for copy detection, so ignore if diff_delta.status() == git2::Delta::Unmodified { unmodified_file_count += 1; return true; } - let mut fsb = file_stats_builder.lock(); - let source_file = diff_delta.old_file(); let source_file_mode = FileMode::from(source_file.mode()); let source_file_path = source_file.path().unwrap_or(UNKNOWN_PATH.as_path()); @@ -102,6 +210,7 @@ impl<'options, 'repo> CommitDiffLoader<'options, 'repo> { let destination_file_mode = FileMode::from(destination_file.mode()); let destination_file_path = destination_file.path().unwrap_or(UNKNOWN_PATH.as_path()); + let mut fsb = file_stats_builder.lock(); fsb.add_file_stat(FileStatus::new( source_file_path, source_file_mode, @@ -125,24 +234,33 @@ impl<'options, 'repo> CommitDiffLoader<'options, 'repo> { fsb.add_diff_line(DiffLine::from(&diff_line)); true }), - ) - .expect("diff.foreach failed. Please report this as a bug."); + ); + + // error caused by early return + if collect_result.is_err() { + return Ok(false); + } - let stats = diff.stats()?; - let number_files_changed = stats.files_changed() - unmodified_file_count; + let mut commit_diff = self.commit_diff.write(); + + let number_files_changed = total_files_changed - unmodified_file_count; let number_insertions = stats.insertions(); let number_deletions = stats.deletions(); let fsb = file_stats_builder.into_inner(); + commit_diff.update(fsb.build(), number_files_changed, number_insertions, number_deletions); + Ok(true) + } +} - Ok(CommitDiff::new( - Commit::from(commit), - parent_commit, - fsb.build(), - number_files_changed, - number_insertions, - number_deletions, - )) +impl Debug for CommitDiffLoader { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + f.debug_struct("CommitDiffLoader") + .field( + "repository", + &format!("Repository({})", &self.repository.path().display()), + ) + .finish_non_exhaustive() } } @@ -154,9 +272,18 @@ mod tests { os::unix::fs::symlink, }; + use git2::Index; + use super::*; use crate::{diff::Origin, test_helpers::with_temp_repository}; + impl CommitDiffLoader { + fn take_diff(mut self) -> CommitDiff { + let diff = std::mem::replace(&mut self.commit_diff, Arc::new(RwLock::new(CommitDiff::new()))); + Arc::try_unwrap(diff).unwrap().into_inner() + } + } + #[cfg(not(tarpaulin_include))] fn _format_status(status: &FileStatus) -> String { let s = match status.status() { @@ -270,49 +397,84 @@ mod tests { } #[cfg(not(tarpaulin_include))] - fn write_normal_file(repository: &crate::git::Repository, name: &str, contents: &[&str]) { - let root = repository.repo_path().parent().unwrap().to_path_buf(); + fn index(repository: &Repository) -> Index { + repository.index().unwrap() + } + + #[cfg(not(tarpaulin_include))] + fn root_path(repository: &Repository) -> PathBuf { + repository.path().to_path_buf().parent().unwrap().to_path_buf() + } + + #[cfg(not(tarpaulin_include))] + fn commit_from_ref<'repo>(repository: &'repo Repository, reference: &str) -> git2::Commit<'repo> { + repository.find_reference(reference).unwrap().peel_to_commit().unwrap() + } + + #[cfg(not(tarpaulin_include))] + fn add_path(repository: &Repository, name: &str) { + index(repository).add_path(PathBuf::from(name).as_path()).unwrap(); + } - let file_path = root.join(name); + #[cfg(not(tarpaulin_include))] + fn write_normal_file(repository: &Repository, name: &str, contents: &[&str]) { + let file_path = root_path(repository).join(name); let mut file = File::create(file_path.as_path()).unwrap(); if !contents.is_empty() { writeln!(file, "{}", contents.join("\n")).unwrap(); } - repository.add_path_to_index(PathBuf::from(name).as_path()).unwrap(); + + index(repository).add_path(PathBuf::from(name).as_path()).unwrap(); } #[cfg(not(tarpaulin_include))] - fn remove_path(repository: &crate::git::Repository, name: &str) { - let root = repository.repo_path().parent().unwrap().to_path_buf(); + fn remove_path(repository: &Repository, name: &str) { + let file_path = root_path(repository).join(name); + _ = remove_file(file_path.as_path()); - let file_path = root.join(name); - _ = remove_file(file_path); - - repository - .remove_path_from_index(PathBuf::from(name).as_path()) - .unwrap(); + index(repository).remove_path(PathBuf::from(name).as_path()).unwrap(); } #[cfg(not(tarpaulin_include))] - fn create_commit(repository: &crate::git::Repository) { + fn create_commit(repository: &Repository) { let sig = git2::Signature::new("name", "name@example.com", &git2::Time::new(1_609_459_200, 0)).unwrap(); - repository - .create_commit_on_index("refs/heads/main", &sig, &sig, "title") + let tree = repository.find_tree(index(repository).write_tree().unwrap()).unwrap(); + let head = commit_from_ref(repository, "refs/heads/main"); + _ = repository + .commit(Some("HEAD"), &sig, &sig, "title", &tree, &[&head]) .unwrap(); } #[cfg(not(tarpaulin_include))] - fn diff_from_head(repository: &crate::git::Repository, options: &CommitDiffLoaderOptions) -> CommitDiff { - let id = repository.commit_id_from_ref("refs/heads/main").unwrap(); - let loader = CommitDiffLoader::new(repository.repository(), options); - loader.load_from_hash(id).unwrap() + fn diff_from_head(repository: Repository, options: CommitDiffLoaderOptions) -> Result { + let commit = commit_from_ref(&repository, "refs/heads/main"); + let hash = commit.id().to_string(); + drop(commit); + let mut loader = CommitDiffLoader::new(repository, options); + loader.load_diff(hash.as_str(), |_| false)?; + Ok(loader) + } + + #[cfg(not(tarpaulin_include))] + fn diff_with_notifier( + repository: Repository, + options: CommitDiffLoaderOptions, + update_notifier: impl DiffUpdateHandlerFn, + ) -> Result { + let commit = commit_from_ref(&repository, "refs/heads/main"); + let hash = commit.id().to_string(); + drop(commit); + let mut loader = CommitDiffLoader::new(repository, options); + loader.load_diff(hash.as_str(), update_notifier)?; + Ok(loader) } #[test] fn load_from_hash_commit_no_parents() { with_temp_repository(|repository| { - let repo = crate::git::Repository::from(repository); - let diff = diff_from_head(&repo, &CommitDiffLoaderOptions::new()); + let loader = diff_from_head(repository, CommitDiffLoaderOptions::new()).unwrap(); + let diff = loader.take_diff(); + assert_eq!(diff.number_files_changed(), 0); assert_eq!(diff.number_insertions(), 0); assert_eq!(diff.number_deletions(), 0); @@ -322,10 +484,11 @@ mod tests { #[test] fn load_from_hash_added_file() { with_temp_repository(|repository| { - let repo = crate::git::Repository::from(repository); - write_normal_file(&repo, "a", &["line1"]); - create_commit(&repo); - let diff = diff_from_head(&repo, &CommitDiffLoaderOptions::new()); + write_normal_file(&repository, "a", &["line1"]); + create_commit(&repository); + let loader = diff_from_head(repository, CommitDiffLoaderOptions::new()).unwrap(); + let diff = loader.take_diff(); + assert_eq!(diff.number_files_changed(), 1); assert_eq!(diff.number_insertions(), 1); assert_eq!(diff.number_deletions(), 0); @@ -336,12 +499,14 @@ mod tests { #[test] fn load_from_hash_removed_file() { with_temp_repository(|repository| { - let repo = crate::git::Repository::from(repository); - write_normal_file(&repo, "a", &["line1"]); - create_commit(&repo); - remove_path(&repo, "a"); - create_commit(&repo); - let diff = diff_from_head(&repo, &CommitDiffLoaderOptions::new()); + write_normal_file(&repository, "a", &["line1"]); + create_commit(&repository); + remove_path(&repository, "a"); + create_commit(&repository); + + let loader = diff_from_head(repository, CommitDiffLoaderOptions::new()).unwrap(); + let diff = loader.take_diff(); + assert_eq!(diff.number_files_changed(), 1); assert_eq!(diff.number_insertions(), 0); assert_eq!(diff.number_deletions(), 1); @@ -358,12 +523,14 @@ mod tests { #[test] fn load_from_hash_modified_file() { with_temp_repository(|repository| { - let repo = crate::git::Repository::from(repository); - write_normal_file(&repo, "a", &["line1"]); - create_commit(&repo); - write_normal_file(&repo, "a", &["line2"]); - create_commit(&repo); - let diff = diff_from_head(&repo, &CommitDiffLoaderOptions::new()); + write_normal_file(&repository, "a", &["line1"]); + create_commit(&repository); + write_normal_file(&repository, "a", &["line2"]); + create_commit(&repository); + + let loader = diff_from_head(repository, CommitDiffLoaderOptions::new()).unwrap(); + let diff = loader.take_diff(); + assert_eq!(diff.number_files_changed(), 1); assert_eq!(diff.number_insertions(), 1); assert_eq!(diff.number_deletions(), 1); @@ -381,12 +548,23 @@ mod tests { #[test] fn load_from_hash_with_context() { with_temp_repository(|repository| { - let repo = crate::git::Repository::from(repository); - write_normal_file(&repo, "a", &["line0", "line1", "line2", "line3", "line4", "line5"]); - create_commit(&repo); - write_normal_file(&repo, "a", &["line0", "line1", "line2", "line3-m", "line4", "line5"]); - create_commit(&repo); - let diff = diff_from_head(&repo, &CommitDiffLoaderOptions::new().context_lines(2)); + write_normal_file(&repository, "a", &[ + "line0", "line1", "line2", "line3", "line4", "line5", + ]); + create_commit(&repository); + write_normal_file(&repository, "a", &[ + "line0", + "line1", + "line2", + "line3-m", + "line4", + "line5", + ]); + create_commit(&repository); + + let loader = diff_from_head(repository, CommitDiffLoaderOptions::new().context_lines(2)).unwrap(); + let diff = loader.take_diff(); + assert_commit_diff!( &diff, "a (n)", @@ -405,12 +583,18 @@ mod tests { #[test] fn load_from_hash_ignore_white_space_change() { with_temp_repository(|repository| { - let repo = crate::git::Repository::from(repository); - write_normal_file(&repo, "a", &[" line0", "line1"]); - create_commit(&repo); - write_normal_file(&repo, "a", &[" line0", " line1-m"]); - create_commit(&repo); - let diff = diff_from_head(&repo, &CommitDiffLoaderOptions::new().ignore_whitespace_change(true)); + write_normal_file(&repository, "a", &[" line0", "line1"]); + create_commit(&repository); + write_normal_file(&repository, "a", &[" line0", " line1-m"]); + create_commit(&repository); + + let loader = diff_from_head( + repository, + CommitDiffLoaderOptions::new().ignore_whitespace_change(true), + ) + .unwrap(); + let diff = loader.take_diff(); + assert_commit_diff!( &diff, "a (n)", @@ -425,12 +609,14 @@ mod tests { #[test] fn load_from_hash_ignore_white_space() { with_temp_repository(|repository| { - let repo = crate::git::Repository::from(repository); - write_normal_file(&repo, "a", &["line0", "line1"]); - create_commit(&repo); - write_normal_file(&repo, "a", &[" line0", " line1-m"]); - create_commit(&repo); - let diff = diff_from_head(&repo, &CommitDiffLoaderOptions::new().ignore_whitespace(true)); + write_normal_file(&repository, "a", &["line0", "line1"]); + create_commit(&repository); + write_normal_file(&repository, "a", &[" line0", " line1-m"]); + create_commit(&repository); + + let loader = diff_from_head(repository, CommitDiffLoaderOptions::new().ignore_whitespace(true)).unwrap(); + let diff = loader.take_diff(); + assert_commit_diff!( &diff, "a (n)", @@ -445,12 +631,14 @@ mod tests { #[test] fn load_from_hash_copies() { with_temp_repository(|repository| { - let repo = crate::git::Repository::from(repository); - write_normal_file(&repo, "a", &["line0"]); - create_commit(&repo); - write_normal_file(&repo, "b", &["line0"]); - create_commit(&repo); - let diff = diff_from_head(&repo, &CommitDiffLoaderOptions::new().copies(true)); + write_normal_file(&repository, "a", &["line0"]); + create_commit(&repository); + write_normal_file(&repository, "b", &["line0"]); + create_commit(&repository); + + let loader = diff_from_head(repository, CommitDiffLoaderOptions::new().copies(true)).unwrap(); + let diff = loader.take_diff(); + assert_eq!(diff.number_files_changed(), 1); assert_eq!(diff.number_insertions(), 0); assert_eq!(diff.number_deletions(), 0); @@ -461,13 +649,15 @@ mod tests { #[test] fn load_from_hash_copies_modified_source() { with_temp_repository(|repository| { - let repo = crate::git::Repository::from(repository); - write_normal_file(&repo, "a", &["line0"]); - create_commit(&repo); - write_normal_file(&repo, "a", &["line0", "a"]); - write_normal_file(&repo, "b", &["line0"]); - create_commit(&repo); - let diff = diff_from_head(&repo, &CommitDiffLoaderOptions::new().copies(true)); + write_normal_file(&repository, "a", &["line0"]); + create_commit(&repository); + write_normal_file(&repository, "a", &["line0", "a"]); + write_normal_file(&repository, "b", &["line0"]); + create_commit(&repository); + + let loader = diff_from_head(repository, CommitDiffLoaderOptions::new().copies(true)).unwrap(); + let diff = loader.take_diff(); + assert_eq!(diff.number_files_changed(), 2); assert_eq!(diff.number_insertions(), 1); assert_eq!(diff.number_deletions(), 0); @@ -486,12 +676,23 @@ mod tests { #[test] fn load_from_hash_interhunk_context() { with_temp_repository(|repository| { - let repo = crate::git::Repository::from(repository); - write_normal_file(&repo, "a", &["line0", "line1", "line2", "line3", "line4", "line5"]); - create_commit(&repo); - write_normal_file(&repo, "a", &["line0", "line1-m", "line2", "line3", "line4-m", "line5"]); - create_commit(&repo); - let diff = diff_from_head(&repo, &CommitDiffLoaderOptions::new().interhunk_context(2)); + write_normal_file(&repository, "a", &[ + "line0", "line1", "line2", "line3", "line4", "line5", + ]); + create_commit(&repository); + write_normal_file(&repository, "a", &[ + "line0", + "line1-m", + "line2", + "line3", + "line4-m", + "line5", + ]); + create_commit(&repository); + + let loader = diff_from_head(repository, CommitDiffLoaderOptions::new().interhunk_context(2)).unwrap(); + let diff = loader.take_diff(); + assert_commit_diff!( &diff, "a (n)", @@ -510,13 +711,15 @@ mod tests { #[test] fn load_from_hash_rename_source_not_modified() { with_temp_repository(|repository| { - let repo = crate::git::Repository::from(repository); - write_normal_file(&repo, "a", &["line0"]); - create_commit(&repo); - remove_path(&repo, "a"); - write_normal_file(&repo, "b", &["line0"]); - create_commit(&repo); - let diff = diff_from_head(&repo, &CommitDiffLoaderOptions::new().renames(true, 100)); + write_normal_file(&repository, "a", &["line0"]); + create_commit(&repository); + remove_path(&repository, "a"); + write_normal_file(&repository, "b", &["line0"]); + create_commit(&repository); + + let loader = diff_from_head(repository, CommitDiffLoaderOptions::new().renames(true, 100)).unwrap(); + let diff = loader.take_diff(); + assert_eq!(diff.number_files_changed(), 1); assert_eq!(diff.number_insertions(), 0); assert_eq!(diff.number_deletions(), 0); @@ -533,13 +736,15 @@ mod tests { // this creates a situation where git detects the rename from the original unmodified // version of "a" before a new file called "a" was created with_temp_repository(|repository| { - let repo = crate::git::Repository::from(repository); - write_normal_file(&repo, "a", &["line0"]); - create_commit(&repo); - write_normal_file(&repo, "a", &["other0"]); - write_normal_file(&repo, "b", &["line0"]); - create_commit(&repo); - let diff = diff_from_head(&repo, &CommitDiffLoaderOptions::new().renames(true, 100)); + write_normal_file(&repository, "a", &["line0"]); + create_commit(&repository); + write_normal_file(&repository, "a", &["other0"]); + write_normal_file(&repository, "b", &["line0"]); + create_commit(&repository); + + let loader = diff_from_head(repository, CommitDiffLoaderOptions::new().renames(true, 100)).unwrap(); + let diff = loader.take_diff(); + assert_eq!(diff.number_files_changed(), 2); assert_eq!(diff.number_insertions(), 1); assert_eq!(diff.number_deletions(), 0); @@ -561,19 +766,21 @@ mod tests { with_temp_repository(|repository| { use std::os::unix::fs::PermissionsExt as _; - let repo = crate::git::Repository::from(repository); + let root = root_path(&repository); - let root = repo.repo_path().parent().unwrap().to_path_buf(); - - write_normal_file(&repo, "a", &["line0"]); - create_commit(&repo); + write_normal_file(&repository, "a", &["line0"]); + create_commit(&repository); let file = File::open(root.join("a")).unwrap(); let mut permissions = file.metadata().unwrap().permissions(); permissions.set_mode(0o755); file.set_permissions(permissions).unwrap(); - repo.add_path_to_index(PathBuf::from("a").as_path()).unwrap(); - create_commit(&repo); - let diff = diff_from_head(&repo, &CommitDiffLoaderOptions::new().renames(true, 100)); + + add_path(&repository, "a"); + create_commit(&repository); + + let loader = diff_from_head(repository, CommitDiffLoaderOptions::new().renames(true, 100)).unwrap(); + let diff = loader.take_diff(); + assert_eq!(diff.number_files_changed(), 1); assert_eq!(diff.number_insertions(), 0); assert_eq!(diff.number_deletions(), 0); @@ -585,18 +792,19 @@ mod tests { #[test] fn load_from_hash_type_changed() { with_temp_repository(|repository| { - let repo = crate::git::Repository::from(repository); - let root = repo.repo_path().parent().unwrap().to_path_buf(); - - write_normal_file(&repo, "a", &["line0"]); - write_normal_file(&repo, "b", &["line0"]); - create_commit(&repo); - remove_path(&repo, "a"); + write_normal_file(&repository, "a", &["line0"]); + write_normal_file(&repository, "b", &["line0"]); + create_commit(&repository); + remove_path(&repository, "a"); + let root = root_path(&repository); symlink(root.join("b"), root.join("a")).unwrap(); - repo.add_path_to_index(PathBuf::from("a").as_path()).unwrap(); - repo.add_path_to_index(PathBuf::from("b").as_path()).unwrap(); - create_commit(&repo); - let diff = diff_from_head(&repo, &CommitDiffLoaderOptions::new()); + add_path(&repository, "a"); + add_path(&repository, "b"); + create_commit(&repository); + + let loader = diff_from_head(repository, CommitDiffLoaderOptions::new()).unwrap(); + let diff = loader.take_diff(); + assert_eq!(diff.number_files_changed(), 1); assert_eq!(diff.number_insertions(), 0); assert_eq!(diff.number_deletions(), 0); @@ -607,35 +815,242 @@ mod tests { #[test] fn load_from_hash_binary_added_file() { with_temp_repository(|repository| { - let repo = crate::git::Repository::from(repository); - // treat all files as binary - write_normal_file(&repo, ".gitattributes", &["a binary"]); - create_commit(&repo); - write_normal_file(&repo, "a", &["line1"]); - create_commit(&repo); - let diff = diff_from_head(&repo, &CommitDiffLoaderOptions::new()); + write_normal_file(&repository, "a", &["line1"]); + create_commit(&repository); + + let loader = diff_from_head(repository, CommitDiffLoaderOptions::new()).unwrap(); + let diff = loader.take_diff(); + assert_eq!(diff.number_files_changed(), 1); - assert_eq!(diff.number_insertions(), 0); + assert_eq!(diff.number_insertions(), 1); assert_eq!(diff.number_deletions(), 0); - assert_commit_diff!(&diff, "a (o,b) > a (n,b)", "Status Added"); + assert_commit_diff!(&diff, "a (o) > a (n)", "Status Added", "@@ -0,0 +1,1 @@", "+ 1| line1"); }); } #[test] fn load_from_hash_binary_modified_file() { with_temp_repository(|repository| { - let repo = crate::git::Repository::from(repository); // treat all files as binary - write_normal_file(&repo, ".gitattributes", &["a binary"]); - write_normal_file(&repo, "a", &["line1"]); - create_commit(&repo); - write_normal_file(&repo, "a", &["line2"]); - create_commit(&repo); - let diff = diff_from_head(&repo, &CommitDiffLoaderOptions::new()); + write_normal_file(&repository, ".gitattributes", &["a binary"]); + write_normal_file(&repository, "a", &["line1"]); + create_commit(&repository); + write_normal_file(&repository, "a", &["line2"]); + create_commit(&repository); + + let loader = diff_from_head(repository, CommitDiffLoaderOptions::new()).unwrap(); + let diff = loader.take_diff(); + assert_eq!(diff.number_files_changed(), 1); assert_eq!(diff.number_insertions(), 0); assert_eq!(diff.number_deletions(), 0); assert_commit_diff!(&diff, "a (n,b)", "Status Modified"); }); } + + #[test] + fn diff_notifier() { + with_temp_repository(|repository| { + for i in 0..10 { + write_normal_file(&repository, format!("a-{i}").as_str(), &["line"]); + } + create_commit(&repository); + + let calls = Arc::new(Mutex::new(Vec::new())); + let notifier_calls = Arc::clone(&calls); + let notifier = move |status| { + let mut c = notifier_calls.lock(); + c.push(status); + false + }; + + _ = diff_with_notifier(repository, CommitDiffLoaderOptions::new(), notifier).unwrap(); + + let c = calls.lock(); + assert_eq!(c.first().unwrap(), &LoadStatus::New); + assert_eq!(c.get(1).unwrap(), &LoadStatus::Diff(0, 10)); + assert!(matches!(c.get(2).unwrap(), &LoadStatus::Diff(_, 10))); + assert!(matches!(c.last().unwrap(), &LoadStatus::DiffComplete)); + }); + } + + #[test] + fn diff_notifier_with_copies() { + with_temp_repository(|repository| { + for i in 0..10 { + write_normal_file(&repository, format!("a-{i}").as_str(), &["line"]); + } + create_commit(&repository); + + let calls = Arc::new(Mutex::new(Vec::new())); + let notifier_calls = Arc::clone(&calls); + let notifier = move |status| { + let mut c = notifier_calls.lock(); + c.push(status); + false + }; + + _ = diff_with_notifier(repository, CommitDiffLoaderOptions::new().copies(true), notifier).unwrap(); + + // Since the exact emitted statues are based on time, this matches a dynamic pattern of: + // - New + // - QuickDiff(0, 10) + // - QuickDiff(>0, 10) + // - CompleteQuickDiff + // - Diff(0, 10) + // - Diff(>0, 10) + // - DiffComplete + let mut pass = false; + let mut expected = LoadStatus::New; + for c in calls.lock().clone() { + match (&expected, c) { + (&LoadStatus::New, LoadStatus::New) => { + expected = LoadStatus::QuickDiff(0, 10); + }, + (&LoadStatus::QuickDiff(0, 10), LoadStatus::QuickDiff(0, 10)) => { + expected = LoadStatus::QuickDiff(1, 10); + }, + (&LoadStatus::QuickDiff(1, 10), LoadStatus::QuickDiff(p, 10)) => { + assert!(p > 0); + expected = LoadStatus::CompleteQuickDiff; + }, + (&LoadStatus::CompleteQuickDiff, LoadStatus::CompleteQuickDiff) => { + expected = LoadStatus::Diff(0, 10); + }, + (&LoadStatus::Diff(0, 10), LoadStatus::Diff(0, 10)) => { + expected = LoadStatus::Diff(1, 10); + }, + (&LoadStatus::Diff(1, 10), LoadStatus::Diff(p, 10)) => { + assert!(p > 0); + expected = LoadStatus::DiffComplete; + }, + (&LoadStatus::DiffComplete, LoadStatus::DiffComplete) => { + pass = true; + }, + (..) => {}, + } + } + + assert!(pass); + }); + } + + #[test] + fn cancel_diff_after_setting_commit() { + with_temp_repository(|repository| { + write_normal_file(&repository, "a", &["line1"]); + create_commit(&repository); + + let calls = Arc::new(Mutex::new(Vec::new())); + let notifier_calls = Arc::clone(&calls); + let notifier = move |status| { + let mut c = notifier_calls.lock(); + c.push(status); + true + }; + + _ = diff_with_notifier(repository, CommitDiffLoaderOptions::new(), notifier).unwrap(); + + let c = calls.lock(); + assert_eq!(c.len(), 1); + assert_eq!(c.first().unwrap(), &LoadStatus::New); + }); + } + + #[test] + fn cancel_diff_after_collect_load_stats() { + with_temp_repository(|repository| { + write_normal_file(&repository, "a", &["line1"]); + create_commit(&repository); + + let calls = Arc::new(Mutex::new(Vec::new())); + let notifier_calls = Arc::clone(&calls); + let notifier = move |status| { + let mut c = notifier_calls.lock(); + c.push(status); + c.len() == 2 + }; + + _ = diff_with_notifier(repository, CommitDiffLoaderOptions::new(), notifier).unwrap(); + + let c = calls.lock(); + assert_eq!(c.len(), 2); + assert_eq!(c.first().unwrap(), &LoadStatus::New); + assert_eq!(c.get(1).unwrap(), &LoadStatus::Diff(0, 1)); + }); + } + + #[test] + fn cancel_diff_during_diff_collect() { + with_temp_repository(|repository| { + for i in 0..10 { + write_normal_file(&repository, format!("a-{i}").as_str(), &["line"]); + } + create_commit(&repository); + + let calls = Arc::new(Mutex::new(Vec::new())); + let notifier_calls = Arc::clone(&calls); + let notifier = move |status| { + let mut c = notifier_calls.lock(); + c.push(status); + c.len() == 4 + }; + + _ = diff_with_notifier(repository, CommitDiffLoaderOptions::new(), notifier).unwrap(); + let c = calls.lock(); + assert_eq!(c.first().unwrap(), &LoadStatus::New); + assert_eq!(c.get(1).unwrap(), &LoadStatus::Diff(0, 10)); + assert!(matches!(c.last().unwrap(), &LoadStatus::Diff(_, 10))); + }); + } + + #[test] + fn cancel_diff_during_quick_diff_collect() { + with_temp_repository(|repository| { + for i in 0..10 { + write_normal_file(&repository, format!("a-{i}").as_str(), &["line"]); + } + create_commit(&repository); + + let calls = Arc::new(Mutex::new(Vec::new())); + let notifier_calls = Arc::clone(&calls); + let notifier = move |status| { + let mut c = notifier_calls.lock(); + c.push(status); + c.len() == 3 + }; + + _ = diff_with_notifier(repository, CommitDiffLoaderOptions::new().copies(true), notifier).unwrap(); + let c = calls.lock(); + assert_eq!(c.first().unwrap(), &LoadStatus::New); + assert_eq!(c.get(1).unwrap(), &LoadStatus::QuickDiff(0, 10)); + assert!(matches!(c.last().unwrap(), &LoadStatus::QuickDiff(_, 10))); + }); + } + + #[test] + fn cancel_diff_during_quick_diff_complete() { + with_temp_repository(|repository| { + for i in 0..10 { + write_normal_file(&repository, format!("a-{i}").as_str(), &["line"]); + } + create_commit(&repository); + + let calls = Arc::new(Mutex::new(Vec::new())); + let notifier_calls = Arc::clone(&calls); + let notifier = move |status| { + let mut c = notifier_calls.lock(); + let rtn = status == LoadStatus::CompleteQuickDiff; + c.push(status); + rtn + }; + + _ = diff_with_notifier(repository, CommitDiffLoaderOptions::new().copies(true), notifier).unwrap(); + let c = calls.lock(); + assert_eq!(c.first().unwrap(), &LoadStatus::New); + assert_eq!(c.get(1).unwrap(), &LoadStatus::QuickDiff(0, 10)); + assert!(matches!(c.get(2).unwrap(), &LoadStatus::QuickDiff(_, 10))); + assert_eq!(c.last().unwrap(), &LoadStatus::CompleteQuickDiff); + }); + } } diff --git a/src/diff/reference.rs b/src/diff/reference.rs index b7d6fe56e..bb80b07f9 100644 --- a/src/diff/reference.rs +++ b/src/diff/reference.rs @@ -74,9 +74,13 @@ mod tests { #[test] fn test() { with_temp_repository(|repository| { - let repo = crate::git::Repository::from(repository); - let oid = repo.head_id("main").unwrap(); - let reference = repo.find_reference("refs/heads/main").unwrap(); + let revision = repository.revparse_single("refs/heads/main").unwrap(); + let oid = revision.id().to_string(); + let reference = repository + .find_reference("refs/heads/main") + .map(|r| Reference::from(&r)) + .unwrap(); + assert_eq!(reference.hash(), format!("{oid}")); assert_eq!(reference.name(), "refs/heads/main"); assert_eq!(reference.shortname(), "main"); diff --git a/src/diff/thread.rs b/src/diff/thread.rs new file mode 100644 index 000000000..6ca34914a --- /dev/null +++ b/src/diff/thread.rs @@ -0,0 +1,192 @@ +mod action; +mod load_status; +mod state; +mod update_handler; + +use std::sync::Arc; + +use captur::capture; +use parking_lot::Mutex; + +pub(crate) use self::{action::Action, load_status::LoadStatus, state::State, update_handler::UpdateHandlerFn}; +use crate::{ + diff::CommitDiffLoader, + runtime::{Installer, Threadable}, +}; + +pub(crate) const THREAD_NAME: &str = "diff"; + +#[derive(Debug)] +pub(crate) struct Thread { + state: State, + commit_diff_loader: Arc>, + update_handler: Arc, +} + +impl Threadable for Thread +where UpdateHandler: UpdateHandlerFn + 'static +{ + fn install(&self, installer: &Installer) { + let state = self.state(); + + installer.spawn(THREAD_NAME, |notifier| { + let update_handler = Arc::clone(&self.update_handler); + let commit_diff_loader = Arc::clone(&self.commit_diff_loader); + move || { + capture!(notifier, state); + + notifier.wait(); + + loop { + notifier.wait(); + if state.is_ended() { + break; + } + + if state.is_cancelled() { + commit_diff_loader.lock().reset(); + } + + let msg = state.receive_update(); + notifier.busy(); + match msg { + Action::Load(hash) => { + state.resume(); + let mut loader = commit_diff_loader.lock(); + if let Err(e) = loader.load_diff(hash.as_str(), |s: LoadStatus| { + state.set_load_status(s); + update_handler(); + state.is_cancelled() + }) { + state.set_load_status(LoadStatus::Error { + msg: e.to_string(), + code: e.code(), + }); + state.cancel(); + update_handler(); + } + }, + Action::StatusChange => {}, + } + } + + notifier.request_end(); + notifier.end(); + } + }); + } + + fn pause(&self) { + self.state.cancel(); + } + + fn resume(&self) { + self.state.resume(); + } + + fn end(&self) { + self.state.end(); + } +} + +impl Thread +where UpdateHandler: UpdateHandlerFn +{ + pub(crate) fn new(commit_diff_loader: CommitDiffLoader, update_handler: UpdateHandler) -> Self { + let commit_diff = commit_diff_loader.commit_diff(); + Self { + state: State::new(commit_diff), + commit_diff_loader: Arc::new(Mutex::new(commit_diff_loader)), + update_handler: Arc::new(update_handler), + } + } + + pub(crate) fn state(&self) -> State { + self.state.clone() + } +} + +#[cfg(test)] +mod tests { + use std::{thread::sleep, time::Duration}; + + use super::*; + use crate::{ + diff::CommitDiffLoaderOptions, + runtime::Status, + test_helpers::{testers, with_temp_repository}, + }; + + #[test] + fn set_pause_resume() { + with_temp_repository(|repository| { + let diff_loader = CommitDiffLoader::new(repository, CommitDiffLoaderOptions::new()); + let diff = diff_loader.commit_diff(); + diff.write().update(vec![], 1, 2, 3); + let thread = Thread::new(diff_loader, || {}); + + let state = thread.state(); + let tester = testers::Threadable::new(); + tester.start_threadable(&thread, THREAD_NAME); + tester.wait_for_status(&Status::Waiting); + thread.pause(); + assert!(state.is_cancelled()); + let mut pass = false; + for _ in 0..10 { + if diff.read().number_deletions() == 0 { + pass = true; + break; + } + sleep(Duration::from_millis(10)); + } + assert!(pass); + thread.resume(); + assert!(!state.is_cancelled()); + state.end(); + tester.wait_for_status(&Status::Ended); + }); + } + + #[test] + fn set_end() { + with_temp_repository(|repository| { + let thread = Thread::new(CommitDiffLoader::new(repository, CommitDiffLoaderOptions::new()), || {}); + + let state = thread.state(); + let tester = testers::Threadable::new(); + tester.start_threadable(&thread, THREAD_NAME); + tester.wait_for_status(&Status::Waiting); + state.end(); + assert!(state.is_ended()); + tester.wait_for_status(&Status::Ended); + }); + } + + #[test] + fn diff_load_error() { + with_temp_repository(|repository| { + let thread = Thread::new(CommitDiffLoader::new(repository, CommitDiffLoaderOptions::new()), || {}); + + let state = thread.state(); + let tester = testers::Threadable::new(); + tester.start_threadable(&thread, THREAD_NAME); + tester.wait_for_status(&Status::Waiting); + + state.start_load("abc123"); + + let mut pass = false; + for _ in 0..10 { + if let LoadStatus::Error { .. } = state.load_status() { + pass = true; + break; + } + sleep(Duration::from_millis(10)); + } + assert!(pass); + assert!(state.is_cancelled()); + + state.end(); + tester.wait_for_status(&Status::Ended); + }); + } +} diff --git a/src/diff/thread/action.rs b/src/diff/thread/action.rs new file mode 100644 index 000000000..9935d5ef0 --- /dev/null +++ b/src/diff/thread/action.rs @@ -0,0 +1,29 @@ +use std::fmt::{Debug, Formatter}; + +#[derive(PartialEq)] +pub(crate) enum Action { + StatusChange, + Load(String), +} + +impl Debug for Action { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + match *self { + Self::StatusChange => write!(f, "StatusChange"), + Self::Load(ref hash) => write!(f, "Load({hash})"), + } + } +} +#[cfg(test)] +mod tests { + use rstest::rstest; + + use super::*; + + #[rstest] + #[case::status_change(Action::StatusChange, "StatusChange")] + #[case::cont(Action::Load(String::from("abc123")), "Load(abc123)")] + fn debug(#[case] action: Action, #[case] expected: &str) { + assert_eq!(format!("{action:?}"), expected); + } +} diff --git a/src/diff/thread/load_status.rs b/src/diff/thread/load_status.rs new file mode 100644 index 000000000..f3e523a5e --- /dev/null +++ b/src/diff/thread/load_status.rs @@ -0,0 +1,11 @@ +use git2::ErrorCode; + +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) enum LoadStatus { + New, + QuickDiff(usize, usize), + CompleteQuickDiff, + Diff(usize, usize), + DiffComplete, + Error { msg: String, code: ErrorCode }, +} diff --git a/src/diff/thread/state.rs b/src/diff/thread/state.rs new file mode 100644 index 000000000..926f10dcb --- /dev/null +++ b/src/diff/thread/state.rs @@ -0,0 +1,150 @@ +use std::sync::{ + Arc, + atomic::{AtomicBool, Ordering}, +}; + +use parking_lot::RwLock; + +use crate::diff::{ + CommitDiff, + thread::{Action, LoadStatus}, +}; + +#[derive(Clone, Debug)] +pub(crate) struct State { + load_status: Arc>, + diff: Arc>, + ended: Arc, + cancelled: Arc, + update_receiver: crossbeam_channel::Receiver, + update_sender: crossbeam_channel::Sender, +} + +impl State { + pub(crate) fn new(diff: Arc>) -> Self { + let (update_sender, update_receiver) = crossbeam_channel::unbounded(); + Self { + load_status: Arc::new(RwLock::new(LoadStatus::New)), + diff, + ended: Arc::new(AtomicBool::from(false)), + cancelled: Arc::new(AtomicBool::from(false)), + update_receiver, + update_sender, + } + } + + pub(crate) fn load_status(&self) -> LoadStatus { + self.load_status.read().clone() + } + + pub(crate) fn set_load_status(&self, status: LoadStatus) { + let mut load_status = self.load_status.write(); + *load_status = status; + } + + pub(crate) fn diff(&self) -> Arc> { + Arc::clone(&self.diff) + } + + pub(crate) fn receive_update(&self) -> Action { + self.update_receiver.recv().unwrap_or(Action::StatusChange) + } + + pub(crate) fn start_load(&self, term: &str) { + self.send_update(Action::Load(String::from(term))); + } + + pub(crate) fn is_cancelled(&self) -> bool { + self.cancelled.load(Ordering::Acquire) || self.ended.load(Ordering::Acquire) + } + + pub(crate) fn is_ended(&self) -> bool { + self.ended.load(Ordering::Acquire) + } + + pub(crate) fn cancel(&self) { + self.cancelled.store(true, Ordering::Release); + self.send_update(Action::StatusChange); + } + + pub(crate) fn resume(&self) { + self.cancelled.store(false, Ordering::Release); + self.send_update(Action::StatusChange); + } + + pub(crate) fn end(&self) { + self.ended.store(true, Ordering::Release); + self.send_update(Action::StatusChange); + } + + fn send_update(&self, action: Action) { + let _result = self.update_sender.send(action); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn create_state() -> State { + State::new(Arc::new(RwLock::new(CommitDiff::new()))) + } + + #[test] + fn send_recv_update() { + let state = create_state(); + state.send_update(Action::Load(String::from("abc123"))); + assert_eq!(state.receive_update(), Action::Load(String::from("abc123"))); + } + + #[test] + fn send_recv_disconnect() { + let (update_sender, _update_receiver) = crossbeam_channel::unbounded(); + let mut state = create_state(); + state.update_sender = update_sender; // replace last reference to sender, to force a disconnect + assert_eq!(state.receive_update(), Action::StatusChange); + } + + #[test] + fn load_status() { + let state = create_state(); + state.set_load_status(LoadStatus::DiffComplete); + assert_eq!(state.load_status(), LoadStatus::DiffComplete); + } + + #[test] + fn start_load() { + let state = create_state(); + state.start_load("term"); + assert_eq!(state.receive_update(), Action::Load(String::from("term"))); + } + + #[test] + fn diff() { + // not much to test here + let state = create_state(); + let _diff = state.diff(); + } + + #[test] + fn cancelled() { + let state = create_state(); + state.cancel(); + assert!(state.is_cancelled()); + } + + #[test] + fn resume() { + let state = create_state(); + state.cancel(); + state.resume(); + assert!(!state.is_cancelled()); + } + + #[test] + fn ended() { + let state = create_state(); + state.end(); + assert!(state.is_ended()); + } +} diff --git a/src/diff/thread/update_handler.rs b/src/diff/thread/update_handler.rs new file mode 100644 index 000000000..ee91e077b --- /dev/null +++ b/src/diff/thread/update_handler.rs @@ -0,0 +1,3 @@ +pub(crate) trait UpdateHandlerFn: Fn() + Sync + Send {} + +impl UpdateHandlerFn for FN {} diff --git a/src/diff/user.rs b/src/diff/user.rs index d0d32c1ba..6a6f9c6e9 100644 --- a/src/diff/user.rs +++ b/src/diff/user.rs @@ -1,7 +1,7 @@ use std::fmt::Display; /// Represents a user within a commit with a name and email address -#[derive(Debug, Eq, PartialEq)] +#[derive(Debug, Clone, Eq, PartialEq)] pub(crate) struct User { name: Option, email: Option, diff --git a/src/git.rs b/src/git.rs index b769f05d8..1054a779e 100644 --- a/src/git.rs +++ b/src/git.rs @@ -7,16 +7,12 @@ //! To facilitate testing the usages of this crate, a set of testing utilities are provided. Since //! these utilities are not tested, and often are optimized for developer experience than //! performance, they should only be used in test code. - mod errors; -mod repository; +use git2::Repository; pub(crate) use git2::{Config, ErrorCode}; -pub(crate) use self::{ - errors::{GitError, RepositoryLoadKind}, - repository::Repository, -}; +pub(crate) use self::errors::{GitError, RepositoryLoadKind}; /// Find and open an existing repository, respecting git environment variables. This will check /// for and use `$GIT_DIR`, and if unset will search for a repository starting in the current @@ -24,8 +20,8 @@ pub(crate) use self::{ /// /// # Errors /// Will result in an error if the repository cannot be opened. -pub(crate) fn open_repository_from_env() -> Result { - git2::Repository::open_from_env().map_err(|e| { +pub(crate) fn open_repository_from_env() -> Result { + Repository::open_from_env().map_err(|e| { GitError::RepositoryLoad { kind: RepositoryLoadKind::Environment, cause: e, diff --git a/src/git/errors.rs b/src/git/errors.rs index d261e1679..956e19a1e 100644 --- a/src/git/errors.rs +++ b/src/git/errors.rs @@ -5,6 +5,7 @@ use std::fmt::{Display, Formatter}; +use git2::ErrorCode; use thiserror::Error; /// The kind of repository load kind. @@ -49,23 +50,33 @@ pub(crate) enum GitError { #[source] cause: git2::Error, }, - /// The configuration could not be loaded - #[cfg(test)] - #[error("Could not load configuration")] - ReferenceNotFound { + /// The commit could not be loaded + #[error("Could not load commit")] + CommitLoad { /// The internal cause of the load error. #[source] cause: git2::Error, }, - /// The commit could not be loaded - #[error("Could not load commit")] - CommitLoad { + /// The diff could not be loaded + #[error("Could not load diff")] + DiffLoad { /// The internal cause of the load error. #[source] cause: git2::Error, }, } +impl GitError { + pub(crate) fn code(&self) -> ErrorCode { + match self { + GitError::RepositoryLoad { cause, .. } + | GitError::ConfigLoad { cause, .. } + | GitError::CommitLoad { cause, .. } + | GitError::DiffLoad { cause, .. } => cause.code(), + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -79,4 +90,37 @@ mod tests { fn repository_load_kind_display_path() { assert_eq!(format!("{}", RepositoryLoadKind::Path), "path"); } + + #[test] + fn code_repository_load() { + let error = GitError::RepositoryLoad { + kind: RepositoryLoadKind::Environment, + cause: git2::Error::from_str("main"), + }; + assert_eq!(error.code(), ErrorCode::GenericError); + } + + #[test] + fn code_config_load() { + let error = GitError::ConfigLoad { + cause: git2::Error::from_str("main"), + }; + assert_eq!(error.code(), ErrorCode::GenericError); + } + + #[test] + fn code_commit_load() { + let error = GitError::CommitLoad { + cause: git2::Error::from_str("main"), + }; + assert_eq!(error.code(), ErrorCode::GenericError); + } + + #[test] + fn code_diff_load() { + let error = GitError::DiffLoad { + cause: git2::Error::from_str("main"), + }; + assert_eq!(error.code(), ErrorCode::GenericError); + } } diff --git a/src/git/repository.rs b/src/git/repository.rs deleted file mode 100644 index 8f83aa27e..000000000 --- a/src/git/repository.rs +++ /dev/null @@ -1,186 +0,0 @@ -use std::fmt::{Debug, Formatter}; - -use crate::{ - diff::{CommitDiff, CommitDiffLoader, CommitDiffLoaderOptions}, - git::GitError, -}; - -/// A light cloneable, simple wrapper around the `git2::Repository` struct -pub(crate) struct Repository { - repository: git2::Repository, -} - -impl Repository { - /// Load a diff for a commit hash - /// - /// # Errors - /// Will result in an error if the commit cannot be loaded. - pub(crate) fn load_commit_diff( - &self, - hash: &str, - config: &CommitDiffLoaderOptions, - ) -> Result { - let oid = self - .repository - .revparse_single(hash) - .map_err(|e| GitError::CommitLoad { cause: e })? - .id(); - let loader = CommitDiffLoader::new(&self.repository, config); - - loader - .load_from_hash(oid) - .map_err(|e| GitError::CommitLoad { cause: e }) - } -} - -impl From for Repository { - fn from(repository: git2::Repository) -> Self { - Self { repository } - } -} - -impl Debug for Repository { - fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), std::fmt::Error> { - f.debug_struct("Repository") - .field("[path]", &self.repository.path()) - .finish() - } -} - -#[cfg(test)] -mod tests { - use std::path::{Path, PathBuf}; - - use git2::{Oid, Signature}; - - use crate::{ - diff::{Commit, Reference}, - git::{GitError, Repository}, - }; - - impl Repository { - /// Find a reference by the reference name. - /// - /// # Errors - /// Will result in an error if the reference cannot be found. - pub(crate) fn find_reference(&self, reference: &str) -> Result { - let git2_reference = self - .repository - .find_reference(reference) - .map_err(|e| GitError::ReferenceNotFound { cause: e })?; - Ok(Reference::from(&git2_reference)) - } - - /// Find a commit by a reference name. - /// - /// # Errors - /// Will result in an error if the reference cannot be found or is not a commit. - pub(crate) fn find_commit(&self, reference: &str) -> Result { - let reference = self - .repository - .find_reference(reference) - .map_err(|e| GitError::ReferenceNotFound { cause: e })?; - Commit::try_from(&reference) - } - - pub(crate) fn repo_path(&self) -> PathBuf { - self.repository.path().to_path_buf() - } - - pub(crate) fn head_id(&self, head_name: &str) -> Result { - let ref_name = format!("refs/heads/{head_name}"); - let revision = self.repository.revparse_single(ref_name.as_str())?; - Ok(revision.id()) - } - - pub(crate) fn commit_id_from_ref(&self, reference: &str) -> Result { - let commit = self.repository.find_reference(reference)?.peel_to_commit()?; - Ok(commit.id()) - } - - pub(crate) fn add_path_to_index(&self, path: &Path) -> Result<(), git2::Error> { - let mut index = self.repository.index()?; - index.add_path(path) - } - - pub(crate) fn remove_path_from_index(&self, path: &Path) -> Result<(), git2::Error> { - let mut index = self.repository.index()?; - index.remove_path(path) - } - - pub(crate) fn create_commit_on_index( - &self, - reference: &str, - author: &Signature<'_>, - committer: &Signature<'_>, - message: &str, - ) -> Result<(), git2::Error> { - let tree = self.repository.find_tree(self.repository.index()?.write_tree()?)?; - let head = self.repository.find_reference(reference)?.peel_to_commit()?; - _ = self - .repository - .commit(Some("HEAD"), author, committer, message, &tree, &[&head])?; - Ok(()) - } - - pub(crate) fn repository(&self) -> &git2::Repository { - &self.repository - } - } -} - -// Paths in Windows make these tests difficult, so disable -#[cfg(all(unix, test))] -mod unix_tests { - use claims::{assert_err_eq, assert_ok}; - use git2::{ErrorClass, ErrorCode}; - - use super::*; - use crate::test_helpers::{create_commit, with_temp_repository}; - - #[test] - fn load_commit_diff() { - with_temp_repository(|repo| { - let repository = Repository::from(repo); - create_commit(&repository, None); - let id = repository.commit_id_from_ref("refs/heads/main").unwrap(); - assert_ok!(repository.load_commit_diff(id.to_string().as_str(), &CommitDiffLoaderOptions::new())); - }); - } - - #[test] - fn load_commit_diff_with_non_commit() { - with_temp_repository(|repo| { - let blob_ref = { - let blob = repo.blob(b"foo").unwrap(); - _ = repo.reference("refs/blob", blob, false, "blob").unwrap(); - blob.to_string() - }; - let repository = Repository::from(repo); - - assert_err_eq!( - repository.load_commit_diff(blob_ref.as_str(), &CommitDiffLoaderOptions::new()), - GitError::CommitLoad { - cause: git2::Error::new( - ErrorCode::NotFound, - ErrorClass::Invalid, - "the requested type does not match the type in the ODB", - ), - } - ); - }); - } - - #[test] - fn fmt() { - with_temp_repository(|repo| { - let repository = Repository::from(repo); - let formatted = format!("{repository:?}"); - let path = repository.repo_path().canonicalize().unwrap(); - assert_eq!( - formatted, - format!("Repository {{ [path]: \"{}/\" }}", path.to_str().unwrap()) - ); - }); - } -} diff --git a/src/input/standard_event.rs b/src/input/standard_event.rs index 3682abf12..6543558a2 100644 --- a/src/input/standard_event.rs +++ b/src/input/standard_event.rs @@ -109,4 +109,6 @@ pub(crate) enum StandardEvent { ExternalCommandError, /// Search was updated SearchUpdate, + /// Diff was updated + DiffUpdate, } diff --git a/src/module/module_provider.rs b/src/module/module_provider.rs index cfcb75cc5..d4e9a36d7 100644 --- a/src/module/module_provider.rs +++ b/src/module/module_provider.rs @@ -1,11 +1,10 @@ use crate::{ application::AppData, - git::Repository, module::{Module, State}, }; pub(crate) trait ModuleProvider { - fn new(repository: Repository, app_data: &AppData) -> Self; + fn new(app_data: &AppData) -> Self; fn get_mut_module(&mut self, _state: State) -> &mut dyn Module; diff --git a/src/module/modules.rs b/src/module/modules.rs index 839495ec6..f1e63430b 100644 --- a/src/module/modules.rs +++ b/src/module/modules.rs @@ -1,6 +1,5 @@ use crate::{ application::AppData, - git::Repository, module::{Module, ModuleProvider, State}, modules::{ConfirmAbort, ConfirmRebase, Error, ExternalEditor, Insert, List, ShowCommit, WindowSizeError}, }; @@ -17,11 +16,11 @@ pub(crate) struct Modules { } impl ModuleProvider for Modules { - fn new(repository: Repository, app_data: &AppData) -> Self { + fn new(app_data: &AppData) -> Self { Self { error: Error::new(app_data), list: List::new(app_data), - show_commit: ShowCommit::new(app_data, repository), + show_commit: ShowCommit::new(app_data), window_size_error: WindowSizeError::new(), confirm_abort: ConfirmAbort::new(app_data), confirm_rebase: ConfirmRebase::new(app_data), @@ -61,33 +60,35 @@ impl ModuleProvider for Modules { mod tests { use std::sync::Arc; - use parking_lot::Mutex; + use parking_lot::{Mutex, lock_api::RwLock}; use super::*; use crate::{ + diff, + diff::CommitDiff, input, search, - test_helpers::{create_config, with_temp_repository, with_todo_file}, + test_helpers::{create_config, with_todo_file}, view, }; pub(crate) fn modules_test(callback: C) where C: FnOnce(Modules) { - with_temp_repository(|repository| { - with_todo_file(&[], |todo_file_context| { - let (_todo_file_path, todo_file) = todo_file_context.to_owned(); - let config = create_config(); - let app_data = AppData::new( - config, - State::WindowSizeError, - Arc::new(Mutex::new(todo_file)), - view::State::new(), - input::State::new(), - search::State::new(), - ); - let modules = Modules::new(Repository::from(repository), &app_data); - callback(modules); - }); + with_todo_file(&[], |todo_file_context| { + let commit_diff = CommitDiff::new(); + let (_todo_file_path, todo_file) = todo_file_context.to_owned(); + let config = create_config(); + let app_data = AppData::new( + config, + State::WindowSizeError, + Arc::new(Mutex::new(todo_file)), + diff::thread::State::new(Arc::new(RwLock::new(commit_diff))), + view::State::new(), + input::State::new(), + search::State::new(), + ); + let modules = Modules::new(&app_data); + callback(modules); }); } diff --git a/src/module/tests.rs b/src/module/tests.rs index 89ee19f87..d1ce5e902 100644 --- a/src/module/tests.rs +++ b/src/module/tests.rs @@ -26,7 +26,7 @@ fn default_trait_method_build_view_data() { testers::module(&[], &[], None, |context| { let mut module = TestModule {}; let view_data = module.build_view_data(&context.render_context); - assert!(view_data.is_empty()); + assert!(!view_data.get_name().is_empty()); }); } diff --git a/src/modules/show_commit.rs b/src/modules/show_commit.rs index eab2b3591..f56bd2ea0 100644 --- a/src/modules/show_commit.rs +++ b/src/modules/show_commit.rs @@ -7,7 +7,7 @@ mod tests; use std::sync::Arc; -use anyhow::{Error, anyhow}; +use anyhow::anyhow; use captur::capture; use parking_lot::Mutex; @@ -19,16 +19,16 @@ use self::{ use crate::{ application::AppData, components::help::Help, - config::{DiffIgnoreWhitespaceSetting, DiffShowWhitespaceSetting}, - diff::{CommitDiff, CommitDiffLoaderOptions}, - git::Repository, + config::DiffShowWhitespaceSetting, + diff, + diff::thread::LoadStatus, input::{Event, InputOptions, KeyBindings, StandardEvent}, module::{Module, State}, process::Results, select, todo_file::TodoFile, util::handle_view_data_scroll, - view::{self, RenderContext, ViewData}, + view::{self, RenderContext, ViewData, ViewLine}, }; // TODO Remove `union` call when bitflags/bitflags#180 is resolved @@ -37,12 +37,10 @@ const INPUT_OPTIONS: InputOptions = InputOptions::UNDO_REDO .union(InputOptions::HELP); pub(crate) struct ShowCommit { - commit_diff_loader_options: CommitDiffLoaderOptions, - diff: Option, + diff_state: diff::thread::State, diff_view_data: ViewData, help: Help, overview_view_data: ViewData, - repository: Repository, state: ShowCommitState, view_state: view::State, todo_file: Arc>, @@ -53,13 +51,15 @@ impl Module for ShowCommit { fn activate(&mut self, _: State) -> Results { let mut results = Results::new(); if let Some(selected_line) = self.todo_file.lock().get_selected_line() { - // skip loading commit data if the currently loaded commit has not changed, this retains - // position after returning to the list view or help - if let Some(diff) = self.diff.as_ref() { - if diff.commit().hash() == selected_line.get_hash() { + { + // skip loading commit data if the currently loaded commit has not changed, this retains + // position after returning to the list view or help + let diff = self.diff_state.diff(); + if diff.read().commit().hash() == selected_line.get_hash() { return results; } } + self.overview_view_data.update_view_data(|updater| { updater.clear(); updater.reset_scroll_position(); @@ -70,18 +70,7 @@ impl Module for ShowCommit { updater.reset_scroll_position(); }); - let new_diff = self - .repository - .load_commit_diff(selected_line.get_hash(), &self.commit_diff_loader_options); - - match new_diff { - Ok(diff) => { - self.diff = Some(diff); - }, - Err(e) => { - results.error_with_return(Error::from(e), State::List); - }, - } + results.load_diff(selected_line.get_hash()); } else { results.error_with_return(anyhow!("No valid commit to show"), State::List); @@ -94,28 +83,48 @@ impl Module for ShowCommit { return self.help.get_view_data(); } - let diff = self.diff.as_ref().unwrap(); // will only fail on programmer error + let diff_arc = self.diff_state.diff(); + let diff = diff_arc.read(); + let load_status = self.diff_state.load_status(); + + if let LoadStatus::Error { code, msg, .. } = load_status { + self.overview_view_data.update_view_data(|updater| { + updater.clear(); + self.view_builder.build_diff_error(updater, code, msg.as_str()); + }); + return &self.overview_view_data; + } + + // There is a small race condition where sometimes the diff loader is still in the process + // of cancelling the previous diff and still has that diff loaded. In that case, we want to + // show a general loading diff. + let todo_line = self.todo_file.lock(); + let selected_line = todo_line.get_selected_line().map_or("", |l| l.get_hash()); + if self.diff_state.is_cancelled() || selected_line.is_empty() || selected_line != diff.commit().hash() { + self.overview_view_data.update_view_data(|updater| { + updater.clear(); + updater.push_line(ViewLine::from("Loading Diff")); + }); + return &self.overview_view_data; + } + let state = &self.state; - let view_builder = &self.view_builder; + let view_builder = &mut self.view_builder; let is_full_width = context.is_full_width(); match *state { ShowCommitState::Overview => { - if self.overview_view_data.is_empty() { - self.overview_view_data.update_view_data(|updater| { - capture!(view_builder, diff); - view_builder.build_view_data_for_overview(updater, diff, is_full_width); - }); - } + self.overview_view_data.update_view_data(|updater| { + capture!(view_builder, diff); + view_builder.build_view_data_for_overview(updater, &diff, &load_status, is_full_width); + }); &self.overview_view_data }, ShowCommitState::Diff => { - if self.diff_view_data.is_empty() { - self.diff_view_data.update_view_data(|updater| { - capture!(view_builder, diff); - view_builder.build_view_data_diff(updater, diff, is_full_width); - }); - } + self.diff_view_data.update_view_data(|updater| { + capture!(view_builder, diff); + view_builder.build_view_data_diff(updater, &diff, &load_status, is_full_width); + }); &self.diff_view_data }, } @@ -145,14 +154,8 @@ impl Module for ShowCommit { default { let mut results = Results::new(); - let active_view_data = match self.state { - ShowCommitState::Overview => &mut self.overview_view_data, - ShowCommitState::Diff => &mut self.diff_view_data, - }; - match event { Event::Standard(StandardEvent::ShowDiff) => { - active_view_data.update_view_data(|updater| updater.clear()); self.state = match self.state { ShowCommitState::Overview => ShowCommitState::Diff, ShowCommitState::Diff => ShowCommitState::Overview, @@ -160,15 +163,14 @@ impl Module for ShowCommit { }, Event::Standard(StandardEvent::Help) => self.help.set_active(), Event::Key(_) => { - active_view_data.update_view_data(|updater| updater.clear()); if self.state == ShowCommitState::Diff { self.state = ShowCommitState::Overview; } else { + results.cancel_diff(); results.state(State::List); } }, - Event::Resize(..) => active_view_data.update_view_data(|updater| updater.clear()), _ => {}, } results @@ -180,7 +182,7 @@ impl Module for ShowCommit { } impl ShowCommit { - pub(crate) fn new(app_data: &AppData, repository: Repository) -> Self { + pub(crate) fn new(app_data: &AppData) -> Self { let overview_view_data = ViewData::new(|updater| { updater.set_show_title(true); updater.set_show_help(true); @@ -200,22 +202,11 @@ impl ShowCommit { || config.diff_show_whitespace == DiffShowWhitespaceSetting::Trailing, ); - let commit_diff_loader_options = CommitDiffLoaderOptions::new() - .context_lines(config.git.diff_context) - .copies(config.git.diff_copies) - .ignore_whitespace(config.diff_ignore_whitespace == DiffIgnoreWhitespaceSetting::All) - .ignore_whitespace_change(config.diff_ignore_whitespace == DiffIgnoreWhitespaceSetting::Change) - .ignore_blank_lines(config.diff_ignore_blank_lines) - .interhunk_context(config.git.diff_interhunk_lines) - .renames(config.git.diff_renames, config.git.diff_rename_limit); - Self { - commit_diff_loader_options, - diff: None, + diff_state: app_data.diff_state(), diff_view_data, help: Help::new_from_keybindings(&get_show_commit_help_lines(&config.key_bindings)), overview_view_data, - repository, state: ShowCommitState::Overview, view_state: app_data.view_state(), todo_file: app_data.todo_file(), diff --git a/src/modules/show_commit/tests.rs b/src/modules/show_commit/tests.rs index 866e63ea3..79ea94f3b 100644 --- a/src/modules/show_commit/tests.rs +++ b/src/modules/show_commit/tests.rs @@ -1,11 +1,12 @@ use anyhow::anyhow; +use git2::ErrorCode; use rstest::rstest; use super::*; use crate::{ assert_rendered_output, assert_results, - diff::{Delta, DiffLine, FileMode, Origin, Status, User}, + diff::{Commit, Delta, DiffLine, FileMode, Origin, Status, User}, input::StandardEvent, process::Artifact, render_line, @@ -14,352 +15,351 @@ use crate::{ builders::{CommitBuilder, CommitDiffBuilder, FileStatusBuilder}, create_config, testers, - with_temp_repository, }, - view::ViewLine, }; fn render_options() -> AssertRenderOptions { AssertRenderOptions::INCLUDE_STYLE | AssertRenderOptions::BODY_ONLY } +fn update_diff(module: &mut ShowCommit, builder: CommitDiffBuilder) { + let diff = module.diff_state.diff(); + let mut diff_lock = diff.write(); + *diff_lock = builder.build(); + module.diff_state.set_load_status(LoadStatus::DiffComplete); +} + #[test] fn load_commit_during_activate() { - with_temp_repository(|repository| { - let repo = Repository::from(repository); - let oid = repo.head_id("main").unwrap(); - let line = format!("pick {oid} comment1"); - testers::module(&[line.as_str()], &[], None, |test_context| { - let mut module = ShowCommit::new(&test_context.app_data(), repo); - assert_results!(test_context.activate(&mut module, State::List)); - assert!(module.diff.is_some()); - }); + let hash = "abcde12345"; + let line = format!("pick {hash} comment1"); + testers::module(&[line.as_str()], &[], None, |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + assert_results!( + test_context.activate(&mut module, State::List), + Artifact::LoadDiff(String::from(hash)) + ); }); } #[test] fn cached_commit_in_activate() { - with_temp_repository(|repository| { - let repo = Repository::from(repository); - let oid = repo.head_id("main").unwrap(); - let line = format!("pick {oid} comment1"); - testers::module(&[line.as_str()], &[], None, |test_context| { - let mut module = ShowCommit::new(&test_context.app_data(), repo); - // would be nice to be able to test that a second call to load_commit_diff did not happen here - assert_results!(test_context.activate(&mut module, State::List)); - assert_results!(test_context.activate(&mut module, State::List)); - }); + let hash = "abcde12345"; + let line = format!("pick {hash} comment1"); + testers::module(&[line.as_str()], &[], None, |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + let diff = test_context.app_data().diff_state().diff(); + // prefill cache + { + let mut diff_lock = diff.write(); + diff_lock.reset(Commit::new_with_hash(hash), None); + } + _ = test_context.activate(&mut module, State::List); + // second call should not trigger a load_diff + assert_results!(test_context.activate(&mut module, State::List)); }); } #[test] fn no_selected_line_in_activate() { - with_temp_repository(|repository| { - let repo = Repository::from(repository); - testers::module(&[], &[], None, |test_context| { - let mut module = ShowCommit::new(&test_context.app_data(), repo); - assert_results!( - test_context.activate(&mut module, State::List), - Artifact::Error(anyhow!("No valid commit to show"), Some(State::List)) - ); - }); + testers::module(&[], &[], None, |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + assert_results!( + test_context.activate(&mut module, State::List), + Artifact::Error(anyhow!("No valid commit to show"), Some(State::List)) + ); }); } #[test] -fn activate_error() { - with_temp_repository(|repository| { - let repo = Repository::from(repository); - testers::module(&["pick aaaaaaaaaa comment1"], &[], None, |test_context| { - let mut module = ShowCommit::new(&test_context.app_data(), repo); - assert_results!( - test_context.activate(&mut module, State::List), - Artifact::Error( - anyhow!( - "Could not load commit: revspec 'aaaaaaaaaa' not found; class=Reference (4); code=NotFound \ - (-3)" - ), - Some(State::List) - ) - ); - }); - }); +fn render_cancelled_diff_race() { + testers::module( + &[ + "pick 0123456789abcdef0123456789abcdef comment1", + "pick abcdef0123456789abcdef0123456789 comment2", + ], + &[], + None, + |test_context| { + { + let diff = test_context.app_data().diff_state().diff(); + let mut diff_lock = diff.write(); + diff_lock.reset(Commit::new_with_hash("abcdef0123456789abcdef0123456789"), None); + } + let mut module = ShowCommit::new(&test_context.app_data()); + + assert_rendered_output!( + Options AssertRenderOptions::BODY_ONLY, + test_context.build_view_data(&mut module), + "Loading Diff" + ); + }, + ); } #[test] fn render_overview_minimal_commit() { - with_temp_repository(|repository| { - let repo = Repository::from(repository); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - None, - |test_context| { - let commit = CommitBuilder::new("0123456789abcdef0123456789abcdef").build(); - let commit_date = commit.committed_date().format("%c %z").to_string(); - let diff = CommitDiffBuilder::new(commit).build(); - let mut module = ShowCommit::new(&test_context.app_data(), repo); - module.diff = Some(diff); - assert_rendered_output!( - Options AssertRenderOptions::INCLUDE_STYLE, - test_context.build_view_data(&mut module), - "{TITLE}{HELP}", - "{LEADING}", - "{IndicatorColor}Commit: {Normal}0123456789abcdef0123456789abcdef", - "{BODY}", - format!("{{IndicatorColor}}Date: {{Normal}}{commit_date}"), - "{Normal}", - "{IndicatorColor}0{Normal} files with {DiffAddColor}0{Normal} insertions and \ - {DiffRemoveColor}0{Normal} deletions" - ); - }, - ); - }); + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |test_context| { + let commit = CommitBuilder::new("0123456789abcdef0123456789abcdef").build(); + let commit_date = commit.committed_date().format("%c %z").to_string(); + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff(&mut module, CommitDiffBuilder::new(commit)); + + assert_rendered_output!( + Options AssertRenderOptions::INCLUDE_STYLE, + test_context.build_view_data(&mut module), + "{TITLE}{HELP}", + "{LEADING}", + "{IndicatorColor}Commit: {Normal}0123456789abcdef0123456789abcdef", + "{BODY}", + format!("{{IndicatorColor}}Date: {{Normal}}{commit_date}"), + "{Normal}", + "{IndicatorColor}0{Normal} files with {DiffAddColor}0{Normal} insertions and \ + {DiffRemoveColor}0{Normal} deletions" + ); + }, + ); } #[test] fn render_overview_minimal_commit_compact() { - with_temp_repository(|repository| { - let repo = Repository::from(repository); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - None, - |mut test_context| { - test_context.render_context.update(30, 300); - let commit = CommitBuilder::new("0123456789abcdef0123456789abcdef").build(); - let commit_date = commit.committed_date().format("%c %z").to_string(); - let diff = CommitDiffBuilder::new(commit).build(); - let mut module = ShowCommit::new(&test_context.app_data(), repo); - module.diff = Some(diff); - assert_rendered_output!( - Options AssertRenderOptions::INCLUDE_STYLE, - test_context.build_view_data(&mut module), - "{TITLE}{HELP}", - "{LEADING}", - "{Normal}01234567", - "{BODY}", - format!("{{IndicatorColor}}D: {{Normal}}{commit_date}"), - "{Normal}", - "{IndicatorColor}0{Normal} / {DiffAddColor}0{Normal} / {DiffRemoveColor}0" - ); - }, - ); - }); + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |mut test_context| { + test_context.render_context.update(30, 300); + let commit = CommitBuilder::new("0123456789abcdef0123456789abcdef").build(); + let commit_date = commit.committed_date().format("%c %z").to_string(); + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff(&mut module, CommitDiffBuilder::new(commit)); + + assert_rendered_output!( + Options AssertRenderOptions::INCLUDE_STYLE, + test_context.build_view_data(&mut module), + "{TITLE}{HELP}", + "{LEADING}", + "{Normal}01234567", + "{BODY}", + format!("{{IndicatorColor}}D: {{Normal}}{commit_date}"), + "{Normal}", + "{IndicatorColor}0{Normal} / {DiffAddColor}0{Normal} / {DiffRemoveColor}0" + ); + }, + ); } #[test] fn render_overview_with_author() { - with_temp_repository(|repository| { - let repo = Repository::from(repository); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - None, - |test_context| { - let diff = CommitDiffBuilder::new( + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new( CommitBuilder::new("0123456789abcdef0123456789abcdef") .author(User::new(Some("John Doe"), Some("john.doe@example.com"))) .build(), - ) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repo); - module.diff = Some(diff); - assert_rendered_output!( - Options render_options(), - Skip 1;2, - test_context.build_view_data(&mut module), - "{IndicatorColor}Author: {Normal}John Doe " - ); - }, - ); - }); + ), + ); + + assert_rendered_output!( + Options render_options(), + Skip 1;2, + test_context.build_view_data(&mut module), + "{IndicatorColor}Author: {Normal}John Doe " + ); + }, + ); } #[test] fn render_overview_with_author_compact() { - with_temp_repository(|repository| { - let repo = Repository::from(repository); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - None, - |mut test_context| { - test_context.render_context.update(30, 300); - let diff = CommitDiffBuilder::new( + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |mut test_context| { + test_context.render_context.update(30, 300); + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new( CommitBuilder::new("0123456789abcdef0123456789abcdef") .author(User::new(Some("John Doe"), Some("john.doe@example.com"))) .build(), - ) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repo); - module.diff = Some(diff); - assert_rendered_output!( - Options render_options(), - Skip 1;2, - test_context.build_view_data(&mut module), - "{IndicatorColor}A: {Normal}John Doe " - ); - }, - ); - }); + ), + ); + + assert_rendered_output!( + Options render_options(), + Skip 1;2, + test_context.build_view_data(&mut module), + "{IndicatorColor}A: {Normal}John Doe " + ); + }, + ); } #[test] fn render_overview_with_committer() { - with_temp_repository(|repository| { - let repo = Repository::from(repository); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - None, - |test_context| { - let diff = CommitDiffBuilder::new( + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new( CommitBuilder::new("0123456789abcdef0123456789abcdef") .committer(User::new(Some("John Doe"), Some("john.doe@example.com"))) .build(), - ) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repo); - module.diff = Some(diff); - assert_rendered_output!( - Options render_options(), - Skip 1;2, - test_context.build_view_data(&mut module), - "{IndicatorColor}Committer: {Normal}John Doe " - ); - }, - ); - }); + ), + ); + + assert_rendered_output!( + Options render_options(), + Skip 1;2, + test_context.build_view_data(&mut module), + "{IndicatorColor}Committer: {Normal}John Doe " + ); + }, + ); } #[test] fn render_overview_with_committer_compact() { - with_temp_repository(|repository| { - let repo = Repository::from(repository); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - None, - |mut test_context| { - test_context.render_context.update(30, 300); - let diff = CommitDiffBuilder::new( + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |mut test_context| { + test_context.render_context.update(30, 300); + + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new( CommitBuilder::new("0123456789abcdef0123456789abcdef") .committer(User::new(Some("John Doe"), Some("john.doe@example.com"))) .build(), - ) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repo); - module.diff = Some(diff); - assert_rendered_output!( - Options render_options(), - Skip 1;2, - test_context.build_view_data(&mut module), - "{IndicatorColor}C: {Normal}John Doe " - ); - }, - ); - }); + ), + ); + + assert_rendered_output!( + Options render_options(), + Skip 1;2, + test_context.build_view_data(&mut module), + "{IndicatorColor}C: {Normal}John Doe " + ); + }, + ); } #[test] fn render_overview_with_commit_summary() { - with_temp_repository(|repository| { - let repo = Repository::from(repository); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - None, - |test_context| { - let diff = CommitDiffBuilder::new( + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new( CommitBuilder::new("0123456789abcdef0123456789abcdef") .summary("Commit title") .build(), - ) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repo); - module.diff = Some(diff); - assert_rendered_output!( - Options render_options(), - Skip 1;2, - test_context.build_view_data(&mut module), - "{Normal}Commit title" - ); - }, - ); - }); + ), + ); + + assert_rendered_output!( + Options render_options(), + Skip 1;2, + test_context.build_view_data(&mut module), + "{Normal}Commit title" + ); + }, + ); } #[test] fn render_overview_with_commit_body() { - with_temp_repository(|repository| { - let repo = Repository::from(repository); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - None, - |test_context| { - let diff = CommitDiffBuilder::new( + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new( CommitBuilder::new("0123456789abcdef0123456789abcdef") .message("Commit body") .build(), - ) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repo); - module.diff = Some(diff); - assert_rendered_output!( - Options render_options(), - Skip 1;2, - test_context.build_view_data(&mut module), - "{Normal}Commit body" - ); - }, - ); - }); + ), + ); + + assert_rendered_output!( + Options render_options(), + Skip 1;2, + test_context.build_view_data(&mut module), + "{Normal}Commit body" + ); + }, + ); } #[test] fn render_overview_with_commit_summary_and_body() { - with_temp_repository(|repository| { - let repo = Repository::from(repository); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - None, - |test_context| { - let diff = CommitDiffBuilder::new( + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new( CommitBuilder::new("0123456789abcdef0123456789abcdef") .summary("Commit title") .message("Commit body") .build(), - ) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repo); - module.diff = Some(diff); - assert_rendered_output!( - Options render_options(), - Skip 1;2, - test_context.build_view_data(&mut module), - "{Normal}Commit title", - "{Normal}", - "{Normal}Commit body" - ); - }, - ); - }); + ), + ); + + assert_rendered_output!( + Options render_options(), + Skip 1;2, + test_context.build_view_data(&mut module), + "{Normal}Commit title", + "{Normal}", + "{Normal}Commit body" + ); + }, + ); } #[test] fn render_overview_with_file_stats() { - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - None, - |test_context| { - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) - .file_statuses(vec![ + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()).file_statuses( + vec![ FileStatusBuilder::new() .source_path("file.1a") .destination_path("file.1b") @@ -396,41 +396,42 @@ fn render_overview_with_file_stats() { .destination_path("file.7a") .status(Status::Other) .build(), - ]) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module.diff = Some(diff); - assert_rendered_output!( - Options render_options(), - Skip 2, - test_context.build_view_data(&mut module), - "{IndicatorColor}0{Normal} files with {DiffAddColor}0{Normal} insertions and \ - {DiffRemoveColor}0{Normal} deletions", - "{DiffChangeColor} renamed: {DiffRemoveColor}file.1a{Normal} → {DiffAddColor}file.1b", - "{DiffAddColor} added: file.2a", - "{DiffRemoveColor} deleted: file.3a", - "{DiffAddColor} copied: {Normal}file.4a → {DiffAddColor}file.4b", - "{DiffChangeColor}modified: file.5a", - "{DiffChangeColor} changed: file.6a", - "{Normal} unknown: file.7a" - ); - }, - ); - }); + ], + ), + ); + + assert_rendered_output!( + Options render_options(), + Skip 2, + test_context.build_view_data(&mut module), + "{IndicatorColor}0{Normal} files with {DiffAddColor}0{Normal} insertions and \ + {DiffRemoveColor}0{Normal} deletions", + "{DiffChangeColor} renamed: {DiffRemoveColor}file.1a{Normal} → {DiffAddColor}file.1b", + "{DiffAddColor} added: file.2a", + "{DiffRemoveColor} deleted: file.3a", + "{DiffAddColor} copied: {Normal}file.4a → {DiffAddColor}file.4b", + "{DiffChangeColor}modified: file.5a", + "{DiffChangeColor} changed: file.6a", + "{Normal} unknown: file.7a" + ); + }, + ); } #[test] fn render_overview_with_file_stats_compact() { - with_temp_repository(|repository| { - let repo = Repository::from(repository); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - None, - |mut test_context| { - test_context.render_context.update(30, 300); - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) - .file_statuses(vec![ + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |mut test_context| { + test_context.render_context.update(30, 300); + + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()).file_statuses( + vec![ FileStatusBuilder::new() .source_path("file.1a") .destination_path("file.1b") @@ -468,264 +469,256 @@ fn render_overview_with_file_stats_compact() { .destination_path("file.7a") .status(Status::Other) .build(), - ]) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repo); - module.diff = Some(diff); - assert_rendered_output!( - Options render_options(), - Skip 2, - test_context.build_view_data(&mut module), - "{IndicatorColor}0{Normal} / {DiffAddColor}0{Normal} / {DiffRemoveColor}0", - "{DiffChangeColor}R {DiffRemoveColor}file.1a{Normal}→{DiffAddColor}file.1b", - "{DiffAddColor}A file.2a", - "{DiffRemoveColor}D file.3a", - "{DiffAddColor}C {Normal}file.4a→{DiffAddColor}file.4b", - "{DiffChangeColor}M file.5a", - "{DiffChangeColor}T file.6a", - "{Normal}X file.7a" - ); - }, - ); - }); -} + ], + ), + ); + assert_rendered_output!( + Options render_options(), + Skip 2, + test_context.build_view_data(&mut module), + "{IndicatorColor}0{Normal} / {DiffAddColor}0{Normal} / {DiffRemoveColor}0", + "{DiffChangeColor}R {DiffRemoveColor}file.1a{Normal}→{DiffAddColor}file.1b", + "{DiffAddColor}A file.2a", + "{DiffRemoveColor}D file.3a", + "{DiffAddColor}C {Normal}file.4a→{DiffAddColor}file.4b", + "{DiffChangeColor}M file.5a", + "{DiffChangeColor}T file.6a", + "{Normal}X file.7a" + ); + }, + ); +} #[test] fn render_overview_single_file_changed() { - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - None, - |test_context| { - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) - .number_files_changed(1) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module.diff = Some(diff); - assert_rendered_output!( - Options render_options(), - Skip 2, - test_context.build_view_data(&mut module), - "{IndicatorColor}1{Normal} file with {DiffAddColor}0{Normal} insertions and \ - {DiffRemoveColor}0{Normal} deletions" - ); - }, - ); - }); + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) + .number_files_changed(1), + ); + + assert_rendered_output!( + Options render_options(), + Skip 2, + test_context.build_view_data(&mut module), + "{IndicatorColor}1{Normal} file with {DiffAddColor}0{Normal} insertions and \ + {DiffRemoveColor}0{Normal} deletions" + ); + }, + ); } #[test] fn render_overview_more_than_one_file_changed() { - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - None, - |test_context| { - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) - .number_files_changed(2) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module.diff = Some(diff); - assert_rendered_output!( - Options render_options(), - Skip 1, - test_context.build_view_data(&mut module), - "{Normal}", - "{IndicatorColor}2{Normal} files with {DiffAddColor}0{Normal} insertions and \ - {DiffRemoveColor}0{Normal} deletions" - ); - }, - ); - }); + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) + .number_files_changed(2), + ); + + assert_rendered_output!( + Options render_options(), + Skip 1, + test_context.build_view_data(&mut module), + "{Normal}", + "{IndicatorColor}2{Normal} files with {DiffAddColor}0{Normal} insertions and \ + {DiffRemoveColor}0{Normal} deletions" + ); + }, + ); } #[test] fn render_overview_single_insertion() { - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - None, - |test_context| { - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) - .number_insertions(1) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module.diff = Some(diff); - assert_rendered_output!( - Options render_options(), - Skip 1, - test_context.build_view_data(&mut module), - "{Normal}", - "{IndicatorColor}0{Normal} files with {DiffAddColor}1{Normal} insertion and \ - {DiffRemoveColor}0{Normal} deletions" - ); - }, - ); - }); + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) + .number_insertions(1), + ); + + assert_rendered_output!( + Options render_options(), + Skip 1, + test_context.build_view_data(&mut module), + "{Normal}", + "{IndicatorColor}0{Normal} files with {DiffAddColor}1{Normal} insertion and \ + {DiffRemoveColor}0{Normal} deletions" + ); + }, + ); } #[test] fn render_overview_more_than_one_insertion() { - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - None, - |test_context| { - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) - .number_insertions(2) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module.diff = Some(diff); - assert_rendered_output!( - Options render_options(), - Skip 1, - test_context.build_view_data(&mut module), - "{Normal}", - "{IndicatorColor}0{Normal} files with {DiffAddColor}2{Normal} insertions and \ - {DiffRemoveColor}0{Normal} deletions" - ); - }, - ); - }); + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) + .number_insertions(2), + ); + + assert_rendered_output!( + Options render_options(), + Skip 1, + test_context.build_view_data(&mut module), + "{Normal}", + "{IndicatorColor}0{Normal} files with {DiffAddColor}2{Normal} insertions and \ + {DiffRemoveColor}0{Normal} deletions" + ); + }, + ); } #[test] fn render_overview_single_deletion() { - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - None, - |test_context| { - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) - .number_deletions(1) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module.diff = Some(diff); - assert_rendered_output!( - Options render_options(), - Skip 1, - test_context.build_view_data(&mut module), - "{Normal}", - "{IndicatorColor}0{Normal} files with {DiffAddColor}0{Normal} insertions and \ - {DiffRemoveColor}1{Normal} deletion" - ); - }, - ); - }); + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) + .number_deletions(1), + ); + + assert_rendered_output!( + Options render_options(), + Skip 1, + test_context.build_view_data(&mut module), + "{Normal}", + "{IndicatorColor}0{Normal} files with {DiffAddColor}0{Normal} insertions and \ + {DiffRemoveColor}1{Normal} deletion" + ); + }, + ); } #[test] fn render_overview_more_than_one_deletion() { - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - None, - |test_context| { - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) - .number_deletions(2) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module.diff = Some(diff); - assert_rendered_output!( - Options render_options(), - Skip 1, - test_context.build_view_data(&mut module), - "{Normal}", - "{IndicatorColor}0{Normal} files with {DiffAddColor}0{Normal} insertions and \ - {DiffRemoveColor}2{Normal} deletions" - ); - }, - ); - }); + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) + .number_deletions(2), + ); + + assert_rendered_output!( + Options render_options(), + Skip 1, + test_context.build_view_data(&mut module), + "{Normal}", + "{IndicatorColor}0{Normal} files with {DiffAddColor}0{Normal} insertions and \ + {DiffRemoveColor}2{Normal} deletions" + ); + }, + ); } #[test] fn render_diff_minimal_commit() { let mut config = create_config(); config.diff_show_whitespace = DiffShowWhitespaceSetting::None; - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - Some(config), - |test_context| { - let diff = - CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()).build(); - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module.diff = Some(diff); - module.state = ShowCommitState::Diff; - assert_rendered_output!( - Options AssertRenderOptions::INCLUDE_STYLE, - test_context.build_view_data(&mut module), - "{TITLE}{HELP}", - "{LEADING}", - "{IndicatorColor}Commit: {Normal}0123456789abcdef0123456789abcdef", - "{IndicatorColor}0{Normal} files with {DiffAddColor}0{Normal} insertions and \ - {DiffRemoveColor}0{Normal} deletions", - "{BODY}", - "{Normal}{Pad(―)}" - ); - }, - ); - }); + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + Some(config), + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()), + ); + + module.state = ShowCommitState::Diff; + assert_rendered_output!( + Options AssertRenderOptions::INCLUDE_STYLE, + test_context.build_view_data(&mut module), + "{TITLE}{HELP}", + "{LEADING}", + "{IndicatorColor}Commit: {Normal}0123456789abcdef0123456789abcdef", + "{IndicatorColor}0{Normal} files with {DiffAddColor}0{Normal} insertions and \ + {DiffRemoveColor}0{Normal} deletions", + "{BODY}", + "{Normal}{Pad(―)}" + ); + }, + ); } #[test] fn render_diff_minimal_commit_compact() { let mut config = create_config(); config.diff_show_whitespace = DiffShowWhitespaceSetting::None; - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - Some(config), - |mut test_context| { - test_context.render_context.update(30, 300); - let diff = - CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()).build(); - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module.diff = Some(diff); - module.state = ShowCommitState::Diff; - assert_rendered_output!( - Options AssertRenderOptions::INCLUDE_STYLE, - test_context.build_view_data(&mut module), - "{TITLE}{HELP}", - "{LEADING}", - "{Normal}01234567", - "{IndicatorColor}0{Normal} / {DiffAddColor}0{Normal} / {DiffRemoveColor}0", - "{BODY}", - "{Normal}{Pad(―)}" - ); - }, - ); - }); + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + Some(config), + |mut test_context| { + test_context.render_context.update(30, 300); + + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()), + ); + + module.state = ShowCommitState::Diff; + assert_rendered_output!( + Options AssertRenderOptions::INCLUDE_STYLE, + test_context.build_view_data(&mut module), + "{TITLE}{HELP}", + "{LEADING}", + "{Normal}01234567", + "{IndicatorColor}0{Normal} / {DiffAddColor}0{Normal} / {DiffRemoveColor}0", + "{BODY}", + "{Normal}{Pad(―)}" + ); + }, + ); } #[test] fn render_diff_basic_file_stats() { - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - None, - |test_context| { - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) - .file_statuses(vec![ + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()).file_statuses( + vec![ FileStatusBuilder::new() .source_path("file.1a") .destination_path("file.1b") @@ -762,228 +755,227 @@ fn render_diff_basic_file_stats() { .destination_path("file.7a") .status(Status::Other) .build(), - ]) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module.diff = Some(diff); - module.state = ShowCommitState::Diff; - assert_rendered_output!( - Options AssertRenderOptions::INCLUDE_STYLE, - Skip 3, - test_context.build_view_data(&mut module), - "{IndicatorColor}0{Normal} files with {DiffAddColor}0{Normal} insertions and \ - {DiffRemoveColor}0{Normal} deletions", - "{BODY}", - "{Normal}{Pad(―)}", - "{DiffChangeColor} renamed: {DiffRemoveColor}file.1a{Normal} → {DiffAddColor}file.1b", - "{Normal}{Pad(―)}", - "{DiffAddColor} added: file.2a", - "{Normal}{Pad(―)}", - "{DiffRemoveColor} deleted: file.3a", - "{Normal}{Pad(―)}", - "{DiffAddColor} copied: {Normal}file.4a → {DiffAddColor}file.4b", - "{Normal}{Pad(―)}", - "{DiffChangeColor}modified: file.5a", - "{Normal}{Pad(―)}", - "{DiffChangeColor} changed: file.6a", - "{Normal}{Pad(―)}", - "{Normal} unknown: file.7a" - ); - }, - ); - }); + ], + ), + ); + + module.state = ShowCommitState::Diff; + assert_rendered_output!( + Options AssertRenderOptions::INCLUDE_STYLE, + Skip 3, + test_context.build_view_data(&mut module), + "{IndicatorColor}0{Normal} files with {DiffAddColor}0{Normal} insertions and \ + {DiffRemoveColor}0{Normal} deletions", + "{BODY}", + "{Normal}{Pad(―)}", + "{DiffChangeColor} renamed: {DiffRemoveColor}file.1a{Normal} → {DiffAddColor}file.1b", + "{Normal}{Pad(―)}", + "{DiffAddColor} added: file.2a", + "{Normal}{Pad(―)}", + "{DiffRemoveColor} deleted: file.3a", + "{Normal}{Pad(―)}", + "{DiffAddColor} copied: {Normal}file.4a → {DiffAddColor}file.4b", + "{Normal}{Pad(―)}", + "{DiffChangeColor}modified: file.5a", + "{Normal}{Pad(―)}", + "{DiffChangeColor} changed: file.6a", + "{Normal}{Pad(―)}", + "{Normal} unknown: file.7a" + ); + }, + ); } #[test] fn render_diff_end_new_line_missing() { let mut config = create_config(); config.diff_show_whitespace = DiffShowWhitespaceSetting::None; - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - Some(config), - |test_context| { - let mut delta = Delta::new("@@ -14,2 +13,3 @@ context", 14, 14, 0, 1); - delta.add_line(DiffLine::new(Origin::Addition, "new line", None, Some(14), false)); - delta.add_line(DiffLine::new(Origin::Addition, "", None, Some(15), true)); - - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) - .file_statuses(vec![ + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + Some(config), + |test_context| { + let mut delta = Delta::new("@@ -14,2 +13,3 @@ context", 14, 14, 0, 1); + delta.add_line(DiffLine::new(Origin::Addition, "new line", None, Some(14), false)); + delta.add_line(DiffLine::new(Origin::Addition, "", None, Some(15), true)); + + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()).file_statuses( + vec![ FileStatusBuilder::new() .source_path("file.txt") .destination_path("file.txt") .status(Status::Modified) .push_delta(delta) .build(), - ]) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module.diff = Some(diff); - module.state = ShowCommitState::Diff; - assert_rendered_output!( - Options AssertRenderOptions::INCLUDE_STYLE, - Skip 3, - test_context.build_view_data(&mut module), - "{IndicatorColor}0{Normal} files with {DiffAddColor}0{Normal} insertions and \ - {DiffRemoveColor}0{Normal} deletions", - "{BODY}", - "{Normal}{Pad(―)}", - "{DiffChangeColor}modified: file.txt", - "", - "{Normal,Dimmed}@@{DiffContextColor} -14,0 +14,1 {Normal,Dimmed}@@{DiffContextColor} context", - "{Normal,Dimmed}{Pad(―)}", - "{Normal} 14| {DiffAddColor}new line", - "{Normal} {DiffContextColor}\\ No newline at end of file" - ); - }, - ); - }); + ], + ), + ); + + module.state = ShowCommitState::Diff; + assert_rendered_output!( + Options AssertRenderOptions::INCLUDE_STYLE, + Skip 3, + test_context.build_view_data(&mut module), + "{IndicatorColor}0{Normal} files with {DiffAddColor}0{Normal} insertions and \ + {DiffRemoveColor}0{Normal} deletions", + "{BODY}", + "{Normal}{Pad(―)}", + "{DiffChangeColor}modified: file.txt", + "", + "{Normal,Dimmed}@@{DiffContextColor} -14,0 +14,1 {Normal,Dimmed}@@{DiffContextColor} context", + "{Normal,Dimmed}{Pad(―)}", + "{Normal} 14| {DiffAddColor}new line", + "{Normal} {DiffContextColor}\\ No newline at end of file" + ); + }, + ); } #[test] fn render_diff_add_line() { let mut config = create_config(); config.diff_show_whitespace = DiffShowWhitespaceSetting::None; - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - Some(config), - |test_context| { - let mut delta = Delta::new("@@ -14,2 +13,3 @@ context", 14, 14, 0, 1); - delta.add_line(DiffLine::new(Origin::Addition, "new line", None, Some(14), false)); - - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) - .file_statuses(vec![ + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + Some(config), + |test_context| { + let mut delta = Delta::new("@@ -14,2 +13,3 @@ context", 14, 14, 0, 1); + delta.add_line(DiffLine::new(Origin::Addition, "new line", None, Some(14), false)); + + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()).file_statuses( + vec![ FileStatusBuilder::new() .source_path("file.txt") .destination_path("file.txt") .status(Status::Modified) .push_delta(delta) .build(), - ]) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module.diff = Some(diff); - module.state = ShowCommitState::Diff; - assert_rendered_output!( - Options AssertRenderOptions::INCLUDE_STYLE, - Skip 3, - test_context.build_view_data(&mut module), - "{IndicatorColor}0{Normal} files with {DiffAddColor}0{Normal} insertions and \ - {DiffRemoveColor}0{Normal} deletions", - "{BODY}", - "{Normal}{Pad(―)}", - "{DiffChangeColor}modified: file.txt", - "", - "{Normal,Dimmed}@@{DiffContextColor} -14,0 +14,1 {Normal,Dimmed}@@{DiffContextColor} context", - "{Normal,Dimmed}{Pad(―)}", - "{Normal} 14| {DiffAddColor}new line" - ); - }, - ); - }); + ], + ), + ); + + module.state = ShowCommitState::Diff; + assert_rendered_output!( + Options AssertRenderOptions::INCLUDE_STYLE, + Skip 3, + test_context.build_view_data(&mut module), + "{IndicatorColor}0{Normal} files with {DiffAddColor}0{Normal} insertions and \ + {DiffRemoveColor}0{Normal} deletions", + "{BODY}", + "{Normal}{Pad(―)}", + "{DiffChangeColor}modified: file.txt", + "", + "{Normal,Dimmed}@@{DiffContextColor} -14,0 +14,1 {Normal,Dimmed}@@{DiffContextColor} context", + "{Normal,Dimmed}{Pad(―)}", + "{Normal} 14| {DiffAddColor}new line" + ); + }, + ); } #[test] fn render_diff_delete_line() { let mut config = create_config(); config.diff_show_whitespace = DiffShowWhitespaceSetting::None; - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - Some(config), - |test_context| { - let mut delta = Delta::new("@@ -14,2 +13,3 @@ context", 14, 14, 0, 1); - delta.add_line(DiffLine::new(Origin::Deletion, "old line", Some(14), None, false)); - - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) - .file_statuses(vec![ + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + Some(config), + |test_context| { + let mut delta = Delta::new("@@ -14,2 +13,3 @@ context", 14, 14, 0, 1); + delta.add_line(DiffLine::new(Origin::Deletion, "old line", Some(14), None, false)); + + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()).file_statuses( + vec![ FileStatusBuilder::new() .source_path("file.txt") .destination_path("file.txt") .status(Status::Modified) .push_delta(delta) .build(), - ]) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module.diff = Some(diff); - module.state = ShowCommitState::Diff; - assert_rendered_output!( - Options AssertRenderOptions::INCLUDE_STYLE, - Skip 3, - test_context.build_view_data(&mut module), - "{IndicatorColor}0{Normal} files with {DiffAddColor}0{Normal} insertions and \ - {DiffRemoveColor}0{Normal} deletions", - "{BODY}", - "{Normal}{Pad(―)}", - "{DiffChangeColor}modified: file.txt", - "", - "{Normal,Dimmed}@@{DiffContextColor} -14,0 +14,1 {Normal,Dimmed}@@{DiffContextColor} context", - "{Normal,Dimmed}{Pad(―)}", - "{Normal}14 | {DiffRemoveColor}old line" - ); - }, - ); - }); + ], + ), + ); + + module.state = ShowCommitState::Diff; + assert_rendered_output!( + Options AssertRenderOptions::INCLUDE_STYLE, + Skip 3, + test_context.build_view_data(&mut module), + "{IndicatorColor}0{Normal} files with {DiffAddColor}0{Normal} insertions and \ + {DiffRemoveColor}0{Normal} deletions", + "{BODY}", + "{Normal}{Pad(―)}", + "{DiffChangeColor}modified: file.txt", + "", + "{Normal,Dimmed}@@{DiffContextColor} -14,0 +14,1 {Normal,Dimmed}@@{DiffContextColor} context", + "{Normal,Dimmed}{Pad(―)}", + "{Normal}14 | {DiffRemoveColor}old line" + ); + }, + ); } #[test] fn render_diff_context_add_remove_lines() { let mut config = create_config(); config.diff_show_whitespace = DiffShowWhitespaceSetting::None; - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - Some(config), - |test_context| { - let mut delta = Delta::new("@@ -14,2 +13,3 @@ context", 14, 14, 0, 1); - delta.add_line(DiffLine::new(Origin::Context, "context 1", Some(13), Some(13), false)); - delta.add_line(DiffLine::new(Origin::Deletion, "old line", Some(14), None, false)); - delta.add_line(DiffLine::new(Origin::Addition, "new line", None, Some(14), false)); - delta.add_line(DiffLine::new(Origin::Context, "context 2", Some(15), Some(15), false)); - - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) - .file_statuses(vec![ + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + Some(config), + |test_context| { + let mut delta = Delta::new("@@ -14,2 +13,3 @@ context", 14, 14, 0, 1); + delta.add_line(DiffLine::new(Origin::Context, "context 1", Some(13), Some(13), false)); + delta.add_line(DiffLine::new(Origin::Deletion, "old line", Some(14), None, false)); + delta.add_line(DiffLine::new(Origin::Addition, "new line", None, Some(14), false)); + delta.add_line(DiffLine::new(Origin::Context, "context 2", Some(15), Some(15), false)); + + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()).file_statuses( + vec![ FileStatusBuilder::new() .source_path("file.txt") .destination_path("file.txt") .status(Status::Modified) .push_delta(delta) .build(), - ]) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module.diff = Some(diff); - module.state = ShowCommitState::Diff; - assert_rendered_output!( - Options AssertRenderOptions::INCLUDE_STYLE, - Skip 3, - test_context.build_view_data(&mut module), - "{IndicatorColor}0{Normal} files with {DiffAddColor}0{Normal} insertions and \ - {DiffRemoveColor}0{Normal} deletions", - "{BODY}", - "{Normal}{Pad(―)}", - "{DiffChangeColor}modified: file.txt", - "", - "{Normal,Dimmed}@@{DiffContextColor} -14,0 +14,1 {Normal,Dimmed}@@{DiffContextColor} context", - "{Normal,Dimmed}{Pad(―)}", - "{Normal}13 13| {DiffContextColor}context 1", - "{Normal}14 | {DiffRemoveColor}old line", - "{Normal} 14| {DiffAddColor}new line", - "{Normal}15 15| {DiffContextColor}context 2" - ); - }, - ); - }); + ], + ), + ); + + module.state = ShowCommitState::Diff; + assert_rendered_output!( + Options AssertRenderOptions::INCLUDE_STYLE, + Skip 3, + test_context.build_view_data(&mut module), + "{IndicatorColor}0{Normal} files with {DiffAddColor}0{Normal} insertions and \ + {DiffRemoveColor}0{Normal} deletions", + "{BODY}", + "{Normal}{Pad(―)}", + "{DiffChangeColor}modified: file.txt", + "", + "{Normal,Dimmed}@@{DiffContextColor} -14,0 +14,1 {Normal,Dimmed}@@{DiffContextColor} context", + "{Normal,Dimmed}{Pad(―)}", + "{Normal}13 13| {DiffContextColor}context 1", + "{Normal}14 | {DiffRemoveColor}old line", + "{Normal} 14| {DiffAddColor}new line", + "{Normal}15 15| {DiffContextColor}context 2" + ); + }, + ); } fn generate_diff_line_context(content: &str, line_num: u32) -> DiffLine { @@ -1016,41 +1008,41 @@ fn render_diff_show_both_whitespace() { config.diff_tab_symbol = String::from("#>"); config.diff_space_symbol = String::from("%"); config.diff_tab_width = 2; - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - Some(config), - |test_context| { - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) - .file_statuses(vec![ + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + Some(config), + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()).file_statuses( + vec![ FileStatusBuilder::new() .source_path("file.txt") .destination_path("file.txt") .status(Status::Modified) .push_delta(generate_white_space_delta()) .build(), - ]) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module.diff = Some(diff); - module.state = ShowCommitState::Diff; - assert_rendered_output!( - Options AssertRenderOptions::INCLUDE_STYLE, - Skip 10, - test_context.build_view_data(&mut module), - render_line!(EndsWith "#>#>{DiffContextColor}sp tabs content"), - render_line!(EndsWith "sp tabs content{DiffWhitespaceColor}#>#>"), - render_line!(EndsWith "#>#>{DiffContextColor}sp tabs content{DiffWhitespaceColor}#>#>"), - render_line!(EndsWith "%%%%{DiffContextColor}sp tabs content"), - render_line!(EndsWith "sp tabs content{DiffWhitespaceColor}%%%%"), - render_line!(EndsWith "%%%%{DiffContextColor}sp tabs content{DiffWhitespaceColor}%%%%"), - render_line!(EndsWith "%#>#>%{DiffContextColor}sp tabs content{DiffWhitespaceColor}#>%%#>") - ); - }, - ); - }); + ], + ), + ); + + module.state = ShowCommitState::Diff; + assert_rendered_output!( + Options AssertRenderOptions::INCLUDE_STYLE, + Skip 10, + test_context.build_view_data(&mut module), + render_line!(EndsWith "#>#>{DiffContextColor}sp tabs content"), + render_line!(EndsWith "sp tabs content{DiffWhitespaceColor}#>#>"), + render_line!(EndsWith "#>#>{DiffContextColor}sp tabs content{DiffWhitespaceColor}#>#>"), + render_line!(EndsWith "%%%%{DiffContextColor}sp tabs content"), + render_line!(EndsWith "sp tabs content{DiffWhitespaceColor}%%%%"), + render_line!(EndsWith "%%%%{DiffContextColor}sp tabs content{DiffWhitespaceColor}%%%%"), + render_line!(EndsWith "%#>#>%{DiffContextColor}sp tabs content{DiffWhitespaceColor}#>%%#>") + ); + }, + ); } #[test] @@ -1060,41 +1052,41 @@ fn render_diff_show_leading_whitespace() { config.diff_tab_symbol = String::from("#>"); config.diff_space_symbol = String::from("%"); config.diff_tab_width = 2; - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - Some(config), - |test_context| { - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) - .file_statuses(vec![ + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + Some(config), + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()).file_statuses( + vec![ FileStatusBuilder::new() .source_path("file.txt") .destination_path("file.txt") .status(Status::Modified) .push_delta(generate_white_space_delta()) .build(), - ]) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module.diff = Some(diff); - module.state = ShowCommitState::Diff; - assert_rendered_output!( - Options AssertRenderOptions::INCLUDE_STYLE, - Skip 10, - test_context.build_view_data(&mut module), - render_line!(EndsWith "#>#>{DiffContextColor}sp tabs content"), - render_line!(EndsWith "sp tabs content{DiffWhitespaceColor}"), - render_line!(EndsWith "#>#>{DiffContextColor}sp tabs content{DiffWhitespaceColor}"), - render_line!(EndsWith "%%%%{DiffContextColor}sp tabs content"), - render_line!(EndsWith "sp tabs content{DiffWhitespaceColor}"), - render_line!(EndsWith "%%%%{DiffContextColor}sp tabs content{DiffWhitespaceColor}"), - render_line!(EndsWith "%#>#>%{DiffContextColor}sp tabs content{DiffWhitespaceColor}") - ); - }, - ); - }); + ], + ), + ); + + module.state = ShowCommitState::Diff; + assert_rendered_output!( + Options AssertRenderOptions::INCLUDE_STYLE, + Skip 10, + test_context.build_view_data(&mut module), + render_line!(EndsWith "#>#>{DiffContextColor}sp tabs content"), + render_line!(EndsWith "sp tabs content{DiffWhitespaceColor}"), + render_line!(EndsWith "#>#>{DiffContextColor}sp tabs content{DiffWhitespaceColor}"), + render_line!(EndsWith "%%%%{DiffContextColor}sp tabs content"), + render_line!(EndsWith "sp tabs content{DiffWhitespaceColor}"), + render_line!(EndsWith "%%%%{DiffContextColor}sp tabs content{DiffWhitespaceColor}"), + render_line!(EndsWith "%#>#>%{DiffContextColor}sp tabs content{DiffWhitespaceColor}") + ); + }, + ); } #[test] @@ -1104,41 +1096,41 @@ fn render_diff_show_no_whitespace() { config.diff_tab_symbol = String::from("#>"); config.diff_space_symbol = String::from("%"); config.diff_tab_width = 2; - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - Some(config), - |test_context| { - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) - .file_statuses(vec![ + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + Some(config), + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()).file_statuses( + vec![ FileStatusBuilder::new() .source_path("file.txt") .destination_path("file.txt") .status(Status::Modified) .push_delta(generate_white_space_delta()) .build(), - ]) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module.diff = Some(diff); - module.state = ShowCommitState::Diff; - assert_rendered_output!( - Options AssertRenderOptions::INCLUDE_STYLE, - Skip 10, - test_context.build_view_data(&mut module), - render_line!(EndsWith " sp tabs content"), - render_line!(EndsWith "sp tabs content"), - render_line!(EndsWith " sp tabs content"), - render_line!(EndsWith " sp tabs content"), - render_line!(EndsWith "sp tabs content"), - render_line!(EndsWith " sp tabs content"), - render_line!(EndsWith " sp tabs content") - ); - }, - ); - }); + ], + ), + ); + + module.state = ShowCommitState::Diff; + assert_rendered_output!( + Options AssertRenderOptions::INCLUDE_STYLE, + Skip 10, + test_context.build_view_data(&mut module), + render_line!(EndsWith " sp tabs content"), + render_line!(EndsWith "sp tabs content"), + render_line!(EndsWith " sp tabs content"), + render_line!(EndsWith " sp tabs content"), + render_line!(EndsWith "sp tabs content"), + render_line!(EndsWith " sp tabs content"), + render_line!(EndsWith " sp tabs content") + ); + }, + ); } #[test] @@ -1148,210 +1140,357 @@ fn render_diff_show_whitespace_all_spaces() { config.diff_tab_symbol = String::from("#>"); config.diff_space_symbol = String::from("%"); config.diff_tab_width = 2; - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef comment1"], - &[], - Some(config), - |test_context| { - let mut delta = Delta::new("@@ -1,7 +1,7 @@ context", 1, 1, 7, 7); - delta.add_line(DiffLine::new(Origin::Addition, " ", None, Some(1), false)); - let diff = CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()) - .file_statuses(vec![ + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + Some(config), + |test_context| { + let mut delta = Delta::new("@@ -1,7 +1,7 @@ context", 1, 1, 7, 7); + delta.add_line(DiffLine::new(Origin::Addition, " ", None, Some(1), false)); + + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()).file_statuses( + vec![ FileStatusBuilder::new() .source_path("file.txt") .destination_path("file.txt") .status(Status::Modified) .push_delta(delta) .build(), - ]) - .build(); - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module.diff = Some(diff); - module.state = ShowCommitState::Diff; - assert_rendered_output!( - Options AssertRenderOptions::INCLUDE_STYLE, - Skip 10, - test_context.build_view_data(&mut module), - "{Normal} 1| {DiffWhitespaceColor}%%%%" - ); - }, - ); - }); + ], + ), + ); + + module.state = ShowCommitState::Diff; + assert_rendered_output!( + Options AssertRenderOptions::INCLUDE_STYLE, + Skip 10, + test_context.build_view_data(&mut module), + "{Normal} 1| {DiffWhitespaceColor}%%%%" + ); + }, + ); +} + +#[test] +fn render_loading_new() { + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()), + ); + + test_context.app_data().diff_state().set_load_status(LoadStatus::New); + + assert_rendered_output!( + Options AssertRenderOptions::INCLUDE_STYLE, + Skip 2, + test_context.build_view_data(&mut module), + "{IndicatorColor}Loading Diff" + ); + }, + ); +} + +#[test] +fn render_loading_quick_diff_progress() { + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()), + ); + + test_context + .app_data() + .diff_state() + .set_load_status(LoadStatus::QuickDiff(5, 12)); + + assert_rendered_output!( + Options AssertRenderOptions::INCLUDE_STYLE, + Skip 8, + test_context.build_view_data(&mut module), + "{IndicatorColor}Loading Diff (-) [5/12]" + ); + }, + ); +} + +#[test] +fn render_loading_quick_diff_complete() { + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()), + ); + + test_context + .app_data() + .diff_state() + .set_load_status(LoadStatus::CompleteQuickDiff); + + assert_rendered_output!( + Options AssertRenderOptions::INCLUDE_STYLE, + Skip 8, + test_context.build_view_data(&mut module), + "{IndicatorColor}Detecting renames and copies" + ); + }, + ); +} + +#[test] +fn render_loading_diff_progress() { + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()), + ); + + test_context + .app_data() + .diff_state() + .set_load_status(LoadStatus::Diff(7, 12)); + + assert_rendered_output!( + Options AssertRenderOptions::INCLUDE_STYLE, + Skip 8, + test_context.build_view_data(&mut module), + "{IndicatorColor}Detecting renames and copies (-) [7/12]" + ); + }, + ); +} + +#[test] +fn render_loading_diff_error_not_found() { + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()), + ); + + test_context.app_data().diff_state().set_load_status(LoadStatus::Error { + msg: String::from("message"), + code: ErrorCode::NotFound, + }); + + assert_rendered_output!( + Options AssertRenderOptions::INCLUDE_STYLE, + Skip 2, + test_context.build_view_data(&mut module), + "{IndicatorColor}Error loading diff. Press any key to return.", + "{Normal}", + "{Normal}Reason:", + "{Normal}Commit not found" + ); + }, + ); +} + +#[test] +fn render_loading_diff_error_other() { + testers::module( + &["pick 0123456789abcdef0123456789abcdef comment1"], + &[], + None, + |test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + update_diff( + &mut module, + CommitDiffBuilder::new(CommitBuilder::new("0123456789abcdef0123456789abcdef").build()), + ); + + test_context.app_data().diff_state().set_load_status(LoadStatus::Error { + msg: String::from("message"), + code: ErrorCode::GenericError, + }); + + assert_rendered_output!( + Options AssertRenderOptions::INCLUDE_STYLE, + Skip 2, + test_context.build_view_data(&mut module), + "{IndicatorColor}Error loading diff. Press any key to return.", + "{Normal}", + "{Normal}Reason:", + "{Normal}message" + ); + }, + ); } #[test] fn handle_event_toggle_diff_to_overview() { - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef c1"], - &[Event::from(StandardEvent::ShowDiff)], - None, - |mut test_context| { - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module - .diff_view_data - .update_view_data(|updater| updater.push_line(ViewLine::from("foo"))); - module.state = ShowCommitState::Diff; - assert_results!( - test_context.handle_event(&mut module), - Artifact::Event(Event::from(StandardEvent::ShowDiff)) - ); - assert!(module.diff_view_data.is_empty()); - assert_eq!(module.state, ShowCommitState::Overview); - }, - ); - }); + testers::module( + &["pick 0123456789abcdef0123456789abcdef c1"], + &[Event::from(StandardEvent::ShowDiff)], + None, + |mut test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + module + .diff_view_data + .update_view_data(|updater| updater.push_line(ViewLine::from("foo"))); + module.state = ShowCommitState::Diff; + assert_results!( + test_context.handle_event(&mut module), + Artifact::Event(Event::from(StandardEvent::ShowDiff)) + ); + assert_eq!(module.state, ShowCommitState::Overview); + }, + ); } #[test] fn handle_event_toggle_overview_to_diff() { - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef c1"], - &[Event::from('d')], - None, - |mut test_context| { - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module - .overview_view_data - .update_view_data(|updater| updater.push_line(ViewLine::from("foo"))); - module.state = ShowCommitState::Overview; - assert_results!( - test_context.handle_event(&mut module), - Artifact::Event(Event::from(StandardEvent::ShowDiff)) - ); - assert!(module.diff_view_data.is_empty()); - assert_eq!(module.state, ShowCommitState::Diff); - }, - ); - }); + testers::module( + &["pick 0123456789abcdef0123456789abcdef c1"], + &[Event::from('d')], + None, + |mut test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + module + .overview_view_data + .update_view_data(|updater| updater.push_line(ViewLine::from("foo"))); + module.state = ShowCommitState::Overview; + assert_results!( + test_context.handle_event(&mut module), + Artifact::Event(Event::from(StandardEvent::ShowDiff)) + ); + assert_eq!(module.state, ShowCommitState::Diff); + }, + ); } #[test] fn handle_event_resize() { - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef c1"], - &[Event::Resize(100, 100)], - None, - |mut test_context| { - let mut module = ShowCommit::new(&test_context.app_data(), repository); - assert_results!( - test_context.handle_event(&mut module), - Artifact::Event(Event::Resize(100, 100)) - ); - }, - ); - }); + testers::module( + &["pick 0123456789abcdef0123456789abcdef c1"], + &[Event::Resize(100, 100)], + None, + |mut test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + assert_results!( + test_context.handle_event(&mut module), + Artifact::Event(Event::Resize(100, 100)) + ); + }, + ); } #[test] fn render_help() { - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick aaa c1"], - &[Event::from(StandardEvent::Help)], - None, - |mut test_context| { - let mut module = ShowCommit::new(&test_context.app_data(), repository); - _ = test_context.handle_all_events(&mut module); - assert_rendered_output!( - Body test_context.build_view_data(&mut module), - " Up |Scroll up", - " Down |Scroll down", - " PageUp |Scroll up half a page", - " PageDown|Scroll down half a page", - " Home |Scroll to the top", - " End |Scroll to the bottom", - " Right |Scroll right", - " Left |Scroll left", - " d |Show full diff", - " ? |Show help" - ); - }, - ); - }); + testers::module( + &["pick aaa c1"], + &[Event::from(StandardEvent::Help)], + None, + |mut test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + _ = test_context.handle_all_events(&mut module); + assert_rendered_output!( + Body test_context.build_view_data(&mut module), + " Up |Scroll up", + " Down |Scroll down", + " PageUp |Scroll up half a page", + " PageDown|Scroll down half a page", + " Home |Scroll to the top", + " End |Scroll to the bottom", + " Right |Scroll right", + " Left |Scroll left", + " d |Show full diff", + " ? |Show help" + ); + }, + ); } #[test] fn handle_help_event_show() { - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick aaa c1"], - &[Event::from(StandardEvent::Help)], - None, - |mut test_context| { - let mut module = ShowCommit::new(&test_context.app_data(), repository); - _ = test_context.handle_all_events(&mut module); - assert!(module.help.is_active()); - }, - ); - }); + testers::module( + &["pick aaa c1"], + &[Event::from(StandardEvent::Help)], + None, + |mut test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + _ = test_context.handle_all_events(&mut module); + assert!(module.help.is_active()); + }, + ); } #[test] fn handle_help_event_hide() { - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick aaa c1"], - &[Event::from(StandardEvent::Help), Event::from('?')], - None, - |mut test_context| { - let mut module = ShowCommit::new(&test_context.app_data(), repository); - _ = test_context.handle_all_events(&mut module); - assert!(!module.help.is_active()); - }, - ); - }); + testers::module( + &["pick aaa c1"], + &[Event::from(StandardEvent::Help), Event::from('?')], + None, + |mut test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + _ = test_context.handle_all_events(&mut module); + assert!(!module.help.is_active()); + }, + ); } #[test] fn handle_event_other_key_from_diff() { - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef c1"], - &[Event::from('a')], - None, - |mut test_context| { - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module.state = ShowCommitState::Diff; - assert_results!( - test_context.handle_event(&mut module), - Artifact::Event(Event::from('a')) - ); - assert_eq!(module.state, ShowCommitState::Overview); - }, - ); - }); + testers::module( + &["pick 0123456789abcdef0123456789abcdef c1"], + &[Event::from('a')], + None, + |mut test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + module.state = ShowCommitState::Diff; + assert_results!( + test_context.handle_event(&mut module), + Artifact::Event(Event::from('a')) + ); + assert_eq!(module.state, ShowCommitState::Overview); + }, + ); } #[test] fn handle_event_other_key_from_overview() { - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module( - &["pick 0123456789abcdef0123456789abcdef c1"], - &[Event::from('a')], - None, - |mut test_context| { - let mut module = ShowCommit::new(&test_context.app_data(), repository); - module.state = ShowCommitState::Overview; - assert_results!( - test_context.handle_event(&mut module), - Artifact::Event(Event::from('a')), - Artifact::ChangeState(State::List) - ); - }, - ); - }); + testers::module( + &["pick 0123456789abcdef0123456789abcdef c1"], + &[Event::from('a')], + None, + |mut test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + module.state = ShowCommitState::Overview; + assert_results!( + test_context.handle_event(&mut module), + Artifact::Event(Event::from('a')), + Artifact::CancelDiff, + Artifact::ChangeState(State::List) + ); + }, + ); } #[rstest] @@ -1362,14 +1501,11 @@ fn handle_event_other_key_from_overview() { #[case::scroll_jump_down(StandardEvent::ScrollJumpDown)] #[case::scroll_jump_up(StandardEvent::ScrollJumpUp)] fn scroll_events(#[case] event: StandardEvent) { - with_temp_repository(|repo| { - let repository = Repository::from(repo); - testers::module(&[], &[Event::from(event)], None, |mut test_context| { - let mut module = ShowCommit::new(&test_context.app_data(), repository); - assert_results!( - test_context.handle_event(&mut module), - Artifact::Event(Event::from(event)) - ); - }); + testers::module(&[], &[Event::from(event)], None, |mut test_context| { + let mut module = ShowCommit::new(&test_context.app_data()); + assert_results!( + test_context.handle_event(&mut module), + Artifact::Event(Event::from(event)) + ); }); } diff --git a/src/modules/show_commit/view_builder.rs b/src/modules/show_commit/view_builder.rs index e88d037f0..f72c49fe7 100644 --- a/src/modules/show_commit/view_builder.rs +++ b/src/modules/show_commit/view_builder.rs @@ -1,5 +1,8 @@ +use git2::ErrorCode; + use crate::{ - diff::{Commit, CommitDiff, DiffLine, Origin}, + components::spin_indicator::SpinIndicator, + diff::{Commit, CommitDiff, DiffLine, Origin, thread::LoadStatus}, display::DisplayColor, modules::show_commit::util::{ get_files_changed_summary, @@ -43,6 +46,7 @@ pub(super) struct ViewBuilder { visible_space_string: String, show_leading_whitespace: bool, show_trailing_whitespace: bool, + spin_indicator: SpinIndicator, } impl ViewBuilder { @@ -53,6 +57,7 @@ impl ViewBuilder { visible_space_string: options.space_character, show_leading_whitespace: options.show_leading_whitespace, show_trailing_whitespace: options.show_trailing_whitespace, + spin_indicator: SpinIndicator::new(), } } @@ -91,12 +96,69 @@ impl ViewBuilder { ViewLine::from(segments) } + fn build_progress(spin: &mut SpinIndicator, msg: &str, progress: Option<(usize, usize)>) -> ViewLine { + if let Some((c, t)) = progress { + spin.refresh(); + ViewLine::from(LineSegment::new_with_color( + format!("{msg} {} [{}/{}]", spin.indicator(), c, t).as_str(), + DisplayColor::IndicatorColor, + )) + } + else { + ViewLine::from(LineSegment::new_with_color(msg, DisplayColor::IndicatorColor)) + } + } + + fn build_loading_status(&mut self, updater: &mut ViewDataUpdater<'_>, load_status: &LoadStatus) -> bool { + if load_status == &LoadStatus::DiffComplete { + return true; + } + + updater.push_trailing_line(ViewBuilder::build_progress( + &mut self.spin_indicator, + match load_status { + LoadStatus::New | LoadStatus::QuickDiff(..) | LoadStatus::DiffComplete | LoadStatus::Error { .. } => { + "Loading Diff" + }, + LoadStatus::CompleteQuickDiff | LoadStatus::Diff(..) => "Detecting renames and copies", + }, + match load_status { + LoadStatus::New + | LoadStatus::CompleteQuickDiff + | LoadStatus::DiffComplete + | LoadStatus::Error { .. } => None, + LoadStatus::QuickDiff(c, t) | LoadStatus::Diff(c, t) => Some((*c, *t)), + }, + )); + + load_status != &LoadStatus::New + } + + pub(super) fn build_diff_error(&mut self, updater: &mut ViewDataUpdater<'_>, code: ErrorCode, msg: &str) { + updater.push_line(ViewLine::from(LineSegment::new_with_color( + "Error loading diff. Press any key to return.", + DisplayColor::IndicatorColor, + ))); + updater.push_line(ViewLine::from("")); + updater.push_line(ViewLine::from("Reason:")); + updater.push_line(ViewLine::from(match code { + ErrorCode::NotFound => "Commit not found", + _ => msg, + })); + } + pub(super) fn build_view_data_for_overview( - &self, + &mut self, updater: &mut ViewDataUpdater<'_>, diff: &CommitDiff, + load_status: &LoadStatus, is_full_width: bool, ) { + updater.clear(); + if !self.build_loading_status(updater, load_status) { + return; + } + let commit = diff.commit(); updater.push_leading_line(Self::build_leading_summary(commit, is_full_width)); // TODO handle authored date @@ -230,11 +292,18 @@ impl ViewBuilder { } pub(super) fn build_view_data_diff( - &self, + &mut self, updater: &mut ViewDataUpdater<'_>, diff: &CommitDiff, + load_status: &LoadStatus, is_full_width: bool, ) { + updater.clear(); + + if !self.build_loading_status(updater, load_status) { + return; + } + updater.push_leading_line(Self::build_leading_summary(diff.commit(), is_full_width)); updater.push_leading_line(get_files_changed_summary(diff, is_full_width)); updater.push_line(ViewLine::new_empty_line().set_padding(PADDING_CHARACTER)); diff --git a/src/process.rs b/src/process.rs index b00b0d845..bca10b0ae 100644 --- a/src/process.rs +++ b/src/process.rs @@ -39,6 +39,7 @@ pub(crate) struct Process { thread_statuses: ThreadStatuses, todo_file: Arc>, view_state: crate::view::State, + diff_state: crate::diff::thread::State, search_state: search::State, } @@ -55,6 +56,7 @@ impl Clone for Process { thread_statuses: self.thread_statuses.clone(), todo_file: Arc::clone(&self.todo_file), view_state: self.view_state.clone(), + diff_state: self.diff_state.clone(), search_state: self.search_state.clone(), } } @@ -82,6 +84,7 @@ impl Process { thread_statuses, todo_file: app_data.todo_file(), view_state: app_data.view_state(), + diff_state: app_data.diff_state(), } } @@ -283,6 +286,17 @@ impl Process { Results::new() } + fn handle_diff_load(&self, hash: &str) -> Results { + self.diff_state.cancel(); + self.diff_state.start_load(hash); + Results::new() + } + + fn handle_diff_cancel(&self) -> Results { + self.diff_state.cancel(); + Results::new() + } + fn handle_results(&self, mut results: Results) { while let Some(artifact) = results.artifact() { results.append(match artifact { @@ -295,6 +309,8 @@ impl Process { Artifact::SearchCancel => self.handle_search_cancel(), Artifact::SearchTerm(search_term) => self.handle_search_term(search_term), Artifact::Searchable(searchable) => self.handle_searchable(searchable), + Artifact::LoadDiff(hash) => self.handle_diff_load(hash.as_str()), + Artifact::CancelDiff => self.handle_diff_cancel(), }); } } diff --git a/src/process/artifact.rs b/src/process/artifact.rs index 37ad898a0..7747648fb 100644 --- a/src/process/artifact.rs +++ b/src/process/artifact.rs @@ -18,6 +18,8 @@ pub(crate) enum Artifact { SearchCancel, SearchTerm(String), Searchable(Box), + LoadDiff(String), + CancelDiff, } impl Debug for Artifact { @@ -32,6 +34,8 @@ impl Debug for Artifact { Self::SearchCancel => write!(f, "SearchCancel"), Self::SearchTerm(ref term) => write!(f, "SearchTerm({term:?})"), Self::Searchable(_) => write!(f, "Searchable(dyn Searchable)"), + Self::LoadDiff(ref hash) => write!(f, "LoadDiff({hash:?})"), + Self::CancelDiff => write!(f, "CancelDiff"), } } } @@ -57,6 +61,8 @@ mod tests { Artifact::Searchable(Box::new(mocks::Searchable::new())), "Searchable(dyn Searchable)" )] + #[case::diff_load(Artifact::LoadDiff(String::from("hash")), "LoadDiff(\"hash\")")] + #[case::diff_cancel(Artifact::CancelDiff, "CancelDiff")] fn debug(#[case] artifact: Artifact, #[case] expected: &str) { assert_eq!(format!("{artifact:?}"), expected); } diff --git a/src/process/results.rs b/src/process/results.rs index a3895311f..57c889caf 100644 --- a/src/process/results.rs +++ b/src/process/results.rs @@ -49,6 +49,14 @@ impl Results { self.artifacts.push_back(Artifact::SearchTerm(String::from(term))); } + pub(crate) fn load_diff(&mut self, hash: &str) { + self.artifacts.push_back(Artifact::LoadDiff(String::from(hash))); + } + + pub(crate) fn cancel_diff(&mut self) { + self.artifacts.push_back(Artifact::CancelDiff); + } + pub(crate) fn external_command(&mut self, command: String, arguments: Vec) { self.artifacts .push_back(Artifact::ExternalCommand((command, arguments))); @@ -180,6 +188,20 @@ mod tests { assert_results!(results, Artifact::Searchable(Box::new(mocks::Searchable::new()))); } + #[test] + fn load_diff() { + let mut results = Results::new(); + results.load_diff("term"); + assert_results!(results, Artifact::LoadDiff(String::from("term"))); + } + + #[test] + fn cancel_diff() { + let mut results = Results::new(); + results.cancel_diff(); + assert_results!(results, Artifact::CancelDiff); + } + #[test] fn external_command() { let mut results = Results::new(); diff --git a/src/process/tests.rs b/src/process/tests.rs index 3542cec5a..dc3b612b0 100644 --- a/src/process/tests.rs +++ b/src/process/tests.rs @@ -626,3 +626,40 @@ fn handle_searchable() { }, ); } + +#[test] +fn handle_diff_load() { + let module = TestModule::new(); + testers::process( + create_test_module_handler(module), + |testers::ProcessTestContext { process, app_data, .. }| { + let mut results = Results::new(); + results.load_diff("term"); + process.handle_results(results); + + let diff_state = app_data.diff_state(); + + assert!(diff_state.is_cancelled()); + assert_eq!(diff_state.receive_update(), crate::diff::thread::Action::StatusChange); // cancel + assert_eq!( + diff_state.receive_update(), + crate::diff::thread::Action::Load(String::from("term")) + ); + }, + ); +} + +#[test] +fn handle_diff_cancel() { + let module = TestModule::new(); + testers::process( + create_test_module_handler(module), + |testers::ProcessTestContext { process, app_data, .. }| { + let mut results = Results::new(); + results.cancel_diff(); + process.handle_results(results); + + assert!(app_data.diff_state().is_cancelled()); + }, + ); +} diff --git a/src/test_helpers/assertions/assert_rendered_output/render_view_data.rs b/src/test_helpers/assertions/assert_rendered_output/render_view_data.rs index fcf291172..81575bc5c 100644 --- a/src/test_helpers/assertions/assert_rendered_output/render_view_data.rs +++ b/src/test_helpers/assertions/assert_rendered_output/render_view_data.rs @@ -15,22 +15,22 @@ pub(crate) fn render_view_data(view_data: &ViewData, options: AssertRenderOption } } - if view_data.is_empty() { + let leading_lines = view_data.leading_lines(); + let body_lines = view_data.lines(); + let trailing_lines = view_data.trailing_lines(); + + if leading_lines.count() == 0 && body_lines.count() == 0 && trailing_lines.count() == 0 { lines.push(String::from("{EMPTY}")); } - if !body_only { - let leading_lines = view_data.leading_lines(); - if !leading_lines.is_empty() { - lines.push(String::from("{LEADING}")); - for line in leading_lines.iter() { - lines.push(render_view_line(line, Some(options))); - } + if !body_only && leading_lines.count() != 0 { + lines.push(String::from("{LEADING}")); + for line in leading_lines.iter() { + lines.push(render_view_line(line, Some(options))); } } - let body_lines = view_data.lines(); - if !body_lines.is_empty() { + if body_lines.count() != 0 { if !body_only { lines.push(String::from("{BODY}")); } @@ -39,13 +39,10 @@ pub(crate) fn render_view_data(view_data: &ViewData, options: AssertRenderOption } } - if !body_only { - let trailing_lines = view_data.trailing_lines(); - if !trailing_lines.is_empty() { - lines.push(String::from("{TRAILING}")); - for line in trailing_lines.iter() { - lines.push(render_view_line(line, Some(options))); - } + if !body_only && trailing_lines.count() != 0 { + lines.push(String::from("{TRAILING}")); + for line in trailing_lines.iter() { + lines.push(render_view_line(line, Some(options))); } } lines diff --git a/src/test_helpers/assertions/assert_results.rs b/src/test_helpers/assertions/assert_results.rs index 84d118c03..48347bb0a 100644 --- a/src/test_helpers/assertions/assert_results.rs +++ b/src/test_helpers/assertions/assert_results.rs @@ -25,6 +25,8 @@ fn _assert_results_format(artifacts: &[Artifact]) -> String { Artifact::SearchCancel => String::from("SearchCancel"), Artifact::SearchTerm(ref term) => format!("SearchTerm({term})"), Artifact::Searchable(ref _searchable) => String::from("SearchCancel(_)"), + Artifact::LoadDiff(ref hash) => format!("LoadDiff({hash:?})"), + Artifact::CancelDiff => String::from("CancelDiff"), } }) .collect::>() @@ -45,7 +47,9 @@ fn compare_artifact(a: &Artifact, b: &Artifact) -> bool { self_command == other_command }, (Artifact::SearchTerm(self_term), Artifact::SearchTerm(other_term)) => self_term == other_term, + (Artifact::LoadDiff(self_hash), Artifact::LoadDiff(other_hash)) => self_hash == other_hash, (Artifact::SearchCancel, Artifact::SearchCancel) + | (Artifact::CancelDiff, Artifact::CancelDiff) | (Artifact::EnqueueResize, Artifact::EnqueueResize) | (Artifact::Searchable(_), Artifact::Searchable(_)) => true, _ => false, diff --git a/src/test_helpers/builders/commit_diff.rs b/src/test_helpers/builders/commit_diff.rs index 1bca0f49e..8b9120586 100644 --- a/src/test_helpers/builders/commit_diff.rs +++ b/src/test_helpers/builders/commit_diff.rs @@ -70,13 +70,15 @@ impl CommitDiffBuilder { /// Return the built `CommitDiff` #[must_use] pub(crate) fn build(self) -> CommitDiff { - CommitDiff::new( - self.commit, - self.parent, + let mut diff = CommitDiff::new(); + diff.reset(self.commit, self.parent); + diff.update( self.file_statuses, self.number_files_changed, self.number_insertions, self.number_deletions, - ) + ); + + diff } } diff --git a/src/test_helpers/create_commit.rs b/src/test_helpers/create_commit.rs index 6020250f7..805654a1c 100644 --- a/src/test_helpers/create_commit.rs +++ b/src/test_helpers/create_commit.rs @@ -1,6 +1,8 @@ use std::sync::LazyLock; -use crate::{git::Repository, test_helpers::JAN_2021_EPOCH}; +use git2::{Repository, Signature}; + +use crate::{diff::Commit, test_helpers::JAN_2021_EPOCH}; static DEFAULT_COMMIT_OPTIONS: LazyLock = LazyLock::new(CreateCommitOptions::new); @@ -96,21 +98,35 @@ impl CreateCommitOptions { } } +fn create_commit_on_index( + repository: &Repository, + reference: &str, + author: &Signature<'_>, + committer: &Signature<'_>, + message: &str, +) -> Result { + let tree = repository.find_tree(repository.index()?.write_tree()?)?; + let head = repository.find_reference(reference)?.peel_to_commit()?; + _ = repository.commit(Some("HEAD"), author, committer, message, &tree, &[&head])?; + + Ok(Commit::from(&repository.find_reference(reference)?.peel_to_commit()?)) +} + /// Create a commit based on the provided options. If `options` is not provided, will create a /// commit using the default options. This function does not add modified or new files to the stage /// before creating a commit. /// /// # Panics /// If any Git operation cannot be performed. -pub(crate) fn create_commit(repository: &Repository, options: Option<&CreateCommitOptions>) { +pub(crate) fn create_commit(repository: &Repository, options: Option<&CreateCommitOptions>) -> Commit { let opts = options.unwrap_or(&DEFAULT_COMMIT_OPTIONS); - let author_sig = git2::Signature::new( + let author_sig = Signature::new( opts.author_name.as_str(), opts.author_email.as_str(), &git2::Time::new(opts.author_time.unwrap_or(opts.committer_time), 0), ) .unwrap(); - let committer_sig = git2::Signature::new( + let committer_sig = Signature::new( opts.committer_name.as_ref().unwrap_or(&opts.author_name).as_str(), opts.committer_email.as_ref().unwrap_or(&opts.author_email).as_str(), &git2::Time::new(opts.committer_time, 0), @@ -118,7 +134,12 @@ pub(crate) fn create_commit(repository: &Repository, options: Option<&CreateComm .unwrap(); let ref_name = format!("refs/heads/{}", opts.head_name); - repository - .create_commit_on_index(ref_name.as_str(), &author_sig, &committer_sig, opts.message.as_str()) - .unwrap(); + create_commit_on_index( + repository, + ref_name.as_str(), + &author_sig, + &committer_sig, + opts.message.as_str(), + ) + .unwrap() } diff --git a/src/test_helpers/git.rs b/src/test_helpers/git.rs deleted file mode 100644 index e69de29bb..000000000 diff --git a/src/test_helpers/shared/module.rs b/src/test_helpers/shared/module.rs index 1a0bb0791..d99206bfc 100644 --- a/src/test_helpers/shared/module.rs +++ b/src/test_helpers/shared/module.rs @@ -1,6 +1,5 @@ use crate::{ application::AppData, - git::Repository, module::{Module, ModuleProvider, State}, }; @@ -15,7 +14,7 @@ impl From for TestModuleProvider { } impl ModuleProvider for TestModuleProvider { - fn new(_: Repository, _: &AppData) -> Self { + fn new(_: &AppData) -> Self { unimplemented!("Not implemented for the TestModuleProvider"); } diff --git a/src/test_helpers/testers/module.rs b/src/test_helpers/testers/module.rs index e96a1052f..42a90710f 100644 --- a/src/test_helpers/testers/module.rs +++ b/src/test_helpers/testers/module.rs @@ -1,11 +1,13 @@ use std::sync::Arc; use captur::capture; -use parking_lot::Mutex; +use parking_lot::{Mutex, lock_api::RwLock}; use crate::{ application::AppData, config::Config, + diff, + diff::CommitDiff, input::Event, module::{Module, State}, process::Results, @@ -88,11 +90,13 @@ where C: FnOnce(ModuleTestContext) { with_view_state(|view_context| { capture!(lines); with_todo_file(lines, |todo_file_context| { + let commit_diff = CommitDiff::new(); let (_git_todo_file, todo_file) = todo_file_context.to_owned(); let app_data = AppData::new( config.unwrap_or_else(create_config), State::WindowSizeError, Arc::new(Mutex::new(todo_file)), + diff::thread::State::new(Arc::new(RwLock::new(commit_diff))), view_context.state.clone(), event_handler_context.state.clone(), crate::search::State::new(), diff --git a/src/test_helpers/testers/process.rs b/src/test_helpers/testers/process.rs index e959fb89d..78e5788ba 100644 --- a/src/test_helpers/testers/process.rs +++ b/src/test_helpers/testers/process.rs @@ -1,9 +1,11 @@ use std::sync::Arc; -use parking_lot::Mutex; +use parking_lot::{Mutex, lock_api::RwLock}; use crate::{ application::AppData, + diff, + diff::CommitDiff, display::Size, input::Event, module::{self, ModuleHandler, State}, @@ -35,6 +37,7 @@ pub(crate) fn process bool { - self.lines.is_empty() && self.lines_leading.is_empty() && self.lines_trailing.is_empty() - } - /// Update the view data using a `ViewDataUpdater`. This allows for batch updating of the `ViewData`. pub(crate) fn update_view_data(&mut self, callback: C) where C: FnOnce(&mut ViewDataUpdater<'_>) { @@ -164,12 +158,6 @@ mod tests { assert!(view_data.show_title()); } - #[test] - fn is_empty() { - let view_data = ViewData::new(|_| {}); - assert!(view_data.is_empty()); - } - #[test] fn update_view_data_without_modifications() { let mut view_data = ViewData::new(|_| {}); @@ -193,9 +181,9 @@ mod tests { view_data.push_leading_line(ViewLine::new_empty_line()); view_data.push_trailing_line(ViewLine::new_empty_line()); view_data.clear(); - assert!(view_data.leading_lines().is_empty()); - assert!(view_data.lines().is_empty()); - assert!(view_data.trailing_lines().is_empty()); + assert_eq!(view_data.leading_lines().count(), 0); + assert_eq!(view_data.lines().count(), 0); + assert_eq!(view_data.trailing_lines().count(), 0); } #[test] @@ -205,9 +193,9 @@ mod tests { view_data.push_leading_line(ViewLine::new_empty_line()); view_data.push_trailing_line(ViewLine::new_empty_line()); view_data.clear_body(); - assert!(!view_data.leading_lines().is_empty()); - assert!(view_data.lines().is_empty()); - assert!(!view_data.trailing_lines().is_empty()); + assert_ne!(view_data.leading_lines().count(), 0); + assert_eq!(view_data.lines().count(), 0); + assert_ne!(view_data.trailing_lines().count(), 0); } #[test] diff --git a/src/view/view_data_updater.rs b/src/view/view_data_updater.rs index f1a3b940c..aeb0e87f0 100644 --- a/src/view/view_data_updater.rs +++ b/src/view/view_data_updater.rs @@ -107,7 +107,7 @@ mod tests { let mut updater = ViewDataUpdater::new(&mut view_data); updater.clear(); assert!(updater.is_modified()); - assert!(view_data.is_empty()); + assert_eq!(view_data.lines().count(), 0); } #[test] @@ -118,7 +118,7 @@ mod tests { let mut updater = ViewDataUpdater::new(&mut view_data); updater.clear_body(); assert!(updater.is_modified()); - assert!(view_data.lines().is_empty()); + assert_eq!(view_data.lines().count(), 0); assert_eq!(view_data.leading_lines().count(), 1); } diff --git a/src/view/view_lines.rs b/src/view/view_lines.rs index 39b190984..aaf1f54d3 100644 --- a/src/view/view_lines.rs +++ b/src/view/view_lines.rs @@ -21,10 +21,6 @@ impl ViewLines { self.lines.len() as u16 } - pub(crate) fn is_empty(&self) -> bool { - self.lines.is_empty() - } - pub(crate) fn iter(&self) -> Iter<'_, ViewLine> { self.lines.iter() } @@ -69,7 +65,7 @@ mod tests { #[test] fn new() { - assert!(ViewLines::new().is_empty()); + assert!(ViewLines::new().lines.is_empty()); } #[test] @@ -84,7 +80,7 @@ mod tests { let mut view_lines = ViewLines::new(); view_lines.push(ViewLine::new_empty_line()); view_lines.clear(); - assert!(view_lines.is_empty()); + assert!(view_lines.lines.is_empty()); } #[test]