Skip to content

Allow patchs in diff format #83

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 13 commits into from
Closed

Allow patchs in diff format #83

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0c9dd6d
Allow patchs in diff format
piranna Oct 15, 2015
d501881
Allow headers with several revisions
piranna Oct 15, 2015
c238f85
Fix call to 'options.loadFile' with empty index
piranna Oct 15, 2015
54edda7
Separated hunk sanity check from applyPatch algorythm
piranna Oct 15, 2015
836a2f8
Do sanity check of all hunks on the diff at once before start applyin…
piranna Oct 16, 2015
720f5ae
Calculate and apply location offset where hunk context can fit
piranna Oct 16, 2015
0c2af7b
Consider correctly both text limits instead of fail when reaching one
piranna Oct 16, 2015
d723875
Test for hunk offsets
piranna Oct 16, 2015
c6b6a02
Improved code documentation
piranna Oct 21, 2015
3908658
Made code easier to understand & minor optimizations
piranna Oct 26, 2015
09b7efe
Split logic for fit candidates on an iterator
piranna Oct 26, 2015
f91668b
Generate library on 'npm install' and 'npm publish' automatically
piranna Oct 26, 2015
3958558
Use explicit undefined
piranna Oct 26, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 66 additions & 6 deletions src/patch/apply.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,21 @@ export function applyPatch(source, uniDiff, options = {}) {
compareLine = options.compareLine || ((lineNumber, line, operation, patchContent) => line === patchContent),
errorCount = 0,
fuzzFactor = options.fuzzFactor || 0,
minLine = 0,
offset = 0,

removeEOFNL,
addEOFNL;

for (let i = 0; i < hunks.length; i++) {
let hunk = hunks[i],
toPos = hunk.newStart - 1;

// Sanity check the input string. Bail if we don't match.
/**
* Checks if the hunk exactly fits on the provided location
*/
function hunkFits(hunk, toPos) {
for (let j = 0; j < hunk.lines.length; j++) {
let line = hunk.lines[j],
operation = line[0],
content = line.substr(1);

if (operation === ' ' || operation === '-') {
// Context sanity check
if (!compareLine(toPos + 1, lines[toPos], operation, content)) {
Expand All @@ -42,8 +44,66 @@ export function applyPatch(source, uniDiff, options = {}) {
return false;
}
}
toPos++;
}
}

return true;
}

// Search best fit offsets for each hunk based on the previous ones
for (let i = 0; i < hunks.length; i++) {
let hunk = hunks[i],
outOfLimits = 0,
localOffset = 0,
toPos = offset + hunk.oldStart - 1;

for (;;) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic here is really hard to follow and I worry that it's going to lead to confusion down the road. Rather than having side effects that terminate the loop in the middle of the block and the loop written as an infinite loop, I think it would be clearer to have the termination case expressed in the for/while statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an optimization, the alternative is to have the old code and use a conditional to check it, or do the check twice on the most common case (the hunk fits at the first try, when localOffset is zero). This has only moved the block to check the current or next line before the loop, so it's checked always in a loop rollback fashion. The same would be achieved by doing the check previously to enter the loop, but code is duplicated. I can add more comments if you don't see it clear.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After coming back to this a few days later, I'm still worried that I'm not going to be able to maintain this.

What if instead we extract out the iteration logic into an iterator? Something like:

function distanceIterator(target, minimum, maximum) {
  let wantForward = true,
      backwardExhausted = false,
      forwardExhausted = false,
      offset = 0;
  return function iterate() {
    // Continue iteration on the rear when the 
    if (forwardExhausted) {
      wantForward = false;
      offset++;
    }

    if (!wantForward) {
      // Check if we have a potential position behind us
      let candidate = target - offset;
      if (candidate > minimum) {
        wantForward = true;
        return candidate;
      } else {
        backwardExhausted = true;
      }
    }

    // Check if we have a potential position in front of us
    candidate = target + offset;
    if (candidate < maximium) {
      wantForward = false;
      offset++;
      return candidate;
    } else if (!backwardExhausted) {
      // We've exhausted the forward section, reenter to continue
      // iteration on the behind.
      forwardExhausted = true;
      return iterate();
    }
  };
}

Which can live in it's own file and we can unit test in isolation. This loop here then becomes something like:

for (let i = 0; i < hunks.length; i++) {
  let ...
      toPos = offest + hunk.oldStart + 1,
      maxLine = lines.length - hunk.oldLines,
      iterator = distanceIterator(toPos, minLine, maxLine),
      toCheck = iterator();

  while (toCheck !== null) {
    if (hunkFits(hunk, toCheck)) {
      hunk.offset = toCheck;
      break;
    }

    toCheck = iterator();
  }
}

(I have not actually tested or even run this code, so it might need some work to get it running)

I think separating general flip flopping iteration and checking the hunk itself into separate code will help with clarity as well as give us a nice reusable component for this iteration should we need it in other locations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think separating general flip flopping iteration and checking the hunk itself into separate code will help with clarity as well as give us a nice reusable component for this iteration should we need it in other locations.

I find this code a little bit more complicated and less eficient, but I like the idea of separate the flip-flop logic from the main loop. I'll fetch your code and try to check it out :-)

// Check if trying to fit beyond text length, and if not, check it fits
// after offset location (or desired location on first iteration)
if (lines.length < toPos + localOffset + hunk.oldLines) {
outOfLimits++;
} else if (hunkFits(hunk, toPos + localOffset)) {
hunk.offset = offset += localOffset;
break;
}

// If we tried to fit hunk before text beginning and beyond text lenght,
// then hunk can't be fit on the text so we raise an error
if (outOfLimits === 2) {
return false;
}

// Reset checks of trying to fit outside text limits and increase offset
// of the current hunk relative to its desired location
outOfLimits = 0;
localOffset++;

// Check if trying to fit before text beginning, and if not, check it fits
// before offset location
if (toPos - localOffset < minLine) {
outOfLimits++;
} else if (hunkFits(hunk, toPos - localOffset)) {
hunk.offset = offset -= localOffset;
break;
}
}

// Set lower text limit to end of the current hunk, so next ones don't try
// to fit over already patched text
minLine = hunk.offset + hunk.oldStart + hunk.oldLines;
}

// Apply patch hunks
for (let i = 0; i < hunks.length; i++) {
let hunk = hunks[i],
toPos = hunk.offset + hunk.newStart - 1;

for (let j = 0; j < hunk.lines.length; j++) {
let line = hunk.lines[j],
operation = line[0],
content = line.substr(1);

if (operation === ' ') {
toPos++;
} else if (operation === '-') {
Expand Down Expand Up @@ -84,7 +144,7 @@ export function applyPatches(uniDiff, options) {
function processIndex() {
let index = uniDiff[currentIndex++];
if (!index) {
options.complete();
return options.complete();
}

options.loadFile(index, function(err, data) {
Expand Down
6 changes: 3 additions & 3 deletions src/patch/parse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@ export function parsePatch(uniDiff, options = {}) {

// Ignore any leading junk
while (i < diffstr.length) {
if ((/^Index:/.test(diffstr[i])) || (/^@@/.test(diffstr[i]))) {
if (/^(Index:|diff -r|@@)/.test(diffstr[i])) {
break;
}
i++;
}

let header = (/^Index: (.*)/.exec(diffstr[i]));
let header = (/^(?:Index:|diff(?: -r \w+)+) (.*)/.exec(diffstr[i]));
if (header) {
index.index = header[1];
i++;
Expand All @@ -35,7 +35,7 @@ export function parsePatch(uniDiff, options = {}) {
index.hunks = [];

while (i < diffstr.length) {
if (/^Index:/.test(diffstr[i])) {
if (/^(Index:|diff -r)/.test(diffstr[i])) {
break;
} else if (/^@@/.test(diffstr[i])) {
index.hunks.push(parseHunk());
Expand Down
21 changes: 21 additions & 0 deletions test/patch/apply.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,27 @@ describe('patch/apply', function() {
+ 'line5\n');
});

it('should succeed when hunk has an offset', function() {
expect(applyPatch(
'line1\n'
+ 'line3\n'
+ 'line4\n'
+ 'line5\n',

'--- test\theader1\n'
+ '+++ test\theader2\n'
+ '@@ -3,2 +3,3 @@\n'
+ ' line1\n'
+ '+line2\n'
+ ' line3\n'))
.to.equal(
'line1\n'
+ 'line2\n'
+ 'line3\n'
+ 'line4\n'
+ 'line5\n');
});

it('should allow custom line comparison', function() {
expect(applyPatch(
'line2\n'
Expand Down