From 7dbe7f3e920ed08b1f2bab4abae6c09da35c80c6 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Sun, 7 Jun 2026 08:59:04 -0400 Subject: [PATCH 1/7] [codex] preserve fsmonitor for worktree Git reads Codex forced core.fsmonitor=false on every internal Git command to prevent repository-selected fsmonitor helpers from running. That also disabled Git's built-in fsmonitor for status, diff, and ls-files, turning those worktree reads into full scans in large repositories. Probe effective core.fsmonitor only for commands that can benefit. Preserve canonical boolean true while mapping false, unset, helper paths, malformed values, and probe failures to false. Current Git boolean semantics start the built-in daemon on demand and fall back to a full scan when it cannot be queried. Share the original five-second deadline between the probe and requested command so the extra process cannot extend the operation's timeout. Do not cache the decision because layered and conditional configuration can change while Codex is running. --- codex-rs/core/src/git_info_tests.rs | 2 +- codex-rs/git-utils/src/info.rs | 48 ++++++++++++++++++++++++++--- 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/codex-rs/core/src/git_info_tests.rs b/codex-rs/core/src/git_info_tests.rs index 946779d06b24..24d68dd30daf 100644 --- a/codex-rs/core/src/git_info_tests.rs +++ b/codex-rs/core/src/git_info_tests.rs @@ -342,7 +342,7 @@ async fn test_get_has_changes_with_untracked_change_returns_true() { #[cfg(unix)] #[tokio::test] -async fn test_get_has_changes_ignores_repo_fsmonitor_config() { +async fn test_get_has_changes_does_not_execute_repo_fsmonitor_helper() { let temp_dir = TempDir::new().expect("Failed to create temp dir"); let repo_path = create_test_git_repo(&temp_dir).await; let helper_path = repo_path.join("fsmonitor-helper.sh"); diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index a188d9a876b8..e0b7426fbb11 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -12,7 +12,8 @@ use serde::Deserialize; use serde::Serialize; use tokio::process::Command; use tokio::time::Duration as TokioDuration; -use tokio::time::timeout; +use tokio::time::Instant as TokioInstant; +use tokio::time::timeout_at; use ts_rs::TS; use crate::GitSha; @@ -390,16 +391,55 @@ pub async fn git_diff_to_remote(cwd: &Path) -> Option { /// Run a git command with a timeout to prevent blocking on large repositories async fn run_git_command_with_timeout(args: &[&str], cwd: &Path) -> Option { + // Only commands that inspect the worktree can recover the cost of probing. + // Built-in fsmonitor avoids worktree and untracked-file scans: + // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/Documentation/git-fsmonitor--daemon.adoc#L49-L57 + // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/Documentation/git-update-index.adoc#L545-L550 + let benefits_from_fsmonitor = matches!(args.first(), Some(&"status")) + || matches!(args.first(), Some(&"ls-files")) + || (matches!(args.first(), Some(&"diff")) && !args.contains(&"--no-index")); + let deadline = TokioInstant::now() + GIT_COMMAND_TIMEOUT; + let fsmonitor_enabled = benefits_from_fsmonitor && { + // Do not cache this decision. Effective Git config is layered, can use + // conditional includes, and can change while Codex is running. + // https://git-scm.com/docs/git-config#SCOPES + // https://git-scm.com/docs/git-config#_conditional_includes + let mut probe = Command::new("git"); + probe + .env("GIT_OPTIONAL_LOCKS", "0") + .env("LC_ALL", "C") + .args(["config", "--type=bool", "--get", "core.fsmonitor"]) + .current_dir(cwd) + .kill_on_drop(true); + let configured = match timeout_at(deadline, probe.output()).await { + Ok(Ok(output)) => Some(output), + _ => None, + }; + match configured { + None => false, + Some(output) => match (output.status.code(), output.stdout.as_slice().trim_ascii()) { + // `git config --get` returns 1 when the key is absent. + // https://git-scm.com/docs/git-config#Documentation/git-config.txt-get + (Some(1), _) | (Some(0), b"false") => false, + (Some(0), b"true") => true, + // `--type=bool` rejects hook pathnames. Keep helpers and any + // malformed or unreadable configuration disabled. + _ => false, + }, + } + }; + let mut command = Command::new("git"); command .env("GIT_OPTIONAL_LOCKS", "0") - // Keep internal Git helper commands independent of configured hook directories. + // Keep internal Git commands independent of repository-selected hooks + // and fsmonitor helpers while preserving built-in fsmonitor acceleration. .args(["-c", &format!("core.hooksPath={DISABLED_HOOKS_PATH}")]) - .args(["-c", "core.fsmonitor=false"]) + .args(["-c", &format!("core.fsmonitor={fsmonitor_enabled}")]) .args(args) .current_dir(cwd) .kill_on_drop(true); - let result = timeout(GIT_COMMAND_TIMEOUT, command.output()).await; + let result = timeout_at(deadline, command.output()).await; match result { Ok(Ok(output)) => Some(output), From 80893eea9662598d4f6371df0eb5f4d09f4c0140 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Mon, 8 Jun 2026 05:24:40 -0700 Subject: [PATCH 2/7] [codex] Gate built-in fsmonitor override Git 2.35.1 and older interpret the boolean-looking value "true" as a filesystem monitor hook path. Before Git 2.26, a successful empty hook response can incorrectly hide tracked changes. The first commit could therefore trade correctness for acceleration on older Git installations. Before preserving true, query `git version --build-options` for the `feature: fsmonitor--daemon` line, which Git exposes specifically for capability checks. Fall back to false when the feature or probe is unavailable, avoiding version parsing and preserving the previous behavior on unsupported builds. Keep both probes within the existing five-second deadline. Cover the unsupported and supported command sequences with a deterministic fake Git executable. --- codex-rs/git-utils/src/info.rs | 83 ++++++++++++++++++++++++++++++++-- 1 file changed, 80 insertions(+), 3 deletions(-) diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index e0b7426fbb11..6d2773e5b48c 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -391,6 +391,14 @@ pub async fn git_diff_to_remote(cwd: &Path) -> Option { /// Run a git command with a timeout to prevent blocking on large repositories async fn run_git_command_with_timeout(args: &[&str], cwd: &Path) -> Option { + run_git_command_with_timeout_from(Path::new("git"), args, cwd).await +} + +async fn run_git_command_with_timeout_from( + git: &Path, + args: &[&str], + cwd: &Path, +) -> Option { // Only commands that inspect the worktree can recover the cost of probing. // Built-in fsmonitor avoids worktree and untracked-file scans: // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/Documentation/git-fsmonitor--daemon.adoc#L49-L57 @@ -399,12 +407,12 @@ async fn run_git_command_with_timeout(args: &[&str], cwd: &Path) -> Option Option output + .stdout + .split(|byte| *byte == b'\n') + .any(|line| line.trim_ascii() == b"feature: fsmonitor--daemon"), + _ => false, + } + }; - let mut command = Command::new("git"); + let mut command = Command::new(git); command .env("GIT_OPTIONAL_LOCKS", "0") // Keep internal Git commands independent of repository-selected hooks @@ -889,6 +918,8 @@ pub async fn current_branch_name(cwd: &Path) -> Option { mod tests { use super::*; use pretty_assertions::assert_eq; + #[cfg(unix)] + use std::os::unix::fs::PermissionsExt; #[test] fn canonicalize_git_remote_url_normalizes_github_variants() { @@ -926,4 +957,50 @@ mod tests { assert_eq!(canonicalize_git_remote_url(remote), None); } } + + #[cfg(unix)] + #[tokio::test] + async fn fsmonitor_override_requires_builtin_daemon_support() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let git = temp_dir.path().join("git"); + let supported = temp_dir.path().join("git.supported"); + let log = temp_dir.path().join("git.log"); + std::fs::write( + &git, + "#!/bin/sh\n\ + printf '%s\\n' \"$*\" >>\"$0.log\"\n\ + case \"$1\" in\n\ + config) printf 'true\\n' ;;\n\ + version) test ! -e \"$0.supported\" || printf 'feature: fsmonitor--daemon\\n' ;;\n\ + esac\n", + ) + .expect("write fake Git"); + let mut permissions = std::fs::metadata(&git) + .expect("read fake Git metadata") + .permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(&git, permissions).expect("mark fake Git executable"); + + run_git_command_with_timeout_from(&git, &["status", "--porcelain"], temp_dir.path()) + .await + .expect("run unsupported fake Git"); + std::fs::write(&supported, "").expect("enable built-in fsmonitor feature"); + run_git_command_with_timeout_from(&git, &["status", "--porcelain"], temp_dir.path()) + .await + .expect("run supported fake Git"); + + let actual = std::fs::read_to_string(log).expect("read fake Git log"); + let disabled_hooks = format!("core.hooksPath={DISABLED_HOOKS_PATH}"); + assert_eq!( + actual.lines().map(str::to_string).collect::>(), + vec![ + "config --type=bool --get core.fsmonitor".to_string(), + "version --build-options".to_string(), + format!("-c {disabled_hooks} -c core.fsmonitor=false status --porcelain"), + "config --type=bool --get core.fsmonitor".to_string(), + "version --build-options".to_string(), + format!("-c {disabled_hooks} -c core.fsmonitor=true status --porcelain"), + ] + ); + } } From 0294cf23c0a03e9c340c13c80c24093d381a46e7 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Mon, 8 Jun 2026 05:30:58 -0700 Subject: [PATCH 3/7] [codex] Make fsmonitor tests hermetic The helper-suppression coverage invoked the ambient Git binary from a core integration test, while the capability coverage exercised the command wrapper directly. This split made the related behavior harder to review and discarded useful subprocess diagnostics. Move the helper case alongside the capability test in git-utils. Drive both through a fake Git executable whose stdout, stderr, status, and arguments are asserted exactly. Record the commands and upstream Git sources used to regenerate the reduced response fixtures. --- codex-rs/core/src/git_info_tests.rs | 40 -------- codex-rs/git-utils/src/info.rs | 148 +++++++++++++++++++++++++--- 2 files changed, 134 insertions(+), 54 deletions(-) diff --git a/codex-rs/core/src/git_info_tests.rs b/codex-rs/core/src/git_info_tests.rs index 24d68dd30daf..e172cd5f20fe 100644 --- a/codex-rs/core/src/git_info_tests.rs +++ b/codex-rs/core/src/git_info_tests.rs @@ -340,46 +340,6 @@ async fn test_get_has_changes_with_untracked_change_returns_true() { assert_eq!(get_has_changes(&repo_path).await, Some(true)); } -#[cfg(unix)] -#[tokio::test] -async fn test_get_has_changes_does_not_execute_repo_fsmonitor_helper() { - let temp_dir = TempDir::new().expect("Failed to create temp dir"); - let repo_path = create_test_git_repo(&temp_dir).await; - let helper_path = repo_path.join("fsmonitor-helper.sh"); - let marker_path = repo_path.join("fsmonitor-ran"); - - fs::write( - &helper_path, - format!( - "#!/bin/sh\nprintf ran > \"{}\"\n", - marker_path.to_string_lossy() - ), - ) - .expect("write fsmonitor helper"); - let mut permissions = fs::metadata(&helper_path) - .expect("read fsmonitor helper metadata") - .permissions(); - permissions.set_mode(0o755); - fs::set_permissions(&helper_path, permissions).expect("mark fsmonitor helper executable"); - - Command::new("git") - .args([ - "config", - "core.fsmonitor", - helper_path.to_string_lossy().as_ref(), - ]) - .current_dir(&repo_path) - .output() - .await - .expect("configure fsmonitor helper"); - - assert_eq!(get_has_changes(&repo_path).await, Some(true)); - assert!( - !marker_path.exists(), - "metadata collection should not invoke repository fsmonitor helpers" - ); -} - #[cfg(unix)] #[tokio::test] async fn test_get_has_changes_ignores_configured_hooks_path() { diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index 6d2773e5b48c..b44984d646af 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -959,19 +959,23 @@ mod tests { } #[cfg(unix)] - #[tokio::test] - async fn fsmonitor_override_requires_builtin_daemon_support() { - let temp_dir = tempfile::tempdir().expect("create temp dir"); + fn write_fake_git(temp_dir: &tempfile::TempDir) -> std::path::PathBuf { let git = temp_dir.path().join("git"); - let supported = temp_dir.path().join("git.supported"); - let log = temp_dir.path().join("git.log"); std::fs::write( &git, "#!/bin/sh\n\ printf '%s\\n' \"$*\" >>\"$0.log\"\n\ case \"$1\" in\n\ - config) printf 'true\\n' ;;\n\ - version) test ! -e \"$0.supported\" || printf 'feature: fsmonitor--daemon\\n' ;;\n\ + config)\n\ + cat \"$0.config.stdout\"\n\ + cat \"$0.config.stderr\" >&2\n\ + exit \"$(cat \"$0.config.status\")\"\n\ + ;;\n\ + version) cat \"$0.build-options\" ;;\n\ + *)\n\ + printf 'worktree output\\n'\n\ + printf 'worktree diagnostics\\n' >&2\n\ + ;;\n\ esac\n", ) .expect("write fake Git"); @@ -981,13 +985,129 @@ mod tests { permissions.set_mode(0o755); std::fs::set_permissions(&git, permissions).expect("mark fake Git executable"); - run_git_command_with_timeout_from(&git, &["status", "--porcelain"], temp_dir.path()) - .await - .expect("run unsupported fake Git"); - std::fs::write(&supported, "").expect("enable built-in fsmonitor feature"); - run_git_command_with_timeout_from(&git, &["status", "--porcelain"], temp_dir.path()) - .await - .expect("run supported fake Git"); + git + } + + #[cfg(unix)] + #[tokio::test] + async fn fsmonitor_override_rejects_configured_helper() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let git = write_fake_git(&temp_dir); + let config_stdout = temp_dir.path().join("git.config.stdout"); + let config_stderr = temp_dir.path().join("git.config.stderr"); + let config_status = temp_dir.path().join("git.config.status"); + let log = temp_dir.path().join("git.log"); + + // Regenerate these reduced fixtures with: + // + // git -c core.fsmonitor=/tmp/fsmonitor-helper \ + // config --type=bool --get core.fsmonitor \ + // >git.config.stdout 2>git.config.stderr + // printf '%s\n' "$?" >git.config.status + // + // `--type=bool` rejects values that name fsmonitor helpers: + // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/builtin/config.c#L264-L280 + // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/config.c#L1263-L1268 + std::fs::write(&config_stdout, "").expect("write helper config stdout"); + std::fs::write( + &config_stderr, + "fatal: bad boolean config value '/tmp/fsmonitor-helper' for 'core.fsmonitor'\n", + ) + .expect("write helper config stderr"); + std::fs::write(&config_status, "128\n").expect("write helper config status"); + + let helper_output = + run_git_command_with_timeout_from(&git, &["status", "--porcelain"], temp_dir.path()) + .await + .expect("run fake Git with helper config"); + assert_eq!( + ( + helper_output.status.code(), + helper_output.stdout, + helper_output.stderr, + ), + ( + Some(0), + b"worktree output\n".to_vec(), + b"worktree diagnostics\n".to_vec(), + ) + ); + + let actual = std::fs::read_to_string(log).expect("read fake Git log"); + let disabled_hooks = format!("core.hooksPath={DISABLED_HOOKS_PATH}"); + assert_eq!( + actual.lines().map(str::to_string).collect::>(), + vec![ + "config --type=bool --get core.fsmonitor".to_string(), + format!("-c {disabled_hooks} -c core.fsmonitor=false status --porcelain"), + ] + ); + } + + #[cfg(unix)] + #[tokio::test] + async fn fsmonitor_override_requires_builtin_daemon_support() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let git = write_fake_git(&temp_dir); + let config_stdout = temp_dir.path().join("git.config.stdout"); + let config_stderr = temp_dir.path().join("git.config.stderr"); + let config_status = temp_dir.path().join("git.config.status"); + let build_options = temp_dir.path().join("git.build-options"); + let log = temp_dir.path().join("git.log"); + + // Regenerate these reduced fixtures with: + // + // git -c core.fsmonitor=true \ + // config --type=bool --get core.fsmonitor \ + // >git.config.stdout 2>git.config.stderr + // printf '%s\n' "$?" >git.config.status + // /path/to/git-without-fsmonitor-daemon version --build-options | + // sed -n '/^feature: fsmonitor--daemon$/p' >git.build-options + // /path/to/git-with-fsmonitor-daemon version --build-options | + // sed -n '/^feature: fsmonitor--daemon$/p' >git.build-options + // + // Git added the feature line specifically for capability tests: + // https://github.com/git/git/commit/dd77cf61a1a2fbf52c94d0cd986d555ad2ba8a4b + std::fs::write(&config_stdout, "true\n").expect("write true config stdout"); + std::fs::write(&config_stderr, "").expect("write empty config stderr"); + std::fs::write(&config_status, "0\n").expect("write successful config status"); + std::fs::write(&build_options, "").expect("write unsupported build options"); + + let unsupported_output = + run_git_command_with_timeout_from(&git, &["status", "--porcelain"], temp_dir.path()) + .await + .expect("run unsupported fake Git"); + assert_eq!( + ( + unsupported_output.status.code(), + unsupported_output.stdout, + unsupported_output.stderr, + ), + ( + Some(0), + b"worktree output\n".to_vec(), + b"worktree diagnostics\n".to_vec(), + ) + ); + + std::fs::write(&build_options, "feature: fsmonitor--daemon\n") + .expect("write supported build options"); + let supported_output = + run_git_command_with_timeout_from(&git, &["status", "--porcelain"], temp_dir.path()) + .await + .expect("run supported fake Git"); + assert_eq!( + ( + supported_output.status.code(), + supported_output.stdout, + supported_output.stderr, + ), + ( + Some(0), + b"worktree output\n".to_vec(), + b"worktree diagnostics\n".to_vec(), + ) + ); let actual = std::fs::read_to_string(log).expect("read fake Git log"); let disabled_hooks = format!("core.hooksPath={DISABLED_HOOKS_PATH}"); From f0dba2abb15768c83f2ee46976d95b0328cb409e Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Mon, 8 Jun 2026 14:24:30 -0700 Subject: [PATCH 4/7] [codex] Preserve fsmonitor in TUI diffs The git-utils wrapper now preserves built-in fsmonitor, but TUI /diff uses the workspace command executor and continued forcing it off for its tracked diff and untracked-file scan. Share the config and capability output checks while leaving process execution with each caller. Probe once per /diff and reuse the result for tracked diff and ls-files. Keep no-index diffs disabled because they do not inspect the index and cannot benefit from fsmonitor. Also query core.fsmonitor without typed conversion. Git converts every matching value before --get selects the last, so a shadowed global helper could mask a repository-local true. Validate only the raw effective value and cover that layered configuration. --- codex-rs/git-utils/src/fsmonitor.rs | 13 ++ codex-rs/git-utils/src/info.rs | 135 +++++++++--- codex-rs/git-utils/src/lib.rs | 3 + codex-rs/tui/src/get_git_diff.rs | 312 ++++++++++++++++++++++++++-- 4 files changed, 425 insertions(+), 38 deletions(-) create mode 100644 codex-rs/git-utils/src/fsmonitor.rs diff --git a/codex-rs/git-utils/src/fsmonitor.rs b/codex-rs/git-utils/src/fsmonitor.rs new file mode 100644 index 000000000000..219f530c9d7d --- /dev/null +++ b/codex-rs/git-utils/src/fsmonitor.rs @@ -0,0 +1,13 @@ +/// Returns whether `git config --null --get core.fsmonitor` reported canonical +/// boolean `true`. +pub fn is_canonical_fsmonitor_true(output: &[u8]) -> bool { + output == b"true\0" +} + +/// Returns whether `git version --build-options` advertises the built-in +/// fsmonitor daemon. +pub fn supports_builtin_fsmonitor(output: &[u8]) -> bool { + output + .split(|byte| *byte == b'\n') + .any(|line| line.trim_ascii() == b"feature: fsmonitor--daemon") +} diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index b44984d646af..61e1cf7eeb48 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -416,7 +416,11 @@ async fn run_git_command_with_timeout_from( probe .env("GIT_OPTIONAL_LOCKS", "0") .env("LC_ALL", "C") - .args(["config", "--type=bool", "--get", "core.fsmonitor"]) + // A typed query formats shadowed values before selecting the + // effective one, so validate only the raw final value below. + // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/builtin/config.c#L482-L514 + // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/builtin/config.c#L611-L614 + .args(["config", "--null", "--get", "core.fsmonitor"]) .current_dir(cwd) .kill_on_drop(true); let configured = match timeout_at(deadline, probe.output()).await { @@ -425,13 +429,12 @@ async fn run_git_command_with_timeout_from( }; match configured { None => false, - Some(output) => match (output.status.code(), output.stdout.as_slice().trim_ascii()) { + Some(output) => match (output.status.code(), output.stdout.as_slice()) { // `git config --get` returns 1 when the key is absent. // https://git-scm.com/docs/git-config#Documentation/git-config.txt-get - (Some(1), _) | (Some(0), b"false") => false, - (Some(0), b"true") => true, - // `--type=bool` rejects hook pathnames. Keep helpers and any - // malformed or unreadable configuration disabled. + (Some(0), value) if crate::is_canonical_fsmonitor_true(value) => true, + // Keep false, unset values, helpers, malformed values, and + // unreadable configuration disabled. _ => false, }, } @@ -450,10 +453,9 @@ async fn run_git_command_with_timeout_from( .current_dir(cwd) .kill_on_drop(true); match timeout_at(deadline, probe.output()).await { - Ok(Ok(output)) if output.status.success() => output - .stdout - .split(|byte| *byte == b'\n') - .any(|line| line.trim_ascii() == b"feature: fsmonitor--daemon"), + Ok(Ok(output)) if output.status.success() => { + crate::supports_builtin_fsmonitor(&output.stdout) + } _ => false, } }; @@ -1001,20 +1003,14 @@ mod tests { // Regenerate these reduced fixtures with: // // git -c core.fsmonitor=/tmp/fsmonitor-helper \ - // config --type=bool --get core.fsmonitor \ + // config --null --get core.fsmonitor \ // >git.config.stdout 2>git.config.stderr // printf '%s\n' "$?" >git.config.status // - // `--type=bool` rejects values that name fsmonitor helpers: - // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/builtin/config.c#L264-L280 - // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/config.c#L1263-L1268 - std::fs::write(&config_stdout, "").expect("write helper config stdout"); - std::fs::write( - &config_stderr, - "fatal: bad boolean config value '/tmp/fsmonitor-helper' for 'core.fsmonitor'\n", - ) - .expect("write helper config stderr"); - std::fs::write(&config_status, "128\n").expect("write helper config status"); + std::fs::write(&config_stdout, b"/tmp/fsmonitor-helper\0") + .expect("write helper config stdout"); + std::fs::write(&config_stderr, "").expect("write helper config stderr"); + std::fs::write(&config_status, "0\n").expect("write helper config status"); let helper_output = run_git_command_with_timeout_from(&git, &["status", "--porcelain"], temp_dir.path()) @@ -1038,12 +1034,99 @@ mod tests { assert_eq!( actual.lines().map(str::to_string).collect::>(), vec![ - "config --type=bool --get core.fsmonitor".to_string(), + "config --null --get core.fsmonitor".to_string(), format!("-c {disabled_hooks} -c core.fsmonitor=false status --porcelain"), ] ); } + #[cfg(unix)] + #[tokio::test] + async fn fsmonitor_override_uses_effective_layered_config_value() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let repo = temp_dir.path().join("repo"); + std::fs::create_dir(&repo).expect("create repository directory"); + let init_status = std::process::Command::new("git") + .args(["init", "-q"]) + .current_dir(&repo) + .status() + .expect("initialize test repository"); + assert_eq!(init_status.code(), Some(0), "initialize test repository"); + + let git = temp_dir.path().join("git"); + let global_config = temp_dir.path().join("git.global"); + let log = temp_dir.path().join("git.log"); + std::fs::write( + &git, + "#!/bin/sh\n\ + printf '%s\\n' \"$*\" >>\"$0.log\"\n\ + case \"$1\" in\n\ + config)\n\ + GIT_CONFIG_NOSYSTEM=1 GIT_CONFIG_GLOBAL=\"$0.global\" exec git \"$@\"\n\ + ;;\n\ + version) printf 'feature: fsmonitor--daemon\\n' ;;\n\ + *) printf 'worktree output\\n' ;;\n\ + esac\n", + ) + .expect("write layered-config Git"); + let mut permissions = std::fs::metadata(&git) + .expect("read layered-config Git metadata") + .permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(&git, permissions).expect("mark layered-config Git executable"); + + let global_status = std::process::Command::new("git") + .args([ + "config", + "--file", + global_config.to_str().expect("global config path"), + "core.fsmonitor", + "/tmp/fsmonitor-helper", + ]) + .status() + .expect("write global fsmonitor helper"); + assert_eq!( + global_status.code(), + Some(0), + "write global fsmonitor helper" + ); + let local_status = std::process::Command::new("git") + .args(["config", "core.fsmonitor", "true"]) + .current_dir(&repo) + .status() + .expect("write local built-in fsmonitor config"); + assert_eq!( + local_status.code(), + Some(0), + "write local built-in fsmonitor config" + ); + + // A typed query formats every matching value before `--get` selects + // the last one, so the shadowed helper would make it fail. Query the + // raw effective value and validate that value instead. + // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/builtin/config.c#L482-L514 + // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/builtin/config.c#L611-L614 + let output = + run_git_command_with_timeout_from(&git, &["status", "--porcelain"], repo.as_path()) + .await + .expect("run Git with layered config"); + assert_eq!( + (output.status.code(), output.stdout), + (Some(0), b"worktree output\n".to_vec()) + ); + + let actual = std::fs::read_to_string(log).expect("read layered-config Git log"); + let disabled_hooks = format!("core.hooksPath={DISABLED_HOOKS_PATH}"); + assert_eq!( + actual.lines().map(str::to_string).collect::>(), + vec![ + "config --null --get core.fsmonitor".to_string(), + "version --build-options".to_string(), + format!("-c {disabled_hooks} -c core.fsmonitor=true status --porcelain"), + ] + ); + } + #[cfg(unix)] #[tokio::test] async fn fsmonitor_override_requires_builtin_daemon_support() { @@ -1058,7 +1141,7 @@ mod tests { // Regenerate these reduced fixtures with: // // git -c core.fsmonitor=true \ - // config --type=bool --get core.fsmonitor \ + // config --null --get core.fsmonitor \ // >git.config.stdout 2>git.config.stderr // printf '%s\n' "$?" >git.config.status // /path/to/git-without-fsmonitor-daemon version --build-options | @@ -1068,7 +1151,7 @@ mod tests { // // Git added the feature line specifically for capability tests: // https://github.com/git/git/commit/dd77cf61a1a2fbf52c94d0cd986d555ad2ba8a4b - std::fs::write(&config_stdout, "true\n").expect("write true config stdout"); + std::fs::write(&config_stdout, b"true\0").expect("write true config stdout"); std::fs::write(&config_stderr, "").expect("write empty config stderr"); std::fs::write(&config_status, "0\n").expect("write successful config status"); std::fs::write(&build_options, "").expect("write unsupported build options"); @@ -1114,10 +1197,10 @@ mod tests { assert_eq!( actual.lines().map(str::to_string).collect::>(), vec![ - "config --type=bool --get core.fsmonitor".to_string(), + "config --null --get core.fsmonitor".to_string(), "version --build-options".to_string(), format!("-c {disabled_hooks} -c core.fsmonitor=false status --porcelain"), - "config --type=bool --get core.fsmonitor".to_string(), + "config --null --get core.fsmonitor".to_string(), "version --build-options".to_string(), format!("-c {disabled_hooks} -c core.fsmonitor=true status --porcelain"), ] diff --git a/codex-rs/git-utils/src/lib.rs b/codex-rs/git-utils/src/lib.rs index 986a7434d2c5..9935b833638f 100644 --- a/codex-rs/git-utils/src/lib.rs +++ b/codex-rs/git-utils/src/lib.rs @@ -2,6 +2,7 @@ mod apply; mod baseline; mod branch; mod errors; +mod fsmonitor; mod info; mod operations; mod platform; @@ -21,6 +22,8 @@ pub use baseline::reset_git_repository; pub use branch::merge_base_with_head; pub use codex_protocol::protocol::GitSha; pub use errors::GitToolingError; +pub use fsmonitor::is_canonical_fsmonitor_true; +pub use fsmonitor::supports_builtin_fsmonitor; pub use info::CommitLogEntry; pub use info::GitDiffToRemote; pub use info::GitInfo; diff --git a/codex-rs/tui/src/get_git_diff.rs b/codex-rs/tui/src/get_git_diff.rs index 7f5545070784..cf3d0ba80b9d 100644 --- a/codex-rs/tui/src/get_git_diff.rs +++ b/codex-rs/tui/src/get_git_diff.rs @@ -11,6 +11,8 @@ use std::time::Duration; use crate::workspace_command::WorkspaceCommand; use crate::workspace_command::WorkspaceCommandExecutor; use crate::workspace_command::WorkspaceCommandOutput; +use codex_git_utils::is_canonical_fsmonitor_true; +use codex_git_utils::supports_builtin_fsmonitor; const DIFF_COMMAND_TIMEOUT: Duration = Duration::from_secs(/*secs*/ 30); const DISABLE_FSMONITOR_CONFIG: &str = "core.fsmonitor=false"; @@ -21,6 +23,12 @@ const DISABLE_HOOKS_CONFIG: &str = if cfg!(windows) { }; const EXECUTABLE_FILTER_CONFIG_PATTERN: &str = r"^filter\..*\.(clean|process)$"; +#[derive(Clone, Copy)] +enum FsmonitorOverride { + Disabled, + BuiltIn, +} + /// Return value of [`get_git_diff`]. /// /// * `bool` – Whether the current working directory is inside a Git repo. @@ -34,12 +42,34 @@ pub(crate) async fn get_git_diff( return Ok((false, String::new())); } + let config_output = run_git_probe( + runner, + cwd, + &["config", "--null", "--get", "core.fsmonitor"], + ) + .await; + let fsmonitor = if config_output.is_ok_and(|output| { + output.success() && is_canonical_fsmonitor_true(output.stdout.as_bytes()) + }) { + match run_git_probe(runner, cwd, &["version", "--build-options"]).await { + Ok(output) + if output.success() && supports_builtin_fsmonitor(output.stdout.as_bytes()) => + { + FsmonitorOverride::BuiltIn + } + _ => FsmonitorOverride::Disabled, + } + } else { + FsmonitorOverride::Disabled + }; + // 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, + fsmonitor, &diff_config_overrides, &[ "diff", @@ -50,7 +80,12 @@ pub(crate) async fn get_git_diff( "--color", ] ), - run_git_capture_stdout(runner, cwd, &["ls-files", "--others", "--exclude-standard"]), + run_git_capture_stdout( + runner, + cwd, + fsmonitor, + &["ls-files", "--others", "--exclude-standard"] + ), ); let tracked_diff = tracked_diff_res?; let untracked_output = untracked_output_res?; @@ -80,7 +115,14 @@ pub(crate) async fn get_git_diff( null_path, file, ]; - let diff = run_git_capture_diff(runner, cwd, &diff_config_overrides, &args).await?; + let diff = run_git_capture_diff( + runner, + cwd, + FsmonitorOverride::Disabled, + &diff_config_overrides, + &args, + ) + .await?; untracked_diff.push_str(&diff); } @@ -92,9 +134,10 @@ pub(crate) async fn get_git_diff( async fn run_git_capture_stdout( runner: &dyn WorkspaceCommandExecutor, cwd: &Path, + fsmonitor: FsmonitorOverride, args: &[&str], ) -> Result { - let output = run_git_command(runner, cwd, &[], args).await?; + let output = run_git_command(runner, cwd, fsmonitor, &[], args).await?; if output.success() { Ok(output.stdout) } else { @@ -110,10 +153,11 @@ async fn run_git_capture_stdout( async fn run_git_capture_diff( runner: &dyn WorkspaceCommandExecutor, cwd: &Path, + fsmonitor: FsmonitorOverride, config_overrides: &[(String, String)], args: &[&str], ) -> Result { - let output = run_git_command(runner, cwd, config_overrides, args).await?; + let output = run_git_command(runner, cwd, fsmonitor, config_overrides, args).await?; if output.success() || output.exit_code == 1 { Ok(output.stdout) } else { @@ -137,7 +181,7 @@ async fn diff_filter_config_overrides( "--get-regexp", EXECUTABLE_FILTER_CONFIG_PATTERN, ]; - let output = run_git_command(runner, cwd, &[], &args).await?; + let output = run_git_command(runner, cwd, FsmonitorOverride::Disabled, &[], &args).await?; if output.exit_code != 0 && output.exit_code != 1 { return Err(format!( "git {:?} failed with status {}", @@ -174,21 +218,52 @@ 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, + FsmonitorOverride::Disabled, + &[], + &["rev-parse", "--is-inside-work-tree"], + ) + .await?; Ok(output.success()) } +async fn run_git_probe( + runner: &dyn WorkspaceCommandExecutor, + cwd: &Path, + args: &[&str], +) -> Result { + let mut argv = Vec::with_capacity(args.len() + 3); + argv.extend([ + "git".to_string(), + "-c".to_string(), + DISABLE_HOOKS_CONFIG.to_string(), + ]); + argv.extend(args.iter().map(|arg| (*arg).to_string())); + let command = WorkspaceCommand::new(argv) + .cwd(cwd.to_path_buf()) + .env("GIT_OPTIONAL_LOCKS", "0") + .env("LC_ALL", "C"); + runner.run(command).await.map_err(|err| err.to_string()) +} + async fn run_git_command( runner: &dyn WorkspaceCommandExecutor, cwd: &Path, + fsmonitor: FsmonitorOverride, config_overrides: &[(String, String)], args: &[&str], ) -> Result { + let fsmonitor_config = match fsmonitor { + FsmonitorOverride::Disabled => DISABLE_FSMONITOR_CONFIG, + FsmonitorOverride::BuiltIn => "core.fsmonitor=true", + }; let mut argv = Vec::with_capacity(args.len() + 5); argv.push("git".to_string()); argv.extend([ "-c".to_string(), - DISABLE_FSMONITOR_CONFIG.to_string(), + fsmonitor_config.to_string(), "-c".to_string(), DISABLE_HOOKS_CONFIG.to_string(), ]); @@ -254,6 +329,11 @@ mod tests { /*exit_code*/ 0, "true\n", ), + response( + git_probe_command(&["config", "--null", "--get", "core.fsmonitor"]), + /*exit_code*/ 0, + "/tmp/fsmonitor-helper\0", + ), response( git_command(&[ "config", @@ -308,6 +388,7 @@ mod tests { &commands, &[ git_command(&["rev-parse", "--is-inside-work-tree"]), + git_probe_command(&["config", "--null", "--get", "core.fsmonitor"]), git_command(&[ "config", "--null", @@ -339,8 +420,181 @@ mod tests { ], &cwd, ); - assert_eq!(commands[2].env, filter_override_env("filter.evil")); - assert_eq!(commands[4].env, filter_override_env("filter.evil")); + assert_eq!(commands[3].env, filter_override_env("filter.evil")); + assert_eq!(commands[5].env, filter_override_env("filter.evil")); + } + + #[tokio::test] + async fn get_git_diff_preserves_builtin_fsmonitor_for_worktree_reads() { + let cwd = PathBuf::from("/workspace"); + let runner = FakeRunner::new(vec![ + response( + git_command(&["rev-parse", "--is-inside-work-tree"]), + /*exit_code*/ 0, + "true\n", + ), + response( + git_probe_command(&["config", "--null", "--get", "core.fsmonitor"]), + /*exit_code*/ 0, + "true\0", + ), + response( + git_probe_command(&["version", "--build-options"]), + /*exit_code*/ 0, + "feature: fsmonitor--daemon\n", + ), + response( + git_command(&[ + "config", + "--null", + "--name-only", + "--get-regexp", + EXECUTABLE_FILTER_CONFIG_PATTERN, + ]), + /*exit_code*/ 1, + "", + ), + response( + git_command_with_fsmonitor( + FsmonitorOverride::BuiltIn, + &[ + "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", + "--color", + ], + ), + /*exit_code*/ 1, + "tracked\n", + ), + response( + git_command_with_fsmonitor( + FsmonitorOverride::BuiltIn, + &["ls-files", "--others", "--exclude-standard"], + ), + /*exit_code*/ 0, + "new.txt\n", + ), + response( + 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", + ), + ]); + + let result = get_git_diff(&runner, &cwd).await; + + assert_eq!(result, Ok((true, "tracked\nuntracked\n".to_string()))); + assert_commands( + &runner.commands(), + &[ + git_command(&["rev-parse", "--is-inside-work-tree"]), + git_probe_command(&["config", "--null", "--get", "core.fsmonitor"]), + git_probe_command(&["version", "--build-options"]), + git_command(&[ + "config", + "--null", + "--name-only", + "--get-regexp", + EXECUTABLE_FILTER_CONFIG_PATTERN, + ]), + git_command_with_fsmonitor( + FsmonitorOverride::BuiltIn, + &[ + "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", + "--color", + ], + ), + git_command_with_fsmonitor( + FsmonitorOverride::BuiltIn, + &["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, + ); + } + + #[tokio::test] + async fn get_git_diff_disables_fsmonitor_without_builtin_daemon_support() { + let cwd = PathBuf::from("/workspace"); + let runner = FakeRunner::new(vec![ + response( + git_command(&["rev-parse", "--is-inside-work-tree"]), + /*exit_code*/ 0, + "true\n", + ), + response( + git_probe_command(&["config", "--null", "--get", "core.fsmonitor"]), + /*exit_code*/ 0, + "true\0", + ), + response( + git_probe_command(&["version", "--build-options"]), + /*exit_code*/ 0, + "", + ), + response( + 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*/ 0, + "", + ), + response( + git_command(&["ls-files", "--others", "--exclude-standard"]), + /*exit_code*/ 0, + "", + ), + ]); + + let result = get_git_diff(&runner, &cwd).await; + + assert_eq!(result, Ok((true, String::new()))); } #[tokio::test] @@ -352,6 +606,11 @@ mod tests { /*exit_code*/ 0, "true\n", ), + response( + git_probe_command(&["config", "--null", "--get", "core.fsmonitor"]), + /*exit_code*/ 1, + "", + ), response( git_command(&[ "config", @@ -396,6 +655,11 @@ mod tests { /*exit_code*/ 0, "true\n", ), + response( + git_probe_command(&["config", "--null", "--get", "core.fsmonitor"]), + /*exit_code*/ 1, + "", + ), response( git_command(&[ "config", @@ -570,10 +834,28 @@ mod tests { } fn git_command(args: &[&str]) -> Vec { + git_command_with_fsmonitor(FsmonitorOverride::Disabled, args) + } + + fn git_command_with_fsmonitor(fsmonitor: FsmonitorOverride, args: &[&str]) -> Vec { + let fsmonitor_config = match fsmonitor { + FsmonitorOverride::Disabled => DISABLE_FSMONITOR_CONFIG, + FsmonitorOverride::BuiltIn => "core.fsmonitor=true", + }; let mut argv = vec![ "git".to_string(), "-c".to_string(), - DISABLE_FSMONITOR_CONFIG.to_string(), + fsmonitor_config.to_string(), + "-c".to_string(), + DISABLE_HOOKS_CONFIG.to_string(), + ]; + argv.extend(args.iter().map(|arg| (*arg).to_string())); + argv + } + + fn git_probe_command(args: &[&str]) -> Vec { + let mut argv = vec![ + "git".to_string(), "-c".to_string(), DISABLE_HOOKS_CONFIG.to_string(), ]; @@ -647,8 +929,14 @@ mod tests { for command in commands { assert_eq!(command.cwd.as_deref(), Some(cwd)); - assert_eq!(command.timeout, DIFF_COMMAND_TIMEOUT); - assert!(command.disable_output_cap); + if command.argv.get(2).map(String::as_str) == Some(DISABLE_HOOKS_CONFIG) { + assert_eq!(command.timeout, Duration::from_secs(/*secs*/ 5)); + assert_eq!(command.output_bytes_cap, 64 * 1024); + assert_eq!(command.disable_output_cap, false); + } else { + assert_eq!(command.timeout, DIFF_COMMAND_TIMEOUT); + assert_eq!(command.disable_output_cap, true); + } } } From 89096fe6d91d9570ea8563dae1937147496f1ffa Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Mon, 8 Jun 2026 16:22:42 -0700 Subject: [PATCH 5/7] [codex] Centralize fsmonitor probe policy The local Git wrapper and TUI /diff use different command runners, but both need to reject configured fsmonitor helpers without discarding the built-in daemon's worktree-scan acceleration. Move config and capability interpretation into git-utils. Read the raw effective value before normalizing it so an invalid value from a lower configuration scope cannot poison the selected value. Accept every boolean form Git accepts, delegating uncommon forms back to Git with an exact-value query, and require the daemon capability before preserving the setting. Probe once at each worktree workflow boundary and reuse the result for all of its commands. Keep metadata-only commands disabled without guessing from argv, and bound each local probe and requested command independently with the existing command timeout. Keep probe execution in the local and workspace adapters. This preserves their existing process and transport behavior without introducing a generic command abstraction. --- codex-rs/git-utils/src/fsmonitor.rs | 132 ++++- codex-rs/git-utils/src/fsmonitor_tests.rs | 139 ++++++ codex-rs/git-utils/src/info.rs | 305 ++++-------- codex-rs/git-utils/src/lib.rs | 5 +- codex-rs/tui/src/get_git_diff.rs | 575 +++++++++------------- 5 files changed, 599 insertions(+), 557 deletions(-) create mode 100644 codex-rs/git-utils/src/fsmonitor_tests.rs diff --git a/codex-rs/git-utils/src/fsmonitor.rs b/codex-rs/git-utils/src/fsmonitor.rs index 219f530c9d7d..f7d7ec651d33 100644 --- a/codex-rs/git-utils/src/fsmonitor.rs +++ b/codex-rs/git-utils/src/fsmonitor.rs @@ -1,13 +1,129 @@ -/// Returns whether `git config --null --get core.fsmonitor` reported canonical -/// boolean `true`. -pub fn is_canonical_fsmonitor_true(output: &[u8]) -> bool { - output == b"true\0" +//! Policy for preserving Git's built-in filesystem monitor. +//! +//! Codex overrides `core.fsmonitor` so repository configuration cannot select +//! an executable helper. Preserve the built-in daemon only when the effective +//! value is boolean true and Git advertises daemon support. +//! +//! The daemon avoids scanning every tracked file and untracked directory: +//! https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/Documentation/git-fsmonitor--daemon.adoc#L49-L57 +//! https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/Documentation/git-update-index.adoc#L545-L550 + +use std::future::Future; + +const CONFIG_PROBE_ARGS: &[&str] = &["config", "--null", "--get", "core.fsmonitor"]; +const CAPABILITY_PROBE_ARGS: &[&str] = &["version", "--build-options"]; + +/// The safe `core.fsmonitor` override for an internal Git command. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum FsmonitorOverride { + /// Disable repository-selected filesystem monitor helpers. + Disabled, + /// Preserve Git's built-in filesystem monitor daemon. + BuiltIn, +} + +impl FsmonitorOverride { + /// Returns the complete Git configuration override. + pub const fn git_config_arg(self) -> &'static str { + match self { + Self::Disabled => "core.fsmonitor=false", + Self::BuiltIn => "core.fsmonitor=true", + } + } } -/// Returns whether `git version --build-options` advertises the built-in -/// fsmonitor daemon. -pub fn supports_builtin_fsmonitor(output: &[u8]) -> bool { - output +/// Executes the Git commands required by [`detect_fsmonitor_override`]. +/// +/// Implementations must return stdout only when Git exits successfully. +/// Timeouts, spawn or transport failures, signal termination, and nonzero exit +/// statuses must return `None`. +pub trait FsmonitorProbeRunner: Send { + /// Runs one bounded probe in the target repository. + fn run_probe(&mut self, args: &[&str]) -> impl Future>> + Send; +} + +/// Returns the safe filesystem monitor override for the target repository. +/// +/// This intentionally probes every time. Effective Git configuration is +/// layered, may use conditional includes, and can change while Codex is +/// running: +/// https://git-scm.com/docs/git-config#SCOPES +/// https://git-scm.com/docs/git-config#_conditional_includes +pub async fn detect_fsmonitor_override( + runner: &mut impl FsmonitorProbeRunner, +) -> FsmonitorOverride { + // A typed query converts every matching value before `--get` selects the + // effective one. A shadowed helper path can therefore make a repository- + // local true fail conversion. Query the raw effective value first. + // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/builtin/config.c#L482-L514 + // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/builtin/config.c#L611-L614 + let Some(config) = runner.run_probe(CONFIG_PROBE_ARGS).await else { + return FsmonitorOverride::Disabled; + }; + let Some(config) = config.strip_suffix(b"\0") else { + return FsmonitorOverride::Disabled; + }; + if config.contains(&0) { + return FsmonitorOverride::Disabled; + } + let Ok(config) = str::from_utf8(config) else { + return FsmonitorOverride::Disabled; + }; + + // Git accepts these case-insensitive spellings directly, as well as + // valueless keys and nonzero integers. Ask Git to normalize uncommon + // spellings, filtering by the raw effective value before conversion so a + // shadowed helper pathname cannot make the query fail. + // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/parse.c#L158-L181 + // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/builtin/config.c#L264-L279 + // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/builtin/config.c#L496-L507 + let configured = if ["true", "yes", "on"] + .iter() + .any(|value| config.eq_ignore_ascii_case(value)) + { + true + } else if ["false", "no", "off"] + .iter() + .any(|value| config.eq_ignore_ascii_case(value)) + { + false + } else { + let typed_args = [ + "config", + "--null", + "--type=bool", + "--fixed-value", + "--get", + "core.fsmonitor", + config, + ]; + matches!( + runner.run_probe(&typed_args).await.as_deref(), + Some(b"true\0") + ) + }; + if !configured { + return FsmonitorOverride::Disabled; + } + + // Git 2.35.1 and older interpret "true" as a hook pathname. Before Git + // 2.26, a successful empty hook response can hide tracked changes. Require + // the feature line Git added specifically for capability checks. + // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/Documentation/config/core.adoc#L90-L99 + // https://github.com/git/git/commit/dd77cf61a1a2fbf52c94d0cd986d555ad2ba8a4b + let Some(build_options) = runner.run_probe(CAPABILITY_PROBE_ARGS).await else { + return FsmonitorOverride::Disabled; + }; + if build_options .split(|byte| *byte == b'\n') .any(|line| line.trim_ascii() == b"feature: fsmonitor--daemon") + { + FsmonitorOverride::BuiltIn + } else { + FsmonitorOverride::Disabled + } } + +#[cfg(test)] +#[path = "fsmonitor_tests.rs"] +mod tests; diff --git a/codex-rs/git-utils/src/fsmonitor_tests.rs b/codex-rs/git-utils/src/fsmonitor_tests.rs new file mode 100644 index 000000000000..e86139421caa --- /dev/null +++ b/codex-rs/git-utils/src/fsmonitor_tests.rs @@ -0,0 +1,139 @@ +use std::collections::VecDeque; +use std::future::Future; + +use pretty_assertions::assert_eq; + +use super::FsmonitorOverride; +use super::FsmonitorProbeRunner; +use super::detect_fsmonitor_override; + +struct ProbeResponse { + args: Vec<&'static str>, + output: Option>, +} + +struct FakeRunner { + responses: VecDeque, +} + +impl FsmonitorProbeRunner for FakeRunner { + fn run_probe(&mut self, args: &[&str]) -> impl Future>> + Send { + let response = self.responses.pop_front().expect("missing probe response"); + assert_eq!(args, response.args); + std::future::ready(response.output) + } +} + +#[tokio::test] +async fn detects_supported_builtin_fsmonitor_values() { + let cases = [ + ( + "missing config", + vec![response(config_args(), /*output*/ None)], + FsmonitorOverride::Disabled, + ), + ( + "helper path", + vec![ + response(config_args(), Some(b"/tmp/fsmonitor-helper\0")), + response( + typed_config_args("/tmp/fsmonitor-helper"), + /*output*/ None, + ), + ], + FsmonitorOverride::Disabled, + ), + ( + "false spelling", + vec![response(config_args(), Some(b"OFF\0"))], + FsmonitorOverride::Disabled, + ), + ( + "unsupported Git", + vec![ + response(config_args(), Some(b"yes\0")), + response(capability_args(), Some(b"")), + ], + FsmonitorOverride::Disabled, + ), + ( + "common true spelling", + vec![ + response(config_args(), Some(b"On\0")), + response(capability_args(), Some(fsmonitor_capability())), + ], + FsmonitorOverride::BuiltIn, + ), + ( + "numeric true", + vec![ + response(config_args(), Some(b"2k\0")), + response(typed_config_args("2k"), Some(b"true\0")), + response(capability_args(), Some(fsmonitor_capability())), + ], + FsmonitorOverride::BuiltIn, + ), + ( + "valueless true", + vec![ + response(config_args(), Some(b"\0")), + response(typed_config_args(""), Some(b"true\0")), + response(capability_args(), Some(fsmonitor_capability())), + ], + FsmonitorOverride::BuiltIn, + ), + ( + "explicit empty false", + vec![ + response(config_args(), Some(b"\0")), + response(typed_config_args(""), Some(b"false\0")), + ], + FsmonitorOverride::Disabled, + ), + ]; + + for (name, responses, expected) in cases { + let mut runner = FakeRunner { + responses: responses.into(), + }; + + let actual = detect_fsmonitor_override(&mut runner).await; + + assert_eq!( + (actual, runner.responses.len()), + (expected, 0), + "case: {name}" + ); + } +} + +fn response(args: Vec<&'static str>, output: Option<&[u8]>) -> ProbeResponse { + ProbeResponse { + args, + output: output.map(<[u8]>::to_vec), + } +} + +fn config_args() -> Vec<&'static str> { + vec!["config", "--null", "--get", "core.fsmonitor"] +} + +fn typed_config_args(value: &'static str) -> Vec<&'static str> { + vec![ + "config", + "--null", + "--type=bool", + "--fixed-value", + "--get", + "core.fsmonitor", + value, + ] +} + +fn capability_args() -> Vec<&'static str> { + vec!["version", "--build-options"] +} + +fn fsmonitor_capability() -> &'static [u8] { + b"feature: fsmonitor--daemon\n" +} diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index 61e1cf7eeb48..84d5a83d58d5 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -12,8 +12,7 @@ use serde::Deserialize; use serde::Serialize; use tokio::process::Command; use tokio::time::Duration as TokioDuration; -use tokio::time::Instant as TokioInstant; -use tokio::time::timeout_at; +use tokio::time::timeout; use ts_rs::TS; use crate::GitSha; @@ -280,7 +279,10 @@ fn trim_git_suffix(value: &str) -> &str { } pub async fn get_has_changes(cwd: &Path) -> Option { - let output = run_git_command_with_timeout(&["status", "--porcelain"], cwd).await?; + let git = Path::new("git"); + let fsmonitor = detect_local_fsmonitor_override(git, cwd).await; + let output = + run_git_command_with_timeout_from(git, &["status", "--porcelain"], cwd, fsmonitor).await?; if !output.status.success() { return None; } @@ -391,86 +393,55 @@ pub async fn git_diff_to_remote(cwd: &Path) -> Option { /// Run a git command with a timeout to prevent blocking on large repositories async fn run_git_command_with_timeout(args: &[&str], cwd: &Path) -> Option { - run_git_command_with_timeout_from(Path::new("git"), args, cwd).await + // These callers only inspect repository metadata. Worktree workflows probe + // once and pass their override directly to the lower-level runner. + run_git_command_with_timeout_from( + Path::new("git"), + args, + cwd, + crate::FsmonitorOverride::Disabled, + ) + .await +} + +struct LocalFsmonitorProbeRunner<'a> { + git: &'a Path, + cwd: &'a Path, +} + +impl crate::FsmonitorProbeRunner for LocalFsmonitorProbeRunner<'_> { + async fn run_probe(&mut self, args: &[&str]) -> Option> { + let mut command = Command::new(self.git); + command.args(args).current_dir(self.cwd).kill_on_drop(true); + match timeout(GIT_COMMAND_TIMEOUT, command.output()).await { + Ok(Ok(output)) if output.status.success() => Some(output.stdout), + _ => None, + } + } +} + +async fn detect_local_fsmonitor_override(git: &Path, cwd: &Path) -> crate::FsmonitorOverride { + let mut runner = LocalFsmonitorProbeRunner { git, cwd }; + crate::detect_fsmonitor_override(&mut runner).await } async fn run_git_command_with_timeout_from( git: &Path, args: &[&str], cwd: &Path, + fsmonitor: crate::FsmonitorOverride, ) -> Option { - // Only commands that inspect the worktree can recover the cost of probing. - // Built-in fsmonitor avoids worktree and untracked-file scans: - // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/Documentation/git-fsmonitor--daemon.adoc#L49-L57 - // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/Documentation/git-update-index.adoc#L545-L550 - let benefits_from_fsmonitor = matches!(args.first(), Some(&"status")) - || matches!(args.first(), Some(&"ls-files")) - || (matches!(args.first(), Some(&"diff")) && !args.contains(&"--no-index")); - let deadline = TokioInstant::now() + GIT_COMMAND_TIMEOUT; - let fsmonitor_configured = benefits_from_fsmonitor && { - // Do not cache this decision. Effective Git config is layered, can use - // conditional includes, and can change while Codex is running. - // https://git-scm.com/docs/git-config#SCOPES - // https://git-scm.com/docs/git-config#_conditional_includes - let mut probe = Command::new(git); - probe - .env("GIT_OPTIONAL_LOCKS", "0") - .env("LC_ALL", "C") - // A typed query formats shadowed values before selecting the - // effective one, so validate only the raw final value below. - // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/builtin/config.c#L482-L514 - // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/builtin/config.c#L611-L614 - .args(["config", "--null", "--get", "core.fsmonitor"]) - .current_dir(cwd) - .kill_on_drop(true); - let configured = match timeout_at(deadline, probe.output()).await { - Ok(Ok(output)) => Some(output), - _ => None, - }; - match configured { - None => false, - Some(output) => match (output.status.code(), output.stdout.as_slice()) { - // `git config --get` returns 1 when the key is absent. - // https://git-scm.com/docs/git-config#Documentation/git-config.txt-get - (Some(0), value) if crate::is_canonical_fsmonitor_true(value) => true, - // Keep false, unset values, helpers, malformed values, and - // unreadable configuration disabled. - _ => false, - }, - } - }; - let fsmonitor_enabled = fsmonitor_configured && { - // Git 2.35.1 and older interpret "true" as a hook pathname. Before - // Git 2.26, an empty successful hook response can hide tracked changes. - // Require the feature line Git added specifically for capability tests. - // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/Documentation/config/core.adoc#L90-L99 - // https://github.com/git/git/commit/dd77cf61a1a2fbf52c94d0cd986d555ad2ba8a4b - let mut probe = Command::new(git); - probe - .env("GIT_OPTIONAL_LOCKS", "0") - .env("LC_ALL", "C") - .args(["version", "--build-options"]) - .current_dir(cwd) - .kill_on_drop(true); - match timeout_at(deadline, probe.output()).await { - Ok(Ok(output)) if output.status.success() => { - crate::supports_builtin_fsmonitor(&output.stdout) - } - _ => false, - } - }; - let mut command = Command::new(git); command .env("GIT_OPTIONAL_LOCKS", "0") // Keep internal Git commands independent of repository-selected hooks // and fsmonitor helpers while preserving built-in fsmonitor acceleration. .args(["-c", &format!("core.hooksPath={DISABLED_HOOKS_PATH}")]) - .args(["-c", &format!("core.fsmonitor={fsmonitor_enabled}")]) + .args(["-c", fsmonitor.git_config_arg()]) .args(args) .current_dir(cwd) .kill_on_drop(true); - let result = timeout_at(deadline, command.output()).await; + let result = timeout(GIT_COMMAND_TIMEOUT, command.output()).await; match result { Ok(Ok(output)) => Some(output), @@ -755,9 +726,15 @@ async fn find_closest_sha(cwd: &Path, branches: &[String], remotes: &[String]) - } async fn diff_against_sha(cwd: &Path, sha: &GitSha) -> Option { - let output = - run_git_command_with_timeout(&["diff", "--no-textconv", "--no-ext-diff", &sha.0], cwd) - .await?; + let git = Path::new("git"); + let fsmonitor = detect_local_fsmonitor_override(git, cwd).await; + let output = run_git_command_with_timeout_from( + git, + &["diff", "--no-textconv", "--no-ext-diff", &sha.0], + cwd, + fsmonitor, + ) + .await?; // 0 is success and no diff. // 1 is success but there is a diff. let exit_ok = output.status.code().is_some_and(|c| c == 0 || c == 1); @@ -766,8 +743,13 @@ async fn diff_against_sha(cwd: &Path, sha: &GitSha) -> Option { } let mut diff = String::from_utf8(output.stdout).ok()?; - if let Some(untracked_output) = - run_git_command_with_timeout(&["ls-files", "--others", "--exclude-standard"], cwd).await + if let Some(untracked_output) = run_git_command_with_timeout_from( + git, + &["ls-files", "--others", "--exclude-standard"], + cwd, + fsmonitor, + ) + .await && untracked_output.status.success() { let untracked: Vec = String::from_utf8(untracked_output.stdout) @@ -793,7 +775,7 @@ async fn diff_against_sha(cwd: &Path, sha: &GitSha) -> Option { null_device, &file_owned, ]; - run_git_command_with_timeout(&args_vec, cwd).await + run_git_command_with_timeout_from(git, &args_vec, cwd, fsmonitor).await }); let results = join_all(futures_iter).await; for extra in results.into_iter().flatten() { @@ -961,23 +943,18 @@ mod tests { } #[cfg(unix)] - fn write_fake_git(temp_dir: &tempfile::TempDir) -> std::path::PathBuf { + #[tokio::test] + async fn fsmonitor_override_rejects_configured_helper() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); let git = temp_dir.path().join("git"); + let log = temp_dir.path().join("git.log"); std::fs::write( &git, "#!/bin/sh\n\ printf '%s\\n' \"$*\" >>\"$0.log\"\n\ case \"$1\" in\n\ - config)\n\ - cat \"$0.config.stdout\"\n\ - cat \"$0.config.stderr\" >&2\n\ - exit \"$(cat \"$0.config.status\")\"\n\ - ;;\n\ - version) cat \"$0.build-options\" ;;\n\ - *)\n\ - printf 'worktree output\\n'\n\ - printf 'worktree diagnostics\\n' >&2\n\ - ;;\n\ + config) printf '/tmp/fsmonitor-helper\\000' ;;\n\ + *) printf 'worktree output\\n' ;;\n\ esac\n", ) .expect("write fake Git"); @@ -987,54 +964,34 @@ mod tests { permissions.set_mode(0o755); std::fs::set_permissions(&git, permissions).expect("mark fake Git executable"); - git - } - - #[cfg(unix)] - #[tokio::test] - async fn fsmonitor_override_rejects_configured_helper() { - let temp_dir = tempfile::tempdir().expect("create temp dir"); - let git = write_fake_git(&temp_dir); - let config_stdout = temp_dir.path().join("git.config.stdout"); - let config_stderr = temp_dir.path().join("git.config.stderr"); - let config_status = temp_dir.path().join("git.config.status"); - let log = temp_dir.path().join("git.log"); + // The config response mirrors: + // git -c core.fsmonitor=/tmp/fsmonitor-helper \ + // config --null --get core.fsmonitor + let fsmonitor = detect_local_fsmonitor_override(&git, temp_dir.path()).await; + let output = run_git_command_with_timeout_from( + &git, + &["status", "--porcelain"], + temp_dir.path(), + fsmonitor, + ) + .await + .expect("run fake Git"); - // Regenerate these reduced fixtures with: - // - // git -c core.fsmonitor=/tmp/fsmonitor-helper \ - // config --null --get core.fsmonitor \ - // >git.config.stdout 2>git.config.stderr - // printf '%s\n' "$?" >git.config.status - // - std::fs::write(&config_stdout, b"/tmp/fsmonitor-helper\0") - .expect("write helper config stdout"); - std::fs::write(&config_stderr, "").expect("write helper config stderr"); - std::fs::write(&config_status, "0\n").expect("write helper config status"); - - let helper_output = - run_git_command_with_timeout_from(&git, &["status", "--porcelain"], temp_dir.path()) - .await - .expect("run fake Git with helper config"); assert_eq!( - ( - helper_output.status.code(), - helper_output.stdout, - helper_output.stderr, - ), - ( - Some(0), - b"worktree output\n".to_vec(), - b"worktree diagnostics\n".to_vec(), - ) + (output.status.code(), output.stdout), + (Some(0), b"worktree output\n".to_vec()) ); - - let actual = std::fs::read_to_string(log).expect("read fake Git log"); let disabled_hooks = format!("core.hooksPath={DISABLED_HOOKS_PATH}"); assert_eq!( - actual.lines().map(str::to_string).collect::>(), + std::fs::read_to_string(log) + .expect("read fake Git log") + .lines() + .map(str::to_string) + .collect::>(), vec![ "config --null --get core.fsmonitor".to_string(), + "config --null --type=bool --fixed-value --get core.fsmonitor /tmp/fsmonitor-helper" + .to_string(), format!("-c {disabled_hooks} -c core.fsmonitor=false status --porcelain"), ] ); @@ -1101,15 +1058,15 @@ mod tests { "write local built-in fsmonitor config" ); - // A typed query formats every matching value before `--get` selects - // the last one, so the shadowed helper would make it fail. Query the - // raw effective value and validate that value instead. - // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/builtin/config.c#L482-L514 - // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/builtin/config.c#L611-L614 - let output = - run_git_command_with_timeout_from(&git, &["status", "--porcelain"], repo.as_path()) - .await - .expect("run Git with layered config"); + let fsmonitor = detect_local_fsmonitor_override(&git, repo.as_path()).await; + let output = run_git_command_with_timeout_from( + &git, + &["status", "--porcelain"], + repo.as_path(), + fsmonitor, + ) + .await + .expect("run Git with layered config"); assert_eq!( (output.status.code(), output.stdout), (Some(0), b"worktree output\n".to_vec()) @@ -1126,84 +1083,4 @@ mod tests { ] ); } - - #[cfg(unix)] - #[tokio::test] - async fn fsmonitor_override_requires_builtin_daemon_support() { - let temp_dir = tempfile::tempdir().expect("create temp dir"); - let git = write_fake_git(&temp_dir); - let config_stdout = temp_dir.path().join("git.config.stdout"); - let config_stderr = temp_dir.path().join("git.config.stderr"); - let config_status = temp_dir.path().join("git.config.status"); - let build_options = temp_dir.path().join("git.build-options"); - let log = temp_dir.path().join("git.log"); - - // Regenerate these reduced fixtures with: - // - // git -c core.fsmonitor=true \ - // config --null --get core.fsmonitor \ - // >git.config.stdout 2>git.config.stderr - // printf '%s\n' "$?" >git.config.status - // /path/to/git-without-fsmonitor-daemon version --build-options | - // sed -n '/^feature: fsmonitor--daemon$/p' >git.build-options - // /path/to/git-with-fsmonitor-daemon version --build-options | - // sed -n '/^feature: fsmonitor--daemon$/p' >git.build-options - // - // Git added the feature line specifically for capability tests: - // https://github.com/git/git/commit/dd77cf61a1a2fbf52c94d0cd986d555ad2ba8a4b - std::fs::write(&config_stdout, b"true\0").expect("write true config stdout"); - std::fs::write(&config_stderr, "").expect("write empty config stderr"); - std::fs::write(&config_status, "0\n").expect("write successful config status"); - std::fs::write(&build_options, "").expect("write unsupported build options"); - - let unsupported_output = - run_git_command_with_timeout_from(&git, &["status", "--porcelain"], temp_dir.path()) - .await - .expect("run unsupported fake Git"); - assert_eq!( - ( - unsupported_output.status.code(), - unsupported_output.stdout, - unsupported_output.stderr, - ), - ( - Some(0), - b"worktree output\n".to_vec(), - b"worktree diagnostics\n".to_vec(), - ) - ); - - std::fs::write(&build_options, "feature: fsmonitor--daemon\n") - .expect("write supported build options"); - let supported_output = - run_git_command_with_timeout_from(&git, &["status", "--porcelain"], temp_dir.path()) - .await - .expect("run supported fake Git"); - assert_eq!( - ( - supported_output.status.code(), - supported_output.stdout, - supported_output.stderr, - ), - ( - Some(0), - b"worktree output\n".to_vec(), - b"worktree diagnostics\n".to_vec(), - ) - ); - - let actual = std::fs::read_to_string(log).expect("read fake Git log"); - let disabled_hooks = format!("core.hooksPath={DISABLED_HOOKS_PATH}"); - assert_eq!( - actual.lines().map(str::to_string).collect::>(), - vec![ - "config --null --get core.fsmonitor".to_string(), - "version --build-options".to_string(), - format!("-c {disabled_hooks} -c core.fsmonitor=false status --porcelain"), - "config --null --get core.fsmonitor".to_string(), - "version --build-options".to_string(), - format!("-c {disabled_hooks} -c core.fsmonitor=true status --porcelain"), - ] - ); - } } diff --git a/codex-rs/git-utils/src/lib.rs b/codex-rs/git-utils/src/lib.rs index 9935b833638f..e04e6044109d 100644 --- a/codex-rs/git-utils/src/lib.rs +++ b/codex-rs/git-utils/src/lib.rs @@ -22,8 +22,9 @@ pub use baseline::reset_git_repository; pub use branch::merge_base_with_head; pub use codex_protocol::protocol::GitSha; pub use errors::GitToolingError; -pub use fsmonitor::is_canonical_fsmonitor_true; -pub use fsmonitor::supports_builtin_fsmonitor; +pub use fsmonitor::FsmonitorOverride; +pub use fsmonitor::FsmonitorProbeRunner; +pub use fsmonitor::detect_fsmonitor_override; pub use info::CommitLogEntry; pub use info::GitDiffToRemote; pub use info::GitInfo; diff --git a/codex-rs/tui/src/get_git_diff.rs b/codex-rs/tui/src/get_git_diff.rs index cf3d0ba80b9d..ab7764b6b3f1 100644 --- a/codex-rs/tui/src/get_git_diff.rs +++ b/codex-rs/tui/src/get_git_diff.rs @@ -11,11 +11,11 @@ use std::time::Duration; use crate::workspace_command::WorkspaceCommand; use crate::workspace_command::WorkspaceCommandExecutor; use crate::workspace_command::WorkspaceCommandOutput; -use codex_git_utils::is_canonical_fsmonitor_true; -use codex_git_utils::supports_builtin_fsmonitor; +use codex_git_utils::FsmonitorOverride; +use codex_git_utils::FsmonitorProbeRunner; +use codex_git_utils::detect_fsmonitor_override; 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 { @@ -23,10 +23,23 @@ const DISABLE_HOOKS_CONFIG: &str = if cfg!(windows) { }; const EXECUTABLE_FILTER_CONFIG_PATTERN: &str = r"^filter\..*\.(clean|process)$"; -#[derive(Clone, Copy)] -enum FsmonitorOverride { - Disabled, - BuiltIn, +// `/diff` may execute Git through a remote workspace, so git-utils owns the +// probe policy while this adapter keeps command execution in the TUI layer. +// WorkspaceCommand bounds each call; `/diff` has no aggregate command deadline. +struct WorkspaceFsmonitorProbeRunner<'a> { + runner: &'a dyn WorkspaceCommandExecutor, + cwd: &'a Path, +} + +impl FsmonitorProbeRunner for WorkspaceFsmonitorProbeRunner<'_> { + async fn run_probe(&mut self, args: &[&str]) -> Option> { + let argv = ["git"].into_iter().chain(args.iter().copied()); + let command = WorkspaceCommand::new(argv).cwd(self.cwd.to_path_buf()); + match self.runner.run(command).await { + Ok(output) if output.success() => Some(output.stdout.into_bytes()), + _ => None, + } + } } /// Return value of [`get_git_diff`]. @@ -42,29 +55,12 @@ pub(crate) async fn get_git_diff( return Ok((false, String::new())); } - let config_output = run_git_probe( - runner, - cwd, - &["config", "--null", "--get", "core.fsmonitor"], - ) - .await; - let fsmonitor = if config_output.is_ok_and(|output| { - output.success() && is_canonical_fsmonitor_true(output.stdout.as_bytes()) - }) { - match run_git_probe(runner, cwd, &["version", "--build-options"]).await { - Ok(output) - if output.success() && supports_builtin_fsmonitor(output.stdout.as_bytes()) => - { - FsmonitorOverride::BuiltIn - } - _ => FsmonitorOverride::Disabled, - } - } else { - FsmonitorOverride::Disabled - }; + // Probe once per `/diff` and reuse the result for all subsequent Git commands. + let mut probe_runner = WorkspaceFsmonitorProbeRunner { runner, cwd }; + let fsmonitor = detect_fsmonitor_override(&mut probe_runner).await; // Keep `/diff` informational: repository configuration must not select executable diff helpers. - let diff_config_overrides = diff_filter_config_overrides(runner, cwd).await?; + let diff_config_overrides = diff_filter_config_overrides(runner, cwd, fsmonitor).await?; let (tracked_diff_res, untracked_output_res) = tokio::join!( run_git_capture_diff( runner, @@ -115,14 +111,8 @@ pub(crate) async fn get_git_diff( null_path, file, ]; - let diff = run_git_capture_diff( - runner, - cwd, - FsmonitorOverride::Disabled, - &diff_config_overrides, - &args, - ) - .await?; + let diff = + run_git_capture_diff(runner, cwd, fsmonitor, &diff_config_overrides, &args).await?; untracked_diff.push_str(&diff); } @@ -173,6 +163,7 @@ async fn run_git_capture_diff( async fn diff_filter_config_overrides( runner: &dyn WorkspaceCommandExecutor, cwd: &Path, + fsmonitor: FsmonitorOverride, ) -> Result, String> { let args = [ "config", @@ -181,7 +172,7 @@ async fn diff_filter_config_overrides( "--get-regexp", EXECUTABLE_FILTER_CONFIG_PATTERN, ]; - let output = run_git_command(runner, cwd, FsmonitorOverride::Disabled, &[], &args).await?; + let output = run_git_command(runner, cwd, fsmonitor, &[], &args).await?; if output.exit_code != 0 && output.exit_code != 1 { return Err(format!( "git {:?} failed with status {}", @@ -218,6 +209,8 @@ async fn inside_git_repo( runner: &dyn WorkspaceCommandExecutor, cwd: &Path, ) -> Result { + // `rev-parse` does not inspect the worktree, and probing before this check + // would also run extra Git commands outside repositories. let output = run_git_command( runner, cwd, @@ -229,25 +222,6 @@ async fn inside_git_repo( Ok(output.success()) } -async fn run_git_probe( - runner: &dyn WorkspaceCommandExecutor, - cwd: &Path, - args: &[&str], -) -> Result { - let mut argv = Vec::with_capacity(args.len() + 3); - argv.extend([ - "git".to_string(), - "-c".to_string(), - DISABLE_HOOKS_CONFIG.to_string(), - ]); - argv.extend(args.iter().map(|arg| (*arg).to_string())); - let command = WorkspaceCommand::new(argv) - .cwd(cwd.to_path_buf()) - .env("GIT_OPTIONAL_LOCKS", "0") - .env("LC_ALL", "C"); - runner.run(command).await.map_err(|err| err.to_string()) -} - async fn run_git_command( runner: &dyn WorkspaceCommandExecutor, cwd: &Path, @@ -255,19 +229,15 @@ async fn run_git_command( config_overrides: &[(String, String)], args: &[&str], ) -> Result { - let fsmonitor_config = match fsmonitor { - FsmonitorOverride::Disabled => DISABLE_FSMONITOR_CONFIG, - FsmonitorOverride::BuiltIn => "core.fsmonitor=true", - }; - let mut argv = Vec::with_capacity(args.len() + 5); - argv.push("git".to_string()); - argv.extend([ - "-c".to_string(), - fsmonitor_config.to_string(), - "-c".to_string(), - DISABLE_HOOKS_CONFIG.to_string(), - ]); - argv.extend(args.iter().map(|arg| (*arg).to_string())); + let argv = [ + "git", + "-c", + fsmonitor.git_config_arg(), + "-c", + DISABLE_HOOKS_CONFIG, + ] + .into_iter() + .chain(args.iter().copied()); let mut command = WorkspaceCommand::new(argv) .cwd(cwd.to_path_buf()) .timeout(DIFF_COMMAND_TIMEOUT) @@ -305,7 +275,10 @@ mod tests { async fn get_git_diff_returns_not_git_for_non_git_cwd() { let cwd = PathBuf::from("/workspace"); let runner = FakeRunner::new(vec![response( - git_command(&["rev-parse", "--is-inside-work-tree"]), + git_command( + FsmonitorOverride::Disabled, + &["rev-parse", "--is-inside-work-tree"], + ), /*exit_code*/ 128, "", )]); @@ -313,11 +286,7 @@ mod tests { let result = get_git_diff(&runner, &cwd).await; assert_eq!(result, Ok((false, String::new()))); - assert_commands( - &runner.commands(), - &[git_command(&["rev-parse", "--is-inside-work-tree"])], - &cwd, - ); + assert_command_metadata(&runner.commands(), &cwd); } #[tokio::test] @@ -325,7 +294,10 @@ mod tests { let cwd = PathBuf::from("/workspace"); let runner = FakeRunner::new(vec![ response( - git_command(&["rev-parse", "--is-inside-work-tree"]), + git_command( + FsmonitorOverride::Disabled, + &["rev-parse", "--is-inside-work-tree"], + ), /*exit_code*/ 0, "true\n", ), @@ -335,46 +307,71 @@ mod tests { "/tmp/fsmonitor-helper\0", ), response( - git_command(&[ + git_probe_command(&[ "config", "--null", - "--name-only", - "--get-regexp", - EXECUTABLE_FILTER_CONFIG_PATTERN, + "--type=bool", + "--fixed-value", + "--get", + "core.fsmonitor", + "/tmp/fsmonitor-helper", ]), + /*exit_code*/ 128, + "", + ), + response( + git_command( + FsmonitorOverride::Disabled, + &[ + "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", - ]), + git_command( + FsmonitorOverride::Disabled, + &[ + "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", + "--color", + ], + ), /*exit_code*/ 1, "tracked\n", ), response( - git_command(&["ls-files", "--others", "--exclude-standard"]), + git_command( + FsmonitorOverride::Disabled, + &["ls-files", "--others", "--exclude-standard"], + ), /*exit_code*/ 0, "new.txt\n", ), response( - git_command(&[ - "diff", - "--no-textconv", - "--no-ext-diff", - "--submodule=short", - "--ignore-submodules=dirty", - "--color", - "--no-index", - "--", - null_device(), - "new.txt", - ]), + git_command( + FsmonitorOverride::Disabled, + &[ + "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", + "--color", + "--no-index", + "--", + null_device(), + "new.txt", + ], + ), /*exit_code*/ 1, "untracked\n", ), @@ -384,52 +381,20 @@ mod tests { assert_eq!(result, Ok((true, "tracked\nuntracked\n".to_string()))); let commands = runner.commands(); - assert_commands( - &commands, - &[ - git_command(&["rev-parse", "--is-inside-work-tree"]), - git_probe_command(&["config", "--null", "--get", "core.fsmonitor"]), - 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[3].env, filter_override_env("filter.evil")); - assert_eq!(commands[5].env, filter_override_env("filter.evil")); + assert_command_metadata(&commands, &cwd); + assert_eq!(commands[4].env, filter_override_env("filter.evil")); + assert_eq!(commands[6].env, filter_override_env("filter.evil")); } #[tokio::test] - async fn get_git_diff_preserves_builtin_fsmonitor_for_worktree_reads() { + async fn get_git_diff_preserves_builtin_fsmonitor_for_diff_workflow() { let cwd = PathBuf::from("/workspace"); let runner = FakeRunner::new(vec![ response( - git_command(&["rev-parse", "--is-inside-work-tree"]), + git_command( + FsmonitorOverride::Disabled, + &["rev-parse", "--is-inside-work-tree"], + ), /*exit_code*/ 0, "true\n", ), @@ -444,18 +409,21 @@ mod tests { "feature: fsmonitor--daemon\n", ), response( - git_command(&[ - "config", - "--null", - "--name-only", - "--get-regexp", - EXECUTABLE_FILTER_CONFIG_PATTERN, - ]), + git_command( + FsmonitorOverride::BuiltIn, + &[ + "config", + "--null", + "--name-only", + "--get-regexp", + EXECUTABLE_FILTER_CONFIG_PATTERN, + ], + ), /*exit_code*/ 1, "", ), response( - git_command_with_fsmonitor( + git_command( FsmonitorOverride::BuiltIn, &[ "diff", @@ -470,7 +438,7 @@ mod tests { "tracked\n", ), response( - git_command_with_fsmonitor( + git_command( FsmonitorOverride::BuiltIn, &["ls-files", "--others", "--exclude-standard"], ), @@ -478,40 +446,7 @@ mod tests { "new.txt\n", ), response( - 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", - ), - ]); - - let result = get_git_diff(&runner, &cwd).await; - - assert_eq!(result, Ok((true, "tracked\nuntracked\n".to_string()))); - assert_commands( - &runner.commands(), - &[ - git_command(&["rev-parse", "--is-inside-work-tree"]), - git_probe_command(&["config", "--null", "--get", "core.fsmonitor"]), - git_probe_command(&["version", "--build-options"]), - git_command(&[ - "config", - "--null", - "--name-only", - "--get-regexp", - EXECUTABLE_FILTER_CONFIG_PATTERN, - ]), - git_command_with_fsmonitor( + git_command( FsmonitorOverride::BuiltIn, &[ "diff", @@ -520,81 +455,21 @@ mod tests { "--submodule=short", "--ignore-submodules=dirty", "--color", + "--no-index", + "--", + null_device(), + "new.txt", ], ), - git_command_with_fsmonitor( - FsmonitorOverride::BuiltIn, - &["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, - ); - } - - #[tokio::test] - async fn get_git_diff_disables_fsmonitor_without_builtin_daemon_support() { - let cwd = PathBuf::from("/workspace"); - let runner = FakeRunner::new(vec![ - response( - git_command(&["rev-parse", "--is-inside-work-tree"]), - /*exit_code*/ 0, - "true\n", - ), - response( - git_probe_command(&["config", "--null", "--get", "core.fsmonitor"]), - /*exit_code*/ 0, - "true\0", - ), - response( - git_probe_command(&["version", "--build-options"]), - /*exit_code*/ 0, - "", - ), - response( - 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*/ 0, - "", - ), - response( - git_command(&["ls-files", "--others", "--exclude-standard"]), - /*exit_code*/ 0, - "", + "untracked\n", ), ]); let result = get_git_diff(&runner, &cwd).await; - assert_eq!(result, Ok((true, String::new()))); + assert_eq!(result, Ok((true, "tracked\nuntracked\n".to_string()))); + assert_command_metadata(&runner.commands(), &cwd); } #[tokio::test] @@ -602,7 +477,10 @@ mod tests { let cwd = PathBuf::from("/workspace"); let runner = FakeRunner::new(vec![ response( - git_command(&["rev-parse", "--is-inside-work-tree"]), + git_command( + FsmonitorOverride::Disabled, + &["rev-parse", "--is-inside-work-tree"], + ), /*exit_code*/ 0, "true\n", ), @@ -612,30 +490,39 @@ mod tests { "", ), response( - git_command(&[ - "config", - "--null", - "--name-only", - "--get-regexp", - EXECUTABLE_FILTER_CONFIG_PATTERN, - ]), + git_command( + FsmonitorOverride::Disabled, + &[ + "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", - ]), + git_command( + FsmonitorOverride::Disabled, + &[ + "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", + "--color", + ], + ), /*exit_code*/ 1, "tracked\n", ), response( - git_command(&["ls-files", "--others", "--exclude-standard"]), + git_command( + FsmonitorOverride::Disabled, + &["ls-files", "--others", "--exclude-standard"], + ), /*exit_code*/ 0, "", ), @@ -644,6 +531,7 @@ mod tests { let result = get_git_diff(&runner, &cwd).await; assert_eq!(result, Ok((true, "tracked\n".to_string()))); + assert_command_metadata(&runner.commands(), &cwd); } #[tokio::test] @@ -651,7 +539,10 @@ mod tests { let cwd = PathBuf::from("/workspace"); let runner = FakeRunner::new(vec![ response( - git_command(&["rev-parse", "--is-inside-work-tree"]), + git_command( + FsmonitorOverride::Disabled, + &["rev-parse", "--is-inside-work-tree"], + ), /*exit_code*/ 0, "true\n", ), @@ -661,30 +552,39 @@ mod tests { "", ), response( - git_command(&[ - "config", - "--null", - "--name-only", - "--get-regexp", - EXECUTABLE_FILTER_CONFIG_PATTERN, - ]), + git_command( + FsmonitorOverride::Disabled, + &[ + "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", - ]), + git_command( + FsmonitorOverride::Disabled, + &[ + "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", + "--color", + ], + ), /*exit_code*/ 2, "", ), response( - git_command(&["ls-files", "--others", "--exclude-standard"]), + git_command( + FsmonitorOverride::Disabled, + &["ls-files", "--others", "--exclude-standard"], + ), /*exit_code*/ 0, "", ), @@ -694,12 +594,11 @@ mod tests { .await .expect_err("unexpected git diff status should fail"); - assert!( - error.contains( - "git [\"diff\", \"--no-textconv\", \"--no-ext-diff\", \"--submodule=short\", \"--ignore-submodules=dirty\", \"--color\"] failed with status 2" - ), - "unexpected error: {error}", + assert_eq!( + error, + "git [\"diff\", \"--no-textconv\", \"--no-ext-diff\", \"--submodule=short\", \"--ignore-submodules=dirty\", \"--color\"] failed with status 2" ); + assert_command_metadata(&runner.commands(), &cwd); } #[cfg(unix)] @@ -769,11 +668,18 @@ mod tests { .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()); + assert_eq!( + ( + result.1.contains("before"), + result.1.contains("after"), + filter_helper.with_extension("sh.ran").exists(), + fsmonitor_helper.with_extension("sh.ran").exists(), + hook_helper.with_extension("sh.ran").exists(), + ), + (true, true, false, false, false), + "diff:\n{}", + result.1 + ); } #[cfg(unix)] @@ -829,38 +735,32 @@ mod tests { .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 { - git_command_with_fsmonitor(FsmonitorOverride::Disabled, args) + assert_eq!( + (result.1, helper.with_extension("sh.ran").exists()), + (String::new(), false) + ); } - fn git_command_with_fsmonitor(fsmonitor: FsmonitorOverride, args: &[&str]) -> Vec { - let fsmonitor_config = match fsmonitor { - FsmonitorOverride::Disabled => DISABLE_FSMONITOR_CONFIG, - FsmonitorOverride::BuiltIn => "core.fsmonitor=true", - }; - let mut argv = vec![ - "git".to_string(), - "-c".to_string(), - fsmonitor_config.to_string(), - "-c".to_string(), - DISABLE_HOOKS_CONFIG.to_string(), - ]; - argv.extend(args.iter().map(|arg| (*arg).to_string())); - argv + fn git_command(fsmonitor: FsmonitorOverride, args: &[&str]) -> Vec { + [ + "git", + "-c", + fsmonitor.git_config_arg(), + "-c", + DISABLE_HOOKS_CONFIG, + ] + .into_iter() + .chain(args.iter().copied()) + .map(str::to_string) + .collect() } fn git_probe_command(args: &[&str]) -> Vec { - let mut argv = vec![ - "git".to_string(), - "-c".to_string(), - DISABLE_HOOKS_CONFIG.to_string(), - ]; - argv.extend(args.iter().map(|arg| (*arg).to_string())); - argv + ["git"] + .into_iter() + .chain(args.iter().copied()) + .map(str::to_string) + .collect() } fn filter_override_env(driver: &str) -> HashMap> { @@ -901,12 +801,18 @@ mod tests { #[cfg(unix)] fn run_git_setup(cwd: &Path, args: &[&str]) { - let status = ProcessCommand::new("git") + let output = ProcessCommand::new("git") .args(args) .current_dir(cwd) - .status() + .output() .expect("run git setup command"); - assert!(status.success(), "git setup command failed: {args:?}"); + assert_eq!( + output.status.code(), + Some(0), + "git setup command failed: {args:?}\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); } #[cfg(unix)] @@ -920,16 +826,14 @@ mod tests { 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(); - assert_eq!(actual, expected); - + fn assert_command_metadata(commands: &[WorkspaceCommand], cwd: &Path) { for command in commands { assert_eq!(command.cwd.as_deref(), Some(cwd)); - if command.argv.get(2).map(String::as_str) == Some(DISABLE_HOOKS_CONFIG) { + if matches!( + command.argv.get(1).map(String::as_str), + Some("config" | "version") + ) { + assert_eq!(command.env, HashMap::new()); assert_eq!(command.timeout, Duration::from_secs(/*secs*/ 5)); assert_eq!(command.output_bytes_cap, 64 * 1024); assert_eq!(command.disable_output_cap, false); @@ -959,6 +863,11 @@ mod tests { } fn commands(&self) -> Vec { + assert_eq!( + self.responses.lock().expect("responses lock").len(), + 0, + "unused fake responses" + ); self.commands.lock().expect("commands lock").clone() } } From 6d893fd1ae65b23eb890ff12164c2c368fa52af1 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Mon, 8 Jun 2026 19:50:09 -0700 Subject: [PATCH 6/7] [codex] Share fsmonitor timeout budget Fsmonitor detection runs before status and diff, but giving the requested command a fresh timeout lets a stalled probe double the existing five-second command budget. Create one deadline at each local worktree workflow boundary and pass it through detection and command execution. This preserves the latency bound while still reusing one fsmonitor decision across the workflow. Inline the probe arguments at their only call sites. --- codex-rs/git-utils/src/fsmonitor.rs | 10 ++--- codex-rs/git-utils/src/info.rs | 58 ++++++++++++++++++++--------- 2 files changed, 45 insertions(+), 23 deletions(-) diff --git a/codex-rs/git-utils/src/fsmonitor.rs b/codex-rs/git-utils/src/fsmonitor.rs index f7d7ec651d33..b4902ec37d06 100644 --- a/codex-rs/git-utils/src/fsmonitor.rs +++ b/codex-rs/git-utils/src/fsmonitor.rs @@ -10,9 +10,6 @@ use std::future::Future; -const CONFIG_PROBE_ARGS: &[&str] = &["config", "--null", "--get", "core.fsmonitor"]; -const CAPABILITY_PROBE_ARGS: &[&str] = &["version", "--build-options"]; - /// The safe `core.fsmonitor` override for an internal Git command. #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum FsmonitorOverride { @@ -57,7 +54,10 @@ pub async fn detect_fsmonitor_override( // local true fail conversion. Query the raw effective value first. // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/builtin/config.c#L482-L514 // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/builtin/config.c#L611-L614 - let Some(config) = runner.run_probe(CONFIG_PROBE_ARGS).await else { + let Some(config) = runner + .run_probe(&["config", "--null", "--get", "core.fsmonitor"]) + .await + else { return FsmonitorOverride::Disabled; }; let Some(config) = config.strip_suffix(b"\0") else { @@ -111,7 +111,7 @@ pub async fn detect_fsmonitor_override( // the feature line Git added specifically for capability checks. // https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2e3ca5313a4e0/Documentation/config/core.adoc#L90-L99 // https://github.com/git/git/commit/dd77cf61a1a2fbf52c94d0cd986d555ad2ba8a4b - let Some(build_options) = runner.run_probe(CAPABILITY_PROBE_ARGS).await else { + let Some(build_options) = runner.run_probe(&["version", "--build-options"]).await else { return FsmonitorOverride::Disabled; }; if build_options diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index 84d5a83d58d5..91e46e70f60b 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -12,7 +12,8 @@ use serde::Deserialize; use serde::Serialize; use tokio::process::Command; use tokio::time::Duration as TokioDuration; -use tokio::time::timeout; +use tokio::time::Instant as TokioInstant; +use tokio::time::timeout_at; use ts_rs::TS; use crate::GitSha; @@ -280,9 +281,16 @@ fn trim_git_suffix(value: &str) -> &str { pub async fn get_has_changes(cwd: &Path) -> Option { let git = Path::new("git"); - let fsmonitor = detect_local_fsmonitor_override(git, cwd).await; - let output = - run_git_command_with_timeout_from(git, &["status", "--porcelain"], cwd, fsmonitor).await?; + let deadline = TokioInstant::now() + GIT_COMMAND_TIMEOUT; + let fsmonitor = detect_local_fsmonitor_override(git, cwd, deadline).await; + let output = run_git_command_with_deadline_from( + git, + &["status", "--porcelain"], + cwd, + fsmonitor, + deadline, + ) + .await?; if !output.status.success() { return None; } @@ -395,11 +403,12 @@ pub async fn git_diff_to_remote(cwd: &Path) -> Option { async fn run_git_command_with_timeout(args: &[&str], cwd: &Path) -> Option { // These callers only inspect repository metadata. Worktree workflows probe // once and pass their override directly to the lower-level runner. - run_git_command_with_timeout_from( + run_git_command_with_deadline_from( Path::new("git"), args, cwd, crate::FsmonitorOverride::Disabled, + TokioInstant::now() + GIT_COMMAND_TIMEOUT, ) .await } @@ -407,29 +416,35 @@ async fn run_git_command_with_timeout(args: &[&str], cwd: &Path) -> Option { git: &'a Path, cwd: &'a Path, + deadline: TokioInstant, } impl crate::FsmonitorProbeRunner for LocalFsmonitorProbeRunner<'_> { async fn run_probe(&mut self, args: &[&str]) -> Option> { let mut command = Command::new(self.git); command.args(args).current_dir(self.cwd).kill_on_drop(true); - match timeout(GIT_COMMAND_TIMEOUT, command.output()).await { + match timeout_at(self.deadline, command.output()).await { Ok(Ok(output)) if output.status.success() => Some(output.stdout), _ => None, } } } -async fn detect_local_fsmonitor_override(git: &Path, cwd: &Path) -> crate::FsmonitorOverride { - let mut runner = LocalFsmonitorProbeRunner { git, cwd }; +async fn detect_local_fsmonitor_override( + git: &Path, + cwd: &Path, + deadline: TokioInstant, +) -> crate::FsmonitorOverride { + let mut runner = LocalFsmonitorProbeRunner { git, cwd, deadline }; crate::detect_fsmonitor_override(&mut runner).await } -async fn run_git_command_with_timeout_from( +async fn run_git_command_with_deadline_from( git: &Path, args: &[&str], cwd: &Path, fsmonitor: crate::FsmonitorOverride, + deadline: TokioInstant, ) -> Option { let mut command = Command::new(git); command @@ -441,7 +456,7 @@ async fn run_git_command_with_timeout_from( .args(args) .current_dir(cwd) .kill_on_drop(true); - let result = timeout(GIT_COMMAND_TIMEOUT, command.output()).await; + let result = timeout_at(deadline, command.output()).await; match result { Ok(Ok(output)) => Some(output), @@ -727,12 +742,14 @@ async fn find_closest_sha(cwd: &Path, branches: &[String], remotes: &[String]) - async fn diff_against_sha(cwd: &Path, sha: &GitSha) -> Option { let git = Path::new("git"); - let fsmonitor = detect_local_fsmonitor_override(git, cwd).await; - let output = run_git_command_with_timeout_from( + let deadline = TokioInstant::now() + GIT_COMMAND_TIMEOUT; + let fsmonitor = detect_local_fsmonitor_override(git, cwd, deadline).await; + let output = run_git_command_with_deadline_from( git, &["diff", "--no-textconv", "--no-ext-diff", &sha.0], cwd, fsmonitor, + deadline, ) .await?; // 0 is success and no diff. @@ -743,11 +760,12 @@ async fn diff_against_sha(cwd: &Path, sha: &GitSha) -> Option { } let mut diff = String::from_utf8(output.stdout).ok()?; - if let Some(untracked_output) = run_git_command_with_timeout_from( + if let Some(untracked_output) = run_git_command_with_deadline_from( git, &["ls-files", "--others", "--exclude-standard"], cwd, fsmonitor, + deadline, ) .await && untracked_output.status.success() @@ -775,7 +793,7 @@ async fn diff_against_sha(cwd: &Path, sha: &GitSha) -> Option { null_device, &file_owned, ]; - run_git_command_with_timeout_from(git, &args_vec, cwd, fsmonitor).await + run_git_command_with_deadline_from(git, &args_vec, cwd, fsmonitor, deadline).await }); let results = join_all(futures_iter).await; for extra in results.into_iter().flatten() { @@ -967,12 +985,14 @@ mod tests { // The config response mirrors: // git -c core.fsmonitor=/tmp/fsmonitor-helper \ // config --null --get core.fsmonitor - let fsmonitor = detect_local_fsmonitor_override(&git, temp_dir.path()).await; - let output = run_git_command_with_timeout_from( + let deadline = TokioInstant::now() + GIT_COMMAND_TIMEOUT; + let fsmonitor = detect_local_fsmonitor_override(&git, temp_dir.path(), deadline).await; + let output = run_git_command_with_deadline_from( &git, &["status", "--porcelain"], temp_dir.path(), fsmonitor, + deadline, ) .await .expect("run fake Git"); @@ -1058,12 +1078,14 @@ mod tests { "write local built-in fsmonitor config" ); - let fsmonitor = detect_local_fsmonitor_override(&git, repo.as_path()).await; - let output = run_git_command_with_timeout_from( + let deadline = TokioInstant::now() + GIT_COMMAND_TIMEOUT; + let fsmonitor = detect_local_fsmonitor_override(&git, repo.as_path(), deadline).await; + let output = run_git_command_with_deadline_from( &git, &["status", "--porcelain"], repo.as_path(), fsmonitor, + deadline, ) .await .expect("run Git with layered config"); From d2b82c00f03db7dc3b296430dbee944a58c6aec3 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Mon, 8 Jun 2026 20:11:55 -0700 Subject: [PATCH 7/7] [codex] Restore per-command Git timeouts Sharing one deadline between fsmonitor detection and subsequent Git commands changes the existing timeout behavior for diff collection. Later ls-files and no-index diffs inherit time spent by earlier commands. The fsmonitor probes only query configuration and build options and do not inspect the worktree or index. Keep them independently bounded and preserve the existing five-second timeout for each requested Git command. --- codex-rs/git-utils/src/info.rs | 60 ++++++++++++---------------------- 1 file changed, 20 insertions(+), 40 deletions(-) diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index 91e46e70f60b..7c0d59462c47 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -12,8 +12,7 @@ use serde::Deserialize; use serde::Serialize; use tokio::process::Command; use tokio::time::Duration as TokioDuration; -use tokio::time::Instant as TokioInstant; -use tokio::time::timeout_at; +use tokio::time::timeout; use ts_rs::TS; use crate::GitSha; @@ -281,16 +280,9 @@ fn trim_git_suffix(value: &str) -> &str { pub async fn get_has_changes(cwd: &Path) -> Option { let git = Path::new("git"); - let deadline = TokioInstant::now() + GIT_COMMAND_TIMEOUT; - let fsmonitor = detect_local_fsmonitor_override(git, cwd, deadline).await; - let output = run_git_command_with_deadline_from( - git, - &["status", "--porcelain"], - cwd, - fsmonitor, - deadline, - ) - .await?; + let fsmonitor = detect_local_fsmonitor_override(git, cwd).await; + let output = + run_git_command_with_timeout_from(git, &["status", "--porcelain"], cwd, fsmonitor).await?; if !output.status.success() { return None; } @@ -403,12 +395,11 @@ pub async fn git_diff_to_remote(cwd: &Path) -> Option { async fn run_git_command_with_timeout(args: &[&str], cwd: &Path) -> Option { // These callers only inspect repository metadata. Worktree workflows probe // once and pass their override directly to the lower-level runner. - run_git_command_with_deadline_from( + run_git_command_with_timeout_from( Path::new("git"), args, cwd, crate::FsmonitorOverride::Disabled, - TokioInstant::now() + GIT_COMMAND_TIMEOUT, ) .await } @@ -416,35 +407,31 @@ async fn run_git_command_with_timeout(args: &[&str], cwd: &Path) -> Option { git: &'a Path, cwd: &'a Path, - deadline: TokioInstant, } impl crate::FsmonitorProbeRunner for LocalFsmonitorProbeRunner<'_> { async fn run_probe(&mut self, args: &[&str]) -> Option> { + // Both probes are fast, bounded metadata queries that do not inspect the + // worktree or index, so do not reduce the requested command's timeout. let mut command = Command::new(self.git); command.args(args).current_dir(self.cwd).kill_on_drop(true); - match timeout_at(self.deadline, command.output()).await { + match timeout(GIT_COMMAND_TIMEOUT, command.output()).await { Ok(Ok(output)) if output.status.success() => Some(output.stdout), _ => None, } } } -async fn detect_local_fsmonitor_override( - git: &Path, - cwd: &Path, - deadline: TokioInstant, -) -> crate::FsmonitorOverride { - let mut runner = LocalFsmonitorProbeRunner { git, cwd, deadline }; +async fn detect_local_fsmonitor_override(git: &Path, cwd: &Path) -> crate::FsmonitorOverride { + let mut runner = LocalFsmonitorProbeRunner { git, cwd }; crate::detect_fsmonitor_override(&mut runner).await } -async fn run_git_command_with_deadline_from( +async fn run_git_command_with_timeout_from( git: &Path, args: &[&str], cwd: &Path, fsmonitor: crate::FsmonitorOverride, - deadline: TokioInstant, ) -> Option { let mut command = Command::new(git); command @@ -456,7 +443,7 @@ async fn run_git_command_with_deadline_from( .args(args) .current_dir(cwd) .kill_on_drop(true); - let result = timeout_at(deadline, command.output()).await; + let result = timeout(GIT_COMMAND_TIMEOUT, command.output()).await; match result { Ok(Ok(output)) => Some(output), @@ -742,14 +729,12 @@ async fn find_closest_sha(cwd: &Path, branches: &[String], remotes: &[String]) - async fn diff_against_sha(cwd: &Path, sha: &GitSha) -> Option { let git = Path::new("git"); - let deadline = TokioInstant::now() + GIT_COMMAND_TIMEOUT; - let fsmonitor = detect_local_fsmonitor_override(git, cwd, deadline).await; - let output = run_git_command_with_deadline_from( + let fsmonitor = detect_local_fsmonitor_override(git, cwd).await; + let output = run_git_command_with_timeout_from( git, &["diff", "--no-textconv", "--no-ext-diff", &sha.0], cwd, fsmonitor, - deadline, ) .await?; // 0 is success and no diff. @@ -760,12 +745,11 @@ async fn diff_against_sha(cwd: &Path, sha: &GitSha) -> Option { } let mut diff = String::from_utf8(output.stdout).ok()?; - if let Some(untracked_output) = run_git_command_with_deadline_from( + if let Some(untracked_output) = run_git_command_with_timeout_from( git, &["ls-files", "--others", "--exclude-standard"], cwd, fsmonitor, - deadline, ) .await && untracked_output.status.success() @@ -793,7 +777,7 @@ async fn diff_against_sha(cwd: &Path, sha: &GitSha) -> Option { null_device, &file_owned, ]; - run_git_command_with_deadline_from(git, &args_vec, cwd, fsmonitor, deadline).await + run_git_command_with_timeout_from(git, &args_vec, cwd, fsmonitor).await }); let results = join_all(futures_iter).await; for extra in results.into_iter().flatten() { @@ -985,14 +969,12 @@ mod tests { // The config response mirrors: // git -c core.fsmonitor=/tmp/fsmonitor-helper \ // config --null --get core.fsmonitor - let deadline = TokioInstant::now() + GIT_COMMAND_TIMEOUT; - let fsmonitor = detect_local_fsmonitor_override(&git, temp_dir.path(), deadline).await; - let output = run_git_command_with_deadline_from( + let fsmonitor = detect_local_fsmonitor_override(&git, temp_dir.path()).await; + let output = run_git_command_with_timeout_from( &git, &["status", "--porcelain"], temp_dir.path(), fsmonitor, - deadline, ) .await .expect("run fake Git"); @@ -1078,14 +1060,12 @@ mod tests { "write local built-in fsmonitor config" ); - let deadline = TokioInstant::now() + GIT_COMMAND_TIMEOUT; - let fsmonitor = detect_local_fsmonitor_override(&git, repo.as_path(), deadline).await; - let output = run_git_command_with_deadline_from( + let fsmonitor = detect_local_fsmonitor_override(&git, repo.as_path()).await; + let output = run_git_command_with_timeout_from( &git, &["status", "--porcelain"], repo.as_path(), fsmonitor, - deadline, ) .await .expect("run Git with layered config");