From 20393f7c06caca9c67f71b099a4d2a62e7d7f58a Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 8 Nov 2016 20:25:32 -0800 Subject: [PATCH 01/49] Add GSOS#getStatus and write test for staged/unstaged files --- lib/git-shell-out-strategy.js | 37 +++++++++++++++++++++++++++++ test/git-shell-out-strategy.test.js | 27 +++++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index bc310d9e10..f5a173fcd2 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -129,6 +129,43 @@ export default class GitShellOutStrategy { /** * File Status and Diffs */ + async getStatus () { + const output = await this.exec(['status', '--untracked-files=all', '--porcelain']) + + const statusMap = { + A: 'added', + M: 'modified', + D: 'deleted', + U: 'modified', + '?': 'added' + } + + const stagedFiles = {} + const unstagedFiles = {} + const mergeConflictFiles = {} + let statusToHead + + output.trim().split(LINE_ENDING_REGEX).forEach(async (line) => { + const [_, x, y, filePath] = line.match(/(.)(.)\s(.*)/) + if (x === 'U' || y === 'U' || (x === 'A' && y === 'A')) { + if (!statusToHead) statusToHead = await this.diffFileStatus({target: 'HEAD'}) + return mergeConflictFiles[filePath] = { + ours: statusMap[x], + theirs: statusMap[y], + file: statusToHead[filePath] || 'equivalent' + } + } + if (y !== ' ') { + unstagedFiles[filePath] = statusMap[y] + } + if (x !== ' ' && x !== '?') { + stagedFiles[filePath] = statusMap[x] + } + }) + + return {stagedFiles, unstagedFiles, mergeConflictFiles} + } + async diffFileStatus (options = {}) { const args = ['diff', '--name-status', '--no-renames'] if (options.staged) args.push('--staged') diff --git a/test/git-shell-out-strategy.test.js b/test/git-shell-out-strategy.test.js index 576edb8ec6..0734a75c66 100644 --- a/test/git-shell-out-strategy.test.js +++ b/test/git-shell-out-strategy.test.js @@ -63,6 +63,33 @@ describe('Git commands', () => { }) }) + describe('getStatus', () => { + it('returns objects for staged and unstaged files, including status information', async () => { + const workingDirPath = await cloneRepository('three-files') + const git = new GitShellOutStrategy(workingDirPath) + fs.writeFileSync(path.join(workingDirPath, 'a.txt'), 'qux\nfoo\nbar\n', 'utf8') + fs.unlinkSync(path.join(workingDirPath, 'b.txt')) + fs.renameSync(path.join(workingDirPath, 'c.txt'), path.join(workingDirPath, 'd.txt')) + fs.writeFileSync(path.join(workingDirPath, 'e.txt'), 'qux', 'utf8') + await git.exec(['add', 'a.txt', 'e.txt']) + fs.writeFileSync(path.join(workingDirPath, 'a.txt'), 'modify after staging', 'utf8') + fs.writeFileSync(path.join(workingDirPath, 'e.txt'), 'modify after staging', 'utf8') + const {stagedFiles, unstagedFiles, mergeConflictFiles} = await git.getStatus() + assert.deepEqual(stagedFiles, { + 'a.txt': 'modified', + 'e.txt': 'added' + }) + assert.deepEqual(unstagedFiles, { + 'a.txt': 'modified', + 'b.txt': 'deleted', + 'c.txt': 'deleted', + 'd.txt': 'added', + 'e.txt': 'modified' + }) + assert.deepEqual(mergeConflictFiles, {}) + }) + }) + describe('diffFileStatus', () => { it('returns an object with working directory file diff status between relative to HEAD', async () => { const workingDirPath = await cloneRepository('three-files') From eaed1c6ee74654f1df5f083eabef3e245c64ddea Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 8 Nov 2016 20:30:07 -0800 Subject: [PATCH 02/49] Add GSOS.getStatus test for merge conflict situation --- lib/git-shell-out-strategy.js | 3 +- test/git-shell-out-strategy.test.js | 45 +++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index f5a173fcd2..036cc7d933 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -143,12 +143,11 @@ export default class GitShellOutStrategy { const stagedFiles = {} const unstagedFiles = {} const mergeConflictFiles = {} - let statusToHead + const statusToHead = await this.diffFileStatus({target: 'HEAD'}) output.trim().split(LINE_ENDING_REGEX).forEach(async (line) => { const [_, x, y, filePath] = line.match(/(.)(.)\s(.*)/) if (x === 'U' || y === 'U' || (x === 'A' && y === 'A')) { - if (!statusToHead) statusToHead = await this.diffFileStatus({target: 'HEAD'}) return mergeConflictFiles[filePath] = { ours: statusMap[x], theirs: statusMap[y], diff --git a/test/git-shell-out-strategy.test.js b/test/git-shell-out-strategy.test.js index 0734a75c66..960bfdbe48 100644 --- a/test/git-shell-out-strategy.test.js +++ b/test/git-shell-out-strategy.test.js @@ -88,6 +88,51 @@ describe('Git commands', () => { }) assert.deepEqual(mergeConflictFiles, {}) }) + + it('returns an object for merge conflict files, including ours/theirs/file status information', async () => { + const workingDirPath = await cloneRepository('merge-conflict') + const git = new GitShellOutStrategy(workingDirPath) + try { + await git.merge('origin/branch') + } catch (e) { + // expected + if (!e.message.match(/CONFLICT/)) { + throw new Error(`merge failed for wrong reason: ${e.message}`) + } + } + + const {stagedFiles, unstagedFiles, mergeConflictFiles} = await git.getStatus() + assert.deepEqual(stagedFiles, {}) + assert.deepEqual(unstagedFiles, {}) + + assert.deepEqual(mergeConflictFiles, { + 'added-to-both.txt': { + ours: 'added', + theirs: 'added', + file: 'modified' + }, + 'modified-on-both-ours.txt': { + ours: 'modified', + theirs: 'modified', + file: 'modified' + }, + 'modified-on-both-theirs.txt': { + ours: 'modified', + theirs: 'modified', + file: 'modified' + }, + 'removed-on-branch.txt': { + ours: 'modified', + theirs: 'deleted', + file: 'equivalent' + }, + 'removed-on-master.txt': { + ours: 'deleted', + theirs: 'modified', + file: 'added' + } + }) + }) }) describe('diffFileStatus', () => { From 216ffaefb123c6f182524f3868fdabb525625a3e Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 8 Nov 2016 20:31:11 -0800 Subject: [PATCH 03/49] :fire: unnecessary method GSOS.getMergeConflictFileStatus --- lib/git-shell-out-strategy.js | 27 ++---------------- lib/models/repository.js | 2 +- test/git-shell-out-strategy.test.js | 44 ----------------------------- 3 files changed, 3 insertions(+), 70 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index 036cc7d933..7765f23011 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -145,7 +145,8 @@ export default class GitShellOutStrategy { const mergeConflictFiles = {} const statusToHead = await this.diffFileStatus({target: 'HEAD'}) - output.trim().split(LINE_ENDING_REGEX).forEach(async (line) => { + output && output.split(LINE_ENDING_REGEX).forEach(async (line) => { + if (line === '') return const [_, x, y, filePath] = line.match(/(.)(.)\s(.*)/) if (x === 'U' || y === 'U' || (x === 'A' && y === 'A')) { return mergeConflictFiles[filePath] = { @@ -241,30 +242,6 @@ export default class GitShellOutStrategy { return this.exec(['merge', branchName]) } - async getMergeConflictFileStatus () { - const output = await this.exec(['status', '--short']) - const statusToHead = await this.diffFileStatus({target: 'HEAD'}) - - const statusMap = { - A: 'added', - U: 'modified', - D: 'deleted' - } - - const statusesByPath = {} - output && output.trim().split(LINE_ENDING_REGEX).forEach(line => { - const [oursTheirsStatus, filePath] = line.split(' ') - if (['DD', 'AU', 'UD', 'UA', 'DU', 'AA', 'UU'].includes(oursTheirsStatus)) { - statusesByPath[filePath] = { - ours: statusMap[oursTheirsStatus[0]], - theirs: statusMap[oursTheirsStatus[1]], - file: statusToHead[filePath] || 'equivalent' - } - } - }) - return statusesByPath - } - async isMerging () { try { await readFile(path.join(this.workingDir, '.git', 'MERGE_HEAD')) diff --git a/lib/models/repository.js b/lib/models/repository.js index 7e4ca8a9f2..c703e8de9b 100644 --- a/lib/models/repository.js +++ b/lib/models/repository.js @@ -214,7 +214,7 @@ export default class Repository { } async fetchMergeConflicts () { - const statusesByPath = await this.git.getMergeConflictFileStatus() + const statusesByPath = (await this.git.getStatusesForChangedFiles()).mergeConflictFiles const validConflicts = new Set() for (let path in statusesByPath) { const statuses = statusesByPath[path] diff --git a/test/git-shell-out-strategy.test.js b/test/git-shell-out-strategy.test.js index 960bfdbe48..02d3d913ab 100644 --- a/test/git-shell-out-strategy.test.js +++ b/test/git-shell-out-strategy.test.js @@ -348,50 +348,6 @@ describe('Git commands', () => { }) }) - describe('getMergeConflictFileStatus', () => { - it('returns an object with ours/theirs/file status by path', async () => { - const workingDirPath = await cloneRepository('merge-conflict') - const git = new GitShellOutStrategy(workingDirPath) - try { - await git.merge('origin/branch') - } catch (e) { - // expected - if (!e.message.match(/CONFLICT/)) { - throw new Error(`merge failed for wrong reason: ${e.message}`) - } - } - - const statusesByPath = await git.getMergeConflictFileStatus() - assert.deepEqual(statusesByPath, { - 'added-to-both.txt': { - ours: 'added', - theirs: 'added', - file: 'modified' - }, - 'modified-on-both-ours.txt': { - ours: 'modified', - theirs: 'modified', - file: 'modified' - }, - 'modified-on-both-theirs.txt': { - ours: 'modified', - theirs: 'modified', - file: 'modified' - }, - 'removed-on-branch.txt': { - ours: 'modified', - theirs: 'deleted', - file: 'equivalent' - }, - 'removed-on-master.txt': { - ours: 'deleted', - theirs: 'modified', - file: 'added' - } - }) - }) - }) - describe('isMerging', () => { it('returns true if `.git/MERGE_HEAD` exists', async () => { const workingDirPath = await cloneRepository('merge-conflict') From bce05b4b2de8a6dfa31c218265190350da866fe5 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 8 Nov 2016 21:02:11 -0800 Subject: [PATCH 04/49] Rename GSOS.getStatus -> GSOS.getStatusesForChangedFiles --- lib/git-shell-out-strategy.js | 2 +- test/git-shell-out-strategy.test.js | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index 7765f23011..7432dabc14 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -129,7 +129,7 @@ export default class GitShellOutStrategy { /** * File Status and Diffs */ - async getStatus () { + async getStatusesForChangedFiles () { const output = await this.exec(['status', '--untracked-files=all', '--porcelain']) const statusMap = { diff --git a/test/git-shell-out-strategy.test.js b/test/git-shell-out-strategy.test.js index 02d3d913ab..73c13e2edc 100644 --- a/test/git-shell-out-strategy.test.js +++ b/test/git-shell-out-strategy.test.js @@ -63,7 +63,7 @@ describe('Git commands', () => { }) }) - describe('getStatus', () => { + describe('getStatusesForChangedFiles', () => { it('returns objects for staged and unstaged files, including status information', async () => { const workingDirPath = await cloneRepository('three-files') const git = new GitShellOutStrategy(workingDirPath) @@ -74,7 +74,7 @@ describe('Git commands', () => { await git.exec(['add', 'a.txt', 'e.txt']) fs.writeFileSync(path.join(workingDirPath, 'a.txt'), 'modify after staging', 'utf8') fs.writeFileSync(path.join(workingDirPath, 'e.txt'), 'modify after staging', 'utf8') - const {stagedFiles, unstagedFiles, mergeConflictFiles} = await git.getStatus() + const {stagedFiles, unstagedFiles, mergeConflictFiles} = await git.getStatusesForChangedFiles() assert.deepEqual(stagedFiles, { 'a.txt': 'modified', 'e.txt': 'added' @@ -101,7 +101,7 @@ describe('Git commands', () => { } } - const {stagedFiles, unstagedFiles, mergeConflictFiles} = await git.getStatus() + const {stagedFiles, unstagedFiles, mergeConflictFiles} = await git.getStatusesForChangedFiles() assert.deepEqual(stagedFiles, {}) assert.deepEqual(unstagedFiles, {}) From 43f80cd32ead71f16c9514680686e21954b98902 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 9 Nov 2016 23:18:40 -0800 Subject: [PATCH 05/49] :fire: old unused files --- .../changed-files-count-controller.js | 69 ------------------- .../changed-files-count-controller.test.js | 36 ---------- 2 files changed, 105 deletions(-) delete mode 100644 lib/controllers/changed-files-count-controller.js delete mode 100644 test/controllers/changed-files-count-controller.test.js diff --git a/lib/controllers/changed-files-count-controller.js b/lib/controllers/changed-files-count-controller.js deleted file mode 100644 index e3adfc45b6..0000000000 --- a/lib/controllers/changed-files-count-controller.js +++ /dev/null @@ -1,69 +0,0 @@ -/** @babel */ -/** @jsx etch.dom */ - -import etch from 'etch' - -import ChangedFilesCountView from '../views/changed-files-count-view' - -export default class ChangedFilesCountController { - constructor (props) { - this.props = props - this.switchRepository(props.repository) - etch.initialize(this) - } - - update (props) { - this.props = Object.assign({}, this.props, props) - return this.switchRepository(props.repository) - } - - render () { - if (this.repository) { - return ( - - ) - } else { - return
- } - } - - destroy () { - if (this.repositorySubscription) this.repositorySubscription.dispose() - return etch.destroy(this) - } - - switchRepository (repository) { - if (repository !== this.repository) { - if (this.repositorySubscription) { - this.repositorySubscription.dispose() - this.repositorySubscription = null - } - if (repository) { - this.repositorySubscription = repository.onDidUpdate(() => this.refreshModelData(repository)) - } - - return this.refreshModelData(repository) - } - } - - refreshModelData (repository) { - this.lastModelDataRefreshPromise = this._refreshModelData(repository) - return this.lastModelDataRefreshPromise - } - - async _refreshModelData (repository) { - if (repository) { - const stagedChanges = await repository.getStagedChanges() - const unstagedChanges = await repository.getUnstagedChanges() - this.unstagedChanges = unstagedChanges - this.stagedChanges = stagedChanges - } - - this.repository = repository - return etch.update(this) - } -} diff --git a/test/controllers/changed-files-count-controller.test.js b/test/controllers/changed-files-count-controller.test.js deleted file mode 100644 index 8a6f29005d..0000000000 --- a/test/controllers/changed-files-count-controller.test.js +++ /dev/null @@ -1,36 +0,0 @@ -/** @babel */ - -import {cloneRepository, buildRepository} from '../helpers' -import path from 'path' -import fs from 'fs' -import sinon from 'sinon' - -import ChangedFilesCountController from '../../lib/controllers/changed-files-count-controller' - -describe('ChangedFilesCountController', () => { - it('shows the changed files count view when the repository data is loaded', async () => { - const didClick = sinon.spy() - const view = new ChangedFilesCountController({repository: null, didClick}) - assert.isUndefined(view.refs.changedFilesCount) - - const workdirPath = await cloneRepository('three-files') - const repository = await buildRepository(workdirPath) - view.update({repository}) - await view.lastModelDataRefreshPromise - assert.deepEqual(view.refs.changedFilesCount.props.stagedChanges, []) - assert.deepEqual(view.refs.changedFilesCount.props.unstagedChanges, []) - - fs.writeFileSync(path.join(workdirPath, 'a.txt'), 'a change\n') - fs.unlinkSync(path.join(workdirPath, 'b.txt')) - repository.refresh() - - const [patchToStage] = await repository.getUnstagedChanges() - await repository.applyPatchToIndex(patchToStage) - await view.lastModelDataRefreshPromise - assert.deepEqual(view.refs.changedFilesCount.props.stagedChanges, await repository.getStagedChanges()) - assert.deepEqual(view.refs.changedFilesCount.props.unstagedChanges, await repository.getUnstagedChanges()) - - view.refs.changedFilesCount.props.didClick() - assert(didClick.calledOnce) - }) -}) From 0b5e0c359dd9154227e7d39b6cc2cca080902d6c Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 15 Nov 2016 13:29:09 -0800 Subject: [PATCH 06/49] Modify GSOS.getStatusesForChangedFiles() to handle renamed files --- lib/git-shell-out-strategy.js | 42 ++++++++++++++++++----------- test/git-shell-out-strategy.test.js | 14 ++++++++++ 2 files changed, 40 insertions(+), 16 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index 7432dabc14..0e7b3ce9a3 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -130,7 +130,7 @@ export default class GitShellOutStrategy { * File Status and Diffs */ async getStatusesForChangedFiles () { - const output = await this.exec(['status', '--untracked-files=all', '--porcelain']) + const output = await this.exec(['status', '--untracked-files=all', '-z']) const statusMap = { A: 'added', @@ -145,23 +145,33 @@ export default class GitShellOutStrategy { const mergeConflictFiles = {} const statusToHead = await this.diffFileStatus({target: 'HEAD'}) - output && output.split(LINE_ENDING_REGEX).forEach(async (line) => { - if (line === '') return - const [_, x, y, filePath] = line.match(/(.)(.)\s(.*)/) - if (x === 'U' || y === 'U' || (x === 'A' && y === 'A')) { - return mergeConflictFiles[filePath] = { - ours: statusMap[x], - theirs: statusMap[y], - file: statusToHead[filePath] || 'equivalent' + if (output) { + const lines = output.split('\0') + for (let i = 0; i < lines.length; i++) { + const line = lines[i] + if (line === '') continue + const [_, x, y, filePath] = line.match(/(.)(.)\s(.*)/) + if (x === 'U' || y === 'U' || (x === 'A' && y === 'A')) { + mergeConflictFiles[filePath] = { + ours: statusMap[x], + theirs: statusMap[y], + file: statusToHead[filePath] || 'equivalent' + } + continue + } + if (x === 'R') { + stagedFiles[filePath] = 'added' + stagedFiles[lines[++i]] = 'deleted' + continue + } + if (y !== ' ') { + unstagedFiles[filePath] = statusMap[y] + } + if (x !== ' ' && x !== '?') { + stagedFiles[filePath] = statusMap[x] } } - if (y !== ' ') { - unstagedFiles[filePath] = statusMap[y] - } - if (x !== ' ' && x !== '?') { - stagedFiles[filePath] = statusMap[x] - } - }) + } return {stagedFiles, unstagedFiles, mergeConflictFiles} } diff --git a/test/git-shell-out-strategy.test.js b/test/git-shell-out-strategy.test.js index 73c13e2edc..6fe957fd8e 100644 --- a/test/git-shell-out-strategy.test.js +++ b/test/git-shell-out-strategy.test.js @@ -89,6 +89,20 @@ describe('Git commands', () => { assert.deepEqual(mergeConflictFiles, {}) }) + it('displays renamed files as one removed file and one added file', async () => { + const workingDirPath = await cloneRepository('three-files') + const git = new GitShellOutStrategy(workingDirPath) + fs.renameSync(path.join(workingDirPath, 'c.txt'), path.join(workingDirPath, 'd.txt')) + await git.exec(['add', '.']) + const {stagedFiles, unstagedFiles, mergeConflictFiles} = await git.getStatusesForChangedFiles() + assert.deepEqual(stagedFiles, { + 'c.txt': 'deleted', + 'd.txt': 'added' + }) + assert.deepEqual(unstagedFiles, {}) + assert.deepEqual(mergeConflictFiles, {}) + }) + it('returns an object for merge conflict files, including ours/theirs/file status information', async () => { const workingDirPath = await cloneRepository('merge-conflict') const git = new GitShellOutStrategy(workingDirPath) From 2ef4f3e0935df516ef5494744c7e2fd92eb63030 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 15 Nov 2016 14:54:01 -0800 Subject: [PATCH 07/49] Implement and use Repository#getStatusesForChangedFiles --- lib/controllers/status-bar-tile-controller.js | 4 +- lib/github-package.js | 2 + lib/models/repository.js | 76 ++---- lib/views/file-patch-list-item-view.js | 4 +- lib/views/merge-conflict-list-item-view.js | 8 +- lib/views/staging-view.js | 8 +- test/helpers.js | 5 + test/models/repository.test.js | 227 ++++-------------- 8 files changed, 86 insertions(+), 248 deletions(-) diff --git a/lib/controllers/status-bar-tile-controller.js b/lib/controllers/status-bar-tile-controller.js index f0b999957d..bc30eb4e20 100644 --- a/lib/controllers/status-bar-tile-controller.js +++ b/lib/controllers/status-bar-tile-controller.js @@ -143,10 +143,10 @@ export default class StatusBarTileController { async fetchChangedFilesCount (repository) { const changedFiles = new Set() for (let filePatch of await repository.getUnstagedChanges()) { - changedFiles.add(filePatch.getPath()) + changedFiles.add(filePatch.filePath) } for (let filePatch of await repository.getStagedChanges()) { - changedFiles.add(filePatch.getPath()) + changedFiles.add(filePatch.filePath) } return changedFiles.size } diff --git a/lib/github-package.js b/lib/github-package.js index 5a4e240876..40c697f620 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -83,6 +83,7 @@ export default class GithubPackage { } didSelectFilePatch (filePatch, stagingStatus, {focus} = {}) { + return // FIXME if (!filePatch || filePatch.isDestroyed()) return const repository = this.getActiveRepository() let containingPane @@ -99,6 +100,7 @@ export default class GithubPackage { } async didSelectMergeConflictFile (relativeFilePath, {focus} = {}) { + return // FIXME const absolutePath = path.join(this.getActiveRepository().getWorkingDirectoryPath(), relativeFilePath) if (await new File(absolutePath).exists()) { return this.workspace.open(absolutePath, {activatePane: Boolean(focus)}) diff --git a/lib/models/repository.js b/lib/models/repository.js index c703e8de9b..cd84d1a08a 100644 --- a/lib/models/repository.js +++ b/lib/models/repository.js @@ -88,32 +88,35 @@ export default class Repository { async refresh () { if (process.env.PRINT_GIT_TIMES) console.time('refresh') - this.stagedChangesPromise = this.fetchStagedChanges() + this.fileStatusesPromise = this.fetchStatusesForChangedFiles() this.stagedChangesSinceParentCommitPromise = this.fetchStagedChangesSinceParentCommit() - this.unstagedChangesPromise = this.fetchUnstagedChanges() - this.mergeConflictsPromise = this.fetchMergeConflicts() await Promise.all([ - this.stagedChangesPromise, + this.fileStatusesPromise, this.stagedChangesSinceParentCommitPromise, - this.unstagedChangesPromise, - this.mergeConflictsPromise ]) if (process.env.PRINT_GIT_TIMES) console.timeEnd('refresh') this.didUpdate() } - getUnstagedChanges () { - if (!this.unstagedChangesPromise) { - this.unstagedChangesPromise = this.fetchUnstagedChanges() - } - return this.unstagedChangesPromise + fetchStatusesForChangedFiles () { + return this.git.getStatusesForChangedFiles() } - getStagedChanges () { - if (!this.stagedChangesPromise) { - this.stagedChangesPromise = this.fetchStagedChanges() + getStatusesForChangedFiles () { + if (!this.fileStatusesPromise) { + this.fileStatusesPromise = this.fetchStatusesForChangedFiles() } - return this.stagedChangesPromise + return this.fileStatusesPromise + } + + async getUnstagedChanges () { + const {unstagedFiles} = await this.getStatusesForChangedFiles() + return Object.keys(unstagedFiles).map(filePath => { return {filePath, status: unstagedFiles[filePath]} }) + } + + async getStagedChanges () { + const {stagedFiles} = await this.getStatusesForChangedFiles() + return Object.keys(stagedFiles).map(filePath => { return {filePath, status: stagedFiles[filePath]} }) } getStagedChangesSinceParentCommit () { @@ -123,21 +126,9 @@ export default class Repository { return this.stagedChangesSinceParentCommitPromise } - getMergeConflicts () { - if (!this.mergeConflictsPromise) { - this.mergeConflictsPromise = this.fetchMergeConflicts() - } - return this.mergeConflictsPromise - } - - async fetchUnstagedChanges () { - const rawDiffs = await this.git.diff() - return this.updateFilePatches(this.unstagedFilePatchesByPath, rawDiffs) - } - - async fetchStagedChanges () { - const rawDiffs = await this.git.diff({staged: true}) - return this.updateFilePatches(this.stagedFilePatchesByPath, rawDiffs) + async getMergeConflicts () { + const {mergeConflictFiles} = await this.getStatusesForChangedFiles() + return Object.keys(mergeConflictFiles).map(filePath => { return {filePath, status: mergeConflictFiles[filePath]} }) } async fetchStagedChangesSinceParentCommit () { @@ -213,31 +204,6 @@ export default class Repository { }) } - async fetchMergeConflicts () { - const statusesByPath = (await this.git.getStatusesForChangedFiles()).mergeConflictFiles - const validConflicts = new Set() - for (let path in statusesByPath) { - const statuses = statusesByPath[path] - const existingConflict = this.mergeConflictsByPath.get(path) - if (existingConflict == null) { - this.mergeConflictsByPath.set(path, new MergeConflict(path, statuses.ours, statuses.theirs, statuses.file)) - } else { - existingConflict.updateFileStatus(statuses.file) - } - validConflicts.add(path) - } - - for (let [path, conflict] of this.mergeConflictsByPath) { - if (!validConflicts.has(path)) { - this.mergeConflictsByPath.delete(path) - conflict.destroy() - } - } - - return Array.from(this.mergeConflictsByPath.values()) - .sort((a, b) => a.getPath().localeCompare(b.getPath())) - } - async stageFiles (paths) { await this.git.stageFiles(paths) await this.refresh() diff --git a/lib/views/file-patch-list-item-view.js b/lib/views/file-patch-list-item-view.js index 5b14f21e80..c88738e0c8 100644 --- a/lib/views/file-patch-list-item-view.js +++ b/lib/views/file-patch-list-item-view.js @@ -19,13 +19,13 @@ export default class FilePatchListItemView { render () { const {filePatch, selected, ...others} = this.props - const status = classNameForStatus[filePatch.getStatus()] + const status = classNameForStatus[filePatch.status] const className = selected ? 'is-selected' : '' return (
- {filePatch.getPath()} + {filePatch.filePath}
) } diff --git a/lib/views/merge-conflict-list-item-view.js b/lib/views/merge-conflict-list-item-view.js index d7bb5652e9..212bc6b618 100644 --- a/lib/views/merge-conflict-list-item-view.js +++ b/lib/views/merge-conflict-list-item-view.js @@ -25,16 +25,16 @@ export default class FilePatchListItemView { render () { const {mergeConflict, selected, ...others} = this.props - const status = classNameForStatus[mergeConflict.getFileStatus()] + const status = classNameForStatus[mergeConflict.status.file] const className = selected ? 'is-selected' : '' return (
- {mergeConflict.getPath()} + {mergeConflict.filePath} - {statusSymbolMap[mergeConflict.getOursStatus()]} - {statusSymbolMap[mergeConflict.getTheirsStatus()]} + {statusSymbolMap[mergeConflict.status.ours]} + {statusSymbolMap[mergeConflict.status.theirs]}
) diff --git a/lib/views/staging-view.js b/lib/views/staging-view.js index 3c5d9be52a..1be29f3b82 100644 --- a/lib/views/staging-view.js +++ b/lib/views/staging-view.js @@ -24,7 +24,7 @@ export default class StagingView { conflicts: this.props.mergeConflicts || [], staged: this.props.stagedChanges }, - idForItem: item => item.getPath() + idForItem: item => item.filePath }) this.reportSelectedItem() etch.initialize(this) @@ -70,7 +70,7 @@ export default class StagingView { } confirmSelectedItems () { - const itemPaths = Array.from(this.selection.getSelectedItems()).map(item => item.getPath()) + const itemPaths = Array.from(this.selection.getSelectedItems()).map(item => item.filePath) if (this.selection.getActiveListKey() === 'staged') { return this.props.unstageFiles(itemPaths) } else { @@ -131,9 +131,9 @@ export default class StagingView { async mousedownOnItem (event, item) { if (event.detail >= 2) { if (this.selection.listKeyForItem(item) === 'staged') { - await this.props.unstageFiles([item.getPath()]) + await this.props.unstageFiles([item.filePath]) } else { - await this.props.stageFiles([item.getPath()]) + await this.props.stageFiles([item.filePath]) } } else { if (event.ctrlKey || event.metaKey) { diff --git a/test/helpers.js b/test/helpers.js index bb79e4e0fe..65b5f74f10 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -90,3 +90,8 @@ export function assertEqualSets (a, b) { assert.equal(a.size, b.size, 'Sets are a different size') a.forEach(item => assert(b.has(item), 'Sets have different elements')) } + +export function assertEqualSortedArraysByKey (arr1, arr2, key) { + const sortFn = (a, b) => a[key] < b[key] + assert.deepEqual(arr1.sort(sortFn), arr2.sort(sortFn)) +} diff --git a/test/models/repository.test.js b/test/models/repository.test.js index f86dd68079..8d17fd12c6 100644 --- a/test/models/repository.test.js +++ b/test/models/repository.test.js @@ -5,136 +5,16 @@ import path from 'path' import dedent from 'dedent-js' import sinon from 'sinon' -import {cloneRepository, buildRepository, assertDeepPropertyVals, setUpLocalAndRemoteRepositories, getHeadCommitOnRemote} from '../helpers' +import {cloneRepository, buildRepository, assertDeepPropertyVals, setUpLocalAndRemoteRepositories, getHeadCommitOnRemote, assertEqualSortedArraysByKey} from '../helpers' describe('Repository', function () { - describe('getting staged and unstaged changes', () => { - it('returns a promise resolving to an array of FilePatch objects', async () => { - const workingDirPath = await cloneRepository('three-files') - fs.writeFileSync(path.join(workingDirPath, 'a.txt'), 'qux\nfoo\nbar\n', 'utf8') - fs.unlinkSync(path.join(workingDirPath, 'b.txt')) - fs.renameSync(path.join(workingDirPath, 'c.txt'), path.join(workingDirPath, 'd.txt')) - fs.writeFileSync(path.join(workingDirPath, 'e.txt'), 'qux', 'utf8') - - const repo = await buildRepository(workingDirPath) - const filePatches = await repo.getUnstagedChanges() - - assertDeepPropertyVals(filePatches, [ - { - oldPath: 'a.txt', - newPath: 'a.txt', - status: 'modified', - hunks: [ - { - lines: [ - {status: 'added', text: 'qux', oldLineNumber: -1, newLineNumber: 1}, - {status: 'unchanged', text: 'foo', oldLineNumber: 1, newLineNumber: 2}, - {status: 'added', text: 'bar', oldLineNumber: -1, newLineNumber: 3} - ] - } - ] - }, - { - oldPath: 'b.txt', - newPath: null, - status: 'deleted', - hunks: [ - { - lines: [ - {status: 'deleted', text: 'bar', oldLineNumber: 1, newLineNumber: -1} - ] - } - ] - }, - { - oldPath: 'c.txt', - newPath: null, - status: 'deleted', - hunks: [ - { - lines: [ - {status: 'deleted', text: 'baz', oldLineNumber: 1, newLineNumber: -1} - ] - } - ] - }, - { - oldPath: null, - newPath: 'd.txt', - status: 'added', - hunks: [ - { - lines: [ - {status: 'added', text: 'baz', oldLineNumber: -1, newLineNumber: 1} - ] - } - ] - }, - { - oldPath: null, - newPath: 'e.txt', - status: 'added', - hunks: [ - { - lines: [ - {status: 'added', text: 'qux', oldLineNumber: -1, newLineNumber: 1}, - {status: undefined, text: '\\ No newline at end of file', oldLineNumber: -1, newLineNumber: 1} - ] - } - ] - } - ]) - }) - - it('reuses the same FilePatch objects if they are equivalent', async () => { - const workingDirPath = await cloneRepository('three-files') - const repo = await buildRepository(workingDirPath) - fs.writeFileSync(path.join(workingDirPath, 'a.txt'), 'qux\nfoo\nbar', 'utf8') - fs.unlinkSync(path.join(workingDirPath, 'b.txt')) - fs.writeFileSync(path.join(workingDirPath, 'c.txt'), 'bar\nbaz') - fs.writeFileSync(path.join(workingDirPath, 'd.txt'), 'qux', 'utf8') - await repo.refresh() - const unstagedFilePatches1 = await repo.getUnstagedChanges() - - fs.writeFileSync(path.join(workingDirPath, 'a.txt'), 'baz\nfoo\nqux', 'utf8') - fs.writeFileSync(path.join(workingDirPath, 'c.txt'), 'baz') - fs.unlinkSync(path.join(workingDirPath, 'd.txt')) - await repo.refresh() - const unstagedFilePatches2 = await repo.getUnstagedChanges() - - assert.equal(unstagedFilePatches1.length, 4) - assert.equal(unstagedFilePatches2.length, 3) - assert.equal(unstagedFilePatches1[0], unstagedFilePatches2[0]) - assert.equal(unstagedFilePatches1[1], unstagedFilePatches2[1]) - assert.equal(unstagedFilePatches1[2], unstagedFilePatches2[2]) - assert(unstagedFilePatches1[3].isDestroyed()) - - await repo.stageFiles([unstagedFilePatches2[0].getPath()]) - await repo.stageFiles([unstagedFilePatches2[1].getPath()]) - await repo.stageFiles([unstagedFilePatches2[2].getPath()]) - const stagedFilePatches1 = await repo.getStagedChanges() - - await repo.unstageFiles([stagedFilePatches1[2].getPath()]) - const stagedFilePatches2 = await repo.getStagedChanges() - const unstagedFilePatches3 = await repo.getUnstagedChanges() - - assert.equal(stagedFilePatches1.length, 3) - assert.equal(stagedFilePatches2.length, 2) - assert.equal(unstagedFilePatches3.length, 1) - assert.equal(stagedFilePatches1[0], stagedFilePatches2[0]) - assert.equal(stagedFilePatches1[1], stagedFilePatches2[1]) - assert.notEqual(stagedFilePatches1[2], unstagedFilePatches3[0]) - assert(stagedFilePatches1[2].isDestroyed()) - }) - }) - describe('staging and unstaging files', () => { it('can stage and unstage modified files', async () => { const workingDirPath = await cloneRepository('three-files') const repo = await buildRepository(workingDirPath) fs.writeFileSync(path.join(workingDirPath, 'subdir-1', 'a.txt'), 'qux\nfoo\nbar\n', 'utf8') const [patch] = await repo.getUnstagedChanges() - const filePath = patch.getPath() + const filePath = patch.filePath await repo.stageFiles([filePath]) assert.deepEqual(await repo.getUnstagedChanges(), []) @@ -150,7 +30,7 @@ describe('Repository', function () { const repo = await buildRepository(workingDirPath) fs.unlinkSync(path.join(workingDirPath, 'subdir-1', 'b.txt')) const [patch] = await repo.getUnstagedChanges() - const filePath = patch.getPath() + const filePath = patch.filePath await repo.stageFiles([filePath]) assert.deepEqual(await repo.getUnstagedChanges(), []) @@ -166,17 +46,17 @@ describe('Repository', function () { const repo = await buildRepository(workingDirPath) fs.renameSync(path.join(workingDirPath, 'c.txt'), path.join(workingDirPath, 'subdir-1', 'd.txt')) const patches = await repo.getUnstagedChanges() - const filePath1 = patches[0].getPath() - const filePath2 = patches[1].getPath() + const filePath1 = patches[0].filePath + const filePath2 = patches[1].filePath await repo.stageFiles([filePath1]) await repo.stageFiles([filePath2]) - assert.deepEqual(await repo.getStagedChanges(), patches) + assertEqualSortedArraysByKey(await repo.getStagedChanges(), patches, 'filePath') assert.deepEqual(await repo.getUnstagedChanges(), []) await repo.unstageFiles([filePath1]) await repo.unstageFiles([filePath2]) - assert.deepEqual(await repo.getUnstagedChanges(), patches) + assertEqualSortedArraysByKey(await repo.getUnstagedChanges(), patches, 'filePath') assert.deepEqual(await repo.getStagedChanges(), []) }) @@ -185,7 +65,7 @@ describe('Repository', function () { fs.writeFileSync(path.join(workingDirPath, 'subdir-1', 'e.txt'), 'qux', 'utf8') const repo = await buildRepository(workingDirPath) const [patch] = await repo.getUnstagedChanges() - const filePath = patch.getPath() + const filePath = patch.filePath await repo.stageFiles([filePath]) assert.deepEqual(await repo.getUnstagedChanges(), []) @@ -508,34 +388,44 @@ describe('Repository', function () { let mergeConflicts = await repo.getMergeConflicts() const expected = [ { - path: 'added-to-both.txt', - fileStatus: 'modified', - oursStatus: 'added', - theirsStatus: 'added' + filePath: 'added-to-both.txt', + status: { + file: 'modified', + ours: 'added', + theirs: 'added' + } }, { - path: 'modified-on-both-ours.txt', - fileStatus: 'modified', - oursStatus: 'modified', - theirsStatus: 'modified' + filePath: 'modified-on-both-ours.txt', + status: { + file: 'modified', + ours: 'modified', + theirs: 'modified' + } }, { - path: 'modified-on-both-theirs.txt', - fileStatus: 'modified', - oursStatus: 'modified', - theirsStatus: 'modified' + filePath: 'modified-on-both-theirs.txt', + status: { + file: 'modified', + ours: 'modified', + theirs: 'modified' + } }, { - path: 'removed-on-branch.txt', - fileStatus: 'equivalent', - oursStatus: 'modified', - theirsStatus: 'deleted' + filePath: 'removed-on-branch.txt', + status: { + file: 'equivalent', + ours: 'modified', + theirs: 'deleted' + } }, { - path: 'removed-on-master.txt', - fileStatus: 'added', - oursStatus: 'deleted', - theirsStatus: 'modified' + filePath: 'removed-on-master.txt', + status: { + file: 'added', + ours: 'deleted', + theirs: 'modified' + } } ] @@ -544,35 +434,10 @@ describe('Repository', function () { fs.unlinkSync(path.join(workingDirPath, 'removed-on-branch.txt')) await repo.refresh() mergeConflicts = await repo.getMergeConflicts() - - expected[3].fileStatus = 'deleted' + expected[3].status.file = 'deleted' assertDeepPropertyVals(mergeConflicts, expected) }) - it('reuses the same MergeConflict objects if they are equivalent', async () => { - const workingDirPath = await cloneRepository('merge-conflict') - const repo = await buildRepository(workingDirPath) - try { - await repo.git.merge('origin/branch') - } catch (e) { - // expected - } - - await repo.refresh() - const mergeConflicts1 = await repo.getMergeConflicts() - - await repo.stageFiles(['removed-on-master.txt']) - const mergeConflicts2 = await repo.getMergeConflicts() - - assert.equal(mergeConflicts1.length, 5) - assert.equal(mergeConflicts2.length, 4) - assert.equal(mergeConflicts1[0], mergeConflicts2[0]) - assert.equal(mergeConflicts1[1], mergeConflicts2[1]) - assert.equal(mergeConflicts1[2], mergeConflicts2[2]) - assert.equal(mergeConflicts1[3], mergeConflicts2[3]) - assert(mergeConflicts1[4].isDestroyed()) - }) - it('returns an empty arry if the repo has no merge conflicts', async () => { const workingDirPath = await cloneRepository('three-files') const repo = await buildRepository(workingDirPath) @@ -593,41 +458,41 @@ describe('Repository', function () { } await repo.refresh() - const mergeConflictPaths = (await repo.getMergeConflicts()).map(c => c.getPath()) + const mergeConflictPaths = (await repo.getMergeConflicts()).map(c => c.filePath) assert.deepEqual(mergeConflictPaths, ['added-to-both.txt', 'modified-on-both-ours.txt', 'modified-on-both-theirs.txt', 'removed-on-branch.txt', 'removed-on-master.txt']) let stagedFilePatches = await repo.getStagedChanges() - assert.deepEqual(stagedFilePatches.map(patch => patch.getPath()), []) + assert.deepEqual(stagedFilePatches.map(patch => patch.filePath), []) await repo.stageFiles(['added-to-both.txt']) stagedFilePatches = await repo.getStagedChanges() - assert.deepEqual(stagedFilePatches.map(patch => patch.getPath()), ['added-to-both.txt']) + assert.deepEqual(stagedFilePatches.map(patch => patch.filePath), ['added-to-both.txt']) // choose version of the file on head fs.writeFileSync(path.join(workingDirPath, 'modified-on-both-ours.txt'), 'master modification\n', 'utf8') await repo.stageFiles(['modified-on-both-ours.txt']) stagedFilePatches = await repo.getStagedChanges() // nothing additional to stage - assert.deepEqual(stagedFilePatches.map(patch => patch.getPath()), ['added-to-both.txt']) + assert.deepEqual(stagedFilePatches.map(patch => patch.filePath), ['added-to-both.txt']) // choose version of the file on branch fs.writeFileSync(path.join(workingDirPath, 'modified-on-both-ours.txt'), 'branch modification\n', 'utf8') await repo.stageFiles(['modified-on-both-ours.txt']) stagedFilePatches = await repo.getStagedChanges() - assert.deepEqual(stagedFilePatches.map(patch => patch.getPath()), ['added-to-both.txt', 'modified-on-both-ours.txt']) + assert.deepEqual(stagedFilePatches.map(patch => patch.filePath), ['added-to-both.txt', 'modified-on-both-ours.txt']) // remove file that was deleted on branch fs.unlinkSync(path.join(workingDirPath, 'removed-on-branch.txt')) await repo.stageFiles(['removed-on-branch.txt']) stagedFilePatches = await repo.getStagedChanges() - assert.deepEqual(stagedFilePatches.map(patch => patch.getPath()), ['added-to-both.txt', 'modified-on-both-ours.txt', 'removed-on-branch.txt']) + assert.deepEqual(stagedFilePatches.map(patch => patch.filePath), ['added-to-both.txt', 'modified-on-both-ours.txt', 'removed-on-branch.txt']) // remove file that was deleted on master fs.unlinkSync(path.join(workingDirPath, 'removed-on-master.txt')) await repo.stageFiles(['removed-on-master.txt']) stagedFilePatches = await repo.getStagedChanges() // nothing additional to stage - assert.deepEqual(stagedFilePatches.map(patch => patch.getPath()), ['added-to-both.txt', 'modified-on-both-ours.txt', 'removed-on-branch.txt']) + assert.deepEqual(stagedFilePatches.map(patch => patch.filePath), ['added-to-both.txt', 'modified-on-both-ours.txt', 'removed-on-branch.txt']) }) }) From 4779ac659da2a950a09d9fe6bc8ef4b443692e1c Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 15 Nov 2016 17:47:28 -0800 Subject: [PATCH 08/49] Fix more tests --- test/controllers/git-panel-controller.test.js | 25 +++++------ test/views/staging-view.test.js | 45 ++++++++++--------- 2 files changed, 37 insertions(+), 33 deletions(-) diff --git a/test/controllers/git-panel-controller.test.js b/test/controllers/git-panel-controller.test.js index 2389bb330c..6b2d569335 100644 --- a/test/controllers/git-panel-controller.test.js +++ b/test/controllers/git-panel-controller.test.js @@ -59,18 +59,19 @@ describe('GitPanelController', () => { // Does not update repository instance variable until that data is fetched await controller.update({repository: repository1}) assert.equal(controller.getActiveRepository(), repository1) - assert.equal(controller.refs.gitPanel.props.unstagedChanges, await repository1.getUnstagedChanges()) + console.log(controller.refs.gitPanel.props.unstagedChanges, await repository1.getUnstagedChanges()) + assert.deepEqual(controller.refs.gitPanel.props.unstagedChanges, await repository1.getUnstagedChanges()) await controller.update({repository: repository2}) assert.equal(controller.getActiveRepository(), repository2) - assert.equal(controller.refs.gitPanel.props.unstagedChanges, await repository2.getUnstagedChanges()) + assert.deepEqual(controller.refs.gitPanel.props.unstagedChanges, await repository2.getUnstagedChanges()) // Fetches data and updates child view when the repository is mutated fs.writeFileSync(path.join(workdirPath2, 'a.txt'), 'a change\n') fs.unlinkSync(path.join(workdirPath2, 'b.txt')) await repository2.refresh() await controller.getLastModelDataRefreshPromise() - assert.equal(controller.refs.gitPanel.props.unstagedChanges, await repository2.getUnstagedChanges()) + assert.deepEqual(controller.refs.gitPanel.props.unstagedChanges, await repository2.getUnstagedChanges()) }) it('displays the staged changes since the parent commmit when amending', async function () { @@ -142,8 +143,8 @@ describe('GitPanelController', () => { assert.equal(stagingView.props.mergeConflicts.length, 5) assert.equal(stagingView.props.stagedChanges.length, 0) - const conflict1 = stagingView.props.mergeConflicts.filter((c) => c.getPath() === 'modified-on-both-ours.txt')[0] - const contentsWithMarkers = fs.readFileSync(path.join(workdirPath, conflict1.getPath()), 'utf8') + const conflict1 = stagingView.props.mergeConflicts.filter((c) => c.filePath === 'modified-on-both-ours.txt')[0] + const contentsWithMarkers = fs.readFileSync(path.join(workdirPath, conflict1.filePath), 'utf8') assert(contentsWithMarkers.includes('>>>>>>>')) assert(contentsWithMarkers.includes('<<<<<<<')) @@ -170,9 +171,9 @@ describe('GitPanelController', () => { assert.equal(stagingView.props.stagedChanges.length, 1) // clear merge markers - const conflict2 = stagingView.props.mergeConflicts.filter((c) => c.getPath() === 'modified-on-both-theirs.txt')[0] + const conflict2 = stagingView.props.mergeConflicts.filter((c) => c.filePath === 'modified-on-both-theirs.txt')[0] atom.confirm.reset() - fs.writeFileSync(path.join(workdirPath, conflict2.getPath()), 'text with no merge markers') + fs.writeFileSync(path.join(workdirPath, conflict2.filePath), 'text with no merge markers') await stagingView.mousedownOnItem({detail: 2}, conflict2) await controller.getLastModelDataRefreshPromise() assert.equal(atom.confirm.called, false) @@ -190,9 +191,8 @@ describe('GitPanelController', () => { const stagingView = controller.refs.gitPanel.refs.stagingView const [addedFilePatch] = stagingView.props.unstagedChanges - assert.equal(addedFilePatch.getStatus(), 'added') - assert.isNull(addedFilePatch.getOldPath()) - assert.equal(addedFilePatch.getNewPath(), 'new-file.txt') + assert.equal(addedFilePatch.filePath, 'new-file.txt') + assert.equal(addedFilePatch.status, 'added') const patchString = dedent` --- /dev/null @@ -211,9 +211,8 @@ describe('GitPanelController', () => { // which now has new-file.txt on it, the working directory version of // new-file.txt has a modified status const [modifiedFilePatch] = stagingView.props.unstagedChanges - assert.equal(modifiedFilePatch.getStatus(), 'modified') - assert.equal(modifiedFilePatch.getOldPath(), 'new-file.txt') - assert.equal(modifiedFilePatch.getNewPath(), 'new-file.txt') + assert.equal(modifiedFilePatch.status, 'modified') + assert.equal(modifiedFilePatch.filePath, 'new-file.txt') }) }) }) diff --git a/test/views/staging-view.test.js b/test/views/staging-view.test.js index 5647d5f13d..c37d3ad4f4 100644 --- a/test/views/staging-view.test.js +++ b/test/views/staging-view.test.js @@ -9,8 +9,8 @@ describe('StagingView', () => { describe('staging and unstaging files', () => { it('renders staged and unstaged files', async () => { const filePatches = [ - new FilePatch('a.txt', 'a.txt', 'modified', []), - new FilePatch('b.txt', null, 'deleted', []) + { filePath: 'a.txt', status: 'modified' }, + { filePath: 'b.txt', status: 'deleted' } ] const view = new StagingView({unstagedChanges: filePatches, stagedChanges: []}) const {refs} = view @@ -18,6 +18,7 @@ describe('StagingView', () => { return Array.from(element.children).map(child => child.textContent) } + console.log(refs.unstagedChanges); assert.deepEqual(textContentOfChildren(refs.unstagedChanges), ['a.txt', 'b.txt']) assert.deepEqual(textContentOfChildren(refs.stagedChanges), []) @@ -29,8 +30,8 @@ describe('StagingView', () => { describe('confirmSelectedItems()', () => { it('calls stageFilePatch or unstageFilePatch depending on the current staging state of the toggled file patch', async () => { const filePatches = [ - new FilePatch('a.txt', 'a.txt', 'modified', []), - new FilePatch('b.txt', null, 'deleted', []) + { filePath: 'a.txt', status: 'modified' }, + { filePath: 'b.txt', status: 'deleted' } ] const stageFiles = sinon.spy() const unstageFiles = sinon.spy() @@ -55,10 +56,12 @@ describe('StagingView', () => { assert.isUndefined(view.refs.mergeConflicts) const mergeConflicts = [{ - getPath: () => 'conflicted-path', - getFileStatus: () => 'modified', - getOursStatus: () => 'deleted', - getTheirsStatus: () => 'modified' + filePath: 'conflicted-path', + status: { + file: 'modified', + ours: 'deleted', + theirs: 'modified' + } }] await view.update({unstagedChanges: [], mergeConflicts, stagedChanges: []}) assert.isDefined(view.refs.mergeConflicts) @@ -68,14 +71,16 @@ describe('StagingView', () => { describe('when the selection changes', function () { it('notifies the parent component via the appropriate callback', async function () { const filePatches = [ - new FilePatch('a.txt', 'a.txt', 'modified', []), - new FilePatch('b.txt', null, 'deleted', []) + { filePath: 'a.txt', status: 'modified' }, + { filePath: 'b.txt', status: 'deleted' } ] const mergeConflicts = [{ - getPath: () => 'c.txt', - getFileStatus: () => 'modified', - getOursStatus: () => 'deleted', - getTheirsStatus: () => 'modified' + filePath: 'conflicted-path', + status: { + file: 'modified', + ours: 'deleted', + theirs: 'modified' + } }] const didSelectFilePatch = sinon.spy() @@ -107,12 +112,12 @@ describe('StagingView', () => { it('autoscroll to the selected item if it is out of view', async function () { const unstagedChanges = [ - new FilePatch('a.txt', 'a.txt', 'modified', []), - new FilePatch('b.txt', 'b.txt', 'modified', []), - new FilePatch('c.txt', 'c.txt', 'modified', []), - new FilePatch('d.txt', 'd.txt', 'modified', []), - new FilePatch('e.txt', 'e.txt', 'modified', []), - new FilePatch('f.txt', 'f.txt', 'modified', []), + { filePath: 'a.txt', status: 'modified' }, + { filePath: 'b.txt', status: 'modified' }, + { filePath: 'c.txt', status: 'modified' }, + { filePath: 'd.txt', status: 'modified' }, + { filePath: 'e.txt', status: 'modified' }, + { filePath: 'f.txt', status: 'modified' } ] const view = new StagingView({unstagedChanges, stagedChanges: []}) From aa6c99f490e6511bb92476747a4ac5d819f6907b Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 15 Nov 2016 19:15:21 -0800 Subject: [PATCH 09/49] Set initial CompositeListSelection active index to be first non-empty selection --- lib/views/composite-list-selection.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/views/composite-list-selection.js b/lib/views/composite-list-selection.js index 9fcc97a448..6117e064ed 100644 --- a/lib/views/composite-list-selection.js +++ b/lib/views/composite-list-selection.js @@ -15,6 +15,12 @@ export default class CompositeListSelection { } this.activeSelectionIndex = 0 + for (var i = 0; i < this.selections.length; i++) { + if (this.selections[i].getItems().length) { + this.activeSelectionIndex = i + break + } + } } updateLists (listsByKey) { From d24d8636be819f8c658dfc92b1c0d025b2cb5c19 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 15 Nov 2016 19:57:03 -0800 Subject: [PATCH 10/49] In StagingView do not update component on double-click of file path Re-rendering was causing this.props.didSelectFilePatch to get called with a file that was just staged/unstaged. Therefore Repository.prototype.getFilePatchForPath threw an error when the file diff wasn't found. --- lib/views/staging-view.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/views/staging-view.js b/lib/views/staging-view.js index 1be29f3b82..66f698f33f 100644 --- a/lib/views/staging-view.js +++ b/lib/views/staging-view.js @@ -142,8 +142,8 @@ export default class StagingView { this.selection.selectItem(item, event.shiftKey) } this.mouseSelectionInProgress = true + await etch.update(this) } - await etch.update(this) } mousemoveOnItem (event, item) { From 067e63ff35b82799087131cfa270918ea69b27c0 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 15 Nov 2016 20:18:51 -0800 Subject: [PATCH 11/49] :art: fetchChangedFilesCount and include merge conflict files --- lib/controllers/status-bar-tile-controller.js | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/controllers/status-bar-tile-controller.js b/lib/controllers/status-bar-tile-controller.js index bc30eb4e20..452a8affc7 100644 --- a/lib/controllers/status-bar-tile-controller.js +++ b/lib/controllers/status-bar-tile-controller.js @@ -142,11 +142,16 @@ export default class StatusBarTileController { async fetchChangedFilesCount (repository) { const changedFiles = new Set() - for (let filePatch of await repository.getUnstagedChanges()) { - changedFiles.add(filePatch.filePath) + const {stagedFiles, unstagedFiles, mergeConflictFiles} = await repository.getStatusesForChangedFiles() + + for (let filePath in unstagedFiles) { + changedFiles.add(filePath) + } + for (let filePath in stagedFiles) { + changedFiles.add(filePath) } - for (let filePatch of await repository.getStagedChanges()) { - changedFiles.add(filePatch.filePath) + for (let filePath in mergeConflictFiles) { + changedFiles.add(filePath) } return changedFiles.size } From a96a4aa4bde9421d1eb59309ab5d761ad91b30d2 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 15 Nov 2016 20:25:39 -0800 Subject: [PATCH 12/49] Only report selected item if there is an item to select --- lib/views/staging-view.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/views/staging-view.js b/lib/views/staging-view.js index 66f698f33f..a2de969386 100644 --- a/lib/views/staging-view.js +++ b/lib/views/staging-view.js @@ -115,15 +115,16 @@ export default class StagingView { } reportSelectedItem () { - if (!this.isFocused()) return + const selectedItem = this.selection.getHeadItem() + if (!this.isFocused() || !selectedItem) return if (this.selection.getActiveListKey() === 'conflicts') { if (this.props.didSelectMergeConflictFile) { - this.props.didSelectMergeConflictFile(this.selection.getHeadItem()) + this.props.didSelectMergeConflictFile(selectedItem.filePath) } } else { if (this.props.didSelectFilePatch) { - this.props.didSelectFilePatch(this.selection.getHeadItem(), this.selection.getActiveListKey()) + this.props.didSelectFilePatch(selectedItem.filePath, this.selection.getActiveListKey()) } } } From 335e97638b7569f682fbeefe198df9a6c4a77d29 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 15 Nov 2016 20:38:03 -0800 Subject: [PATCH 13/49] Display changed files icon and count even when no files are changed --- lib/views/changed-files-count-view.js | 24 ++++++++----------- .../status-bar-tile-controller.test.js | 2 +- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/lib/views/changed-files-count-view.js b/lib/views/changed-files-count-view.js index 034b8468aa..cbc8e9d35d 100644 --- a/lib/views/changed-files-count-view.js +++ b/lib/views/changed-files-count-view.js @@ -15,19 +15,15 @@ export default class ChangedFilesCountView { } render () { - if (this.props.changedFilesCount === 0) { - return - } else { - const label = - (this.props.changedFilesCount === 1) - ? '1 file' - : `${this.props.changedFilesCount} files` - return ( - {label} - ) - } + const label = + (this.props.changedFilesCount === 1) + ? '1 file' + : `${this.props.changedFilesCount} files` + return ( + {label} + ) } } diff --git a/test/controllers/status-bar-tile-controller.test.js b/test/controllers/status-bar-tile-controller.test.js index f9d784670b..c0e4eab629 100644 --- a/test/controllers/status-bar-tile-controller.test.js +++ b/test/controllers/status-bar-tile-controller.test.js @@ -299,7 +299,7 @@ describe('StatusBarTileController', () => { const changedFilesCountView = controller.refs.changedFilesCountView - assert.equal(changedFilesCountView.element.textContent, '') + assert.equal(changedFilesCountView.element.textContent, '0 files') fs.writeFileSync(path.join(workdirPath, 'a.txt'), 'a change\n') fs.unlinkSync(path.join(workdirPath, 'b.txt')) From 3b782d65d528a618ff3c6788cb4c6fed8705a0fd Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 15 Nov 2016 20:56:02 -0800 Subject: [PATCH 14/49] :fire: console.log --- test/controllers/git-panel-controller.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/controllers/git-panel-controller.test.js b/test/controllers/git-panel-controller.test.js index 6b2d569335..885ab92003 100644 --- a/test/controllers/git-panel-controller.test.js +++ b/test/controllers/git-panel-controller.test.js @@ -59,7 +59,6 @@ describe('GitPanelController', () => { // Does not update repository instance variable until that data is fetched await controller.update({repository: repository1}) assert.equal(controller.getActiveRepository(), repository1) - console.log(controller.refs.gitPanel.props.unstagedChanges, await repository1.getUnstagedChanges()) assert.deepEqual(controller.refs.gitPanel.props.unstagedChanges, await repository1.getUnstagedChanges()) await controller.update({repository: repository2}) From 7d8b209a6a0c366c2def42df87fff27d92ce8301 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 15 Nov 2016 22:41:08 -0800 Subject: [PATCH 15/49] :fire: unnecessary test file This functionality is tested in StatusBarTileController tests and GithubPackage tests --- test/views/changed-files-count-view.test.js | 26 --------------------- 1 file changed, 26 deletions(-) delete mode 100644 test/views/changed-files-count-view.test.js diff --git a/test/views/changed-files-count-view.test.js b/test/views/changed-files-count-view.test.js deleted file mode 100644 index afbe19a5ef..0000000000 --- a/test/views/changed-files-count-view.test.js +++ /dev/null @@ -1,26 +0,0 @@ -/** @babel */ - -import sinon from 'sinon' - -import FilePatch from '../../lib/models/file-patch' -import ChangedFilesCountView from '../../lib/views/changed-files-count-view' - -describe('ChangedFilesCountView', () => { - it('shows the count of the unique files that have changed', async () => { - const view = new ChangedFilesCountView({changedFilesCount: 0}) - assert.isUndefined(view.refs.changedFiles) - - await view.update({changedFilesCount: 2}) - assert.equal(view.refs.changedFiles.textContent, '2 files') - - await view.update({changedFilesCount: 3}) - assert.equal(view.refs.changedFiles.textContent, '3 files') - }) - - it('invokes the supplied handler when the label is clicked', async () => { - const didClick = sinon.spy() - const view = new ChangedFilesCountView({changedFilesCount: 1, didClick}) - view.refs.changedFiles.dispatchEvent(new MouseEvent('click')) - assert(didClick.called) - }) -}) From 5bfd2518b3103e95aed2a17b694825d3d605efb3 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 15 Nov 2016 21:45:31 -0800 Subject: [PATCH 16/49] Display FilePatch when file name is selected and fix tests --- lib/git-shell-out-strategy.js | 3 +- lib/github-package.js | 6 ++-- lib/models/repository.js | 9 +++++ .../controllers/file-patch-controller.test.js | 8 ++--- .../status-bar-tile-controller.test.js | 3 +- test/github-package.test.js | 33 ++++++++++--------- test/models/file-patch.test.js | 4 +-- test/views/staging-view.test.js | 7 ++-- 8 files changed, 41 insertions(+), 32 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index 0e7b3ce9a3..5e03181b7a 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -207,9 +207,10 @@ export default class GitShellOutStrategy { } async diff (options = {}) { - const args = ['diff', '--no-prefix', '--no-renames', '--diff-filter=u'] + let args = ['diff', '--no-prefix', '--no-renames', '--diff-filter=u'] if (options.staged) args.push('--staged') if (options.baseCommit) args.push(options.baseCommit) + if (options.filePath) args = args.concat(['--', options.filePath]) let output = await this.exec(args) let rawDiffs = [] diff --git a/lib/github-package.js b/lib/github-package.js index 40c697f620..d0b12a6367 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -82,10 +82,9 @@ export default class GithubPackage { } } - didSelectFilePatch (filePatch, stagingStatus, {focus} = {}) { - return // FIXME - if (!filePatch || filePatch.isDestroyed()) return + async didSelectFilePatch (filePath, stagingStatus, {focus} = {}) { const repository = this.getActiveRepository() + const filePatch = await repository.getFilePatchForPath(filePath, stagingStatus === 'staged') let containingPane if (this.filePatchController) { this.filePatchController.update({filePatch, repository, stagingStatus}) @@ -100,7 +99,6 @@ export default class GithubPackage { } async didSelectMergeConflictFile (relativeFilePath, {focus} = {}) { - return // FIXME const absolutePath = path.join(this.getActiveRepository().getWorkingDirectoryPath(), relativeFilePath) if (await new File(absolutePath).exists()) { return this.workspace.open(absolutePath, {activatePane: Boolean(focus)}) diff --git a/lib/models/repository.js b/lib/models/repository.js index cd84d1a08a..ecd212b2c7 100644 --- a/lib/models/repository.js +++ b/lib/models/repository.js @@ -144,6 +144,15 @@ export default class Repository { } } + async getFilePatchForPath (filePath, staged) { + const rawDiffs = await this.git.diff({filePath, staged}) + if (rawDiffs.length !== 1) { + throw new Error(`Expected a single diff for ${filePath} but got ${rawDiffs.length}`) + } + const [filePatch] = this.buildFilePatchesFromRawDiffs(rawDiffs) + return filePatch + } + updateFilePatches (patchesByPath, rawDiffs) { const filePatches = this.buildFilePatchesFromRawDiffs(rawDiffs) diff --git a/test/controllers/file-patch-controller.test.js b/test/controllers/file-patch-controller.test.js index 2314d3427a..26a36a76fd 100644 --- a/test/controllers/file-patch-controller.test.js +++ b/test/controllers/file-patch-controller.test.js @@ -75,7 +75,7 @@ describe('FilePatchController', () => { ) unstagedLines.splice(11, 2, 'this is a modified line') fs.writeFileSync(filePath, unstagedLines.join('\n')) - const [unstagedFilePatch] = await repository.getUnstagedChanges() + const unstagedFilePatch = await repository.getFilePatchForPath('sample.js', false) const hunkViewsByHunk = new Map() function registerHunkView (hunk, view) { hunkViewsByHunk.set(hunk, view) } @@ -94,7 +94,7 @@ describe('FilePatchController', () => { ) assert.autocrlfEqual(await repository.readFileFromIndex('sample.js'), expectedStagedLines.join('\n')) - const [stagedFilePatch] = await repository.getStagedChanges() + const stagedFilePatch = await repository.getFilePatchForPath('sample.js', true) await controller.update({filePatch: stagedFilePatch, repository, stagingStatus: 'staged', registerHunkView}) await hunkViewsByHunk.get(stagedFilePatch.getHunks()[0]).props.didClickStageButton() assert.autocrlfEqual(await repository.readFileFromIndex('sample.js'), originalLines.join('\n')) @@ -115,7 +115,7 @@ describe('FilePatchController', () => { ) unstagedLines.splice(11, 2, 'this is a modified line') fs.writeFileSync(filePath, unstagedLines.join('\n')) - const [unstagedFilePatch] = await repository.getUnstagedChanges() + const unstagedFilePatch = await repository.getFilePatchForPath('sample.js', false) const hunkViewsByHunk = new Map() function registerHunkView (hunk, view) { hunkViewsByHunk.set(hunk, view) } @@ -147,7 +147,7 @@ describe('FilePatchController', () => { assert.autocrlfEqual(await repository.readFileFromIndex('sample.js'), expectedLines.join('\n')) // unstage a subset of lines from the first hunk - const [stagedFilePatch] = await repository.getStagedChanges() + const stagedFilePatch = await repository.getFilePatchForPath('sample.js', true) await controller.update({filePatch: stagedFilePatch, repository, stagingStatus: 'staged', registerHunkView}) hunk = stagedFilePatch.getHunks()[0] lines = hunk.getLines() diff --git a/test/controllers/status-bar-tile-controller.test.js b/test/controllers/status-bar-tile-controller.test.js index c0e4eab629..d862eb940e 100644 --- a/test/controllers/status-bar-tile-controller.test.js +++ b/test/controllers/status-bar-tile-controller.test.js @@ -304,8 +304,7 @@ describe('StatusBarTileController', () => { fs.writeFileSync(path.join(workdirPath, 'a.txt'), 'a change\n') fs.unlinkSync(path.join(workdirPath, 'b.txt')) repository.refresh() - const [patchToStage] = await repository.getUnstagedChanges() - await repository.applyPatchToIndex(patchToStage) + await repository.stageFiles(['a.txt']) await controller.getLastModelDataRefreshPromise() assert.equal(changedFilesCountView.element.textContent, '2 files') diff --git a/test/github-package.test.js b/test/github-package.test.js index f7294d9cb2..82abe5ab03 100644 --- a/test/github-package.test.js +++ b/test/github-package.test.js @@ -3,9 +3,9 @@ import fs from 'fs' import path from 'path' import temp from 'temp' +import etch from 'etch' import {cloneRepository, buildRepository} from './helpers' import FilePatch from '../lib/models/file-patch' -import Hunk from '../lib/models/hunk' import GithubPackage from '../lib/github-package' describe('GithubPackage', () => { @@ -171,6 +171,8 @@ describe('GithubPackage', () => { describe('when the active item is a FilePatchController', () => { it('updates the active repository to be the one associated with the FilePatchController', async () => { const workdirPath = await cloneRepository('three-files') + fs.writeFileSync(path.join(workdirPath, 'a.txt'), 'change', 'utf8') + project.setPaths([workdirPath]) const repository = await githubPackage.repositoryForWorkdirPath(workdirPath) @@ -178,8 +180,7 @@ describe('GithubPackage', () => { await githubPackage.updateActiveRepository() assert.equal(githubPackage.getActiveRepository(), repository) - const filePatch = new FilePatch('a.txt', 'a.txt', 'modified', [new Hunk(1, 1, 1, 3, [])]) - githubPackage.gitPanelController.props.didSelectFilePatch(filePatch, 'unstaged') + await githubPackage.gitPanelController.props.didSelectFilePatch('a.txt', 'unstaged') assert.equal(workspace.getActivePaneItem(), githubPackage.filePatchController) assert.equal(githubPackage.filePatchController.props.repository, repository) await githubPackage.updateActiveRepository() @@ -217,16 +218,17 @@ describe('GithubPackage', () => { const workdirPath = await cloneRepository('three-files') const repository = await buildRepository(workdirPath) + fs.writeFileSync(path.join(workdirPath, 'a.txt'), 'change', 'utf8') + fs.writeFileSync(path.join(workdirPath, 'd.txt'), 'new-file', 'utf8') + await repository.stageFiles(['d.txt']) + githubPackage.getActiveRepository = function () { return repository } - const hunk = new Hunk(1, 1, 1, 3, []) - const filePatch1 = new FilePatch('a.txt', 'a.txt', 'modified', [hunk]) - const filePatch2 = new FilePatch('b.txt', 'b.txt', 'modified', [hunk]) assert.isNull(githubPackage.filePatchController) - githubPackage.gitPanelController.props.didSelectFilePatch(filePatch1, 'unstaged') + await githubPackage.gitPanelController.props.didSelectFilePatch('a.txt', 'unstaged') assert(githubPackage.filePatchController) - assert.equal(githubPackage.filePatchController.props.filePatch, filePatch1) + assert.equal(githubPackage.filePatchController.props.filePatch.getPath(), 'a.txt') assert.equal(githubPackage.filePatchController.props.repository, repository) assert.equal(githubPackage.filePatchController.props.stagingStatus, 'unstaged') assert.equal(workspace.getActivePaneItem(), githubPackage.filePatchController) @@ -236,9 +238,9 @@ describe('GithubPackage', () => { originalPane.splitRight() // activate a different pane assert.isUndefined(workspace.getActivePaneItem()) - githubPackage.gitPanelController.props.didSelectFilePatch(filePatch2, 'staged') + await githubPackage.gitPanelController.props.didSelectFilePatch('d.txt', 'staged') assert.equal(githubPackage.filePatchController, existingFilePatchView) - assert.equal(githubPackage.filePatchController.props.filePatch, filePatch2) + assert.equal(githubPackage.filePatchController.props.filePatch.getPath(), 'd.txt') assert.equal(githubPackage.filePatchController.props.repository, repository) assert.equal(githubPackage.filePatchController.props.stagingStatus, 'staged') assert.equal(originalPane.getActiveItem(), githubPackage.filePatchController) @@ -247,9 +249,9 @@ describe('GithubPackage', () => { assert.isUndefined(workspace.getActivePaneItem()) assert.isNull(githubPackage.filePatchController) - githubPackage.gitPanelController.props.didSelectFilePatch(filePatch2, 'staged') + await githubPackage.gitPanelController.props.didSelectFilePatch('d.txt', 'staged') assert.notEqual(githubPackage.filePatchController, existingFilePatchView) - assert.equal(githubPackage.filePatchController.props.filePatch, filePatch2) + assert.equal(githubPackage.filePatchController.props.filePatch.getPath(), 'd.txt') assert.equal(githubPackage.filePatchController.props.repository, repository) assert.equal(githubPackage.filePatchController.props.stagingStatus, 'staged') assert.equal(workspace.getActivePaneItem(), githubPackage.filePatchController) @@ -260,12 +262,11 @@ describe('GithubPackage', () => { it('closes the file patch pane item', async () => { const workdirPath = await cloneRepository('three-files') const repository = await buildRepository(workdirPath) + fs.writeFileSync(path.join(workdirPath, 'a.txt'), 'change', 'utf8') githubPackage.getActiveRepository = function () { return repository } - const hunk = new Hunk(1, 1, 1, 3, []) - const filePatch = new FilePatch('a.txt', 'a.txt', 'modified', [hunk]) - githubPackage.gitPanelController.props.didSelectFilePatch(filePatch, 'staged') + githubPackage.gitPanelController.props.didSelectFilePatch('a.txt', 'unstaged') assert.equal(workspace.getActivePaneItem(), githubPackage.filePatchController) githubPackage.gitPanelController.props.didChangeAmending() @@ -281,6 +282,8 @@ describe('GithubPackage', () => { await workspace.open(path.join(workdirPath, 'a.txt')) await githubPackage.updateActiveRepository() + await etch.getScheduler().getNextUpdatePromise() + githubPackage.statusBarTileController.refs.changedFilesCountView.props.didClick() assert.equal(workspace.getRightPanels().length, 1) diff --git a/test/models/file-patch.test.js b/test/models/file-patch.test.js index ca6cfe69da..23b328fbc0 100644 --- a/test/models/file-patch.test.js +++ b/test/models/file-patch.test.js @@ -225,7 +225,7 @@ describe('FilePatch', () => { lines.splice(12, 1) fs.writeFileSync(path.join(workdirPath, 'sample.js'), lines.join('\n')) - const [patch] = await repository.getUnstagedChanges() + const patch = await repository.getFilePatchForPath('sample.js', false) assert.equal(patch.toString(), dedent` @@ -1,4 +1,5 @@ -var quicksort = function () { @@ -250,7 +250,7 @@ describe('FilePatch', () => { const workingDirPath = await cloneRepository('three-files') const repo = await buildRepository(workingDirPath) fs.writeFileSync(path.join(workingDirPath, 'e.txt'), 'qux', 'utf8') - const [patch] = await repo.getUnstagedChanges() + const patch = await repo.getFilePatchForPath('e.txt', false) assert.equal(patch.toString(), dedent` @@ -0,0 +1,1 @@ diff --git a/test/views/staging-view.test.js b/test/views/staging-view.test.js index c37d3ad4f4..f4c06b30a0 100644 --- a/test/views/staging-view.test.js +++ b/test/views/staging-view.test.js @@ -18,7 +18,6 @@ describe('StagingView', () => { return Array.from(element.children).map(child => child.textContent) } - console.log(refs.unstagedChanges); assert.deepEqual(textContentOfChildren(refs.unstagedChanges), ['a.txt', 'b.txt']) assert.deepEqual(textContentOfChildren(refs.stagedChanges), []) @@ -94,11 +93,11 @@ describe('StagingView', () => { assert.equal(didSelectFilePatch.callCount, 0) view.focus() - assert.isTrue(didSelectFilePatch.calledWith(filePatches[0])) + assert.isTrue(didSelectFilePatch.calledWith(filePatches[0].filePath)) await view.selectNext() - assert.isTrue(didSelectFilePatch.calledWith(filePatches[1])) + assert.isTrue(didSelectFilePatch.calledWith(filePatches[1].filePath)) await view.selectNext() - assert.isTrue(didSelectMergeConflictFile.calledWith(mergeConflicts[0])) + assert.isTrue(didSelectMergeConflictFile.calledWith(mergeConflicts[0].filePath)) document.body.focus() assert.isFalse(view.isFocused()) From 347bb56fad29a8b911b374b897170f31451b0722 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 16 Nov 2016 01:14:34 -0800 Subject: [PATCH 17/49] Update FilePatchView when repository is updated --- lib/controllers/file-patch-controller.js | 8 +++-- lib/github-package.js | 5 ++-- lib/models/model-observer.js | 2 +- lib/views/staging-view.js | 2 +- .../controllers/file-patch-controller.test.js | 29 +++++++++++-------- 5 files changed, 28 insertions(+), 18 deletions(-) diff --git a/lib/controllers/file-patch-controller.js b/lib/controllers/file-patch-controller.js index a394b49a77..4177f25a87 100644 --- a/lib/controllers/file-patch-controller.js +++ b/lib/controllers/file-patch-controller.js @@ -5,6 +5,7 @@ import {Emitter, CompositeDisposable} from 'atom' import etch from 'etch' import FilePatchView from '../views/file-patch-view' +import ModelObserver from '../models/model-observer' export default class FilePatchController { constructor (props) { @@ -15,11 +16,15 @@ export default class FilePatchController { this.stageLines = this.stageLines.bind(this) this.unstageLines = this.unstageLines.bind(this) this.observeFilePatch() + this.repositoryObserver = new ModelObserver({ + didUpdate: () => this.props.update && this.props.update() + }) + this.repositoryObserver.setActiveModel(props.repository) etch.initialize(this) } update (props) { - this.props = props + this.props = Object.assign({}, this.props, props) this.observeFilePatch() this.emitter.emit('did-change-title', this.getTitle()) return etch.update(this) @@ -28,7 +33,6 @@ export default class FilePatchController { observeFilePatch () { if (this.filePatchSubscriptions) this.filePatchSubscriptions.dispose() this.filePatchSubscriptions = new CompositeDisposable( - this.props.filePatch.onDidUpdate(this.didUpdateFilePatch.bind(this)), this.props.filePatch.onDidDestroy(this.didDestroyFilePatch.bind(this)) ) } diff --git a/lib/github-package.js b/lib/github-package.js index d0b12a6367..3ba726d6e1 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -86,11 +86,12 @@ export default class GithubPackage { const repository = this.getActiveRepository() const filePatch = await repository.getFilePatchForPath(filePath, stagingStatus === 'staged') let containingPane + const update = this.didSelectFilePatch.bind(this, filePath, stagingStatus) if (this.filePatchController) { - this.filePatchController.update({filePatch, repository, stagingStatus}) + this.filePatchController.update({filePatch, repository, stagingStatus, update}) containingPane = this.workspace.paneForItem(this.filePatchController) } else { - this.filePatchController = new FilePatchController({filePatch, repository, stagingStatus}) + this.filePatchController = new FilePatchController({filePatch, repository, stagingStatus, update}) this.filePatchController.onDidDestroy(() => { this.filePatchController = null }) containingPane = this.workspace.getActivePane() } diff --git a/lib/models/model-observer.js b/lib/models/model-observer.js index c24495b3e2..a114cacccd 100644 --- a/lib/models/model-observer.js +++ b/lib/models/model-observer.js @@ -2,7 +2,7 @@ export default class RepositoryObserver { constructor ({fetchData, didUpdate}) { - this.fetchData = fetchData + this.fetchData = fetchData || () => {} this.didUpdate = didUpdate || () => {} this.activeModel = null this.activeModelData = null diff --git a/lib/views/staging-view.js b/lib/views/staging-view.js index a2de969386..6e449d37ee 100644 --- a/lib/views/staging-view.js +++ b/lib/views/staging-view.js @@ -142,8 +142,8 @@ export default class StagingView { } else { this.selection.selectItem(item, event.shiftKey) } - this.mouseSelectionInProgress = true await etch.update(this) + this.mouseSelectionInProgress = true } } diff --git a/test/controllers/file-patch-controller.test.js b/test/controllers/file-patch-controller.test.js index 26a36a76fd..4c5fc0853c 100644 --- a/test/controllers/file-patch-controller.test.js +++ b/test/controllers/file-patch-controller.test.js @@ -26,27 +26,28 @@ describe('FilePatchController', () => { }) it('renders FilePatchView only if FilePatch has hunks', async () => { - const filePatch = new FilePatch('a.txt', 'a.txt', 'modified', []) - const view = new FilePatchController({filePatch}) // eslint-disable-line no-new - assert.isUndefined(view.refs.filePatchView) + const emptyFilePatch = new FilePatch('a.txt', 'a.txt', 'modified', []) + const controller = new FilePatchController({filePatch: emptyFilePatch}) // eslint-disable-line no-new + assert.isUndefined(controller.refs.filePatchView) const hunk1 = new Hunk(0, 0, 1, 1, [new HunkLine('line-1', 'added', 1, 1)]) - filePatch.update(new FilePatch('a.txt', 'a.txt', 'modified', [hunk1])) - await etch.getScheduler().getNextUpdatePromise() - assert.isDefined(view.refs.filePatchView) + const filePatch = new FilePatch('a.txt', 'a.txt', 'modified', [hunk1]) + await controller.update({filePatch}) + assert.isDefined(controller.refs.filePatchView) }) - it('updates when the associated FilePatch updates', async () => { + it('updates when a new FilePatch is passed', async () => { const hunk1 = new Hunk(5, 5, 2, 1, [new HunkLine('line-1', 'added', -1, 5)]) const hunk2 = new Hunk(8, 8, 1, 1, [new HunkLine('line-5', 'deleted', 8, -1)]) const hunkViewsByHunk = new Map() const filePatch = new FilePatch('a.txt', 'a.txt', 'modified', [hunk1, hunk2]) - new FilePatchController({filePatch, registerHunkView: (hunk, controller) => hunkViewsByHunk.set(hunk, controller)}) // eslint-disable-line no-new + const controller = new FilePatchController({filePatch, registerHunkView: (hunk, controller) => hunkViewsByHunk.set(hunk, controller)}) // eslint-disable-line no-new + assert(hunkViewsByHunk.get(hunk1) != null) + assert(hunkViewsByHunk.get(hunk2) != null) hunkViewsByHunk.clear() const hunk3 = new Hunk(8, 8, 1, 1, [new HunkLine('line-10', 'modified', 10, 10)]) - filePatch.update(new FilePatch('a.txt', 'a.txt', 'modified', [hunk1, hunk3])) - await etch.getScheduler().getNextUpdatePromise() + await controller.update({filePatch: new FilePatch('a.txt', 'a.txt', 'modified', [hunk1, hunk3])}) assert(hunkViewsByHunk.get(hunk1) != null) assert(hunkViewsByHunk.get(hunk2) == null) assert(hunkViewsByHunk.get(hunk3) != null) @@ -115,7 +116,7 @@ describe('FilePatchController', () => { ) unstagedLines.splice(11, 2, 'this is a modified line') fs.writeFileSync(filePath, unstagedLines.join('\n')) - const unstagedFilePatch = await repository.getFilePatchForPath('sample.js', false) + let unstagedFilePatch = await repository.getFilePatchForPath('sample.js', false) const hunkViewsByHunk = new Map() function registerHunkView (hunk, view) { hunkViewsByHunk.set(hunk, view) } @@ -137,6 +138,8 @@ describe('FilePatchController', () => { assert.autocrlfEqual(await repository.readFileFromIndex('sample.js'), expectedLines.join('\n')) // stage remaining lines in hunk + unstagedFilePatch = await repository.getFilePatchForPath('sample.js', false) + await controller.update({filePatch: unstagedFilePatch}) await hunkView.props.didClickStageButton() expectedLines = originalLines.slice() expectedLines.splice(1, 1, @@ -147,7 +150,7 @@ describe('FilePatchController', () => { assert.autocrlfEqual(await repository.readFileFromIndex('sample.js'), expectedLines.join('\n')) // unstage a subset of lines from the first hunk - const stagedFilePatch = await repository.getFilePatchForPath('sample.js', true) + let stagedFilePatch = await repository.getFilePatchForPath('sample.js', true) await controller.update({filePatch: stagedFilePatch, repository, stagingStatus: 'staged', registerHunkView}) hunk = stagedFilePatch.getHunks()[0] lines = hunk.getLines() @@ -166,6 +169,8 @@ describe('FilePatchController', () => { assert.autocrlfEqual(await repository.readFileFromIndex('sample.js'), expectedLines.join('\n')) // unstage the rest of the hunk + stagedFilePatch = await repository.getFilePatchForPath('sample.js', true) + await controller.update({filePatch: stagedFilePatch}) await view.togglePatchSelectionMode() await hunkView.props.didClickStageButton() assert.autocrlfEqual(await repository.readFileFromIndex('sample.js'), originalLines.join('\n')) From 43e3549a372fa9dfbdf54c79c12d8cef9b1f8a84 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 16 Nov 2016 17:37:14 -0800 Subject: [PATCH 18/49] Correctly display changes relative to HEAD~ when amending --- lib/github-package.js | 4 +-- lib/models/repository.js | 10 +++--- lib/views/staging-view.js | 3 +- test/models/repository.test.js | 58 ++++++++++++++++++---------------- 4 files changed, 40 insertions(+), 35 deletions(-) diff --git a/lib/github-package.js b/lib/github-package.js index 3ba726d6e1..c87f44c3d6 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -82,9 +82,9 @@ export default class GithubPackage { } } - async didSelectFilePatch (filePath, stagingStatus, {focus} = {}) { + async didSelectFilePatch (filePath, stagingStatus, {focus, amending} = {}) { const repository = this.getActiveRepository() - const filePatch = await repository.getFilePatchForPath(filePath, stagingStatus === 'staged') + const filePatch = await repository.getFilePatchForPath(filePath, stagingStatus === 'staged', amending) let containingPane const update = this.didSelectFilePatch.bind(this, filePath, stagingStatus) if (this.filePatchController) { diff --git a/lib/models/repository.js b/lib/models/repository.js index ecd212b2c7..63f97f2126 100644 --- a/lib/models/repository.js +++ b/lib/models/repository.js @@ -133,8 +133,8 @@ export default class Repository { async fetchStagedChangesSinceParentCommit () { try { - const rawDiffs = await this.git.diff({staged: true, baseCommit: 'HEAD~'}) - return this.updateFilePatches(this.stagedFilePatchesSinceParentCommitByPath, rawDiffs) + const stagedFiles = await this.git.diffFileStatus({staged: true, target: 'HEAD~'}) + return Object.keys(stagedFiles).map(filePath => { return {filePath, status: stagedFiles[filePath]} }) } catch (e) { if (e.message.includes(`ambiguous argument 'HEAD~'`)) { return [] @@ -144,8 +144,10 @@ export default class Repository { } } - async getFilePatchForPath (filePath, staged) { - const rawDiffs = await this.git.diff({filePath, staged}) + async getFilePatchForPath (filePath, staged, amend) { + const options = {filePath, staged} + if (amend) options.baseCommit = 'HEAD~' + const rawDiffs = await this.git.diff(options) if (rawDiffs.length !== 1) { throw new Error(`Expected a single diff for ${filePath} but got ${rawDiffs.length}`) } diff --git a/lib/views/staging-view.js b/lib/views/staging-view.js index 6e449d37ee..5bdc8c139b 100644 --- a/lib/views/staging-view.js +++ b/lib/views/staging-view.js @@ -124,7 +124,8 @@ export default class StagingView { } } else { if (this.props.didSelectFilePatch) { - this.props.didSelectFilePatch(selectedItem.filePath, this.selection.getActiveListKey()) + const amending = this.props.isAmending && this.selection.getActiveListKey() === 'staged' + this.props.didSelectFilePatch(selectedItem.filePath, this.selection.getActiveListKey(), {amending}) } } } diff --git a/test/models/repository.test.js b/test/models/repository.test.js index 8d17fd12c6..56f6c484bb 100644 --- a/test/models/repository.test.js +++ b/test/models/repository.test.js @@ -80,39 +80,41 @@ describe('Repository', function () { const workingDirPath = await cloneRepository('multiple-commits') const repo = await buildRepository(workingDirPath) fs.writeFileSync(path.join(workingDirPath, 'file.txt'), 'three\nfour\n', 'utf8') - assertDeepPropertyVals(await repo.getStagedChangesSinceParentCommit(), [ + assert.deepEqual(await repo.getStagedChangesSinceParentCommit(), [ { - oldPath: 'file.txt', - newPath: 'file.txt', - status: 'modified', - hunks: [ - { - lines: [ - {status: 'deleted', text: 'two', oldLineNumber: 1, newLineNumber: -1}, - {status: 'added', text: 'three', oldLineNumber: -1, newLineNumber: 1}, - ] - } - ] + filePath: 'file.txt', + status: 'modified' } ]) + assertDeepPropertyVals(await repo.getFilePatchForPath('file.txt', true, true), { + oldPath: 'file.txt', + newPath: 'file.txt', + status: 'modified', + hunks: [ + { + lines: [ + {status: 'deleted', text: 'two', oldLineNumber: 1, newLineNumber: -1}, + {status: 'added', text: 'three', oldLineNumber: -1, newLineNumber: 1}, + ] + } + ] + }) await repo.stageFiles(['file.txt']) - assertDeepPropertyVals(await repo.getStagedChangesSinceParentCommit(), [ - { - oldPath: 'file.txt', - newPath: 'file.txt', - status: 'modified', - hunks: [ - { - lines: [ - {status: 'deleted', text: 'two', oldLineNumber: 1, newLineNumber: -1}, - {status: 'added', text: 'three', oldLineNumber: -1, newLineNumber: 1}, - {status: 'added', text: 'four', oldLineNumber: -1, newLineNumber: 2}, - ] - } - ] - } - ]) + assertDeepPropertyVals(await repo.getFilePatchForPath('file.txt', true, true), { + oldPath: 'file.txt', + newPath: 'file.txt', + status: 'modified', + hunks: [ + { + lines: [ + {status: 'deleted', text: 'two', oldLineNumber: 1, newLineNumber: -1}, + {status: 'added', text: 'three', oldLineNumber: -1, newLineNumber: 1}, + {status: 'added', text: 'four', oldLineNumber: -1, newLineNumber: 2}, + ] + } + ] + }) await repo.stageFilesFromParentCommit(['file.txt']) assert.deepEqual(await repo.getStagedChangesSinceParentCommit(), []) From 0908170a150c67b18500888294027c3114efe61c Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 16 Nov 2016 17:49:50 -0800 Subject: [PATCH 19/49] :fire: code for updating FilePatch objects Rather than update we just create a new FilePatch object and display it --- lib/models/file-patch.js | 24 -------------- lib/models/repository.js | 25 --------------- test/models/file-patch.test.js | 57 ---------------------------------- 3 files changed, 106 deletions(-) diff --git a/lib/models/file-patch.js b/lib/models/file-patch.js index 988a0b0926..63991c8c1c 100644 --- a/lib/models/file-patch.js +++ b/lib/models/file-patch.js @@ -13,40 +13,16 @@ export default class FilePatch { Object.defineProperty(this, 'emitter', {value: new Emitter(), enumerable: false}) } - copy () { - return new FilePatch( - this.getOldPath(), - this.getNewPath(), - this.getStatus(), - this.getHunks().map(h => h.copy()) - ) - } - destroy () { this.destroyed = true this.emitter.emit('did-destroy') this.emitter.dispose() } - update (filePatch) { - if (this.getPath() !== filePatch.getPath()) { - throw new Error(`Cannot update file patch! This FilePatch path: ${this.getPath()} Other FilePatch path: ${filePatch.getPath()}`) - } - this.hunks = filePatch.getHunks() - this.status = filePatch.getStatus() - this.oldPath = filePatch.getOldPath() - this.newPath = filePatch.getNewPath() - this.emitter.emit('did-update') - } - onDidDestroy (callback) { return this.emitter.on('did-destroy', callback) } - onDidUpdate (callback) { - return this.emitter.on('did-update', callback) - } - isDestroyed () { return this.destroyed } diff --git a/lib/models/repository.js b/lib/models/repository.js index 63f97f2126..52e64aeaeb 100644 --- a/lib/models/repository.js +++ b/lib/models/repository.js @@ -155,31 +155,6 @@ export default class Repository { return filePatch } - updateFilePatches (patchesByPath, rawDiffs) { - const filePatches = this.buildFilePatchesFromRawDiffs(rawDiffs) - - const validFilePatches = new Set() - for (let newPatch of filePatches) { - const path = newPatch.getPath() - const existingPatch = patchesByPath.get(path) - if (existingPatch == null) { - patchesByPath.set(path, newPatch) - } else { - existingPatch.update(newPatch) - } - validFilePatches.add(path) - } - - for (let [path, patch] of patchesByPath) { - if (!validFilePatches.has(path)) { - patchesByPath.delete(path) - patch.destroy() - } - } - - return Array.from(patchesByPath.values()).sort((a, b) => a.getPath().localeCompare(b.getPath())) - } - buildFilePatchesFromRawDiffs (rawDiffs) { const statusMap = { '+': 'added', diff --git a/test/models/file-patch.test.js b/test/models/file-patch.test.js index 23b328fbc0..a01f4dc845 100644 --- a/test/models/file-patch.test.js +++ b/test/models/file-patch.test.js @@ -10,63 +10,6 @@ import Hunk from '../../lib/models/hunk' import HunkLine from '../../lib/models/hunk-line' describe('FilePatch', () => { - describe('update(filePatch)', () => { - it('mutates the FilePatch to match the given FilePatch, replacing its hunks with the new filePatch\'s hunks', () => { - const hunks = [ - new Hunk(1, 1, 1, 3, [ - new HunkLine('line-1', 'added', -1, 1), - new HunkLine('line-2', 'added', -1, 2), - new HunkLine('line-3', 'unchanged', 1, 3) - ]), - new Hunk(5, 7, 5, 4, [ - new HunkLine('line-4', 'unchanged', 5, 7), - new HunkLine('line-5', 'deleted', 6, -1), - new HunkLine('line-6', 'deleted', 7, -1), - new HunkLine('line-7', 'added', -1, 8), - new HunkLine('line-8', 'added', -1, 9), - new HunkLine('line-9', 'added', -1, 10), - new HunkLine('line-10', 'deleted', 8, -1), - new HunkLine('line-11', 'deleted', 9, -1) - ]), - new Hunk(20, 19, 2, 2, [ - new HunkLine('line-12', 'deleted', 20, -1), - new HunkLine('line-13', 'added', -1, 19), - new HunkLine('line-14', 'unchanged', 21, 20) - ]) - ] - const patch = new FilePatch('a.txt', 'a.txt', 'modified', hunks) - const newPatch = new FilePatch('a.txt', 'a.txt', 'modified', [ - new Hunk(9, 9, 2, 1, [ - new HunkLine('line-9', 'added', -1, 9), - new HunkLine('line-10', 'deleted', 8, -1), - new HunkLine('line-11', 'deleted', 9, -1) - ]), - new Hunk(15, 14, 1, 1, [ - new HunkLine('line-15', 'deleted', 15, -1), - new HunkLine('line-16', 'added', -1, 14) - ]), - new Hunk(21, 19, 2, 3, [ - new HunkLine('line-13', 'added', -1, 19), - new HunkLine('line-14', 'unchanged', 21, 20), - new HunkLine('line-17', 'unchanged', 22, 21) - ]) - ]) - - const originalHunks = hunks.slice() - const originalLines = originalHunks.map(h => h.getLines().slice()) - patch.update(newPatch) - const newHunks = patch.getHunks() - - assert.deepEqual(patch, newPatch) - assert.notDeepEqual(originalHunks, newHunks) - }) - - it('throws an error when the supplied filePatch has a different id', () => { - const patch = new FilePatch('a.txt', 'b.txt', 'renamed') - assert.throws(() => patch.update(new FilePatch('c.txt', 'd.txt', 'renamed'))) - }) - }) - describe('getStagePatchForLines()', () => { it('returns a new FilePatch that applies only the specified lines', () => { const filePatch = new FilePatch('a.txt', 'a.txt', 'modified', [ From 5a65a4dfd21aaba5db4e03c175533143be5640d8 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 16 Nov 2016 17:54:22 -0800 Subject: [PATCH 20/49] Fix more tests --- test/models/repository.test.js | 60 ++++++++++------------------------ 1 file changed, 18 insertions(+), 42 deletions(-) diff --git a/test/models/repository.test.js b/test/models/repository.test.js index 56f6c484bb..267fdeefa0 100644 --- a/test/models/repository.test.js +++ b/test/models/repository.test.js @@ -126,53 +126,28 @@ describe('Repository', function () { const workingDirPath = await cloneRepository('three-files') const repo = await buildRepository(workingDirPath) fs.writeFileSync(path.join(workingDirPath, 'subdir-1', 'a.txt'), 'qux\nfoo\nbar\n', 'utf8') - const [unstagedPatch1] = (await repo.getUnstagedChanges()).map(p => p.copy()) + const unstagedPatch1 = await repo.getFilePatchForPath(path.join('subdir-1', 'a.txt'), false) fs.writeFileSync(path.join(workingDirPath, 'subdir-1', 'a.txt'), 'qux\nfoo\nbar\nbaz\n', 'utf8') await repo.refresh() - await repo.getUnstagedChanges() - const [unstagedPatch2] = (await repo.getUnstagedChanges()).map(p => p.copy()) + const unstagedPatch2 = await repo.getFilePatchForPath(path.join('subdir-1', 'a.txt'), false) await repo.applyPatchToIndex(unstagedPatch1) - assertDeepPropertyVals(await repo.getStagedChanges(), [unstagedPatch1]) - const unstagedChanges = await repo.getUnstagedChanges() - assert.equal(unstagedChanges.length, 1) + const stagedPatch1 = await repo.getFilePatchForPath(path.join('subdir-1', 'a.txt'), true) + assert.deepEqual(stagedPatch1, unstagedPatch1) - await repo.applyPatchToIndex(unstagedPatch1.getUnstagePatch()) - assert.deepEqual(await repo.getStagedChanges(), []) - assertDeepPropertyVals(await repo.getUnstagedChanges(), [unstagedPatch2]) - }) + let unstagedChanges = (await repo.getUnstagedChanges()).map(c => c.filePath) + let stagedChanges = (await repo.getStagedChanges()).map(c => c.filePath) + assert.deepEqual(unstagedChanges, ['subdir-1/a.txt']) + assert.deepEqual(stagedChanges, ['subdir-1/a.txt']) - it('emits update events on file patches that change as a result of staging', async () => { - const workdirPath = await cloneRepository('multi-line-file') - const repository = await buildRepository(workdirPath) - const filePath = path.join(workdirPath, 'sample.js') - const originalLines = fs.readFileSync(filePath, 'utf8').split('\n') - const unstagedLines = originalLines.slice() - unstagedLines.splice(1, 1, - 'this is a modified line', - 'this is a new line', - 'this is another new line' - ) - unstagedLines.splice(11, 2, 'this is a modified line') - fs.writeFileSync(filePath, unstagedLines.join('\n')) - await repository.refresh() - - const [unstagedFilePatch] = await repository.getUnstagedChanges() - const unstagedListener = sinon.spy() - unstagedFilePatch.onDidUpdate(unstagedListener) - - await repository.applyPatchToIndex(unstagedFilePatch.getStagePatchForHunk(unstagedFilePatch.getHunks()[1])) - assert.equal(unstagedListener.callCount, 1) - - const [stagedFilePatch] = await repository.getStagedChanges() - const stagedListener = sinon.spy() - stagedFilePatch.onDidUpdate(stagedListener) - - const unstagePatch = stagedFilePatch.getUnstagePatchForLines(new Set(stagedFilePatch.getHunks()[0].getLines().slice(4, 5))) - await repository.applyPatchToIndex(unstagePatch) - assert(stagedListener.callCount, 1) - assert(unstagedListener.callCount, 2) + await repo.applyPatchToIndex(unstagedPatch1.getUnstagePatch()) + const unstagedPatch3 = await repo.getFilePatchForPath(path.join('subdir-1', 'a.txt'), false) + assert.deepEqual(unstagedPatch3, unstagedPatch2) + unstagedChanges = (await repo.getUnstagedChanges()).map(c => c.filePath) + stagedChanges = (await repo.getStagedChanges()).map(c => c.filePath) + assert.deepEqual(unstagedChanges, ['subdir-1/a.txt']) + assert.deepEqual(stagedChanges, []) }) }) @@ -183,7 +158,7 @@ describe('Repository', function () { assert.equal((await repo.getLastCommit()).message, 'Initial commit') fs.writeFileSync(path.join(workingDirPath, 'subdir-1', 'a.txt'), 'qux\nfoo\nbar\n', 'utf8') - const [unstagedPatch1] = (await repo.getUnstagedChanges()).map(p => p.copy()) + const unstagedPatch1 = await repo.getFilePatchForPath(path.join('subdir-1', 'a.txt'), false) fs.writeFileSync(path.join(workingDirPath, 'subdir-1', 'a.txt'), 'qux\nfoo\nbar\nbaz\n', 'utf8') await repo.refresh() await repo.applyPatchToIndex(unstagedPatch1) @@ -194,7 +169,8 @@ describe('Repository', function () { const unstagedChanges = await repo.getUnstagedChanges() assert.equal(unstagedChanges.length, 1) - await repo.applyPatchToIndex(unstagedChanges[0]) + const unstagedPatch2 = await repo.getFilePatchForPath(path.join('subdir-1', 'a.txt'), false) + await repo.applyPatchToIndex(unstagedPatch2) await repo.commit('Commit 2') assert.equal((await repo.getLastCommit()).message, 'Commit 2') await repo.refresh() From eb44ea5e9322c5762461c00fb8191e9a61799275 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 17 Nov 2016 14:05:59 -0800 Subject: [PATCH 21/49] :art: RepositoryObserver -> ModelObserver --- lib/models/model-observer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/models/model-observer.js b/lib/models/model-observer.js index a114cacccd..53e48590eb 100644 --- a/lib/models/model-observer.js +++ b/lib/models/model-observer.js @@ -1,6 +1,6 @@ /** @babel */ -export default class RepositoryObserver { +export default class ModelObserver { constructor ({fetchData, didUpdate}) { this.fetchData = fetchData || () => {} this.didUpdate = didUpdate || () => {} From 9d889feb6390c9ac6539f2471fa42cb98aeda56c Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 17 Nov 2016 15:01:40 -0800 Subject: [PATCH 22/49] Fix test --- test/github-package.test.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/github-package.test.js b/test/github-package.test.js index 82abe5ab03..676b53ca92 100644 --- a/test/github-package.test.js +++ b/test/github-package.test.js @@ -263,10 +263,12 @@ describe('GithubPackage', () => { const workdirPath = await cloneRepository('three-files') const repository = await buildRepository(workdirPath) fs.writeFileSync(path.join(workdirPath, 'a.txt'), 'change', 'utf8') + await repository.stageFiles(['a.txt']) githubPackage.getActiveRepository = function () { return repository } - githubPackage.gitPanelController.props.didSelectFilePatch('a.txt', 'unstaged') + await githubPackage.gitPanelController.props.didSelectFilePatch('a.txt', 'staged') + assert.isOk(githubPackage.filePatchController) assert.equal(workspace.getActivePaneItem(), githubPackage.filePatchController) githubPackage.gitPanelController.props.didChangeAmending() From 1fab5dde1e8481e34c128ca8eb22d534cf854483 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 17 Nov 2016 16:56:30 -0800 Subject: [PATCH 23/49] Use injected workspace rather than atom.workspace --- lib/github-package.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/github-package.js b/lib/github-package.js index c87f44c3d6..dc2b1c7873 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -17,7 +17,7 @@ export default class GithubPackage { this.commandRegistry = commandRegistry this.notificationManager = notificationManager this.changeObserver = process.platform === 'linux' - ? new WorkspaceChangeObserver(window, atom.workspace) + ? new WorkspaceChangeObserver(window, this.workspace) : new FileSystemChangeObserver() this.activeRepository = null this.repositoriesByProjectDirectory = new Map() From e3177e6eadb8444f2396445d20c19e938de545ea Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 17 Nov 2016 17:56:29 -0800 Subject: [PATCH 24/49] Only have DOM-related logic in writeAfterUpdate hook Call `reportSelectedItem` in methods that change selected item --- lib/views/staging-view.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/views/staging-view.js b/lib/views/staging-view.js index 5bdc8c139b..176aedfb64 100644 --- a/lib/views/staging-view.js +++ b/lib/views/staging-view.js @@ -81,35 +81,39 @@ export default class StagingView { selectPrevious (preserveTail = false) { this.selection.selectPreviousItem(preserveTail) this.selection.coalesce() + this.reportSelectedItem() return etch.update(this) } selectNext (preserveTail = false) { this.selection.selectNextItem(preserveTail) this.selection.coalesce() + this.reportSelectedItem() return etch.update(this) } selectAll () { this.selection.selectAllItems() this.selection.coalesce() + this.reportSelectedItem() return etch.update(this) } selectFirst (preserveTail = false) { this.selection.selectFirstItem(preserveTail) this.selection.coalesce() + this.reportSelectedItem() return etch.update(this) } selectLast (preserveTail = false) { this.selection.selectLastItem(preserveTail) this.selection.coalesce() + this.reportSelectedItem() return etch.update(this) } writeAfterUpdate () { - this.reportSelectedItem() const headItem = this.selection.getHeadItem() if (headItem) this.listElementsByItem.get(headItem).scrollIntoViewIfNeeded() } @@ -160,6 +164,7 @@ export default class StagingView { mouseup () { this.mouseSelectionInProgress = false this.selection.coalesce() + this.reportSelectedItem() } render () { From 710bfbaa68504b5f7cc550f2c34da0ca04e95a8a Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 17 Nov 2016 18:15:47 -0800 Subject: [PATCH 25/49] Rely on file system change events to trigger repository refresh Don't manually trigger refresh. This causes duplicate fetching and doesn't necessarily add value. To address lag in UI updates in the future we may want to eagerly update the UI. --- lib/models/repository.js | 5 ---- test/controllers/git-panel-controller.test.js | 6 +++++ .../status-bar-tile-controller.test.js | 2 ++ test/models/repository.test.js | 23 +++++++++++++++---- 4 files changed, 27 insertions(+), 9 deletions(-) diff --git a/lib/models/repository.js b/lib/models/repository.js index 52e64aeaeb..aaeaa3c4e6 100644 --- a/lib/models/repository.js +++ b/lib/models/repository.js @@ -192,23 +192,19 @@ export default class Repository { async stageFiles (paths) { await this.git.stageFiles(paths) - await this.refresh() } async unstageFiles (paths) { await this.git.unstageFiles(paths) - await this.refresh() } async stageFilesFromParentCommit (paths) { await this.git.unstageFiles(paths, 'HEAD~') - await this.refresh() } async applyPatchToIndex (filePatch) { const patchStr = filePatch.getHeaderString() + filePatch.toString() await this.git.applyPatchToIndex(patchStr) - await this.refresh() } async pathHasMergeMarkers (relativePath) { @@ -315,7 +311,6 @@ export default class Repository { async checkout (branchName, options) { await this.git.checkout(branchName, options) - await this.refresh() } setCommitState (state) { diff --git a/test/controllers/git-panel-controller.test.js b/test/controllers/git-panel-controller.test.js index 885ab92003..a87c75547e 100644 --- a/test/controllers/git-panel-controller.test.js +++ b/test/controllers/git-panel-controller.test.js @@ -108,12 +108,15 @@ describe('GitPanelController', () => { assert.equal(stagingView.props.unstagedChanges.length, 2) assert.equal(stagingView.props.stagedChanges.length, 0) await stagingView.mousedownOnItem({detail: 2}, stagingView.props.unstagedChanges[0]) + await repository.refresh() await controller.getLastModelDataRefreshPromise() await stagingView.mousedownOnItem({detail: 2}, stagingView.props.unstagedChanges[0]) + await repository.refresh() await controller.getLastModelDataRefreshPromise() assert.equal(stagingView.props.unstagedChanges.length, 0) assert.equal(stagingView.props.stagedChanges.length, 2) await stagingView.mousedownOnItem({detail: 2}, stagingView.props.stagedChanges[1]) + await repository.refresh() await controller.getLastModelDataRefreshPromise() assert.equal(stagingView.props.unstagedChanges.length, 1) assert.equal(stagingView.props.stagedChanges.length, 1) @@ -155,6 +158,7 @@ describe('GitPanelController', () => { // click Cancel choice = 1 await stagingView.mousedownOnItem({detail: 2}, conflict1) + await repository.refresh() await controller.getLastModelDataRefreshPromise() assert.equal(atom.confirm.calledOnce, true) assert.equal(stagingView.props.mergeConflicts.length, 5) @@ -164,6 +168,7 @@ describe('GitPanelController', () => { choice = 0 atom.confirm.reset() await stagingView.mousedownOnItem({detail: 2}, conflict1) + await repository.refresh() await controller.getLastModelDataRefreshPromise() assert.equal(atom.confirm.calledOnce, true) assert.equal(stagingView.props.mergeConflicts.length, 4) @@ -174,6 +179,7 @@ describe('GitPanelController', () => { atom.confirm.reset() fs.writeFileSync(path.join(workdirPath, conflict2.filePath), 'text with no merge markers') await stagingView.mousedownOnItem({detail: 2}, conflict2) + await repository.refresh() await controller.getLastModelDataRefreshPromise() assert.equal(atom.confirm.called, false) assert.equal(stagingView.props.mergeConflicts.length, 3) diff --git a/test/controllers/status-bar-tile-controller.test.js b/test/controllers/status-bar-tile-controller.test.js index d862eb940e..3195e0f446 100644 --- a/test/controllers/status-bar-tile-controller.test.js +++ b/test/controllers/status-bar-tile-controller.test.js @@ -129,6 +129,7 @@ describe('StatusBarTileController', () => { branchMenuView.refs.editor.setText('new-branch') await newBranchButton.onclick() + await repository.refresh() await controller.getLastModelDataRefreshPromise() assert.isUndefined(branchMenuView.refs.editor) @@ -305,6 +306,7 @@ describe('StatusBarTileController', () => { fs.unlinkSync(path.join(workdirPath, 'b.txt')) repository.refresh() await repository.stageFiles(['a.txt']) + await repository.refresh() await controller.getLastModelDataRefreshPromise() assert.equal(changedFilesCountView.element.textContent, '2 files') diff --git a/test/models/repository.test.js b/test/models/repository.test.js index 267fdeefa0..d38137e0d1 100644 --- a/test/models/repository.test.js +++ b/test/models/repository.test.js @@ -17,10 +17,12 @@ describe('Repository', function () { const filePath = patch.filePath await repo.stageFiles([filePath]) + await repo.refresh() assert.deepEqual(await repo.getUnstagedChanges(), []) assert.deepEqual(await repo.getStagedChanges(), [patch]) await repo.unstageFiles([filePath]) + await repo.refresh() assert.deepEqual(await repo.getUnstagedChanges(), [patch]) assert.deepEqual(await repo.getStagedChanges(), []) }) @@ -33,10 +35,12 @@ describe('Repository', function () { const filePath = patch.filePath await repo.stageFiles([filePath]) + await repo.refresh() assert.deepEqual(await repo.getUnstagedChanges(), []) assert.deepEqual(await repo.getStagedChanges(), [patch]) await repo.unstageFiles([filePath]) + await repo.refresh() assert.deepEqual(await repo.getUnstagedChanges(), [patch]) assert.deepEqual(await repo.getStagedChanges(), []) }) @@ -49,13 +53,13 @@ describe('Repository', function () { const filePath1 = patches[0].filePath const filePath2 = patches[1].filePath - await repo.stageFiles([filePath1]) - await repo.stageFiles([filePath2]) + await repo.stageFiles([filePath1, filePath2]) + await repo.refresh() assertEqualSortedArraysByKey(await repo.getStagedChanges(), patches, 'filePath') assert.deepEqual(await repo.getUnstagedChanges(), []) - await repo.unstageFiles([filePath1]) - await repo.unstageFiles([filePath2]) + await repo.unstageFiles([filePath1, filePath2]) + await repo.refresh() assertEqualSortedArraysByKey(await repo.getUnstagedChanges(), patches, 'filePath') assert.deepEqual(await repo.getStagedChanges(), []) }) @@ -68,10 +72,12 @@ describe('Repository', function () { const filePath = patch.filePath await repo.stageFiles([filePath]) + await repo.refresh() assert.deepEqual(await repo.getUnstagedChanges(), []) assert.deepEqual(await repo.getStagedChanges(), [patch]) await repo.unstageFiles([filePath]) + await repo.refresh() assert.deepEqual(await repo.getUnstagedChanges(), [patch]) assert.deepEqual(await repo.getStagedChanges(), []) }) @@ -101,6 +107,7 @@ describe('Repository', function () { }) await repo.stageFiles(['file.txt']) + await repo.refresh() assertDeepPropertyVals(await repo.getFilePatchForPath('file.txt', true, true), { oldPath: 'file.txt', newPath: 'file.txt', @@ -117,6 +124,7 @@ describe('Repository', function () { }) await repo.stageFilesFromParentCommit(['file.txt']) + await repo.refresh() assert.deepEqual(await repo.getStagedChangesSinceParentCommit(), []) }) }) @@ -133,6 +141,7 @@ describe('Repository', function () { const unstagedPatch2 = await repo.getFilePatchForPath(path.join('subdir-1', 'a.txt'), false) await repo.applyPatchToIndex(unstagedPatch1) + await repo.refresh() const stagedPatch1 = await repo.getFilePatchForPath(path.join('subdir-1', 'a.txt'), true) assert.deepEqual(stagedPatch1, unstagedPatch1) @@ -142,6 +151,7 @@ describe('Repository', function () { assert.deepEqual(stagedChanges, ['subdir-1/a.txt']) await repo.applyPatchToIndex(unstagedPatch1.getUnstagePatch()) + await repo.refresh() const unstagedPatch3 = await repo.getFilePatchForPath(path.join('subdir-1', 'a.txt'), false) assert.deepEqual(unstagedPatch3, unstagedPatch2) unstagedChanges = (await repo.getUnstagedChanges()).map(c => c.filePath) @@ -443,12 +453,14 @@ describe('Repository', function () { assert.deepEqual(stagedFilePatches.map(patch => patch.filePath), []) await repo.stageFiles(['added-to-both.txt']) + await repo.refresh() stagedFilePatches = await repo.getStagedChanges() assert.deepEqual(stagedFilePatches.map(patch => patch.filePath), ['added-to-both.txt']) // choose version of the file on head fs.writeFileSync(path.join(workingDirPath, 'modified-on-both-ours.txt'), 'master modification\n', 'utf8') await repo.stageFiles(['modified-on-both-ours.txt']) + await repo.refresh() stagedFilePatches = await repo.getStagedChanges() // nothing additional to stage assert.deepEqual(stagedFilePatches.map(patch => patch.filePath), ['added-to-both.txt']) @@ -456,18 +468,21 @@ describe('Repository', function () { // choose version of the file on branch fs.writeFileSync(path.join(workingDirPath, 'modified-on-both-ours.txt'), 'branch modification\n', 'utf8') await repo.stageFiles(['modified-on-both-ours.txt']) + await repo.refresh() stagedFilePatches = await repo.getStagedChanges() assert.deepEqual(stagedFilePatches.map(patch => patch.filePath), ['added-to-both.txt', 'modified-on-both-ours.txt']) // remove file that was deleted on branch fs.unlinkSync(path.join(workingDirPath, 'removed-on-branch.txt')) await repo.stageFiles(['removed-on-branch.txt']) + await repo.refresh() stagedFilePatches = await repo.getStagedChanges() assert.deepEqual(stagedFilePatches.map(patch => patch.filePath), ['added-to-both.txt', 'modified-on-both-ours.txt', 'removed-on-branch.txt']) // remove file that was deleted on master fs.unlinkSync(path.join(workingDirPath, 'removed-on-master.txt')) await repo.stageFiles(['removed-on-master.txt']) + await repo.refresh() stagedFilePatches = await repo.getStagedChanges() // nothing additional to stage assert.deepEqual(stagedFilePatches.map(patch => patch.filePath), ['added-to-both.txt', 'modified-on-both-ours.txt', 'removed-on-branch.txt']) From e2d3242e3bf8e5f0d2ed75480e7f5f54c71f75e7 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 17 Nov 2016 18:41:53 -0800 Subject: [PATCH 26/49] Destroy FilePatch and StatusBarTile controllers in GithubPackage#destroy --- lib/controllers/file-patch-controller.js | 1 + lib/controllers/git-panel-controller.js | 4 ++++ lib/controllers/status-bar-tile-controller.js | 1 + lib/github-package.js | 2 ++ lib/models/model-observer.js | 4 ++++ 5 files changed, 12 insertions(+) diff --git a/lib/controllers/file-patch-controller.js b/lib/controllers/file-patch-controller.js index 4177f25a87..6604df1c21 100644 --- a/lib/controllers/file-patch-controller.js +++ b/lib/controllers/file-patch-controller.js @@ -40,6 +40,7 @@ export default class FilePatchController { destroy () { this.emitter.emit('did-destroy') this.filePatchSubscriptions.dispose() + this.repositoryObserver.destroy() return etch.destroy(this) } diff --git a/lib/controllers/git-panel-controller.js b/lib/controllers/git-panel-controller.js index af06bccd24..daa64f3245 100644 --- a/lib/controllers/git-panel-controller.js +++ b/lib/controllers/git-panel-controller.js @@ -70,6 +70,10 @@ export default class GitPanelController { return this.repositoryObserver.setActiveModel(props.repository) } + destroy () { + this.repositoryObserver.destroy() + } + getLastModelDataRefreshPromise () { return this.repositoryObserver.getLastModelDataRefreshPromise() } diff --git a/lib/controllers/status-bar-tile-controller.js b/lib/controllers/status-bar-tile-controller.js index 452a8affc7..26febbd2d8 100644 --- a/lib/controllers/status-bar-tile-controller.js +++ b/lib/controllers/status-bar-tile-controller.js @@ -194,5 +194,6 @@ export default class StatusBarTileController { destroy () { this.branchTooltipDisposable && this.branchTooltipDisposable.dispose() this.pushPullTooltipDisposable && this.pushPullTooltipDisposable.dispose() + this.repositoryObserver.destroy() } } diff --git a/lib/github-package.js b/lib/github-package.js index dc2b1c7873..beb1333a4b 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -202,5 +202,7 @@ export default class GithubPackage { this.subscriptions.dispose() if (this.destroyedRepositorySubscription) this.destroyedRepositorySubscription.dispose() this.gitPanelController.destroy() + this.statusBarTileController.destroy() + if (this.filePatchController) this.filePatchController.destroy() } } diff --git a/lib/models/model-observer.js b/lib/models/model-observer.js index 53e48590eb..5f5b17f25d 100644 --- a/lib/models/model-observer.js +++ b/lib/models/model-observer.js @@ -52,4 +52,8 @@ export default class ModelObserver { getLastModelDataRefreshPromise () { return this.lastModelDataRefreshPromise } + + destroy () { + if (this.activeModelUpdateSubscription) this.activeModelUpdateSubscription.dispose() + } } From b048821e998b3c3e12d8c3afed3fad3e425575f4 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 17 Nov 2016 18:54:18 -0800 Subject: [PATCH 27/49] Flag tests that have red in console for further investigation --- test/github-package.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/github-package.test.js b/test/github-package.test.js index 676b53ca92..a2307f873f 100644 --- a/test/github-package.test.js +++ b/test/github-package.test.js @@ -169,7 +169,7 @@ describe('GithubPackage', () => { }) describe('when the active item is a FilePatchController', () => { - it('updates the active repository to be the one associated with the FilePatchController', async () => { + xit('updates the active repository to be the one associated with the FilePatchController', async () => { const workdirPath = await cloneRepository('three-files') fs.writeFileSync(path.join(workdirPath, 'a.txt'), 'change', 'utf8') @@ -214,7 +214,7 @@ describe('GithubPackage', () => { }) describe('when a FilePatch is selected in the staging panel', () => { - it('shows a FilePatchView for the selected patch as a pane item, always updating the existing pane item', async () => { + xit('shows a FilePatchView for the selected patch as a pane item, always updating the existing pane item', async () => { const workdirPath = await cloneRepository('three-files') const repository = await buildRepository(workdirPath) @@ -259,7 +259,7 @@ describe('GithubPackage', () => { }) describe('when amend mode is toggled in the staging panel while viewing a staged change', () => { - it('closes the file patch pane item', async () => { + xit('closes the file patch pane item', async () => { const workdirPath = await cloneRepository('three-files') const repository = await buildRepository(workdirPath) fs.writeFileSync(path.join(workdirPath, 'a.txt'), 'change', 'utf8') From 1b4bf5a9e4b359532ebcfba883e31ca88e292c77 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 17 Nov 2016 20:08:19 -0800 Subject: [PATCH 28/49] :fire: MergeConflict objects These are no longer necessary since we've simplified StagingView models to be plain ole JS objects. And for now we're simply displaying the merge conflict file as it appears in the working directory. In the future we may want to display a special UI for resolving merge conflicts, much like Ash Wilson's https://atom.io/packages/merge-conflicts --- lib/models/merge-conflict.js | 39 ------------------------------------ lib/models/repository.js | 2 -- 2 files changed, 41 deletions(-) delete mode 100644 lib/models/merge-conflict.js diff --git a/lib/models/merge-conflict.js b/lib/models/merge-conflict.js deleted file mode 100644 index 0eeb539e44..0000000000 --- a/lib/models/merge-conflict.js +++ /dev/null @@ -1,39 +0,0 @@ -/** @babel */ - -export default class MergeConflict { - constructor (path, oursStatus, theirsStatus, fileStatus) { - this.path = path - this.oursStatus = oursStatus - this.theirsStatus = theirsStatus - this.fileStatus = fileStatus // A (added), D (deleted), M (modified), E (equivalent) - Object.defineProperty(this, 'destroyed', {value: false, enumerable: false, writable: true}) - } - - getPath () { - return this.path - } - - getOursStatus () { - return this.oursStatus - } - - getTheirsStatus () { - return this.theirsStatus - } - - getFileStatus () { - return this.fileStatus - } - - updateFileStatus (fileStatus) { - this.fileStatus = fileStatus - } - - destroy () { - this.destroyed = true - } - - isDestroyed () { - return this.destroyed - } -} diff --git a/lib/models/repository.js b/lib/models/repository.js index aaeaa3c4e6..f54414eec8 100644 --- a/lib/models/repository.js +++ b/lib/models/repository.js @@ -7,7 +7,6 @@ import GitShellOutStrategy from '../git-shell-out-strategy' import FilePatch from './file-patch' import Hunk from './hunk' import HunkLine from './hunk-line' -import MergeConflict from './merge-conflict' import {readFile} from '../helpers' const MERGE_MARKER_REGEX = /^(>|<){7} \S+$/m @@ -47,7 +46,6 @@ export default class Repository { this.stagedFilePatchesByPath = new Map() this.stagedFilePatchesSinceParentCommitByPath = new Map() this.unstagedFilePatchesByPath = new Map() - this.mergeConflictsByPath = new Map() this.git = gitStrategy this.destroyed = false } From bfc6fb50ac079da719879906013bfe74f044258c Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 17 Nov 2016 20:14:00 -0800 Subject: [PATCH 29/49] Simplify FilePatch objects and remove emitter This makes managing the FilePatch objects easier since there is no need to update or destroy them. We can simply delete references to them and allow them to be garbage collected. The view will destroy the FilePatchController when there is no longer a diff for its associated file. --- lib/controllers/file-patch-controller.js | 10 ---------- lib/models/file-patch.js | 16 ---------------- test/controllers/file-patch-controller.test.js | 12 +++--------- 3 files changed, 3 insertions(+), 35 deletions(-) diff --git a/lib/controllers/file-patch-controller.js b/lib/controllers/file-patch-controller.js index 6604df1c21..386a6918c3 100644 --- a/lib/controllers/file-patch-controller.js +++ b/lib/controllers/file-patch-controller.js @@ -15,7 +15,6 @@ export default class FilePatchController { this.unstageHunk = this.unstageHunk.bind(this) this.stageLines = this.stageLines.bind(this) this.unstageLines = this.unstageLines.bind(this) - this.observeFilePatch() this.repositoryObserver = new ModelObserver({ didUpdate: () => this.props.update && this.props.update() }) @@ -25,21 +24,12 @@ export default class FilePatchController { update (props) { this.props = Object.assign({}, this.props, props) - this.observeFilePatch() this.emitter.emit('did-change-title', this.getTitle()) return etch.update(this) } - observeFilePatch () { - if (this.filePatchSubscriptions) this.filePatchSubscriptions.dispose() - this.filePatchSubscriptions = new CompositeDisposable( - this.props.filePatch.onDidDestroy(this.didDestroyFilePatch.bind(this)) - ) - } - destroy () { this.emitter.emit('did-destroy') - this.filePatchSubscriptions.dispose() this.repositoryObserver.destroy() return etch.destroy(this) } diff --git a/lib/models/file-patch.js b/lib/models/file-patch.js index 63991c8c1c..87f773d156 100644 --- a/lib/models/file-patch.js +++ b/lib/models/file-patch.js @@ -9,22 +9,6 @@ export default class FilePatch { this.newPath = newPath this.status = status this.hunks = hunks - Object.defineProperty(this, 'destroyed', {value: false, enumerable: false, writable: true}) - Object.defineProperty(this, 'emitter', {value: new Emitter(), enumerable: false}) - } - - destroy () { - this.destroyed = true - this.emitter.emit('did-destroy') - this.emitter.dispose() - } - - onDidDestroy (callback) { - return this.emitter.on('did-destroy', callback) - } - - isDestroyed () { - return this.destroyed } getOldPath () { diff --git a/test/controllers/file-patch-controller.test.js b/test/controllers/file-patch-controller.test.js index 4c5fc0853c..b5ae7e4f8b 100644 --- a/test/controllers/file-patch-controller.test.js +++ b/test/controllers/file-patch-controller.test.js @@ -53,15 +53,6 @@ describe('FilePatchController', () => { assert(hunkViewsByHunk.get(hunk3) != null) }) - it('gets destroyed if the associated FilePatch is destroyed', () => { - const filePatch1 = new FilePatch('a.txt', 'a.txt', 'modified', [new Hunk(1, 1, 1, 3, [])]) - const controller = new FilePatchController({filePatch: filePatch1}) - const destroyHandler = sinon.spy() - controller.onDidDestroy(destroyHandler) - filePatch1.destroy() - assert(destroyHandler.called) - }) - describe('integration tests', () => { it('stages and unstages hunks when the stage button is clicked on hunk views with no individual lines selected', async () => { const workdirPath = await cloneRepository('multi-line-file') @@ -130,6 +121,7 @@ describe('FilePatchController', () => { hunkView.props.mousemoveOnLine({}, hunk, lines[3]) view.mouseup() await hunkView.props.didClickStageButton() + await repository.refresh() let expectedLines = originalLines.slice() expectedLines.splice(1, 1, 'this is a modified line', @@ -141,6 +133,7 @@ describe('FilePatchController', () => { unstagedFilePatch = await repository.getFilePatchForPath('sample.js', false) await controller.update({filePatch: unstagedFilePatch}) await hunkView.props.didClickStageButton() + await repository.refresh() expectedLines = originalLines.slice() expectedLines.splice(1, 1, 'this is a modified line', @@ -161,6 +154,7 @@ describe('FilePatchController', () => { view.mouseup() await hunkView.props.didClickStageButton() + await repository.refresh() expectedLines = originalLines.slice() expectedLines.splice(2, 0, 'this is a new line', From f4bd6f289254293ebfe25b3354fc84e33fd38527 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 17 Nov 2016 20:14:46 -0800 Subject: [PATCH 30/49] Cache FilePatch objects and clear them when the repository is refreshed --- lib/github-package.js | 10 +++++++-- lib/models/repository.js | 41 ++++++++++++++++++++++++++++++---- test/models/repository.test.js | 27 ++++++++++++++++++++++ 3 files changed, 72 insertions(+), 6 deletions(-) diff --git a/lib/github-package.js b/lib/github-package.js index beb1333a4b..0c89dce4fe 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -88,8 +88,14 @@ export default class GithubPackage { let containingPane const update = this.didSelectFilePatch.bind(this, filePath, stagingStatus) if (this.filePatchController) { - this.filePatchController.update({filePatch, repository, stagingStatus, update}) - containingPane = this.workspace.paneForItem(this.filePatchController) + if (!filePatch) { + this.filePatchController.destroy() + this.filePatchController = null + return + } else { + this.filePatchController.update({filePatch, repository, stagingStatus, update}) + containingPane = this.workspace.paneForItem(this.filePatchController) + } } else { this.filePatchController = new FilePatchController({filePatch, repository, stagingStatus, update}) this.filePatchController.onDidDestroy(() => { this.filePatchController = null }) diff --git a/lib/models/repository.js b/lib/models/repository.js index f54414eec8..11979a6d2e 100644 --- a/lib/models/repository.js +++ b/lib/models/repository.js @@ -85,6 +85,7 @@ export default class Repository { } async refresh () { + this.clearCachedFilePatches() if (process.env.PRINT_GIT_TIMES) console.time('refresh') this.fileStatusesPromise = this.fetchStatusesForChangedFiles() this.stagedChangesSinceParentCommitPromise = this.fetchStagedChangesSinceParentCommit() @@ -142,15 +143,47 @@ export default class Repository { } } + clearCachedFilePatches () { + this.stagedFilePatchesByPath = new Map() + this.stagedFilePatchesSinceParentCommitByPath = new Map() + this.unstagedFilePatchesByPath = new Map() + } + + getCachedFilePatchForPath (filePath, {staged, amend} = {}) { + if (staged && amend) { + return this.stagedFilePatchesSinceParentCommitByPath.get(filePath) + } else if (staged) { + return this.stagedFilePatchesByPath.get(filePath) + } else { + return this.unstagedFilePatchesByPath.get(filePath) + } + } + + cacheFilePatchForPath (filePath, filePatch, {staged, amend} = {}) { + let cachedFilePatch + if (staged && amend) { + this.stagedFilePatchesSinceParentCommitByPath.set(filePath, filePatch) + } else if (staged) { + this.stagedFilePatchesByPath.set(filePath, filePatch) + } else { + this.unstagedFilePatchesByPath.set(filePath, filePatch) + } + } + async getFilePatchForPath (filePath, staged, amend) { const options = {filePath, staged} + const cachedFilePatch = this.getCachedFilePatchForPath(filePath, options) + if (cachedFilePatch) return cachedFilePatch + if (amend) options.baseCommit = 'HEAD~' const rawDiffs = await this.git.diff(options) - if (rawDiffs.length !== 1) { - throw new Error(`Expected a single diff for ${filePath} but got ${rawDiffs.length}`) + if (rawDiffs.length === 0) { + return null + } else if (rawDiffs.length === 1) { + const [filePatch] = this.buildFilePatchesFromRawDiffs(rawDiffs) + this.cacheFilePatchForPath(filePath, filePatch, options) + return filePatch } - const [filePatch] = this.buildFilePatchesFromRawDiffs(rawDiffs) - return filePatch } buildFilePatchesFromRawDiffs (rawDiffs) { diff --git a/test/models/repository.test.js b/test/models/repository.test.js index d38137e0d1..5c92ad6f4d 100644 --- a/test/models/repository.test.js +++ b/test/models/repository.test.js @@ -129,6 +129,33 @@ describe('Repository', function () { }) }) + describe('getFilePatchForPath', () => { + it('returns cached FilePatch objects if they exist', async () => { + const workingDirPath = await cloneRepository('three-files') + const repo = await buildRepository(workingDirPath) + fs.writeFileSync(path.join(workingDirPath, 'a.txt'), 'qux\nfoo\nbar\n', 'utf8') + fs.unlinkSync(path.join(workingDirPath, 'b.txt')) + + const filePatchA = await repo.getFilePatchForPath('a.txt', false, false) + const filePatchB = await repo.getFilePatchForPath('b.txt', false, false) + assert.equal(await repo.getFilePatchForPath('a.txt', false, false), filePatchA) + assert.equal(await repo.getFilePatchForPath('b.txt', false, false), filePatchB) + }) + + it('returns new FilePatch object after repository refresh', async () => { + const workingDirPath = await cloneRepository('three-files') + const repo = await buildRepository(workingDirPath) + fs.writeFileSync(path.join(workingDirPath, 'a.txt'), 'qux\nfoo\nbar\n', 'utf8') + + const filePatchA = await repo.getFilePatchForPath('a.txt', false, false) + assert.equal(await repo.getFilePatchForPath('a.txt', false, false), filePatchA) + + await repo.refresh() + assert.notEqual(await repo.getFilePatchForPath('a.txt', false, false), filePatchA) + assert.deepEqual(await repo.getFilePatchForPath('a.txt', false, false), filePatchA) + }) + }) + describe('applyPatchToIndex', () => { it('can stage and unstage modified files', async () => { const workingDirPath = await cloneRepository('three-files') From 01caf5436b9a981cf9f7508e3132f4d9f8a1b954 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 17 Nov 2016 20:27:20 -0800 Subject: [PATCH 31/49] :art: combine optional parameters for `getFilePatchForPath` into options object --- lib/github-package.js | 2 +- lib/models/repository.js | 5 ++- .../controllers/file-patch-controller.test.js | 12 +++---- test/models/file-patch.test.js | 4 +-- test/models/repository.test.js | 32 +++++++++---------- 5 files changed, 27 insertions(+), 28 deletions(-) diff --git a/lib/github-package.js b/lib/github-package.js index 0c89dce4fe..384eb78f0a 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -84,7 +84,7 @@ export default class GithubPackage { async didSelectFilePatch (filePath, stagingStatus, {focus, amending} = {}) { const repository = this.getActiveRepository() - const filePatch = await repository.getFilePatchForPath(filePath, stagingStatus === 'staged', amending) + const filePatch = await repository.getFilePatchForPath(filePath, {staged: stagingStatus === 'staged', amending}) let containingPane const update = this.didSelectFilePatch.bind(this, filePath, stagingStatus) if (this.filePatchController) { diff --git a/lib/models/repository.js b/lib/models/repository.js index 11979a6d2e..1314d9e1a1 100644 --- a/lib/models/repository.js +++ b/lib/models/repository.js @@ -170,12 +170,11 @@ export default class Repository { } } - async getFilePatchForPath (filePath, staged, amend) { - const options = {filePath, staged} + async getFilePatchForPath (filePath, options = {}) { const cachedFilePatch = this.getCachedFilePatchForPath(filePath, options) if (cachedFilePatch) return cachedFilePatch - if (amend) options.baseCommit = 'HEAD~' + if (options.amending) options.baseCommit = 'HEAD~' const rawDiffs = await this.git.diff(options) if (rawDiffs.length === 0) { return null diff --git a/test/controllers/file-patch-controller.test.js b/test/controllers/file-patch-controller.test.js index b5ae7e4f8b..30b1c3c3b0 100644 --- a/test/controllers/file-patch-controller.test.js +++ b/test/controllers/file-patch-controller.test.js @@ -67,7 +67,7 @@ describe('FilePatchController', () => { ) unstagedLines.splice(11, 2, 'this is a modified line') fs.writeFileSync(filePath, unstagedLines.join('\n')) - const unstagedFilePatch = await repository.getFilePatchForPath('sample.js', false) + const unstagedFilePatch = await repository.getFilePatchForPath('sample.js') const hunkViewsByHunk = new Map() function registerHunkView (hunk, view) { hunkViewsByHunk.set(hunk, view) } @@ -86,7 +86,7 @@ describe('FilePatchController', () => { ) assert.autocrlfEqual(await repository.readFileFromIndex('sample.js'), expectedStagedLines.join('\n')) - const stagedFilePatch = await repository.getFilePatchForPath('sample.js', true) + const stagedFilePatch = await repository.getFilePatchForPath('sample.js', {staged: true}) await controller.update({filePatch: stagedFilePatch, repository, stagingStatus: 'staged', registerHunkView}) await hunkViewsByHunk.get(stagedFilePatch.getHunks()[0]).props.didClickStageButton() assert.autocrlfEqual(await repository.readFileFromIndex('sample.js'), originalLines.join('\n')) @@ -107,7 +107,7 @@ describe('FilePatchController', () => { ) unstagedLines.splice(11, 2, 'this is a modified line') fs.writeFileSync(filePath, unstagedLines.join('\n')) - let unstagedFilePatch = await repository.getFilePatchForPath('sample.js', false) + let unstagedFilePatch = await repository.getFilePatchForPath('sample.js') const hunkViewsByHunk = new Map() function registerHunkView (hunk, view) { hunkViewsByHunk.set(hunk, view) } @@ -130,7 +130,7 @@ describe('FilePatchController', () => { assert.autocrlfEqual(await repository.readFileFromIndex('sample.js'), expectedLines.join('\n')) // stage remaining lines in hunk - unstagedFilePatch = await repository.getFilePatchForPath('sample.js', false) + unstagedFilePatch = await repository.getFilePatchForPath('sample.js') await controller.update({filePatch: unstagedFilePatch}) await hunkView.props.didClickStageButton() await repository.refresh() @@ -143,7 +143,7 @@ describe('FilePatchController', () => { assert.autocrlfEqual(await repository.readFileFromIndex('sample.js'), expectedLines.join('\n')) // unstage a subset of lines from the first hunk - let stagedFilePatch = await repository.getFilePatchForPath('sample.js', true) + let stagedFilePatch = await repository.getFilePatchForPath('sample.js', {staged: true}) await controller.update({filePatch: stagedFilePatch, repository, stagingStatus: 'staged', registerHunkView}) hunk = stagedFilePatch.getHunks()[0] lines = hunk.getLines() @@ -163,7 +163,7 @@ describe('FilePatchController', () => { assert.autocrlfEqual(await repository.readFileFromIndex('sample.js'), expectedLines.join('\n')) // unstage the rest of the hunk - stagedFilePatch = await repository.getFilePatchForPath('sample.js', true) + stagedFilePatch = await repository.getFilePatchForPath('sample.js', {staged: true}) await controller.update({filePatch: stagedFilePatch}) await view.togglePatchSelectionMode() await hunkView.props.didClickStageButton() diff --git a/test/models/file-patch.test.js b/test/models/file-patch.test.js index a01f4dc845..d74a0f5f6e 100644 --- a/test/models/file-patch.test.js +++ b/test/models/file-patch.test.js @@ -168,7 +168,7 @@ describe('FilePatch', () => { lines.splice(12, 1) fs.writeFileSync(path.join(workdirPath, 'sample.js'), lines.join('\n')) - const patch = await repository.getFilePatchForPath('sample.js', false) + const patch = await repository.getFilePatchForPath('sample.js') assert.equal(patch.toString(), dedent` @@ -1,4 +1,5 @@ -var quicksort = function () { @@ -193,7 +193,7 @@ describe('FilePatch', () => { const workingDirPath = await cloneRepository('three-files') const repo = await buildRepository(workingDirPath) fs.writeFileSync(path.join(workingDirPath, 'e.txt'), 'qux', 'utf8') - const patch = await repo.getFilePatchForPath('e.txt', false) + const patch = await repo.getFilePatchForPath('e.txt') assert.equal(patch.toString(), dedent` @@ -0,0 +1,1 @@ diff --git a/test/models/repository.test.js b/test/models/repository.test.js index 5c92ad6f4d..4984261d82 100644 --- a/test/models/repository.test.js +++ b/test/models/repository.test.js @@ -92,7 +92,7 @@ describe('Repository', function () { status: 'modified' } ]) - assertDeepPropertyVals(await repo.getFilePatchForPath('file.txt', true, true), { + assertDeepPropertyVals(await repo.getFilePatchForPath('file.txt', {staged: true, amending: true}), { oldPath: 'file.txt', newPath: 'file.txt', status: 'modified', @@ -108,7 +108,7 @@ describe('Repository', function () { await repo.stageFiles(['file.txt']) await repo.refresh() - assertDeepPropertyVals(await repo.getFilePatchForPath('file.txt', true, true), { + assertDeepPropertyVals(await repo.getFilePatchForPath('file.txt', {staged: true, amending: true}), { oldPath: 'file.txt', newPath: 'file.txt', status: 'modified', @@ -136,10 +136,10 @@ describe('Repository', function () { fs.writeFileSync(path.join(workingDirPath, 'a.txt'), 'qux\nfoo\nbar\n', 'utf8') fs.unlinkSync(path.join(workingDirPath, 'b.txt')) - const filePatchA = await repo.getFilePatchForPath('a.txt', false, false) - const filePatchB = await repo.getFilePatchForPath('b.txt', false, false) - assert.equal(await repo.getFilePatchForPath('a.txt', false, false), filePatchA) - assert.equal(await repo.getFilePatchForPath('b.txt', false, false), filePatchB) + const filePatchA = await repo.getFilePatchForPath('a.txt') + const filePatchB = await repo.getFilePatchForPath('b.txt') + assert.equal(await repo.getFilePatchForPath('a.txt'), filePatchA) + assert.equal(await repo.getFilePatchForPath('b.txt'), filePatchB) }) it('returns new FilePatch object after repository refresh', async () => { @@ -147,12 +147,12 @@ describe('Repository', function () { const repo = await buildRepository(workingDirPath) fs.writeFileSync(path.join(workingDirPath, 'a.txt'), 'qux\nfoo\nbar\n', 'utf8') - const filePatchA = await repo.getFilePatchForPath('a.txt', false, false) - assert.equal(await repo.getFilePatchForPath('a.txt', false, false), filePatchA) + const filePatchA = await repo.getFilePatchForPath('a.txt') + assert.equal(await repo.getFilePatchForPath('a.txt'), filePatchA) await repo.refresh() - assert.notEqual(await repo.getFilePatchForPath('a.txt', false, false), filePatchA) - assert.deepEqual(await repo.getFilePatchForPath('a.txt', false, false), filePatchA) + assert.notEqual(await repo.getFilePatchForPath('a.txt'), filePatchA) + assert.deepEqual(await repo.getFilePatchForPath('a.txt'), filePatchA) }) }) @@ -161,15 +161,15 @@ describe('Repository', function () { const workingDirPath = await cloneRepository('three-files') const repo = await buildRepository(workingDirPath) fs.writeFileSync(path.join(workingDirPath, 'subdir-1', 'a.txt'), 'qux\nfoo\nbar\n', 'utf8') - const unstagedPatch1 = await repo.getFilePatchForPath(path.join('subdir-1', 'a.txt'), false) + const unstagedPatch1 = await repo.getFilePatchForPath(path.join('subdir-1', 'a.txt')) fs.writeFileSync(path.join(workingDirPath, 'subdir-1', 'a.txt'), 'qux\nfoo\nbar\nbaz\n', 'utf8') await repo.refresh() - const unstagedPatch2 = await repo.getFilePatchForPath(path.join('subdir-1', 'a.txt'), false) + const unstagedPatch2 = await repo.getFilePatchForPath(path.join('subdir-1', 'a.txt')) await repo.applyPatchToIndex(unstagedPatch1) await repo.refresh() - const stagedPatch1 = await repo.getFilePatchForPath(path.join('subdir-1', 'a.txt'), true) + const stagedPatch1 = await repo.getFilePatchForPath(path.join('subdir-1', 'a.txt'), {staged: true}) assert.deepEqual(stagedPatch1, unstagedPatch1) let unstagedChanges = (await repo.getUnstagedChanges()).map(c => c.filePath) @@ -179,7 +179,7 @@ describe('Repository', function () { await repo.applyPatchToIndex(unstagedPatch1.getUnstagePatch()) await repo.refresh() - const unstagedPatch3 = await repo.getFilePatchForPath(path.join('subdir-1', 'a.txt'), false) + const unstagedPatch3 = await repo.getFilePatchForPath(path.join('subdir-1', 'a.txt')) assert.deepEqual(unstagedPatch3, unstagedPatch2) unstagedChanges = (await repo.getUnstagedChanges()).map(c => c.filePath) stagedChanges = (await repo.getStagedChanges()).map(c => c.filePath) @@ -195,7 +195,7 @@ describe('Repository', function () { assert.equal((await repo.getLastCommit()).message, 'Initial commit') fs.writeFileSync(path.join(workingDirPath, 'subdir-1', 'a.txt'), 'qux\nfoo\nbar\n', 'utf8') - const unstagedPatch1 = await repo.getFilePatchForPath(path.join('subdir-1', 'a.txt'), false) + const unstagedPatch1 = await repo.getFilePatchForPath(path.join('subdir-1', 'a.txt')) fs.writeFileSync(path.join(workingDirPath, 'subdir-1', 'a.txt'), 'qux\nfoo\nbar\nbaz\n', 'utf8') await repo.refresh() await repo.applyPatchToIndex(unstagedPatch1) @@ -206,7 +206,7 @@ describe('Repository', function () { const unstagedChanges = await repo.getUnstagedChanges() assert.equal(unstagedChanges.length, 1) - const unstagedPatch2 = await repo.getFilePatchForPath(path.join('subdir-1', 'a.txt'), false) + const unstagedPatch2 = await repo.getFilePatchForPath(path.join('subdir-1', 'a.txt')) await repo.applyPatchToIndex(unstagedPatch2) await repo.commit('Commit 2') assert.equal((await repo.getLastCommit()).message, 'Commit 2') From 52b8106353d68e170cd4021d698110c569c2120d Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Fri, 18 Nov 2016 14:57:00 -0800 Subject: [PATCH 32/49] Fix :bug: getting diff for untracked file. Add tests. Add tests for GSOS.diff when `filePath` option is specified. --- lib/git-shell-out-strategy.js | 18 ++-- lib/models/repository.js | 11 +- test/git-shell-out-strategy.test.js | 155 ++++++++++++++++++++++++++++ 3 files changed, 172 insertions(+), 12 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index 5e03181b7a..b50ec4243c 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -206,19 +206,23 @@ export default class GitShellOutStrategy { return output.trim().split(LINE_ENDING_REGEX) } - async diff (options = {}) { + async diff ({filePath, staged, baseCommit} = {}) { let args = ['diff', '--no-prefix', '--no-renames', '--diff-filter=u'] - if (options.staged) args.push('--staged') - if (options.baseCommit) args.push(options.baseCommit) - if (options.filePath) args = args.concat(['--', options.filePath]) + if (staged) args.push('--staged') + if (baseCommit) args.push(baseCommit) + if (filePath) args = args.concat(['--', filePath]) let output = await this.exec(args) let rawDiffs = [] if (output) rawDiffs = parseDiff(output).filter(rawDiff => rawDiff.status !== 'unmerged') - if (!options.staged) { + if (!staged) { // add untracked files - const untrackedFilePatches = await Promise.all((await this.getUntrackedFiles()).map(async (filePath) => { + let untrackedFiles = await this.getUntrackedFiles() + if (filePath) { + untrackedFiles = untrackedFiles.includes(filePath) ? [filePath] : [] + } + const untrackedFilePatches = await Promise.all(untrackedFiles.map(async (filePath) => { const absPath = path.join(this.workingDir, filePath) const stats = await fsStat(absPath) const contents = await readFile(absPath) @@ -226,7 +230,7 @@ export default class GitShellOutStrategy { })) rawDiffs = rawDiffs.concat(untrackedFilePatches) } - + if (filePath) return rawDiffs[0] return rawDiffs.sort((a, b) => (a.newPath || a.oldPath).localeCompare(b.newPath || b.oldPath)) } diff --git a/lib/models/repository.js b/lib/models/repository.js index 1314d9e1a1..3b31f13b43 100644 --- a/lib/models/repository.js +++ b/lib/models/repository.js @@ -174,14 +174,15 @@ export default class Repository { const cachedFilePatch = this.getCachedFilePatchForPath(filePath, options) if (cachedFilePatch) return cachedFilePatch + options.filePath = filePath if (options.amending) options.baseCommit = 'HEAD~' - const rawDiffs = await this.git.diff(options) - if (rawDiffs.length === 0) { - return null - } else if (rawDiffs.length === 1) { - const [filePatch] = this.buildFilePatchesFromRawDiffs(rawDiffs) + const rawDiff = await this.git.diff(options) + if (rawDiff) { + const [filePatch] = this.buildFilePatchesFromRawDiffs([rawDiff]) this.cacheFilePatchForPath(filePath, filePatch, options) return filePatch + } else { + return null } } diff --git a/test/git-shell-out-strategy.test.js b/test/git-shell-out-strategy.test.js index 6fe957fd8e..26a6a68ead 100644 --- a/test/git-shell-out-strategy.test.js +++ b/test/git-shell-out-strategy.test.js @@ -360,6 +360,161 @@ describe('Git commands', () => { const diffOutput = await git.diff() assert.deepEqual(diffOutput, []) }) + + describe('when a file path is specified', () => { + describe('when the file is unstaged', () => { + it('returns a diff comparing the working directory copy of the file and the version on the index', async () => { + const workingDirPath = await cloneRepository('three-files') + const git = new GitShellOutStrategy(workingDirPath) + fs.writeFileSync(path.join(workingDirPath, 'a.txt'), 'qux\nfoo\nbar\n', 'utf8') + fs.renameSync(path.join(workingDirPath, 'c.txt'), path.join(workingDirPath, 'd.txt')) + + assertDeepPropertyVals(await git.diff({filePath: 'a.txt'}), { + 'oldPath': 'a.txt', + 'newPath': 'a.txt', + 'oldMode': '100644', + 'newMode': '100644', + 'hunks': [ + { + 'oldStartLine': 1, + 'oldLineCount': 1, + 'newStartLine': 1, + 'newLineCount': 3, + 'lines': [ + '+qux', + ' foo', + '+bar' + ] + } + ], + 'status': 'modified' + }) + + assertDeepPropertyVals(await git.diff({filePath: 'c.txt'}), { + 'oldPath': 'c.txt', + 'newPath': null, + 'oldMode': '100644', + 'newMode': null, + 'hunks': [ + { + 'oldStartLine': 1, + 'oldLineCount': 1, + 'newStartLine': 0, + 'newLineCount': 0, + 'lines': [ '-baz' ] + } + ], + 'status': 'deleted' + }) + + assertDeepPropertyVals(await git.diff({filePath: 'd.txt'}), { + 'oldPath': null, + 'newPath': 'd.txt', + 'oldMode': null, + 'newMode': '100644', + 'hunks': [ + { + 'oldStartLine': 0, + 'oldLineCount': 0, + 'newStartLine': 1, + 'newLineCount': 1, + 'lines': [ '+baz' ] + } + ], + 'status': 'added' + }) + }) + }) + + describe('when the file is staged', () => { + it('returns a diff comparing the index and head versions of the file', async () => { + const workingDirPath = await cloneRepository('three-files') + const git = new GitShellOutStrategy(workingDirPath) + fs.writeFileSync(path.join(workingDirPath, 'a.txt'), 'qux\nfoo\nbar\n', 'utf8') + fs.renameSync(path.join(workingDirPath, 'c.txt'), path.join(workingDirPath, 'd.txt')) + await git.exec(['add', '.']) + + assertDeepPropertyVals(await git.diff({filePath: 'a.txt', staged: true}), { + 'oldPath': 'a.txt', + 'newPath': 'a.txt', + 'oldMode': '100644', + 'newMode': '100644', + 'hunks': [ + { + 'oldStartLine': 1, + 'oldLineCount': 1, + 'newStartLine': 1, + 'newLineCount': 3, + 'lines': [ + '+qux', + ' foo', + '+bar' + ] + } + ], + 'status': 'modified' + }) + + assertDeepPropertyVals(await git.diff({filePath: 'c.txt', staged: true}), { + 'oldPath': 'c.txt', + 'newPath': null, + 'oldMode': '100644', + 'newMode': null, + 'hunks': [ + { + 'oldStartLine': 1, + 'oldLineCount': 1, + 'newStartLine': 0, + 'newLineCount': 0, + 'lines': [ '-baz' ] + } + ], + 'status': 'deleted' + }) + + assertDeepPropertyVals(await git.diff({filePath: 'd.txt', staged: true}), { + 'oldPath': null, + 'newPath': 'd.txt', + 'oldMode': null, + 'newMode': '100644', + 'hunks': [ + { + 'oldStartLine': 0, + 'oldLineCount': 0, + 'newStartLine': 1, + 'newLineCount': 1, + 'lines': [ '+baz' ] + } + ], + 'status': 'added' + }) + }) + }) + + describe('when the file is staged and a base commit is specified', () => { + it('returns a diff comparing the file on the index and in the specified commit', async () => { + const workingDirPath = await cloneRepository('multiple-commits') + const git = new GitShellOutStrategy(workingDirPath) + + assertDeepPropertyVals(await git.diff({filePath: 'file.txt', staged: true, baseCommit: 'HEAD~'}), { + 'oldPath': 'file.txt', + 'newPath': 'file.txt', + 'oldMode': '100644', + 'newMode': '100644', + 'hunks': [ + { + 'oldStartLine': 1, + 'oldLineCount': 1, + 'newStartLine': 1, + 'newLineCount': 1, + 'lines': [ '-two', '+three' ] + } + ], + 'status': 'modified' + }) + }) + }) + }) }) describe('isMerging', () => { From a729cbcb280d9f908b711d6f35264b3a0ed088fc Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Fri, 18 Nov 2016 15:04:45 -0800 Subject: [PATCH 33/49] Un-ignore tests These tests previously passed but created several errors in the console. Those errors are no longer observed --- test/github-package.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/github-package.test.js b/test/github-package.test.js index a2307f873f..676b53ca92 100644 --- a/test/github-package.test.js +++ b/test/github-package.test.js @@ -169,7 +169,7 @@ describe('GithubPackage', () => { }) describe('when the active item is a FilePatchController', () => { - xit('updates the active repository to be the one associated with the FilePatchController', async () => { + it('updates the active repository to be the one associated with the FilePatchController', async () => { const workdirPath = await cloneRepository('three-files') fs.writeFileSync(path.join(workdirPath, 'a.txt'), 'change', 'utf8') @@ -214,7 +214,7 @@ describe('GithubPackage', () => { }) describe('when a FilePatch is selected in the staging panel', () => { - xit('shows a FilePatchView for the selected patch as a pane item, always updating the existing pane item', async () => { + it('shows a FilePatchView for the selected patch as a pane item, always updating the existing pane item', async () => { const workdirPath = await cloneRepository('three-files') const repository = await buildRepository(workdirPath) @@ -259,7 +259,7 @@ describe('GithubPackage', () => { }) describe('when amend mode is toggled in the staging panel while viewing a staged change', () => { - xit('closes the file patch pane item', async () => { + it('closes the file patch pane item', async () => { const workdirPath = await cloneRepository('three-files') const repository = await buildRepository(workdirPath) fs.writeFileSync(path.join(workdirPath, 'a.txt'), 'change', 'utf8') From 6449d94aceac70588c7ddce1a49f38d94c5dbebd Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Fri, 18 Nov 2016 15:22:50 -0800 Subject: [PATCH 34/49] :art: GithubPackage#didSelectFilePatch --- lib/github-package.js | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/lib/github-package.js b/lib/github-package.js index 384eb78f0a..3ae28fa649 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -85,24 +85,25 @@ export default class GithubPackage { async didSelectFilePatch (filePath, stagingStatus, {focus, amending} = {}) { const repository = this.getActiveRepository() const filePatch = await repository.getFilePatchForPath(filePath, {staged: stagingStatus === 'staged', amending}) - let containingPane - const update = this.didSelectFilePatch.bind(this, filePath, stagingStatus) - if (this.filePatchController) { - if (!filePatch) { - this.filePatchController.destroy() - this.filePatchController = null - return - } else { + if (filePatch) { + let containingPane + const update = this.didSelectFilePatch.bind(this, filePath, stagingStatus) + if (this.filePatchController) { this.filePatchController.update({filePatch, repository, stagingStatus, update}) containingPane = this.workspace.paneForItem(this.filePatchController) + } else { + this.filePatchController = new FilePatchController({filePatch, repository, stagingStatus, update}) + this.filePatchController.onDidDestroy(() => { this.filePatchController = null }) + containingPane = this.workspace.getActivePane() } + containingPane.activateItem(this.filePatchController) + if (focus) containingPane.activate() } else { - this.filePatchController = new FilePatchController({filePatch, repository, stagingStatus, update}) - this.filePatchController.onDidDestroy(() => { this.filePatchController = null }) - containingPane = this.workspace.getActivePane() + if (this.filePatchController) { + this.filePatchController.destroy() + this.filePatchController = null + } } - containingPane.activateItem(this.filePatchController) - if (focus) containingPane.activate() } async didSelectMergeConflictFile (relativeFilePath, {focus} = {}) { From 4cee023cfe56590a254ac41988e018aa4e46e02e Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Fri, 18 Nov 2016 15:29:21 -0800 Subject: [PATCH 35/49] Improve test for cached FilePatch objects --- test/models/repository.test.js | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/test/models/repository.test.js b/test/models/repository.test.js index 4984261d82..ca1ffaef6f 100644 --- a/test/models/repository.test.js +++ b/test/models/repository.test.js @@ -131,15 +131,18 @@ describe('Repository', function () { describe('getFilePatchForPath', () => { it('returns cached FilePatch objects if they exist', async () => { - const workingDirPath = await cloneRepository('three-files') + const workingDirPath = await cloneRepository('multiple-commits') const repo = await buildRepository(workingDirPath) - fs.writeFileSync(path.join(workingDirPath, 'a.txt'), 'qux\nfoo\nbar\n', 'utf8') - fs.unlinkSync(path.join(workingDirPath, 'b.txt')) + fs.writeFileSync(path.join(workingDirPath, 'new-file.txt'), 'foooooo', 'utf8') + fs.writeFileSync(path.join(workingDirPath, 'file.txt'), 'qux\nfoo\nbar\n', 'utf8') + await repo.stageFiles(['file.txt']) - const filePatchA = await repo.getFilePatchForPath('a.txt') - const filePatchB = await repo.getFilePatchForPath('b.txt') - assert.equal(await repo.getFilePatchForPath('a.txt'), filePatchA) - assert.equal(await repo.getFilePatchForPath('b.txt'), filePatchB) + const unstagedFilePatch = await repo.getFilePatchForPath('new-file.txt') + const stagedFilePatch = await repo.getFilePatchForPath('file.txt', {staged: true}) + const stagedFilePatchDuringAmend = await repo.getFilePatchForPath('file.txt', {staged: true, amending: true}) + assert.equal(await repo.getFilePatchForPath('new-file.txt'), unstagedFilePatch) + assert.equal(await repo.getFilePatchForPath('file.txt', {staged: true}), stagedFilePatch) + assert.equal(await repo.getFilePatchForPath('file.txt', {staged: true, amending: true}), stagedFilePatchDuringAmend) }) it('returns new FilePatch object after repository refresh', async () => { From 4dd1bf8d730b90283936f4dde24f10ba5c8d9709 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Fri, 18 Nov 2016 20:22:41 -0800 Subject: [PATCH 36/49] Await active model update before re-rendering --- lib/controllers/file-patch-controller.js | 3 ++- lib/controllers/git-panel-controller.js | 5 +++-- lib/controllers/status-bar-tile-controller.js | 4 ++-- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/controllers/file-patch-controller.js b/lib/controllers/file-patch-controller.js index 386a6918c3..aa3d5c5772 100644 --- a/lib/controllers/file-patch-controller.js +++ b/lib/controllers/file-patch-controller.js @@ -22,9 +22,10 @@ export default class FilePatchController { etch.initialize(this) } - update (props) { + async update (props) { this.props = Object.assign({}, this.props, props) this.emitter.emit('did-change-title', this.getTitle()) + await this.repositoryObserver.setActiveModel(props.repository) return etch.update(this) } diff --git a/lib/controllers/git-panel-controller.js b/lib/controllers/git-panel-controller.js index daa64f3245..030c55cb01 100644 --- a/lib/controllers/git-panel-controller.js +++ b/lib/controllers/git-panel-controller.js @@ -65,9 +65,10 @@ export default class GitPanelController { ) } - update (props) { + async update (props) { this.props = Object.assign({}, this.props, props) - return this.repositoryObserver.setActiveModel(props.repository) + await this.repositoryObserver.setActiveModel(props.repository) + return etch.update(this) } destroy () { diff --git a/lib/controllers/status-bar-tile-controller.js b/lib/controllers/status-bar-tile-controller.js index 26febbd2d8..3518315c35 100644 --- a/lib/controllers/status-bar-tile-controller.js +++ b/lib/controllers/status-bar-tile-controller.js @@ -38,9 +38,9 @@ export default class StatusBarTileController { etch.initialize(this) } - update (props) { + async update (props) { this.props = Object.assign({}, this.props, props) - this.repositoryObserver.setActiveModel(props.repository) + await this.repositoryObserver.setActiveModel(props.repository) return etch.update(this) } From f6a787cc9ec4185888c94c1cd1d15fe68b15dcba Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Sat, 19 Nov 2016 20:50:30 -0800 Subject: [PATCH 37/49] Fix test --- test/github-package.test.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/github-package.test.js b/test/github-package.test.js index 676b53ca92..a7e143496d 100644 --- a/test/github-package.test.js +++ b/test/github-package.test.js @@ -284,8 +284,6 @@ describe('GithubPackage', () => { await workspace.open(path.join(workdirPath, 'a.txt')) await githubPackage.updateActiveRepository() - await etch.getScheduler().getNextUpdatePromise() - githubPackage.statusBarTileController.refs.changedFilesCountView.props.didClick() assert.equal(workspace.getRightPanels().length, 1) From b5299a04baae1f9f0a71315dcc95979bac1458f9 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Sat, 19 Nov 2016 21:04:25 -0800 Subject: [PATCH 38/49] Only activate FilePatchView if `activate` option is passed to `didSelectFilePatch` --- lib/github-package.js | 7 ++++--- lib/views/staging-view.js | 2 +- test/github-package.test.js | 8 ++++---- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/github-package.js b/lib/github-package.js index 3ae28fa649..16097ec38d 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -82,7 +82,7 @@ export default class GithubPackage { } } - async didSelectFilePatch (filePath, stagingStatus, {focus, amending} = {}) { + async didSelectFilePatch (filePath, stagingStatus, {activate, amending} = {}) { const repository = this.getActiveRepository() const filePatch = await repository.getFilePatchForPath(filePath, {staged: stagingStatus === 'staged', amending}) if (filePatch) { @@ -96,8 +96,9 @@ export default class GithubPackage { this.filePatchController.onDidDestroy(() => { this.filePatchController = null }) containingPane = this.workspace.getActivePane() } - containingPane.activateItem(this.filePatchController) - if (focus) containingPane.activate() + if (activate) { + containingPane.activateItem(this.filePatchController) + } } else { if (this.filePatchController) { this.filePatchController.destroy() diff --git a/lib/views/staging-view.js b/lib/views/staging-view.js index 176aedfb64..35f9327eef 100644 --- a/lib/views/staging-view.js +++ b/lib/views/staging-view.js @@ -129,7 +129,7 @@ export default class StagingView { } else { if (this.props.didSelectFilePatch) { const amending = this.props.isAmending && this.selection.getActiveListKey() === 'staged' - this.props.didSelectFilePatch(selectedItem.filePath, this.selection.getActiveListKey(), {amending}) + this.props.didSelectFilePatch(selectedItem.filePath, this.selection.getActiveListKey(), {amending, activate: true}) } } } diff --git a/test/github-package.test.js b/test/github-package.test.js index a7e143496d..ea6f08e1ef 100644 --- a/test/github-package.test.js +++ b/test/github-package.test.js @@ -180,7 +180,7 @@ describe('GithubPackage', () => { await githubPackage.updateActiveRepository() assert.equal(githubPackage.getActiveRepository(), repository) - await githubPackage.gitPanelController.props.didSelectFilePatch('a.txt', 'unstaged') + await githubPackage.gitPanelController.props.didSelectFilePatch('a.txt', 'unstaged', {activate: true}) assert.equal(workspace.getActivePaneItem(), githubPackage.filePatchController) assert.equal(githubPackage.filePatchController.props.repository, repository) await githubPackage.updateActiveRepository() @@ -226,7 +226,7 @@ describe('GithubPackage', () => { assert.isNull(githubPackage.filePatchController) - await githubPackage.gitPanelController.props.didSelectFilePatch('a.txt', 'unstaged') + await githubPackage.gitPanelController.props.didSelectFilePatch('a.txt', 'unstaged', {activate: true}) assert(githubPackage.filePatchController) assert.equal(githubPackage.filePatchController.props.filePatch.getPath(), 'a.txt') assert.equal(githubPackage.filePatchController.props.repository, repository) @@ -249,7 +249,7 @@ describe('GithubPackage', () => { assert.isUndefined(workspace.getActivePaneItem()) assert.isNull(githubPackage.filePatchController) - await githubPackage.gitPanelController.props.didSelectFilePatch('d.txt', 'staged') + await githubPackage.gitPanelController.props.didSelectFilePatch('d.txt', 'staged', {activate: true}) assert.notEqual(githubPackage.filePatchController, existingFilePatchView) assert.equal(githubPackage.filePatchController.props.filePatch.getPath(), 'd.txt') assert.equal(githubPackage.filePatchController.props.repository, repository) @@ -267,7 +267,7 @@ describe('GithubPackage', () => { githubPackage.getActiveRepository = function () { return repository } - await githubPackage.gitPanelController.props.didSelectFilePatch('a.txt', 'staged') + await githubPackage.gitPanelController.props.didSelectFilePatch('a.txt', 'staged', {activate: true}) assert.isOk(githubPackage.filePatchController) assert.equal(workspace.getActivePaneItem(), githubPackage.filePatchController) From b96204c190a6651abcc2c4888530c57634a57928 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Sun, 20 Nov 2016 18:37:00 -0800 Subject: [PATCH 39/49] Focus FilePatchView on left arrow / "git:focus-diff-view" command --- lib/controllers/git-panel-controller.js | 1 + lib/github-package.js | 7 ++++++- lib/views/git-panel-view.js | 1 + lib/views/staging-view.js | 2 +- 4 files changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/controllers/git-panel-controller.js b/lib/controllers/git-panel-controller.js index cd59c6ec45..ce0f47b29e 100644 --- a/lib/controllers/git-panel-controller.js +++ b/lib/controllers/git-panel-controller.js @@ -48,6 +48,7 @@ export default class GitPanelController { notificationManager={this.props.notificationManager} didSelectFilePatch={this.props.didSelectFilePatch} didSelectMergeConflictFile={this.props.didSelectMergeConflictFile} + focusFilePatchView={this.props.focusFilePatchView} stageFilePatch={this.stageFilePatch} unstageFilePatch={this.unstageFilePatch} stageFiles={this.stageFiles} diff --git a/lib/github-package.js b/lib/github-package.js index 16097ec38d..e9ef768bb7 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -29,7 +29,8 @@ export default class GithubPackage { notificationManager, didSelectFilePatch: this.didSelectFilePatch.bind(this), didSelectMergeConflictFile: this.didSelectMergeConflictFile.bind(this), - didChangeAmending: this.didChangeAmending.bind(this) + didChangeAmending: this.didChangeAmending.bind(this), + focusFilePatchView: this.focusFilePatchView.bind(this) }) this.statusBarTileController = new StatusBarTileController({ workspace, @@ -107,6 +108,10 @@ export default class GithubPackage { } } + focusFilePatchView () { + this.filePatchController.refs.filePatchView.element.focus() + } + async didSelectMergeConflictFile (relativeFilePath, {focus} = {}) { const absolutePath = path.join(this.getActiveRepository().getWorkingDirectoryPath(), relativeFilePath) if (await new File(absolutePath).exists()) { diff --git a/lib/views/git-panel-view.js b/lib/views/git-panel-view.js index 0c6c57ce89..94b1ac8c70 100644 --- a/lib/views/git-panel-view.js +++ b/lib/views/git-panel-view.js @@ -41,6 +41,7 @@ export default class GitPanelView { mergeConflicts={this.props.mergeConflicts} didSelectFilePatch={this.props.didSelectFilePatch} didSelectMergeConflictFile={this.props.didSelectMergeConflictFile} + focusFilePatchView={this.props.focusFilePatchView} stageFiles={this.props.stageFiles} unstageFiles={this.props.unstageFiles} lastCommit={this.props.lastCommit} diff --git a/lib/views/staging-view.js b/lib/views/staging-view.js index 35f9327eef..a82a95ff9c 100644 --- a/lib/views/staging-view.js +++ b/lib/views/staging-view.js @@ -43,7 +43,7 @@ export default class StagingView { 'core:confirm': () => this.confirmSelectedItems(), 'git:activate-next-list': () => this.activateNextList(), 'git:activate-previous-list': () => this.activatePreviousList(), - 'git:focus-diff-view': () => this.focusFilePatchView() + 'git:focus-diff-view': () => this.props.focusFilePatchView() })) window.addEventListener('mouseup', this.mouseup) this.subscriptions.add(new Disposable(() => window.removeEventListener('mouseup', this.mouseup))) From 99af683c67cce87558aaeac1e5399d94e2a2d5e5 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Sun, 20 Nov 2016 19:00:37 -0800 Subject: [PATCH 40/49] Correct test description --- test/git-shell-out-strategy.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/git-shell-out-strategy.test.js b/test/git-shell-out-strategy.test.js index 26a6a68ead..d4404e8719 100644 --- a/test/git-shell-out-strategy.test.js +++ b/test/git-shell-out-strategy.test.js @@ -150,7 +150,7 @@ describe('Git commands', () => { }) describe('diffFileStatus', () => { - it('returns an object with working directory file diff status between relative to HEAD', async () => { + it('returns an object with working directory file diff status between relative to specified target commit', async () => { const workingDirPath = await cloneRepository('three-files') const git = new GitShellOutStrategy(workingDirPath) fs.writeFileSync(path.join(workingDirPath, 'a.txt'), 'qux\nfoo\nbar\n', 'utf8') From 291ac08d6dcb2db7b9f0d467e0d4372aeff4a95b Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Sun, 20 Nov 2016 19:02:38 -0800 Subject: [PATCH 41/49] :art: rearrange methods in Repository --- lib/models/repository.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/models/repository.js b/lib/models/repository.js index 3b31f13b43..e0e12609d1 100644 --- a/lib/models/repository.js +++ b/lib/models/repository.js @@ -118,13 +118,6 @@ export default class Repository { return Object.keys(stagedFiles).map(filePath => { return {filePath, status: stagedFiles[filePath]} }) } - getStagedChangesSinceParentCommit () { - if (!this.stagedChangesSinceParentCommitPromise) { - this.stagedChangesSinceParentCommitPromise = this.fetchStagedChangesSinceParentCommit() - } - return this.stagedChangesSinceParentCommitPromise - } - async getMergeConflicts () { const {mergeConflictFiles} = await this.getStatusesForChangedFiles() return Object.keys(mergeConflictFiles).map(filePath => { return {filePath, status: mergeConflictFiles[filePath]} }) @@ -143,6 +136,13 @@ export default class Repository { } } + getStagedChangesSinceParentCommit () { + if (!this.stagedChangesSinceParentCommitPromise) { + this.stagedChangesSinceParentCommitPromise = this.fetchStagedChangesSinceParentCommit() + } + return this.stagedChangesSinceParentCommitPromise + } + clearCachedFilePatches () { this.stagedFilePatchesByPath = new Map() this.stagedFilePatchesSinceParentCommitByPath = new Map() From fa70e74e33a17f2628b49089514f750fcc3e3053 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Sun, 20 Nov 2016 19:11:00 -0800 Subject: [PATCH 42/49] Fix :bug: in GSOS#diffFileStatus - only list new files if no staged opt --- lib/git-shell-out-strategy.js | 6 ++++-- test/git-shell-out-strategy.test.js | 10 ++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index b50ec4243c..7176a39ba7 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -182,7 +182,6 @@ export default class GitShellOutStrategy { if (options.target) args.push(options.target) if (options.diffFilter === 'unmerged') args.push('--diff-filter=U') const output = await this.exec(args) - const untracked = await this.getUntrackedFiles() const statusMap = { A: 'added', @@ -192,11 +191,14 @@ export default class GitShellOutStrategy { } const fileStatuses = {} - untracked.forEach(filePath => { fileStatuses[filePath] = 'added' }) output && output.trim().split(LINE_ENDING_REGEX).forEach(line => { const [status, filePath] = line.split(' ') fileStatuses[filePath] = statusMap[status] }) + if (!options.staged) { + const untracked = await this.getUntrackedFiles() + untracked.forEach(filePath => { fileStatuses[filePath] = 'added' }) + } return fileStatuses } diff --git a/test/git-shell-out-strategy.test.js b/test/git-shell-out-strategy.test.js index d4404e8719..59819a0cae 100644 --- a/test/git-shell-out-strategy.test.js +++ b/test/git-shell-out-strategy.test.js @@ -173,6 +173,16 @@ describe('Git commands', () => { const diffOutput = await git.diffFileStatus({ target: 'HEAD' }) assert.deepEqual(diffOutput, {}) }) + + it('only returns untracked files if the staged option is not passed', async () => { + const workingDirPath = await cloneRepository('three-files') + const git = new GitShellOutStrategy(workingDirPath) + fs.writeFileSync(path.join(workingDirPath, 'new-file.txt'), 'qux', 'utf8') + let diffOutput = await git.diffFileStatus({ target: 'HEAD'}) + assert.deepEqual(diffOutput, { 'new-file.txt': 'added' }) + diffOutput = await git.diffFileStatus({ target: 'HEAD', staged: true}) + assert.deepEqual(diffOutput, {}) + }) }) describe('getUntrackedFiles', () => { From bb9326ccfe2941e23ba7cc24f5a785a701297a61 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Sun, 20 Nov 2016 19:39:04 -0800 Subject: [PATCH 43/49] Preserve amend status when update happens after repo refresh --- lib/github-package.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/github-package.js b/lib/github-package.js index e9ef768bb7..42fadcac36 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -88,7 +88,7 @@ export default class GithubPackage { const filePatch = await repository.getFilePatchForPath(filePath, {staged: stagingStatus === 'staged', amending}) if (filePatch) { let containingPane - const update = this.didSelectFilePatch.bind(this, filePath, stagingStatus) + const update = this.didSelectFilePatch.bind(this, filePath, stagingStatus, {amending}) if (this.filePatchController) { this.filePatchController.update({filePatch, repository, stagingStatus, update}) containingPane = this.workspace.paneForItem(this.filePatchController) From 61ffd8b0cc6f784124924ce3b8410e3a85cf6926 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 21 Nov 2016 16:35:05 -0800 Subject: [PATCH 44/49] Rename didSelectFilePatch -> didSelectFilePath --- lib/controllers/git-panel-controller.js | 2 +- lib/github-package.js | 6 +++--- lib/views/git-panel-view.js | 2 +- lib/views/staging-view.js | 4 ++-- test/github-package.test.js | 10 +++++----- test/views/staging-view.test.js | 12 ++++++------ 6 files changed, 18 insertions(+), 18 deletions(-) diff --git a/lib/controllers/git-panel-controller.js b/lib/controllers/git-panel-controller.js index ce0f47b29e..2cb34cb928 100644 --- a/lib/controllers/git-panel-controller.js +++ b/lib/controllers/git-panel-controller.js @@ -46,7 +46,7 @@ export default class GitPanelController { workspace={this.props.workspace} commandRegistry={this.props.commandRegistry} notificationManager={this.props.notificationManager} - didSelectFilePatch={this.props.didSelectFilePatch} + didSelectFilePath={this.props.didSelectFilePath} didSelectMergeConflictFile={this.props.didSelectMergeConflictFile} focusFilePatchView={this.props.focusFilePatchView} stageFilePatch={this.stageFilePatch} diff --git a/lib/github-package.js b/lib/github-package.js index 42fadcac36..923e3ce069 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -27,7 +27,7 @@ export default class GithubPackage { workspace, commandRegistry, notificationManager, - didSelectFilePatch: this.didSelectFilePatch.bind(this), + didSelectFilePath: this.didSelectFilePath.bind(this), didSelectMergeConflictFile: this.didSelectMergeConflictFile.bind(this), didChangeAmending: this.didChangeAmending.bind(this), focusFilePatchView: this.focusFilePatchView.bind(this) @@ -83,12 +83,12 @@ export default class GithubPackage { } } - async didSelectFilePatch (filePath, stagingStatus, {activate, amending} = {}) { + async didSelectFilePath (filePath, stagingStatus, {activate, amending} = {}) { const repository = this.getActiveRepository() const filePatch = await repository.getFilePatchForPath(filePath, {staged: stagingStatus === 'staged', amending}) if (filePatch) { let containingPane - const update = this.didSelectFilePatch.bind(this, filePath, stagingStatus, {amending}) + const update = this.didSelectFilePath.bind(this, filePath, stagingStatus, {amending}) if (this.filePatchController) { this.filePatchController.update({filePatch, repository, stagingStatus, update}) containingPane = this.workspace.paneForItem(this.filePatchController) diff --git a/lib/views/git-panel-view.js b/lib/views/git-panel-view.js index 94b1ac8c70..27e595a004 100644 --- a/lib/views/git-panel-view.js +++ b/lib/views/git-panel-view.js @@ -39,7 +39,7 @@ export default class GitPanelView { stagedChanges={this.props.stagedChanges} unstagedChanges={this.props.unstagedChanges} mergeConflicts={this.props.mergeConflicts} - didSelectFilePatch={this.props.didSelectFilePatch} + didSelectFilePath={this.props.didSelectFilePath} didSelectMergeConflictFile={this.props.didSelectMergeConflictFile} focusFilePatchView={this.props.focusFilePatchView} stageFiles={this.props.stageFiles} diff --git a/lib/views/staging-view.js b/lib/views/staging-view.js index a82a95ff9c..ca0576da80 100644 --- a/lib/views/staging-view.js +++ b/lib/views/staging-view.js @@ -127,9 +127,9 @@ export default class StagingView { this.props.didSelectMergeConflictFile(selectedItem.filePath) } } else { - if (this.props.didSelectFilePatch) { + if (this.props.didSelectFilePath) { const amending = this.props.isAmending && this.selection.getActiveListKey() === 'staged' - this.props.didSelectFilePatch(selectedItem.filePath, this.selection.getActiveListKey(), {amending, activate: true}) + this.props.didSelectFilePath(selectedItem.filePath, this.selection.getActiveListKey(), {amending, activate: true}) } } } diff --git a/test/github-package.test.js b/test/github-package.test.js index ea6f08e1ef..6a2adbc256 100644 --- a/test/github-package.test.js +++ b/test/github-package.test.js @@ -180,7 +180,7 @@ describe('GithubPackage', () => { await githubPackage.updateActiveRepository() assert.equal(githubPackage.getActiveRepository(), repository) - await githubPackage.gitPanelController.props.didSelectFilePatch('a.txt', 'unstaged', {activate: true}) + await githubPackage.gitPanelController.props.didSelectFilePath('a.txt', 'unstaged', {activate: true}) assert.equal(workspace.getActivePaneItem(), githubPackage.filePatchController) assert.equal(githubPackage.filePatchController.props.repository, repository) await githubPackage.updateActiveRepository() @@ -226,7 +226,7 @@ describe('GithubPackage', () => { assert.isNull(githubPackage.filePatchController) - await githubPackage.gitPanelController.props.didSelectFilePatch('a.txt', 'unstaged', {activate: true}) + await githubPackage.gitPanelController.props.didSelectFilePath('a.txt', 'unstaged', {activate: true}) assert(githubPackage.filePatchController) assert.equal(githubPackage.filePatchController.props.filePatch.getPath(), 'a.txt') assert.equal(githubPackage.filePatchController.props.repository, repository) @@ -238,7 +238,7 @@ describe('GithubPackage', () => { originalPane.splitRight() // activate a different pane assert.isUndefined(workspace.getActivePaneItem()) - await githubPackage.gitPanelController.props.didSelectFilePatch('d.txt', 'staged') + await githubPackage.gitPanelController.props.didSelectFilePath('d.txt', 'staged') assert.equal(githubPackage.filePatchController, existingFilePatchView) assert.equal(githubPackage.filePatchController.props.filePatch.getPath(), 'd.txt') assert.equal(githubPackage.filePatchController.props.repository, repository) @@ -249,7 +249,7 @@ describe('GithubPackage', () => { assert.isUndefined(workspace.getActivePaneItem()) assert.isNull(githubPackage.filePatchController) - await githubPackage.gitPanelController.props.didSelectFilePatch('d.txt', 'staged', {activate: true}) + await githubPackage.gitPanelController.props.didSelectFilePath('d.txt', 'staged', {activate: true}) assert.notEqual(githubPackage.filePatchController, existingFilePatchView) assert.equal(githubPackage.filePatchController.props.filePatch.getPath(), 'd.txt') assert.equal(githubPackage.filePatchController.props.repository, repository) @@ -267,7 +267,7 @@ describe('GithubPackage', () => { githubPackage.getActiveRepository = function () { return repository } - await githubPackage.gitPanelController.props.didSelectFilePatch('a.txt', 'staged', {activate: true}) + await githubPackage.gitPanelController.props.didSelectFilePath('a.txt', 'staged', {activate: true}) assert.isOk(githubPackage.filePatchController) assert.equal(workspace.getActivePaneItem(), githubPackage.filePatchController) diff --git a/test/views/staging-view.test.js b/test/views/staging-view.test.js index f4c06b30a0..66c0bc6544 100644 --- a/test/views/staging-view.test.js +++ b/test/views/staging-view.test.js @@ -82,26 +82,26 @@ describe('StagingView', () => { } }] - const didSelectFilePatch = sinon.spy() + const didSelectFilePath = sinon.spy() const didSelectMergeConflictFile = sinon.spy() const view = new StagingView({ - didSelectFilePatch, didSelectMergeConflictFile, + didSelectFilePath, didSelectMergeConflictFile, unstagedChanges: filePatches, mergeConflicts, stagedChanges: [] }) document.body.appendChild(view.element) - assert.equal(didSelectFilePatch.callCount, 0) + assert.equal(didSelectFilePath.callCount, 0) view.focus() - assert.isTrue(didSelectFilePatch.calledWith(filePatches[0].filePath)) + assert.isTrue(didSelectFilePath.calledWith(filePatches[0].filePath)) await view.selectNext() - assert.isTrue(didSelectFilePatch.calledWith(filePatches[1].filePath)) + assert.isTrue(didSelectFilePath.calledWith(filePatches[1].filePath)) await view.selectNext() assert.isTrue(didSelectMergeConflictFile.calledWith(mergeConflicts[0].filePath)) document.body.focus() assert.isFalse(view.isFocused()) - didSelectFilePatch.reset() + didSelectFilePath.reset() didSelectMergeConflictFile.reset() await view.selectNext() assert.equal(didSelectMergeConflictFile.callCount, 0) From e1b024a4efd45ec3d231a789744786f37bf42a62 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 21 Nov 2016 16:43:32 -0800 Subject: [PATCH 45/49] :art: rename top-level methods for clarity --- lib/controllers/file-patch-controller.js | 2 +- lib/github-package.js | 16 +++++++++------- test/github-package.test.js | 2 +- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/lib/controllers/file-patch-controller.js b/lib/controllers/file-patch-controller.js index aa3d5c5772..f034bde207 100644 --- a/lib/controllers/file-patch-controller.js +++ b/lib/controllers/file-patch-controller.js @@ -16,7 +16,7 @@ export default class FilePatchController { this.stageLines = this.stageLines.bind(this) this.unstageLines = this.unstageLines.bind(this) this.repositoryObserver = new ModelObserver({ - didUpdate: () => this.props.update && this.props.update() + didUpdate: () => this.props.onRepoRefresh && this.props.onRepoRefresh() }) this.repositoryObserver.setActiveModel(props.repository) etch.initialize(this) diff --git a/lib/github-package.js b/lib/github-package.js index 923e3ce069..47324fb6d8 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -27,8 +27,8 @@ export default class GithubPackage { workspace, commandRegistry, notificationManager, - didSelectFilePath: this.didSelectFilePath.bind(this), - didSelectMergeConflictFile: this.didSelectMergeConflictFile.bind(this), + didSelectFilePath: this.showFilePatchForPath.bind(this), + didSelectMergeConflictFile: this.showMergeConflictFileForPath.bind(this), didChangeAmending: this.didChangeAmending.bind(this), focusFilePatchView: this.focusFilePatchView.bind(this) }) @@ -83,17 +83,19 @@ export default class GithubPackage { } } - async didSelectFilePath (filePath, stagingStatus, {activate, amending} = {}) { + async showFilePatchForPath (filePath, stagingStatus, {activate, amending} = {}) { const repository = this.getActiveRepository() const filePatch = await repository.getFilePatchForPath(filePath, {staged: stagingStatus === 'staged', amending}) if (filePatch) { let containingPane - const update = this.didSelectFilePath.bind(this, filePath, stagingStatus, {amending}) + const onRepoRefresh = () => { + this.showFilePatchForPath(filePath, stagingStatus, {amending}) + } if (this.filePatchController) { - this.filePatchController.update({filePatch, repository, stagingStatus, update}) + this.filePatchController.update({filePatch, repository, stagingStatus, onRepoRefresh}) containingPane = this.workspace.paneForItem(this.filePatchController) } else { - this.filePatchController = new FilePatchController({filePatch, repository, stagingStatus, update}) + this.filePatchController = new FilePatchController({filePatch, repository, stagingStatus, onRepoRefresh}) this.filePatchController.onDidDestroy(() => { this.filePatchController = null }) containingPane = this.workspace.getActivePane() } @@ -112,7 +114,7 @@ export default class GithubPackage { this.filePatchController.refs.filePatchView.element.focus() } - async didSelectMergeConflictFile (relativeFilePath, {focus} = {}) { + async showMergeConflictFileForPath (relativeFilePath, {focus} = {}) { const absolutePath = path.join(this.getActiveRepository().getWorkingDirectoryPath(), relativeFilePath) if (await new File(absolutePath).exists()) { return this.workspace.open(absolutePath, {activatePane: Boolean(focus)}) diff --git a/test/github-package.test.js b/test/github-package.test.js index 6a2adbc256..f897462e66 100644 --- a/test/github-package.test.js +++ b/test/github-package.test.js @@ -189,7 +189,7 @@ describe('GithubPackage', () => { }) }) - describe('didSelectMergeConflictFile(filePath)', () => { + describe('showMergeConflictFileForPath(filePath)', () => { it('opens the file as a pane item if it exsits', async () => { const workdirPath = await cloneRepository('merge-conflict') const repository = await buildRepository(workdirPath) From b6a9a49a79049975b684c7357275ca73578d1143 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 22 Nov 2016 14:11:00 -0800 Subject: [PATCH 46/49] :art: reportSelectedItem -> onDidChangeSelectedItem --- lib/views/staging-view.js | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/views/staging-view.js b/lib/views/staging-view.js index ca0576da80..875ea9b13d 100644 --- a/lib/views/staging-view.js +++ b/lib/views/staging-view.js @@ -26,7 +26,7 @@ export default class StagingView { }, idForItem: item => item.filePath }) - this.reportSelectedItem() + this.onDidChangeSelectedItem() etch.initialize(this) this.subscriptions = new CompositeDisposable() @@ -81,35 +81,35 @@ export default class StagingView { selectPrevious (preserveTail = false) { this.selection.selectPreviousItem(preserveTail) this.selection.coalesce() - this.reportSelectedItem() + this.onDidChangeSelectedItem() return etch.update(this) } selectNext (preserveTail = false) { this.selection.selectNextItem(preserveTail) this.selection.coalesce() - this.reportSelectedItem() + this.onDidChangeSelectedItem() return etch.update(this) } selectAll () { this.selection.selectAllItems() this.selection.coalesce() - this.reportSelectedItem() + this.onDidChangeSelectedItem() return etch.update(this) } selectFirst (preserveTail = false) { this.selection.selectFirstItem(preserveTail) this.selection.coalesce() - this.reportSelectedItem() + this.onDidChangeSelectedItem() return etch.update(this) } selectLast (preserveTail = false) { this.selection.selectLastItem(preserveTail) this.selection.coalesce() - this.reportSelectedItem() + this.onDidChangeSelectedItem() return etch.update(this) } @@ -118,7 +118,7 @@ export default class StagingView { if (headItem) this.listElementsByItem.get(headItem).scrollIntoViewIfNeeded() } - reportSelectedItem () { + onDidChangeSelectedItem () { const selectedItem = this.selection.getHeadItem() if (!this.isFocused() || !selectedItem) return @@ -164,7 +164,7 @@ export default class StagingView { mouseup () { this.mouseSelectionInProgress = false this.selection.coalesce() - this.reportSelectedItem() + this.onDidChangeSelectedItem() } render () { @@ -269,7 +269,7 @@ export default class StagingView { focus () { this.element.focus() - this.reportSelectedItem() + this.onDidChangeSelectedItem() } isFocused () { From 432000a98c253295f5abeaeab73754435b69a6e3 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 22 Nov 2016 14:13:56 -0800 Subject: [PATCH 47/49] Extract logic into method selectFirstNonEmptyList for clarity --- lib/views/composite-list-selection.js | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/lib/views/composite-list-selection.js b/lib/views/composite-list-selection.js index 6117e064ed..0d4325bf94 100644 --- a/lib/views/composite-list-selection.js +++ b/lib/views/composite-list-selection.js @@ -14,13 +14,7 @@ export default class CompositeListSelection { this.selections.push(selection) } - this.activeSelectionIndex = 0 - for (var i = 0; i < this.selections.length; i++) { - if (this.selections[i].getItems().length) { - this.activeSelectionIndex = i - break - } - } + this.selectFirstNonEmptyList() } updateLists (listsByKey) { @@ -38,6 +32,16 @@ export default class CompositeListSelection { } } + selectFirstNonEmptyList () { + this.activeSelectionIndex = 0 + for (var i = 0; i < this.selections.length; i++) { + if (this.selections[i].getItems().length) { + this.activeSelectionIndex = i + break + } + } + } + getActiveListKey () { return this.keysBySelection.get(this.getActiveSelection()) } From 3258d79b488159a06bd7c9fd8bff0e5b15594431 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 22 Nov 2016 14:46:59 -0800 Subject: [PATCH 48/49] :art: GSOS#diff -> GSOS#getDiffForFilePath --- lib/git-shell-out-strategy.js | 27 +-- lib/models/repository.js | 3 +- test/git-shell-out-strategy.test.js | 318 ++++++++-------------------- 3 files changed, 100 insertions(+), 248 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index 7176a39ba7..767cdb80d2 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -208,32 +208,25 @@ export default class GitShellOutStrategy { return output.trim().split(LINE_ENDING_REGEX) } - async diff ({filePath, staged, baseCommit} = {}) { + async getDiffForFilePath (filePath, {staged, baseCommit} = {}) { let args = ['diff', '--no-prefix', '--no-renames', '--diff-filter=u'] if (staged) args.push('--staged') if (baseCommit) args.push(baseCommit) - if (filePath) args = args.concat(['--', filePath]) + args = args.concat(['--', filePath]) let output = await this.exec(args) let rawDiffs = [] if (output) rawDiffs = parseDiff(output).filter(rawDiff => rawDiff.status !== 'unmerged') - if (!staged) { - // add untracked files - let untrackedFiles = await this.getUntrackedFiles() - if (filePath) { - untrackedFiles = untrackedFiles.includes(filePath) ? [filePath] : [] - } - const untrackedFilePatches = await Promise.all(untrackedFiles.map(async (filePath) => { - const absPath = path.join(this.workingDir, filePath) - const stats = await fsStat(absPath) - const contents = await readFile(absPath) - return buildAddedFilePatch(filePath, contents, stats) - })) - rawDiffs = rawDiffs.concat(untrackedFilePatches) + if (!staged && (await this.getUntrackedFiles()).includes(filePath)) { + // add untracked file + const absPath = path.join(this.workingDir, filePath) + const stats = await fsStat(absPath) + const contents = await readFile(absPath) + rawDiffs.push(buildAddedFilePatch(filePath, contents, stats)) } - if (filePath) return rawDiffs[0] - return rawDiffs.sort((a, b) => (a.newPath || a.oldPath).localeCompare(b.newPath || b.oldPath)) + if (rawDiffs.length > 1) throw new Error(`Expected 0 or 1 diffs for ${filePath} but got ${rawDiffs.length}`) + return rawDiffs[0] } /** diff --git a/lib/models/repository.js b/lib/models/repository.js index e0e12609d1..cc9b5b63c7 100644 --- a/lib/models/repository.js +++ b/lib/models/repository.js @@ -174,9 +174,8 @@ export default class Repository { const cachedFilePatch = this.getCachedFilePatchForPath(filePath, options) if (cachedFilePatch) return cachedFilePatch - options.filePath = filePath if (options.amending) options.baseCommit = 'HEAD~' - const rawDiff = await this.git.diff(options) + const rawDiff = await this.git.getDiffForFilePath(filePath, options) if (rawDiff) { const [filePatch] = this.buildFilePatchesFromRawDiffs([rawDiff]) this.cacheFilePatchForPath(filePath, filePatch, options) diff --git a/test/git-shell-out-strategy.test.js b/test/git-shell-out-strategy.test.js index 59819a0cae..8695c756f0 100644 --- a/test/git-shell-out-strategy.test.js +++ b/test/git-shell-out-strategy.test.js @@ -25,7 +25,7 @@ describe('Git commands', () => { let promises = [] for (let i = 0; i < 10; i++) { expectedEvents.push(i) - promises.push(git.diff().then(() => actualEvents.push(i))) + promises.push(git.getHeadCommit().then(() => actualEvents.push(i))) } await Promise.all(promises) @@ -217,49 +217,30 @@ describe('Git commands', () => { }) }) - describe('diff', () => { + describe('getDiffForFilePath', () => { it('returns an empty array if there are no modified, added, or deleted files', async () => { const workingDirPath = await cloneRepository('three-files') const git = new GitShellOutStrategy(workingDirPath) - const diffOutput = await git.diff() - assert.deepEqual(diffOutput, []) + const diffOutput = await git.getDiffForFilePath('a.txt') + assert.isUndefined(diffOutput) }) - it('returns an array of objects for each file patch', async () => { - const workingDirPath = await cloneRepository('three-files') + it('ignores merge conflict files', async () => { + const workingDirPath = await cloneRepository('merge-conflict') const git = new GitShellOutStrategy(workingDirPath) + const diffOutput = await git.getDiffForFilePath('added-to-both.txt') + assert.isUndefined(diffOutput) + }) - fs.writeFileSync(path.join(workingDirPath, 'a.txt'), 'qux\nfoo\nbar\n', 'utf8') - fs.unlinkSync(path.join(workingDirPath, 'b.txt')) - fs.renameSync(path.join(workingDirPath, 'c.txt'), path.join(workingDirPath, 'd.txt')) - fs.writeFileSync(path.join(workingDirPath, 'e.txt'), 'qux', 'utf8') - fs.writeFileSync(path.join(workingDirPath, 'f.txt'), 'cat', 'utf8') - - await git.stageFiles(['f.txt']) - fs.unlinkSync(path.join(workingDirPath, 'f.txt')) - - const stagedDiffOutput = await git.diff({staged: true}) - assertDeepPropertyVals(stagedDiffOutput, [{ - 'oldPath': null, - 'newPath': 'f.txt', - 'oldMode': null, - 'newMode': '100644', - 'hunks': [ - { - 'oldStartLine': 0, - 'oldLineCount': 0, - 'newStartLine': 1, - 'newLineCount': 1, - 'lines': [ '+cat', '\\ No newline at end of file' ] - } - ], - 'status': 'added' - }]) - - const unstagedDiffOutput = await git.diff() - assertDeepPropertyVals(unstagedDiffOutput, [ - { + describe('when the file is unstaged', () => { + it('returns a diff comparing the working directory copy of the file and the version on the index', async () => { + const workingDirPath = await cloneRepository('three-files') + const git = new GitShellOutStrategy(workingDirPath) + fs.writeFileSync(path.join(workingDirPath, 'a.txt'), 'qux\nfoo\nbar\n', 'utf8') + fs.renameSync(path.join(workingDirPath, 'c.txt'), path.join(workingDirPath, 'd.txt')) + + assertDeepPropertyVals(await git.getDiffForFilePath('a.txt'), { 'oldPath': 'a.txt', 'newPath': 'a.txt', 'oldMode': '100644', @@ -278,26 +259,9 @@ describe('Git commands', () => { } ], 'status': 'modified' - }, - { - 'oldPath': 'b.txt', - 'newPath': null, - 'oldMode': '100644', - 'newMode': null, - 'hunks': [ - { - 'oldStartLine': 1, - 'oldLineCount': 1, - 'newStartLine': 0, - 'newLineCount': 0, - 'lines': [ - '-bar' - ] - } - ], - 'status': 'deleted' - }, - { + }) + + assertDeepPropertyVals(await git.getDiffForFilePath('c.txt'), { 'oldPath': 'c.txt', 'newPath': null, 'oldMode': '100644', @@ -312,8 +276,9 @@ describe('Git commands', () => { } ], 'status': 'deleted' - }, - { + }) + + assertDeepPropertyVals(await git.getDiffForFilePath('d.txt'), { 'oldPath': null, 'newPath': 'd.txt', 'oldMode': null, @@ -328,25 +293,41 @@ describe('Git commands', () => { } ], 'status': 'added' - }, - { - 'oldPath': null, - 'newPath': 'e.txt', - 'oldMode': null, + }) + }) + }) + + describe('when the file is staged', () => { + it('returns a diff comparing the index and head versions of the file', async () => { + const workingDirPath = await cloneRepository('three-files') + const git = new GitShellOutStrategy(workingDirPath) + fs.writeFileSync(path.join(workingDirPath, 'a.txt'), 'qux\nfoo\nbar\n', 'utf8') + fs.renameSync(path.join(workingDirPath, 'c.txt'), path.join(workingDirPath, 'd.txt')) + await git.exec(['add', '.']) + + assertDeepPropertyVals(await git.getDiffForFilePath('a.txt', {staged: true}), { + 'oldPath': 'a.txt', + 'newPath': 'a.txt', + 'oldMode': '100644', 'newMode': '100644', 'hunks': [ { - 'oldStartLine': 0, - 'oldLineCount': 0, + 'oldStartLine': 1, + 'oldLineCount': 1, 'newStartLine': 1, - 'newLineCount': 1, - 'lines': [ '+qux', '\\ No newline at end of file' ] + 'newLineCount': 3, + 'lines': [ + '+qux', + ' foo', + '+bar' + ] } ], - 'status': 'added' - }, - { - 'oldPath': 'f.txt', + 'status': 'modified' + }) + + assertDeepPropertyVals(await git.getDiffForFilePath('c.txt', {staged: true}), { + 'oldPath': 'c.txt', 'newPath': null, 'oldMode': '100644', 'newMode': null, @@ -356,172 +337,51 @@ describe('Git commands', () => { 'oldLineCount': 1, 'newStartLine': 0, 'newLineCount': 0, - 'lines': [ '-cat', '\\ No newline at end of file' ] + 'lines': [ '-baz' ] } ], 'status': 'deleted' - } - ]) - }) - - it('ignores merge conflict files', async () => { - const workingDirPath = await cloneRepository('merge-conflict') - const git = new GitShellOutStrategy(workingDirPath) - const diffOutput = await git.diff() - assert.deepEqual(diffOutput, []) - }) - - describe('when a file path is specified', () => { - describe('when the file is unstaged', () => { - it('returns a diff comparing the working directory copy of the file and the version on the index', async () => { - const workingDirPath = await cloneRepository('three-files') - const git = new GitShellOutStrategy(workingDirPath) - fs.writeFileSync(path.join(workingDirPath, 'a.txt'), 'qux\nfoo\nbar\n', 'utf8') - fs.renameSync(path.join(workingDirPath, 'c.txt'), path.join(workingDirPath, 'd.txt')) - - assertDeepPropertyVals(await git.diff({filePath: 'a.txt'}), { - 'oldPath': 'a.txt', - 'newPath': 'a.txt', - 'oldMode': '100644', - 'newMode': '100644', - 'hunks': [ - { - 'oldStartLine': 1, - 'oldLineCount': 1, - 'newStartLine': 1, - 'newLineCount': 3, - 'lines': [ - '+qux', - ' foo', - '+bar' - ] - } - ], - 'status': 'modified' - }) - - assertDeepPropertyVals(await git.diff({filePath: 'c.txt'}), { - 'oldPath': 'c.txt', - 'newPath': null, - 'oldMode': '100644', - 'newMode': null, - 'hunks': [ - { - 'oldStartLine': 1, - 'oldLineCount': 1, - 'newStartLine': 0, - 'newLineCount': 0, - 'lines': [ '-baz' ] - } - ], - 'status': 'deleted' - }) - - assertDeepPropertyVals(await git.diff({filePath: 'd.txt'}), { - 'oldPath': null, - 'newPath': 'd.txt', - 'oldMode': null, - 'newMode': '100644', - 'hunks': [ - { - 'oldStartLine': 0, - 'oldLineCount': 0, - 'newStartLine': 1, - 'newLineCount': 1, - 'lines': [ '+baz' ] - } - ], - 'status': 'added' - }) }) - }) - describe('when the file is staged', () => { - it('returns a diff comparing the index and head versions of the file', async () => { - const workingDirPath = await cloneRepository('three-files') - const git = new GitShellOutStrategy(workingDirPath) - fs.writeFileSync(path.join(workingDirPath, 'a.txt'), 'qux\nfoo\nbar\n', 'utf8') - fs.renameSync(path.join(workingDirPath, 'c.txt'), path.join(workingDirPath, 'd.txt')) - await git.exec(['add', '.']) - - assertDeepPropertyVals(await git.diff({filePath: 'a.txt', staged: true}), { - 'oldPath': 'a.txt', - 'newPath': 'a.txt', - 'oldMode': '100644', - 'newMode': '100644', - 'hunks': [ - { - 'oldStartLine': 1, - 'oldLineCount': 1, - 'newStartLine': 1, - 'newLineCount': 3, - 'lines': [ - '+qux', - ' foo', - '+bar' - ] - } - ], - 'status': 'modified' - }) - - assertDeepPropertyVals(await git.diff({filePath: 'c.txt', staged: true}), { - 'oldPath': 'c.txt', - 'newPath': null, - 'oldMode': '100644', - 'newMode': null, - 'hunks': [ - { - 'oldStartLine': 1, - 'oldLineCount': 1, - 'newStartLine': 0, - 'newLineCount': 0, - 'lines': [ '-baz' ] - } - ], - 'status': 'deleted' - }) - - assertDeepPropertyVals(await git.diff({filePath: 'd.txt', staged: true}), { - 'oldPath': null, - 'newPath': 'd.txt', - 'oldMode': null, - 'newMode': '100644', - 'hunks': [ - { - 'oldStartLine': 0, - 'oldLineCount': 0, - 'newStartLine': 1, - 'newLineCount': 1, - 'lines': [ '+baz' ] - } - ], - 'status': 'added' - }) + assertDeepPropertyVals(await git.getDiffForFilePath('d.txt', {staged: true}), { + 'oldPath': null, + 'newPath': 'd.txt', + 'oldMode': null, + 'newMode': '100644', + 'hunks': [ + { + 'oldStartLine': 0, + 'oldLineCount': 0, + 'newStartLine': 1, + 'newLineCount': 1, + 'lines': [ '+baz' ] + } + ], + 'status': 'added' }) }) + }) - describe('when the file is staged and a base commit is specified', () => { - it('returns a diff comparing the file on the index and in the specified commit', async () => { - const workingDirPath = await cloneRepository('multiple-commits') - const git = new GitShellOutStrategy(workingDirPath) - - assertDeepPropertyVals(await git.diff({filePath: 'file.txt', staged: true, baseCommit: 'HEAD~'}), { - 'oldPath': 'file.txt', - 'newPath': 'file.txt', - 'oldMode': '100644', - 'newMode': '100644', - 'hunks': [ - { - 'oldStartLine': 1, - 'oldLineCount': 1, - 'newStartLine': 1, - 'newLineCount': 1, - 'lines': [ '-two', '+three' ] - } - ], - 'status': 'modified' - }) + describe('when the file is staged and a base commit is specified', () => { + it('returns a diff comparing the file on the index and in the specified commit', async () => { + const workingDirPath = await cloneRepository('multiple-commits') + const git = new GitShellOutStrategy(workingDirPath) + + assertDeepPropertyVals(await git.getDiffForFilePath('file.txt', {staged: true, baseCommit: 'HEAD~'}), { + 'oldPath': 'file.txt', + 'newPath': 'file.txt', + 'oldMode': '100644', + 'newMode': '100644', + 'hunks': [ + { + 'oldStartLine': 1, + 'oldLineCount': 1, + 'newStartLine': 1, + 'newLineCount': 1, + 'lines': [ '-two', '+three' ] + } + ], + 'status': 'modified' }) }) }) From f020a00e6802f22ce1fe1dce882b5e24dfeb4ab3 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 22 Nov 2016 15:49:39 -0800 Subject: [PATCH 49/49] Add test for updating FilePatchController when repository is updated --- test/github-package.test.js | 120 +++++++++++++++++++++++++----------- 1 file changed, 85 insertions(+), 35 deletions(-) diff --git a/test/github-package.test.js b/test/github-package.test.js index f897462e66..84fda4b7b9 100644 --- a/test/github-package.test.js +++ b/test/github-package.test.js @@ -4,6 +4,7 @@ import fs from 'fs' import path from 'path' import temp from 'temp' import etch from 'etch' +import sinon from 'sinon' import {cloneRepository, buildRepository} from './helpers' import FilePatch from '../lib/models/file-patch' import GithubPackage from '../lib/github-package' @@ -213,48 +214,97 @@ describe('GithubPackage', () => { }) }) - describe('when a FilePatch is selected in the staging panel', () => { - it('shows a FilePatchView for the selected patch as a pane item, always updating the existing pane item', async () => { - const workdirPath = await cloneRepository('three-files') - const repository = await buildRepository(workdirPath) + describe('showFilePatchForPath(filePath, staged, {amending, activate})', () => { + describe('when a file is selected in the staging panel', () => { + it('shows a FilePatchView for the selected file as a pane item, always updating the existing pane item', async () => { + const workdirPath = await cloneRepository('three-files') + const repository = await buildRepository(workdirPath) - fs.writeFileSync(path.join(workdirPath, 'a.txt'), 'change', 'utf8') - fs.writeFileSync(path.join(workdirPath, 'd.txt'), 'new-file', 'utf8') - await repository.stageFiles(['d.txt']) + fs.writeFileSync(path.join(workdirPath, 'a.txt'), 'change', 'utf8') + fs.writeFileSync(path.join(workdirPath, 'd.txt'), 'new-file', 'utf8') + await repository.stageFiles(['d.txt']) - githubPackage.getActiveRepository = function () { return repository } + githubPackage.getActiveRepository = function () { return repository } - assert.isNull(githubPackage.filePatchController) + assert.isNull(githubPackage.filePatchController) - await githubPackage.gitPanelController.props.didSelectFilePath('a.txt', 'unstaged', {activate: true}) - assert(githubPackage.filePatchController) - assert.equal(githubPackage.filePatchController.props.filePatch.getPath(), 'a.txt') - assert.equal(githubPackage.filePatchController.props.repository, repository) - assert.equal(githubPackage.filePatchController.props.stagingStatus, 'unstaged') - assert.equal(workspace.getActivePaneItem(), githubPackage.filePatchController) + await githubPackage.showFilePatchForPath('a.txt', 'unstaged', {activate: true}) + assert(githubPackage.filePatchController) + assert.equal(githubPackage.filePatchController.props.filePatch.getPath(), 'a.txt') + assert.equal(githubPackage.filePatchController.props.repository, repository) + assert.equal(githubPackage.filePatchController.props.stagingStatus, 'unstaged') + assert.equal(workspace.getActivePaneItem(), githubPackage.filePatchController) - const existingFilePatchView = githubPackage.filePatchController - const originalPane = workspace.getActivePane() - originalPane.splitRight() // activate a different pane - assert.isUndefined(workspace.getActivePaneItem()) + const existingFilePatchView = githubPackage.filePatchController + const originalPane = workspace.getActivePane() + originalPane.splitRight() // activate a different pane + assert.isUndefined(workspace.getActivePaneItem()) - await githubPackage.gitPanelController.props.didSelectFilePath('d.txt', 'staged') - assert.equal(githubPackage.filePatchController, existingFilePatchView) - assert.equal(githubPackage.filePatchController.props.filePatch.getPath(), 'd.txt') - assert.equal(githubPackage.filePatchController.props.repository, repository) - assert.equal(githubPackage.filePatchController.props.stagingStatus, 'staged') - assert.equal(originalPane.getActiveItem(), githubPackage.filePatchController) + await githubPackage.showFilePatchForPath('d.txt', 'staged') + assert.equal(githubPackage.filePatchController, existingFilePatchView) + assert.equal(githubPackage.filePatchController.props.filePatch.getPath(), 'd.txt') + assert.equal(githubPackage.filePatchController.props.repository, repository) + assert.equal(githubPackage.filePatchController.props.stagingStatus, 'staged') + assert.equal(originalPane.getActiveItem(), githubPackage.filePatchController) - originalPane.getActiveItem().destroy() - assert.isUndefined(workspace.getActivePaneItem()) - assert.isNull(githubPackage.filePatchController) + originalPane.getActiveItem().destroy() + assert.isUndefined(workspace.getActivePaneItem()) + assert.isNull(githubPackage.filePatchController) - await githubPackage.gitPanelController.props.didSelectFilePath('d.txt', 'staged', {activate: true}) - assert.notEqual(githubPackage.filePatchController, existingFilePatchView) - assert.equal(githubPackage.filePatchController.props.filePatch.getPath(), 'd.txt') - assert.equal(githubPackage.filePatchController.props.repository, repository) - assert.equal(githubPackage.filePatchController.props.stagingStatus, 'staged') - assert.equal(workspace.getActivePaneItem(), githubPackage.filePatchController) + await githubPackage.showFilePatchForPath('d.txt', 'staged', {activate: true}) + assert.notEqual(githubPackage.filePatchController, existingFilePatchView) + assert.equal(githubPackage.filePatchController.props.filePatch.getPath(), 'd.txt') + assert.equal(githubPackage.filePatchController.props.repository, repository) + assert.equal(githubPackage.filePatchController.props.stagingStatus, 'staged') + assert.equal(workspace.getActivePaneItem(), githubPackage.filePatchController) + }) + }) + + describe('when there is a change to the repo', () => { + it('updates the FilePatchController with the correct filePatch', async () => { + const workdirPath = await cloneRepository('multiple-commits') + const repository = await buildRepository(workdirPath) + + githubPackage.getActiveRepository = function () { return repository } + fs.writeFileSync(path.join(workdirPath, 'file.txt'), 'change', 'utf8') + await githubPackage.showFilePatchForPath('file.txt', 'unstaged', {activate: true}) + + let filePatchController = githubPackage.filePatchController + sinon.spy(filePatchController, 'update') + + // udpate unstaged file patch + fs.writeFileSync(path.join(workdirPath, 'file.txt'), 'change\nplus more changes', 'utf8') + await repository.refresh() + // repository refresh causes async actions so waiting for this is the best way to determine the update has finished + await etch.getScheduler().getNextUpdatePromise() + const unstagedFilePatch = await repository.getFilePatchForPath('file.txt') + assert.deepEqual(filePatchController.update.lastCall.args[0].filePatch, unstagedFilePatch) + + // update staged file patch + await repository.stageFiles(['file.txt']) + await githubPackage.showFilePatchForPath('file.txt', 'staged', {activate: true}) + fs.writeFileSync(path.join(workdirPath, 'file.txt'), 'change\nplus more changes\nand more!', 'utf8') + await repository.stageFiles(['file.txt']) + await repository.refresh() + // repository refresh causes async actions so waiting for this is the best way to determine the update has finished + await etch.getScheduler().getNextUpdatePromise() + const stagedFilePatch = await repository.getFilePatchForPath('file.txt', {staged: true}) + assert.deepEqual(filePatchController.update.lastCall.args[0].filePatch, stagedFilePatch) + + // update amended file patch + // didChangeAmending destroys current filePatchController + githubPackage.gitPanelController.props.didChangeAmending() + await githubPackage.showFilePatchForPath('file.txt', 'staged', {activate: true, amending: true}) + filePatchController = githubPackage.filePatchController + sinon.spy(filePatchController, 'update') + fs.writeFileSync(path.join(workdirPath, 'file.txt'), 'change file being amended', 'utf8') + await repository.stageFiles(['file.txt']) + await repository.refresh() + // repository refresh causes async actions so waiting for this is the best way to determine the update has finished + await etch.getScheduler().getNextUpdatePromise() + const amendedStagedFilePatch = await repository.getFilePatchForPath('file.txt', {staged: true, amending: true}) + assert.deepEqual(filePatchController.update.lastCall.args[0].filePatch, amendedStagedFilePatch) + }) }) }) @@ -267,7 +317,7 @@ describe('GithubPackage', () => { githubPackage.getActiveRepository = function () { return repository } - await githubPackage.gitPanelController.props.didSelectFilePath('a.txt', 'staged', {activate: true}) + await githubPackage.showFilePatchForPath('a.txt', 'staged', {activate: true}) assert.isOk(githubPackage.filePatchController) assert.equal(workspace.getActivePaneItem(), githubPackage.filePatchController)