From f6adc9fbcc19569b047fb425d509081508f882e6 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Fri, 3 Jul 2026 20:32:11 -0700 Subject: [PATCH] fix(git): preserve patch parsing compatibility --- codex-rs/git-utils/src/apply.rs | 76 +++++++++++++++++++-- codex-rs/git-utils/src/patch_paths.rs | 23 +++++-- codex-rs/git-utils/src/patch_paths_tests.rs | 4 +- 3 files changed, 90 insertions(+), 13 deletions(-) diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index 5ba64abde2..8526a62682 100644 --- a/codex-rs/git-utils/src/apply.rs +++ b/codex-rs/git-utils/src/apply.rs @@ -48,12 +48,12 @@ pub struct ApplyGitResult { pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { let git = GitRunner::for_cwd_io(&req.cwd)?; let mut cfg_parts = configured_git_config_parts(); - ensure_no_worktree_primary_config_sources(&git, &req.cwd)?; let requested_cwd = std::fs::canonicalize(&req.cwd)?; - let git_root = resolve_git_root(&git, &req.cwd, &cfg_parts)?; - if git_root != requested_cwd { - ensure_no_worktree_primary_config_sources(&git, &git_root)?; - } + let expected_root = crate::get_git_repo_root(&requested_cwd) + .ok_or_else(|| io::Error::other("not a Git repository")) + .and_then(std::fs::canonicalize)?; + ensure_no_worktree_primary_config_sources(&git, &expected_root)?; + let git_root = resolve_git_root(&git, &expected_root, &cfg_parts)?; // Write unified diff into a temporary file let (tmpdir, patch_path) = write_temp_patch(&req.diff)?; @@ -359,6 +359,72 @@ mod tests { assert!(root.join("hello.txt").exists()); } + #[test] + fn apply_resolves_relative_primary_config_from_repository_root() { + let _g = env_lock().lock().unwrap(); + if std::env::var_os("CODEX_GIT_UTILS_APPLY_ENV_CHILD").is_none() { + run_isolated_test( + "apply::tests::apply_resolves_relative_primary_config_from_repository_root", + &[("GIT_CONFIG_GLOBAL", OsStr::new("../external/config"))], + ); + return; + } + + let fixture = tempfile::tempdir().expect("fixture"); + let root = fixture.path().join("repo"); + let nested_cwd = root.join("nested"); + let external_config = fixture.path().join("external/config"); + let mismatched_nested_config = root.join("external/config"); + std::fs::create_dir_all(&nested_cwd).expect("nested cwd"); + std::fs::create_dir_all(external_config.parent().expect("config parent")) + .expect("external config directory"); + std::fs::create_dir_all( + mismatched_nested_config + .parent() + .expect("mismatched config parent"), + ) + .expect("mismatched config directory"); + std::fs::write(&external_config, "[codex]\n\tprobe = loaded\n").expect("external config"); + std::fs::write(&mismatched_nested_config, "[invalid\n") + .expect("mismatched nested-cwd config"); + let (init_code, _, init_err) = run(&root, &["git", "init"]); + assert_eq!(init_code, 0, "init repository: {init_err}"); + + let result = apply_git_patch(&ApplyGitRequest { + cwd: nested_cwd, + diff: "diff --git a/hello.txt b/hello.txt\nnew file mode 100644\n--- /dev/null\n+++ b/hello.txt\n@@ -0,0 +1 @@\n+hello\n".to_string(), + revert: false, + preflight: false, + }) + .expect("apply with root-relative primary config"); + assert_eq!(result.exit_code, 0, "apply result: {result:?}"); + assert_eq!(read_file_normalized(&root.join("hello.txt")), "hello\n"); + } + + #[test] + fn numstat_path_discovery_does_not_preempt_apply_whitespace_result() { + let _g = env_lock().lock().unwrap(); + let repo = init_repo(); + let root = repo.path(); + let (config_code, _, config_err) = + run(root, &["git", "config", "apply.whitespace", "error"]); + assert_eq!(config_code, 0, "configure whitespace policy: {config_err}"); + + let result = apply_git_patch(&ApplyGitRequest { + cwd: root.to_path_buf(), + diff: "diff --git a/trailing.txt b/trailing.txt\nnew file mode 100644\n--- /dev/null\n+++ b/trailing.txt\n@@ -0,0 +1 @@\n+trailing \n".to_string(), + revert: false, + preflight: false, + }) + .expect("path discovery must leave apply failure in the structured result"); + assert_ne!(result.exit_code, 0); + assert!( + result.stderr.contains("trailing whitespace"), + "apply result: {result:?}" + ); + assert!(!root.join("trailing.txt").exists()); + } + #[test] fn apply_uses_cwd_repo_despite_inherited_repository_selectors() { let _g = env_lock().lock().unwrap(); diff --git a/codex-rs/git-utils/src/patch_paths.rs b/codex-rs/git-utils/src/patch_paths.rs index 378a245859..782c24e08d 100644 --- a/codex-rs/git-utils/src/patch_paths.rs +++ b/codex-rs/git-utils/src/patch_paths.rs @@ -11,6 +11,10 @@ use crate::git_command::GitRunner; use crate::git_config::path_is_within; use crate::git_config_sources::ensure_no_worktree_primary_config_sources; +// Path discovery must not turn a repository's whitespace policy into an +// API-level parser failure before the real apply can report its status. +const APPLY_NUMSTAT_ARGS: [&str; 3] = ["--numstat", "--whitespace=nowarn", "-z"]; + /// Extract paths with Git from a cwd whose config sources have already been /// authorized for `git_config_args`. pub(crate) fn extract_effective_paths_from_patch( @@ -63,11 +67,18 @@ fn extract_paths_from_patch_from_cwd(diff_text: &str, cwd: &Path) -> Vec }; let paths = (|| -> io::Result> { let git = GitRunner::for_cwd_io(cwd)?; - let git_root = crate::get_git_repo_root(cwd) - .ok_or_else(|| io::Error::other("not a Git repository"))?; - let git_root = std::fs::canonicalize(git_root)?; - ensure_no_worktree_primary_config_sources(&git, &git_root)?; - extract_effective_paths_from_patch(&git, &git_root, &patch_path, /*revert*/ false, &[]) + let authorized_cwd = crate::get_git_repo_root(cwd) + .map(std::fs::canonicalize) + .transpose()? + .unwrap_or(std::fs::canonicalize(cwd)?); + ensure_no_worktree_primary_config_sources(&git, &authorized_cwd)?; + extract_effective_paths_from_patch( + &git, + &authorized_cwd, + &patch_path, + /*revert*/ false, + &[], + ) })() .unwrap_or_default(); drop(tmpdir); @@ -83,7 +94,7 @@ fn git_apply_numstat_paths( ) -> io::Result> { let mut cmd = git.command_for_cwd(authorized_cwd)?; cmd.args(git_config_args); - cmd.args(["apply", "--numstat", "-z"]); + cmd.arg("apply").args(APPLY_NUMSTAT_ARGS); if revert { cmd.arg("-R"); } diff --git a/codex-rs/git-utils/src/patch_paths_tests.rs b/codex-rs/git-utils/src/patch_paths_tests.rs index 4d6643a97a..bf2396b22a 100644 --- a/codex-rs/git-utils/src/patch_paths_tests.rs +++ b/codex-rs/git-utils/src/patch_paths_tests.rs @@ -75,8 +75,8 @@ fn effective_paths(diff: &str, revert: bool) -> io::Result> { } fn best_effort_paths(diff: &str) -> Vec { - let repo = init_repo(); - extract_paths_from_patch_from_cwd(diff, repo.path()) + let cwd = tempfile::tempdir().expect("non-repository cwd"); + extract_paths_from_patch_from_cwd(diff, cwd.path()) } #[test]