Merge 2f0635348f into 8619c07f3f

asyncgit/src/sync/mod.rs

@@ -20,6 +20,7 @@ mod ignore;
 mod logwalker;
 mod merge;
 mod patches;
+mod push_todo;
 mod rebase;
 pub mod remotes;
 mod repository;
@@ -78,6 +79,9 @@ pub use merge::{
 	continue_pending_rebase, merge_branch, merge_commit, merge_msg,
 	mergehead_ids, rebase_progress,
 };
+pub use push_todo::{
+	find_push_todo_markers, format_push_todo_markers, PushTodoMarker,
+};
 pub use rebase::rebase_branch;
 pub use remotes::{
 	add_remote, delete_remote, get_default_remote,

asyncgit/src/sync/push_todo.rs

@@ -0,0 +1,189 @@
+//! Detect TODO/FIXME markers in commits that would be pushed.
+
+use super::{commit_files::get_commit_diff, repository::repo, CommitId, RepoPath};
+use crate::error::Result;
+use git2::{BranchType, Diff, Oid, Repository};
+use scopetime::scope_time;
+
+/// Location of a TODO/FIXME marker in a commit to be pushed.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub struct PushTodoMarker {
+	///
+	pub commit: CommitId,
+	///
+	pub commit_short: String,
+	///
+	pub file: String,
+	///
+	pub line: u32,
+	///
+	pub kind: String,
+}
+
+///
+pub fn find_push_todo_markers(
+	repo_path: &RepoPath,
+	branch: &str,
+) -> Result<Vec<PushTodoMarker>> {
+	scope_time!("find_push_todo_markers");
+
+	let repo = repo(repo_path)?;
+	let local_branch = repo.find_branch(branch, BranchType::Local)?;
+	let upstream_oid = local_branch
+		.upstream()
+		.ok()
+		.and_then(|upstream| {
+			upstream
+				.into_reference()
+				.peel_to_commit()
+				.ok()
+				.map(|commit| commit.id())
+		});
+	let local_oid = local_branch.into_reference().peel_to_commit()?.id();
+
+	let commit_ids = if let Some(upstream_oid) = upstream_oid {
+		commits_in_range(&repo, upstream_oid, local_oid)?
+	} else {
+		vec![CommitId::new(local_oid)]
+	};
+
+	let mut markers = Vec::new();
+	for id in commit_ids {
+		let commit = repo.find_commit(id.into())?;
+		let short = commit.id().to_string()[..7.min(commit.id().to_string().len())]
+			.to_string();
+		let diff = get_commit_diff(&repo, id, None, None, None)?;
+		collect_todo_markers_from_diff(&diff, id, short, &mut markers);
+	}
+
+	Ok(markers)
+}
+
+///
+pub fn format_push_todo_markers(markers: &[PushTodoMarker]) -> String {
+	const LIMIT: usize = 15;
+	let mut lines: Vec<String> = markers
+		.iter()
+		.take(LIMIT)
+		.map(|m| {
+			format!(
+				"  {} {}:{} ({})",
+				m.commit_short, m.file, m.line, m.kind
+			)
+		})
+		.collect();
+	if markers.len() > LIMIT {
+		lines.push(format!(
+			"  … and {} more",
+			markers.len() - LIMIT
+		));
+	}
+	lines.join("\n")
+}
+
+fn commits_in_range(
+	repo: &Repository,
+	upstream: Oid,
+	local: Oid,
+) -> Result<Vec<CommitId>> {
+	let mut revwalk = repo.revwalk()?;
+	revwalk.hide(upstream)?;
+	revwalk.push(local)?;
+	revwalk
+		.map(|id| id.map(CommitId::new))
+		.collect::<std::result::Result<Vec<_>, _>>()
+		.map_err(Into::into)
+}
+
+fn collect_todo_markers_from_diff(
+	diff: &Diff<'_>,
+	commit: CommitId,
+	commit_short: String,
+	out: &mut Vec<PushTodoMarker>,
+) {
+	let _ = diff.foreach(
+		&mut |_delta, _progress| true,
+		None,
+		None,
+		Some(&mut |delta, _hunk, line| {
+			if line.origin() != '+' {
+				return true;
+			}
+			let Some(text) = std::str::from_utf8(line.content()).ok() else {
+				return true;
+			};
+			let Some(kind) = line_contains_marker(text) else {
+				return true;
+			};
+			let file = delta
+				.new_file()
+				.path()
+				.and_then(|p| p.to_str())
+				.unwrap_or("?")
+				.to_string();
+			out.push(PushTodoMarker {
+				commit,
+				commit_short: commit_short.clone(),
+				file,
+				line: line.new_lineno().unwrap_or(0),
+				kind: kind.to_string(),
+			});
+			true
+		}),
+	);
+}
+
+fn line_contains_marker(line: &str) -> Option<&'static str> {
+	if contains_word(line, "TODO") {
+		Some("TODO")
+	} else if contains_word(line, "FIXME") {
+		Some("FIXME")
+	} else {
+		None
+	}
+}
+
+fn contains_word(haystack: &str, word: &str) -> bool {
+	haystack
+		.split(|c: char| !c.is_ascii_alphanumeric() && c != '_')
+		.any(|part| part.eq_ignore_ascii_case(word))
+}
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+	use crate::sync::{
+		tests::{repo_init, write_commit_file},
+		RepoPath,
+	};
+
+	#[test]
+	fn test_find_push_todo_markers() -> Result<()> {
+		let (_td, repo) = repo_init()?;
+		let root = repo.path().parent().unwrap();
+		let repo_path: &RepoPath =
+			&root.as_os_str().to_str().unwrap().into();
+
+		write_commit_file(&repo, "todo.rs", "fn ok() {}\n", "init");
+		write_commit_file(
+			&repo,
+			"todo.rs",
+			"fn ok() { // TODO: fix }\n",
+			"add todo",
+		);
+
+		let markers = find_push_todo_markers(repo_path, "master")?;
+		assert_eq!(markers.len(), 1);
+		assert_eq!(markers[0].kind, "TODO");
+		assert_eq!(markers[0].file, "todo.rs");
+
+		Ok(())
+	}
+
+	#[test]
+	fn test_contains_word() {
+		assert!(contains_word("// TODO: fix", "TODO"));
+		assert!(!contains_word("// TODOLIST", "TODO"));
+		assert!(contains_word("FIXME here", "FIXME"));
+	}
+}

src/app.rs

@@ -148,6 +148,25 @@ impl Environment {
 	}
 }
 
+/// Resolves `--file` to a path relative to the repository workdir (as used by the file tree).
+fn resolve_select_file(file: PathBuf, repo: &RepoPath) -> Result<Option<PathBuf>> {
+	let workdir = PathBuf::from(repo_work_dir(repo)?);
+	let workdir_canon = workdir.canonicalize()?;
+
+	let candidates = [
+		file.canonicalize().ok(),
+		workdir.join(&file).canonicalize().ok(),
+	];
+
+	for abs in candidates.into_iter().flatten() {
+		if let Ok(rel) = abs.strip_prefix(&workdir_canon) {
+			return Ok(Some(Path::new(".").join(rel)));
+		}
+	}
+
+	Ok(None)
+}
+
 // public interface
 impl App {
 	///
@@ -178,14 +197,7 @@ impl App {
 
 		let mut select_file: Option<PathBuf> = None;
 		let tab = if let Some(file) = cliargs.select_file {
-			// convert to relative git path
-			if let Ok(abs) = file.canonicalize() {
-				if let Ok(path) = abs.strip_prefix(
-					env.repo.borrow().gitpath().canonicalize()?,
-				) {
-					select_file = Some(Path::new(".").join(path));
-				}
-			}
+			select_file = resolve_select_file(file, &env.repo.borrow())?;
 			2
 		} else {
 			env.options.borrow().current_tab()
@@ -1031,6 +1043,20 @@ impl App {
 					undo_last_commit(&self.repo.borrow())
 				);
 			}
+			Action::PushDespiteTodos { branch, force, .. } => {
+				if force {
+					self.queue.push(InternalEvent::ConfirmAction(
+						Action::ForcePush(branch, force),
+					));
+				} else {
+					self.queue.push(InternalEvent::Push(
+						branch,
+						PushType::Branch,
+						force,
+						false,
+					));
+				}
+			}
 		}
 
 		flags.insert(NeedsUpdate::ALL);

src/popups/confirm.rs

@@ -214,6 +214,10 @@ impl ConfirmPopup {
                     strings::confirm_title_undo_commit(),
                     strings::confirm_msg_undo_commit(),
                 ),
+				Action::PushDespiteTodos { details, .. } => (
+					strings::confirm_title_push_todos(),
+					strings::confirm_msg_push_todos(details),
+				),
             };
 		}
 

src/queue.rs

@@ -58,6 +58,11 @@ pub enum Action {
 	AbortRebase,
 	AbortRevert,
 	UndoCommit,
+	PushDespiteTodos {
+		branch: String,
+		force: bool,
+		details: String,
+	},
 }
 
 #[derive(Debug)]

src/strings.rs

@@ -300,6 +300,14 @@ pub fn confirm_msg_force_push(
         "Confirm force push to branch '{branch_ref}' ?  This may rewrite history."
     )
 }
+pub fn confirm_title_push_todos() -> String {
+	"Push with TODO/FIXME".to_string()
+}
+pub fn confirm_msg_push_todos(details: &str) -> String {
+	format!(
+		"The following commits contain TODO or FIXME markers in added lines:\n\n{details}\n\nPush anyway?"
+	)
+}
 pub fn log_title(_key_config: &SharedKeyConfig) -> String {
 	"Commit".to_string()
 }

src/tabs/status.rs

@@ -565,22 +565,47 @@ impl Status {
 	fn push(&self, force: bool) {
 		if self.can_push() {
 			if let Some(branch) = self.git_branch_name.last() {
-				if force {
-					self.queue.push(InternalEvent::ConfirmAction(
-						Action::ForcePush(branch, force),
-					));
-				} else {
-					self.queue.push(InternalEvent::Push(
-						branch,
-						PushType::Branch,
-						force,
-						false,
-					));
+				match sync::find_push_todo_markers(
+					&self.repo.borrow(),
+					branch.as_str(),
+				) {
+					Ok(markers) if !markers.is_empty() => {
+						let details =
+							sync::format_push_todo_markers(&markers);
+						self.queue.push(InternalEvent::ConfirmAction(
+							Action::PushDespiteTodos {
+								branch: branch.clone(),
+								force,
+								details,
+							},
+						));
+					}
+					_ => {
+						self.push_without_todo_warning(
+							branch.as_str(),
+							force,
+						);
+					}
 				}
 			}
 		}
 	}
 
+	fn push_without_todo_warning(&self, branch: &str, force: bool) {
+		if force {
+			self.queue.push(InternalEvent::ConfirmAction(
+				Action::ForcePush(branch.to_string(), force),
+			));
+		} else {
+			self.queue.push(InternalEvent::Push(
+				branch.to_string(),
+				PushType::Branch,
+				force,
+				false,
+			));
+		}
+	}
+
 	fn fetch(&self) {
 		if self.can_fetch() {
 			self.queue.push(InternalEvent::FetchRemotes);
@@ -845,6 +870,16 @@ impl Component for Status {
 				) && self.can_focus_diff()
 				{
 					self.switch_focus(Focus::Diff).map(Into::into)
+				} else if key_match(
+					k,
+					self.key_config.keys.move_left,
+				) && self.is_focus_on_diff()
+				{
+					self.switch_focus(match self.diff_target {
+						DiffTarget::Stage => Focus::Stage,
+						DiffTarget::WorkingDir => Focus::WorkDir,
+					})
+					.map(Into::into)
 				} else if key_match(
 					k,
 					self.key_config.keys.exit_popup,