[Move] Patch apply and revert onto the bundled Git (#385)

[juanmaguitar]

Why

src/patch-apply.js was the last engine the app carried of its own: 549 lines matching and writing hunks with the diff package, plus a path sandbox, a rollback, CRLF preservation, rename handling and per-hunk diagnosis, all written when isomorphic-git had no merge machinery. With #401 to #408 below it, "does this patch fit this checkout" can be answered by the same git apply a mentor or Trac's committer will run, and the write can be Git's. Decided on 2026-09-08 with JuanMa: move to git apply, hybrid, with tests/e2e/journeys/patch-apply.spec.js as the judge. Stacked on #408.

What changes

• src/git-write.cjs: applyPatch(dir, text, { check, reverse, platform }): apply --whitespace=nowarn -p1 [--check] [--reverse] - on stdin, okCodes [0, 1, 128], the windowsArgs prefix (the core.autocrlf view is what lets an LF patch fit a CRLF checkout a host Git made on Windows). No --index: the index stays untouched and a new file stays untracked, as before. Stderr is kept for the log and never parsed.
• src/patch-plan.cjs: rewritePatchPaths(text) puts every header path (diff --git, Index:, ---/+++, rename/copy from/to) through the same stripPathPrefix + mapToSrcLayout the parser uses and re-emits a//b/, so -p1 lands them where the preview said; quoted paths are refused as they already were. splitPatchSections(text) cuts the text per file (path, from, binary with or without data), so one file can be checked alone and a data-less binary section left out; parsePatchFiles parses section by section over it, so a binary or a pure rename ahead of a text file is no longer swallowed by jsdiff.
• src/patch-apply.js rewritten around Git, same signature and result shapes (main.js, apply-conflict.cjs and the IPC tests unchanged): parse, rewrite, drop "Binary files differ" sections into skipped (a section that carries its data now applies), whole --check; on refusal, --check per section to name the files, worded in JS (target missing or present, "points outside the site folder" with symlinks followed, else diagnoseHunks regions, kept verbatim); notApplied (Revert dead-ends when the patch is no longer in the checkout #183) when every section fails to reverse and the whole patch applies forwards; on a clean check, a snapshot of both ends of every section, the write, and rollback(dir, snapshot) if Git exits non-zero after all (an I/O failure part-way, which Git does not undo; verified). Gone: resolveFile, resolveInside, dominantEol, patchIsAbsent, the write loop.
• src/trunk-update.js: ensureAutocrlf / createCrlfCompatibleFs deleted (an in-memory view built for isomorphic-git, a no-op at every call site since the reads moved) with their seven tests; the three call sites in main.js and the applier go; src/pr-files.cjs's comment says crlfArgs.
• package.json: isomorphic-git is a devDependency (four test fixtures still build repositories with it, until Phase 4: isomorphic-git leaves, and the issues parked on this decision are re-asked #386). It stays in the packaged app all the same: @wp-playground/storage depends on it, so the installer does not shrink here.
• Docs: docs/guide/applying-patches.md (the apply is git apply, all or nothing; binaries with data apply, data-less ones are named), docs/guide/getting-started.md (no flow is on isomorphic-git), AGENTS.md and the review standard (patch apply on git apply through the primitives; the JS keeps the path rewrite, the wording and the snapshot, none of which writes; the generator's output must pass git apply --check), the self-review skill's list.
• Tests: applyPatch argv and integration (check writes nothing, apply and reverse, index untouched, a path outside refused with 128, CRLF under the Windows view); rewritePatchPaths and splitPatchSections on every header shape; the applier's 45 tests kept where they describe behaviour, with the internals' tests replaced (rollback over a snapshot, the CRLF case under both platforms, a binary with data applied); a new agreement test in ipc-wiring: every shape git:get-patch emits (deletion, addition, empty add and delete, no trailing newline, a binary named above) passes git apply --check and applies in a second checkout. Journeys unchanged: 18/18.

Diff size. ~1,400 changed lines outside the lock in two commits (the move, then the self-review answers), more than half of them tests moved or deleted (the seven CRLF-view tests, the internals' tests, patch-apply.js itself losing 300).

How to test this

Platforms: macOS and Windows.

Starting state: the Buildkite artifact for the current head installed; a site created by this build with a ticket linked.

1. In Apply a patch or PR, apply a pull request diff (paste a PR number of wordpress-develop). Expected: the preview lists src/… paths; Apply and rebuild applies, the banner says what is applied; git status --porcelain in the site folder shows the files unstaged (M, ??), nothing staged. Revert this patch: the checkout is back, git status --porcelain clean.
2. A Trac attachment written against the old layout (paths like wp-admin/…, no a/ b/): the preview names src/wp-admin/… and the apply lands there.
3. A patch that does not fit (apply the same PR twice, or one against an older trunk). Expected: "The checkout was not changed", the files named, and inside each which regions and why (already in your checkout, or the code around it changed), as before.
4. A .diff made with git diff --binary that adds an image, applied from a file. Expected: the preview does not list it as skipped and the image lands. A .patch that only says "Binary files differ" (a pull request's diff from GitHub, a Trac attachment): the preview names it as skipped and the apply skips it.
5. Edit a file the patch touches, then Revert. Expected: refused by name with the regions you edited over (Tell the contributor's own edits apart from the patch they applied #306). Discard your changes, Revert again: fine.
6. Windows: steps 1 and 3 on a site adopted from a host Git checkout with CRLF files, if one is at hand; on a site this build created, step 1.
7. The packaged app: git apply is the bundled one (ps / Task Manager shows dugite/git/bin/git apply during step 1, briefly).

What must not have happened: anything staged by an apply; a half-applied patch after a refusal (git status unchanged); a file written outside the site folder from a crafted patch (Git refuses ../ and symlinked paths: error: invalid path).

Risks and limitations

• A CRLF file in a checkout a host Git made, on macOS, receives LF lines from Git where the old applier matched the file's ending. macOS hosts seldom set core.autocrlf, a site the app cloned is LF throughout, and Windows is covered by the core.autocrlf view; documented in the module header.
• A write Git leaves half done (an I/O failure part-way) is put back from the snapshot; a rollback that fails is reported as it was before (rolledBack: false, recovery).
• Per-hunk wording is still the diff package's guess (evidence, not proof, as before); Git's decision is the one that writes.
• The generator stays hand-rolled; the new agreement test is what keeps its output applicable by Git.
• Two rollback tests are skipped on Windows (git apply exits 0 without writing when a leading path component is a file, on Windows #413). They inject an I/O failure with a regular file where the patch wants a directory; POSIX Git fails part-way, the bundled Git on Windows exits 0 on the same patch. Whether Git wrote nothing and said so, or --check would have refused first, needs a Windows machine; until then the snapshot-and-rollback path is covered on POSIX only. Found on this PR's Windows CI together with a fixture-shape problem (the patch fixtures looked like adopted checkouts, so Windows saw CRLF where the patch said LF), fixed in 684e220.
• Review: see the Review outcome section.

Related

Part of #364. Closes #385: this is the last of its four flows, and every box on that issue is ticked by #403, #404, #405, #407, #408 and this PR. Stacked on #408 (merge that first). Next: #411 retires isomorphic-git from the fixtures and package.json. Follow-up: #413.

---

Design decisions and alternatives considered

• Hybrid, not a full replacement. git apply says "patch failed: file:line" and no more; the per-hunk "already applied" versus "moved" with a line to search for (A patch that no longer applies is a dead end, and the app is already holding the way out #282, "No longer applies" hides a patch that is already in the tree — sometimes in the same file as one that is not #226) is what the panel is built on, so diagnoseHunks stays as wording over the files Git named. Nothing in JS writes any more.
• Per-section --check to name failures, not stderr parsing. The standard forbids parsing human-facing output; a section checked on its own answers with an exit code.
• The path rewrite on the text, not --directory. The mapping is per file (root files stay, wp-* moves under src/), which a single --directory prefix cannot express.
• Keep the snapshot. Probed: Git does not undo files it wrote before an I/O failure. The old module's promise ("the checkout was put back as it was") is kept the same way it was made, from a copy taken before the write.
• Keep reverseFile. Only to word a failing reverse (Tell the contributor's own edits apart from the patch they applied #306); Git does its own --reverse.
• isomorphic-git to devDependencies now, not in Phase 4: isomorphic-git leaves, and the issues parked on this decision are re-asked #386: nothing under src/ requires it, and a require sneaking back in would fail the packaged app's tests rather than pass silently.

Review outcome (required — see AGENTS.md)

/self-review against the parent branch, judgement pass run with fresh context (the diff and the review standard only). npm run lint clean, unit suite green on both Node runtimes before and after (1178, then 1182), journeys 18/18, packaged smoke 9/9. 5 [fix here] · 2 [follow-up]; the five and one of the two are in the second commit.

Applied

• Architecture 🟡: a patch whose only sections are data-less binaries was refused ("does not change any files") where the old applier succeeded with them named. Restored, with a test.
• Architecture 🟡: the preview kept calling every binary "will be skipped" while the applier now applies the ones that carry their data. parsePatchFiles marks hasBinaryData and planApply lists only the data-less ones. Found on the way: jsdiff swallows a section with no hunks (a binary, a pure rename, an empty file) whenever another section follows it, so a binary placed before a text file was already missing from the preview; parsePatchFiles now parses section by section over splitPatchSections, and the file list is the section list. Test for the swallowed shape.
• Performance 🟡: a refusal checked every section with a fresh windowsArgs (a git config read each on Windows). The prefix is resolved once and passed down, and the per-section pass stops at 20 files with a line saying how many were not checked one by one; the notApplied answer needs every section checked and says nothing past the cap.
• Tests 🔵: the four gaps named: a binary-only patch, the fallback that shows Git's own line (two sections on one file), a reverse of a binary that carries its data, a rename whose destination exists.
• Docs 🔵: the guide claimed a pull request's .diff carries binary data; GitHub's diff media type does not. It says git diff --binary now.
• Follow-up taken 🔵: rollback leaves a file whose bytes still match the snapshot alone, so a file Git never reached keeps its mtime and an edit made between the check and the write is not overwritten.

Follow-up, not here

• Architecture 🔵: the ---/+++ header heuristic in rewritePatchPaths and splitPatchSections is line-local: a hunk that removes a line starting -- right before one adding a line starting ++ would be read as a header, giving a refusal with a nonsense reason (never a bad write). Tracking hunk state (@@ counts) closes it; noted in Disabled ticket-card buttons explain themselves through title, which Chromium hides and assistive tech skips #409's area of small follow-ups.

Rules the standard does not cover (left for a later edit of the instructions file): a handler pair that promises then performs (git:preview-patch / git:apply-patch) is reviewed as one unit, so a capability the writer gains the previewer gains too; a Git spawn inside a loop over user-supplied input needs a ceiling and a hoisted per-directory prefix; when an implementation is swapped for a tool, enumerate the old implementation's early returns and confirm each still has a test.

Checked and sound: notApplied equivalent to or stricter than the old patchIsAbsent; both ends of a rename in the snapshot; the sandbox is Git's (../ refused with 128, symlinks refused, patch text on stdin, mapToSrcLayout only ever prepends src/); one primitive on runGit with windowsArgs and nothing parsed from its output; legacySiteBlock still gates the handler; no require('isomorphic-git') under src/ and the package still shipped through @wp-playground/storage, as the body says.

Screenshots: none; the panel and its sentences are unchanged. The terminal line changes from the old "Applied N files." to the same text after Git's own check.

🤖 Generated with Claude Code

https://claude.ai/code/session_01J6koA9mvjSExVaKJp4nvSx

[juanmaguitar]

@coderabbitai review

[coderabbitai]

Warning

Review limit reached

Next included review available in 29 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: d3c5f31a-c38b-432d-a41d-6821200da9bd

📥 Commits

Reviewing files that changed from the base of the PR and between 247ebff and 4b34b76.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json, !package-lock.json

📒 Files selected for processing (19)

• .claude/skills/self-review/SKILL.md
• .github/instructions/code-review.instructions.md
• AGENTS.md
• docs/guide/applying-patches.md
• docs/guide/getting-started.md
• package.json
• src/git-read.cjs
• src/git-write.cjs
• src/main.js
• src/patch-apply.js
• src/patch-plan.cjs
• src/pr-files.cjs
• src/trunk-update.js
• tests/unit/git-write.integration.test.cjs
• tests/unit/git-write.test.cjs
• tests/unit/ipc-wiring.test.cjs
• tests/unit/patch-apply.integration.test.cjs
• tests/unit/patch-plan.test.cjs
• tests/unit/trunk-update.integration.test.cjs

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[juanmaguitar]

Windows walkthrough, 2026-09-09, Windows 11 VM with no Git installed, Buildkite artifact of #411 (a52e684, same tree as the current stack heads after the chain rebase).

With ticket 60001 linked and an edit in the WIP: apply PR #13444 of wordpress-develop (2 files under src/). status --porcelain shows both as M (nothing staged); the Review modal warns the PR is part of the checkout and refuses to submit it as the ticket's work. Applying the same PR again: "PR #13444 is already applied. Revert it before applying another patch. The checkout was not changed." Revert: "Reverted the patch", status empty, the ticket's own edit still there and the Review diff back to that one hunk.