Fix: search in log (#1838)

7 files changed, 205 insertions, 198 deletions

diff --git a/Makefile b/Makefile
index 029b6813..e07163e2 100644
--- a/Makefile
+++ b/Makefile
@@ -2,7 +2,7 @@
 .PHONY: debug build-release release-linux-musl test clippy clippy-pedantic install install-debug
 
 ARGS=-l
-# ARGS=-l -d ~/code/extern/pbrt-v4
+# ARGS=-l -d ~/code/extern/linux
 # ARGS=-l -d ~/code/git-bare-test.git -w ~/code/git-bare-test
 
 profile:
diff --git a/asyncgit/src/revlog.rs b/asyncgit/src/revlog.rs
index a42fe55f..df02c3e9 100644
--- a/asyncgit/src/revlog.rs
+++ b/asyncgit/src/revlog.rs
@@ -1,7 +1,7 @@
 use crate::{
 	error::Result,
 	sync::{repo, CommitId, LogWalker, LogWalkerFilter, RepoPath},
-	AsyncGitNotification,
+	AsyncGitNotification, Error,
 };
 use crossbeam_channel::Sender;
 use scopetime::scope_time;
@@ -15,7 +15,7 @@ use std::{
 };
 
 ///
-#[derive(PartialEq, Eq)]
+#[derive(PartialEq, Eq, Debug)]
 pub enum FetchStatus {
 	/// previous fetch still running
 	Pending,
@@ -40,6 +40,7 @@ pub struct AsyncLog {
 	pending: Arc<AtomicBool>,
 	background: Arc<AtomicBool>,
 	filter: Option<LogWalkerFilter>,
+	partial_extract: AtomicBool,
 	repo: RepoPath,
 }
 
@@ -65,6 +66,7 @@ impl AsyncLog {
 			pending: Arc::new(AtomicBool::new(false)),
 			background: Arc::new(AtomicBool::new(false)),
 			filter,
+			partial_extract: AtomicBool::new(false),
 		}
 	}
 
@@ -89,21 +91,26 @@ impl AsyncLog {
 
 	///
 	pub fn get_items(&self) -> Result<Vec<CommitId>> {
+		if self.partial_extract.load(Ordering::Relaxed) {
+			return Err(Error::Generic(String::from("Faulty usage of AsyncLog: Cannot partially extract items and rely on get_items slice to still work!")));
+		}
+
 		let list = &self.current.lock()?.commits;
 		Ok(list.clone())
 	}
 
 	///
-	pub fn get_last_duration(&self) -> Result<Duration> {
-		Ok(self.current.lock()?.duration)
+	pub fn extract_items(&self) -> Result<Vec<CommitId>> {
+		self.partial_extract.store(true, Ordering::Relaxed);
+		let list = &mut self.current.lock()?.commits;
+		let result = list.clone();
+		list.clear();
+		Ok(result)
 	}
 
 	///
-	pub fn position(&self, id: CommitId) -> Result<Option<usize>> {
-		let list = &self.current.lock()?.commits;
-		let position = list.iter().position(|&x| x == id);
-
-		Ok(position)
+	pub fn get_last_duration(&self) -> Result<Duration> {
+		Ok(self.current.lock()?.duration)
 	}
 
 	///
@@ -143,6 +150,8 @@ impl AsyncLog {
 			return Ok(FetchStatus::NoChange);
 		}
 
+		self.pending.store(true, Ordering::Relaxed);
+
 		self.clear()?;
 
 		let arc_current = Arc::clone(&self.current);
@@ -152,8 +161,6 @@ impl AsyncLog {
 		let filter = self.filter.clone();
 		let repo_path = self.repo.clone();
 
-		self.pending.store(true, Ordering::Relaxed);
-
 		if let Ok(head) = repo(&self.repo)?.head() {
 			*self.current_head.lock()? =
 				head.target().map(CommitId::new);
@@ -192,17 +199,16 @@ impl AsyncLog {
 		let r = repo(repo_path)?;
 		let mut walker =
 			LogWalker::new(&r, LIMIT_COUNT)?.filter(filter);
+
 		loop {
 			entries.clear();
-			let res_is_err = walker.read(&mut entries).is_err();
+			let read = walker.read(&mut entries)?;
 
-			if !res_is_err {
-				let mut current = arc_current.lock()?;
-				current.commits.extend(entries.iter());
-				current.duration = start_time.elapsed();
-			}
+			let mut current = arc_current.lock()?;
+			current.commits.extend(entries.iter());
+			current.duration = start_time.elapsed();
 
-			if res_is_err || entries.len() <= 1 {
+			if read == 0 {
 				break;
 			}
 			Self::notify(sender);
@@ -213,15 +219,19 @@ impl AsyncLog {
 				} else {
 					SLEEP_FOREGROUND
 				};
+
 			thread::sleep(sleep_duration);
 		}
 
+		log::trace!("revlog visited: {}", walker.visited());
+
 		Ok(())
 	}
 
 	fn clear(&mut self) -> Result<()> {
 		self.current.lock()?.commits.clear();
 		*self.current_head.lock()? = None;
+		self.partial_extract.store(false, Ordering::Relaxed);
 		Ok(())
 	}
 
diff --git a/asyncgit/src/sync/logwalker.rs b/asyncgit/src/sync/logwalker.rs
index b67aed7f..718fdcb9 100644
--- a/asyncgit/src/sync/logwalker.rs
+++ b/asyncgit/src/sync/logwalker.rs
@@ -262,6 +262,11 @@ impl<'a> LogWalker<'a> {
 	}
 
 	///
+	pub fn visited(&self) -> usize {
+		self.visited.len()
+	}
+
+	///
 	#[must_use]
 	pub fn filter(self, filter: Option<LogWalkerFilter>) -> Self {
 		Self { filter, ..self }
diff --git a/src/components/commitlist.rs b/src/components/commitlist.rs
index cbfeb821..27208e60 100644
--- a/src/components/commitlist.rs
+++ b/src/components/commitlist.rs
@@ -13,7 +13,7 @@ use crate::{
 };
 use anyhow::Result;
 use asyncgit::sync::{
-	checkout_commit, BranchDetails, BranchInfo, CommitId,
+	self, checkout_commit, BranchDetails, BranchInfo, CommitId,
 	RepoPathRef, Tags,
 };
 use chrono::{DateTime, Local};
@@ -37,14 +37,16 @@ use std::{
 };
 
 const ELEMENTS_PER_LINE: usize = 9;
+const SLICE_SIZE: usize = 1200;
 
 ///
 pub struct CommitList {
 	repo: RepoPathRef,
 	title: Box<str>,
 	selection: usize,
-	count_total: usize,
 	items: ItemBatch,
+	highlights: Option<HashSet<CommitId>>,
+	commits: Vec<CommitId>,
 	marked: Vec<(usize, CommitId)>,
 	scroll_state: (Instant, f32),
 	tags: Option<Tags>,
@@ -71,7 +73,8 @@ impl CommitList {
 			items: ItemBatch::default(),
 			marked: Vec::with_capacity(2),
 			selection: 0,
-			count_total: 0,
+			commits: Vec::new(),
+			highlights: None,
 			scroll_state: (Instant::now(), 0_f32),
 			tags: None,
 			local_branches: BTreeMap::default(),
@@ -86,29 +89,6 @@ impl CommitList {
 	}
 
 	///
-	pub const fn selection(&self) -> usize {
-		self.selection
-	}
-
-	/// will return view size or None before the first render
-	pub fn current_size(&self) -> Option<(u16, u16)> {
-		self.current_size.get()
-	}
-
-	///
-	pub fn set_count_total(&mut self, total: usize) {
-		self.count_total = total;
-		self.selection =
-			cmp::min(self.selection, self.selection_max());
-	}
-
-	///
-	#[allow(clippy::missing_const_for_fn)]
-	pub fn selection_max(&self) -> usize {
-		self.count_total.saturating_sub(1)
-	}
-
-	///
 	pub const fn tags(&self) -> Option<&Tags> {
 		self.tags.as_ref()
 	}
@@ -131,13 +111,6 @@ impl CommitList {
 	}
 
 	///
-	pub fn selected_entry_marked(&self) -> bool {
-		self.selected_entry()
-			.and_then(|e| self.is_marked(&e.id))
-			.unwrap_or_default()
-	}
-
-	///
 	pub fn marked_count(&self) -> usize {
 		self.marked.len()
 	}
@@ -160,6 +133,7 @@ impl CommitList {
 		commits
 	}
 
+	///
 	pub fn copy_commit_hash(&self) -> Result<()> {
 		let marked = self.marked.as_slice();
 		let yank: Option<String> = match marked {
@@ -201,6 +175,110 @@ impl CommitList {
 		Ok(())
 	}
 
+	///
+	pub fn checkout(&mut self) {
+		if let Some(commit_hash) =
+			self.selected_entry().map(|entry| entry.id)
+		{
+			try_or_popup!(
+				self,
+				"failed to checkout commit:",
+				checkout_commit(&self.repo.borrow(), commit_hash)
+			);
+		}
+	}
+
+	///
+	pub fn set_local_branches(
+		&mut self,
+		local_branches: Vec<BranchInfo>,
+	) {
+		self.local_branches.clear();
+
+		for local_branch in local_branches {
+			self.local_branches
+				.entry(local_branch.top_commit)
+				.or_default()
+				.push(local_branch);
+		}
+	}
+
+	///
+	pub fn set_remote_branches(
+		&mut self,
+		remote_branches: Vec<BranchInfo>,
+	) {
+		self.remote_branches.clear();
+
+		for remote_branch in remote_branches {
+			self.remote_branches
+				.entry(remote_branch.top_commit)
+				.or_default()
+				.push(remote_branch);
+		}
+	}
+
+	///
+	pub fn set_commits(&mut self, commits: Vec<CommitId>) {
+		self.commits = commits;
+		self.fetch_commits();
+	}
+
+	///
+	pub fn refresh_extend_data(&mut self, commits: Vec<CommitId>) {
+		let new_commits = !commits.is_empty();
+		self.commits.extend(commits);
+
+		let selection = self.selection();
+		let selection_max = self.selection_max();
+
+		if self.needs_data(selection, selection_max) || new_commits {
+			self.fetch_commits();
+		}
+	}
+
+	///
+	pub fn set_highlighting(
+		&mut self,
+		highlighting: Option<HashSet<CommitId>>,
+	) {
+		self.highlights = highlighting;
+		self.select_next_highlight();
+		self.fetch_commits();
+	}
+
+	///
+	pub fn select_commit(&mut self, id: CommitId) -> Result<()> {
+		let position = self.commits.iter().position(|&x| x == id);
+
+		if let Some(position) = position {
+			self.selection = position;
+			Ok(())
+		} else {
+			anyhow::bail!("Could not select commit. It might not be loaded yet or it might be on a different branch.");
+		}
+	}
+
+	const fn selection(&self) -> usize {
+		self.selection
+	}
+
+	/// will return view size or None before the first render
+	fn current_size(&self) -> Option<(u16, u16)> {
+		self.current_size.get()
+	}
+
+	#[allow(clippy::missing_const_for_fn)]
+	fn selection_max(&self) -> usize {
+		self.commits.len().saturating_sub(1)
+	}
+
+	fn selected_entry_marked(&self) -> bool {
+		self.selected_entry()
+			.and_then(|e| self.is_marked(&e.id))
+			.unwrap_or_default()
+	}
+
 	fn move_selection(&mut self, scroll: ScrollType) -> Result<bool> {
 		let needs_update = if self.items.highlighting() {
 			self.move_selection_highlighting(scroll)?
@@ -239,11 +317,7 @@ impl CommitList {
 
 			self.selection = new_selection;
 
-			if self
-				.selected_entry()
-				.map(|entry| entry.highlighted)
-				.unwrap_or_default()
-			{
+			if self.selection_highlighted() {
 				return Ok(true);
 			}
 		}
@@ -562,65 +636,11 @@ impl CommitList {
 		self.selection.saturating_sub(self.items.index_offset())
 	}
 
-	pub fn select_entry(&mut self, position: usize) {
-		self.selection = position;
-	}
-
-	pub fn checkout(&mut self) {
-		if let Some(commit_hash) =
-			self.selected_entry().map(|entry| entry.id)
-		{
-			try_or_popup!(
-				self,
-				"failed to checkout commit:",
-				checkout_commit(&self.repo.borrow(), commit_hash)
-			);
-		}
-	}
-
-	pub fn set_local_branches(
-		&mut self,
-		local_branches: Vec<BranchInfo>,
-	) {
-		self.local_branches.clear();
-
-		for local_branch in local_branches {
-			self.local_branches
-				.entry(local_branch.top_commit)
-				.or_default()
-				.push(local_branch);
-		}
-	}
-
-	pub fn set_remote_branches(
-		&mut self,
-		remote_branches: Vec<BranchInfo>,
-	) {
-		self.remote_branches.clear();
-
-		for remote_branch in remote_branches {
-			self.remote_branches
-				.entry(remote_branch.top_commit)
-				.or_default()
-				.push(remote_branch);
-		}
-	}
-
-	///
-	pub fn set_items(
-		&mut self,
-		start_index: usize,
-		commits: Vec<asyncgit::sync::CommitInfo>,
-		highlighted: &Option<HashSet<CommitId>>,
-	) {
-		self.items.set_items(start_index, commits, highlighted);
-
-		if self.items.highlighting() {
-			self.select_next_highlight();
+	fn select_next_highlight(&mut self) {
+		if self.highlights.is_none() {
+			return;
 		}
-	}
 
-	fn select_next_highlight(&mut self) {
 		let old_selection = self.selection;
 
 		let mut offset = 0;
@@ -655,15 +675,42 @@ impl CommitList {
 	}
 
 	fn selection_highlighted(&mut self) -> bool {
-		self.selected_entry()
-			.map(|entry| entry.highlighted)
+		let commit = self.commits[self.selection];
+
+		self.highlights
+			.as_ref()
+			.map(|highlights| highlights.contains(&commit))
 			.unwrap_or_default()
 	}
 
-	///
-	pub fn needs_data(&self, idx: usize, idx_max: usize) -> bool {
+	fn needs_data(&self, idx: usize, idx_max: usize) -> bool {
 		self.items.needs_data(idx, idx_max)
 	}
+
+	fn fetch_commits(&mut self) {
+		let want_min =
+			self.selection().saturating_sub(SLICE_SIZE / 2);
+		let commits = self.commits.len();
+
+		let want_min = want_min.min(commits);
+		let slice_end =
+			want_min.saturating_add(SLICE_SIZE).min(commits);
+
+		let commits = sync::get_commits_info(
+			&self.repo.borrow(),
+			&self.commits[want_min..slice_end],
+			self.current_size().map_or(100u16, |size| size.0).into(),
+		);
+
+		if let Ok(commits) = commits {
+			self.items.set_items(
+				want_min,
+				commits,
+				//TODO: optimize via sharable data (BTreeMap that preserves order and lookup)
+				&self.highlights.clone(),
+			);
+		}
+	}
 }
 
 impl DrawableComponent for CommitList {
@@ -690,8 +737,8 @@ impl DrawableComponent for CommitList {
 		let title = format!(
 			"{} {}/{}",
 			self.title,
-			self.count_total.saturating_sub(self.selection),
-			self.count_total,
+			self.commits.len().saturating_sub(self.selection),
+			self.commits.len(),
 		);
 
 		f.render_widget(
@@ -718,7 +765,7 @@ impl DrawableComponent for CommitList {
 			f,
 			area,
 			&self.theme,
-			self.count_total,
+			self.commits.len(),
 			self.selection,
 			Orientation::Vertical,
 		);
diff --git a/src/components/utils/logitems.rs b/src/components/utils/logitems.rs
index 8689393d..fb7115d0 100644
--- a/src/components/utils/logitems.rs
+++ b/src/components/utils/logitems.rs
@@ -113,8 +113,6 @@ impl ItemBatch {
 		commits: Vec<CommitInfo>,
 		highlighted: &Option<HashSet<CommitId>>,
 	) {
-		log::debug!("highlighted: {:?}", highlighted);
-
 		self.items.clear();
 		self.items.extend(commits.into_iter().map(|c| {
 			let id = c.id;
diff --git a/src/tabs/revlog.rs b/src/tabs/revlog.rs
index af0cea00..0bdb87f5 100644
--- a/src/tabs/revlog.rs
+++ b/src/tabs/revlog.rs
@@ -33,10 +33,8 @@ use ratatui::{
 use std::{collections::HashSet, rc::Rc, time::Duration};
 use sync::CommitTags;
 
-const SLICE_SIZE: usize = 1200;
-
 struct LogSearchResult {
-	commits: Vec<CommitId>,
+	commits: usize,
 	options: LogFilterSearchOptions,
 	duration: Duration,
 }
@@ -132,21 +130,14 @@ impl Revlog {
 	///
 	pub fn update(&mut self) -> Result<()> {
 		if self.is_visible() {
-			let log_changed =
-				self.git_log.fetch()? == FetchStatus::Started;
-
-			let search_changed = self.update_search_state()?;
-			let log_changed = log_changed || search_changed;
+			if self.git_log.fetch()? == FetchStatus::Started {
+				self.list.clear();
+			}
 
-			self.list.set_count_total(self.git_log.count()?);
+			self.update_search_state()?;
 
-			let selection = self.list.selection();
-			let selection_max = self.list.selection_max();
-			if self.list.needs_data(selection, selection_max)
-				|| log_changed
-			{
-				self.fetch_commits()?;
-			}
+			self.list
+				.refresh_extend_data(self.git_log.extract_items()?);
 
 			self.git_tags.request(Duration::from_secs(3), false)?;
 
@@ -211,27 +202,6 @@ impl Revlog {
 		Ok(())
 	}
 
-	fn fetch_commits(&mut self) -> Result<()> {
-		let want_min =
-			self.list.selection().saturating_sub(SLICE_SIZE / 2);
-
-		let commits = sync::get_commits_info(
-			&self.repo.borrow(),
-			&self.git_log.get_slice(want_min, SLICE_SIZE)?,
-			self.list
-				.current_size()
-				.map_or(100u16, |size| size.0)
-				.into(),
-		);
-
-		if let Ok(commits) = commits {
-			let highlighted = self.search_result_set();
-			self.list.set_items(want_min, commits, &highlighted);
-		}
-
-		Ok(())
-	}
-
 	fn selected_commit(&self) -> Option<CommitId> {
 		self.list.selected_entry().map(|e| e.id)
 	}
@@ -247,16 +217,9 @@ impl Revlog {
 		})
 	}
 
+	///
 	pub fn select_commit(&mut self, id: CommitId) -> Result<()> {
-		let position = self.git_log.position(id)?;
-
-		if let Some(position) = position {
-			self.list.select_entry(position);
-
-			Ok(())
-		} else {
-			anyhow::bail!("Could not select commit in revlog. It might not be loaded yet or it might be on a different branch.");
-		}
+		self.list.select_commit(id)
 	}
 
 	fn revert_commit(&self) -> Result<()> {
@@ -299,30 +262,16 @@ impl Revlog {
 				Some(filter),
 			);
 
-			async_find.fetch()?;
+			assert_eq!(async_find.fetch()?, FetchStatus::Started);
 
 			self.search = LogSearch::Searching(async_find, options);
 
-			self.fetch_commits()?;
+			self.list.set_highlighting(None);
 		}
 
 		Ok(())
 	}
 
-	fn search_result_set(&self) -> Option<HashSet<CommitId>> {
-		if let LogSearch::Results(results) = &self.search {
-			Some(
-				results
-					.commits
-					.iter()
-					.map(CommitId::clone)
-					.collect::<HashSet<_>>(),
-			)
-		} else {
-			None
-		}
-	}
-
 	fn update_search_state(&mut self) -> Result<bool> {
 		let changes = match &self.search {
 			LogSearch::Off | LogSearch::Results(_) => false,
@@ -330,11 +279,17 @@ impl Revlog {
 				if search.is_pending() {
 					false
 				} else {
-					let results = search.get_items()?;
+					let results = search.extract_items()?;
+					let commits = results.len();
 					let duration = search.get_last_duration()?;
+
+					self.list.set_highlighting(Some(
+						results.into_iter().collect::<HashSet<_>>(),
+					));
+
 					self.search =
 						LogSearch::Results(LogSearchResult {
-							commits: results,
+							commits,
 							options: options.clone(),
 							duration,
 						});
@@ -350,7 +305,6 @@ impl Revlog {
 		!matches!(self.search, LogSearch::Off)
 	}
 
-	//TODO: draw time a search took
 	fn draw_search<B: Backend>(&self, f: &mut Frame<B>, area: Rect) {
 		let text = match &self.search {
 			LogSearch::Searching(_, options) => {
@@ -363,7 +317,7 @@ impl Revlog {
 				format!(
 					"'{}' (hits: {}) (duration: {:?})",
 					results.options.search_pattern.clone(),
-					results.commits.len(),
+					results.commits,
 					results.duration,
 				)
 			}
@@ -456,7 +410,7 @@ impl Component for Revlog {
 				) {
 					if self.can_leave_search() {
 						self.search = LogSearch::Off;
-						self.fetch_commits()?;
+						self.list.set_highlighting(None);
 						return Ok(EventState::Consumed);
 					}
 				} else if key_match(k, self.key_config.keys.copy) {
diff --git a/src/tabs/stashlist.rs b/src/tabs/stashlist.rs
index 1ac691b0..67e40087 100644
--- a/src/tabs/stashlist.rs
+++ b/src/tabs/stashlist.rs
@@ -48,14 +48,7 @@ impl StashList {
 	pub fn update(&mut self) -> Result<()> {
 		if self.is_visible() {
 			let stashes = sync::get_stashes(&self.repo.borrow())?;
-			let commits = sync::get_commits_info(
-				&self.repo.borrow(),
-				stashes.as_slice(),
-				100,
-			)?;
-
-			self.list.set_count_total(commits.len());
-			self.list.set_items(0, commits, &None);
+			self.list.set_commits(stashes);
 		}
 
 		Ok(())