Skip to content

Split diffs based on file headers instead of 'Index:' metadata #88

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

Merged
merged 5 commits into from
Nov 12, 2015
Merged

Split diffs based on file headers instead of 'Index:' metadata #88

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
acb20d3
Split diffs based on file headers instead of 'Index:' metadata
piranna Oct 29, 2015
a4178de
Removed style changes
piranna Oct 29, 2015
41e3e29
Fix lint on regexp
piranna Nov 5, 2015
e9a40c3
Merge remote-tracking branch 'origin/splitDiffs' into splitDiffs
piranna Nov 5, 2015
3ab1df8
Less restrictive regexp & 'should handle patches without Index' test
piranna Nov 6, 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
43 changes: 21 additions & 22 deletions src/patch/parse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,41 +7,40 @@ export function parsePatch(uniDiff, options = {}) {
let index = {};
list.push(index);

// Ignore any leading junk
// Parse diff metadata
while (i < diffstr.length) {
if (/^(Index:|diff -r|@@)/.test(diffstr[i])) {
let line = diffstr[i];

// File header found, end parsing diff metadata
if (/^(\-\-\-|\+\+\+|@@)/.test(line)) {
Copy link
Owner

Choose a reason for hiding this comment

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

@@ isn't in the spec is it? This seems to be a change the previous behavior.

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 left it because it was set before. We could remove it if you want, but it doesn't hurt and would allow to accept patches without the file headers (I was thinking about propose it in other pull-request).

break;
}
i++;
}

let header = (/^(?:Index:|diff(?: -r \w+)+) (.*)/.exec(diffstr[i]));
if (header) {
index.index = header[1];
i++;

if (/^===/.test(diffstr[i])) {
i++;
// Diff index
let header = /^(?:Index:|diff(?: -r \w+)+)\s+(.+?)\s*$/.exec(line);
if (header) {
index.index = header[1];
}

parseFileHeader(index);
parseFileHeader(index);
} else {
// Ignore erant header components that might occur at the start of the file
parseFileHeader({});
parseFileHeader({});
i++;
}

parseFileHeader(index);
parseFileHeader(index);

// Parse hunks
index.hunks = [];

while (i < diffstr.length) {
if (/^(Index:|diff -r)/.test(diffstr[i])) {
let line = diffstr[i];

if (/^(Index:|diff|\-\-\-|\+\+\+)\s/.test(line)) {
break;
} else if (/^@@/.test(diffstr[i])) {
} else if (/^@@\s/.test(line)) {
Copy link
Owner

Choose a reason for hiding this comment

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

More restrictive than before. Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To prevent the case where the line start with @@whatever, that in strict mode it should throw an error instead of think it's a new hunk.

Copy link
Owner

Choose a reason for hiding this comment

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

Relax both this and the changes down below. This is not a spec we are dealing with but a bunch of random projects that are hobbled together based on arbitrary documentation. We should follow Postel's law here.

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 we should follow the spec and the projects bad using it should fix themselves because being less restrictive here would lead to errors otherwise, but since we already are accepting hunks without file headers (that's not supported by the standard, but there's no technical issues for accept it) and there's other projects using the library that would break, I'll remove the \s.

index.hunks.push(parseHunk());
} else if (diffstr[i] && options.strict) {
} else if (line && options.strict) {
// Ignore unexpected content unless in strict mode
throw new Error('Unknown line ' + (i + 1) + ' ' + JSON.stringify(diffstr[i]));
throw new Error('Unknown line ' + (i + 1) + ' ' + JSON.stringify(line));
} else {
i++;
}
Expand All @@ -51,7 +50,7 @@ export function parsePatch(uniDiff, options = {}) {
// Parses the --- and +++ headers, if none are found, no lines
// are consumed.
function parseFileHeader(index) {
let fileHeader = (/^(\-\-\-|\+\+\+)\s(\S+)\s?(.*)/.exec(diffstr[i]));
let fileHeader = /^(\-\-\-|\+\+\+)\s+(\S+)\s?(.+)/.exec(diffstr[i]);
Copy link
Owner

Choose a reason for hiding this comment

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

Your parenthesis change here introduces a linter error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, I missed it. Done.

if (fileHeader) {
let keyPrefix = fileHeader[1] === '---' ? 'old' : 'new';
index[keyPrefix + 'FileName'] = fileHeader[2];
Expand Down
4 changes: 2 additions & 2 deletions test/patch/parse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,8 +248,8 @@ Index: test2
});
it('should throw on invalid input in strict mode', function() {
expect(function() {
parsePatch('Index: foo\nfoo', {strict: true});
}).to['throw'](/Unknown line 2 "foo"/);
parsePatch('Index: foo\n+++', {strict: true});
}).to['throw'](/Unknown line 2 "\+\+\+"/);
Copy link
Owner

Choose a reason for hiding this comment

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

Please add positive test cases.

});
});
});