From 03e0fa77f150e55194018cb7b387480449a07ce8 Mon Sep 17 00:00:00 2001 From: Andrei Gherzan Date: Wed, 22 Jul 2026 23:35:40 +0100 Subject: [PATCH] feat: validate only commits not yet on any remote When pushing a new branch, the pre-push hook derived its base from a single remote's HEAD. If that remote was behind (e.g. a fork whose main is a stale copy of upstream), it re-validated commits already reviewed and merged elsewhere, and could reject a push over commits the author never touched. Base validation on every remote-tracking ref instead: a commit already present on any remote has been published and does not need rechecking. Excluding a set of refs rather than a single base also lets the root commit be validated, which a single exclusive base can never reach. Closes: #10 Signed-off-by: Andrei Gherzan --- README.md | 11 +++ githooks/pre-push | 41 +++++---- src/git.rs | 165 ++++++++++++++++++++++++++++++++++--- src/lib.rs | 4 +- src/main.rs | 49 ++++++++++- tests/integration_tests.rs | 92 +++++++++++++++++++-- 6 files changed, 324 insertions(+), 38 deletions(-) diff --git a/README.md b/README.md index 0b6cee1..8439c67 100644 --- a/README.md +++ b/README.md @@ -260,6 +260,17 @@ pass its path explicitly: gitlance --message-file .git/COMMIT_EDITMSG ``` +### Validating Unpublished Commits + +With `--not-on-remotes`, gitlance validates only the commits reachable from +`--head` that are not yet present on any remote. This makes it convenient to use +in a local `pre-push` git hook. It exits successfully when there is nothing new +to check: + +```bash +gitlance --head --not-on-remotes +``` + ## Installation ### GitHub Action diff --git a/githooks/pre-push b/githooks/pre-push index 5c5996b..d49ab5a 100755 --- a/githooks/pre-push +++ b/githooks/pre-push @@ -34,35 +34,34 @@ do continue fi - # Determine base commit + # Run gitlance + echo "Checking git commit logs with gitlance:" + echo if [ "$remote_oid" = "0000000000000000000000000000000000000000" ]; then - # New branch - try to find common ancestor with remote HEAD - base="" - if remote_head=$(git ls-remote "$remote" HEAD 2>/dev/null | awk '{print $1}') && [ -n "$remote_head" ]; then - base=$(git merge-base "$remote_head" "$local_oid" 2>/dev/null) || true - fi - # Fall back to root commit if no common ancestor found - if [ -z "$base" ]; then - base=$(git rev-list --max-parents=0 "$local_oid" | head -1) + # New branch: validate commits not yet present on any remote. gitlance + # derives the set from remote-tracking refs, which is more reliable than + # a single remote's HEAD (a stale remote would re-check known commits). + if ! gitlance --head "$local_oid" --not-on-remotes; then + echo "" + echo "Push rejected: Commit validation failed" + echo "To fix: git commit --amend, or git rebase -i" + echo "To bypass: git push --no-verify" + exit 1 fi else - # Existing branch - find common ancestor to only check divergent commits + # Existing branch: check only commits that diverge from the remote. if ! base=$(git merge-base "$remote_oid" "$local_oid"); then echo "Error: Failed to find merge-base between remote and local commits" echo "To bypass: git push --no-verify" exit 1 fi - fi - - # Run gitlance - echo "Checking git commit logs with gitlance:" - echo - if ! gitlance --base "$base" --head "$local_oid"; then - echo "" - echo "Push rejected: Commit validation failed" - echo "To fix: git commit --amend, or git rebase -i $base" - echo "To bypass: git push --no-verify" - exit 1 + if ! gitlance --base "$base" --head "$local_oid"; then + echo "" + echo "Push rejected: Commit validation failed" + echo "To fix: git commit --amend, or git rebase -i $base" + echo "To bypass: git push --no-verify" + exit 1 + fi fi done diff --git a/src/git.rs b/src/git.rs index 7ce436d..33bd52a 100644 --- a/src/git.rs +++ b/src/git.rs @@ -63,21 +63,46 @@ pub fn resolve_ref(repo: &Repository, refspec: &str) -> Result }) } -/// Gets all commits in the range [base, head] -/// Returns commits from base (exclusive) to head (inclusive) +/// Lists all remote-tracking refs (`refs/remotes/*`) in the repository. /// -/// Accepts any valid git revision specification for base and head: +/// These represent everything already pushed to or fetched from any remote, +/// and are used to determine which commits are genuinely new. +pub fn remote_tracking_refs(repo: &Repository) -> Result, CheckError> { + let refs = repo + .references_glob("refs/remotes/*") + .map_err(|e| CheckError::Git(format!("Failed to list remote-tracking refs: {}", e)))?; + + let mut names = Vec::new(); + for reference in refs { + let reference = reference + .map_err(|e| CheckError::Git(format!("Failed to read remote-tracking ref: {}", e)))?; + let name = reference + .name() + .map_err(|e| CheckError::Git(format!("Remote-tracking ref has invalid name: {}", e)))?; + names.push(name.to_string()); + } + + Ok(names) +} + +/// Gets all commits reachable from `head` but not from any of `excludes`. +/// +/// This is the core commit-selection routine. Each exclude ref is hidden from +/// the revwalk, so the result contains only commits unique to `head`. Passing a +/// single base gives a `base..head` range; passing all remote-tracking refs +/// gives the commits that are not yet on any remote. +/// +/// Accepts any valid git revision specification for `head` and each exclude: /// - Full/short SHAs, branches, tags, HEAD~n, etc. /// /// If `skip_merge_commits` is true, merge commits (commits with more than one parent) /// are excluded from the results. -pub fn get_commits_in_range( +pub fn get_commits_excluding( repo: &Repository, - base: &str, head: &str, + excludes: &[impl AsRef], skip_merge_commits: bool, ) -> Result, CheckError> { - let base_oid = resolve_ref(repo, base)?; let head_oid = resolve_ref(repo, head)?; let mut revwalk = repo @@ -89,10 +114,14 @@ pub fn get_commits_in_range( .push(head_oid) .map_err(|e| CheckError::Git(format!("Failed to push head to revwalk: {}", e)))?; - // Don't include the base commit itself - revwalk - .hide(base_oid) - .map_err(|e| CheckError::Git(format!("Failed to hide base in revwalk: {}", e)))?; + // Hide each exclude ref so its commits (and their ancestors) are omitted + for exclude in excludes { + let exclude = exclude.as_ref(); + let exclude_oid = resolve_ref(repo, exclude)?; + revwalk.hide(exclude_oid).map_err(|e| { + CheckError::Git(format!("Failed to hide '{}' in revwalk: {}", exclude, e)) + })?; + } let mut commits = Vec::new(); @@ -121,6 +150,23 @@ pub fn get_commits_in_range( Ok(commits) } +/// Gets all commits in the range [base, head] +/// Returns commits from base (exclusive) to head (inclusive) +/// +/// Accepts any valid git revision specification for base and head: +/// - Full/short SHAs, branches, tags, HEAD~n, etc. +/// +/// If `skip_merge_commits` is true, merge commits (commits with more than one parent) +/// are excluded from the results. +pub fn get_commits_in_range( + repo: &Repository, + base: &str, + head: &str, + skip_merge_commits: bool, +) -> Result, CheckError> { + get_commits_excluding(repo, head, &[base], skip_merge_commits) +} + #[cfg(test)] mod tests { use super::*; @@ -442,4 +488,103 @@ mod tests { let commit_clone = commit.clone(); assert_eq!(commit_clone.sha, commit.sha); } + + #[test] + fn test_remote_tracking_refs_lists_remote_refs() { + let temp_dir = TempDir::new().expect("Failed to create temp dir"); + let repo_path = create_test_repo( + temp_dir + .path() + .to_str() + .expect("Failed to convert temp dir path to string"), + ); + let repo = open_repo(&repo_path).expect("Failed to open repo"); + + let base_sha = create_commit(&repo_path, "initial"); + + // Simulate a fetched remote-tracking ref by writing it directly. + run_cmd( + &repo_path, + "git", + &["update-ref", "refs/remotes/origin/main", &base_sha], + ); + + let refs = remote_tracking_refs(&repo).expect("Failed to list remote-tracking refs"); + assert_eq!(refs, vec!["refs/remotes/origin/main".to_string()]); + } + + #[test] + fn test_get_commits_excluding_empty_excludes_includes_root() { + let temp_dir = TempDir::new().expect("Failed to create temp dir"); + let repo_path = create_test_repo( + temp_dir + .path() + .to_str() + .expect("Failed to convert temp dir path to string"), + ); + let repo = open_repo(&repo_path).expect("Failed to open repo"); + + let _base_sha = create_commit(&repo_path, "initial"); + let _sha1 = create_commit(&repo_path, "second commit"); + + // With no excludes, every commit reachable from head is returned, + // including the root commit that has no parent. + let commits = get_commits_excluding(&repo, "HEAD", &[] as &[&str], false) + .expect("Failed to get commits"); + + assert_eq!(commits.len(), 2); + assert_eq!(commits[0].message.trim(), "second commit"); + assert_eq!(commits[1].message.trim(), "initial"); + } + + #[test] + fn test_get_commits_excluding_head_fully_excluded_is_empty() { + let temp_dir = TempDir::new().expect("Failed to create temp dir"); + let repo_path = create_test_repo( + temp_dir + .path() + .to_str() + .expect("Failed to convert temp dir path to string"), + ); + let repo = open_repo(&repo_path).expect("Failed to open repo"); + + let _base_sha = create_commit(&repo_path, "initial"); + let _sha1 = create_commit(&repo_path, "second commit"); + + // Excluding head itself leaves nothing new. + let commits = + get_commits_excluding(&repo, "HEAD", &["HEAD"], false).expect("Failed to get commits"); + + assert!(commits.is_empty()); + } + + #[test] + fn test_get_commits_excluding_via_remote_tracking_refs() { + let temp_dir = TempDir::new().expect("Failed to create temp dir"); + let repo_path = create_test_repo( + temp_dir + .path() + .to_str() + .expect("Failed to convert temp dir path to string"), + ); + let repo = open_repo(&repo_path).expect("Failed to open repo"); + + let _base_sha = create_commit(&repo_path, "initial"); + let sha1 = create_commit(&repo_path, "second commit"); + let _sha2 = create_commit(&repo_path, "third commit"); + + // Pretend the first two commits are already published on a remote. + run_cmd( + &repo_path, + "git", + &["update-ref", "refs/remotes/origin/main", &sha1], + ); + + let excludes = remote_tracking_refs(&repo).expect("Failed to list remote-tracking refs"); + let commits = + get_commits_excluding(&repo, "HEAD", &excludes, false).expect("Failed to get commits"); + + assert_eq!(commits.len(), 1); + assert_eq!(commits[0].message.trim(), "third commit"); + } } diff --git a/src/lib.rs b/src/lib.rs index 449d0f4..027d5c4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -11,7 +11,9 @@ pub mod output; pub mod test_utils; pub use error::CheckError; -pub use git::{get_commits_in_range, open_repo, resolve_ref}; +pub use git::{ + get_commits_excluding, get_commits_in_range, open_repo, remote_tracking_refs, resolve_ref, +}; /// Length to abbreviate SHAs in output messages const SHA_ABBREV_LEN: usize = 8; diff --git a/src/main.rs b/src/main.rs index d88b239..954f571 100644 --- a/src/main.rs +++ b/src/main.rs @@ -29,8 +29,13 @@ struct Cli { #[arg(long, global = true)] skip_merge_commits: bool, + /// Validate only commits not yet present on any remote (requires --head, + /// mutually exclusive with --base). Exits successfully when nothing is new. + #[arg(long, global = true, conflicts_with = "base")] + not_on_remotes: bool, + /// Validate a single commit message from file (e.g. .git/COMMIT_EDITMSG) - #[arg(long, global = true, conflicts_with_all = ["base", "head", "skip_merge_commits", "repo"])] + #[arg(long, global = true, conflicts_with_all = ["base", "head", "skip_merge_commits", "repo", "not_on_remotes"])] message_file: Option, } @@ -78,6 +83,48 @@ fn main() { exit(1); } } + } else if cli.not_on_remotes { + // Validate only commits reachable from head but not from any remote. + let head = cli.head.or_else(|| std::env::var("HEAD_REF").ok()); + + let head = match head { + Some(h) => h, + None => { + output::error("Missing --head (or HEAD_REF)"); + exit(1); + } + }; + + let repo = match git::open_repo(cli.repo.as_deref().unwrap_or(".")) { + Ok(r) => r, + Err(e) => { + output::error(&format!("Failed to open repository: {}", e)); + exit(1); + } + }; + + let excludes = match git::remote_tracking_refs(&repo) { + Ok(refs) => refs, + Err(e) => { + output::error(&format!("Failed to list remote-tracking refs: {}", e)); + exit(1); + } + }; + + match git::get_commits_excluding(&repo, &head, &excludes, cli.skip_merge_commits) { + // An empty result means every commit is already on a remote, so + // there is nothing new to validate. This is a clean pass, not an + // error: the revwalk semantics make emptiness a precise signal. + Ok(commits) if commits.is_empty() => { + println!("No new commits to check."); + exit(0); + } + Ok(commits) => commits, + Err(e) => { + output::error(&format!("Failed to get commits: {}", e)); + exit(1); + } + } } else { let base = cli.base.or_else(|| std::env::var("BASE_REF").ok()); let head = cli.head.or_else(|| std::env::var("HEAD_REF").ok()); diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index 6610792..13797e3 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -7,15 +7,16 @@ mod tests { use gitlance::test_utils::*; use tempfile::TempDir; - /// Runs the binary with specific check and arguments (for integration testing). - /// Arguments can be omitted (None) to test error cases. - fn run_check( + /// Builds and runs the binary with the given arguments, returning the raw + /// process output. Any argument can be omitted (None) to test error cases. + fn run( check: Option<&str>, repo_path: Option<&str>, base: Option<&str>, head: Option<&str>, message_file: Option<&str>, - ) -> bool { + not_on_remotes: bool, + ) -> std::process::Output { use assert_cmd::Command; let mut cmd = Command::cargo_bin("gitlance").expect("Failed to find binary"); @@ -37,8 +38,25 @@ mod tests { if let Some(f) = message_file { cmd.args(["--message-file", f]); } + if not_on_remotes { + cmd.arg("--not-on-remotes"); + } - cmd.ok().is_ok() + cmd.output().expect("Failed to run binary") + } + + /// Runs the binary and reports whether it exited successfully. + /// Arguments can be omitted (None) to test error cases. + fn run_check( + check: Option<&str>, + repo_path: Option<&str>, + base: Option<&str>, + head: Option<&str>, + message_file: Option<&str>, + ) -> bool { + run(check, repo_path, base, head, message_file, false) + .status + .success() } // ===== All Checks Tests ===== @@ -342,6 +360,70 @@ mod tests { ); } + // ===== Not-on-remotes Tests ===== + + #[test] + fn test_not_on_remotes_checks_only_unpublished_commits() { + let temp_dir = TempDir::new().expect("Failed to create temp dir"); + let repo_path = create_test_repo( + temp_dir + .path() + .to_str() + .expect("Failed to convert temp dir path to string"), + ); + + let published = create_commit(&repo_path, "chore: initial"); + let message = "feat: add feature\n\nSigned-off-by: Test User "; + let _new = create_commit(&repo_path, message); + + // Mark the first commit as already present on a remote. + run_cmd( + &repo_path, + "git", + &["update-ref", "refs/remotes/origin/main", &published], + ); + + let output = run(None, Some(&repo_path), None, Some("HEAD"), None, true); + let stdout = String::from_utf8_lossy(&output.stdout); + + assert!(output.status.success(), "Expected checks to pass"); + assert!( + stdout.contains("Testing 1 commit"), + "Expected only the unpublished commit to be checked, got: {}", + stdout + ); + } + + #[test] + fn test_not_on_remotes_passes_when_nothing_new() { + let temp_dir = TempDir::new().expect("Failed to create temp dir"); + let repo_path = create_test_repo( + temp_dir + .path() + .to_str() + .expect("Failed to convert temp dir path to string"), + ); + + let head = create_commit(&repo_path, "chore: initial"); + + // Every commit is already on a remote. + run_cmd( + &repo_path, + "git", + &["update-ref", "refs/remotes/origin/main", &head], + ); + + let output = run(None, Some(&repo_path), None, Some("HEAD"), None, true); + let stdout = String::from_utf8_lossy(&output.stdout); + + assert!(output.status.success(), "Expected a clean pass"); + assert!( + stdout.contains("No new commits to check"), + "Expected clean-pass message, got: {}", + stdout + ); + } + // ===== Message File Tests ===== #[test]