From d43fab946464264a052f3e62f2af11f327d53502 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Thu, 2 Jul 2026 04:33:41 -0700 Subject: [PATCH 1/2] fix(git-utils): reuse runner for merge-base --- codex-rs/git-utils/src/branch.rs | 119 ++++++++++++++++++++++---- codex-rs/git-utils/src/git_command.rs | 17 ++++ codex-rs/git-utils/src/operations.rs | 53 ++++++++++-- 3 files changed, 165 insertions(+), 24 deletions(-) diff --git a/codex-rs/git-utils/src/branch.rs b/codex-rs/git-utils/src/branch.rs index fd5d1d05a1..13cad88f7b 100644 --- a/codex-rs/git-utils/src/branch.rs +++ b/codex-rs/git-utils/src/branch.rs @@ -2,10 +2,11 @@ use std::ffi::OsString; use std::path::Path; use crate::GitToolingError; +use crate::git_command::GitRunner; use crate::operations::ensure_git_repository; use crate::operations::resolve_head; use crate::operations::resolve_repository_root; -use crate::operations::run_git_for_stdout; +use crate::operations::run_git_for_stdout_with_runner; /// Returns the merge-base commit between `HEAD` and the latest version between local /// and remote of the provided branch, if both exist. @@ -16,25 +17,28 @@ pub fn merge_base_with_head( repo_path: &Path, branch: &str, ) -> Result, GitToolingError> { - ensure_git_repository(repo_path)?; - let repo_root = resolve_repository_root(repo_path)?; - let head = match resolve_head(repo_root.as_path())? { + let git = GitRunner::for_cwd_io(repo_path)?; + ensure_git_repository(&git, repo_path)?; + let repo_root = resolve_repository_root(&git, repo_path)?; + let head = match resolve_head(&git, repo_root.as_path())? { Some(head) => head, None => return Ok(None), }; - let Some(branch_ref) = resolve_branch_ref(repo_root.as_path(), branch)? else { + let Some(branch_ref) = resolve_branch_ref(&git, repo_root.as_path(), branch)? else { return Ok(None); }; - let preferred_ref = - if let Some(upstream) = resolve_upstream_if_remote_ahead(repo_root.as_path(), branch)? { - resolve_branch_ref(repo_root.as_path(), &upstream)?.unwrap_or(branch_ref) - } else { - branch_ref - }; + let preferred_ref = if let Some(upstream) = + resolve_upstream_if_remote_ahead(&git, repo_root.as_path(), branch)? + { + resolve_branch_ref(&git, repo_root.as_path(), &upstream)?.unwrap_or(branch_ref) + } else { + branch_ref + }; - let merge_base = run_git_for_stdout( + let merge_base = run_git_for_stdout_with_runner( + &git, repo_root.as_path(), vec![ OsString::from("merge-base"), @@ -47,8 +51,13 @@ pub fn merge_base_with_head( Ok(Some(merge_base)) } -fn resolve_branch_ref(repo_root: &Path, branch: &str) -> Result, GitToolingError> { - let rev = run_git_for_stdout( +fn resolve_branch_ref( + git: &GitRunner, + repo_root: &Path, + branch: &str, +) -> Result, GitToolingError> { + let rev = run_git_for_stdout_with_runner( + git, repo_root, vec![ OsString::from("rev-parse"), @@ -66,10 +75,12 @@ fn resolve_branch_ref(repo_root: &Path, branch: &str) -> Result, } fn resolve_upstream_if_remote_ahead( + git: &GitRunner, repo_root: &Path, branch: &str, ) -> Result, GitToolingError> { - let upstream = match run_git_for_stdout( + let upstream = match run_git_for_stdout_with_runner( + git, repo_root, vec![ OsString::from("rev-parse"), @@ -90,7 +101,8 @@ fn resolve_upstream_if_remote_ahead( Err(other) => return Err(other), }; - let counts = match run_git_for_stdout( + let counts = match run_git_for_stdout_with_runner( + git, repo_root, vec![ OsString::from("rev-list"), @@ -120,7 +132,13 @@ fn resolve_upstream_if_remote_ahead( mod tests { use super::merge_base_with_head; use crate::GitToolingError; + use crate::git_command::GitRunner; + use crate::git_command::git_runner_construction_count; + use crate::git_command::reset_git_runner_construction_count; + use crate::operations::ensure_git_repository; + use crate::operations::resolve_repository_root; use pretty_assertions::assert_eq; + use std::io; use std::path::Path; use std::process::Command; use tempfile::tempdir; @@ -187,8 +205,14 @@ mod tests { run_git_in(repo, &["checkout", "feature"]); let expected = run_git_stdout(repo, &["merge-base", "HEAD", "main"]); + reset_git_runner_construction_count(); let merge_base = merge_base_with_head(repo, "main")?; assert_eq!(merge_base, Some(expected)); + assert_eq!( + git_runner_construction_count(), + 1, + "one high-level merge-base call must retain one Git runner" + ); Ok(()) } @@ -253,4 +277,67 @@ mod tests { Ok(()) } + + #[test] + fn merge_base_returns_none_when_head_is_unborn() -> Result<(), GitToolingError> { + let temp = tempdir()?; + let repo = temp.path(); + init_test_repo(repo); + + let merge_base = merge_base_with_head(repo, "main")?; + assert_eq!(merge_base, None); + + Ok(()) + } + + #[test] + fn merge_base_reports_non_repository() { + let temp = tempdir().expect("tempdir"); + let path = temp.path(); + + let error = merge_base_with_head(path, "main").expect_err("non-repository must fail"); + let GitToolingError::NotAGitRepository { path: error_path } = error else { + panic!("expected non-repository error, got {error:?}"); + }; + assert_eq!(error_path, path); + } + + #[cfg(unix)] + #[test] + fn retained_runner_refuses_gitdir_retarget_between_subcommands() { + use std::os::unix::fs::symlink; + + let fixture = tempdir().expect("fixture"); + let repo = fixture.path().join("repo"); + let admin = fixture.path().join("external-admin"); + std::fs::create_dir_all(&repo).expect("create repository"); + run_git_in( + fixture.path(), + &[ + "init", + "--separate-git-dir", + admin.to_str().expect("UTF-8 admin path"), + repo.to_str().expect("UTF-8 repository path"), + ], + ); + + reset_git_runner_construction_count(); + let git = GitRunner::for_cwd(&repo).expect("operation-scoped Git runner"); + ensure_git_repository(&git, &repo).expect("first merge-base subcommand"); + + symlink(&admin, repo.join("switch")).expect("Git-dir route switch"); + std::fs::write(repo.join(".git"), "gitdir: switch\n").expect("retarget Git-dir marker"); + + let error = resolve_repository_root(&git, &repo) + .expect_err("retained authority must refuse the second subcommand"); + let GitToolingError::Io(error) = error else { + panic!("expected metadata revalidation error, got {error:?}"); + }; + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied, "{error}"); + assert_eq!( + git_runner_construction_count(), + 1, + "subcommands must not rediscover authority after metadata retargeting" + ); + } } diff --git a/codex-rs/git-utils/src/git_command.rs b/codex-rs/git-utils/src/git_command.rs index e78420b842..924ed32385 100644 --- a/codex-rs/git-utils/src/git_command.rs +++ b/codex-rs/git-utils/src/git_command.rs @@ -61,6 +61,8 @@ impl GitCommand { impl GitRunner { pub(crate) fn for_cwd(cwd: &Path) -> Result { + #[cfg(test)] + GIT_RUNNER_CONSTRUCTION_COUNT.with(|count| count.set(count.get() + 1)); let authority = repository_authority_for_cwd(cwd)?; let search_path = std::env::var_os("PATH").ok_or(GitReadError::NoTrustedGit)?; Self::from_search_path(authority, &search_path) @@ -125,6 +127,21 @@ impl GitRunner { } } +#[cfg(test)] +thread_local! { + static GIT_RUNNER_CONSTRUCTION_COUNT: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +#[cfg(test)] +pub(crate) fn reset_git_runner_construction_count() { + GIT_RUNNER_CONSTRUCTION_COUNT.with(|count| count.set(0)); +} + +#[cfg(test)] +pub(crate) fn git_runner_construction_count() -> usize { + GIT_RUNNER_CONSTRUCTION_COUNT.with(std::cell::Cell::get) +} + pub(crate) fn repository_authority_for_cwd( cwd: &Path, ) -> Result { diff --git a/codex-rs/git-utils/src/operations.rs b/codex-rs/git-utils/src/operations.rs index 27c3e17ec4..24123d1ea4 100644 --- a/codex-rs/git-utils/src/operations.rs +++ b/codex-rs/git-utils/src/operations.rs @@ -7,8 +7,9 @@ use crate::GitToolingError; use crate::git_command::GitRunner; use crate::safe_git::DISABLED_HOOKS_PATH; -pub(crate) fn ensure_git_repository(path: &Path) -> Result<(), GitToolingError> { - match run_git_for_stdout( +pub(crate) fn ensure_git_repository(git: &GitRunner, path: &Path) -> Result<(), GitToolingError> { + match run_git_for_stdout_with_runner( + git, path, vec![ OsString::from("rev-parse"), @@ -29,8 +30,12 @@ pub(crate) fn ensure_git_repository(path: &Path) -> Result<(), GitToolingError> } } -pub(crate) fn resolve_head(path: &Path) -> Result, GitToolingError> { - match run_git_for_stdout( +pub(crate) fn resolve_head( + git: &GitRunner, + path: &Path, +) -> Result, GitToolingError> { + match run_git_for_stdout_with_runner( + git, path, vec![ OsString::from("rev-parse"), @@ -45,8 +50,12 @@ pub(crate) fn resolve_head(path: &Path) -> Result, GitToolingErro } } -pub(crate) fn resolve_repository_root(path: &Path) -> Result { - let root = run_git_for_stdout( +pub(crate) fn resolve_repository_root( + git: &GitRunner, + path: &Path, +) -> Result { + let root = run_git_for_stdout_with_runner( + git, path, vec![ OsString::from("rev-parse"), @@ -70,6 +79,7 @@ where Ok(()) } +#[cfg(test)] pub(crate) fn run_git_for_stdout( dir: &Path, args: I, @@ -79,7 +89,21 @@ where I: IntoIterator, S: AsRef, { - let run = run_git(dir, args, env)?; + let git = GitRunner::for_cwd_io(dir)?; + run_git_for_stdout_with_runner(&git, dir, args, env) +} + +pub(crate) fn run_git_for_stdout_with_runner( + git: &GitRunner, + dir: &Path, + args: I, + env: Option<&[(OsString, OsString)]>, +) -> Result +where + I: IntoIterator, + S: AsRef, +{ + let run = run_git_with_runner(git, dir, args, env)?; String::from_utf8(run.output.stdout) .map(|value| value.trim().to_string()) .map_err(|source| GitToolingError::GitOutputUtf8 { @@ -93,6 +117,20 @@ fn run_git( args: I, env: Option<&[(OsString, OsString)]>, ) -> Result +where + I: IntoIterator, + S: AsRef, +{ + let git = GitRunner::for_cwd_io(dir)?; + run_git_with_runner(&git, dir, args, env) +} + +fn run_git_with_runner( + git: &GitRunner, + dir: &Path, + args: I, + env: Option<&[(OsString, OsString)]>, +) -> Result where I: IntoIterator, S: AsRef, @@ -109,7 +147,6 @@ where args_vec.push(OsString::from(arg.as_ref())); } let command_string = build_command_string(&args_vec); - let git = GitRunner::for_cwd_io(dir)?; let mut command = git.command_for_cwd(dir)?; if let Some(envs) = env { for (key, value) in envs { From 10948913cd50a6790da012da5d02993befe4dd97 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Thu, 2 Jul 2026 04:48:32 -0700 Subject: [PATCH 2/2] test(git-utils): gate Unix-only merge-base imports --- codex-rs/git-utils/src/branch.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/codex-rs/git-utils/src/branch.rs b/codex-rs/git-utils/src/branch.rs index 13cad88f7b..8d5c8f1e03 100644 --- a/codex-rs/git-utils/src/branch.rs +++ b/codex-rs/git-utils/src/branch.rs @@ -132,12 +132,16 @@ fn resolve_upstream_if_remote_ahead( mod tests { use super::merge_base_with_head; use crate::GitToolingError; + #[cfg(unix)] use crate::git_command::GitRunner; use crate::git_command::git_runner_construction_count; use crate::git_command::reset_git_runner_construction_count; + #[cfg(unix)] use crate::operations::ensure_git_repository; + #[cfg(unix)] use crate::operations::resolve_repository_root; use pretty_assertions::assert_eq; + #[cfg(unix)] use std::io; use std::path::Path; use std::process::Command;