diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index e5c7aeadeb..870d3684fb 100644 --- a/codex-rs/git-utils/src/apply.rs +++ b/codex-rs/git-utils/src/apply.rs @@ -90,13 +90,14 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { }); } - // Avoid three-way machinery entirely when the patch applies cleanly. - // A selected merge driver is relevant only to the three-way fallback. - let mut plain_check_args = vec![ - "apply".to_string(), - "--check".to_string(), - "--index".to_string(), - ]; + // Avoid three-way machinery entirely when the patch applies cleanly. A + // reverse check must inspect the working tree before staging: requiring + // the old index to match would misclassify a clean undo as a three-way + // fallback. Forward direct application still requires index agreement. + let mut plain_check_args = vec!["apply".to_string(), "--check".to_string()]; + if !req.revert { + plain_check_args.push("--index".to_string()); + } if req.revert { plain_check_args.push("-R".to_string()); } @@ -104,15 +105,21 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { let (plain_check_code, _, _) = run_git(&git, &git_root, &cfg_parts, &plain_check_args)?; let mut args = vec!["apply".to_string()]; - if plain_check_code != 0 { - ensure_no_selected_merge_drivers(&git, &git_root, &patch_paths, &cfg_parts)?; - if req.revert { - // Stage WT paths first to avoid index mismatch on three-way revert. - stage_effective_paths(&git, &git_root, &patch_paths, &cfg_parts)?; + let merge_guard = if plain_check_code != 0 { + let guard = ensure_no_selected_merge_drivers(&git, &git_root, &patch_paths, &cfg_parts)?; + if let Some(guard) = &guard { + cfg_parts.extend_from_slice(guard.git_config_args()); } args.push("--3way".to_string()); + guard } else { args.push("--index".to_string()); + None + }; + if req.revert { + // Stage only after the worktree-only applicability decision. The + // guarded config is shared with the final Git command. + stage_effective_paths(&git, &git_root, &patch_paths, &cfg_parts)?; } if req.revert { args.push("-R".to_string()); @@ -121,6 +128,7 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { let cmd_for_log = render_command_for_log(&git_root, &cfg_parts, &args); let (code, stdout, stderr) = run_git(&git, &git_root, &cfg_parts, &args)?; + drop(merge_guard); let (mut applied_paths, mut skipped_paths, mut conflicted_paths) = parse_git_apply_output(&stdout, &stderr); diff --git a/codex-rs/git-utils/src/merge_driver.rs b/codex-rs/git-utils/src/merge_driver.rs index 73ded7f3e3..265fd6c9a8 100644 --- a/codex-rs/git-utils/src/merge_driver.rs +++ b/codex-rs/git-utils/src/merge_driver.rs @@ -8,6 +8,7 @@ use std::process::Stdio; use crate::git_command::GitRunner; use crate::git_config::GitConfigEntry; +use crate::safe_git::GitConfigOverrideFile; #[cfg(test)] use crate::safe_git::isolate_git_command_environment; use crate::safe_git::read_effective_config_with_fallback; @@ -19,7 +20,7 @@ pub(crate) fn ensure_no_selected_merge_drivers( cwd: &Path, paths: &[String], git_config_args: &[String], -) -> io::Result<()> { +) -> io::Result> { let entries = read_merge_config(git, cwd, git_config_args)?; let attributes = read_merge_attributes(git, cwd, paths, git_config_args)?; if let Some((driver, path)) = untrusted_driver_selection(&entries, &attributes)? { @@ -30,7 +31,27 @@ pub(crate) fn ensure_no_selected_merge_drivers( ), )); } - Ok(()) + + let driver_keys = entries + .values() + .filter(|entry| entry.key != "merge.default" && !entry.value.is_empty()) + .map(|entry| entry.key.clone()) + .collect::>(); + if driver_keys.is_empty() { + return Ok(None); + } + + let guard = GitConfigOverrideFile::new("merge-driver-neutralization.gitconfig")?; + for key in driver_keys { + guard.add_value( + git, + cwd, + &key, + "", + &format!("Git merge-driver neutralization for {key:?}"), + )?; + } + Ok(Some(guard)) } fn read_merge_config( @@ -176,3 +197,7 @@ fn invalid_config_entry(message: &str) -> io::Error { #[cfg(test)] #[path = "merge_driver_tests.rs"] mod tests; + +#[cfg(test)] +#[path = "merge_driver_race_tests.rs"] +mod race_tests; diff --git a/codex-rs/git-utils/src/merge_driver_race_tests.rs b/codex-rs/git-utils/src/merge_driver_race_tests.rs new file mode 100644 index 0000000000..5688c03fa0 --- /dev/null +++ b/codex-rs/git-utils/src/merge_driver_race_tests.rs @@ -0,0 +1,307 @@ +use super::*; +use crate::apply::ApplyGitRequest; +use crate::apply::apply_git_patch; +use pretty_assertions::assert_eq; +use std::fs::OpenOptions; +use std::path::Path; +use std::path::PathBuf; +use std::thread; +use std::time::Duration; +use std::time::Instant; + +const OLD_DRIVER_COMMAND: &str = "git config codex.oldmergeran true && false"; +const NEW_DRIVER_COMMAND: &str = "git config codex.newmergeran true && false"; + +fn run(cwd: &Path, args: &[&str]) -> (i32, String, String) { + let mut command = std::process::Command::new("git"); + isolate_git_command_environment(&mut command); + let output = command + .args(args) + .current_dir(cwd) + .output() + .expect("run Git"); + ( + output.status.code().unwrap_or(-1), + String::from_utf8_lossy(&output.stdout).into_owned(), + String::from_utf8_lossy(&output.stderr).into_owned(), + ) +} + +fn run_success(cwd: &Path, args: &[&str]) -> String { + let (code, stdout, stderr) = run(cwd, args); + assert_eq!(code, 0, "git {args:?}: {stderr}"); + stdout +} + +fn init_repo() -> tempfile::TempDir { + let repo = tempfile::tempdir().expect("repo tempdir"); + let root = repo.path(); + run_success(root, &["init"]); + run_success(root, &["config", "user.email", "codex@example.com"]); + run_success(root, &["config", "user.name", "Codex"]); + run_success(root, &["config", "core.autocrlf", "false"]); + repo +} + +fn configure_driver(root: &Path, driver: &str, command: &str) { + run_success( + root, + &["config", &format!("merge.{driver}.driver"), command], + ); +} + +fn configured_marker_exists(root: &Path, marker: &str) -> bool { + run(root, &["config", "--get", marker]).0 == 0 +} + +fn build_conflicting_patch(root: &Path, attributes: &str) -> String { + std::fs::write(root.join(".gitattributes"), attributes).expect("write base attributes"); + std::fs::write(root.join("target.txt"), "top\nbase\nbottom\n").expect("write base"); + run_success(root, &["add", ".gitattributes", "target.txt"]); + run_success(root, &["commit", "-m", "base"]); + let base = run_success(root, &["rev-parse", "HEAD"]); + let base = base.trim(); + + std::fs::write(root.join("target.txt"), "top\ntheirs\nbottom\n").expect("write theirs"); + run_success(root, &["add", "target.txt"]); + run_success(root, &["commit", "-m", "theirs"]); + let patch = run_success( + root, + &["diff", "--full-index", base, "HEAD", "--", "target.txt"], + ); + + run_success(root, &["checkout", "-b", "ours", base]); + std::fs::write(root.join("target.txt"), "top\nours\nbottom\n").expect("write ours"); + run_success(root, &["add", "target.txt"]); + run_success(root, &["commit", "-m", "ours"]); + patch +} + +fn request(root: &Path, diff: String, revert: bool) -> ApplyGitRequest { + ApplyGitRequest { + cwd: root.to_path_buf(), + diff, + revert, + preflight: false, + } +} + +fn index_tree(root: &Path) -> String { + run_success(root, &["write-tree"]) +} + +fn run_isolated_test(test_name: &str) { + let environment = tempfile::tempdir().expect("isolated Git environment"); + let global_config = environment.path().join("global.gitconfig"); + let system_config = environment.path().join("system.gitconfig"); + let trace = environment.path().join("trace.jsonl"); + std::fs::write(&global_config, "").expect("empty global config"); + std::fs::write(&system_config, "").expect("empty system config"); + std::fs::write(&trace, "").expect("empty trace"); + + let mut command = std::process::Command::new(std::env::current_exe().expect("test binary")); + isolate_git_command_environment(&mut command); + let output = command + .arg(test_name) + .arg("--exact") + .arg("--nocapture") + .env("CODEX_GIT_UTILS_MERGE_RACE_CHILD", "1") + .env("GIT_CONFIG_GLOBAL", &global_config) + .env("GIT_CONFIG_SYSTEM", &system_config) + .env("GIT_TRACE2_EVENT", &trace) + .env("RUST_TEST_THREADS", "1") + .output() + .expect("run isolated test process"); + assert!( + output.status.success(), + "isolated test {test_name} failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); +} + +fn trace_path() -> PathBuf { + PathBuf::from(std::env::var_os("GIT_TRACE2_EVENT").expect("trace path")) +} + +fn wait_for_merge_attribute_probe(trace: &Path) -> bool { + let deadline = Instant::now() + Duration::from_secs(10); + while Instant::now() < deadline { + let contents = std::fs::read_to_string(trace).unwrap_or_default(); + if contents + .find("check-attr") + .is_some_and(|offset| contents[offset..].contains("\"event\":\"exit\"")) + { + return true; + } + thread::yield_now(); + } + false +} + +fn append_driver_config(root: &Path, driver: &str, command: &str) { + let mut config = OpenOptions::new() + .append(true) + .open(root.join(".git/config")) + .expect("open local config"); + writeln!(config, "\n[merge \"{driver}\"]\n\tdriver = {command}") + .expect("append replacement merge driver"); +} + +#[test] +fn clean_reverse_checks_worktree_before_staging_and_skips_merge_driver() { + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join(".gitattributes"), "file.txt merge=demo\n").expect("write attributes"); + std::fs::write(root.join("file.txt"), "old\n").expect("write old file"); + run_success(root, &["add", ".gitattributes", "file.txt"]); + run_success(root, &["commit", "-m", "base"]); + configure_driver(root, "demo", OLD_DRIVER_COMMAND); + + std::fs::write(root.join("file.txt"), "new\n").expect("write unstaged forward state"); + let patch = "diff --git a/file.txt b/file.txt\n--- a/file.txt\n+++ b/file.txt\n@@ -1 +1 @@\n-old\n+new\n"; + let result = apply_git_patch(&request(root, patch.to_string(), /*revert*/ true)) + .expect("clean reverse apply"); + + assert_eq!(result.exit_code, 0, "{}", result.stderr); + assert!(result.cmd_for_log.contains("--index")); + assert!(!result.cmd_for_log.contains("--3way")); + assert_eq!( + std::fs::read_to_string(root.join("file.txt")).expect("read restored file"), + "old\n" + ); + assert!(!configured_marker_exists(root, "codex.oldmergeran")); + assert!(run_success(root, &["status", "--porcelain"]).is_empty()); +} + +#[test] +fn selected_empty_and_equals_named_drivers_reject_without_marker_or_index_mutation() { + for (driver, attribute) in [ + ("", "target.txt merge=\n"), + ("x=y", "target.txt merge=x=y\n"), + ] { + let repo = init_repo(); + let root = repo.path(); + let patch = build_conflicting_patch(root, attribute); + configure_driver(root, driver, OLD_DRIVER_COMMAND); + let before_tree = index_tree(root); + let before_contents = + std::fs::read_to_string(root.join("target.txt")).expect("read before contents"); + + let error = apply_git_patch(&request(root, patch, /*revert*/ false)) + .expect_err("reject selected merge driver"); + + assert_eq!(error.kind(), io::ErrorKind::Unsupported, "{driver:?}"); + assert!(!configured_marker_exists(root, "codex.oldmergeran")); + assert_eq!(index_tree(root), before_tree, "{driver:?}"); + assert_eq!( + std::fs::read_to_string(root.join("target.txt")).expect("read after contents"), + before_contents, + "{driver:?}" + ); + assert!(run_success(root, &["status", "--porcelain"]).is_empty()); + } +} + +#[test] +fn post_probe_attribute_change_cannot_run_empty_named_driver() { + if std::env::var_os("CODEX_GIT_UTILS_MERGE_RACE_CHILD").is_none() { + run_isolated_test( + "merge_driver::race_tests::post_probe_attribute_change_cannot_run_empty_named_driver", + ); + return; + } + + let repo = init_repo(); + let root = repo.path(); + let patch = build_conflicting_patch(root, "# initially safe\n"); + configure_driver(root, "", OLD_DRIVER_COMMAND); + let trace = trace_path(); + std::fs::write(&trace, "").expect("clear fixture trace"); + let attributes = root.join(".gitattributes"); + let watcher = thread::spawn(move || { + let observed = wait_for_merge_attribute_probe(&trace); + if observed { + std::fs::write(attributes, "target.txt merge=\n") + .expect("select empty-name merge driver"); + } + observed + }); + + let result = apply_git_patch(&request(root, patch, /*revert*/ false)) + .expect("neutralized three-way apply"); + assert!(watcher.join().expect("attribute watcher")); + + assert_ne!(result.exit_code, 0); + assert!(!configured_marker_exists(root, "codex.oldmergeran")); +} + +#[test] +fn post_probe_attribute_and_same_driver_command_change_stays_neutralized() { + if std::env::var_os("CODEX_GIT_UTILS_MERGE_RACE_CHILD").is_none() { + run_isolated_test( + "merge_driver::race_tests::post_probe_attribute_and_same_driver_command_change_stays_neutralized", + ); + return; + } + + let repo = init_repo(); + let root = repo.path(); + let patch = build_conflicting_patch(root, "# initially safe\n"); + configure_driver(root, "x=y", OLD_DRIVER_COMMAND); + let trace = trace_path(); + std::fs::write(&trace, "").expect("clear fixture trace"); + let watcher_root = root.to_path_buf(); + let watcher = thread::spawn(move || { + let observed = wait_for_merge_attribute_probe(&trace); + if observed { + append_driver_config(&watcher_root, "x=y", NEW_DRIVER_COMMAND); + std::fs::write( + watcher_root.join(".gitattributes"), + "target.txt merge=x=y\n", + ) + .expect("select replacement merge driver"); + } + observed + }); + + let result = apply_git_patch(&request(root, patch, /*revert*/ false)) + .expect("neutralized three-way apply"); + assert!(watcher.join().expect("config watcher")); + + assert_ne!(result.exit_code, 0); + assert!(!configured_marker_exists(root, "codex.oldmergeran")); + assert!(!configured_marker_exists(root, "codex.newmergeran")); +} + +#[test] +fn same_patch_attribute_activation_does_not_run_merge_driver() { + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join(".gitattributes"), "# initially safe\n") + .expect("write base attributes"); + std::fs::write(root.join("target.txt"), "top\nbase\nbottom\n").expect("write base"); + run_success(root, &["add", ".gitattributes", "target.txt"]); + run_success(root, &["commit", "-m", "base"]); + let base = run_success(root, &["rev-parse", "HEAD"]); + let base = base.trim(); + + std::fs::write(root.join(".gitattributes"), "target.txt merge=x=y\n") + .expect("write patch attributes"); + std::fs::write(root.join("target.txt"), "top\ntheirs\nbottom\n").expect("write theirs"); + run_success(root, &["add", ".gitattributes", "target.txt"]); + run_success(root, &["commit", "-m", "theirs"]); + let patch = run_success(root, &["diff", "--full-index", base, "HEAD"]); + + run_success(root, &["checkout", "-b", "ours", base]); + std::fs::write(root.join("target.txt"), "top\nours\nbottom\n").expect("write ours"); + run_success(root, &["add", "target.txt"]); + run_success(root, &["commit", "-m", "ours"]); + configure_driver(root, "x=y", OLD_DRIVER_COMMAND); + + let result = apply_git_patch(&request(root, patch, /*revert*/ false)) + .expect("same-patch attribute activation remains neutralized"); + + assert_ne!(result.exit_code, 0); + assert!(!configured_marker_exists(root, "codex.oldmergeran")); +} diff --git a/codex-rs/git-utils/src/safe_git.rs b/codex-rs/git-utils/src/safe_git.rs index f36e78abc3..a9390c6ab6 100644 --- a/codex-rs/git-utils/src/safe_git.rs +++ b/codex-rs/git-utils/src/safe_git.rs @@ -4,6 +4,7 @@ use std::io; use std::io::Seek; use std::io::Write; use std::path::Path; +use std::path::PathBuf; use std::process::Command; use std::process::Stdio; @@ -15,6 +16,63 @@ use crate::git_config::parse_effective_config_with_origins; pub(crate) const DISABLED_HOOKS_PATH: &str = if cfg!(windows) { "NUL" } else { "/dev/null" }; pub(crate) const EXECUTABLE_FILTER_CONFIG_PATTERN: &str = r"^filter\..*\.(clean|smudge|process)$"; +#[derive(Debug)] +pub(crate) struct GitConfigOverrideFile { + git_config_args: [String; 2], + config_path: PathBuf, + _config_dir: tempfile::TempDir, +} + +impl GitConfigOverrideFile { + pub(crate) fn new(file_name: &str) -> io::Result { + let config_dir = tempfile::tempdir()?; + let config_path = config_dir.path().join(file_name); + std::fs::write(&config_path, [])?; + let config_path_arg = config_path.to_str().ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidData, + "non-UTF-8 Git config override path", + ) + })?; + Ok(Self { + git_config_args: ["-c".to_string(), format!("include.path={config_path_arg}")], + config_path, + _config_dir: config_dir, + }) + } + + pub(crate) fn git_config_args(&self) -> &[String] { + &self.git_config_args + } + + pub(crate) fn add_value( + &self, + git: &GitRunner, + cwd: &Path, + key: &str, + value: &str, + description: &str, + ) -> io::Result<()> { + let mut command = git.command(); + command + .args(["config", "--file"]) + .arg(&self.config_path) + .arg("--add") + .arg(key) + .arg(value) + .current_dir(cwd); + let output = git.output(command)?; + if !output.status.success() { + return Err(io::Error::other(format!( + "failed to write {description} (status {}): {}", + output.status, + String::from_utf8_lossy(&output.stderr).trim() + ))); + } + Ok(()) + } +} + #[derive(Debug, Clone, PartialEq, Eq)] enum FilterAttributeValue { Driver(String), @@ -22,18 +80,15 @@ enum FilterAttributeValue { } pub(crate) struct GitFilterNeutralization { - git_config_args: Vec, - _config_dir: Option, - filter_config: BTreeMap, + config_override: Option, } impl GitFilterNeutralization { pub(crate) fn git_config_args(&self) -> &[String] { - &self.git_config_args - } - - pub(crate) fn filter_value(&self, driver: &str, name: &str) -> Option<&str> { - effective_filter_value(&self.filter_config, driver, name) + self.config_override + .as_ref() + .map(GitConfigOverrideFile::git_config_args) + .unwrap_or_default() } } @@ -109,9 +164,7 @@ fn ensure_no_selected_executable_git_filters_for( }; if target_drivers.is_empty() { return Ok(GitFilterNeutralization { - git_config_args: Vec::new(), - _config_dir: None, - filter_config: entries, + config_override: None, }); } let paths = paths @@ -138,9 +191,7 @@ fn ensure_no_selected_executable_git_filters_for( } if neutralized_drivers.is_empty() { return Ok(GitFilterNeutralization { - git_config_args: Vec::new(), - _config_dir: None, - filter_config: entries, + config_override: None, }); } executable_filter_guard(git, cwd, entries, &neutralized_drivers) @@ -152,62 +203,33 @@ fn executable_filter_guard( filter_config: BTreeMap, executable_drivers: &BTreeSet, ) -> io::Result { - let config_dir = tempfile::tempdir()?; - let config_path = config_dir.path().join("filter-neutralization.gitconfig"); - std::fs::write(&config_path, [])?; - let guard = GitFilterNeutralization { - git_config_args: vec![ - "-c".to_string(), - format!( - "include.path={}", - config_path - .to_str() - .ok_or_else(|| invalid_filter_output("non-UTF-8 filter guard path"))? - ), - ], - _config_dir: Some(config_dir), - filter_config, - }; + let config_override = GitConfigOverrideFile::new("filter-neutralization.gitconfig")?; for driver in executable_drivers { debug_assert!(["clean", "smudge", "process"].into_iter().any(|name| { - guard - .filter_value(driver, name) + effective_filter_value(&filter_config, driver, name) .is_some_and(|value| !value.is_empty()) })); + let description = format!("Git filter neutralization for {driver:?}"); for command in ["clean", "smudge", "process"] { - guard.write_config_value(git, cwd, &config_path, driver, command, "")?; + config_override.add_value( + git, + cwd, + &format!("filter.{driver}.{command}"), + "", + &description, + )?; } - guard.write_config_value(git, cwd, &config_path, driver, "required", "false")?; - } - Ok(guard) -} - -impl GitFilterNeutralization { - fn write_config_value( - &self, - git: &GitRunner, - cwd: &Path, - config_path: &Path, - driver: &str, - name: &str, - value: &str, - ) -> io::Result<()> { - let mut command = git.command(); - command - .args(["config", "--file"]) - .arg(config_path) - .args(["--add", &format!("filter.{driver}.{name}"), value]) - .current_dir(cwd); - let output = git.output(command)?; - if !output.status.success() { - return Err(io::Error::other(format!( - "failed to write Git filter neutralization for {driver:?} (status {}): {}", - output.status, - String::from_utf8_lossy(&output.stderr).trim() - ))); - } - Ok(()) + config_override.add_value( + git, + cwd, + &format!("filter.{driver}.required"), + "false", + &description, + )?; } + Ok(GitFilterNeutralization { + config_override: Some(config_override), + }) } #[derive(Clone, Copy)]