diff --git a/codex-rs/git-utils/src/patch_paths_tests.rs b/codex-rs/git-utils/src/patch_paths_tests.rs index 52000767e6..889d46dc6f 100644 --- a/codex-rs/git-utils/src/patch_paths_tests.rs +++ b/codex-rs/git-utils/src/patch_paths_tests.rs @@ -1017,17 +1017,46 @@ fn staging_allows_selected_symlink_while_neutralizing_off_path_filters() { #[cfg(unix)] #[test] fn staging_allows_optional_smudge_only_but_refuses_required_or_malformed_smudge() { - for (required, accepted) in [("false", true), ("yes", false), ("not-a-bool", false)] { + use std::io::Write as _; + + for (required, accepted) in [ + ("absent", true), + ("empty", true), + ("false", true), + ("yes", false), + ("implicit", false), + ("not-a-bool", false), + ] { let fixture = init_racy_filter_fixture("selected", "smudge"); let root = fixture.repo.path(); - assert_eq!( - run( - root, - &["git", "config", "filter.selected.required", required], - ) - .0, - 0 - ); + if matches!(required, "absent" | "implicit") { + assert_eq!( + run( + root, + &["git", "config", "--unset-all", "filter.selected.required"], + ) + .0, + 0 + ); + } + match required { + "absent" => {} + "implicit" => { + let mut config = std::fs::OpenOptions::new() + .append(true) + .open(root.join(".git/config")) + .expect("open repository config"); + writeln!(config, "[filter \"selected\"]\n\trequired") + .expect("write implicit true required value"); + } + value => { + let value = if value == "empty" { "" } else { value }; + assert_eq!( + run(root, &["git", "config", "filter.selected.required", value],).0, + 0 + ); + } + } std::fs::write(root.join("selected.txt"), "new selected\n") .expect("modify selected target"); let before = git_index_bytes(root); diff --git a/codex-rs/git-utils/src/safe_git.rs b/codex-rs/git-utils/src/safe_git.rs index 5a2561811d..f36e78abc3 100644 --- a/codex-rs/git-utils/src/safe_git.rs +++ b/codex-rs/git-utils/src/safe_git.rs @@ -13,8 +13,7 @@ use crate::git_config::parse_effective_config; 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|required)$"; +pub(crate) const EXECUTABLE_FILTER_CONFIG_PATTERN: &str = r"^filter\..*\.(clean|smudge|process)$"; #[derive(Debug, Clone, PartialEq, Eq)] enum FilterAttributeValue { @@ -126,7 +125,7 @@ fn ensure_no_selected_executable_git_filters_for( } let refused = neutralized_drivers.contains(driver) || matches!(execution, FilterExecution::GitAdd) - && git_filter_required(git, cwd, &entries, driver)?; + && git_filter_required(git, cwd, driver, git_config_args)?; if refused { return Err(io::Error::new( io::ErrorKind::Unsupported, @@ -531,31 +530,25 @@ fn effective_filter_value<'a>( fn git_filter_required( git: &GitRunner, cwd: &Path, - entries: &BTreeMap, driver: &str, + git_config_args: &[String], ) -> io::Result { - let Some(value) = effective_filter_value(entries, driver, "required") else { - return Ok(false); - }; - let config = format!("codex.required={value}"); let mut command = git.command(); command .env("GIT_OPTIONAL_LOCKS", "0") - .args([ - "-c", - &config, - "config", - "--type=bool", - "--get", - "codex.required", - ]) + .args(git_config_args) + .args(["config", "--type=bool", "--get"]) + .arg(format!("filter.{driver}.required")) .current_dir(cwd); let output = git.output(command)?; + if output.status.code() == Some(1) && output.stderr.is_empty() { + return Ok(false); + } if !output.status.success() { return Err(io::Error::new( io::ErrorKind::Unsupported, format!( - "refusing selected Git filter {driver:?} with malformed required value {value:?}: {}", + "refusing selected Git filter {driver:?} with malformed required value: {}", String::from_utf8_lossy(&output.stderr).trim() ), )); diff --git a/codex-rs/git-utils/src/safe_git_tests.rs b/codex-rs/git-utils/src/safe_git_tests.rs index 39ffd242bf..328c76e77f 100644 --- a/codex-rs/git-utils/src/safe_git_tests.rs +++ b/codex-rs/git-utils/src/safe_git_tests.rs @@ -92,43 +92,16 @@ fn git_add_filter_policy_rejects_clean_and_process_but_allows_smudge_only() { } #[test] -fn filter_snapshot_retains_required_without_treating_it_as_executable() { - let mut entries = filter_entries( - GitConfigScope::Local, - Path::new("config"), - "filter.demo.smudge", - "smudge-command", - ); - entries.extend(filter_entries( +fn required_config_is_not_treated_as_an_executable_command() { + let entries = filter_entries( GitConfigScope::Command, Path::new("command line:"), "filter.demo.required", "true", - )); + ); assert_eq!( executable_filter_drivers(&entries).expect("executable drivers"), - BTreeSet::from(["demo".to_string()]) - ); - let neutralization = GitFilterNeutralization { - git_config_args: Vec::new(), - _config_dir: None, - filter_config: entries, - }; - assert_eq!( - neutralization.filter_value("demo", "required"), - Some("true") - ); - - let required_only = filter_entries( - GitConfigScope::Local, - Path::new("config"), - "filter.demo.required", - "true", - ); - assert!( - executable_filter_drivers(&required_only) - .expect("required-only config") - .is_empty() + BTreeSet::new() ); }