From 4f3ecfcd7c08b63ed2d5a0b095b4971e440895f0 Mon Sep 17 00:00:00 2001 From: Stephan Dilly Date: Mon, 31 Jan 2022 20:56:59 +0100 Subject: [PATCH] fix left arrow closing popup (#1121) * fix left arrow closing popup * fix pageup/down being borked * adhere to global diff options * fix build for rust 1.50 * show revision count in header * allow blaming any specific file revision * show blame command * allow opening history from blame --- asyncgit/src/blame.rs | 11 +++- asyncgit/src/diff.rs | 2 + asyncgit/src/revlog.rs | 2 +- asyncgit/src/sync/blame.rs | 29 +++++++--- asyncgit/src/sync/commit_files.rs | 27 +++++++-- asyncgit/src/sync/diff.rs | 16 ++++-- asyncgit/src/sync/logwalker.rs | 1 + src/app.rs | 5 +- src/components/blame_file.rs | 79 +++++++++++++++----------- src/components/file_revlog.rs | 93 +++++++++++++++++-------------- src/components/revision_files.rs | 3 +- src/components/status_tree.rs | 2 + src/queue.rs | 2 +- src/strings.rs | 10 +++- 14 files changed, 181 insertions(+), 101 deletions(-) diff --git a/asyncgit/src/blame.rs b/asyncgit/src/blame.rs index 35998646..9c848cb0 100644 --- a/asyncgit/src/blame.rs +++ b/asyncgit/src/blame.rs @@ -1,7 +1,7 @@ use crate::{ error::Result, hash, - sync::{self, FileBlame, RepoPath}, + sync::{self, CommitId, FileBlame, RepoPath}, AsyncGitNotification, }; use crossbeam_channel::Sender; @@ -18,6 +18,8 @@ use std::{ pub struct BlameParams { /// path to the file to blame pub file_path: String, + /// blame at a specific revision + pub commit_id: Option, } struct Request(R, Option); @@ -145,8 +147,11 @@ impl AsyncBlame { arc_current: &Arc>>, hash: u64, ) -> Result { - let file_blame = - sync::blame::blame_file(repo_path, ¶ms.file_path)?; + let file_blame = sync::blame::blame_file( + repo_path, + ¶ms.file_path, + params.commit_id, + )?; let mut notify = false; { diff --git a/asyncgit/src/diff.rs b/asyncgit/src/diff.rs index e52dea1e..a2685b4f 100644 --- a/asyncgit/src/diff.rs +++ b/asyncgit/src/diff.rs @@ -175,11 +175,13 @@ impl AsyncDiff { repo_path, id, params.path.clone(), + Some(params.options), )?, DiffType::Commits(ids) => sync::diff::get_diff_commits( repo_path, ids, params.path.clone(), + Some(params.options), )?, }; diff --git a/asyncgit/src/revlog.rs b/asyncgit/src/revlog.rs index c620b8b5..6ed93fef 100644 --- a/asyncgit/src/revlog.rs +++ b/asyncgit/src/revlog.rs @@ -59,7 +59,7 @@ impl AsyncLog { } /// - pub fn count(&mut self) -> Result { + pub fn count(&self) -> Result { Ok(self.current.lock()?.len()) } diff --git a/asyncgit/src/sync/blame.rs b/asyncgit/src/sync/blame.rs index 1cd19c7e..0a5005d2 100644 --- a/asyncgit/src/sync/blame.rs +++ b/asyncgit/src/sync/blame.rs @@ -5,6 +5,7 @@ use crate::{ error::{Error, Result}, sync::{get_commits_info, repository::repo}, }; +use git2::BlameOptions; use scopetime::scope_time; use std::collections::{HashMap, HashSet}; use std::io::{BufRead, BufReader}; @@ -56,12 +57,17 @@ fn fixup_windows_path(path: &str) -> String { pub fn blame_file( repo_path: &RepoPath, file_path: &str, + commit_id: Option, ) -> Result { scope_time!("blame_file"); let repo = repo(repo_path)?; - let commit_id = utils::get_head_repo(&repo)?; + let commit_id = if let Some(commit_id) = commit_id { + commit_id + } else { + utils::get_head_repo(&repo)? + }; let spec = format!( "{}:{}", @@ -76,7 +82,11 @@ pub fn blame_file( return Err(Error::NoBlameOnBinaryFile); } - let blame = repo.blame_file(Path::new(file_path), None)?; + let mut opts = BlameOptions::new(); + opts.newest_commit(commit_id.into()); + + let blame = + repo.blame_file(Path::new(file_path), Some(&mut opts))?; let reader = BufReader::new(blob.content()); @@ -160,7 +170,10 @@ mod tests { let repo_path: &RepoPath = &root.as_os_str().to_str().unwrap().into(); - assert!(matches!(blame_file(&repo_path, "foo"), Err(_))); + assert!(matches!( + blame_file(&repo_path, "foo", None), + Err(_) + )); File::create(&root.join(file_path))? .write_all(b"line 1\n")?; @@ -168,7 +181,7 @@ mod tests { stage_add_file(repo_path, file_path)?; commit(repo_path, "first commit")?; - let blame = blame_file(&repo_path, "foo")?; + let blame = blame_file(&repo_path, "foo", None)?; assert!(matches!( blame.lines.as_slice(), @@ -192,7 +205,7 @@ mod tests { stage_add_file(repo_path, file_path)?; commit(repo_path, "second commit")?; - let blame = blame_file(&repo_path, "foo")?; + let blame = blame_file(&repo_path, "foo", None)?; assert!(matches!( blame.lines.as_slice(), @@ -219,14 +232,14 @@ mod tests { file.write(b"line 3\n")?; - let blame = blame_file(&repo_path, "foo")?; + let blame = blame_file(&repo_path, "foo", None)?; assert_eq!(blame.lines.len(), 2); stage_add_file(repo_path, file_path)?; commit(repo_path, "third commit")?; - let blame = blame_file(&repo_path, "foo")?; + let blame = blame_file(&repo_path, "foo", None)?; assert_eq!(blame.lines.len(), 3); @@ -251,6 +264,6 @@ mod tests { stage_add_file(repo_path, file_path).unwrap(); commit(repo_path, "first commit").unwrap(); - assert!(blame_file(&repo_path, "bar\\foo").is_ok()); + assert!(blame_file(&repo_path, "bar\\foo", None).is_ok()); } } diff --git a/asyncgit/src/sync/commit_files.rs b/asyncgit/src/sync/commit_files.rs index 88774305..d7fbf7fc 100644 --- a/asyncgit/src/sync/commit_files.rs +++ b/asyncgit/src/sync/commit_files.rs @@ -1,10 +1,12 @@ //! Functions for getting infos about files in commits -use super::{stash::is_stash_commit, CommitId, RepoPath}; +use super::{ + diff::DiffOptions, stash::is_stash_commit, CommitId, RepoPath, +}; use crate::{ error::Result, sync::repository::repo, StatusItem, StatusItemType, }; -use git2::{Diff, DiffOptions, Repository}; +use git2::{Diff, Repository}; use scopetime::scope_time; use std::cmp::Ordering; @@ -19,9 +21,9 @@ pub fn get_commit_files( let repo = repo(repo_path)?; let diff = if let Some(other) = other { - get_compare_commits_diff(&repo, (id, other), None)? + get_compare_commits_diff(&repo, (id, other), None, None)? } else { - get_commit_diff(repo_path, &repo, id, None)? + get_commit_diff(repo_path, &repo, id, None, None)? }; let res = diff @@ -49,6 +51,7 @@ pub fn get_compare_commits_diff( repo: &Repository, ids: (CommitId, CommitId), pathspec: Option, + options: Option, ) -> Result> { // scope_time!("get_compare_commits_diff"); @@ -67,7 +70,12 @@ pub fn get_compare_commits_diff( let trees = (commits.0.tree()?, commits.1.tree()?); - let mut opts = DiffOptions::new(); + let mut opts = git2::DiffOptions::new(); + if let Some(options) = options { + opts.context_lines(options.context); + opts.ignore_whitespace(options.ignore_whitespace); + opts.interhunk_lines(options.interhunk_lines); + } if let Some(p) = &pathspec { opts.pathspec(p.clone()); } @@ -88,6 +96,7 @@ pub fn get_commit_diff<'a>( repo: &'a Repository, id: CommitId, pathspec: Option, + options: Option, ) -> Result> { // scope_time!("get_commit_diff"); @@ -102,7 +111,12 @@ pub fn get_commit_diff<'a>( None }; - let mut opts = DiffOptions::new(); + let mut opts = git2::DiffOptions::new(); + if let Some(options) = options { + opts.context_lines(options.context); + opts.ignore_whitespace(options.ignore_whitespace); + opts.interhunk_lines(options.interhunk_lines); + } if let Some(p) = &pathspec { opts.pathspec(p.clone()); } @@ -121,6 +135,7 @@ pub fn get_commit_diff<'a>( repo, CommitId::new(untracked_commit), pathspec, + options, )?; diff.merge(&untracked_diff)?; diff --git a/asyncgit/src/sync/diff.rs b/asyncgit/src/sync/diff.rs index e0f00d5a..f42252c0 100644 --- a/asyncgit/src/sync/diff.rs +++ b/asyncgit/src/sync/diff.rs @@ -214,12 +214,14 @@ pub fn get_diff_commit( repo_path: &RepoPath, id: CommitId, p: String, + options: Option, ) -> Result { scope_time!("get_diff_commit"); let repo = repo(repo_path)?; let work_dir = work_dir(&repo)?; - let diff = get_commit_diff(repo_path, &repo, id, Some(p))?; + let diff = + get_commit_diff(repo_path, &repo, id, Some(p), options)?; raw_diff_to_file_diff(&diff, work_dir) } @@ -229,13 +231,18 @@ pub fn get_diff_commits( repo_path: &RepoPath, ids: (CommitId, CommitId), p: String, + options: Option, ) -> Result { scope_time!("get_diff_commits"); let repo = repo(repo_path)?; let work_dir = work_dir(&repo)?; - let diff = - get_compare_commits_diff(&repo, (ids.0, ids.1), Some(p))?; + let diff = get_compare_commits_diff( + &repo, + (ids.0, ids.1), + Some(p), + options, + )?; raw_diff_to_file_diff(&diff, work_dir) } @@ -649,7 +656,8 @@ mod tests { let id = commit(repo_path, "").unwrap(); let diff = - get_diff_commit(repo_path, id, String::new()).unwrap(); + get_diff_commit(repo_path, id, String::new(), None) + .unwrap(); dbg!(&diff); assert_eq!(diff.sizes, (1, 2)); diff --git a/asyncgit/src/sync/logwalker.rs b/asyncgit/src/sync/logwalker.rs index b78c37ed..15873e29 100644 --- a/asyncgit/src/sync/logwalker.rs +++ b/asyncgit/src/sync/logwalker.rs @@ -49,6 +49,7 @@ pub fn diff_contains_file( repo, *commit_id, Some(file_path.clone()), + None, )?; let contains_file = diff.deltas().len() > 0; diff --git a/src/app.rs b/src/app.rs index bc10a1d3..654830fc 100644 --- a/src/app.rs +++ b/src/app.rs @@ -129,6 +129,7 @@ impl App { sender, theme.clone(), key_config.clone(), + options.clone(), ), revision_files_popup: RevisionFilesPopup::new( repo.clone(), @@ -714,8 +715,8 @@ impl App { InternalEvent::TagCommit(id) => { self.tag_commit_popup.open(id)?; } - InternalEvent::BlameFile(path) => { - self.blame_file_popup.open(&path)?; + InternalEvent::BlameFile(path, commit_id) => { + self.blame_file_popup.open(&path, commit_id)?; flags .insert(NeedsUpdate::ALL | NeedsUpdate::COMMANDS); } diff --git a/src/components/blame_file.rs b/src/components/blame_file.rs index 3fe92098..62512351 100644 --- a/src/components/blame_file.rs +++ b/src/components/blame_file.rs @@ -33,7 +33,7 @@ pub struct BlameFileComponent { queue: Queue, async_blame: AsyncBlame, visible: bool, - file_path: Option, + params: Option, file_blame: Option, table_state: std::cell::Cell, key_config: SharedKeyConfig, @@ -175,6 +175,16 @@ impl Component for BlameFileComponent { ) .order(1), ); + out.push( + CommandInfo::new( + strings::commands::open_file_history( + &self.key_config, + ), + true, + self.file_blame.is_some(), + ) + .order(1), + ); } visibility_blocking(self) @@ -205,19 +215,23 @@ impl Component for BlameFileComponent { } else if key == self.key_config.keys.page_up { self.move_selection(ScrollType::PageUp); } else if key == self.key_config.keys.focus_right { - self.hide(); - - return self.selected_commit().map_or( - Ok(EventState::NotConsumed), - |id| { - self.queue.push( - InternalEvent::InspectCommit( - id, None, - ), - ); - Ok(EventState::Consumed) - }, - ); + if let Some(id) = self.selected_commit() { + self.hide(); + self.queue.push( + InternalEvent::InspectCommit(id, None), + ); + } + } else if key == self.key_config.keys.file_history { + if let Some(filepath) = self + .params + .as_ref() + .map(|p| p.file_path.clone()) + { + self.hide(); + self.queue.push( + InternalEvent::OpenFileRevlog(filepath), + ); + } } return Ok(EventState::Consumed); @@ -261,7 +275,7 @@ impl BlameFileComponent { ), queue: queue.clone(), visible: false, - file_path: None, + params: None, file_blame: None, table_state: std::cell::Cell::new(TableState::default()), key_config, @@ -270,8 +284,15 @@ impl BlameFileComponent { } /// - pub fn open(&mut self, file_path: &str) -> Result<()> { - self.file_path = Some(file_path.into()); + pub fn open( + &mut self, + file_path: &str, + commit_id: Option, + ) -> Result<()> { + self.params = Some(BlameParams { + file_path: file_path.into(), + commit_id, + }); self.file_blame = None; self.table_state.get_mut().select(Some(0)); self.show()?; @@ -300,24 +321,20 @@ impl BlameFileComponent { fn update(&mut self) -> Result<()> { if self.is_visible() { - if let Some(file_path) = &self.file_path { - let blame_params = BlameParams { - file_path: file_path.into(), - }; - + if let Some(params) = &self.params { if let Some(( previous_blame_params, last_file_blame, )) = self.async_blame.last()? { - if previous_blame_params == blame_params { + if previous_blame_params == *params { self.file_blame = Some(last_file_blame); return Ok(()); } } - self.async_blame.request(blame_params)?; + self.async_blame.request(params.clone())?; } } @@ -328,27 +345,27 @@ impl BlameFileComponent { fn get_title(&self) -> String { match ( self.any_work_pending(), - self.file_path.as_ref(), + self.params.as_ref(), self.file_blame.as_ref(), ) { - (true, Some(file_path), _) => { + (true, Some(params), _) => { format!( "{} -- {} -- ", - self.title, file_path + self.title, params.file_path ) } - (false, Some(file_path), Some(file_blame)) => { + (false, Some(params), Some(file_blame)) => { format!( "{} -- {} -- {}", self.title, - file_path, + params.file_path, file_blame.commit_id.get_short_string() ) } - (false, Some(file_path), None) => { + (false, Some(params), None) => { format!( "{} -- {} -- ", - self.title, file_path + self.title, params.file_path ) } _ => format!("{} -- ", self.title), diff --git a/src/components/file_revlog.rs b/src/components/file_revlog.rs index c6d3ea98..4ce7b122 100644 --- a/src/components/file_revlog.rs +++ b/src/components/file_revlog.rs @@ -1,5 +1,5 @@ -use super::utils::logitems::ItemBatch; use super::visibility_blocking; +use super::{utils::logitems::ItemBatch, SharedOptions}; use crate::{ components::{ event_pump, CommandBlocking, CommandInfo, Component, @@ -13,8 +13,7 @@ use crate::{ use anyhow::Result; use asyncgit::{ sync::{ - diff::DiffOptions, diff_contains_file, get_commits_info, - CommitId, RepoPathRef, + diff_contains_file, get_commits_info, CommitId, RepoPathRef, }, AsyncDiff, AsyncGitNotification, AsyncLog, DiffParams, DiffType, FetchStatus, @@ -47,6 +46,7 @@ pub struct FileRevlogComponent { items: ItemBatch, count_total: usize, key_config: SharedKeyConfig, + options: SharedOptions, current_width: std::cell::Cell, current_height: std::cell::Cell, } @@ -59,6 +59,7 @@ impl FileRevlogComponent { sender: &Sender, theme: SharedTheme, key_config: SharedKeyConfig, + options: SharedOptions, ) -> Self { Self { theme: theme.clone(), @@ -85,6 +86,7 @@ impl FileRevlogComponent { key_config, current_width: std::cell::Cell::new(0), current_height: std::cell::Cell::new(0), + options, } } @@ -169,7 +171,7 @@ impl FileRevlogComponent { let diff_params = DiffParams { path: file_path.clone(), diff_type: DiffType::Commit(commit_id), - options: DiffOptions::default(), + options: self.options.borrow().diff, }; if let Some((params, last)) = @@ -243,10 +245,20 @@ impl FileRevlogComponent { } fn get_title(&self) -> String { + let selected = { + let table = self.table_state.take(); + let res = table.selected().unwrap_or_default(); + self.table_state.set(table); + res + }; + let revisions = self.get_max_selection(); + self.file_path.as_ref().map_or( "".into(), |file_path| { - strings::file_log_title(&self.key_config, file_path) + strings::file_log_title( + file_path, selected, revisions, + ) }, ) } @@ -282,8 +294,8 @@ impl FileRevlogComponent { .collect() } - fn get_max_selection(&mut self) -> usize { - self.git_log.as_mut().map_or(0, |log| { + fn get_max_selection(&self) -> usize { + self.git_log.as_ref().map_or(0, |log| { log.count().unwrap_or(0).saturating_sub(1) }) } @@ -293,6 +305,7 @@ impl FileRevlogComponent { let old_selection = table_state.selected().unwrap_or(0); let max_selection = self.get_max_selection(); + let height_in_items = self.current_height.get() / 2; let new_selection = match scroll_type { ScrollType::Up => old_selection.saturating_sub(1), @@ -301,13 +314,10 @@ impl FileRevlogComponent { } ScrollType::Home => 0, ScrollType::End => max_selection, - ScrollType::PageUp => old_selection.saturating_sub( - self.current_height.get().saturating_sub(2), - ), + ScrollType::PageUp => old_selection + .saturating_sub(height_in_items.saturating_sub(2)), ScrollType::PageDown => old_selection - .saturating_add( - self.current_height.get().saturating_sub(2), - ) + .saturating_add(height_in_items.saturating_sub(2)) .min(max_selection), }; @@ -418,53 +428,46 @@ impl Component for FileRevlogComponent { if let Event::Key(key) = event { if key == self.key_config.keys.exit_popup { self.hide(); - - return Ok(EventState::Consumed); } else if key == self.key_config.keys.focus_right && self.can_focus_diff() { self.diff.focus(true); - return Ok(EventState::Consumed); } else if key == self.key_config.keys.focus_left { if self.diff.focused() { self.diff.focus(false); - } else { - self.hide(); } - return Ok(EventState::Consumed); } else if key == self.key_config.keys.enter { - self.hide(); - - return self.selected_commit().map_or( - Ok(EventState::NotConsumed), - |id| { - self.queue.push( - InternalEvent::InspectCommit( - id, None, - ), - ); - Ok(EventState::Consumed) - }, - ); + if let Some(id) = self.selected_commit() { + self.hide(); + self.queue.push( + InternalEvent::InspectCommit(id, None), + ); + }; + } else if key == self.key_config.keys.blame { + if let Some(file) = self.file_path.clone() { + self.hide(); + self.queue.push(InternalEvent::BlameFile( + file, + self.selected_commit(), + )); + } } else if key == self.key_config.keys.move_up { - self.move_selection(ScrollType::Up) + self.move_selection(ScrollType::Up); } else if key == self.key_config.keys.move_down { - self.move_selection(ScrollType::Down) + self.move_selection(ScrollType::Down); } else if key == self.key_config.keys.shift_up || key == self.key_config.keys.home { - self.move_selection(ScrollType::Home) + self.move_selection(ScrollType::Home); } else if key == self.key_config.keys.shift_down || key == self.key_config.keys.end { - self.move_selection(ScrollType::End) + self.move_selection(ScrollType::End); } else if key == self.key_config.keys.page_up { - self.move_selection(ScrollType::PageUp) + self.move_selection(ScrollType::PageUp); } else if key == self.key_config.keys.page_down { - self.move_selection(ScrollType::PageDown) - } else { - false - }; + self.move_selection(ScrollType::PageDown); + } } return Ok(EventState::Consumed); @@ -497,6 +500,14 @@ impl Component for FileRevlogComponent { ) .order(1), ); + out.push( + CommandInfo::new( + strings::commands::blame_file(&self.key_config), + true, + self.selected_commit().is_some(), + ) + .order(1), + ); out.push(CommandInfo::new( strings::commands::diff_focus_right(&self.key_config), diff --git a/src/components/revision_files.rs b/src/components/revision_files.rs index 4d4d3802..df6b9d72 100644 --- a/src/components/revision_files.rs +++ b/src/components/revision_files.rs @@ -133,7 +133,8 @@ impl RevisionFilesComponent { fn blame(&self) -> bool { self.selected_file_path().map_or(false, |path| { - self.queue.push(InternalEvent::BlameFile(path)); + self.queue + .push(InternalEvent::BlameFile(path, self.revision)); true }) diff --git a/src/components/status_tree.rs b/src/components/status_tree.rs index 422775de..c2a2104b 100644 --- a/src/components/status_tree.rs +++ b/src/components/status_tree.rs @@ -418,8 +418,10 @@ impl Component for StatusTreeComponent { return if e == self.key_config.keys.blame { match (&self.queue, self.selection_file()) { (Some(queue), Some(status_item)) => { + //TODO: use correct revision here queue.push(InternalEvent::BlameFile( status_item.path, + None, )); Ok(EventState::Consumed) diff --git a/src/queue.rs b/src/queue.rs index 863b6890..d10d6dd1 100644 --- a/src/queue.rs +++ b/src/queue.rs @@ -79,7 +79,7 @@ pub enum InternalEvent { /// Tags, /// - BlameFile(String), + BlameFile(String, Option), /// OpenFileRevlog(String), /// diff --git a/src/strings.rs b/src/strings.rs index e4f11e92..7d9d59a6 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -270,10 +270,14 @@ pub fn log_title(_key_config: &SharedKeyConfig) -> String { "Commit".to_string() } pub fn file_log_title( - _key_config: &SharedKeyConfig, file_path: &str, + selected: usize, + revisions: usize, ) -> String { - format!("Commits for file {}", file_path) + format!( + "Revisions of '{}' ({}/{})", + file_path, selected, revisions + ) } pub fn blame_title(_key_config: &SharedKeyConfig) -> String { "Blame".to_string() @@ -1057,7 +1061,7 @@ pub mod commands { key_config.get_hint(key_config.keys.blame), ), "open blame view of selected file", - CMD_GROUP_LOG, + CMD_GROUP_GENERAL, ) } pub fn open_file_history(