diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index e01903aa16..f26c8e7dd9 100644 --- a/codex-rs/git-utils/src/apply.rs +++ b/codex-rs/git-utils/src/apply.rs @@ -16,6 +16,7 @@ use crate::apply_output::parse_git_apply_output; use crate::git_command::GitRunner; use crate::guarded_config::GuardedGitConfig; use crate::patch_paths::extract_patch_path_inventory_guarded; +use crate::reverse_staging::ReverseApplyMode; use crate::reverse_staging::stage_effective_paths_for_reverse; #[cfg(test)] use crate::safe_git::DISABLED_HOOKS_PATH; @@ -43,6 +44,21 @@ pub struct ApplyGitResult { pub cmd_for_log: String, } +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum ApplyStrategy { + Direct, + ThreeWay, +} + +impl ApplyStrategy { + fn reverse_mode(self) -> ReverseApplyMode { + match self { + Self::Direct => ReverseApplyMode::Direct, + Self::ThreeWay => ReverseApplyMode::ThreeWay, + } + } +} + /// Apply a unified diff to the target repository by shelling out to `git apply`. /// /// When [`ApplyGitRequest::preflight`] is `true`, this behaves like `git apply --check` and @@ -111,25 +127,43 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { plain_check_args.push(patch_arg.clone()); let (plain_check_code, _, _) = run_guarded_apply(&config, &plain_check_args)?; - let mut args = vec!["apply".to_string()]; - if plain_check_code != 0 { + let strategy = if plain_check_code != 0 { config.install_three_way_merge_policy()?; - args.push("--3way".to_string()); + ApplyStrategy::ThreeWay } else { - args.push("--index".to_string()); - } + ApplyStrategy::Direct + }; if req.revert { // Stage only after the worktree-only applicability decision. The // guarded config is shared with the final Git command. - stage_effective_paths_for_reverse(&mut config, patch_paths)?; + stage_effective_paths_for_reverse(&mut config, patch_paths, strategy.reverse_mode())?; } - if req.revert { - args.push("-R".to_string()); - } - args.push(patch_arg); - let cmd_for_log = config.render_command_for_log(&args)?; - let (code, stdout, stderr) = run_guarded_apply(&config, &args)?; + let (cmd_for_log, output) = match strategy { + ApplyStrategy::Direct => { + let mut args = vec!["apply".to_string(), "--index".to_string()]; + if req.revert { + args.push("-R".to_string()); + } + args.push(patch_arg); + let rendered = config.render_command_for_log(&args)?; + let (code, stdout, stderr) = run_guarded_apply(&config, &args)?; + (rendered, (code, stdout, stderr)) + } + ApplyStrategy::ThreeWay => { + let rendered = config.render_three_way_apply_for_log(req.revert, &patch_arg)?; + let output = config.run_three_way_apply(req.revert, &patch_arg)?; + ( + rendered, + ( + output.status.code().unwrap_or(-1), + String::from_utf8_lossy(&output.stdout).into_owned(), + String::from_utf8_lossy(&output.stderr).into_owned(), + ), + ) + } + }; + let (code, stdout, stderr) = output; let (mut applied_paths, mut skipped_paths, mut conflicted_paths) = if code == 0 { (patch_path_inventory.primary_paths, Vec::new(), Vec::new()) diff --git a/codex-rs/git-utils/src/git_command.rs b/codex-rs/git-utils/src/git_command.rs index eb39ded9e7..f04bfd7d2d 100644 --- a/codex-rs/git-utils/src/git_command.rs +++ b/codex-rs/git-utils/src/git_command.rs @@ -41,6 +41,73 @@ pub(crate) struct GitCommand { inner: Command, } +/// App-owned common repository metadata for one final three-way apply. +/// +/// The real per-worktree Git directory remains selected for HEAD and index +/// state, while this directory replaces every common config and attribute +/// source that could define a repository-selected executable helper. +pub(crate) struct IsolatedGitCommonDir { + root: tempfile::TempDir, +} + +impl IsolatedGitCommonDir { + pub(crate) fn config_path(&self) -> PathBuf { + self.root.path().join("config") + } + + fn system_config_path(&self) -> PathBuf { + self.root.path().join("system.gitconfig") + } + + fn global_config_path(&self) -> PathBuf { + self.root.path().join("global.gitconfig") + } + + fn home_path(&self) -> PathBuf { + self.root.path().join("home") + } + + fn xdg_config_home(&self) -> PathBuf { + self.root.path().join("xdg") + } + + fn validate(&self) -> io::Result<()> { + for path in [ + self.config_path(), + self.system_config_path(), + self.global_config_path(), + self.root.path().join("info/attributes"), + ] { + let metadata = std::fs::symlink_metadata(&path)?; + if !metadata.is_file() || metadata.file_type().is_symlink() { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + format!("isolated Git metadata file changed at {}", path.display()), + )); + } + } + for path in [ + self.root.path().join("objects"), + self.root.path().join("refs"), + self.root.path().join("info"), + self.home_path(), + self.xdg_config_home(), + ] { + let metadata = std::fs::symlink_metadata(&path)?; + if !metadata.is_dir() || metadata.file_type().is_symlink() { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + format!( + "isolated Git metadata directory changed at {}", + path.display() + ), + )); + } + } + Ok(()) + } +} + impl GitCommand { pub(crate) fn arg(&mut self, arg: impl AsRef) -> &mut Self { self.inner.arg(arg); @@ -134,6 +201,29 @@ impl GitRunner { self.config_environment.value(name) } + pub(crate) fn create_isolated_common_dir(&self) -> io::Result { + let root = tempfile::tempdir()?; + self.authority + .ensure_config_source_is_not_worktree_controlled( + root.path(), + "owned isolated Git common directory", + )?; + for path in ["objects", "refs", "info", "home", "xdg"] { + std::fs::create_dir_all(root.path().join(path))?; + } + for path in [ + "config", + "system.gitconfig", + "global.gitconfig", + "info/attributes", + ] { + std::fs::write(root.path().join(path), [])?; + } + let isolated = IsolatedGitCommonDir { root }; + isolated.validate()?; + Ok(isolated) + } + pub(crate) fn output(&self, mut command: GitCommand) -> io::Result { self.revalidate_active_repository_metadata()?; isolate_git_command_environment(&mut command.inner); @@ -142,6 +232,60 @@ impl GitRunner { command.inner.output() } + pub(crate) fn output_in_isolated_common_dir( + &self, + mut command: GitCommand, + isolated: &IsolatedGitCommonDir, + ) -> io::Result { + self.revalidate_active_repository_metadata()?; + isolated.validate()?; + self.authority + .ensure_config_source_is_not_worktree_controlled( + isolated.root.path(), + "owned isolated Git common directory", + )?; + isolate_git_command_environment(&mut command.inner); + scrub_repository_and_config_environment(&mut command.inner); + + let git_dir = self.authority.active_git_dir().ok_or_else(|| { + io::Error::new( + io::ErrorKind::PermissionDenied, + "active Git directory is unavailable for isolated three-way apply", + ) + })?; + let common_dir = self.authority.active_common_dir().ok_or_else(|| { + io::Error::new( + io::ErrorKind::PermissionDenied, + "active Git common directory is unavailable for isolated three-way apply", + ) + })?; + let worktree = self.authority.active_worktree_root(); + command + .inner + .env("GIT_DIR", git_dir) + .env("GIT_COMMON_DIR", isolated.root.path()) + .env("GIT_WORK_TREE", worktree) + .env("GIT_INDEX_FILE", git_dir.join("index")) + .env("GIT_OBJECT_DIRECTORY", common_dir.join("objects")) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_CONFIG_SYSTEM", isolated.system_config_path()) + .env("GIT_CONFIG_GLOBAL", isolated.global_config_path()) + .env("GIT_CONFIG_COUNT", "0") + .env("GIT_ATTR_NOSYSTEM", "1") + .env("GIT_NO_REPLACE_OBJECTS", "1") + .env("HOME", isolated.home_path()) + .env("XDG_CONFIG_HOME", isolated.xdg_config_home()); + #[cfg(windows)] + command + .inner + .env("APPDATA", isolated.home_path()) + .env("PROGRAMDATA", isolated.home_path()) + .env("USERPROFILE", isolated.home_path()); + command.inner.envs(crate::local_only_git_env()); + harden_git_launch_environment(&mut command.inner, &self.safe_path); + command.inner.output() + } + fn revalidate_active_repository_metadata(&self) -> io::Result<()> { self.authority.revalidate_active_repository_metadata() } @@ -168,6 +312,63 @@ impl GitRunner { } } +fn scrub_repository_and_config_environment(command: &mut Command) { + let mut names = std::env::vars_os() + .map(|(name, _)| name) + .filter(|name| isolated_launch_variable(name)) + .collect::>(); + names.extend( + command + .get_envs() + .filter(|&(name, _)| isolated_launch_variable(name)) + .map(|(name, _)| name.to_os_string()), + ); + names.sort(); + names.dedup(); + for name in names { + command.env_remove(name); + } +} + +fn isolated_launch_variable(name: &OsStr) -> bool { + let name = name.to_string_lossy().to_ascii_uppercase(); + matches!( + name.as_str(), + "GIT_DIR" + | "GIT_COMMON_DIR" + | "GIT_WORK_TREE" + | "GIT_INDEX_FILE" + | "GIT_INDEX_VERSION" + | "GIT_OBJECT_DIRECTORY" + | "GIT_ALTERNATE_OBJECT_DIRECTORIES" + | "GIT_NAMESPACE" + | "GIT_QUARANTINE_PATH" + | "GIT_GRAFT_FILE" + | "GIT_SHALLOW_FILE" + | "GIT_REPLACE_REF_BASE" + | "GIT_NO_REPLACE_OBJECTS" + | "GIT_ATTR_SOURCE" + | "GIT_ATTR_NOSYSTEM" + | "GIT_CONFIG" + | "GIT_CONFIG_GLOBAL" + | "GIT_CONFIG_SYSTEM" + | "GIT_CONFIG_NOSYSTEM" + | "GIT_CONFIG_COUNT" + | "GIT_CONFIG_PARAMETERS" + | "GIT_DEFAULT_HASH" + | "GIT_DEFAULT_REF_FORMAT" + | "GIT_REFERENCE_BACKEND" + | "HOME" + | "XDG_CONFIG_HOME" + | "APPDATA" + | "PROGRAMDATA" + | "USERPROFILE" + | "HOMEDRIVE" + | "HOMEPATH" + ) || name.starts_with("GIT_CONFIG_KEY_") + || name.starts_with("GIT_CONFIG_VALUE_") +} + #[cfg(test)] thread_local! { static GIT_RUNNER_CONSTRUCTION_COUNT: std::cell::Cell = const { std::cell::Cell::new(0) }; diff --git a/codex-rs/git-utils/src/guarded_config.rs b/codex-rs/git-utils/src/guarded_config.rs index 33c82f7b5c..7f5f5f9098 100644 --- a/codex-rs/git-utils/src/guarded_config.rs +++ b/codex-rs/git-utils/src/guarded_config.rs @@ -364,9 +364,6 @@ impl<'git> GuardedGitConfig<'git> { if let Some(neutralizer) = apply.and_then(FilterPolicySnapshot::neutralizer) { neutralizer.append_to(&self.identity, &mut command)?; } - if let Some(merge) = &self.merge { - merge.append_to(&self.identity, &mut command)?; - } if let Some(neutralizer) = git_add.and_then(FilterPolicySnapshot::neutralizer) { neutralizer.append_to(&self.identity, &mut command)?; } @@ -550,11 +547,82 @@ impl<'git> GuardedGitConfig<'git> { Ok(apply.checked_paths()) } + pub(crate) fn run_three_way_apply( + &self, + revert: bool, + patch_path: &str, + ) -> io::Result { + let (apply, _git_add) = self.ordered_filter_snapshots()?; + if apply.is_none() || !self.merge_policy_installed { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "three-way apply policy is not installed", + )); + } + let merge = self.merge.as_ref().ok_or_else(|| { + io::Error::new( + io::ErrorKind::PermissionDenied, + "isolated three-way config is unavailable", + ) + })?; + let isolated = merge.common_dir(&self.identity)?; + let mut command = self + .sources + .git + .command_for_cwd(&self.sources.canonical_root)?; + append_safe_scalar_overrides(&mut command); + BoundSubcommand::Apply.append_to(&mut command); + command.arg("--3way"); + if revert { + command.arg("-R"); + } + command.arg(patch_path); + self.sources + .git + .output_in_isolated_common_dir(command, isolated) + } + + pub(crate) fn render_three_way_apply_for_log( + &self, + revert: bool, + patch_path: &str, + ) -> io::Result { + let merge = self.merge.as_ref().ok_or_else(|| { + io::Error::new( + io::ErrorKind::PermissionDenied, + "isolated three-way config is unavailable", + ) + })?; + let _ = merge.common_dir(&self.identity)?; + let mut parts = vec![ + "env".to_string(), + "GIT_COMMON_DIR=".to_string(), + "git".to_string(), + ]; + parts.extend(safe_scalar_override_args()); + parts.push("apply".to_string()); + parts.push("--3way".to_string()); + if revert { + parts.push("-R".to_string()); + } + parts.push(patch_path.to_string()); + Ok(format!( + "(cd {} && {})", + quote_shell(&self.sources.canonical_root.display().to_string()), + parts + .into_iter() + .map(|part| quote_shell(&part)) + .collect::>() + .join(" ") + )) + } + #[cfg(test)] - pub(crate) fn merge_include_arg(&self) -> Option<&str> { + pub(crate) fn merge_common_config_path(&self) -> Option { self.merge .as_ref() - .map(SealedMergeConfigOverride::include_arg) + .and_then(|merge| merge.common_dir(&self.identity).ok()) + .map(crate::git_command::IsolatedGitCommonDir::config_path) } pub(crate) fn authorize_git_add_filter_paths(&mut self, paths: &[String]) -> io::Result<()> { @@ -581,9 +649,6 @@ impl<'git> GuardedGitConfig<'git> { if let Some(neutralizer) = apply.and_then(FilterPolicySnapshot::neutralizer) { neutralizer.append_rendered_args(&self.identity, &mut parts)?; } - if let Some(merge) = &self.merge { - merge.append_rendered_args(&self.identity, &mut parts)?; - } if let Some(neutralizer) = git_add.and_then(FilterPolicySnapshot::neutralizer) { neutralizer.append_rendered_args(&self.identity, &mut parts)?; } diff --git a/codex-rs/git-utils/src/guarded_config/merge_overlay.rs b/codex-rs/git-utils/src/guarded_config/merge_overlay.rs index 355b6e520b..02f553b562 100644 --- a/codex-rs/git-utils/src/guarded_config/merge_overlay.rs +++ b/codex-rs/git-utils/src/guarded_config/merge_overlay.rs @@ -7,10 +7,11 @@ use std::sync::Arc; use super::BoundSubcommand; use super::CapabilityIdentity; use super::GuardedGitConfig; -use crate::git_command::GitCommand; +use crate::git_command::IsolatedGitCommonDir; use crate::git_config::GitConfigEntry; const MERGE_CONFIG_PATTERN: &str = r"^(merge\.default|merge\..*\.driver)$"; +const SANITIZED_CONFIG_PATTERN: &str = r"^(core\.(repositoryformatversion|filemode|symlinks|ignorecase|precomposeunicode|protecthfs|protectntfs|trustctime|checkstat|longpaths|fscache|splitindex|sparsecheckout|sparsecheckoutcone|autocrlf|eol|safecrlf|checkroundtripencoding|bigfilethreshold|whitespace)|extensions\.(objectformat|compatobjectformat)|index\.(sparse|version)|apply\.(whitespace|ignorewhitespace)|merge\.conflictstyle)$"; /// A complete, fresh merge-config read bound to one authorized operation. /// @@ -36,60 +37,37 @@ impl MergeConfigSnapshot { Ok(()) } - fn nonempty_driver_keys(&self) -> io::Result> { - let mut keys = Vec::new(); - for entry in self.entries.values() { - if entry.key == "merge.default" { - continue; - } - let _driver = entry - .key - .strip_prefix("merge.") - .and_then(|key| key.strip_suffix(".driver")) - .ok_or_else(|| { - io::Error::new( - io::ErrorKind::InvalidData, - "malformed effective Git merge-driver key", - ) - })?; - if !entry.value.is_empty() { - keys.push(entry.key.as_str()); - } + fn sanitized_default_driver(&self) -> &str { + match self + .entries + .get("merge.default") + .map(|entry| entry.value.as_str()) + { + Some("binary") => "binary", + Some("union") => "union", + _ => "text", } - Ok(keys) } } -/// A sealed merge-driver-only include bound to one operation. +/// A sealed, helper-free common repository view bound to one operation. /// -/// Construction accepts only a complete `MergeConfigSnapshot`, and the type -/// exposes neither raw keys nor its include argument outside this module. +/// The real Git directory, index, worktree, and object store remain selected; +/// only common config and attributes are replaced for the final three-way +/// child. Construction accepts a complete merge snapshot so a caller cannot +/// attach an unreviewed or partial view. pub(super) struct SealedMergeConfigOverride { owner: Arc, - include_arg: String, - _config_dir: tempfile::TempDir, + common_dir: IsolatedGitCommonDir, } impl SealedMergeConfigOverride { - pub(super) fn append_to( + pub(super) fn common_dir( &self, owner: &Arc, - command: &mut GitCommand, - ) -> io::Result<()> { + ) -> io::Result<&IsolatedGitCommonDir> { self.ensure_owner(owner)?; - command.args(["-c", &self.include_arg]); - Ok(()) - } - - pub(super) fn append_rendered_args( - &self, - owner: &Arc, - args: &mut Vec, - ) -> io::Result<()> { - self.ensure_owner(owner)?; - args.push("-c".to_string()); - args.push(self.include_arg.clone()); - Ok(()) + Ok(&self.common_dir) } fn ensure_owner(&self, owner: &Arc) -> io::Result<()> { @@ -101,11 +79,6 @@ impl SealedMergeConfigOverride { } Ok(()) } - - #[cfg(test)] - pub(super) fn include_arg(&self) -> &str { - &self.include_arg - } } impl<'git> GuardedGitConfig<'git> { @@ -135,8 +108,8 @@ impl<'git> GuardedGitConfig<'git> { ), )); } - let neutralizer = self.build_merge_override(&snapshot)?; - self.attach_merge_override(neutralizer) + let isolated = self.build_merge_override(&snapshot)?; + self.attach_merge_override(isolated) } /// Read merge-driver policy from the frozen, authorized base invocation. @@ -188,48 +161,43 @@ impl<'git> GuardedGitConfig<'git> { fn build_merge_override( &self, snapshot: &MergeConfigSnapshot, - ) -> io::Result> { + ) -> io::Result { snapshot.ensure_owner(&self.identity)?; if self.merge_policy_installed { return Err(io::Error::new( io::ErrorKind::PermissionDenied, - "a merge neutralizer is already attached", + "an isolated three-way config is already attached", )); } - let driver_keys = snapshot.nonempty_driver_keys()?; - if driver_keys.is_empty() { - return Ok(None); - } - let config_dir = tempfile::tempdir()?; - let config_path = config_dir - .path() - .join("merge-driver-neutralization.gitconfig"); - self.ensure_owned_config_path(&config_path, "owned Git merge-driver neutralization")?; - std::fs::write(&config_path, [])?; - for key in driver_keys { - self.write_merge_override_value(&config_path, key)?; + let common_dir = self.sources.git.create_isolated_common_dir()?; + let config_path = common_dir.config_path(); + self.ensure_owned_config_path(&config_path, "owned isolated Git common config")?; + let entries = self + .sources + .read_effective(SANITIZED_CONFIG_PATTERN, "three-way allowlist")?; + for entry in entries.values() { + self.write_sanitized_config_value(&config_path, &entry.key, &entry.value)?; } - let config_path = config_path.to_str().ok_or_else(|| { - io::Error::new(io::ErrorKind::InvalidData, "non-UTF-8 merge guard path") - })?; + self.write_sanitized_config_value(&config_path, "core.bare", "false")?; + self.write_sanitized_config_value( + &config_path, + "merge.default", + snapshot.sanitized_default_driver(), + )?; #[cfg(test)] MERGE_OVERLAY_COUNT.with(|count| count.set(count.get() + 1)); - Ok(Some(SealedMergeConfigOverride { + Ok(SealedMergeConfigOverride { owner: Arc::clone(&self.identity), - include_arg: format!("include.path={config_path}"), - _config_dir: config_dir, - })) + common_dir, + }) } - fn attach_merge_override( - &mut self, - neutralizer: Option, - ) -> io::Result<()> { + fn attach_merge_override(&mut self, isolated: SealedMergeConfigOverride) -> io::Result<()> { if self.merge_policy_installed { return Err(io::Error::new( io::ErrorKind::PermissionDenied, - "a second merge neutralizer is not permitted", + "a second isolated three-way config is not permitted", )); } let [apply] = self.filters.as_slice() else { @@ -244,28 +212,30 @@ impl<'git> GuardedGitConfig<'git> { "merge policy requires an apply filter snapshot", )); } - if let Some(neutralizer) = &neutralizer { - neutralizer.ensure_owner(&self.identity)?; - } - self.merge = neutralizer; + isolated.ensure_owner(&self.identity)?; + self.merge = Some(isolated); self.merge_policy_installed = true; Ok(()) } - fn write_merge_override_value(&self, config_path: &Path, key: &str) -> io::Result<()> { + fn write_sanitized_config_value( + &self, + config_path: &Path, + key: &str, + value: &str, + ) -> io::Result<()> { let mut command = self .sources .git .command_for_cwd(&self.sources.canonical_root)?; command - .args(&self.sources.base_config_args) .args(["config", "--file"]) .arg(config_path) - .args(["--add", key, ""]); + .args(["--add", key, value]); let output = self.sources.git.output(command)?; if !output.status.success() { return Err(io::Error::other(format!( - "failed to write Git merge-driver neutralization for {key:?} (status {}): {}", + "failed to write isolated Git config value {key:?} (status {}): {}", output.status, String::from_utf8_lossy(&output.stderr).trim() ))); diff --git a/codex-rs/git-utils/src/guarded_config/merge_overlay_tests.rs b/codex-rs/git-utils/src/guarded_config/merge_overlay_tests.rs index 069e4ff8c5..39fa26e970 100644 --- a/codex-rs/git-utils/src/guarded_config/merge_overlay_tests.rs +++ b/codex-rs/git-utils/src/guarded_config/merge_overlay_tests.rs @@ -35,10 +35,9 @@ fn sealed_merge_override_rejects_another_operation_identity() { let snapshot = first.read_merge_config_snapshot().expect("merge snapshot"); let neutralizer = first .build_merge_override(&snapshot) - .expect("build merge override") - .expect("physical merge override"); + .expect("build isolated merge config"); let error = second - .attach_merge_override(Some(neutralizer)) + .attach_merge_override(neutralizer) .expect_err("cross-operation override must refuse"); assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); } diff --git a/codex-rs/git-utils/src/guarded_config_tests.rs b/codex-rs/git-utils/src/guarded_config_tests.rs index d849e35627..dac7b0b7f2 100644 --- a/codex-rs/git-utils/src/guarded_config_tests.rs +++ b/codex-rs/git-utils/src/guarded_config_tests.rs @@ -183,7 +183,7 @@ fn zero_driver_merge_policy_still_occupies_the_single_install_slot() { .install_three_way_merge_policy() .expect("zero-driver merge policy"); assert!(config.merge_policy_installed); - assert!(config.merge.is_none()); + assert!(config.merge.is_some()); let error = config .install_three_way_merge_policy() @@ -192,10 +192,10 @@ fn zero_driver_merge_policy_still_occupies_the_single_install_slot() { } #[test] -fn command_order_is_apply_filter_then_merge_then_git_add_filter() { +fn isolated_merge_config_is_not_attached_to_preparatory_commands() { if std::env::var_os("CODEX_GIT_UTILS_GUARDED_CONFIG_ENV_CHILD").is_none() { run_isolated_config_test( - "guarded_config::tests::command_order_is_apply_filter_then_merge_then_git_add_filter", + "guarded_config::tests::isolated_merge_config_is_not_attached_to_preparatory_commands", ); return; } @@ -226,26 +226,24 @@ fn command_order_is_apply_filter_then_merge_then_git_add_filter() { .expect("apply filter overlay") .include_arg .clone(); - let merge_include = config - .merge_include_arg() - .expect("merge overlay") - .to_string(); + let merge_config = config + .merge_common_config_path() + .expect("isolated merge config"); let git_add_include = config.filters[1] .neutralizer() .expect("Git-add filter overlay") .include_arg .clone(); - let overlay_paths = [&apply_include, &merge_include, &git_add_include] + let overlay_paths = [&apply_include, &git_add_include] .map(|include| PathBuf::from(include.strip_prefix("include.path=").expect("include path"))); let rendered = config .render_command_for_log(&["ls-files".to_string()]) .expect("render command"); let apply_offset = rendered.find(&apply_include).expect("apply include"); - let merge_offset = rendered.find(&merge_include).expect("merge include"); let git_add_offset = rendered.find(&git_add_include).expect("Git-add include"); - assert!(apply_offset < merge_offset && merge_offset < git_add_offset); - assert_eq!(rendered.matches(&merge_include).count(), 1); + assert!(apply_offset < git_add_offset); + assert!(!rendered.contains(&merge_config.display().to_string())); let output = config .ls_files_command() @@ -254,9 +252,11 @@ fn command_order_is_apply_filter_then_merge_then_git_add_filter() { .expect("execute bound command"); assert!(output.status.success()); assert!(overlay_paths.iter().all(|path| path.is_file())); + assert!(merge_config.is_file()); drop(config); assert!(overlay_paths.iter().all(|path| !path.exists())); + assert!(!merge_config.exists()); } #[test] diff --git a/codex-rs/git-utils/src/merge_driver_race_tests.rs b/codex-rs/git-utils/src/merge_driver_race_tests.rs index 53d45f5b6a..07a8e5a697 100644 --- a/codex-rs/git-utils/src/merge_driver_race_tests.rs +++ b/codex-rs/git-utils/src/merge_driver_race_tests.rs @@ -2,6 +2,7 @@ use super::*; use crate::apply::ApplyGitRequest; use crate::apply::apply_git_patch; use pretty_assertions::assert_eq; +use std::ffi::OsStr; use std::fs::OpenOptions; use std::io::Write; use std::path::Path; @@ -10,8 +11,8 @@ use std::thread; use std::time::Duration; use std::time::Instant; -const OLD_DRIVER_COMMAND: &str = "git config codex.oldmergeran true && false"; -const NEW_DRIVER_COMMAND: &str = "git config codex.newmergeran true && false"; +const OLD_DRIVER_COMMAND: &str = "git config --file .git/config codex.oldmergeran true && false"; +const NEW_DRIVER_COMMAND: &str = "git config --file .git/config codex.newmergeran true && false"; const TRACE_MUTATION_MARKER: &str = "CODEX_MERGE_DRIVER_TEST_MUTATION"; fn run(cwd: &Path, args: &[&str]) -> (i32, String, String) { @@ -109,6 +110,10 @@ fn index_tree(root: &Path) -> String { } fn run_isolated_test(test_name: &str) { + run_isolated_test_with_env(test_name, &[]); +} + +fn run_isolated_test_with_env(test_name: &str, extra_env: &[(&str, &OsStr)]) { let environment = tempfile::tempdir().expect("isolated Git environment"); let global_config = environment.path().join("global.gitconfig"); let system_config = environment.path().join("system.gitconfig"); @@ -119,7 +124,7 @@ fn run_isolated_test(test_name: &str) { let mut command = std::process::Command::new(std::env::current_exe().expect("test binary")); isolate_git_command_environment(&mut command); - let output = command + command .arg(test_name) .arg("--exact") .arg("--nocapture") @@ -127,9 +132,11 @@ fn run_isolated_test(test_name: &str) { .env("GIT_CONFIG_GLOBAL", &global_config) .env("GIT_CONFIG_SYSTEM", &system_config) .env("GIT_TRACE2_EVENT", &trace) - .env("RUST_TEST_THREADS", "1") - .output() - .expect("run isolated test process"); + .env("RUST_TEST_THREADS", "1"); + for (name, value) in extra_env { + command.env(name, value); + } + let output = command.output().expect("run isolated test process"); assert!( output.status.success(), "isolated test {test_name} failed:\nstdout:\n{}\nstderr:\n{}", @@ -189,6 +196,26 @@ fn append_driver_config(root: &Path, driver: &str, command: &str) { .expect("append replacement merge driver"); } +fn append_default_and_driver_config(root: &Path, driver: &str, command: &str) { + let mut config = OpenOptions::new() + .append(true) + .open(root.join(".git/config")) + .expect("open local config"); + writeln!( + config, + "\n[merge]\n\tdefault = {driver}\n[merge \"{driver}\"]\n\tdriver = {command}" + ) + .expect("append replacement default merge driver"); +} + +#[derive(Clone, Copy, Debug)] +enum DriverNamespaceRace { + EmptyKnownKey, + SelectedMissingKey, + BrandNewAttributeAndKey, + BrandNewDefaultAndKey, +} + #[test] fn clean_reverse_checks_worktree_before_staging_and_skips_merge_driver() { let repo = init_repo(); @@ -378,6 +405,164 @@ fn post_probe_attribute_and_same_driver_command_change_stays_neutralized() { assert!(!configured_marker_exists(root, "codex.newmergeran")); } +#[test] +fn isolated_three_way_config_blocks_driver_namespace_introduction_races() { + if std::env::var_os("CODEX_GIT_UTILS_MERGE_RACE_CHILD").is_none() { + run_isolated_test( + "merge_driver::race_tests::isolated_three_way_config_blocks_driver_namespace_introduction_races", + ); + return; + } + + for race in [ + DriverNamespaceRace::EmptyKnownKey, + DriverNamespaceRace::SelectedMissingKey, + DriverNamespaceRace::BrandNewAttributeAndKey, + DriverNamespaceRace::BrandNewDefaultAndKey, + ] { + let repo = init_repo(); + let root = repo.path(); + let attributes = match race { + DriverNamespaceRace::EmptyKnownKey | DriverNamespaceRace::SelectedMissingKey => { + "target.txt merge=demo\n" + } + DriverNamespaceRace::BrandNewAttributeAndKey + | DriverNamespaceRace::BrandNewDefaultAndKey => "# initially safe\n", + }; + let patch = build_conflicting_patch(root, attributes); + if matches!(race, DriverNamespaceRace::EmptyKnownKey) { + configure_driver(root, "demo", ""); + } + let trace = trace_path(); + std::fs::write(&trace, "").expect("clear fixture trace"); + let watcher_trace = trace.clone(); + let watcher_root = root.to_path_buf(); + let watcher = thread::spawn(move || { + let observed = wait_for_merge_attribute_probe(&watcher_trace); + if observed { + match race { + DriverNamespaceRace::EmptyKnownKey + | DriverNamespaceRace::SelectedMissingKey => { + append_driver_config(&watcher_root, "demo", NEW_DRIVER_COMMAND); + } + DriverNamespaceRace::BrandNewAttributeAndKey => { + append_driver_config(&watcher_root, "brandnew", NEW_DRIVER_COMMAND); + std::fs::write( + watcher_root.join(".gitattributes"), + "target.txt merge=brandnew\n", + ) + .expect("select brand-new merge driver"); + } + DriverNamespaceRace::BrandNewDefaultAndKey => { + append_default_and_driver_config( + &watcher_root, + "brandnew", + NEW_DRIVER_COMMAND, + ); + } + } + record_trace_mutation(&watcher_trace); + } + observed + }); + + let result = apply_git_patch(&request(root, patch, /*revert*/ false)) + .expect("run isolated three-way apply"); + assert!(watcher.join().expect("namespace watcher"), "{race:?}"); + + assert_mutation_happened_between_probe_and_three_way(&trace); + assert_ne!(result.exit_code, 0, "{race:?}"); + assert!(result.cmd_for_log.contains("GIT_COMMON_DIR=")); + assert!(!configured_marker_exists(root, "codex.oldmergeran")); + assert!(!configured_marker_exists(root, "codex.newmergeran")); + } +} + +#[test] +fn isolated_three_way_config_scrubs_command_config_channels() { + if std::env::var_os("CODEX_GIT_UTILS_MERGE_RACE_CHILD").is_none() { + run_isolated_test_with_env( + "merge_driver::race_tests::isolated_three_way_config_scrubs_command_config_channels", + &[ + ("GIT_CONFIG_COUNT", OsStr::new("1")), + ("GIT_CONFIG_KEY_0", OsStr::new("merge.env-selected.driver")), + ( + "GIT_CONFIG_VALUE_0", + OsStr::new("git config --file .git/config codex.envmergeran true && false"), + ), + ( + "CODEX_APPLY_GIT_CFG", + OsStr::new( + "merge.base-selected.driver=git config --file .git/config codex.basemergeran true && false", + ), + ), + ], + ); + return; + } + + for driver in ["env-selected", "base-selected"] { + let repo = init_repo(); + let root = repo.path(); + let patch = build_conflicting_patch(root, "# initially safe\n"); + let trace = trace_path(); + std::fs::write(&trace, "").expect("clear fixture trace"); + let watcher_trace = trace.clone(); + let attributes = root.join(".gitattributes"); + let watcher = thread::spawn(move || { + let observed = wait_for_merge_attribute_probe(&watcher_trace); + if observed { + std::fs::write(attributes, format!("target.txt merge={driver}\n")) + .expect("select command-config merge driver"); + record_trace_mutation(&watcher_trace); + } + observed + }); + + let result = apply_git_patch(&request(root, patch, /*revert*/ false)) + .expect("run isolated three-way apply"); + assert!(watcher.join().expect("config-channel watcher"), "{driver}"); + + assert_mutation_happened_between_probe_and_three_way(&trace); + assert_ne!(result.exit_code, 0, "{driver}"); + assert!(!configured_marker_exists(root, "codex.envmergeran")); + assert!(!configured_marker_exists(root, "codex.basemergeran")); + } +} + +#[test] +fn isolated_three_way_config_updates_the_linked_worktree_index() { + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("seed.txt"), "seed\n").expect("write seed"); + run_success(root, &["add", "seed.txt"]); + run_success(root, &["commit", "-m", "seed"]); + + let linked_parent = tempfile::tempdir().expect("linked worktree parent"); + let linked_root = linked_parent.path().join("linked"); + run_success( + root, + &[ + "worktree", + "add", + "-b", + "isolated-three-way-linked", + linked_root.to_str().expect("UTF-8 linked path"), + ], + ); + let patch = build_conflicting_patch(&linked_root, "# no custom merge driver\n"); + + let result = apply_git_patch(&request(&linked_root, patch, /*revert*/ false)) + .expect("run linked-worktree three-way apply"); + + assert_ne!(result.exit_code, 0); + assert!(result.cmd_for_log.contains("GIT_COMMON_DIR=")); + let unmerged = run_success(&linked_root, &["ls-files", "--unmerged", "target.txt"]); + assert!(unmerged.contains(" 1\ttarget.txt"), "{unmerged}"); + assert!(unmerged.contains(" 2\ttarget.txt"), "{unmerged}"); + assert!(unmerged.contains(" 3\ttarget.txt"), "{unmerged}"); +} + #[test] fn same_patch_attribute_activation_does_not_run_merge_driver() { let repo = init_repo(); diff --git a/codex-rs/git-utils/src/merge_driver_tests.rs b/codex-rs/git-utils/src/merge_driver_tests.rs index ba87579397..1ec4aad060 100644 --- a/codex-rs/git-utils/src/merge_driver_tests.rs +++ b/codex-rs/git-utils/src/merge_driver_tests.rs @@ -23,7 +23,7 @@ fn apply_allows_unused_global_merge_driver() { let system_config = config_dir.path().join("system.gitconfig"); std::fs::write( &global_config, - "[merge \"unused\"]\n\tdriver = git config codex.mergeran true && false\n", + "[merge \"unused\"]\n\tdriver = git config --file .git/config codex.mergeran true && false\n", ) .expect("write global config"); std::fs::write(&system_config, "").expect("write system config"); @@ -71,7 +71,7 @@ fn apply_rejects_global_merge_driver_before_three_way() { let system_config = config_dir.path().join("system.gitconfig"); std::fs::write( &global_config, - "[merge \"codex-test\"]\n\tdriver = git config codex.mergeran true && false\n", + "[merge \"codex-test\"]\n\tdriver = git config --file .git/config codex.mergeran true && false\n", ) .expect("write global config"); std::fs::write(&system_config, "").expect("write system config"); @@ -173,7 +173,7 @@ fn three_way_apply_allows_unrelated_local_merge_driver() { "git", "config", "merge.unused.driver", - "git config codex.mergeran true && false", + "git config --file .git/config codex.mergeran true && false", ], ); assert_eq!( @@ -206,6 +206,139 @@ fn three_way_apply_allows_unrelated_local_merge_driver() { assert_ne!(marker_code, 0, "unused merge driver must not run"); } +#[test] +fn isolated_three_way_config_preserves_builtin_union_default() { + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("file.txt"), "top\nbase\nbottom\n").expect("write base"); + let (add_code, _, add_err) = run(root, &["git", "add", "file.txt"]); + assert_eq!(add_code, 0, "add base: {add_err}"); + let (commit_code, _, commit_err) = run(root, &["git", "commit", "-m", "base"]); + assert_eq!(commit_code, 0, "commit base: {commit_err}"); + let (base_code, base, base_err) = run(root, &["git", "rev-parse", "HEAD"]); + assert_eq!(base_code, 0, "resolve base: {base_err}"); + + std::fs::write(root.join("file.txt"), "top\ntheirs\nbottom\n").expect("write theirs"); + let (add_code, _, add_err) = run(root, &["git", "add", "file.txt"]); + assert_eq!(add_code, 0, "add theirs: {add_err}"); + let (commit_code, _, commit_err) = run(root, &["git", "commit", "-m", "theirs"]); + assert_eq!(commit_code, 0, "commit theirs: {commit_err}"); + let (diff_code, diff, diff_err) = run( + root, + &[ + "git", + "diff", + "--full-index", + base.trim(), + "HEAD", + "--", + "file.txt", + ], + ); + assert_eq!(diff_code, 0, "create full-index patch: {diff_err}"); + + let (checkout_code, _, checkout_err) = + run(root, &["git", "checkout", "-b", "ours", base.trim()]); + assert_eq!(checkout_code, 0, "checkout base: {checkout_err}"); + std::fs::write(root.join("file.txt"), "top\nours\nbottom\n").expect("write ours"); + let (commit_code, _, commit_err) = run(root, &["git", "commit", "-am", "ours"]); + assert_eq!(commit_code, 0, "commit ours: {commit_err}"); + let (config_code, _, config_err) = run(root, &["git", "config", "merge.default", "union"]); + assert_eq!(config_code, 0, "configure union default: {config_err}"); + + let result = apply_git_patch(&ApplyGitRequest { + cwd: root.to_path_buf(), + diff, + revert: false, + preflight: false, + }) + .expect("apply with builtin union default"); + + assert_eq!(result.exit_code, 0, "{}", result.stderr); + assert!(result.cmd_for_log.contains("GIT_COMMON_DIR=")); + let contents = std::fs::read_to_string(root.join("file.txt")) + .expect("read union merge") + .replace("\r\n", "\n"); + assert!(contents.contains("ours\n"), "{contents}"); + assert!(contents.contains("theirs\n"), "{contents}"); + assert!(!contents.contains("<<<<<<<"), "{contents}"); +} + +#[test] +fn isolated_three_way_config_preserves_core_whitespace_error_classification() { + let repo = init_repo(); + let root = repo.path(); + let diff = build_clean_three_way_addition(root, "patched \n"); + assert_eq!( + run(root, &["git", "config", "apply.whitespace", "error"]).0, + 0 + ); + assert_eq!( + run( + root, + &["git", "config", "core.whitespace", "-trailing-space"] + ) + .0, + 0 + ); + configure_unused_marker_driver(root); + + let result = apply_git_patch(&ApplyGitRequest { + cwd: root.to_path_buf(), + diff, + revert: false, + preflight: false, + }) + .expect("run forced three-way apply with whitespace error policy"); + + assert_eq!(result.exit_code, 0, "{result:?}"); + assert!(result.cmd_for_log.contains("GIT_COMMON_DIR=")); + let contents = std::fs::read_to_string(root.join("file.txt")) + .expect("read whitespace result") + .replace("\r\n", "\n"); + assert_eq!(contents, "current\nbase\npatched \n"); + let marker = run(root, &["git", "config", "--get", "codex.mergeran"]); + assert_ne!(marker.0, 0, "unused merge driver ran"); +} + +#[test] +fn isolated_three_way_config_preserves_core_whitespace_fix_output() { + for (classifier, expected_line) in [ + ("-indent-with-non-tab", " patched\n"), + ("indent-with-non-tab", "\tpatched\n"), + ] { + let repo = init_repo(); + let root = repo.path(); + let diff = build_clean_three_way_addition(root, " patched\n"); + assert_eq!( + run(root, &["git", "config", "apply.whitespace", "fix"]).0, + 0 + ); + assert_eq!( + run(root, &["git", "config", "core.whitespace", classifier]).0, + 0 + ); + configure_unused_marker_driver(root); + + let result = apply_git_patch(&ApplyGitRequest { + cwd: root.to_path_buf(), + diff, + revert: false, + preflight: false, + }) + .expect("run forced three-way apply with whitespace fix policy"); + + assert_eq!(result.exit_code, 0, "{classifier}: {result:?}"); + assert!(result.cmd_for_log.contains("GIT_COMMON_DIR=")); + let contents = std::fs::read_to_string(root.join("file.txt")) + .expect("read fixed whitespace result") + .replace("\r\n", "\n"); + assert_eq!(contents, format!("current\nbase\n{expected_line}")); + let marker = run(root, &["git", "config", "--get", "codex.mergeran"]); + assert_ne!(marker.0, 0, "unused merge driver ran for {classifier}"); + } +} + #[test] fn apply_allows_clean_patch_with_selected_merge_driver() { let repo = init_repo(); @@ -223,7 +356,7 @@ fn apply_allows_clean_patch_with_selected_merge_driver() { "git", "config", "merge.codex-test.driver", - "git config codex.mergeran true && false", + "git config --file .git/config codex.mergeran true && false", ], ); assert_eq!(config_code, 0, "configure merge driver: {config_err}"); @@ -275,7 +408,7 @@ fn reverse_apply_allows_clean_patch_with_selected_merge_driver() { "git", "config", "merge.codex-test.driver", - "git config codex.mergeran true && false", + "git config --file .git/config codex.mergeran true && false", ], ); assert_eq!(config_code, 0, "configure merge driver: {config_err}"); @@ -336,7 +469,7 @@ fn preflight_does_not_probe_or_run_selected_merge_driver() { "git", "config", "merge.codex-test.driver", - "git config codex.mergeran true && false", + "git config --file .git/config codex.mergeran true && false", ], ); assert_eq!(config_code, 0, "configure merge driver: {config_err}"); @@ -397,7 +530,7 @@ fn merge_override_rejects_process_temp_directory_inside_worktree() { "git", "config", "merge.unused.driver", - "git config codex.mergeran true && false", + "git config --file .git/config codex.mergeran true && false", ], ); assert_eq!(config_code, 0, "configure merge driver: {config_err}"); @@ -642,6 +775,32 @@ fn build_three_way_fixture(root: &Path) -> String { patch.1 } +fn build_clean_three_way_addition(root: &Path, added_line: &str) -> String { + std::fs::write(root.join("file.txt"), "base\n").expect("write base"); + assert_eq!(run(root, &["git", "add", "file.txt"]).0, 0); + assert_eq!(run(root, &["git", "commit", "-m", "base"]).0, 0); + std::fs::write(root.join("file.txt"), format!("base\n{added_line}")).expect("write patch side"); + let patch = run(root, &["git", "diff", "--full-index", "--", "file.txt"]); + assert_eq!(patch.0, 0, "create patch: {}", patch.2); + assert_eq!(run(root, &["git", "checkout", "--", "file.txt"]).0, 0); + std::fs::write(root.join("file.txt"), "current\nbase\n").expect("write current side"); + assert_eq!(run(root, &["git", "add", "file.txt"]).0, 0); + patch.1 +} + +fn configure_unused_marker_driver(root: &Path) { + let configured = run( + root, + &[ + "git", + "config", + "merge.unused.driver", + "git config --file .git/config codex.mergeran true && false", + ], + ); + assert_eq!(configured.0, 0, "configure unused driver: {}", configured.2); +} + fn run(cwd: &Path, args: &[&str]) -> (i32, String, String) { let mut command = std::process::Command::new(args[0]); isolate_git_command_environment(&mut command); diff --git a/codex-rs/git-utils/src/repository_authority/authority.rs b/codex-rs/git-utils/src/repository_authority/authority.rs index cbdd194ce9..e0e1bc24cb 100644 --- a/codex-rs/git-utils/src/repository_authority/authority.rs +++ b/codex-rs/git-utils/src/repository_authority/authority.rs @@ -190,13 +190,22 @@ impl RepositoryAuthority { }) } - #[cfg(all(test, unix))] pub(crate) fn active_git_dir(&self) -> Option<&Path> { self.active_metadata .as_ref() .map(|metadata| metadata.git_dir.as_path()) } + pub(crate) fn active_common_dir(&self) -> Option<&Path> { + self.active_metadata + .as_ref() + .map(|metadata| metadata.common_dir.as_path()) + } + + pub(crate) fn active_worktree_root(&self) -> &Path { + &self.active_worktree_root + } + #[cfg(test)] pub(crate) fn contains_root(&self, root: &Path) -> bool { self.roots diff --git a/codex-rs/git-utils/src/reverse_apply_tests.rs b/codex-rs/git-utils/src/reverse_apply_tests.rs index c700a6b803..ce88e73db0 100644 --- a/codex-rs/git-utils/src/reverse_apply_tests.rs +++ b/codex-rs/git-utils/src/reverse_apply_tests.rs @@ -158,6 +158,8 @@ fn assert_refused_without_mutation(root: &Path, patch: &str) { let before_leaves = [ leaf_snapshot(root, "old.txt"), leaf_snapshot(root, "new.txt"), + leaf_snapshot(root, "file.txt"), + leaf_snapshot(root, "other.txt"), ]; let error = apply_git_patch(&request(root, patch)).expect_err("unsafe reverse must fail"); assert_eq!(error.kind(), io::ErrorKind::PermissionDenied, "{error}"); @@ -165,7 +167,9 @@ fn assert_refused_without_mutation(root: &Path, patch: &str) { assert_eq!( [ leaf_snapshot(root, "old.txt"), - leaf_snapshot(root, "new.txt") + leaf_snapshot(root, "new.txt"), + leaf_snapshot(root, "file.txt"), + leaf_snapshot(root, "other.txt") ], before_leaves ); @@ -195,6 +199,65 @@ fn reverse_delete_and_rename_preserve_divergent_staged_blobs() { } } +#[test] +fn reverse_three_way_refuses_staged_content_change_matching_the_worktree() { + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("file.txt"), b"top\nbase\nbottom\n").expect("base file"); + run_success(root, &["add", "file.txt"]); + run_success(root, &["commit", "-qm", "base"]); + let base = String::from_utf8(run_success(root, &["rev-parse", "HEAD"])).expect("base OID"); + std::fs::write(root.join("file.txt"), b"top\ntheirs\nbottom\n").expect("theirs file"); + run_success(root, &["add", "file.txt"]); + run_success(root, &["commit", "-qm", "theirs"]); + let patch = String::from_utf8(run_success( + root, + &[ + "diff", + "--full-index", + base.trim(), + "HEAD", + "--", + "file.txt", + ], + )) + .expect("patch"); + std::fs::write(root.join("file.txt"), b"top\nuser-staged\nbottom\n").expect("user staged file"); + run_success(root, &["add", "file.txt"]); + + assert_refused_without_mutation(root, &patch); +} + +#[test] +fn reverse_three_way_refuses_staged_non_candidate_before_staging_another_path() { + let repo = init_repo(); + let root = repo.path(); + for path in ["file.txt", "other.txt"] { + std::fs::write(root.join(path), b"top\nbase\nbottom\n").expect("base file"); + } + run_success(root, &["add", "."]); + run_success(root, &["commit", "-qm", "base"]); + let base = String::from_utf8(run_success(root, &["rev-parse", "HEAD"])).expect("base OID"); + for path in ["file.txt", "other.txt"] { + std::fs::write(root.join(path), b"top\ntheirs\nbottom\n").expect("theirs file"); + } + run_success(root, &["add", "."]); + run_success(root, &["commit", "-qm", "theirs"]); + let patch = String::from_utf8(run_success( + root, + &["diff", "--full-index", base.trim(), "HEAD"], + )) + .expect("patch"); + + std::fs::write(root.join("file.txt"), b"top\nuser-staged\nbottom\n") + .expect("staged non-candidate"); + run_success(root, &["add", "file.txt"]); + std::fs::write(root.join("other.txt"), b"top\nunstaged\nbottom\n") + .expect("unstaged staging candidate"); + + assert_refused_without_mutation(root, &patch); +} + #[test] fn reverse_rename_accepts_each_partially_staged_form() { for staged_endpoints in ["old", "new", "both"] { diff --git a/codex-rs/git-utils/src/reverse_staging.rs b/codex-rs/git-utils/src/reverse_staging.rs index c82d07eb87..198a00c2be 100644 --- a/codex-rs/git-utils/src/reverse_staging.rs +++ b/codex-rs/git-utils/src/reverse_staging.rs @@ -10,9 +10,16 @@ use crate::patch_paths::confine_patch_paths_guarded; use crate::patch_paths::leaf_is_traversable_directory; use crate::patch_paths::leaf_may_run_git_content_filter; +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) enum ReverseApplyMode { + Direct, + ThreeWay, +} + pub(crate) fn stage_effective_paths_for_reverse( config: &mut GuardedGitConfig<'_>, paths: &[String], + mode: ReverseApplyMode, ) -> io::Result<()> { let confined = confine_patch_paths_guarded(config, paths)?; let mut worktree_paths = BTreeMap::new(); @@ -82,6 +89,15 @@ pub(crate) fn stage_effective_paths_for_reverse( "refusing to stage reverse patch path {path:?} because it has intent-to-add index state" ))); } + if mode == ReverseApplyMode::ThreeWay + && let Some(path) = exact_paths + .iter() + .find(|path| cached_visible_status.contains_key(*path)) + { + return Err(reverse_staging_error(format!( + "refusing a three-way reverse patch because it would replace existing staged data for {path:?}" + ))); + } let worktree_changed_paths = read_changed_paths(config, &exact_paths)?; let mut staging_candidates = Vec::new();