codex/git: isolate three-way reverse staging

This commit is contained in:
Chris Bookholt
2026-07-03 16:13:55 -07:00
parent 0b31ecb5bb
commit 49664d3c75
11 changed files with 833 additions and 132 deletions
+46 -12
View File
@@ -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<ApplyGitResult> {
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())
+201
View File
@@ -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<OsStr>) -> &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<IsolatedGitCommonDir> {
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<std::process::Output> {
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<std::process::Output> {
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::<Vec<_>>();
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<usize> = const { std::cell::Cell::new(0) };
+73 -8
View File
@@ -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<std::process::Output> {
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<String> {
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=<isolated>".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::<Vec<_>>()
.join(" ")
))
}
#[cfg(test)]
pub(crate) fn merge_include_arg(&self) -> Option<&str> {
pub(crate) fn merge_common_config_path(&self) -> Option<PathBuf> {
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)?;
}
@@ -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<Vec<&str>> {
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<CapabilityIdentity>,
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<CapabilityIdentity>,
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<CapabilityIdentity>,
args: &mut Vec<String>,
) -> 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<CapabilityIdentity>) -> 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<Option<SealedMergeConfigOverride>> {
) -> io::Result<SealedMergeConfigOverride> {
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<SealedMergeConfigOverride>,
) -> 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()
)));
@@ -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);
}
+11 -11
View File
@@ -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]
@@ -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=<isolated>"));
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=<isolated>"));
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();
+166 -7
View File
@@ -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=<isolated>"));
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=<isolated>"));
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=<isolated>"));
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);
@@ -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
+64 -1
View File
@@ -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"] {
+16
View File
@@ -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();