From dffc4bf75dc9eeb0727f668c95bb7bd72315581d Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Mon, 8 Jun 2026 21:32:46 -0700 Subject: [PATCH] [codex] preserve fsmonitor for worktree Git reads (#26880) Codex forces `core.fsmonitor=false` on internal Git commands so a repository cannot select an executable fsmonitor helper. This also disables Git's built-in daemon for `status`, `diff`, and `ls-files`, turning those worktree reads into full scans in large repositories. Read the raw effective `core.fsmonitor` value and preserve it only when Git interprets it as true and advertises built-in daemon support through `git version --build-options`. Query uncommon boolean spellings back through Git using the exact effective value. Unset, false, helper paths, malformed values, probe failures, and unsupported Git builds continue to force `core.fsmonitor=false`. Centralize this policy in `git-utils` while keeping process execution in the existing local and workspace-command adapters. Probe once per worktree workflow and reuse the result for its Git commands, including the TUI `/diff` path. Metadata-only commands and repository discovery remain disabled without probing. Each probe and requested Git process keeps its own existing timeout, and the decision is not cached because layered and conditional Git configuration can change while Codex runs. --------- Co-authored-by: Chris Bookholt --- codex-rs/core/src/git_info_tests.rs | 40 -- codex-rs/git-utils/src/fsmonitor.rs | 129 ++++++ codex-rs/git-utils/src/fsmonitor_tests.rs | 139 ++++++ codex-rs/git-utils/src/info.rs | 219 +++++++++- codex-rs/git-utils/src/lib.rs | 4 + codex-rs/tui/src/get_git_diff.rs | 503 +++++++++++++++------- 6 files changed, 831 insertions(+), 203 deletions(-) create mode 100644 codex-rs/git-utils/src/fsmonitor.rs create mode 100644 codex-rs/git-utils/src/fsmonitor_tests.rs diff --git a/codex-rs/core/src/git_info_tests.rs b/codex-rs/core/src/git_info_tests.rs index 946779d06..e172cd5f2 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_ignores_repo_fsmonitor_config() { - 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/fsmonitor.rs b/codex-rs/git-utils/src/fsmonitor.rs new file mode 100644 index 000000000..b4902ec37 --- /dev/null +++ b/codex-rs/git-utils/src/fsmonitor.rs @@ -0,0 +1,129 @@ +//! 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; + +/// 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", + } + } +} + +/// 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", "--null", "--get", "core.fsmonitor"]) + .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(&["version", "--build-options"]).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 000000000..e86139421 --- /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 a188d9a87..7c0d59462 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -279,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; } @@ -390,12 +393,53 @@ 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 { - let mut command = Command::new("git"); + // 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> { + // 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(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 { + 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", fsmonitor.git_config_arg()]) .args(args) .current_dir(cwd) .kill_on_drop(true); @@ -684,9 +728,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); @@ -695,8 +745,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) @@ -722,7 +777,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() { @@ -849,6 +904,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() { @@ -886,4 +943,146 @@ mod tests { assert_eq!(canonicalize_git_remote_url(remote), None); } } + + #[cfg(unix)] + #[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) printf '/tmp/fsmonitor-helper\\000' ;;\n\ + *) printf 'worktree output\\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"); + + // 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"); + + assert_eq!( + (output.status.code(), output.stdout), + (Some(0), b"worktree output\n".to_vec()) + ); + let disabled_hooks = format!("core.hooksPath={DISABLED_HOOKS_PATH}"); + assert_eq!( + 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"), + ] + ); + } + + #[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" + ); + + 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()) + ); + + 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"), + ] + ); + } } diff --git a/codex-rs/git-utils/src/lib.rs b/codex-rs/git-utils/src/lib.rs index 986a7434d..e04e60441 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,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::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 7f5545070..ab7764b6b 100644 --- a/codex-rs/tui/src/get_git_diff.rs +++ b/codex-rs/tui/src/get_git_diff.rs @@ -11,9 +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::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 { @@ -21,6 +23,25 @@ const DISABLE_HOOKS_CONFIG: &str = if cfg!(windows) { }; const EXECUTABLE_FILTER_CONFIG_PATTERN: &str = r"^filter\..*\.(clean|process)$"; +// `/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`]. /// /// * `bool` – Whether the current working directory is inside a Git repo. @@ -34,12 +55,17 @@ pub(crate) async fn get_git_diff( return Ok((false, String::new())); } + // 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, cwd, + fsmonitor, &diff_config_overrides, &[ "diff", @@ -50,7 +76,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 +111,8 @@ 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, fsmonitor, &diff_config_overrides, &args).await?; untracked_diff.push_str(&diff); } @@ -92,9 +124,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 +143,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 { @@ -129,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", @@ -137,7 +172,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, fsmonitor, &[], &args).await?; if output.exit_code != 0 && output.exit_code != 1 { return Err(format!( "git {:?} failed with status {}", @@ -174,25 +209,35 @@ 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?; + // `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, + FsmonitorOverride::Disabled, + &[], + &["rev-parse", "--is-inside-work-tree"], + ) + .await?; Ok(output.success()) } async fn run_git_command( runner: &dyn WorkspaceCommandExecutor, cwd: &Path, + fsmonitor: FsmonitorOverride, config_overrides: &[(String, String)], args: &[&str], ) -> Result { - let mut argv = Vec::with_capacity(args.len() + 5); - argv.push("git".to_string()); - argv.extend([ - "-c".to_string(), - DISABLE_FSMONITOR_CONFIG.to_string(), - "-c".to_string(), - DISABLE_HOOKS_CONFIG.to_string(), - ]); - argv.extend(args.iter().map(|arg| (*arg).to_string())); + 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) @@ -230,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, "", )]); @@ -238,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] @@ -250,51 +294,84 @@ 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", ), response( - git_command(&[ + git_probe_command(&["config", "--null", "--get", "core.fsmonitor"]), + /*exit_code*/ 0, + "/tmp/fsmonitor-helper\0", + ), + response( + 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", ), @@ -304,43 +381,95 @@ 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_command(&[ - "config", - "--null", - "--name-only", - "--get-regexp", - EXECUTABLE_FILTER_CONFIG_PATTERN, - ]), - git_command(&[ - "diff", - "--no-textconv", - "--no-ext-diff", - "--submodule=short", - "--ignore-submodules=dirty", - "--color", - ]), - git_command(&["ls-files", "--others", "--exclude-standard"]), - git_command(&[ - "diff", - "--no-textconv", - "--no-ext-diff", - "--submodule=short", - "--ignore-submodules=dirty", - "--color", - "--no-index", - "--", - null_device(), - "new.txt", - ]), - ], - &cwd, - ); - assert_eq!(commands[2].env, filter_override_env("filter.evil")); + assert_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_diff_workflow() { + let cwd = PathBuf::from("/workspace"); + let runner = FakeRunner::new(vec![ + response( + git_command( + FsmonitorOverride::Disabled, + &["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( + FsmonitorOverride::BuiltIn, + &[ + "config", + "--null", + "--name-only", + "--get-regexp", + EXECUTABLE_FILTER_CONFIG_PATTERN, + ], + ), + /*exit_code*/ 1, + "", + ), + response( + git_command( + FsmonitorOverride::BuiltIn, + &[ + "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", + "--color", + ], + ), + /*exit_code*/ 1, + "tracked\n", + ), + response( + git_command( + FsmonitorOverride::BuiltIn, + &["ls-files", "--others", "--exclude-standard"], + ), + /*exit_code*/ 0, + "new.txt\n", + ), + response( + git_command( + FsmonitorOverride::BuiltIn, + &[ + "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_command_metadata(&runner.commands(), &cwd); } #[tokio::test] @@ -348,35 +477,52 @@ 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", ), response( - git_command(&[ - "config", - "--null", - "--name-only", - "--get-regexp", - EXECUTABLE_FILTER_CONFIG_PATTERN, - ]), + git_probe_command(&["config", "--null", "--get", "core.fsmonitor"]), /*exit_code*/ 1, "", ), response( - git_command(&[ - "diff", - "--no-textconv", - "--no-ext-diff", - "--submodule=short", - "--ignore-submodules=dirty", - "--color", - ]), + git_command( + FsmonitorOverride::Disabled, + &[ + "config", + "--null", + "--name-only", + "--get-regexp", + EXECUTABLE_FILTER_CONFIG_PATTERN, + ], + ), + /*exit_code*/ 1, + "", + ), + response( + 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, "", ), @@ -385,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] @@ -392,35 +539,52 @@ 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", ), response( - git_command(&[ - "config", - "--null", - "--name-only", - "--get-regexp", - EXECUTABLE_FILTER_CONFIG_PATTERN, - ]), + git_probe_command(&["config", "--null", "--get", "core.fsmonitor"]), /*exit_code*/ 1, "", ), response( - git_command(&[ - "diff", - "--no-textconv", - "--no-ext-diff", - "--submodule=short", - "--ignore-submodules=dirty", - "--color", - ]), + git_command( + FsmonitorOverride::Disabled, + &[ + "config", + "--null", + "--name-only", + "--get-regexp", + EXECUTABLE_FILTER_CONFIG_PATTERN, + ], + ), + /*exit_code*/ 1, + "", + ), + response( + 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, "", ), @@ -430,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)] @@ -505,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)] @@ -565,20 +735,32 @@ mod tests { .await .expect("generate diff without inspecting submodule worktrees"); - assert!(result.1.is_empty()); - assert!(!helper.with_extension("sh.ran").exists()); + assert_eq!( + (result.1, helper.with_extension("sh.ran").exists()), + (String::new(), false) + ); } - fn git_command(args: &[&str]) -> Vec { - let mut argv = vec![ - "git".to_string(), - "-c".to_string(), - DISABLE_FSMONITOR_CONFIG.to_string(), - "-c".to_string(), - DISABLE_HOOKS_CONFIG.to_string(), - ]; - argv.extend(args.iter().map(|arg| (*arg).to_string())); - argv + fn 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 { + ["git"] + .into_iter() + .chain(args.iter().copied()) + .map(str::to_string) + .collect() } fn filter_override_env(driver: &str) -> HashMap> { @@ -619,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)] @@ -638,17 +826,21 @@ 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)); - assert_eq!(command.timeout, DIFF_COMMAND_TIMEOUT); - assert!(command.disable_output_cap); + 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); + } else { + assert_eq!(command.timeout, DIFF_COMMAND_TIMEOUT); + assert_eq!(command.disable_output_cap, true); + } } } @@ -671,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() } }