Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 10 additions & 1 deletion lib/containers/pr-changed-files-container.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,16 @@ export default class PullRequestChangedFilesContainer extends React.Component {
}

buildPatch(rawDiff) {
const diffs = parseDiff(rawDiff);
const diffs = parseDiff(rawDiff).map(diff => {

Choose a reason for hiding this comment

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

do we also need to worry about where we're using parseDiff in git-shell-out-strategy? If so, we should consider extracting this functionality to some kind of helper.

I seem to remember some chatter that git gives us different output than the REST api does, so this might not actually be a problem.

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 seem to remember some chatter that git gives us different output than the REST api does,

Indeed! It is a git diff default behaviour to use a/ to prefix the source file and b/ to prefix the destination file. In git-shell-out-strategy, we work around this by using --no-prefix flag to get diffs, but for changed files tab, we don't have that option since we're getting the diff from the REST API.

// diff coming from API will have the defaul git diff prefixes a/ and b/
// e.g. a/file1.js and b/file2.js
// see https://git-scm.com/docs/git-diff#_generating_patches_with_p
return {
...diff,
newPath: diff.newPath ? diff.newPath.replace(/^[a|b]\//, '') : diff.newPath,
oldPath: diff.oldPath ? diff.oldPath.replace(/^[a|b]\//, '') : diff.oldPath,
};
});
return buildMultiFilePatch(diffs);
}

Expand Down
9 changes: 8 additions & 1 deletion test/containers/pr-changed-files-container.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React from 'react';
import {shallow} from 'enzyme';
import {parse as parseDiff} from 'what-the-diff';

import rawDiff from '../fixtures/diffs/raw-diff';
import {rawDiff, rawDiffWithPathPrefix} from '../fixtures/diffs/raw-diff';
import {buildMultiFilePatch} from '../../lib/models/patch';
import {getEndpoint} from '../../lib/models/endpoint';

Expand Down Expand Up @@ -72,6 +72,13 @@ describe('PullRequestChangedFilesContainer', function() {
assert.strictEqual(diffURL, 'https://api.github.com/repos/smashwilson/pushbot/pulls/12');
});

it('builds multifilepatch without the a/ and b/ prefixes in file paths', function() {
const wrapper = shallow(buildApp());
const {filePatches} = wrapper.instance().buildPatch(rawDiffWithPathPrefix);
assert.notMatch(filePatches[0].newFile.path, /^[a|b]\//);
assert.notMatch(filePatches[0].oldFile.path, /^[a|b]\//);
});

it('passes loaded diff data through to the controller', async function() {
const wrapper = shallow(buildApp({
token: '4321',
Expand Down
14 changes: 13 additions & 1 deletion test/fixtures/diffs/raw-diff.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import dedent from 'dedent-js';

const rawDiff = dedent`
diff --git file.txt file.txt
index 83db48f..bf269f4 100644
Expand All @@ -10,4 +11,15 @@ const rawDiff = dedent`
+new line
line3
`;
export default rawDiff;
const rawDiffWithPathPrefix = dedent`
diff --git a/badpath.txt b/badpath.txt
index af607bb..cfac420 100644
--- a/badpath.txt
+++ b/badpath.txt
@@ -1,2 +1,3 @@
line0
-line1
+line1.5
+line2
`;
export {rawDiff, rawDiffWithPathPrefix};