From 9a4dc92746147103cf655728c33ea32600a807d8 Mon Sep 17 00:00:00 2001 From: Jozef Mokry Date: Wed, 8 Oct 2025 09:08:05 +0200 Subject: [PATCH] Check for commits from others in PRs This PR adds a new flag `checkForCommitsFromOthers` that when enabled (false by default) will check if the last commit on the PR is from the same committer as that of the local commit. This is useful for scenarios when either multiple people collaborate on the same PR, or when the CI is set up such that it automatically adds a code formatting commit if needed. Currently in such scenarios it is very easy to run `spr diff` and lose all of the work from others that was added to the PR since you ran `spr diff` the last time. Test Plan: I tried running `spr diff` in a repo where CI automatically pushes a commit to fix code formatting. The check was triggered. The first time I followed the instructions with `git merge`, the second time I typed "yes". (In my test case the `git merge` did not actually work because the the automated CI commit was just removing the trailing whitespace I added in my test PR, thus making the whole PR empty. Therefore the `git merge` locally did not trigger a merge conflict, nor did it remove the whitespace I had in my local commit. Maybe a different merge strategy is needed? Or maybe the extra commits from the PR branch need to be squash-rebased into the local commit? Open to suggestions) --- src/commands/diff.rs | 45 +++++++++++++++++++++++++++++++++++++++++++- src/config.rs | 4 ++++ src/main.rs | 5 +++++ src/output.rs | 15 +++++++++++++++ 4 files changed, 68 insertions(+), 1 deletion(-) diff --git a/src/commands/diff.rs b/src/commands/diff.rs index df0eb1f..250be60 100644 --- a/src/commands/diff.rs +++ b/src/commands/diff.rs @@ -18,7 +18,7 @@ use crate::{ PullRequestUpdate, }, message::{MessageSection, validate_commit_message}, - output::{output, write_commit_title}, + output::{output, write_commit_info, write_commit_title}, utils::{parse_name_list, remove_all_parens, slugify}, }; use git2::Oid; @@ -435,6 +435,49 @@ async fn diff_impl( } } + if let Some(ref pull_request) = pull_request + && config.check_for_commits_from_others + { + let last_pr_commit = git.repo().find_commit(pull_request.head_oid)?; + let last_pr_committer = last_pr_commit.committer(); + let local_commit = git.repo().find_commit(local_commit.oid)?; + let local_committer = local_commit.committer(); + if local_committer.name() != last_pr_committer.name() { + let commit_id = last_pr_commit.id().to_string(); + let summary = last_pr_commit.summary().unwrap_or(""); + let pr_committer = last_pr_committer.name().unwrap_or("unknown"); + output("⚠️", "The last PR commit is not from you:")?; + write_commit_info(&commit_id[0..7], &summary, &pr_committer)?; + let pr_branch_name = pull_request.head.branch_name().to_string(); + let input = tokio::task::spawn_blocking(move || { + dialoguer::Input::<String>::new() + .with_prompt(formatdoc!( + " + + Please select what you want do: + 1) Type 'yes' to continue (risks losing changes from others) + 2) Hit Enter to stop + + Then manually merge in the changes from the PR: + + git fetch origin {pr_branch_name} + git merge --squash origin/{pr_branch_name} + git commit --amend + + Then run `spr diff` again. + + ", + )) + .allow_empty(true) + .interact_text() + }) + .await??; + if input.is_empty() || input != "yes" { + bail!("Aborted as per user request"); + } + } + } + // Check if there is a base branch on GitHub already. That's the case when // there is an existing Pull Request, and its base is not the master branch. let base_branch = if let Some(ref pr) = pull_request { diff --git a/src/config.rs b/src/config.rs index f054c61..cee1033 100644 --- a/src/config.rs +++ b/src/config.rs @@ -18,6 +18,7 @@ pub struct Config { pub auth_token: String, pub require_approval: bool, pub require_test_plan: bool, + pub check_for_commits_from_others: bool, } impl Config { @@ -30,6 +31,7 @@ impl Config { auth_token: String, require_approval: bool, require_test_plan: bool, + check_for_commits_from_others: bool, ) -> Self { let master_ref = GitHubBranch::new_from_branch_name(&master_branch, &master_branch); @@ -41,6 +43,7 @@ impl Config { auth_token, require_approval, require_test_plan, + check_for_commits_from_others, } } @@ -106,6 +109,7 @@ mod tests { "xyz".into(), false, true, + false, ) } diff --git a/src/main.rs b/src/main.rs index 2d77961..6e7f1a3 100644 --- a/src/main.rs +++ b/src/main.rs @@ -138,6 +138,10 @@ pub async fn spr() -> Result<()> { .get_bool("spr.requireTestPlan") .ok() .unwrap_or(true); + let check_for_commits_from_others = git_config + .get_bool("spr.checkForCommitsFromOthers") + .ok() + .unwrap_or(false); let github_auth_token = match cli.github_auth_token { Some(v) => Ok(v), @@ -152,6 +156,7 @@ pub async fn spr() -> Result<()> { github_auth_token.clone(), require_approval, require_test_plan, + check_for_commits_from_others, ); debug!("config: {:?}", config); diff --git a/src/output.rs b/src/output.rs index e8e54c8..28dffa2 100644 --- a/src/output.rs +++ b/src/output.rs @@ -42,3 +42,18 @@ pub fn write_commit_title(prepared_commit: &PreparedCommit) -> Result<()> { ))?; Ok(()) } + +pub fn write_commit_info( + short_id: &str, + title: &str, + committer: &str, +) -> Result<()> { + let term = console::Term::stdout(); + term.write_line(&format!( + "{} {} (from {})", + console::style(short_id).italic(), + console::style(title).yellow(), + console::style(committer) + ))?; + Ok(()) +}