openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-19 17:28:46 +00:00
Code Issues Packages Projects Releases Wiki Activity

git-utils: fix selected filter edge cases

Browse Source
This commit is contained in:
Chris Bookholt
2026-07-01 15:17:42 -07:00
parent 0b84913ea8
commit 5b1fe8c0f2
5 changed files with 407 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/git-utils/src/apply.rs
+5 -1
View File
@@ -59,7 +59,7 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result<ApplyGitResult> {
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::*;
codex-rs/git-utils/src/apply_filter_tests.rs
+227
View File
@@ -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));
}
codex-rs/git-utils/src/patch_paths.rs
+4 -2
View File
@@ -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(())
codex-rs/git-utils/src/safe_git.rs
+157 -15
View File
@@ -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::<Vec<_>>();
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<u8>],
git_config_args: &[String],
executable_drivers: &BTreeSet<String>,
) -> io::Result<BTreeMap<Vec<u8>, 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<Vec<u8>, FilterAttributeValue>,
git_config_args: &[String],
executable_drivers: &BTreeSet<String>,
) -> io::Result<BTreeMap<Vec<u8>, 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<bool> {
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<std::process::Output> {
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<std::ffi::OsString> {
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<std::ffi::OsString> {
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<String, GitConfigEntry>,
attributes: &BTreeMap<Vec<u8>, String>,
) -> io::Result<Option<(String, Vec<u8>)>> {
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<String, GitConfigEntry>,
) -> io::Result<BTreeSet<String>> {
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<String> {
@@ -208,7 +348,6 @@ fn filter_driver_name(key: &str) -> io::Result<String> {
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<u8>]) -> io::Result<(
fn parse_filter_attributes(
output: &[u8],
expected_paths: &[Vec<u8>],
) -> io::Result<BTreeMap<Vec<u8>, String>> {
) -> io::Result<BTreeMap<Vec<u8>, 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",
));
codex-rs/git-utils/src/safe_git_tests.rs
+14 -4
View File
@@ -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 [