diff --git a/asyncgit/src/sync/mod.rs b/asyncgit/src/sync/mod.rs index 2a5f413e8f..cd8cb414d7 100644 --- a/asyncgit/src/sync/mod.rs +++ b/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, diff --git a/asyncgit/src/sync/push_todo.rs b/asyncgit/src/sync/push_todo.rs new file mode 100644 index 0000000000..2bb1943831 --- /dev/null +++ b/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> { + 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 = 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> { + let mut revwalk = repo.revwalk()?; + revwalk.hide(upstream)?; + revwalk.push(local)?; + revwalk + .map(|id| id.map(CommitId::new)) + .collect::, _>>() + .map_err(Into::into) +} + +fn collect_todo_markers_from_diff( + diff: &Diff<'_>, + commit: CommitId, + commit_short: String, + out: &mut Vec, +) { + 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")); + } +} diff --git a/src/app.rs b/src/app.rs index 8626aa3b8e..968d15ae40 100644 --- a/src/app.rs +++ b/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> { + 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 = 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); diff --git a/src/popups/confirm.rs b/src/popups/confirm.rs index 9910a321f3..66224abd4f 100644 --- a/src/popups/confirm.rs +++ b/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), + ), }; } diff --git a/src/queue.rs b/src/queue.rs index 5cdfe3cef0..a083d56b4d 100644 --- a/src/queue.rs +++ b/src/queue.rs @@ -58,6 +58,11 @@ pub enum Action { AbortRebase, AbortRevert, UndoCommit, + PushDespiteTodos { + branch: String, + force: bool, + details: String, + }, } #[derive(Debug)] diff --git a/src/strings.rs b/src/strings.rs index f66a9e93f5..a0b62bbe78 100644 --- a/src/strings.rs +++ b/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() } diff --git a/src/tabs/status.rs b/src/tabs/status.rs index 135cf18e4e..000fa10c9a 100644 --- a/src/tabs/status.rs +++ b/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,