From 5b1fe8c0f2077b7933e75f79204e50fde885c672 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Wed, 1 Jul 2026 15:17:42 -0700 Subject: [PATCH] git-utils: fix selected filter edge cases --- codex-rs/git-utils/src/apply.rs | 6 +- codex-rs/git-utils/src/apply_filter_tests.rs | 227 +++++++++++++++++++ codex-rs/git-utils/src/patch_paths.rs | 6 +- codex-rs/git-utils/src/safe_git.rs | 172 ++++++++++++-- codex-rs/git-utils/src/safe_git_tests.rs | 18 +- 5 files changed, 407 insertions(+), 22 deletions(-) create mode 100644 codex-rs/git-utils/src/apply_filter_tests.rs diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index 4dd850b435..effc46d217 100644 --- a/codex-rs/git-utils/src/apply.rs +++ b/codex-rs/git-utils/src/apply.rs @@ -59,7 +59,7 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { if req.revert && !req.preflight { // Stage WT paths first to avoid index mismatch on revert. - stage_effective_paths(&git, &git_root, &patch_paths)?; + stage_effective_paths(&git, &git_root, &patch_paths, &cfg_parts)?; } // Build git args @@ -255,6 +255,10 @@ fn render_command_for_log(cwd: &Path, git_cfg: &[String], args: &[String]) -> St #[path = "apply_transport_tests.rs"] mod transport_tests; +#[cfg(test)] +#[path = "apply_filter_tests.rs"] +mod filter_tests; + #[cfg(test)] mod tests { use super::*; diff --git a/codex-rs/git-utils/src/apply_filter_tests.rs b/codex-rs/git-utils/src/apply_filter_tests.rs new file mode 100644 index 0000000000..f41f58e3eb --- /dev/null +++ b/codex-rs/git-utils/src/apply_filter_tests.rs @@ -0,0 +1,227 @@ +use super::*; +use pretty_assertions::assert_eq; +use std::ffi::OsStr; +use std::path::Path; + +const PATCH: &str = + "diff --git a/file.txt b/file.txt\n--- a/file.txt\n+++ b/file.txt\n@@ -1 +1 @@\n-orig\n+next\n"; +const FILTER_COMMAND: &str = "git config codex.filterran true && git hash-object --stdin"; + +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]) { + let (code, _, stderr) = run(cwd, args); + assert_eq!(code, 0, "git {args:?}: {stderr}"); +} + +fn init_repo() -> tempfile::TempDir { + let repo = tempfile::tempdir().expect("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"]); + std::fs::write(root.join("file.txt"), "orig\n").expect("write file"); + run_success(root, &["add", "file.txt"]); + run_success(root, &["commit", "-m", "seed"]); + repo +} + +fn configure_filter(root: &Path, driver: &str) { + run_success( + root, + &["config", &format!("filter.{driver}.clean"), FILTER_COMMAND], + ); +} + +fn configured_filter_ran(root: &Path) -> bool { + run(root, &["config", "--get", "codex.filterran"]).0 == 0 +} + +fn request(root: &Path, revert: bool, preflight: bool) -> ApplyGitRequest { + ApplyGitRequest { + cwd: root.to_path_buf(), + diff: PATCH.to_string(), + revert, + preflight, + } +} + +fn run_isolated_test(test_name: &str, env: &[(&str, &OsStr)]) { + let mut command = std::process::Command::new(std::env::current_exe().expect("test binary")); + isolate_git_command_environment(&mut command); + command + .arg(test_name) + .arg("--exact") + .arg("--nocapture") + .env("CODEX_GIT_UTILS_APPLY_FILTER_ENV_CHILD", "1") + .env("RUST_TEST_THREADS", "1"); + for (name, value) in env { + command.env(name, value); + } + let output = command.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) + ); +} + +#[test] +fn reverse_staging_uses_command_scoped_filter_override() { + if std::env::var_os("CODEX_GIT_UTILS_APPLY_FILTER_ENV_CHILD").is_none() { + run_isolated_test( + "apply::filter_tests::reverse_staging_uses_command_scoped_filter_override", + &[("CODEX_APPLY_GIT_CFG", OsStr::new("filter.demo.clean="))], + ); + return; + } + + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join(".gitattributes"), "file.txt filter=demo\n") + .expect("write attributes"); + run_success(root, &["add", ".gitattributes"]); + run_success(root, &["commit", "-m", "attributes"]); + configure_filter(root, "demo"); + + let forward = apply_git_patch(&request( + root, /*revert*/ false, /*preflight*/ false, + )) + .expect("apply with disabled command-scoped filter"); + assert_eq!(forward.exit_code, 0); + let reverse = apply_git_patch(&request( + root, /*revert*/ true, /*preflight*/ false, + )) + .expect("reverse with disabled command-scoped filter"); + assert_eq!(reverse.exit_code, 0); + assert!(!configured_filter_ran(root)); + assert_eq!( + std::fs::read_to_string(root.join("file.txt")).expect("read file"), + "orig\n" + ); +} + +#[test] +fn apply_treats_empty_name_filter_as_selected_only() { + for command in ["clean", "smudge", "process"] { + let repo = init_repo(); + let root = repo.path(); + run_success( + root, + &["config", &format!("filter..{command}"), FILTER_COMMAND], + ); + + std::fs::write(root.join(".gitattributes"), "file.txt filter=other\n") + .expect("write unrelated attributes"); + let unused = apply_git_patch(&request( + root, /*revert*/ false, /*preflight*/ true, + )) + .expect("allow unused empty-name filter"); + assert_eq!(unused.exit_code, 0, "{command}"); + assert!(!configured_filter_ran(root), "{command}"); + + std::fs::write(root.join(".gitattributes"), "file.txt filter=\n") + .expect("select empty-name filter"); + let selected = apply_git_patch(&request( + root, /*revert*/ false, /*preflight*/ true, + )) + .expect_err("reject selected empty-name filter"); + assert_eq!(selected.kind(), io::ErrorKind::Unsupported, "{command}"); + assert!(!configured_filter_ran(root), "{command}"); + } +} + +#[test] +fn apply_distinguishes_filter_sentinels_from_literal_driver_names() { + for (driver, sentinel_attribute) in + [("set", "filter"), ("unset", "-filter"), ("unspecified", "")] + { + let repo = init_repo(); + let root = repo.path(); + configure_filter(root, driver); + run_success( + root, + &["config", &format!("filter.{driver}.smudge"), FILTER_COMMAND], + ); + run_success( + root, + &[ + "config", + &format!("filter.{driver}.process"), + FILTER_COMMAND, + ], + ); + + let attributes = if sentinel_attribute.is_empty() { + String::new() + } else { + format!("file.txt {sentinel_attribute}\n") + }; + std::fs::write(root.join(".gitattributes"), attributes).expect("write sentinel attribute"); + let sentinel = apply_git_patch(&request( + root, /*revert*/ false, /*preflight*/ true, + )) + .expect("allow attribute sentinel"); + assert_eq!(sentinel.exit_code, 0, "{driver}"); + assert!(!configured_filter_ran(root), "{driver}"); + + std::fs::write( + root.join(".gitattributes"), + format!("file.txt filter={driver}\n"), + ) + .expect("write literal driver attribute"); + let selected = apply_git_patch(&request( + root, /*revert*/ false, /*preflight*/ true, + )) + .expect_err("reject literal sentinel-named driver"); + assert_eq!(selected.kind(), io::ErrorKind::Unsupported, "{driver}"); + assert!(!configured_filter_ran(root), "{driver}"); + } +} + +#[test] +fn apply_allows_effectively_empty_empty_name_filter() { + if std::env::var_os("CODEX_GIT_UTILS_APPLY_FILTER_ENV_CHILD").is_none() { + run_isolated_test( + "apply::filter_tests::apply_allows_effectively_empty_empty_name_filter", + &[( + "CODEX_APPLY_GIT_CFG", + OsStr::new("filter..clean=,filter..smudge=,filter..process="), + )], + ); + return; + } + + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join(".gitattributes"), "file.txt filter=\n") + .expect("select empty-name filter"); + for command in ["clean", "smudge", "process"] { + run_success( + root, + &["config", &format!("filter..{command}"), FILTER_COMMAND], + ); + } + run_success(root, &["config", "filter.unused.clean", "false"]); + + let result = apply_git_patch(&request( + root, /*revert*/ false, /*preflight*/ true, + )) + .expect("allow effectively empty empty-name filter"); + assert_eq!(result.exit_code, 0); + assert!(!configured_filter_ran(root)); +} diff --git a/codex-rs/git-utils/src/patch_paths.rs b/codex-rs/git-utils/src/patch_paths.rs index d6889fce89..7d606b94a5 100644 --- a/codex-rs/git-utils/src/patch_paths.rs +++ b/codex-rs/git-utils/src/patch_paths.rs @@ -230,13 +230,14 @@ pub fn stage_paths(git_root: &Path, diff: &str) -> io::Result<()> { let (tmpdir, patch_path) = write_temp_patch(diff)?; let paths = extract_effective_paths_from_patch(&git, &patch_path, /*revert*/ true)?; let _guard = tmpdir; - stage_effective_paths(&git, git_root, &paths) + stage_effective_paths(&git, git_root, &paths, &[]) } pub(crate) fn stage_effective_paths( git: &GitRunner, git_root: &Path, paths: &[String], + git_config_args: &[String], ) -> io::Result<()> { let confined = confine_patch_paths(git, git_root, paths)?; let mut existing = Vec::new(); @@ -260,7 +261,8 @@ pub(crate) fn stage_effective_paths( "--".to_string(), ]; args.extend(existing); - let config_parts = safe_git_config_parts(); + let mut config_parts = git_config_args.to_vec(); + config_parts.extend(safe_git_config_parts()); let (_code, _, _) = run_git(git, git_root, &config_parts, &args)?; // We do not hard fail staging; best-effort is OK. Return Ok even on non-zero. Ok(()) diff --git a/codex-rs/git-utils/src/safe_git.rs b/codex-rs/git-utils/src/safe_git.rs index 4d28c631c3..84172da83d 100644 --- a/codex-rs/git-utils/src/safe_git.rs +++ b/codex-rs/git-utils/src/safe_git.rs @@ -15,6 +15,12 @@ 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, Clone, PartialEq, Eq)] +enum FilterAttributeValue { + Driver(String), + AmbiguousSentinel(String), +} + const ISOLATED_GIT_ENVIRONMENT: [&str; 11] = [ "GIT_DIR", "GIT_WORK_TREE", @@ -49,14 +55,16 @@ pub(crate) fn ensure_no_selected_executable_git_filters( git_config_args: &[String], ) -> io::Result<()> { let entries = read_filter_config(git, cwd, git_config_args)?; - if !entries.values().any(|entry| !entry.value.is_empty()) { + let executable_drivers = executable_filter_drivers(&entries)?; + if executable_drivers.is_empty() { return Ok(()); } let paths = paths .iter() .map(|path| path.as_bytes().to_vec()) .collect::>(); - let attributes = read_filter_attributes(git, cwd, &paths, git_config_args)?; + let attributes = + read_filter_attributes(git, cwd, &paths, git_config_args, &executable_drivers)?; if let Some((driver, path)) = selected_executable_filter(&entries, &attributes)? { return Err(io::Error::new( io::ErrorKind::Unsupported, @@ -147,6 +155,7 @@ fn read_filter_attributes( cwd: &Path, paths: &[Vec], git_config_args: &[String], + executable_drivers: &BTreeSet, ) -> io::Result, String>> { if paths.is_empty() { return Ok(BTreeMap::new()); @@ -179,13 +188,149 @@ fn read_filter_attributes( String::from_utf8_lossy(&output.stderr).trim() ))); } - parse_filter_attributes(&output.stdout, paths) + let attributes = parse_filter_attributes(&output.stdout, paths)?; + resolve_filter_attribute_sentinels(git, cwd, attributes, git_config_args, executable_drivers) +} + +fn resolve_filter_attribute_sentinels( + git: &GitRunner, + cwd: &Path, + attributes: BTreeMap, FilterAttributeValue>, + git_config_args: &[String], + executable_drivers: &BTreeSet, +) -> io::Result, String>> { + let mut resolved = BTreeMap::new(); + for (path, attribute) in attributes { + match attribute { + FilterAttributeValue::Driver(driver) => { + resolved.insert(path, driver); + } + FilterAttributeValue::AmbiguousSentinel(driver) => { + if executable_drivers.contains(&driver) + && sentinel_spelling_selects_filter_driver( + git, + cwd, + &path, + &driver, + git_config_args, + )? + { + resolved.insert(path, driver); + } + } + } + } + Ok(resolved) +} + +/// `git check-attr` serializes both its three special states and literal +/// driver names with the same `set`, `unset`, and `unspecified` strings. Ask +/// Git to resolve the ambiguity with every command for that driver overridden +/// to empty. A required literal driver fails while a special state succeeds. +/// Retrying with the driver optional distinguishes that expected failure from +/// an unrelated probe error. No filter process or shell is started. +fn sentinel_spelling_selects_filter_driver( + git: &GitRunner, + cwd: &Path, + path: &[u8], + driver: &str, + git_config_args: &[String], +) -> io::Result { + let required = run_sentinel_selection_probe( + git, + cwd, + path, + driver, + git_config_args, + /*required*/ true, + )?; + if required.status.success() { + return Ok(false); + } + let optional = run_sentinel_selection_probe( + git, + cwd, + path, + driver, + git_config_args, + /*required*/ false, + )?; + if optional.status.success() { + return Ok(true); + } + Err(io::Error::other(format!( + "git filter attribute selection probe failed with required status {} and optional status {}: {}", + required.status, + optional.status, + String::from_utf8_lossy(&optional.stderr).trim() + ))) +} + +fn run_sentinel_selection_probe( + git: &GitRunner, + cwd: &Path, + path: &[u8], + driver: &str, + git_config_args: &[String], + required: bool, +) -> io::Result { + let mut command = git.command(); + command + .env("GIT_OPTIONAL_LOCKS", "0") + .args(git_config_args) + .args([ + "-c", + &format!("core.hooksPath={DISABLED_HOOKS_PATH}"), + "-c", + "core.fsmonitor=false", + "-c", + &format!("filter.{driver}.required={required}"), + "-c", + &format!("filter.{driver}.clean="), + "-c", + &format!("filter.{driver}.smudge="), + "-c", + &format!("filter.{driver}.process="), + "hash-object", + "--stdin", + ]) + .arg("--path") + .arg(git_path_argument(path)?) + .current_dir(cwd) + .stdin(Stdio::null()); + git.output(command) +} + +#[cfg(unix)] +fn git_path_argument(path: &[u8]) -> io::Result { + use std::os::unix::ffi::OsStringExt; + + Ok(std::ffi::OsString::from_vec(path.to_vec())) +} + +#[cfg(not(unix))] +fn git_path_argument(path: &[u8]) -> io::Result { + let path = std::str::from_utf8(path) + .map_err(|_| invalid_filter_output("non-UTF-8 Git filter attribute path"))?; + Ok(path.into()) } fn selected_executable_filter( entries: &BTreeMap, attributes: &BTreeMap, String>, ) -> io::Result)>> { + let executable_drivers = executable_filter_drivers(entries)?; + for (path, driver) in attributes { + if executable_drivers.contains(driver) { + return Ok(Some((driver.clone(), path.clone()))); + } + } + Ok(None) +} + +fn executable_filter_drivers( + entries: &BTreeMap, +) -> io::Result> { let mut executable_drivers = BTreeSet::new(); for entry in entries.values() { let driver = filter_driver_name(&entry.key)?; @@ -193,12 +338,7 @@ fn selected_executable_filter( executable_drivers.insert(driver); } } - for (path, driver) in attributes { - if executable_drivers.contains(driver) { - return Ok(Some((driver.clone(), path.clone()))); - } - } - Ok(None) + Ok(executable_drivers) } fn filter_driver_name(key: &str) -> io::Result { @@ -208,7 +348,6 @@ fn filter_driver_name(key: &str) -> io::Result { let driver = [".clean", ".smudge", ".process"] .into_iter() .find_map(|suffix| remainder.strip_suffix(suffix)) - .filter(|driver| !driver.is_empty()) .ok_or_else(|| invalid_filter_output("malformed filter config key"))?; Ok(driver.to_string()) } @@ -230,7 +369,7 @@ fn write_nul_paths(input: &mut std::fs::File, paths: &[Vec]) -> io::Result<( fn parse_filter_attributes( output: &[u8], expected_paths: &[Vec], -) -> io::Result, String>> { +) -> io::Result, FilterAttributeValue>> { let expected = expected_paths .iter() .map(Vec::as_slice) @@ -258,10 +397,13 @@ fn parse_filter_attributes( } let driver = std::str::from_utf8(record[2]) .map_err(|_| invalid_filter_output("non-UTF-8 Git filter attribute value"))?; - if attributes - .insert(record[0].to_vec(), driver.to_string()) - .is_some() - { + let value = match driver { + "set" | "unset" | "unspecified" => { + FilterAttributeValue::AmbiguousSentinel(driver.to_string()) + } + _ => FilterAttributeValue::Driver(driver.to_string()), + }; + if attributes.insert(record[0].to_vec(), value).is_some() { return Err(invalid_filter_output( "duplicate Git filter attribute record", )); diff --git a/codex-rs/git-utils/src/safe_git_tests.rs b/codex-rs/git-utils/src/safe_git_tests.rs index 7f5dca8a83..3fa123644d 100644 --- a/codex-rs/git-utils/src/safe_git_tests.rs +++ b/codex-rs/git-utils/src/safe_git_tests.rs @@ -57,6 +57,14 @@ fn selected_filter_policy_allows_effective_empty_value() { ); } +#[test] +fn filter_driver_parser_accepts_empty_name() { + assert_eq!( + filter_driver_name("filter..clean").expect("empty filter name"), + "" + ); +} + #[test] fn filter_attribute_parser_rejects_malformed_or_unexpected_records() { let paths = vec![b"a.txt".to_vec(), b"b.txt".to_vec()]; @@ -64,12 +72,14 @@ fn filter_attribute_parser_rejects_malformed_or_unexpected_records() { parse_filter_attributes(b"a.txt\0filter\0unspecified\0b.txt\0filter\0lfs\0", &paths) .expect("parse attributes"); assert_eq!( - parsed.get(b"a.txt".as_slice()).map(String::as_str), - Some("unspecified") + parsed.get(b"a.txt".as_slice()), + Some(&FilterAttributeValue::AmbiguousSentinel( + "unspecified".to_string() + )) ); assert_eq!( - parsed.get(b"b.txt".as_slice()).map(String::as_str), - Some("lfs") + parsed.get(b"b.txt".as_slice()), + Some(&FilterAttributeValue::Driver("lfs".to_string())) ); for output in [