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

@@ -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<CommitId>,
 }
 
 struct Request<R, A>(R, Option<A>);
@@ -145,8 +147,11 @@ impl AsyncBlame {
 		arc_current: &Arc<Mutex<Request<u64, FileBlame>>>,
 		hash: u64,
 	) -> Result<bool> {
-		let file_blame =
-			sync::blame::blame_file(repo_path, &params.file_path)?;
+		let file_blame = sync::blame::blame_file(
+			repo_path,
+			&params.file_path,
+			params.commit_id,
+		)?;
 
 		let mut notify = false;
 		{

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),
 			)?,
 		};
 

asyncgit/src/revlog.rs

@@ -59,7 +59,7 @@ impl AsyncLog {
 	}
 
 	///
-	pub fn count(&mut self) -> Result<usize> {
+	pub fn count(&self) -> Result<usize> {
 		Ok(self.current.lock()?.len())
 	}
 

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<CommitId>,
 ) -> Result<FileBlame> {
 	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());
 	}
 }

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<String>,
+	options: Option<DiffOptions>,
 ) -> Result<Diff<'_>> {
 	// 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<String>,
+	options: Option<DiffOptions>,
 ) -> Result<Diff<'a>> {
 	// 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)?;

asyncgit/src/sync/diff.rs

@@ -214,12 +214,14 @@ pub fn get_diff_commit(
 	repo_path: &RepoPath,
 	id: CommitId,
 	p: String,
+	options: Option<DiffOptions>,
 ) -> Result<FileDiff> {
 	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<DiffOptions>,
 ) -> Result<FileDiff> {
 	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));

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;

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);
 			}

src/components/blame_file.rs

@@ -33,7 +33,7 @@ pub struct BlameFileComponent {
 	queue: Queue,
 	async_blame: AsyncBlame,
 	visible: bool,
-	file_path: Option<String>,
+	params: Option<BlameParams>,
 	file_blame: Option<FileBlame>,
 	table_state: std::cell::Cell<TableState>,
 	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<CommitId>,
+	) -> 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!(
 					"{} -- {} -- <calculating.. (who is to blame?)>",
-					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!(
 					"{} -- {} -- <no blame available>",
-					self.title, file_path
+					self.title, params.file_path
 				)
 			}
 			_ => format!("{} -- <no blame available>", self.title),

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<usize>,
 	current_height: std::cell::Cell<usize>,
 }
@@ -59,6 +59,7 @@ impl FileRevlogComponent {
 		sender: &Sender<AsyncGitNotification>,
 		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(
 			"<no history available>".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),

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
 		})

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)

src/queue.rs

@@ -79,7 +79,7 @@ pub enum InternalEvent {
 	///
 	Tags,
 	///
-	BlameFile(String),
+	BlameFile(String, Option<CommitId>),
 	///
 	OpenFileRevlog(String),
 	///

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(