Skip to content

Commit b730f71

Browse files
authored
refactor(gumby): use only diff.patch to apply patch #4306
Problem: The current implementation of the diff viewer relies on the full changed source code being returned by the ExportResultArchive API. This is unnecessary and led to some customer issues when the download size was too large. We should be able to show the diff viewer using only the diff.patch file returned by the API. Solution: Get rid of the "parse-diff" library and instead use "diff" (pre-existing dependency) to apply the patch to the user's project.
1 parent aef6057 commit b730f71

File tree

5 files changed

+90
-104
lines changed

5 files changed

+90
-104
lines changed

src/codewhisperer/models/model.ts

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -248,12 +248,6 @@ export enum BuildSystem {
248248
Unknown = 'Unknown',
249249
}
250250

251-
export enum DropdownStep {
252-
STEP_1 = 1,
253-
STEP_2 = 2,
254-
STEP_3 = 3,
255-
}
256-
257251
export class ZipManifest {
258252
sourcesRoot: string = 'sources/'
259253
dependenciesRoot: string | undefined = 'dependencies/'
@@ -276,6 +270,7 @@ export class TransformByQState {
276270
private summaryFilePath: string = ''
277271

278272
private resultArchiveFilePath: string = ''
273+
private projectCopyFilePath: string = ''
279274

280275
private polledJobStatus: string = ''
281276

@@ -349,6 +344,10 @@ export class TransformByQState {
349344
return this.resultArchiveFilePath
350345
}
351346

347+
public getProjectCopyFilePath() {
348+
return this.projectCopyFilePath
349+
}
350+
352351
public getJobFailureReason() {
353352
return this.jobFailureReason
354353
}
@@ -425,6 +424,10 @@ export class TransformByQState {
425424
this.resultArchiveFilePath = filePath
426425
}
427426

427+
public setProjectCopyFilePath(filePath: string) {
428+
this.projectCopyFilePath = filePath
429+
}
430+
428431
public setJobFailureReason(reason: string) {
429432
this.jobFailureReason = reason
430433
}

src/codewhisperer/service/transformationResultsViewProvider.ts

Lines changed: 67 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
import AdmZip from 'adm-zip'
77
import os from 'os'
88
import fs from 'fs-extra'
9-
import parseDiff from 'parse-diff'
9+
import { parsePatch, applyPatches, ParsedDiff } from 'diff'
1010
import path from 'path'
1111
import vscode from 'vscode'
1212

@@ -102,31 +102,6 @@ export class AddedChangeNode extends ProposedChangeNode {
102102
}
103103
}
104104

105-
export class RemovedChangeNode extends ProposedChangeNode {
106-
readonly pathToOldContents: string
107-
override resourcePath: string
108-
109-
constructor(pathToOldContents: string) {
110-
super()
111-
this.pathToOldContents = pathToOldContents
112-
this.resourcePath = pathToOldContents
113-
}
114-
115-
override generateCommand(): vscode.Command {
116-
return {
117-
command: 'vscode.open',
118-
arguments: [vscode.Uri.file(this.pathToOldContents)],
119-
title: 'Removed Change',
120-
}
121-
}
122-
override generateDescription(): string {
123-
return 'R'
124-
}
125-
override saveFile(): void {
126-
fs.removeSync(this.pathToOldContents)
127-
}
128-
}
129-
130105
enum ReviewState {
131106
ToReview,
132107
Reviewed_Accepted,
@@ -137,19 +112,74 @@ export class DiffModel {
137112
changes: ProposedChangeNode[] = []
138113

139114
/**
140-
*
115+
* This function creates a copy of the changed files of the user's project so that the diff.patch can be applied to them
116+
* @param pathToWorkspace Path to the project that was transformed
117+
* @param changedFiles List of files that were changed
118+
* @returns Path to the folder containing the copied files
119+
*/
120+
public copyProject(pathToWorkspace: string, changedFiles: ParsedDiff[]) {
121+
const pathToTmpSrcDir = path.join(os.tmpdir(), `project-copy-${Date.now()}`)
122+
fs.mkdirSync(pathToTmpSrcDir)
123+
changedFiles.forEach(file => {
124+
const pathToTmpFile = path.join(pathToTmpSrcDir, file.oldFileName!.substring(2))
125+
// use mkdirsSync to create parent directories in pathToTmpFile too
126+
fs.mkdirsSync(path.dirname(pathToTmpFile))
127+
const pathToOldFile = path.join(pathToWorkspace, file.oldFileName!.substring(2))
128+
// pathToOldFile will not exist for new files such as summary.md
129+
if (fs.existsSync(pathToOldFile)) {
130+
fs.copyFileSync(pathToOldFile, pathToTmpFile)
131+
}
132+
})
133+
return pathToTmpSrcDir
134+
}
135+
136+
/**
141137
* @param pathToDiff Path to the diff.patch file expected to be located in the archive returned by ExportResultsArchive
142-
* @param pathToTmpSrcDir Path to the directory containing changed source files
143-
* @param pathToWorkspace Path to the current open workspace directory
144-
* @returns
138+
* @param pathToWorkspace Path to the project that was transformed
139+
* @returns List of nodes containing the paths of files that were modified, added, or removed
145140
*/
146-
public parseDiff(pathToDiff: string, pathToTmpSrcDir: string, pathToWorkspace: string): ProposedChangeNode[] {
141+
public parseDiff(pathToDiff: string, pathToWorkspace: string): ProposedChangeNode[] {
147142
const diffContents = fs.readFileSync(pathToDiff, 'utf8')
148-
const changedFiles = parseDiff(diffContents)
149-
143+
const changedFiles = parsePatch(diffContents)
144+
// path to the directory containing copy of the changed files in the transformed project
145+
const pathToTmpSrcDir = this.copyProject(pathToWorkspace, changedFiles)
146+
transformByQState.setProjectCopyFilePath(pathToTmpSrcDir)
147+
148+
applyPatches(changedFiles, {
149+
loadFile: function (fileObj, callback) {
150+
// load original contents of file
151+
const filePath = path.join(pathToWorkspace, fileObj.oldFileName!.substring(2))
152+
if (!fs.existsSync(filePath)) {
153+
// must be a new file (ex. summary.md), so pass empty string as original contents and do not pass error
154+
callback(undefined, '')
155+
} else {
156+
// must be a modified file (most common), so pass original contents
157+
const fileContents = fs.readFileSync(filePath, 'utf-8')
158+
callback(undefined, fileContents)
159+
}
160+
},
161+
// by now, 'content' contains the changes from the patch
162+
patched: function (fileObj, content, callback) {
163+
const filePath = path.join(pathToTmpSrcDir, fileObj.newFileName!.substring(2))
164+
// write changed contents to the copy of the original file (or create a new file)
165+
fs.writeFileSync(filePath, content)
166+
callback(undefined)
167+
},
168+
complete: function (err) {
169+
if (err) {
170+
getLogger().error(`CodeTransform: ${err} when applying patch`)
171+
} else {
172+
getLogger().info('CodeTransform: Patch applied successfully')
173+
}
174+
},
175+
})
150176
this.changes = changedFiles.flatMap(file => {
151-
const originalPath = path.join(pathToWorkspace, file.from !== undefined ? file.from : '')
152-
const tmpChangedPath = path.join(pathToTmpSrcDir, file.to !== undefined ? file.to : '')
177+
/* ex. file.oldFileName = 'a/src/java/com/project/component/MyFile.java'
178+
* ex. file.newFileName = 'b/src/java/com/project/component/MyFile.java'
179+
* use substring(2) to ignore the 'a/' and 'b/'
180+
*/
181+
const originalPath = path.join(pathToWorkspace, file.oldFileName!.substring(2))
182+
const tmpChangedPath = path.join(pathToTmpSrcDir, file.newFileName!.substring(2))
153183

154184
const originalFileExists = fs.existsSync(originalPath)
155185
const changedFileExists = fs.existsSync(tmpChangedPath)
@@ -158,8 +188,6 @@ export class DiffModel {
158188
return new ModifiedChangeNode(originalPath, tmpChangedPath)
159189
} else if (!originalFileExists && changedFileExists) {
160190
return new AddedChangeNode(originalPath, tmpChangedPath)
161-
} else if (originalFileExists && !changedFileExists) {
162-
return new RemovedChangeNode(originalPath)
163191
}
164192
return []
165193
})
@@ -322,7 +350,6 @@ export class ProposedTransformationExplorer {
322350
zip.extractAllTo(pathContainingArchive)
323351
diffModel.parseDiff(
324352
path.join(pathContainingArchive, ExportResultArchiveStructure.PathToDiffPatch),
325-
path.join(pathContainingArchive, ExportResultArchiveStructure.PathToSourceDir),
326353
transformByQState.getProjectPath()
327354
)
328355
await vscode.commands.executeCommand(
@@ -373,8 +400,9 @@ export class ProposedTransformationExplorer {
373400
await vscode.commands.executeCommand('setContext', 'gumby.reviewState', TransformByQReviewStatus.NotStarted)
374401
transformDataProvider.refresh()
375402
await vscode.window.showInformationMessage(CodeWhispererConstants.changesAppliedMessage)
376-
// delete result archive after changes accepted
403+
// delete result archive and copied source code after changes accepted
377404
fs.rmSync(transformByQState.getResultArchiveFilePath(), { recursive: true, force: true })
405+
fs.rmSync(transformByQState.getProjectCopyFilePath(), { recursive: true, force: true })
378406
telemetry.codeTransform_vcsViewerSubmitted.emit({
379407
codeTransformSessionId: codeTransformTelemetryState.getSessionId(),
380408
codeTransformJobId: transformByQState.getJobId(),

src/test/codewhisperer/service/resources/modifiedFile.diff

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,6 @@ index 084f378..1705101 100644
33
--- a/README.md
44
+++ b/README.md
55
@@ -1,3 +1,5 @@
6-
+This line is added
7-
+
8-
# Building Java Projects with Gradle
9-
10-
##### This guide walks you through using Gradle to build a simple Java project.
6+
This guide walks you through using Gradle to build a simple Java project.
7+
+This line is added.
8+
\ No newline at end of file

src/test/codewhisperer/service/resources/removedFile.diff

Lines changed: 0 additions & 25 deletions
This file was deleted.

src/test/codewhisperer/service/transformationResultsHandler.test.ts

Lines changed: 11 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,10 @@
55
import assert from 'assert'
66
import sinon from 'sinon'
77
import fs from 'fs-extra'
8+
import os from 'os'
89
import {
910
DiffModel,
1011
AddedChangeNode,
11-
RemovedChangeNode,
1212
ModifiedChangeNode,
1313
} from '../../../codewhisperer/service/transformationResultsViewProvider'
1414
import path from 'path'
@@ -26,7 +26,6 @@ describe('DiffModel', function () {
2626
const testDiffModel = new DiffModel()
2727

2828
const workspacePath = 'workspace'
29-
const tmpPath = 'tmp'
3029

3130
sinon.replace(fs, 'existsSync', path => {
3231
const pathStr = path.toString()
@@ -37,50 +36,33 @@ describe('DiffModel', function () {
3736
return true
3837
})
3938

40-
testDiffModel.parseDiff(getTestFilePath('resources/addedFile.diff'), tmpPath, workspacePath)
39+
testDiffModel.parseDiff(getTestFilePath('resources/addedFile.diff'), workspacePath)
4140

4241
assert.strictEqual(testDiffModel.changes.length, 1)
4342
const change = testDiffModel.changes[0]
4443

4544
assert.strictEqual(change instanceof AddedChangeNode, true)
4645
})
4746

48-
it('WHEN parsing a diff patch where a file was removed THEN returns an array representing the removed file', async function () {
49-
const testDiffModel = new DiffModel()
50-
51-
const workspacePath = 'workspace'
52-
const tmpPath = 'tmp'
53-
54-
sinon.replace(fs, 'existsSync', path => {
55-
const pathStr = path.toString()
56-
if (pathStr.includes(workspacePath)) {
57-
return true
58-
}
59-
60-
return false
61-
})
62-
63-
testDiffModel.parseDiff(getTestFilePath('resources/removedFile.diff'), tmpPath, workspacePath)
64-
65-
assert.strictEqual(testDiffModel.changes.length, 1)
66-
const change = testDiffModel.changes[0]
67-
68-
assert.strictEqual(change instanceof RemovedChangeNode, true)
69-
})
70-
7147
it('WHEN parsing a diff patch where a file was modified THEN returns an array representing the modified file', async function () {
7248
const testDiffModel = new DiffModel()
7349

74-
const workspacePath = 'workspace'
75-
const tmpPath = 'tmp'
50+
const workspacePath = os.tmpdir()
7651

7752
sinon.replace(fs, 'existsSync', path => true)
7853

79-
testDiffModel.parseDiff(getTestFilePath('resources/modifiedFile.diff'), tmpPath, workspacePath)
54+
fs.writeFileSync(
55+
path.join(workspacePath, 'README.md'),
56+
'This guide walks you through using Gradle to build a simple Java project.'
57+
)
58+
59+
testDiffModel.parseDiff(getTestFilePath('resources/modifiedFile.diff'), workspacePath)
8060

8161
assert.strictEqual(testDiffModel.changes.length, 1)
8262
const change = testDiffModel.changes[0]
8363

8464
assert.strictEqual(change instanceof ModifiedChangeNode, true)
65+
66+
fs.rmSync(path.join(workspacePath, 'README.md'))
8567
})
8668
})

0 commit comments

Comments
 (0)