Add Git support, and otherwise variously improve & fix parsePatch (and other unified diff format functions)#672
Conversation
ExplodingCabbage
force-pushed
the
git-support-attempt-2-with-claude-opus
branch
from
ea3046c to
bfaa2e2
Compare
…-2-with-claude-opus Conflicts: release-notes.md
ExplodingCabbage
changed the title
|
I suppose it's worth letting Copilot review before I merge this. Let's see if I get anything useful from it... |
There was a problem hiding this comment.
Pull request overview
This PR expands jsdiff’s unified-diff tooling to understand and emit Git-style patches (including extended headers for renames/copies/creates/deletes/mode changes), while also improving robustness around parsing/formatting edge cases and adding extensive test coverage and documentation updates.
Changes:
- Add parsing/formatting/reversing support for
diff --gitpatches (extended headers, hunkless patches, quoting/escaping). - Make
parsePatchmore tolerant of real-world “garbage” between/after diffs and add stricter validation for unpaired---/+++headers and hunk line-count spillover. - Update types/docs/release notes and add new tests (including a README example-backed integration test).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/patch/reverse.js | Adds/updates reversePatch expectations for Git rename/copy/create/delete/mode-change cases. |
| test/patch/readme-rename-example.js | New Node-only test that executes the README Git-patch example against real files. |
| test/patch/parse.js | Adds extensive parsePatch coverage for GNU diff, SVN noise, trailing garbage, and Git extended headers/quoting edge cases. |
| test/patch/create.js | Adds formatPatch tests for undefined filenames, non-mutation, and Git header emission/round-tripping. |
| src/types.ts | Extends StructuredPatch to allow undefined filenames and adds Git-related metadata fields. |
| src/patch/reverse.ts | Updates reversePatch to handle Git prefixes, modes, create/delete flags, and copy/rename semantics. |
| src/patch/parse.ts | Enhances parsePatch to recognize Git diffs + extended headers and tolerate interspersed non-diff content. |
| src/patch/create.ts | Enhances formatPatch to emit Git-style headers/extended headers and to quote filenames when needed. |
| release-notes.md | Documents new behavior, breaking type change, and Git diff support. |
| karma.conf.js | Excludes the new Node-dependent README example test from browser runs. |
| README.md | Documents Git patch behavior/fields and adds an applyPatches example for renames/copies/mode changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function quoteFileNameIfNeeded(s: string): string { | ||
| if (!needsQuoting(s)) { | ||
| return s; | ||
| } | ||
|
|
||
| let result = '"'; | ||
| const bytes = new TextEncoder().encode(s); | ||
| let i = 0; |
There was a problem hiding this comment.
quoteFileNameIfNeeded unconditionally uses new TextEncoder() to UTF-8 encode filenames. TextEncoder is not available in all ES5 environments (e.g. older browsers), so this can throw at runtime despite the README claiming ES5 compatibility. Consider feature-detecting TextEncoder and falling back to a small UTF-8 encoder (or a Node-only Buffer.from(str,'utf8') path) so patch formatting doesn’t hard-require the Encoding API.
| const bytes = [parseInt(s.substring(j, j + 3), 8)]; | ||
| j += 3; | ||
| while (s[j] === '\\' && s[j + 1] >= '0' && s[j + 1] <= '7') { | ||
| if (j + 3 >= s.length || s[j + 2] < '0' || s[j + 2] > '7' || s[j + 3] < '0' || s[j + 3] > '7') { | ||
| return null; | ||
| } | ||
| bytes.push(parseInt(s.substring(j + 1, j + 4), 8)); | ||
| j += 4; | ||
| } | ||
| result += new TextDecoder('utf-8').decode(new Uint8Array(bytes)); | ||
| continue; // j already points at the next character |
There was a problem hiding this comment.
parseQuotedFileName decodes octal escapes via new TextDecoder('utf-8') and Uint8Array. These globals are not guaranteed in ES5 environments, so parsing quoted filenames can crash at runtime in older browsers. Please add a fallback UTF-8 decoder (or conditional use of the Encoding API) so parsePatch remains usable where ES5 is supported.
| let gitOldName = patch.oldFileName ?? ''; | ||
| let gitNewName = patch.newFileName ?? ''; | ||
| if (patch.isCreate && gitOldName === '/dev/null') { | ||
| gitOldName = gitNewName.replace(/^b\//, 'a/'); | ||
| } else if (patch.isDelete && gitNewName === '/dev/null') { | ||
| gitNewName = gitOldName.replace(/^a\//, 'b/'); | ||
| } | ||
| ret.push('diff --git ' + quoteFileNameIfNeeded(gitOldName) + ' ' + quoteFileNameIfNeeded(gitNewName)); |
There was a problem hiding this comment.
When patch.isGit is true but oldFileName/newFileName are undefined, this emits diff --git (empty paths) plus potentially other extended headers. That output isn’t a valid Git/unified diff and is probably worse than omitting headers or throwing. Consider either (a) skipping the git header when filenames are missing, aligning with the “silently skip headers when filenames are undefined” behavior, or (b) throwing a clear error for isGit patches without filenames.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
See the release notes for more detail on what's changed here.
The code (but not the prose and comments) were written pretty much exclusively by Claude Opus 4.6. I rewrote the release notes and most of the docs from scratch and significantly edited most of the comments, because Opus is stupid about writing prose, and I had to handhold it through a lot of modifications to the code to get to the point that everything was correct. Still, feels like without AI adding support for Git's diff format would've felt like too big a task to even try to take on, and each of the many, many points where we discovered an additional complication we hadn't considered or yet another pre-existing bug would've further dashed my morale. With AI, it's done!