Preserve merge conflict contents in imported worktrees - #77
Open
wesm wants to merge 13 commits into
Open
Conversation
Merge-request isolation currently reports conflicts without preserving the other side in the working file. Define a fallback that uses the trusted Git executable for normal three-way merges while keeping the PATH-hijack boundary. Keep trusted global attribute-driver policy separate from this focused data preservation fix. Generated with Codex Co-authored-by: Codex <codex@openai.com>
Git already shell-quotes merge labels, so adding double quotes around those placeholders would make hostile command substitutions executable. A missing persisted executable would also look like an ordinary conflict and reproduce the data-hiding failure. Use fixed labels, an explicit executable guard, and documented binary-status mapping so the implementation plan preserves the untrusted-tree boundary. Generated with Codex Co-authored-by: Codex <codex@openai.com>
Mapping every status above 128 would hide a signaled merge process as an ordinary conflict and could leave the current side without markers. Limit the binary exception to merge-file's exact status 255 and preserve all signal failures. Make the Windows command path and cross-platform test coverage explicit before implementation starts. Generated with Codex Co-authored-by: Codex <codex@openai.com>
The design now has verified decisions for shell quoting, missing executables, binary conflicts, signal failures, and Git for Windows paths. Record the test-first execution sequence before production code changes so these security and data-preservation constraints stay coupled to behavior tests. Generated with Codex Co-authored-by: Codex <codex@openai.com>
Keep POSIX quoting behavior in one reusable internal package so Git command builders and related tests use the same safe treatment of spaces and quotes. This removes the duplicated private helper without changing credential-helper command output. Generated with Codex Co-authored-by: Codex <codex@openai.com>
Imported merge-request worktrees used a merge driver that reported every merge as a conflict without writing the base or other side to the worktree. This hid useful conflict contents and made non-overlapping edits impossible to merge normally. Use the resolved Git executable for three-way merge-file behavior. Keep fixed marker labels and propagate operation failures so untrusted labels, PATH changes, and missing executables cannot turn errors into incomplete conflicts. Generated with Codex Co-authored-by: Codex <codex@openai.com>
The PATH regression could pass after any pre-driver merge error and no longer exercised the safe diff replacement. The label test also combined branch and subject payloads in a rebase, which did not prove branch-label behavior. Require the expected unmerged state and diff3 contents after preserving the successful diff probe. Exercise hostile branch and subject labels through merge and rebase separately so each Git label path remains covered. Generated with Codex Co-authored-by: Codex <codex@openai.com>
Future merge-driver changes need the same boundary as the current repair. Without a durable contract, a later implementation could again hide text conflict contents or turn an unavailable Git process into a per-file conflict. Record the observable behavior for executable resolution, text and binary conflicts, process failures, and Unix and Git for Windows support. Generated with Codex Co-authored-by: Codex <codex@openai.com>
Git expands merge-driver placeholders across the complete configured command before the shell parses it. A resolved executable path containing `%Y` could therefore splice an untrusted branch label into the command despite POSIX shell quoting and run its command substitutions. Keep literal percent signs intact through Git's template layer before applying shell quoting. Record the two escaping layers and the existing platform Git version floors so future changes preserve the same boundary. Generated with Codex Co-authored-by: Codex <codex@openai.com>
Git before 2.42 turns every positive custom merge-driver result into a conflict. Git merge-file also uses status 255 for both binary content and input or output failures. Together, those rules can hide a real operation failure behind a current-only conflict. Require Git 2.42 outside Windows. Classify binary inputs with the pinned Git executable in an attribute-free, helper-free context, then pass all text merge statuses through. Binary conflicts stay local without masking merge failures. Generated with Codex Co-authored-by: Codex <codex@openai.com>
Merge input classification could inherit a caller's repository bindings or large-file threshold. Plain text could then look binary and leave only the current side in an ordinary conflict even though merge-file could preserve all three inputs. Clear repository bindings and counted configuration before classification. Pin the classifier to merge-file's maximum text size so ambient Git state cannot change the result. Generated with Codex Co-authored-by: Codex <codex@openai.com>
The counted-config regression proves that the nested classifier resets GIT_CONFIG_COUNT. It cannot independently detect removal of the explicit large-file threshold because the same reset removes its test input. Supply a one-byte threshold through an isolated global config instead. Removing only the 1023m pin now reproduces the current-only conflict and protects the reason for matching merge-file's text limit. Generated with Codex Co-authored-by: Codex <codex@openai.com>
roborev: Combined Review (
|
The merge-driver contract says a crashed merge process must fail the whole operation, not record a conflict. Nothing tested that. The existing coverage stops at status 255, which the driver reaches by returning normally, so a change that mapped every status above 128 back to a conflict would keep every test green while restoring the original bug: an unmerged path whose working file holds only the current side. Kill the merge process with a signal and require the operation to abort with a clean tree. Reintroducing the status mapping now fails this test. Also assert that no classifier helper marker appears in the hooks directory. The classifier runs Git there with -C, so a helper invoked from that directory would have left its marker where nothing looked. Document that a caller's Git runner governs process policy rather than which Git installation the merge driver pins. Git runs the driver itself and cannot route it back through the callback, so a runner pointing at another Git does not redirect it. Generated with Claude Code (claude-fable-5) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
roborev: Combined Review (
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge-request imports replaced every tree-selectable custom merge driver with
f() { return 1; }; f. That reports a conflict without writing%A, so Gitrecorded all three stages in the index while the working file kept only the
current side — no conflict markers, no other-side content. Staging the file
dropped the other side's changes silently. Reported as kenn-io/kwt#112.
The replacement now performs Git's own three-way text merge, so an imported
worktree behaves like an ordinary one: non-overlapping edits merge cleanly,
overlapping edits produce diff3 markers carrying base, current, and other, and
only a real overlap returns a conflict.
The driver is built at import time around the absolute path of the same
gitexecutable
git/cmd.Runneruses, resolved before the untrusted worktree ismaterialized so an imported tree cannot steer it through
PATH. Everyinvocation guards that path with
[ -x ]and returns 129 if it moved, whichGit treats as a driver failure and aborts the operation instead of recording a
current-only conflict. Each input pair is then classified with
git diff --no-index --numstat: binary content returns 1 and stays an ordinaryper-file conflict holding the current bytes, matching Git's built-in binary
driver, while text goes to
git merge-file --diff3with its status passedthrough unchanged.
Two decisions carry the risk.
The command uses fixed
current/base/otherlabels rather than%S,%X,and
%Y. Git substitutes%A,%O, and%Bunquoted, so the template needsdouble quotes around them, but it substitutes the label placeholders already
single-quoted — inside those same double quotes, a branch name or commit
subject containing
$(...)becomes executable during any merge or rebase inthe imported worktree. Not interpolating the labels removes that interaction
entirely.
TestUntrustedTreeMergeDriverDoesNotEvaluateGitLabelscovers boththe branch-name and commit-subject routes.
Binary input is caught by preclassification rather than by mapping
merge-file's exit status, because 255 covers both binary rejection and I/Ofailure; mapping it would turn a real failure back into a silent current-only
conflict. The classifier runs under the pinned executable with repository
bindings unset,
GIT_CONFIG_COUNT=0, system attributes disabled, discoveryceilinged, and
--no-ext-diff --no-textconv, so nothing the tree selects cansteer classification.
core.bigFileThreshold=1023mmatchesmerge-file'smaximum text size so the two agree on what counts as binary.
Untrusted-tree import now requires Git 2.42.0 on non-Windows platforms, up from
2.39.1. Before 2.42 Git collapsed every positive custom-driver status into an
ordinary conflict, so the missing-executable guard could not be told apart from
a conflict. The Git for Windows floor is unchanged at 2.53.0.windows.3. On
Windows the pinned path is emitted drive-qualified with forward slashes, which
both Git for Windows' shell and the Windows loader accept.
git/internal/shellquotenow owns the POSIX single-quote rule thatgit/cmdkept privately, so the credential helper and the merge driver share one
implementation.
Ten behavior tests in
git/managed/untrusted_tree_merge_test.godrive thepersisted driver through real merge-request imports, and the existing
PATH-hijack test now plants both
gitandshshims in the tree and runs amerge through them. The isolation boundary itself is unchanged: this is a Git
execution boundary, not an OS sandbox.