kpdecker / jsdiff

A javascript text differencing implementation.
BSD 3-Clause "New" or "Revised" License
7.92k stars 496 forks source link

Default value of line delimiters when a patch is applied (#228) #393

Closed Cinedin closed 8 months ago

Cinedin commented 1 year ago

Fixes #228

Since a structured patch does not define its line delimiters (#157), this change is needed to prevent an error when the returned value of structuredPatch is used in applyPatch. The current API does not change:

const oldContent = 'ANYTHING';
const newContent = 'SOMETHING';
const patch = structuredPatch('test.txt', 'test.txt', oldContent, newContent);

expect(applyPatch(oldContent, patch)).to.equal(newContent);
ExplodingCabbage commented 8 months ago

Hmm. This PR is roughly a duplicate of https://github.com/kpdecker/jsdiff/pull/384. I have the same doubt about it as I do about #384, though: wouldn't it make more sense to update structuredPatch to include a linedelimiters property in its output, thus making its return type consistent with parsePatch, rather than modifying applyPatch to accommodate the (arbitrary-seemingly, probably accidental?) inconsistency between the two patch formats?

ExplodingCabbage commented 8 months ago

I provisionally plan to fix this by fixing structuredPatch, and to leave applyPatch alone (and close both this and #384), but I am interested in hearing out any argument for why modifying applyPatch would be better!

ExplodingCabbage commented 8 months ago

After looking more closely, I think we should just rip out all the linedelimiters logic from parsePatch and always assume, in there and in applyPatch, that the line delimiter is a \n. The stuff parsePatch is doing, letting you have a patch where lines of the patch are delimited by, say, vertical tab characters, is inconsistent with the diff and patch Unix tools, seems kinda pointless to me, and breaks jsdiff's ability to correctly parse a unified diff format diff generated using diff -u foo bar if foo or bar contained any of the characters (like vertical tabs) that jsdiff treats as line breaks in the diff but that the diff and patch CLI tools just treat as ordinary characters.

I'm gonna merge this, and then put up another PR fixing the linedelimiters stuff.