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() } }