Repository navigation
Fix #18: merge keeps a mode change from either side - #20
Merged
Merged
Conversation
Merge, fast-forward, cherry-pick and commit cases that lose a mode change made on one side, or print it as a create/delete pair, compared with git 2.34.1.
merge-ort compared blob hashes only, so a side that changed only the mode looked unchanged, and the three-way paths wrote whichever side's mode that branch of the code used. Compare hash and mode in the match mask and in triviallyResolve, and pick a regular file's merged mode the way git's handle_content_merge does: if ours kept the base mode, take theirs. The same rule applies to the processEntry shortcuts, the clean diff3 result, the conflicted worktree blob, the plain rename branches, rename/rename to one path and rename/rename(1to2). An add/add of one path with different modes is a conflict, as in git. The commit and diffstat summaries print git's "mode change" line for a modified or renamed path whose mode changed.
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
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.
Fixes #18
Summary
git mergenow keeps a file mode change made on either side. The issue's script prints the same output as git 2.34.1, byte for byte. That covers them | 0row, themode change 100644 => 100755lines,100755forf,mandpin the clean merge, and the stages of the conflicted one. Cherry-pick, rebase, revert and stash apply go through the same merge-ort code, so they pick up the fix too. A cherry-pick of a commit that only runschmod +xused to stop with "The previous cherry-pick is now empty". It now commits the mode change, as git does.Two related changes came out of comparing against git:
CONFLICT (add/add), even if the content is identical. just-git used to resolve it silently with ours' mode. It now conflicts the same way.create mode/delete modepair. They now printmode change 100644 => 100755 f, and a renamed path whose mode changed gets git'smode change 100644 => 100755line after therenameline. This also fixes the third Open item in Fix #10: checkout sets the executable bit, and status/diff report mode changes #15.Root cause
collectMergeInfobuiltmatchMaskfrom blob hashes, andtriviallyResolvedecided "unchanged", "only ours changed" and "only theirs changed" by hash, then copied that side's mode. A side that changed only the mode looked unchanged. InprocessEntrythe three-way shortcuts (oh === bh,th === bh,oh === th) copied one side's mode, and the clean diff3 result and the conflicted worktree blob usedours.mode. The plain rename branches indetectAndProcessRenameshad the same pattern, as did rename/rename to one path and rename/rename(1to2).Git compares mode and oid together in
collect_merge_info_callback, andhandle_content_mergemerges the mode on its own:The fix
All in
src/lib/merge-ort.ts, plus the summary lines insrc/lib/commit-summary.ts.sameVersion(a, b)compares hash and mode.matchMaskandtriviallyResolveuse it.mergeFileMode(base, ours, theirs)returnsours === base ? theirs : ourswhen all three are100644or100755, andnullotherwise. Callers fall back to the mode they used before, so symlink, gitlink and type-change handling is unchanged.processEntryuses the merged mode in the three shortcuts, the clean diff3 result, the clean merge driver result, and the worktree blob of binary, driver and diff3 conflicts. Stages 1, 2 and 3 keep each side's own mode, which was already right.addAddConflicthelper. The conflict message hasAuto-mergingonly when the contents differ, which matches git.Messages for mode-only cases match git. A mode-only difference prints no
Auto-merging, because the shortcuts return before it is pushed.Tests
test/commands/merge.test.ts, "file modes", 9 tests. Each asserts exact stdout, empty stderr, the exit code andgit ls-files -s. They cover theirs mode-only, ours mode-only plus theirs content, both content plus theirs mode, a content conflict with theirs' mode change, both sides making the same mode change, add/add with different modes, theirs' mode change to a file ours renamed, rename/rename(1to2), and the fast-forward summary. Every expected string was checked against git 2.34.1 with the same setup.test/lib/merge.test.ts, 2 tests. A conflicted content merge where theirs or ours changed the mode writes100755for the file in the result tree, which is what checkout writes to the worktree. Stages 1, 2 and 3 carry each side's mode.test/commands/cherry-pick.test.ts: cherry-pick of a mode-only commit commits it and prints themode changeline.test/commands/commit.test.ts: commit summaries for a mode-only change and for a content plus mode change.test/interop/merge-modes.test.ts, 2 tests. Real git builds the issue's repository on disk and is copied twice. Real git merges in one copy and just-git in the other. stdout, stderr, exit code and real git'sls-files -smust match, for the clean merge and the conflicted one. The conflicted file's content is compared too.The tests land in a commit before the fix. On the
b509001source, 13 of the 15 new tests fail. The two that pass are the "same mode change on both sides" test and the lib test where ours changed the mode. The old code got both right by accident, and they are there to pin the new rule.bun test: 3527 pass, 29 skip, 0 fail. The baseline atb509001is 3512 pass, 29 skip, 0 fail.bun checkis clean andbun gen-docsproduces no change. I did not run the oracle suite.Open
FileSystemhas nochmod, so the worktree file keeps its old permission bits after a merge. The result tree now carries the merged mode, so checkout will apply it once Fix #10: checkout sets the executable bit, and status/diff report mode changes #15 lands. Until then, resolving a conflict withgit addstages the worktree's mode, which can still be the old one.git stashon main cannot save a mode-only change (Open item 2 in Fix #10: checkout sets the executable bit, and status/diff report mode changes #15), and an applied mode only reaches the worktree, so there is no stash test here.handleRenameAddAddand rename/delete still pick a side's mode. In the cases I tried, their index result matched git, so I left them alone.Auto-merging f.ftog. Theirs editsfand addsgwith the base content. Git takes theirs' edit atg. just-git keeps the unedited content and prints an extraAuto-merging f.