Block selected merge drivers before three-way patch application#30854
Block selected merge drivers before three-way patch application#30854bookholt-oai wants to merge 14 commits into
Conversation
…4394-git-06-merge-guard # Conflicts: # codex-rs/git-utils/src/apply.rs
…ec-4394-git-06-merge-guard
…ec-4394-git-06-merge-guard # Conflicts: # codex-rs/git-utils/src/patch_paths.rs
…ec-4394-git-06-merge-guard
# Conflicts: # codex-rs/git-utils/src/apply.rs # codex-rs/git-utils/src/patch_paths.rs # codex-rs/git-utils/src/safe_git.rs # codex-rs/git-utils/src/safe_git_tests.rs
…ec-4394-git-06-merge-guard
…ec-4394-git-06-merge-guard
…4394-git-06-merge-guard
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b31ecb5bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !entry.value.is_empty() { | ||
| keys.push(entry.key.as_str()); | ||
| } |
There was a problem hiding this comment.
When a merge driver key exists but has an empty value at the policy snapshot, this filter omits it from the neutralizer entirely. In the race window after the attribute/config probe but before the final git apply --3way, the same driver can be changed from empty to a command and selected by attributes/defaults; because no override for that known key is attached, the final Git child re-reads the now-nonempty driver and can execute it. Include all discovered merge.*.driver keys in the override, while still rejecting only those selected with nonempty values at probe time.
Useful? React with 👍 / 👎.
| for name in candidate_driver_names(attribute, default) { | ||
| if drivers.get(name).is_some_and(|value| !value.is_empty()) { | ||
| return Ok(Some((name.to_string(), path.clone()))); |
There was a problem hiding this comment.
If a path already has merge=demo but merge.demo.driver is absent during this probe, this treats the selection as safe and no override for merge.demo.driver is written. In the same post-probe config race covered by these guards, adding that driver before the final git apply --3way lets Git execute it because the command line contains no merge.demo.driver= neutralizer. Track and blank selected non-sentinel driver names even when they are currently unconfigured before allowing fallback.
Useful? React with 👍 / 👎.
| if staging_candidates.is_empty() { | ||
| return Ok(()); |
There was a problem hiding this comment.
When a reverse patch falls back to --3way, an already-staged change whose worktree matches the index is not added to staging_candidates, so this early return skips the cached_visible_status check entirely. For example, reversing a delete patch while the deleted path has a user-staged modification causes the final git apply -R --3way to proceed and turn that staged data into an add/add conflict instead of refusing without mutation. Check for staged status across all effective reverse paths before returning.
Useful? React with 👍 / 👎.
1 participant
Why
When a patch does not apply cleanly,
git apply --3waycan run a custom merge driver selected by the repository. Reverse application can also damage unrelated staged work if its index state is not checked carefully.Approach
Try a normal indexed apply first. If it works, do not read or invoke merge-driver policy at all.
Before a real three-way fallback, reread the effective merge configuration and attributes for every exact patch path. Reject any selected nonempty custom driver and neutralize all known merge drivers through the final Git child.
Keep the repository authority, filter policy, merge policy, and staging protections attached to one operation. For reverse application, use narrow checks for index and worktree state and refuse conflicts, divergent staging, unsafe flags, aliases, sparse or ignore hazards, and directory races before mutation.
This PR is stacked on #30850 and should land after it.
Testing
codex-git-utilstests passed in Cargo and the inherited Bazel shards.