diff --git a/codex-rs/tui/src/get_git_diff.rs b/codex-rs/tui/src/get_git_diff.rs index a7b4b668fbac..7f5545070784 100644 --- a/codex-rs/tui/src/get_git_diff.rs +++ b/codex-rs/tui/src/get_git_diff.rs @@ -13,6 +13,13 @@ use crate::workspace_command::WorkspaceCommandExecutor; use crate::workspace_command::WorkspaceCommandOutput; const DIFF_COMMAND_TIMEOUT: Duration = Duration::from_secs(/*secs*/ 30); +const DISABLE_FSMONITOR_CONFIG: &str = "core.fsmonitor=false"; +const DISABLE_HOOKS_CONFIG: &str = if cfg!(windows) { + "core.hooksPath=NUL" +} else { + "core.hooksPath=/dev/null" +}; +const EXECUTABLE_FILTER_CONFIG_PATTERN: &str = r"^filter\..*\.(clean|process)$"; /// Return value of [`get_git_diff`]. /// @@ -27,9 +34,22 @@ pub(crate) async fn get_git_diff( return Ok((false, String::new())); } - // Run tracked diff and untracked file listing in parallel. + // Keep `/diff` informational: repository configuration must not select executable diff helpers. + let diff_config_overrides = diff_filter_config_overrides(runner, cwd).await?; let (tracked_diff_res, untracked_output_res) = tokio::join!( - run_git_capture_diff(runner, cwd, &["diff", "--color"]), + run_git_capture_diff( + runner, + cwd, + &diff_config_overrides, + &[ + "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", + "--color", + ] + ), run_git_capture_stdout(runner, cwd, &["ls-files", "--others", "--exclude-standard"]), ); let tracked_diff = tracked_diff_res?; @@ -48,8 +68,19 @@ pub(crate) async fn get_git_diff( .map(str::trim) .filter(|s| !s.is_empty()) { - let args = ["diff", "--color", "--no-index", "--", null_path, file]; - let diff = run_git_capture_diff(runner, cwd, &args).await?; + let args = [ + "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", + "--color", + "--no-index", + "--", + null_path, + file, + ]; + let diff = run_git_capture_diff(runner, cwd, &diff_config_overrides, &args).await?; untracked_diff.push_str(&diff); } @@ -63,7 +94,7 @@ async fn run_git_capture_stdout( cwd: &Path, args: &[&str], ) -> Result { - let output = run_git_command(runner, cwd, args).await?; + let output = run_git_command(runner, cwd, &[], args).await?; if output.success() { Ok(output.stdout) } else { @@ -79,9 +110,10 @@ async fn run_git_capture_stdout( async fn run_git_capture_diff( runner: &dyn WorkspaceCommandExecutor, cwd: &Path, + config_overrides: &[(String, String)], args: &[&str], ) -> Result { - let output = run_git_command(runner, cwd, args).await?; + let output = run_git_command(runner, cwd, config_overrides, args).await?; if output.success() || output.exit_code == 1 { Ok(output.stdout) } else { @@ -92,32 +124,88 @@ async fn run_git_capture_diff( } } +/// Return Git configuration overrides that prevent configured filter drivers +/// from executing while generating diffs. +async fn diff_filter_config_overrides( + runner: &dyn WorkspaceCommandExecutor, + cwd: &Path, +) -> Result, String> { + let args = [ + "config", + "--null", + "--name-only", + "--get-regexp", + EXECUTABLE_FILTER_CONFIG_PATTERN, + ]; + let output = run_git_command(runner, cwd, &[], &args).await?; + if output.exit_code != 0 && output.exit_code != 1 { + return Err(format!( + "git {:?} failed with status {}", + args, output.exit_code + )); + } + + let mut drivers = output + .stdout + .split('\0') + .filter_map(|key| { + key.strip_suffix(".clean") + .or_else(|| key.strip_suffix(".process")) + }) + .map(str::to_string) + .collect::>(); + drivers.sort(); + drivers.dedup(); + + Ok(drivers + .into_iter() + .flat_map(|driver| { + [ + (format!("{driver}.clean"), String::new()), + (format!("{driver}.process"), String::new()), + (format!("{driver}.required"), "false".to_string()), + ] + }) + .collect()) +} + /// Determine if the current directory is inside a Git repository. async fn inside_git_repo( runner: &dyn WorkspaceCommandExecutor, cwd: &Path, ) -> Result { - let output = run_git_command(runner, cwd, &["rev-parse", "--is-inside-work-tree"]).await?; + let output = run_git_command(runner, cwd, &[], &["rev-parse", "--is-inside-work-tree"]).await?; Ok(output.success()) } async fn run_git_command( runner: &dyn WorkspaceCommandExecutor, cwd: &Path, + config_overrides: &[(String, String)], args: &[&str], ) -> Result { - let mut argv = Vec::with_capacity(args.len() + 1); + let mut argv = Vec::with_capacity(args.len() + 5); argv.push("git".to_string()); + argv.extend([ + "-c".to_string(), + DISABLE_FSMONITOR_CONFIG.to_string(), + "-c".to_string(), + DISABLE_HOOKS_CONFIG.to_string(), + ]); argv.extend(args.iter().map(|arg| (*arg).to_string())); - runner - .run( - WorkspaceCommand::new(argv) - .cwd(cwd.to_path_buf()) - .timeout(DIFF_COMMAND_TIMEOUT) - .disable_output_cap(), - ) - .await - .map_err(|err| err.to_string()) + let mut command = WorkspaceCommand::new(argv) + .cwd(cwd.to_path_buf()) + .timeout(DIFF_COMMAND_TIMEOUT) + .disable_output_cap(); + if !config_overrides.is_empty() { + command = command.env("GIT_CONFIG_COUNT", config_overrides.len().to_string()); + for (index, (key, value)) in config_overrides.iter().enumerate() { + command = command + .env(format!("GIT_CONFIG_KEY_{index}"), key) + .env(format!("GIT_CONFIG_VALUE_{index}"), value); + } + } + runner.run(command).await.map_err(|err| err.to_string()) } #[cfg(test)] @@ -125,17 +213,24 @@ mod tests { use super::*; use crate::workspace_command::WorkspaceCommandError; use pretty_assertions::assert_eq; + use std::collections::HashMap; use std::collections::VecDeque; + #[cfg(unix)] + use std::fs; use std::future::Future; + #[cfg(unix)] + use std::os::unix::fs::PermissionsExt; use std::path::PathBuf; use std::pin::Pin; + #[cfg(unix)] + use std::process::Command as ProcessCommand; use std::sync::Mutex; #[tokio::test] async fn get_git_diff_returns_not_git_for_non_git_cwd() { let cwd = PathBuf::from("/workspace"); let runner = FakeRunner::new(vec![response( - &["git", "rev-parse", "--is-inside-work-tree"], + git_command(&["rev-parse", "--is-inside-work-tree"]), /*exit_code*/ 128, "", )]); @@ -145,40 +240,61 @@ mod tests { assert_eq!(result, Ok((false, String::new()))); assert_commands( &runner.commands(), - &[&["git", "rev-parse", "--is-inside-work-tree"]], + &[git_command(&["rev-parse", "--is-inside-work-tree"])], &cwd, ); } #[tokio::test] - async fn get_git_diff_concatenates_tracked_and_untracked_diffs() { + async fn get_git_diff_disables_helpers_for_tracked_and_untracked_diffs() { let cwd = PathBuf::from("/workspace"); let runner = FakeRunner::new(vec![ response( - &["git", "rev-parse", "--is-inside-work-tree"], + git_command(&["rev-parse", "--is-inside-work-tree"]), /*exit_code*/ 0, "true\n", ), response( - &["git", "diff", "--color"], + git_command(&[ + "config", + "--null", + "--name-only", + "--get-regexp", + EXECUTABLE_FILTER_CONFIG_PATTERN, + ]), + /*exit_code*/ 0, + "filter.evil.clean\0filter.evil.process\0", + ), + response( + git_command(&[ + "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", + "--color", + ]), /*exit_code*/ 1, "tracked\n", ), response( - &["git", "ls-files", "--others", "--exclude-standard"], + git_command(&["ls-files", "--others", "--exclude-standard"]), /*exit_code*/ 0, "new.txt\n", ), response( - &[ - "git", + git_command(&[ "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", "--color", "--no-index", "--", null_device(), "new.txt", - ], + ]), /*exit_code*/ 1, "untracked\n", ), @@ -187,24 +303,44 @@ mod tests { let result = get_git_diff(&runner, &cwd).await; assert_eq!(result, Ok((true, "tracked\nuntracked\n".to_string()))); + let commands = runner.commands(); assert_commands( - &runner.commands(), + &commands, &[ - &["git", "rev-parse", "--is-inside-work-tree"], - &["git", "diff", "--color"], - &["git", "ls-files", "--others", "--exclude-standard"], - &[ - "git", + git_command(&["rev-parse", "--is-inside-work-tree"]), + git_command(&[ + "config", + "--null", + "--name-only", + "--get-regexp", + EXECUTABLE_FILTER_CONFIG_PATTERN, + ]), + git_command(&[ + "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", + "--color", + ]), + git_command(&["ls-files", "--others", "--exclude-standard"]), + git_command(&[ "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", "--color", "--no-index", "--", null_device(), "new.txt", - ], + ]), ], &cwd, ); + assert_eq!(commands[2].env, filter_override_env("filter.evil")); + assert_eq!(commands[4].env, filter_override_env("filter.evil")); } #[tokio::test] @@ -212,17 +348,35 @@ mod tests { let cwd = PathBuf::from("/workspace"); let runner = FakeRunner::new(vec![ response( - &["git", "rev-parse", "--is-inside-work-tree"], + git_command(&["rev-parse", "--is-inside-work-tree"]), /*exit_code*/ 0, "true\n", ), response( - &["git", "diff", "--color"], + git_command(&[ + "config", + "--null", + "--name-only", + "--get-regexp", + EXECUTABLE_FILTER_CONFIG_PATTERN, + ]), + /*exit_code*/ 1, + "", + ), + response( + git_command(&[ + "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", + "--color", + ]), /*exit_code*/ 1, "tracked\n", ), response( - &["git", "ls-files", "--others", "--exclude-standard"], + git_command(&["ls-files", "--others", "--exclude-standard"]), /*exit_code*/ 0, "", ), @@ -238,13 +392,35 @@ mod tests { let cwd = PathBuf::from("/workspace"); let runner = FakeRunner::new(vec![ response( - &["git", "rev-parse", "--is-inside-work-tree"], + git_command(&["rev-parse", "--is-inside-work-tree"]), /*exit_code*/ 0, "true\n", ), - response(&["git", "diff", "--color"], /*exit_code*/ 2, ""), response( - &["git", "ls-files", "--others", "--exclude-standard"], + git_command(&[ + "config", + "--null", + "--name-only", + "--get-regexp", + EXECUTABLE_FILTER_CONFIG_PATTERN, + ]), + /*exit_code*/ 1, + "", + ), + response( + git_command(&[ + "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", + "--color", + ]), + /*exit_code*/ 2, + "", + ), + response( + git_command(&["ls-files", "--others", "--exclude-standard"]), /*exit_code*/ 0, "", ), @@ -255,14 +431,180 @@ mod tests { .expect_err("unexpected git diff status should fail"); assert!( - error.contains("git [\"diff\", \"--color\"] failed with status 2"), + error.contains( + "git [\"diff\", \"--no-textconv\", \"--no-ext-diff\", \"--submodule=short\", \"--ignore-submodules=dirty\", \"--color\"] failed with status 2" + ), "unexpected error: {error}", ); } - fn response(argv: &[&str], exit_code: i32, stdout: &str) -> FakeResponse { + #[cfg(unix)] + #[tokio::test] + async fn get_git_diff_does_not_execute_configured_filters_fsmonitor_or_hooks() { + let tempdir = tempfile::tempdir().expect("create temp directory"); + let repo = tempdir.path().join("repo"); + fs::create_dir(&repo).expect("create test repository directory"); + run_git_setup(&repo, &["init", "-q"]); + run_git_setup(&repo, &["config", "user.name", "test"]); + run_git_setup(&repo, &["config", "user.email", "test@example.com"]); + fs::write(repo.join(".gitattributes"), "*.txt filter=x=y\n").expect("write attributes"); + fs::write(repo.join("tracked.txt"), "before\n").expect("write tracked file"); + fs::write(repo.join("unchanged.txt"), "unchanged\n").expect("write unchanged file"); + run_git_setup( + &repo, + &["add", ".gitattributes", "tracked.txt", "unchanged.txt"], + ); + run_git_setup(&repo, &["commit", "-qm", "initial"]); + + let filter_helper = tempdir.path().join("filter-helper.sh"); + let fsmonitor_helper = tempdir.path().join("fsmonitor-helper.sh"); + let hooks_dir = tempdir.path().join("hooks"); + let hook_helper = hooks_dir.join("post-index-change"); + fs::create_dir(&hooks_dir).expect("create hooks directory"); + write_marker_helper(&filter_helper); + write_marker_helper(&fsmonitor_helper); + write_marker_helper(&hook_helper); + run_git_setup( + &repo, + &[ + "config", + "filter.x=y.clean", + filter_helper.to_str().expect("filter helper path"), + ], + ); + run_git_setup( + &repo, + &[ + "config", + "filter.x=y.process", + filter_helper.to_str().expect("filter helper path"), + ], + ); + run_git_setup(&repo, &["config", "filter.x=y.required", "true"]); + run_git_setup( + &repo, + &[ + "config", + "core.fsmonitor", + fsmonitor_helper.to_str().expect("fsmonitor helper path"), + ], + ); + run_git_setup( + &repo, + &[ + "config", + "core.hooksPath", + hooks_dir.to_str().expect("hooks directory path"), + ], + ); + std::thread::sleep(Duration::from_secs(/*secs*/ 1)); + fs::write(repo.join("unchanged.txt"), "unchanged\n").expect("refresh unchanged file"); + fs::write(repo.join("tracked.txt"), "after\n").expect("modify tracked file"); + + let result = get_git_diff(&LocalRunner, &repo) + .await + .expect("generate diff without invoking helpers"); + + assert!(result.1.contains("before")); + assert!(result.1.contains("after")); + assert!(!filter_helper.with_extension("sh.ran").exists()); + assert!(!fsmonitor_helper.with_extension("sh.ran").exists()); + assert!(!hook_helper.with_extension("sh.ran").exists()); + } + + #[cfg(unix)] + #[tokio::test] + async fn get_git_diff_does_not_execute_helpers_while_checking_dirty_submodules() { + let tempdir = tempfile::tempdir().expect("create temp directory"); + let child = tempdir.path().join("child"); + let repo = tempdir.path().join("repo"); + fs::create_dir(&child).expect("create child repository directory"); + fs::create_dir(&repo).expect("create parent repository directory"); + run_git_setup(&child, &["init", "-q"]); + run_git_setup(&child, &["config", "user.name", "test"]); + run_git_setup(&child, &["config", "user.email", "test@example.com"]); + fs::write(child.join(".gitattributes"), "*.txt filter=evil\n") + .expect("write child attributes"); + fs::write(child.join("tracked.txt"), "before\n").expect("write child tracked file"); + run_git_setup(&child, &["add", ".gitattributes", "tracked.txt"]); + run_git_setup(&child, &["commit", "-qm", "initial"]); + + run_git_setup(&repo, &["init", "-q"]); + run_git_setup(&repo, &["config", "user.name", "test"]); + run_git_setup(&repo, &["config", "user.email", "test@example.com"]); + run_git_setup( + &repo, + &[ + "-c", + "protocol.file.allow=always", + "submodule", + "add", + "-q", + child.to_str().expect("child repository path"), + "child", + ], + ); + run_git_setup(&repo, &["commit", "-qm", "add submodule"]); + + let helper = tempdir.path().join("submodule-helper.sh"); + write_marker_helper(&helper); + let checkout = repo.join("child"); + run_git_setup( + &checkout, + &[ + "config", + "filter.evil.clean", + helper.to_str().expect("submodule helper path"), + ], + ); + run_git_setup(&checkout, &["config", "filter.evil.required", "true"]); + std::thread::sleep(Duration::from_secs(/*secs*/ 1)); + fs::write(checkout.join("tracked.txt"), "before\n").expect("refresh child tracked file"); + + let result = get_git_diff(&LocalRunner, &repo) + .await + .expect("generate diff without inspecting submodule worktrees"); + + assert!(result.1.is_empty()); + assert!(!helper.with_extension("sh.ran").exists()); + } + + fn git_command(args: &[&str]) -> Vec { + let mut argv = vec![ + "git".to_string(), + "-c".to_string(), + DISABLE_FSMONITOR_CONFIG.to_string(), + "-c".to_string(), + DISABLE_HOOKS_CONFIG.to_string(), + ]; + argv.extend(args.iter().map(|arg| (*arg).to_string())); + argv + } + + fn filter_override_env(driver: &str) -> HashMap> { + HashMap::from([ + ("GIT_CONFIG_COUNT".to_string(), Some("3".to_string())), + ( + "GIT_CONFIG_KEY_0".to_string(), + Some(format!("{driver}.clean")), + ), + ("GIT_CONFIG_VALUE_0".to_string(), Some(String::new())), + ( + "GIT_CONFIG_KEY_1".to_string(), + Some(format!("{driver}.process")), + ), + ("GIT_CONFIG_VALUE_1".to_string(), Some(String::new())), + ( + "GIT_CONFIG_KEY_2".to_string(), + Some(format!("{driver}.required")), + ), + ("GIT_CONFIG_VALUE_2".to_string(), Some("false".to_string())), + ]) + } + + fn response(argv: Vec, exit_code: i32, stdout: &str) -> FakeResponse { FakeResponse { - argv: argv.iter().map(|arg| (*arg).to_string()).collect(), + argv, output: WorkspaceCommandOutput { exit_code, stdout: stdout.to_string(), @@ -275,15 +617,32 @@ mod tests { if cfg!(windows) { "NUL" } else { "/dev/null" } } - fn assert_commands(commands: &[WorkspaceCommand], expected: &[&[&str]], cwd: &Path) { + #[cfg(unix)] + fn run_git_setup(cwd: &Path, args: &[&str]) { + let status = ProcessCommand::new("git") + .args(args) + .current_dir(cwd) + .status() + .expect("run git setup command"); + assert!(status.success(), "git setup command failed: {args:?}"); + } + + #[cfg(unix)] + fn write_marker_helper(path: &Path) { + fs::write(path, "#!/bin/sh\nprintf ran >> \"$0.ran\"\nexit 1\n") + .expect("write helper script"); + let mut permissions = fs::metadata(path) + .expect("read helper metadata") + .permissions(); + permissions.set_mode(/*mode*/ 0o755); + fs::set_permissions(path, permissions).expect("make helper executable"); + } + + fn assert_commands(commands: &[WorkspaceCommand], expected: &[Vec], cwd: &Path) { let actual: Vec> = commands .iter() .map(|command| command.argv.clone()) .collect(); - let expected: Vec> = expected - .iter() - .map(|argv| argv.iter().map(|arg| (*arg).to_string()).collect()) - .collect(); assert_eq!(actual, expected); for command in commands { @@ -336,4 +695,44 @@ mod tests { }) } } + + #[cfg(unix)] + struct LocalRunner; + + #[cfg(unix)] + impl WorkspaceCommandExecutor for LocalRunner { + fn run( + &self, + command: WorkspaceCommand, + ) -> Pin< + Box< + dyn Future> + + Send + + '_, + >, + > { + Box::pin(async move { + let mut process = ProcessCommand::new(&command.argv[0]); + process + .args(&command.argv[1..]) + .current_dir(command.cwd.expect("test command cwd")); + for (key, value) in command.env { + match value { + Some(value) => { + process.env(key, value); + } + None => { + process.env_remove(key); + } + } + } + let output = process.output().expect("run test command"); + Ok(WorkspaceCommandOutput { + exit_code: output.status.code().expect("test command exit code"), + stdout: String::from_utf8(output.stdout).expect("utf8 stdout"), + stderr: String::from_utf8(output.stderr).expect("utf8 stderr"), + }) + }) + } + } }