From f5ccd16fecbaaa67cf4853e324b69e4a7a25f3f5 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 23 Oct 2018 14:57:05 -0700 Subject: [PATCH 01/44] Revert "Merge pull request #1754 from atom/revert-1713-feature/commit-template" This reverts commit c96687a82186a1bf2bb827d35ec6baf262c62941, reversing changes made to 4bc7fdec69ba63dc1db0f436cbcc645bcbd9671b. --- lib/get-repo-pipeline-manager.js | 3 +- lib/git-shell-out-strategy.js | 27 ++++++++++++++ lib/models/repository-states/present.js | 14 ++++++++ lib/models/repository-states/state.js | 4 +++ lib/models/repository.js | 1 + test/controllers/commit-controller.test.js | 33 ++++++++++++++++++ test/fixtures/repo-commit-template/a.txt | 1 + test/fixtures/repo-commit-template/b.txt | 1 + test/fixtures/repo-commit-template/c.txt | 1 + .../repo-commit-template/dot-git/HEAD | 1 + .../repo-commit-template/dot-git/config | 9 +++++ .../repo-commit-template/dot-git/description | 1 + .../repo-commit-template/dot-git/index | Bin 0 -> 634 bytes .../repo-commit-template/dot-git/info/exclude | 6 ++++ .../repo-commit-template/dot-git/logs/HEAD | 2 ++ .../dot-git/logs/refs/heads/master | 2 ++ .../25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99 | Bin 0 -> 19 bytes .../3f/af8f2e3b6247c9d7e86d3849612cba95e94af4 | Bin 0 -> 179 bytes .../4b/825dc642cb6eb9a060e54bf8d69288fbee4904 | Bin 0 -> 15 bytes .../57/16ca5987cbf97d6bb54920bea6adde242d87e6 | Bin 0 -> 19 bytes .../76/018072e09c5d31c8c6e3113b8aa0fe625195ca | Bin 0 -> 19 bytes .../d4/d73b42443436ee23eebd43174645bb4b9eaf31 | 2 ++ .../ea/e629e0574add4da0f67056fd3ec128c18b2c47 | Bin 0 -> 102 bytes .../ef/608d1399c3c033cff3f9f4a59fd77ae1f38594 | Bin 0 -> 151 bytes .../f9/14a98b83588057c160f6a29b8c0a0f54e44548 | Bin 0 -> 50 bytes .../dot-git/refs/heads/master | 1 + .../repo-commit-template/gitmessage.txt | 1 + .../repo-commit-template/subdir-1/a.txt | 1 + .../repo-commit-template/subdir-1/b.txt | 1 + .../repo-commit-template/subdir-1/c.txt | 1 + test/git-strategies.test.js | 26 ++++++++++++++ test/helpers.js | 10 +++++- 32 files changed, 147 insertions(+), 2 deletions(-) create mode 100644 test/fixtures/repo-commit-template/a.txt create mode 100644 test/fixtures/repo-commit-template/b.txt create mode 100644 test/fixtures/repo-commit-template/c.txt create mode 100644 test/fixtures/repo-commit-template/dot-git/HEAD create mode 100644 test/fixtures/repo-commit-template/dot-git/config create mode 100644 test/fixtures/repo-commit-template/dot-git/description create mode 100644 test/fixtures/repo-commit-template/dot-git/index create mode 100644 test/fixtures/repo-commit-template/dot-git/info/exclude create mode 100644 test/fixtures/repo-commit-template/dot-git/logs/HEAD create mode 100644 test/fixtures/repo-commit-template/dot-git/logs/refs/heads/master create mode 100644 test/fixtures/repo-commit-template/dot-git/objects/25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99 create mode 100644 test/fixtures/repo-commit-template/dot-git/objects/3f/af8f2e3b6247c9d7e86d3849612cba95e94af4 create mode 100644 test/fixtures/repo-commit-template/dot-git/objects/4b/825dc642cb6eb9a060e54bf8d69288fbee4904 create mode 100644 test/fixtures/repo-commit-template/dot-git/objects/57/16ca5987cbf97d6bb54920bea6adde242d87e6 create mode 100644 test/fixtures/repo-commit-template/dot-git/objects/76/018072e09c5d31c8c6e3113b8aa0fe625195ca create mode 100644 test/fixtures/repo-commit-template/dot-git/objects/d4/d73b42443436ee23eebd43174645bb4b9eaf31 create mode 100644 test/fixtures/repo-commit-template/dot-git/objects/ea/e629e0574add4da0f67056fd3ec128c18b2c47 create mode 100644 test/fixtures/repo-commit-template/dot-git/objects/ef/608d1399c3c033cff3f9f4a59fd77ae1f38594 create mode 100644 test/fixtures/repo-commit-template/dot-git/objects/f9/14a98b83588057c160f6a29b8c0a0f54e44548 create mode 100644 test/fixtures/repo-commit-template/dot-git/refs/heads/master create mode 100644 test/fixtures/repo-commit-template/gitmessage.txt create mode 100644 test/fixtures/repo-commit-template/subdir-1/a.txt create mode 100644 test/fixtures/repo-commit-template/subdir-1/b.txt create mode 100644 test/fixtures/repo-commit-template/subdir-1/c.txt diff --git a/lib/get-repo-pipeline-manager.js b/lib/get-repo-pipeline-manager.js index 489eea22a1..0e3bddc549 100644 --- a/lib/get-repo-pipeline-manager.js +++ b/lib/get-repo-pipeline-manager.js @@ -210,7 +210,8 @@ export default function({confirm, notificationManager, workspace}) { commitPipeline.addMiddleware('failed-to-commit-error', async (next, repository) => { try { const result = await next(); - repository.setCommitMessage(''); + const message = await repository.getCommitMessageFromTemplate(); + repository.setCommitMessage(message || ''); destroyFilePatchPaneItems({onlyStaged: true}, workspace); return result; } catch (error) { diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index 2b67539a46..a1bd858bb3 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -415,6 +415,33 @@ export default class GitShellOutStrategy { return this.exec(args, {writeOperation: true}); } + async getCommitMessageFromTemplate() { + let templatePath = await this.getConfig('commit.template'); + if (!templatePath) { + return ''; + } + + /** + * Get absolute path from git path + * https://git-scm.com/docs/git-config#git-config-pathname + */ + const homeDir = os.homedir(); + const regex = new RegExp('^~([^/]*)/'); + templatePath = templatePath.trim().replace(regex, (_, user) => { + return `${user ? path.join(path.dirname(homeDir), user) : homeDir}/`; + }); + + if (!path.isAbsolute(templatePath)) { + templatePath = path.join(this.workingDir, templatePath); + } + + if (!await fileExists(templatePath)) { + return ''; + } + const message = await fs.readFile(templatePath, {encoding: 'utf8'}); + return message.trim(); + } + unstageFiles(paths, commit = 'HEAD') { if (paths.length === 0) { return Promise.resolve(null); } const args = ['reset', commit, '--'].concat(paths.map(toGitPathSep)); diff --git a/lib/models/repository-states/present.js b/lib/models/repository-states/present.js index 4e6d421d73..5c1f7a4a92 100644 --- a/lib/models/repository-states/present.js +++ b/lib/models/repository-states/present.js @@ -41,6 +41,7 @@ export default class Present extends State { this.operationStates = new OperationStates({didUpdate: this.didUpdate.bind(this)}); this.commitMessage = ''; + this.setCommitMessageFromTemplate(); if (history) { this.discardHistory.updateHistory(history); @@ -55,6 +56,19 @@ export default class Present extends State { return this.commitMessage; } + async setCommitMessageFromTemplate() { + const message = await this.getCommitMessageFromTemplate(); + if (!message) { + return; + } + this.setCommitMessage(message); + this.didUpdate(); + } + + async getCommitMessageFromTemplate() { + return await this.git().getCommitMessageFromTemplate(); + } + getOperationStates() { return this.operationStates; } diff --git a/lib/models/repository-states/state.js b/lib/models/repository-states/state.js index 9c66928e7b..afdd3c2c59 100644 --- a/lib/models/repository-states/state.js +++ b/lib/models/repository-states/state.js @@ -373,6 +373,10 @@ export default class State { return ''; } + getCommitMessageFromTemplate() { + return unsupportedOperationPromise(this, 'getCommitMessageFromTemplate'); + } + // Cache getCache() { diff --git a/lib/models/repository.js b/lib/models/repository.js index ab50db5f52..0d80e05950 100644 --- a/lib/models/repository.js +++ b/lib/models/repository.js @@ -358,6 +358,7 @@ const delegates = [ 'setCommitMessage', 'getCommitMessage', + 'getCommitMessageFromTemplate', 'getCache', ]; diff --git a/test/controllers/commit-controller.test.js b/test/controllers/commit-controller.test.js index a214d0e081..0bcc6679f7 100644 --- a/test/controllers/commit-controller.test.js +++ b/test/controllers/commit-controller.test.js @@ -60,6 +60,9 @@ describe('CommitController', function() { const repository1 = await buildRepository(workdirPath1); const workdirPath2 = await cloneRepository('three-files'); const repository2 = await buildRepository(workdirPath2); + const workdirPath3 = await cloneRepository('commit-template'); + const repository3 = await buildRepository(workdirPath3); + const templateCommitMessage = await repository3.git.getCommitMessageFromTemplate(); app = React.cloneElement(app, {repository: repository1}); const wrapper = shallow(app, {disableLifecycleMethods: true}); @@ -74,8 +77,23 @@ describe('CommitController', function() { wrapper.setProps({repository: repository1}); assert.equal(wrapper.instance().getCommitMessage(), 'message 1'); + wrapper.setProps({repository: repository3}); + await assert.async.strictEqual(wrapper.instance().getCommitMessage(), templateCommitMessage); }); + + describe('when commit.template config is set', function() { + it('populates the commit message with the template', async function() { + const workdirPath = await cloneRepository('commit-template'); + const repository = await buildRepository(workdirPath); + const templateCommitMessage = await repository.git.getCommitMessageFromTemplate(); + app = React.cloneElement(app, {repository}); + const wrapper = shallow(app, {disableLifecycleMethods: true}); + await assert.async.strictEqual(wrapper.instance().getCommitMessage(), templateCommitMessage); + }); + }); + + describe('the passed commit message', function() { let repository; @@ -140,6 +158,21 @@ describe('CommitController', function() { assert.strictEqual(repository.getCommitMessage(), ''); }); + it('reload the commit messages from commit template', async function() { + const repoPath = await cloneRepository('commit-template'); + const repo = await buildRepositoryWithPipeline(repoPath, {confirm, notificationManager, workspace}); + const templateCommitMessage = await repo.git.getCommitMessageFromTemplate(); + const commitStub = sinon.stub().callsFake((...args) => repo.commit(...args)); + const app2 = React.cloneElement(app, {repository: repo, commit: commitStub}); + + await fs.writeFile(path.join(repoPath, 'a.txt'), 'some changes', {encoding: 'utf8'}); + await repo.git.exec(['add', '.']); + + const wrapper = shallow(app2, {disableLifecycleMethods: true}); + await wrapper.instance().commit('some message'); + assert.strictEqual(repo.getCommitMessage(), templateCommitMessage); + }); + it('sets the verbatim flag when committing from the mini editor', async function() { await fs.writeFile(path.join(workdirPath, 'a.txt'), 'some changes', {encoding: 'utf8'}); await repository.git.exec(['add', '.']); diff --git a/test/fixtures/repo-commit-template/a.txt b/test/fixtures/repo-commit-template/a.txt new file mode 100644 index 0000000000..257cc5642c --- /dev/null +++ b/test/fixtures/repo-commit-template/a.txt @@ -0,0 +1 @@ +foo diff --git a/test/fixtures/repo-commit-template/b.txt b/test/fixtures/repo-commit-template/b.txt new file mode 100644 index 0000000000..5716ca5987 --- /dev/null +++ b/test/fixtures/repo-commit-template/b.txt @@ -0,0 +1 @@ +bar diff --git a/test/fixtures/repo-commit-template/c.txt b/test/fixtures/repo-commit-template/c.txt new file mode 100644 index 0000000000..76018072e0 --- /dev/null +++ b/test/fixtures/repo-commit-template/c.txt @@ -0,0 +1 @@ +baz diff --git a/test/fixtures/repo-commit-template/dot-git/HEAD b/test/fixtures/repo-commit-template/dot-git/HEAD new file mode 100644 index 0000000000..cb089cd89a --- /dev/null +++ b/test/fixtures/repo-commit-template/dot-git/HEAD @@ -0,0 +1 @@ +ref: refs/heads/master diff --git a/test/fixtures/repo-commit-template/dot-git/config b/test/fixtures/repo-commit-template/dot-git/config new file mode 100644 index 0000000000..52b216917a --- /dev/null +++ b/test/fixtures/repo-commit-template/dot-git/config @@ -0,0 +1,9 @@ +[core] + repositoryformatversion = 0 + filemode = true + bare = false + logallrefupdates = true + ignorecase = true + precomposeunicode = true +[commit] + template = gitmessage.txt diff --git a/test/fixtures/repo-commit-template/dot-git/description b/test/fixtures/repo-commit-template/dot-git/description new file mode 100644 index 0000000000..498b267a8c --- /dev/null +++ b/test/fixtures/repo-commit-template/dot-git/description @@ -0,0 +1 @@ +Unnamed repository; edit this file 'description' to name the repository. diff --git a/test/fixtures/repo-commit-template/dot-git/index b/test/fixtures/repo-commit-template/dot-git/index new file mode 100644 index 0000000000000000000000000000000000000000..064b3f3adc4f9d0fc3c482a70cba225911936212 GIT binary patch literal 634 zcmZ?q402{*U|<4b_UO&Q%Y@^<9)i(~3=Av`wqHva7#f!_Ffe`vsu2NV7S)=gDLNY$ zgnZ~ZVXr&IF6^(}zL^ZHiFzd!K&3zc)}O7U2&19ql%ksxE_N!i{q)b;?5&;(`|E#?bbH1q(We#6@W=U>padBdLD$GETdqm}?yoJ$FcdbP?4{JE_6_+NZ zWESZf>cayJY>t-jH5d&wZymaMSi=ip9z5W{=9vBa45Ojutw%QxYq%lIg9l(pkgF>& zTCEt&6%4uFC-e%>Jbb|T{O6xvmd?Lk_3(4+6quJ7j1>&HUOm%%5bkx?cfq#;V9GeC eaj;v*ecSuJ?D7jr_FNVZiJvO}^4Q|L0U7{%+uH#E literal 0 HcmV?d00001 diff --git a/test/fixtures/repo-commit-template/dot-git/info/exclude b/test/fixtures/repo-commit-template/dot-git/info/exclude new file mode 100644 index 0000000000..a5196d1be8 --- /dev/null +++ b/test/fixtures/repo-commit-template/dot-git/info/exclude @@ -0,0 +1,6 @@ +# git ls-files --others --exclude-from=.git/info/exclude +# Lines that start with '#' are comments. +# For a project mostly in C, the following would be a good set of +# exclude patterns (uncomment them if you want to use them): +# *.[oa] +# *~ diff --git a/test/fixtures/repo-commit-template/dot-git/logs/HEAD b/test/fixtures/repo-commit-template/dot-git/logs/HEAD new file mode 100644 index 0000000000..678e774168 --- /dev/null +++ b/test/fixtures/repo-commit-template/dot-git/logs/HEAD @@ -0,0 +1,2 @@ +0000000000000000000000000000000000000000 d4d73b42443436ee23eebd43174645bb4b9eaf31 Gaurav Chikhale 1538479109 +0530 commit (initial): Initial commit +d4d73b42443436ee23eebd43174645bb4b9eaf31 3faf8f2e3b6247c9d7e86d3849612cba95e94af4 Gaurav Chikhale 1538479155 +0530 commit: Add git message diff --git a/test/fixtures/repo-commit-template/dot-git/logs/refs/heads/master b/test/fixtures/repo-commit-template/dot-git/logs/refs/heads/master new file mode 100644 index 0000000000..678e774168 --- /dev/null +++ b/test/fixtures/repo-commit-template/dot-git/logs/refs/heads/master @@ -0,0 +1,2 @@ +0000000000000000000000000000000000000000 d4d73b42443436ee23eebd43174645bb4b9eaf31 Gaurav Chikhale 1538479109 +0530 commit (initial): Initial commit +d4d73b42443436ee23eebd43174645bb4b9eaf31 3faf8f2e3b6247c9d7e86d3849612cba95e94af4 Gaurav Chikhale 1538479155 +0530 commit: Add git message diff --git a/test/fixtures/repo-commit-template/dot-git/objects/25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99 b/test/fixtures/repo-commit-template/dot-git/objects/25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99 new file mode 100644 index 0000000000000000000000000000000000000000..bdcf704c9e663f3a11b3146b1b455bc2581b4761 GIT binary patch literal 19 acmb^)#C@5-_ugq9xq9V=;Nnfiq+2m1Fl`G#EQFf(CW zmZ?J54cP;pr6gc%1 I09Vl}{eDC-*#H0l literal 0 HcmV?d00001 diff --git a/test/fixtures/repo-commit-template/dot-git/objects/ef/608d1399c3c033cff3f9f4a59fd77ae1f38594 b/test/fixtures/repo-commit-template/dot-git/objects/ef/608d1399c3c033cff3f9f4a59fd77ae1f38594 new file mode 100644 index 0000000000000000000000000000000000000000..a90cc1ccb50a8c244093ae8c44b4903ad0394260 GIT binary patch literal 151 zcmV;I0BHYs0V^p=O;s>7H)Aj~FfcPQQApG)sVHGktvQ;avvEPlhn^Gmx>M}J{@U%E z3005;RuC?BDzg3b&)V#*o(lVxt-YtB+x`ryAQ`NnjIp8U!JJsb6UQD4T6Zn@mlQbl z6jVWaW=U>padBdLDo&Lq20)-tT$+@US)^;o@amc7gK)3Az6-t;0G)DB<6yUrI{@-l FOLuJZM@Ikv literal 0 HcmV?d00001 diff --git a/test/fixtures/repo-commit-template/dot-git/objects/f9/14a98b83588057c160f6a29b8c0a0f54e44548 b/test/fixtures/repo-commit-template/dot-git/objects/f9/14a98b83588057c160f6a29b8c0a0f54e44548 new file mode 100644 index 0000000000000000000000000000000000000000..6e37521f9b2536eebd27c18ce54b0f62bd26e992 GIT binary patch literal 50 zcmV-20L}k+0V^p=O;s>9WiT-S0)^tzq?F7eT| Date: Tue, 23 Oct 2018 15:42:11 -0700 Subject: [PATCH 02/44] :art: getCommitMessageFromTemplate tests. Return null if config not set Co-Authored-By: Tilde Ann Thurium --- lib/git-shell-out-strategy.js | 5 ++-- test/git-strategies.test.js | 49 +++++++++++++++++++++++------------ 2 files changed, 36 insertions(+), 18 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index a1bd858bb3..aaa283a1ad 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -418,7 +418,7 @@ export default class GitShellOutStrategy { async getCommitMessageFromTemplate() { let templatePath = await this.getConfig('commit.template'); if (!templatePath) { - return ''; + return null; } /** @@ -426,6 +426,7 @@ export default class GitShellOutStrategy { * https://git-scm.com/docs/git-config#git-config-pathname */ const homeDir = os.homedir(); + // TODO: figure out what this is doing: const regex = new RegExp('^~([^/]*)/'); templatePath = templatePath.trim().replace(regex, (_, user) => { return `${user ? path.join(path.dirname(homeDir), user) : homeDir}/`; @@ -436,7 +437,7 @@ export default class GitShellOutStrategy { } if (!await fileExists(templatePath)) { - return ''; + throw new Error(`Invalid commit template path set in Git config: ${templatePath}`); } const message = await fs.readFile(templatePath, {encoding: 'utf8'}); return message.trim(); diff --git a/test/git-strategies.test.js b/test/git-strategies.test.js index c63fe9164b..c1f310fc95 100644 --- a/test/git-strategies.test.js +++ b/test/git-strategies.test.js @@ -87,29 +87,46 @@ import * as reporterProxy from '../lib/reporter-proxy'; }); describe('getCommitMessageFromTemplate', function() { - it('supports repository root path', async function() { - const workingDirPath = await cloneRepository('commit-template'); + it('gets commit message from template', async function() { + const workingDirPath = await cloneRepository('three-files'); const git = createTestStrategy(workingDirPath); - const absTemplatePath = path.join(workingDirPath, 'gitmessage.txt'); - const commitTemplate = fs.readFileSync(absTemplatePath, 'utf8').trim(); - const message = await git.getCommitMessageFromTemplate(); - assert.equal(message, commitTemplate); + const templateText = 'some commit message'; + + const commitMsgTemplatePath = path.join(workingDirPath, '.gitmessage'); + await fs.writeFile(commitMsgTemplatePath, templateText, {encoding: 'utf8'}); + + await git.setConfig('commit.template', commitMsgTemplatePath); + assert.equal(await git.getCommitMessageFromTemplate(), templateText); }); - it('supports relative path', async function() { - const workingDirPath = await cloneRepository('commit-template'); + it('if config is not set return null', async function() { + const workingDirPath = await cloneRepository('three-files'); const git = createTestStrategy(workingDirPath); - const homeDir = os.homedir(); - const absTemplatePath = path.join(homeDir, '.gitMessageSample.txt'); - await fs.writeFile(absTemplatePath, 'some commit message', {encoding: 'utf8'}); - await git.exec(['config', '--local', 'commit.template', '~/.gitMessageSample.txt']); - const message = await git.getCommitMessageFromTemplate(); - assert.equal(message, 'some commit message'); - fs.removeSync(absTemplatePath); + assert.strictEqual(await git.getConfig('commit.template'), ''); + assert.isNull(await git.getCommitMessageFromTemplate()); }); - }); + it('if config is set but file does not exist throw an error', async function() { + const workingDirPath = await cloneRepository('three-files'); + const git = createTestStrategy(workingDirPath); + + const nonExistentCommitTemplatePath = path.join(workingDirPath, 'file-that-doesnt-exist'); + await git.setConfig('commit.template', nonExistentCommitTemplatePath); + await assert.isRejected( + git.getCommitMessageFromTemplate(), + `Invalid commit template path set in Git config: ${nonExistentCommitTemplatePath}`, + ); + }); + + it('test that ~ gets expanded correctly', async function() { + + }); + + it('test that relative path in repo is covered', async function() { + + }); + }); if (process.platform === 'win32') { describe('getStatusBundle()', function() { From 49a1d31f9ac7f64dc4d71e41c48a9151fcd9e93f Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 13:12:17 -0700 Subject: [PATCH 03/44] Restore state from last commit only if undo is successful Repro steps: open repo with merge conflict click undo see error (cannot do a soft reset in the middle of a merge) expected: no change to commit message box actual: commit message from last commit appears in box --- lib/controllers/git-tab-controller.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/controllers/git-tab-controller.js b/lib/controllers/git-tab-controller.js index 8744a518ba..cf84a812b4 100644 --- a/lib/controllers/git-tab-controller.js +++ b/lib/controllers/git-tab-controller.js @@ -275,13 +275,15 @@ export default class GitTabController extends React.Component { const repo = this.props.repository; const lastCommit = await repo.getLastCommit(); if (lastCommit.isUnbornRef()) { return null; } + + await repo.undoLastCommit(); repo.setCommitMessage(lastCommit.getFullMessage()); const coAuthors = lastCommit.getCoAuthors().map(author => new Author(author.email, author.name)); this.updateSelectedCoAuthors(coAuthors); - return repo.undoLastCommit(); + return null; } async abortMerge() { From 3ebda2f2924be0752b9ce6fe02059485822dea68 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 13:17:41 -0700 Subject: [PATCH 04/44] Pass events to `observeFilesystemChange` in order to use action property --- lib/models/repository-states/present.js | 7 ++++++- lib/models/workdir-context.js | 3 +-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/models/repository-states/present.js b/lib/models/repository-states/present.js index 5c1f7a4a92..5c74e5f06c 100644 --- a/lib/models/repository-states/present.js +++ b/lib/models/repository-states/present.js @@ -91,7 +91,8 @@ export default class Present extends State { this.didUpdate(); } - observeFilesystemChange(paths) { + invalidateCacheAfterFilesystemChange(events) { + const paths = events.map(e => e.special || e.path); const keys = new Set(); for (let i = 0; i < paths.length; i++) { const fullPath = paths[i]; @@ -159,6 +160,10 @@ export default class Present extends State { } } + observeFilesystemChange(events) { + this.invalidateCacheAfterFilesystemChange(events); + } + refresh() { this.cache.clear(); this.didUpdate(); diff --git a/lib/models/workdir-context.js b/lib/models/workdir-context.js index b7a5ed749b..3c29a405ee 100644 --- a/lib/models/workdir-context.js +++ b/lib/models/workdir-context.js @@ -50,8 +50,7 @@ export default class WorkdirContext { // Wire up event forwarding among models this.subs.add(this.repository.onDidChangeState(this.repositoryChangedState)); this.subs.add(this.observer.onDidChange(events => { - const paths = events.map(e => e.special || e.path); - this.repository.observeFilesystemChange(paths); + this.repository.observeFilesystemChange(events); })); this.subs.add(this.observer.onDidChangeWorkdirOrHead(() => this.emitter.emit('did-change-workdir-or-head'))); From b6ef6d2c3314db494ee76059a6f382d8cf177837 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 13:21:32 -0700 Subject: [PATCH 05/44] Update commit message after file system change for MERGE_HEAD or config --- lib/models/repository-states/present.js | 41 +++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/lib/models/repository-states/present.js b/lib/models/repository-states/present.js index 5c74e5f06c..03b6637637 100644 --- a/lib/models/repository-states/present.js +++ b/lib/models/repository-states/present.js @@ -160,8 +160,49 @@ export default class Present extends State { } } + isCommitMessageClean() { + if (this.commitMessage === '') { + return true; + } else if (this.commitMessageTemplate) { + return this.commitMessage === this.commitMessageTemplate; + } + } + + async updateCommitMessageAfterFileSystemChange(events) { + for (let i = 0; i < events.length; i++) { + const event = events[i]; + const endsWith = (...segments) => event.path.endsWith(path.join(...segments)); + + if (endsWith('.git', 'MERGE_HEAD')) { + if (event.action === 'created') { + if (this.isCommitMessageClean()) { // is it really necessary to check if commit message is clean? + this.setCommitMessage(await this.repository.getMergeMessage()); + // this.didUpdate(); + } + } else if (event.action === 'deleted') { + this.setCommitMessage(this.commitMessageTemplate || ''); + // this.didUpdate(); + } + } + + if (endsWith('.git', 'config')) { + // this won't catch changes made to the template file itself... + + // question -- can filewatcher watch folders that have symlinks to files that change? + // should we create a folder in .atom that symlinks to global config file and commit message template file? to watch for changes and update correctly? + const template = await this.getCommitMessageFromTemplate(); + if (this.commitMessageTemplate !== template) { + this.setCommitMessageTemplate(template); + this.setCommitMessage(template); + // this.didUpdate(); + } + } + } + } + observeFilesystemChange(events) { this.invalidateCacheAfterFilesystemChange(events); + this.updateCommitMessageAfterFileSystemChange(events); } refresh() { From d89569109e3663dc011444ceebdd9fd82cca67a8 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 13:21:41 -0700 Subject: [PATCH 06/44] :shirt: --- lib/models/repository-states/present.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/models/repository-states/present.js b/lib/models/repository-states/present.js index 03b6637637..bf0b1b3bf0 100644 --- a/lib/models/repository-states/present.js +++ b/lib/models/repository-states/present.js @@ -166,6 +166,7 @@ export default class Present extends State { } else if (this.commitMessageTemplate) { return this.commitMessage === this.commitMessageTemplate; } + return false; } async updateCommitMessageAfterFileSystemChange(events) { From bcbbff87f2a68e68d27f22ac869476415e04a64e Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 13:25:15 -0700 Subject: [PATCH 07/44] Load initial commit message based on template or merge state --- lib/models/repository-states/present.js | 35 +++++++++++++++---------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/lib/models/repository-states/present.js b/lib/models/repository-states/present.js index bf0b1b3bf0..122af49707 100644 --- a/lib/models/repository-states/present.js +++ b/lib/models/repository-states/present.js @@ -41,7 +41,8 @@ export default class Present extends State { this.operationStates = new OperationStates({didUpdate: this.didUpdate.bind(this)}); this.commitMessage = ''; - this.setCommitMessageFromTemplate(); + this.commitMessageTemplate = null; + this.fetchInitialMessage(); if (history) { this.discardHistory.updateHistory(history); @@ -50,23 +51,32 @@ export default class Present extends State { setCommitMessage(message) { this.commitMessage = message; + this.didUpdate(); } - getCommitMessage() { - return this.commitMessage; + setCommitMessageTemplate(template) { + this.commitMessageTemplate = template; } - async setCommitMessageFromTemplate() { - const message = await this.getCommitMessageFromTemplate(); - if (!message) { - return; + async fetchInitialMessage() { + const mergeMessage = await this.repository.getMergeMessage(); + const template = await this.getCommitMessageFromTemplate(); + if (template) { + this.commitMessageTemplate = template; + } + if (mergeMessage) { + this.setCommitMessage(mergeMessage); + } else if (template) { + this.setCommitMessage(template); } - this.setCommitMessage(message); - this.didUpdate(); } - async getCommitMessageFromTemplate() { - return await this.git().getCommitMessageFromTemplate(); + getCommitMessage() { + return this.commitMessage; + } + + getCommitMessageFromTemplate() { + return this.git().getCommitMessageFromTemplate(); } getOperationStates() { @@ -188,9 +198,6 @@ export default class Present extends State { if (endsWith('.git', 'config')) { // this won't catch changes made to the template file itself... - - // question -- can filewatcher watch folders that have symlinks to files that change? - // should we create a folder in .atom that symlinks to global config file and commit message template file? to watch for changes and update correctly? const template = await this.getCommitMessageFromTemplate(); if (this.commitMessageTemplate !== template) { this.setCommitMessageTemplate(template); From b8e55ab6d3271e95a6a537b94d6f93ed99cfa8f6 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 13:25:48 -0700 Subject: [PATCH 08/44] :art: variable message -> template --- lib/get-repo-pipeline-manager.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/get-repo-pipeline-manager.js b/lib/get-repo-pipeline-manager.js index 0e3bddc549..ee4df4375a 100644 --- a/lib/get-repo-pipeline-manager.js +++ b/lib/get-repo-pipeline-manager.js @@ -210,8 +210,8 @@ export default function({confirm, notificationManager, workspace}) { commitPipeline.addMiddleware('failed-to-commit-error', async (next, repository) => { try { const result = await next(); - const message = await repository.getCommitMessageFromTemplate(); - repository.setCommitMessage(message || ''); + const template = await repository.getCommitMessageFromTemplate(); + repository.setCommitMessage(template || ''); destroyFilePatchPaneItems({onlyStaged: true}, workspace); return result; } catch (error) { From ca10d1379b5fcf10fa728af02ab16abf4313eb66 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 13:27:05 -0700 Subject: [PATCH 09/44] Don't set merge message in controller, now handled in repository --- lib/controllers/commit-controller.js | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/lib/controllers/commit-controller.js b/lib/controllers/commit-controller.js index 37837c8502..5bba94c5e3 100644 --- a/lib/controllers/commit-controller.js +++ b/lib/controllers/commit-controller.js @@ -27,7 +27,6 @@ export default class CommitController extends React.Component { repository: PropTypes.object.isRequired, isMerging: PropTypes.bool.isRequired, - mergeMessage: PropTypes.string, mergeConflictsExist: PropTypes.bool.isRequired, stagedChangesExist: PropTypes.bool.isRequired, lastCommit: PropTypes.object.isRequired, @@ -73,10 +72,6 @@ export default class CommitController extends React.Component { } }), ); - - if (this.props.isMerging && !this.getCommitMessage()) { - this.setCommitMessage(this.props.mergeMessage || ''); - } } render() { @@ -110,13 +105,6 @@ export default class CommitController extends React.Component { ); } - // eslint-disable-next-line camelcase - UNSAFE_componentWillReceiveProps(nextProps) { - if (!this.props.isMerging && nextProps.isMerging && !this.getCommitMessage()) { - this.setCommitMessage(nextProps.mergeMessage || ''); - } - } - componentWillUnmount() { this.subscriptions.dispose(); } @@ -137,9 +125,7 @@ export default class CommitController extends React.Component { setCommitMessage(message) { if (!this.props.repository.isPresent()) { return; } - const changed = this.props.repository.getCommitMessage() !== message; this.props.repository.setCommitMessage(message); - if (changed) { this.forceUpdate(); } } getCommitMessage() { From 99fe5420dd4e0982a7d8deb62cf6a330097c163c Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 13:27:48 -0700 Subject: [PATCH 10/44] Fix repo test now that we are passing events and not paths --- test/models/repository.test.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/models/repository.test.js b/test/models/repository.test.js index f72833b17e..65c61642a2 100644 --- a/test/models/repository.test.js +++ b/test/models/repository.test.js @@ -1850,10 +1850,9 @@ describe('Repository', function() { return new Promise((resolve, reject) => { eventCallback = () => { const matchingPaths = observedEvents - .map(event => event.path) - .filter(eventPath => { + .filter(event => { for (const suffix of pending) { - if (eventPath.endsWith(suffix)) { + if (event.path.endsWith(suffix)) { pending.delete(suffix); return true; } From 40b015e27067dfaef449d9c0c0992bff3ad3df1b Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 13:28:23 -0700 Subject: [PATCH 11/44] WIP tests for CommitController --- test/controllers/commit-controller.test.js | 74 ++++++++++++---------- 1 file changed, 40 insertions(+), 34 deletions(-) diff --git a/test/controllers/commit-controller.test.js b/test/controllers/commit-controller.test.js index 0bcc6679f7..0c42487b53 100644 --- a/test/controllers/commit-controller.test.js +++ b/test/controllers/commit-controller.test.js @@ -55,14 +55,47 @@ describe('CommitController', function() { atomEnvironment.destroy(); }); + describe('when commit.template config is set', function() { + it('populates the commit message with the template', async function() { + const workdirPath = await cloneRepository('commit-template'); + const repository = await buildRepository(workdirPath); + const templateCommitMessage = await repository.git.getCommitMessageFromTemplate(); + app = React.cloneElement(app, {repository}); + const wrapper = shallow(app, {disableLifecycleMethods: true}); + await assert.async.strictEqual(wrapper.instance().getCommitMessage(), templateCommitMessage); + }); + + it('restores template after committig', async function() { + const templateText = 'some commit message'; + const commitMsgTemplatePath = path.join(workdirPath, '.gitmessage'); + await fs.writeFile(commitMsgTemplatePath, templateText, {encoding: 'utf8'}); + await repository.git.setConfig('commit.template', commitMsgTemplatePath); + + await fs.writeFile(path.join(workdirPath, 'a.txt'), 'some changes', {encoding: 'utf8'}); + await repository.git.exec(['add', '.']); + + const wrapper = shallow(app, {disableLifecycleMethods: true}); + await wrapper.instance().commit('some message'); + assert.strictEqual(repository.getCommitMessage(), templateText); + }); + }); + it('correctly updates state when switching repos', async function() { const workdirPath1 = await cloneRepository('three-files'); const repository1 = await buildRepository(workdirPath1); const workdirPath2 = await cloneRepository('three-files'); const repository2 = await buildRepository(workdirPath2); - const workdirPath3 = await cloneRepository('commit-template'); - const repository3 = await buildRepository(workdirPath3); - const templateCommitMessage = await repository3.git.getCommitMessageFromTemplate(); + + // set commit template for repository2 + const templateText = 'some commit message'; + const commitMsgTemplatePath = path.join(workdirPath2, '.gitmessage'); + await fs.writeFile(commitMsgTemplatePath, templateText, {encoding: 'utf8'}); + await repository2.git.setConfig('commit.template', commitMsgTemplatePath); + // assert.strictEqual(await repository2.getCommitMessageFromTemplate(), templateText); + + // const workdirPath3 = await cloneRepository('commit-template'); + // const repository3 = await buildRepository(workdirPath3); + // const templateCommitMessage = await repository3.git.getCommitMessageFromTemplate(); app = React.cloneElement(app, {repository: repository1}); const wrapper = shallow(app, {disableLifecycleMethods: true}); @@ -70,30 +103,18 @@ describe('CommitController', function() { assert.strictEqual(wrapper.instance().getCommitMessage(), ''); wrapper.instance().setCommitMessage('message 1'); + assert.equal(wrapper.instance().getCommitMessage(), 'message 1'); wrapper.setProps({repository: repository2}); - - assert.strictEqual(wrapper.instance().getCommitMessage(), ''); + await assert.async.strictEqual(wrapper.instance().getCommitMessage(), templateText); wrapper.setProps({repository: repository1}); assert.equal(wrapper.instance().getCommitMessage(), 'message 1'); - wrapper.setProps({repository: repository3}); - await assert.async.strictEqual(wrapper.instance().getCommitMessage(), templateCommitMessage); - }); - - describe('when commit.template config is set', function() { - it('populates the commit message with the template', async function() { - const workdirPath = await cloneRepository('commit-template'); - const repository = await buildRepository(workdirPath); - const templateCommitMessage = await repository.git.getCommitMessageFromTemplate(); - app = React.cloneElement(app, {repository}); - const wrapper = shallow(app, {disableLifecycleMethods: true}); - await assert.async.strictEqual(wrapper.instance().getCommitMessage(), templateCommitMessage); - }); + // wrapper.setProps({repository: repository3}); + // await assert.async.strictEqual(wrapper.instance().getCommitMessage(), templateCommitMessage); }); - describe('the passed commit message', function() { let repository; @@ -158,21 +179,6 @@ describe('CommitController', function() { assert.strictEqual(repository.getCommitMessage(), ''); }); - it('reload the commit messages from commit template', async function() { - const repoPath = await cloneRepository('commit-template'); - const repo = await buildRepositoryWithPipeline(repoPath, {confirm, notificationManager, workspace}); - const templateCommitMessage = await repo.git.getCommitMessageFromTemplate(); - const commitStub = sinon.stub().callsFake((...args) => repo.commit(...args)); - const app2 = React.cloneElement(app, {repository: repo, commit: commitStub}); - - await fs.writeFile(path.join(repoPath, 'a.txt'), 'some changes', {encoding: 'utf8'}); - await repo.git.exec(['add', '.']); - - const wrapper = shallow(app2, {disableLifecycleMethods: true}); - await wrapper.instance().commit('some message'); - assert.strictEqual(repo.getCommitMessage(), templateCommitMessage); - }); - it('sets the verbatim flag when committing from the mini editor', async function() { await fs.writeFile(path.join(workdirPath, 'a.txt'), 'some changes', {encoding: 'utf8'}); await repository.git.exec(['add', '.']); From ade7de86d60bc5199988fc986ce30c6995c788da Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 12:13:22 -0700 Subject: [PATCH 12/44] Suppress repo update when typing commit message See https://github.com/atom/github/pull/1464 (for performance) --- lib/controllers/commit-controller.js | 6 +++--- lib/models/repository-states/present.js | 6 ++++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/controllers/commit-controller.js b/lib/controllers/commit-controller.js index 5bba94c5e3..7c44257da7 100644 --- a/lib/controllers/commit-controller.js +++ b/lib/controllers/commit-controller.js @@ -123,9 +123,9 @@ export default class CommitController extends React.Component { return this.props.commit(msg.trim(), {amend, coAuthors, verbatim}); } - setCommitMessage(message) { + setCommitMessage(message, options) { if (!this.props.repository.isPresent()) { return; } - this.props.repository.setCommitMessage(message); + this.props.repository.setCommitMessage(message, options); } getCommitMessage() { @@ -140,7 +140,7 @@ export default class CommitController extends React.Component { if (!this.props.repository.isPresent()) { return; } - this.setCommitMessage(newMessage); + this.setCommitMessage(newMessage, {suppressUpdate: true}); } getCommitMessageEditors() { diff --git a/lib/models/repository-states/present.js b/lib/models/repository-states/present.js index 122af49707..168584f980 100644 --- a/lib/models/repository-states/present.js +++ b/lib/models/repository-states/present.js @@ -49,9 +49,11 @@ export default class Present extends State { } } - setCommitMessage(message) { + setCommitMessage(message, {suppressUpdate} = {suppressUpdate: false}) { this.commitMessage = message; - this.didUpdate(); + if (!suppressUpdate) { + this.didUpdate(); + } } setCommitMessageTemplate(template) { From 7c18ddb0a91729b3069416014d6df6844d8f699e Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 13:35:34 -0700 Subject: [PATCH 13/44] Don't trim commit message template It's best to assume that template users craft their template exactly as they want it. For example, I have two empty lines at the beginning of mine so that I can quickly type my message. --- lib/git-shell-out-strategy.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index aaa283a1ad..da0a6b554a 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -440,7 +440,7 @@ export default class GitShellOutStrategy { throw new Error(`Invalid commit template path set in Git config: ${templatePath}`); } const message = await fs.readFile(templatePath, {encoding: 'utf8'}); - return message.trim(); + return message; } unstageFiles(paths, commit = 'HEAD') { From 7a731381874003c3511d095aa0b6718be0b950b7 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 13:39:05 -0700 Subject: [PATCH 14/44] :art: rename getCommitMessageFromTemplate to getCommitMessageTemplate --- lib/get-repo-pipeline-manager.js | 2 +- lib/git-shell-out-strategy.js | 6 +++--- lib/models/repository-states/present.js | 8 ++++---- lib/models/repository-states/state.js | 4 ++-- lib/models/repository.js | 2 +- test/controllers/commit-controller.test.js | 6 +++--- test/git-strategies.test.js | 8 ++++---- 7 files changed, 18 insertions(+), 18 deletions(-) diff --git a/lib/get-repo-pipeline-manager.js b/lib/get-repo-pipeline-manager.js index ee4df4375a..b153920ddc 100644 --- a/lib/get-repo-pipeline-manager.js +++ b/lib/get-repo-pipeline-manager.js @@ -210,7 +210,7 @@ export default function({confirm, notificationManager, workspace}) { commitPipeline.addMiddleware('failed-to-commit-error', async (next, repository) => { try { const result = await next(); - const template = await repository.getCommitMessageFromTemplate(); + const template = await repository.getCommitMessageTemplate(); repository.setCommitMessage(template || ''); destroyFilePatchPaneItems({onlyStaged: true}, workspace); return result; diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index da0a6b554a..d1399a16fa 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -415,7 +415,7 @@ export default class GitShellOutStrategy { return this.exec(args, {writeOperation: true}); } - async getCommitMessageFromTemplate() { + async getCommitMessageTemplate() { let templatePath = await this.getConfig('commit.template'); if (!templatePath) { return null; @@ -439,8 +439,8 @@ export default class GitShellOutStrategy { if (!await fileExists(templatePath)) { throw new Error(`Invalid commit template path set in Git config: ${templatePath}`); } - const message = await fs.readFile(templatePath, {encoding: 'utf8'}); - return message; + const template = await fs.readFile(templatePath, {encoding: 'utf8'}); + return template; } unstageFiles(paths, commit = 'HEAD') { diff --git a/lib/models/repository-states/present.js b/lib/models/repository-states/present.js index 168584f980..a19b822792 100644 --- a/lib/models/repository-states/present.js +++ b/lib/models/repository-states/present.js @@ -62,7 +62,7 @@ export default class Present extends State { async fetchInitialMessage() { const mergeMessage = await this.repository.getMergeMessage(); - const template = await this.getCommitMessageFromTemplate(); + const template = await this.getCommitMessageTemplate(); if (template) { this.commitMessageTemplate = template; } @@ -77,8 +77,8 @@ export default class Present extends State { return this.commitMessage; } - getCommitMessageFromTemplate() { - return this.git().getCommitMessageFromTemplate(); + getCommitMessageTemplate() { + return this.git().getCommitMessageTemplate(); } getOperationStates() { @@ -200,7 +200,7 @@ export default class Present extends State { if (endsWith('.git', 'config')) { // this won't catch changes made to the template file itself... - const template = await this.getCommitMessageFromTemplate(); + const template = await this.getCommitMessageTemplate(); if (this.commitMessageTemplate !== template) { this.setCommitMessageTemplate(template); this.setCommitMessage(template); diff --git a/lib/models/repository-states/state.js b/lib/models/repository-states/state.js index afdd3c2c59..5e6b7373f1 100644 --- a/lib/models/repository-states/state.js +++ b/lib/models/repository-states/state.js @@ -373,8 +373,8 @@ export default class State { return ''; } - getCommitMessageFromTemplate() { - return unsupportedOperationPromise(this, 'getCommitMessageFromTemplate'); + getCommitMessageTemplate() { + return unsupportedOperationPromise(this, 'getCommitMessageTemplate'); } // Cache diff --git a/lib/models/repository.js b/lib/models/repository.js index 0d80e05950..ef54885474 100644 --- a/lib/models/repository.js +++ b/lib/models/repository.js @@ -358,7 +358,7 @@ const delegates = [ 'setCommitMessage', 'getCommitMessage', - 'getCommitMessageFromTemplate', + 'getCommitMessageTemplate', 'getCache', ]; diff --git a/test/controllers/commit-controller.test.js b/test/controllers/commit-controller.test.js index 0c42487b53..225d6d69f2 100644 --- a/test/controllers/commit-controller.test.js +++ b/test/controllers/commit-controller.test.js @@ -59,7 +59,7 @@ describe('CommitController', function() { it('populates the commit message with the template', async function() { const workdirPath = await cloneRepository('commit-template'); const repository = await buildRepository(workdirPath); - const templateCommitMessage = await repository.git.getCommitMessageFromTemplate(); + const templateCommitMessage = await repository.git.getCommitMessageTemplate(); app = React.cloneElement(app, {repository}); const wrapper = shallow(app, {disableLifecycleMethods: true}); await assert.async.strictEqual(wrapper.instance().getCommitMessage(), templateCommitMessage); @@ -91,11 +91,11 @@ describe('CommitController', function() { const commitMsgTemplatePath = path.join(workdirPath2, '.gitmessage'); await fs.writeFile(commitMsgTemplatePath, templateText, {encoding: 'utf8'}); await repository2.git.setConfig('commit.template', commitMsgTemplatePath); - // assert.strictEqual(await repository2.getCommitMessageFromTemplate(), templateText); + // assert.strictEqual(await repository2.getCommitMessageTemplate(), templateText); // const workdirPath3 = await cloneRepository('commit-template'); // const repository3 = await buildRepository(workdirPath3); - // const templateCommitMessage = await repository3.git.getCommitMessageFromTemplate(); + // const templateCommitMessage = await repository3.git.getCommitMessageTemplate(); app = React.cloneElement(app, {repository: repository1}); const wrapper = shallow(app, {disableLifecycleMethods: true}); diff --git a/test/git-strategies.test.js b/test/git-strategies.test.js index c1f310fc95..4d61ba4b94 100644 --- a/test/git-strategies.test.js +++ b/test/git-strategies.test.js @@ -86,7 +86,7 @@ import * as reporterProxy from '../lib/reporter-proxy'; }); }); - describe('getCommitMessageFromTemplate', function() { + describe('getCommitMessageTemplate', function() { it('gets commit message from template', async function() { const workingDirPath = await cloneRepository('three-files'); const git = createTestStrategy(workingDirPath); @@ -96,7 +96,7 @@ import * as reporterProxy from '../lib/reporter-proxy'; await fs.writeFile(commitMsgTemplatePath, templateText, {encoding: 'utf8'}); await git.setConfig('commit.template', commitMsgTemplatePath); - assert.equal(await git.getCommitMessageFromTemplate(), templateText); + assert.equal(await git.getCommitMessageTemplate(), templateText); }); it('if config is not set return null', async function() { @@ -104,7 +104,7 @@ import * as reporterProxy from '../lib/reporter-proxy'; const git = createTestStrategy(workingDirPath); assert.strictEqual(await git.getConfig('commit.template'), ''); - assert.isNull(await git.getCommitMessageFromTemplate()); + assert.isNull(await git.getCommitMessageTemplate()); }); it('if config is set but file does not exist throw an error', async function() { @@ -114,7 +114,7 @@ import * as reporterProxy from '../lib/reporter-proxy'; const nonExistentCommitTemplatePath = path.join(workingDirPath, 'file-that-doesnt-exist'); await git.setConfig('commit.template', nonExistentCommitTemplatePath); await assert.isRejected( - git.getCommitMessageFromTemplate(), + git.getCommitMessageTemplate(), `Invalid commit template path set in Git config: ${nonExistentCommitTemplatePath}`, ); }); From b16525c9a21529dcbcdd4eaa069de609cd00427d Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 13:40:34 -0700 Subject: [PATCH 15/44] Enable commit button only if message contains non-comment lines --- lib/views/commit-view.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/views/commit-view.js b/lib/views/commit-view.js index f472b60cee..30aa57db6d 100644 --- a/lib/views/commit-view.js +++ b/lib/views/commit-view.js @@ -468,13 +468,16 @@ export default class CommitView extends React.Component { } } + isValidMessage() { + return this.editor && this.editor.getText().replace(/^#.*$/gm, '').trim().length !== 0; + } + commitIsEnabled(amend) { - const messageExists = this.editor && this.editor.getText().length !== 0; return !this.props.isCommitting && (amend || this.props.stagedChangesExist) && !this.props.mergeConflictsExist && this.props.lastCommit.isPresent() && - (this.props.deactivateCommitBox || (amend || messageExists)); + (this.props.deactivateCommitBox || (amend || this.isValidMessage())); } commitButtonText() { From b3cc2c318a67ec4ceab0eb0ac82a4b7197e9b25d Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 13:41:26 -0700 Subject: [PATCH 16/44] Set cursor to beginning of commit message template We don't want it to be at the end --- lib/views/commit-view.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/views/commit-view.js b/lib/views/commit-view.js index 30aa57db6d..889da4cddc 100644 --- a/lib/views/commit-view.js +++ b/lib/views/commit-view.js @@ -591,6 +591,11 @@ export default class CommitView extends React.Component { if (focus === CommitView.focus.EDITOR) { if (this.refEditor.map(focusElement).getOr(false)) { + if (this.editor && this.editor.getText().length > 0 && !this.isValidMessage()) { + // there is likely a commit message template present + // we want the cursor to be at the beginning, not at the and of the template + this.editor.setCursorBufferPosition([0, 0]); + } return true; } } From 5a7b2d4789da3a25f5ff967e933115ef90db1b6d Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 14:19:38 -0700 Subject: [PATCH 17/44] Reset commit message after a successful abort triggered through the UI According to @smashwilson > it may be worth triggering some of that behavior manually if a user aborts (or, eventually, initiates) a merge through our UI. in terms of reliability - on all three platforms there is some risk of dropped events that increases with the number of events that arrive in quick succession (like if a ton of files are created at once). merges in huge repositories _might_ trigger this but it shouldn't be common. Co-Authored-By: Ash Wilson --- lib/models/repository-states/present.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/models/repository-states/present.js b/lib/models/repository-states/present.js index a19b822792..156cc112b7 100644 --- a/lib/models/repository-states/present.js +++ b/lib/models/repository-states/present.js @@ -351,7 +351,10 @@ export default class Present extends State { Keys.filePatch.all, Keys.index.all, ], - () => this.git().abortMerge(), + async () => { + await this.git().abortMerge(); + this.setCommitMessage(this.commitMessageTemplate || ''); + }, ); } From b3588ef93f7116294032269d5a81a98075882d0b Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 16:17:32 -0700 Subject: [PATCH 18/44] Add repository tests for commit message updating due to file system events --- test/models/repository.test.js | 189 +++++++++++++++++++++++++++++++++ 1 file changed, 189 insertions(+) diff --git a/test/models/repository.test.js b/test/models/repository.test.js index 65c61642a2..329a750485 100644 --- a/test/models/repository.test.js +++ b/test/models/repository.test.js @@ -2108,4 +2108,193 @@ describe('Repository', function() { }); }); }); + + describe('updating commit message', function() { + let workdir, sub; + let observedEvents, eventCallback; + + async function wireUpObserver(fixtureName = 'multi-commits-files', existingWorkdir = null) { + observedEvents = []; + eventCallback = () => {}; + + workdir = existingWorkdir || await cloneRepository(fixtureName); + const repository = new Repository(workdir); + await repository.getLoadPromise(); + + const observer = new FileSystemChangeObserver(repository); + + sub = new CompositeDisposable( + new Disposable(async () => { + await observer.destroy(); + repository.destroy(); + }), + ); + + sub.add(observer.onDidChange(events => { + observedEvents.push(...events); + eventCallback(); + })); + + return {repository, observer}; + } + + function expectEvents(repository, ...suffixes) { + const pending = new Set(suffixes); + return new Promise((resolve, reject) => { + eventCallback = () => { + const matchingPaths = observedEvents + .filter(event => { + for (const suffix of pending) { + if (event.path.endsWith(suffix)) { + pending.delete(suffix); + return true; + } + } + return false; + }); + + if (matchingPaths.length > 0) { + repository.observeFilesystemChange(matchingPaths); + } + + if (pending.size === 0) { + resolve(); + } + }; + + if (observedEvents.length > 0) { + eventCallback(); + } + }); + } + + afterEach(function() { + sub && sub.dispose(); + }); + + describe('config commit.template change', function() { + it('updates commit messages to new template', async function() { + const {repository, observer} = await wireUpObserver(); + await observer.start(); + + assert.strictEqual(repository.getCommitMessage(), ''); + + const templatePath = path.join(workdir, 'a.txt'); + await repository.git.setConfig('commit.template', templatePath); + await expectEvents( + repository, + path.join('.git', 'config'), + ); + await assert.async.strictEqual(repository.getCommitMessage(), fs.readFileSync(templatePath, 'utf8')); + }); + }); + + describe('merge events', function() { + describe('when commit message is empty', function() { + it('merge message is set as new commit message', async function() { + const {repository, observer} = await wireUpObserver('merge-conflict'); + await observer.start(); + + assert.strictEqual(repository.getCommitMessage(), ''); + await assert.isRejected(repository.git.merge('origin/branch')); + await expectEvents( + repository, + path.join('.git', 'MERGE_HEAD'), + ); + await assert.async.strictEqual(repository.getCommitMessage(), await repository.getMergeMessage()); + }); + }); + + describe('when commit message contains unmodified template', function() { + it('merge message is set as new commit message', async function() { + const {repository, observer} = await wireUpObserver('merge-conflict'); + await observer.start(); + + const templatePath = path.join(workdir, 'added-to-both.txt'); + const templateText = fs.readFileSync(templatePath, 'utf8'); + await repository.git.setConfig('commit.template', templatePath); + await expectEvents( + repository, + path.join('.git', 'config'), + ); + + await assert.async.strictEqual(repository.getCommitMessage(), templateText); + + await assert.isRejected(repository.git.merge('origin/branch')); + await expectEvents( + repository, + path.join('.git', 'MERGE_HEAD'), + ); + await assert.async.strictEqual(repository.getCommitMessage(), await repository.getMergeMessage()); + }); + }); + + describe('when commit message is "dirty"', function() { + it('leaves commit message as is', async function() { + const {repository, observer} = await wireUpObserver('merge-conflict'); + await observer.start(); + + const dirtyMessage = 'foo bar baz'; + repository.setCommitMessage(dirtyMessage); + await assert.isRejected(repository.git.merge('origin/branch')); + await expectEvents( + repository, + path.join('.git', 'MERGE_HEAD'), + ); + assert.strictEqual(repository.getCommitMessage(), dirtyMessage); + }); + }); + + describe('when merge is aborted', function() { + it('merge message gets cleared', async function() { + const {repository, observer} = await wireUpObserver('merge-conflict'); + await observer.start(); + await assert.isRejected(repository.git.merge('origin/branch')); + await expectEvents( + repository, + path.join('.git', 'MERGE_HEAD'), + ); + await assert.async.strictEqual(repository.getCommitMessage(), await repository.getMergeMessage()); + + await repository.abortMerge(); + await expectEvents( + repository, + path.join('.git', 'MERGE_HEAD'), + ); + assert.strictEqual(repository.getCommitMessage(), ''); + + }); + + describe('when commit message template is present', function() { + it('sets template as commit message', async function() { + const {repository, observer} = await wireUpObserver('merge-conflict'); + await observer.start(); + + const templatePath = path.join(workdir, 'added-to-both.txt'); + const templateText = fs.readFileSync(templatePath, 'utf8'); + await repository.git.setConfig('commit.template', templatePath); + await expectEvents( + repository, + path.join('.git', 'config'), + ); + + await assert.isRejected(repository.git.merge('origin/branch')); + await expectEvents( + repository, + path.join('.git', 'MERGE_HEAD'), + ); + await assert.async.strictEqual(repository.getCommitMessage(), await repository.getMergeMessage()); + + await repository.abortMerge(); + await expectEvents( + repository, + path.join('.git', 'MERGE_HEAD'), + ); + + await assert.async.strictEqual(repository.getCommitMessage(), templateText); + }); + }); + }); + }); + }); }); From 4a1525b7e11c591a8e32f4fb61f8110430f9e0fe Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 16:17:56 -0700 Subject: [PATCH 19/44] Trim message before checking if clean --- lib/models/repository-states/present.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/models/repository-states/present.js b/lib/models/repository-states/present.js index 156cc112b7..a574e83138 100644 --- a/lib/models/repository-states/present.js +++ b/lib/models/repository-states/present.js @@ -173,7 +173,7 @@ export default class Present extends State { } isCommitMessageClean() { - if (this.commitMessage === '') { + if (this.commitMessage.trim() === '') { return true; } else if (this.commitMessageTemplate) { return this.commitMessage === this.commitMessageTemplate; From 2c2857e916dde4f2639394fa1412ab2afab6a203 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 16:19:38 -0700 Subject: [PATCH 20/44] :fire: merge message tests in CommitController (now handled in repo) --- test/controllers/commit-controller.test.js | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/test/controllers/commit-controller.test.js b/test/controllers/commit-controller.test.js index 225d6d69f2..10718f416e 100644 --- a/test/controllers/commit-controller.test.js +++ b/test/controllers/commit-controller.test.js @@ -40,7 +40,6 @@ describe('CommitController', function() { isMerging={false} mergeConflictsExist={false} stagedChangesExist={false} - mergeMessage={''} lastCommit={lastCommit} currentBranch={nullBranch} userStore={store} @@ -139,21 +138,6 @@ describe('CommitController', function() { assert.strictEqual(wrapper.getCommitMessage(), 'new message'); assert.isFalse(wrapper.props.repository.state.didUpdate.called); }); - - describe('when a merge message is defined', function() { - it('is set to the merge message when merging', function() { - app = React.cloneElement(app, {isMerging: true, mergeMessage: 'merge conflict!'}); - const wrapper = shallow(app, {disableLifecycleMethods: true}); - assert.strictEqual(wrapper.find('CommitView').prop('message'), 'merge conflict!'); - }); - - it('is set to getCommitMessage() if it is set', function() { - repository.setCommitMessage('some commit message'); - app = React.cloneElement(app, {isMerging: true, mergeMessage: 'merge conflict!'}); - const wrapper = shallow(app, {disableLifecycleMethods: true}); - assert.strictEqual(wrapper.find('CommitView').prop('message'), 'some commit message'); - }); - }); }); describe('committing', function() { From f780ad100545389674c4ce0e9f4c7f161e68a346 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 16:22:18 -0700 Subject: [PATCH 21/44] Fix WorkdirContext test --- test/models/workdir-context.test.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/models/workdir-context.test.js b/test/models/workdir-context.test.js index c4d8df2371..a7732e9d09 100644 --- a/test/models/workdir-context.test.js +++ b/test/models/workdir-context.test.js @@ -64,9 +64,10 @@ describe('WorkdirContext', function() { sinon.spy(repo, 'observeFilesystemChange'); - context.getChangeObserver().didChange([{path: path.join('a', 'b', 'c')}]); + const events = [{path: path.join('a', 'b', 'c')}]; + context.getChangeObserver().didChange(events); - assert.isTrue(repo.observeFilesystemChange.calledWith([path.join('a', 'b', 'c')])); + assert.isTrue(repo.observeFilesystemChange.calledWith(events)); }); it('re-emits an event on workdir or head change', async function() { From 5ee83db1dd30b580582ad89c70c4b3988d8f91c8 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 19:06:21 -0700 Subject: [PATCH 22/44] Fix CommitController tests Co-Authored-By: Matt --- lib/controllers/commit-controller.js | 3 ++ test/controllers/commit-controller.test.js | 63 ++++++++++------------ 2 files changed, 30 insertions(+), 36 deletions(-) diff --git a/lib/controllers/commit-controller.js b/lib/controllers/commit-controller.js index 7c44257da7..9239fb7575 100644 --- a/lib/controllers/commit-controller.js +++ b/lib/controllers/commit-controller.js @@ -125,7 +125,10 @@ export default class CommitController extends React.Component { setCommitMessage(message, options) { if (!this.props.repository.isPresent()) { return; } + const changed = this.props.repository.getCommitMessage() !== message; this.props.repository.setCommitMessage(message, options); + // ask @smashwilson -- why do we need to do this?? + if (changed) { this.forceUpdate(); } } getCommitMessage() { diff --git a/test/controllers/commit-controller.test.js b/test/controllers/commit-controller.test.js index 10718f416e..c21f3ca67f 100644 --- a/test/controllers/commit-controller.test.js +++ b/test/controllers/commit-controller.test.js @@ -54,31 +54,6 @@ describe('CommitController', function() { atomEnvironment.destroy(); }); - describe('when commit.template config is set', function() { - it('populates the commit message with the template', async function() { - const workdirPath = await cloneRepository('commit-template'); - const repository = await buildRepository(workdirPath); - const templateCommitMessage = await repository.git.getCommitMessageTemplate(); - app = React.cloneElement(app, {repository}); - const wrapper = shallow(app, {disableLifecycleMethods: true}); - await assert.async.strictEqual(wrapper.instance().getCommitMessage(), templateCommitMessage); - }); - - it('restores template after committig', async function() { - const templateText = 'some commit message'; - const commitMsgTemplatePath = path.join(workdirPath, '.gitmessage'); - await fs.writeFile(commitMsgTemplatePath, templateText, {encoding: 'utf8'}); - await repository.git.setConfig('commit.template', commitMsgTemplatePath); - - await fs.writeFile(path.join(workdirPath, 'a.txt'), 'some changes', {encoding: 'utf8'}); - await repository.git.exec(['add', '.']); - - const wrapper = shallow(app, {disableLifecycleMethods: true}); - await wrapper.instance().commit('some message'); - assert.strictEqual(repository.getCommitMessage(), templateText); - }); - }); - it('correctly updates state when switching repos', async function() { const workdirPath1 = await cloneRepository('three-files'); const repository1 = await buildRepository(workdirPath1); @@ -86,15 +61,10 @@ describe('CommitController', function() { const repository2 = await buildRepository(workdirPath2); // set commit template for repository2 - const templateText = 'some commit message'; - const commitMsgTemplatePath = path.join(workdirPath2, '.gitmessage'); - await fs.writeFile(commitMsgTemplatePath, templateText, {encoding: 'utf8'}); - await repository2.git.setConfig('commit.template', commitMsgTemplatePath); - // assert.strictEqual(await repository2.getCommitMessageTemplate(), templateText); - - // const workdirPath3 = await cloneRepository('commit-template'); - // const repository3 = await buildRepository(workdirPath3); - // const templateCommitMessage = await repository3.git.getCommitMessageTemplate(); + const templatePath = path.join(workdirPath2, 'a.txt'); + const templateText = fs.readFileSync(templatePath, 'utf8'); + await repository2.git.setConfig('commit.template', templatePath); + await assert.async.strictEqual(repository2.getCommitMessage(), templateText); app = React.cloneElement(app, {repository: repository1}); const wrapper = shallow(app, {disableLifecycleMethods: true}); @@ -106,12 +76,13 @@ describe('CommitController', function() { wrapper.setProps({repository: repository2}); await assert.async.strictEqual(wrapper.instance().getCommitMessage(), templateText); + wrapper.instance().setCommitMessage('message 2'); wrapper.setProps({repository: repository1}); assert.equal(wrapper.instance().getCommitMessage(), 'message 1'); - // wrapper.setProps({repository: repository3}); - // await assert.async.strictEqual(wrapper.instance().getCommitMessage(), templateCommitMessage); + wrapper.setProps({repository: repository2}); + assert.equal(wrapper.instance().getCommitMessage(), 'message 2'); }); describe('the passed commit message', function() { @@ -196,6 +167,26 @@ describe('CommitController', function() { assert.strictEqual(repository.getCommitMessage(), 'some message'); }); + it('restores template after committing', async function() { + const templatePath = path.join(workdirPath, 'a.txt'); + const templateText = fs.readFileSync(templatePath, 'utf8'); + await repository.git.setConfig('commit.template', templatePath); + await assert.async.strictEqual(repository.getCommitMessage(), templateText); + + const nonTemplateText = 'some new text...'; + repository.setCommitMessage(nonTemplateText); + + assert.strictEqual(repository.getCommitMessage(), nonTemplateText); + + app = React.cloneElement(app, {repository}); + const wrapper = shallow(app, {disableLifecycleMethods: true}); + await fs.writeFile(path.join(workdirPath, 'new-file.txt'), 'some changes', {encoding: 'utf8'}); + await repository.stageFiles(['new-file.txt']); + await wrapper.instance().commit(nonTemplateText); + + await assert.async.strictEqual(repository.getCommitMessage(), templateText); + }); + describe('message formatting', function() { let commitSpy, wrapper; From 6e45f0cfc7d9c22a21fec5ef849dc99574a54ca9 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 19:08:19 -0700 Subject: [PATCH 23/44] :fire: commit-template test fixture --- test/fixtures/repo-commit-template/a.txt | 1 - test/fixtures/repo-commit-template/b.txt | 1 - test/fixtures/repo-commit-template/c.txt | 1 - test/fixtures/repo-commit-template/dot-git/HEAD | 1 - test/fixtures/repo-commit-template/dot-git/config | 9 --------- .../repo-commit-template/dot-git/description | 1 - test/fixtures/repo-commit-template/dot-git/index | Bin 634 -> 0 bytes .../repo-commit-template/dot-git/info/exclude | 6 ------ .../repo-commit-template/dot-git/logs/HEAD | 2 -- .../dot-git/logs/refs/heads/master | 2 -- .../25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99 | Bin 19 -> 0 bytes .../3f/af8f2e3b6247c9d7e86d3849612cba95e94af4 | Bin 179 -> 0 bytes .../4b/825dc642cb6eb9a060e54bf8d69288fbee4904 | Bin 15 -> 0 bytes .../57/16ca5987cbf97d6bb54920bea6adde242d87e6 | Bin 19 -> 0 bytes .../76/018072e09c5d31c8c6e3113b8aa0fe625195ca | Bin 19 -> 0 bytes .../d4/d73b42443436ee23eebd43174645bb4b9eaf31 | 2 -- .../ea/e629e0574add4da0f67056fd3ec128c18b2c47 | Bin 102 -> 0 bytes .../ef/608d1399c3c033cff3f9f4a59fd77ae1f38594 | Bin 151 -> 0 bytes .../f9/14a98b83588057c160f6a29b8c0a0f54e44548 | Bin 50 -> 0 bytes .../dot-git/refs/heads/master | 1 - test/fixtures/repo-commit-template/gitmessage.txt | 1 - test/fixtures/repo-commit-template/subdir-1/a.txt | 1 - test/fixtures/repo-commit-template/subdir-1/b.txt | 1 - test/fixtures/repo-commit-template/subdir-1/c.txt | 1 - 24 files changed, 31 deletions(-) delete mode 100644 test/fixtures/repo-commit-template/a.txt delete mode 100644 test/fixtures/repo-commit-template/b.txt delete mode 100644 test/fixtures/repo-commit-template/c.txt delete mode 100644 test/fixtures/repo-commit-template/dot-git/HEAD delete mode 100644 test/fixtures/repo-commit-template/dot-git/config delete mode 100644 test/fixtures/repo-commit-template/dot-git/description delete mode 100644 test/fixtures/repo-commit-template/dot-git/index delete mode 100644 test/fixtures/repo-commit-template/dot-git/info/exclude delete mode 100644 test/fixtures/repo-commit-template/dot-git/logs/HEAD delete mode 100644 test/fixtures/repo-commit-template/dot-git/logs/refs/heads/master delete mode 100644 test/fixtures/repo-commit-template/dot-git/objects/25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99 delete mode 100644 test/fixtures/repo-commit-template/dot-git/objects/3f/af8f2e3b6247c9d7e86d3849612cba95e94af4 delete mode 100644 test/fixtures/repo-commit-template/dot-git/objects/4b/825dc642cb6eb9a060e54bf8d69288fbee4904 delete mode 100644 test/fixtures/repo-commit-template/dot-git/objects/57/16ca5987cbf97d6bb54920bea6adde242d87e6 delete mode 100644 test/fixtures/repo-commit-template/dot-git/objects/76/018072e09c5d31c8c6e3113b8aa0fe625195ca delete mode 100644 test/fixtures/repo-commit-template/dot-git/objects/d4/d73b42443436ee23eebd43174645bb4b9eaf31 delete mode 100644 test/fixtures/repo-commit-template/dot-git/objects/ea/e629e0574add4da0f67056fd3ec128c18b2c47 delete mode 100644 test/fixtures/repo-commit-template/dot-git/objects/ef/608d1399c3c033cff3f9f4a59fd77ae1f38594 delete mode 100644 test/fixtures/repo-commit-template/dot-git/objects/f9/14a98b83588057c160f6a29b8c0a0f54e44548 delete mode 100644 test/fixtures/repo-commit-template/dot-git/refs/heads/master delete mode 100644 test/fixtures/repo-commit-template/gitmessage.txt delete mode 100644 test/fixtures/repo-commit-template/subdir-1/a.txt delete mode 100644 test/fixtures/repo-commit-template/subdir-1/b.txt delete mode 100644 test/fixtures/repo-commit-template/subdir-1/c.txt diff --git a/test/fixtures/repo-commit-template/a.txt b/test/fixtures/repo-commit-template/a.txt deleted file mode 100644 index 257cc5642c..0000000000 --- a/test/fixtures/repo-commit-template/a.txt +++ /dev/null @@ -1 +0,0 @@ -foo diff --git a/test/fixtures/repo-commit-template/b.txt b/test/fixtures/repo-commit-template/b.txt deleted file mode 100644 index 5716ca5987..0000000000 --- a/test/fixtures/repo-commit-template/b.txt +++ /dev/null @@ -1 +0,0 @@ -bar diff --git a/test/fixtures/repo-commit-template/c.txt b/test/fixtures/repo-commit-template/c.txt deleted file mode 100644 index 76018072e0..0000000000 --- a/test/fixtures/repo-commit-template/c.txt +++ /dev/null @@ -1 +0,0 @@ -baz diff --git a/test/fixtures/repo-commit-template/dot-git/HEAD b/test/fixtures/repo-commit-template/dot-git/HEAD deleted file mode 100644 index cb089cd89a..0000000000 --- a/test/fixtures/repo-commit-template/dot-git/HEAD +++ /dev/null @@ -1 +0,0 @@ -ref: refs/heads/master diff --git a/test/fixtures/repo-commit-template/dot-git/config b/test/fixtures/repo-commit-template/dot-git/config deleted file mode 100644 index 52b216917a..0000000000 --- a/test/fixtures/repo-commit-template/dot-git/config +++ /dev/null @@ -1,9 +0,0 @@ -[core] - repositoryformatversion = 0 - filemode = true - bare = false - logallrefupdates = true - ignorecase = true - precomposeunicode = true -[commit] - template = gitmessage.txt diff --git a/test/fixtures/repo-commit-template/dot-git/description b/test/fixtures/repo-commit-template/dot-git/description deleted file mode 100644 index 498b267a8c..0000000000 --- a/test/fixtures/repo-commit-template/dot-git/description +++ /dev/null @@ -1 +0,0 @@ -Unnamed repository; edit this file 'description' to name the repository. diff --git a/test/fixtures/repo-commit-template/dot-git/index b/test/fixtures/repo-commit-template/dot-git/index deleted file mode 100644 index 064b3f3adc4f9d0fc3c482a70cba225911936212..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 634 zcmZ?q402{*U|<4b_UO&Q%Y@^<9)i(~3=Av`wqHva7#f!_Ffe`vsu2NV7S)=gDLNY$ zgnZ~ZVXr&IF6^(}zL^ZHiFzd!K&3zc)}O7U2&19ql%ksxE_N!i{q)b;?5&;(`|E#?bbH1q(We#6@W=U>padBdLD$GETdqm}?yoJ$FcdbP?4{JE_6_+NZ zWESZf>cayJY>t-jH5d&wZymaMSi=ip9z5W{=9vBa45Ojutw%QxYq%lIg9l(pkgF>& zTCEt&6%4uFC-e%>Jbb|T{O6xvmd?Lk_3(4+6quJ7j1>&HUOm%%5bkx?cfq#;V9GeC eaj;v*ecSuJ?D7jr_FNVZiJvO}^4Q|L0U7{%+uH#E diff --git a/test/fixtures/repo-commit-template/dot-git/info/exclude b/test/fixtures/repo-commit-template/dot-git/info/exclude deleted file mode 100644 index a5196d1be8..0000000000 --- a/test/fixtures/repo-commit-template/dot-git/info/exclude +++ /dev/null @@ -1,6 +0,0 @@ -# git ls-files --others --exclude-from=.git/info/exclude -# Lines that start with '#' are comments. -# For a project mostly in C, the following would be a good set of -# exclude patterns (uncomment them if you want to use them): -# *.[oa] -# *~ diff --git a/test/fixtures/repo-commit-template/dot-git/logs/HEAD b/test/fixtures/repo-commit-template/dot-git/logs/HEAD deleted file mode 100644 index 678e774168..0000000000 --- a/test/fixtures/repo-commit-template/dot-git/logs/HEAD +++ /dev/null @@ -1,2 +0,0 @@ -0000000000000000000000000000000000000000 d4d73b42443436ee23eebd43174645bb4b9eaf31 Gaurav Chikhale 1538479109 +0530 commit (initial): Initial commit -d4d73b42443436ee23eebd43174645bb4b9eaf31 3faf8f2e3b6247c9d7e86d3849612cba95e94af4 Gaurav Chikhale 1538479155 +0530 commit: Add git message diff --git a/test/fixtures/repo-commit-template/dot-git/logs/refs/heads/master b/test/fixtures/repo-commit-template/dot-git/logs/refs/heads/master deleted file mode 100644 index 678e774168..0000000000 --- a/test/fixtures/repo-commit-template/dot-git/logs/refs/heads/master +++ /dev/null @@ -1,2 +0,0 @@ -0000000000000000000000000000000000000000 d4d73b42443436ee23eebd43174645bb4b9eaf31 Gaurav Chikhale 1538479109 +0530 commit (initial): Initial commit -d4d73b42443436ee23eebd43174645bb4b9eaf31 3faf8f2e3b6247c9d7e86d3849612cba95e94af4 Gaurav Chikhale 1538479155 +0530 commit: Add git message diff --git a/test/fixtures/repo-commit-template/dot-git/objects/25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99 b/test/fixtures/repo-commit-template/dot-git/objects/25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99 deleted file mode 100644 index bdcf704c9e663f3a11b3146b1b455bc2581b4761..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 19 acmb^)#C@5-_ugq9xq9V=;Nnfiq+2m1Fl`G#EQFf(CW zmZ?J54cP;pr6gc%1 I09Vl}{eDC-*#H0l diff --git a/test/fixtures/repo-commit-template/dot-git/objects/ef/608d1399c3c033cff3f9f4a59fd77ae1f38594 b/test/fixtures/repo-commit-template/dot-git/objects/ef/608d1399c3c033cff3f9f4a59fd77ae1f38594 deleted file mode 100644 index a90cc1ccb50a8c244093ae8c44b4903ad0394260..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 151 zcmV;I0BHYs0V^p=O;s>7H)Aj~FfcPQQApG)sVHGktvQ;avvEPlhn^Gmx>M}J{@U%E z3005;RuC?BDzg3b&)V#*o(lVxt-YtB+x`ryAQ`NnjIp8U!JJsb6UQD4T6Zn@mlQbl z6jVWaW=U>padBdLDo&Lq20)-tT$+@US)^;o@amc7gK)3Az6-t;0G)DB<6yUrI{@-l FOLuJZM@Ikv diff --git a/test/fixtures/repo-commit-template/dot-git/objects/f9/14a98b83588057c160f6a29b8c0a0f54e44548 b/test/fixtures/repo-commit-template/dot-git/objects/f9/14a98b83588057c160f6a29b8c0a0f54e44548 deleted file mode 100644 index 6e37521f9b2536eebd27c18ce54b0f62bd26e992..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 50 zcmV-20L}k+0V^p=O;s>9WiT-S0)^tzq?F7eT| Date: Wed, 24 Oct 2018 19:10:51 -0700 Subject: [PATCH 24/44] Clean up cloneRepository We don't want to be setting up a commit template for every single test that runs. We do it locally for the tests that need it --- test/helpers.js | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/test/helpers.js b/test/helpers.js index 1d4c707324..a8fd009e1a 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -42,17 +42,9 @@ export async function cloneRepository(repoName = 'three-files') { if (!cachedClonedRepos[repoName]) { const cachedPath = temp.mkdirSync('git-fixture-cache-'); const git = new GitShellOutStrategy(cachedPath); - const repoPath = path.join(__dirname, 'fixtures', `repo-${repoName}`, 'dot-git'); - - let templatePath = ''; - try { - templatePath = await git.exec(['config', '--file', repoPath + '/config', 'commit.template']); - } catch (err) {} - - await git.clone(repoPath, {noLocal: true}); + await git.clone(path.join(__dirname, 'fixtures', `repo-${repoName}`, 'dot-git'), {noLocal: true}); await git.exec(['config', '--local', 'core.autocrlf', 'false']); await git.exec(['config', '--local', 'commit.gpgsign', 'false']); - await git.exec(['config', '--local', 'commit.template', templatePath]); await git.exec(['config', '--local', 'user.email', FAKE_USER.email]); await git.exec(['config', '--local', 'user.name', FAKE_USER.name]); await git.exec(['config', '--local', 'push.default', 'simple']); From d1450845752233339df1beab1258879f6bc27e8a Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 24 Oct 2018 19:30:09 -0700 Subject: [PATCH 25/44] :fire: comment Co-Authored-By: Matt --- lib/git-shell-out-strategy.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index d1399a16fa..a7564d354d 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -426,7 +426,6 @@ export default class GitShellOutStrategy { * https://git-scm.com/docs/git-config#git-config-pathname */ const homeDir = os.homedir(); - // TODO: figure out what this is doing: const regex = new RegExp('^~([^/]*)/'); templatePath = templatePath.trim().replace(regex, (_, user) => { return `${user ? path.join(path.dirname(homeDir), user) : homeDir}/`; @@ -439,8 +438,7 @@ export default class GitShellOutStrategy { if (!await fileExists(templatePath)) { throw new Error(`Invalid commit template path set in Git config: ${templatePath}`); } - const template = await fs.readFile(templatePath, {encoding: 'utf8'}); - return template; + return await fs.readFile(templatePath, {encoding: 'utf8'}); } unstageFiles(paths, commit = 'HEAD') { From 8a40a74d9fdd7c0cc284dec8a30b8ba84208fb7e Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 25 Oct 2018 11:21:31 -0700 Subject: [PATCH 26/44] Fix and clean up GitStrategies tests --- test/git-strategies.test.js | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/test/git-strategies.test.js b/test/git-strategies.test.js index 4d61ba4b94..f981b6ad43 100644 --- a/test/git-strategies.test.js +++ b/test/git-strategies.test.js @@ -103,7 +103,7 @@ import * as reporterProxy from '../lib/reporter-proxy'; const workingDirPath = await cloneRepository('three-files'); const git = createTestStrategy(workingDirPath); - assert.strictEqual(await git.getConfig('commit.template'), ''); + assert.isNotOk(await git.getConfig('commit.template')); // falsy value of null or '' assert.isNull(await git.getCommitMessageTemplate()); }); @@ -118,14 +118,6 @@ import * as reporterProxy from '../lib/reporter-proxy'; `Invalid commit template path set in Git config: ${nonExistentCommitTemplatePath}`, ); }); - - it('test that ~ gets expanded correctly', async function() { - - }); - - it('test that relative path in repo is covered', async function() { - - }); }); if (process.platform === 'win32') { From 63ffa6960305008ff121f02900e4a175adc7e89b Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Thu, 25 Oct 2018 15:07:30 -0700 Subject: [PATCH 27/44] add comment explaining regex Co-Authored-By: Katrina Uychaco --- lib/git-shell-out-strategy.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index a7564d354d..49e134972d 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -426,8 +426,15 @@ export default class GitShellOutStrategy { * https://git-scm.com/docs/git-config#git-config-pathname */ const homeDir = os.homedir(); + // this regex attempts to get the specified user's home directory + // Ex: on Mac ~kuychaco/ is expanded to the specified user’s home directory (/Users/kuychaco) + + // Regex translation: + // ^~ line starts with tilde + // ([^/]*)/ captures non-forwardslash characters before first slash const regex = new RegExp('^~([^/]*)/'); templatePath = templatePath.trim().replace(regex, (_, user) => { + // if no user is specified, fall back to using the home directory. return `${user ? path.join(path.dirname(homeDir), user) : homeDir}/`; }); From c15f1d025b541f33a5ede74cc254b211a08f7570 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Thu, 25 Oct 2018 15:21:17 -0700 Subject: [PATCH 28/44] cleanup some dead code Co-Authored-By: Katrina Uychaco --- lib/models/repository-states/present.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/models/repository-states/present.js b/lib/models/repository-states/present.js index a574e83138..477575f22e 100644 --- a/lib/models/repository-states/present.js +++ b/lib/models/repository-states/present.js @@ -188,13 +188,11 @@ export default class Present extends State { if (endsWith('.git', 'MERGE_HEAD')) { if (event.action === 'created') { - if (this.isCommitMessageClean()) { // is it really necessary to check if commit message is clean? + if (this.isCommitMessageClean()) { this.setCommitMessage(await this.repository.getMergeMessage()); - // this.didUpdate(); } } else if (event.action === 'deleted') { this.setCommitMessage(this.commitMessageTemplate || ''); - // this.didUpdate(); } } @@ -204,7 +202,6 @@ export default class Present extends State { if (this.commitMessageTemplate !== template) { this.setCommitMessageTemplate(template); this.setCommitMessage(template); - // this.didUpdate(); } } } From fd1d1cfcbbe328ab103e4828a9c85d15616dfa3b Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Thu, 25 Oct 2018 15:38:27 -0700 Subject: [PATCH 29/44] add comment about isValidMessage regex Co-Authored-By: Katrina Uychaco --- lib/views/commit-view.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/views/commit-view.js b/lib/views/commit-view.js index 889da4cddc..63e4a8a479 100644 --- a/lib/views/commit-view.js +++ b/lib/views/commit-view.js @@ -469,6 +469,8 @@ export default class CommitView extends React.Component { } isValidMessage() { + // ensure that there are at least some non-comment lines in the commit message. + // Commented lines are stripped out of commit messages by git, by default configuration. return this.editor && this.editor.getText().replace(/^#.*$/gm, '').trim().length !== 0; } From 3d9440549abdd45345caf1e3cf48a495b105c6b8 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Thu, 25 Oct 2018 15:47:39 -0700 Subject: [PATCH 30/44] :fire: unused dependencies. Co-Authored-By: Katrina Uychaco --- test/git-strategies.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/git-strategies.test.js b/test/git-strategies.test.js index f981b6ad43..9d0ccce2f4 100644 --- a/test/git-strategies.test.js +++ b/test/git-strategies.test.js @@ -1,7 +1,6 @@ import fs from 'fs-extra'; import path from 'path'; import http from 'http'; -import os from 'os'; import mkdirp from 'mkdirp'; import dedent from 'dedent-js'; From 9b606ba232c7f3f0427c4e423148cb940db60dfa Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Thu, 25 Oct 2018 17:24:55 -0700 Subject: [PATCH 31/44] test merged changes + cleanup Co-Authored-By: Katrina Uychaco --- lib/controllers/commit-controller.js | 1 - lib/views/commit-view.js | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/controllers/commit-controller.js b/lib/controllers/commit-controller.js index 4220d97de5..db88bc8fe8 100644 --- a/lib/controllers/commit-controller.js +++ b/lib/controllers/commit-controller.js @@ -110,7 +110,6 @@ export default class CommitController extends React.Component { } componentDidUpdate(prevProps) { - // todo: test that this still works as expected this.commitMessageBuffer.setTextViaDiff(this.getCommitMessage()); } diff --git a/lib/views/commit-view.js b/lib/views/commit-view.js index ddfc7f3e62..e7835fa98c 100644 --- a/lib/views/commit-view.js +++ b/lib/views/commit-view.js @@ -463,8 +463,7 @@ export default class CommitView extends React.Component { isValidMessage() { // ensure that there are at least some non-comment lines in the commit message. // Commented lines are stripped out of commit messages by git, by default configuration. - // todo: test this! - return this.props.messageBuffer && this.props.messageBuffer.getText().replace(/^#.*$/gm, '').trim().length !== 0; + return this.props.messageBuffer.getText().replace(/^#.*$/gm, '').trim().length !== 0; } commitIsEnabled(amend) { From 6379f30b78a221c2650801a1b83f713e307f491e Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Thu, 25 Oct 2018 17:52:34 -0700 Subject: [PATCH 32/44] fix issue with setting cursor buffer position This only works for commit message templates where every line starts with a #. We explored using isCommitMessageClean to try and handle commit message templates that have uncommented lines. We're not calling didUpdate every time the commit message is updated, so it's out of sync, and we decided it was getting too edge casey out there. Co-Authored-By: Katrina Uychaco --- lib/views/commit-view.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/views/commit-view.js b/lib/views/commit-view.js index e7835fa98c..5b0560c589 100644 --- a/lib/views/commit-view.js +++ b/lib/views/commit-view.js @@ -588,8 +588,7 @@ export default class CommitView extends React.Component { if (this.props.messageBuffer.getText().length > 0 && !this.isValidMessage()) { // there is likely a commit message template present // we want the cursor to be at the beginning, not at the and of the template - // todo: make sure this actually works - this.refEditorComponent.get().setCursorBufferPosition([0, 0]); + this.refEditorComponent.get().getModel().setCursorBufferPosition([0, 0]); } return true; } From d33137223362f3c7b51c36baaa25e57ebb364602 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Fri, 26 Oct 2018 11:08:32 -0700 Subject: [PATCH 33/44] Retry flakey file patch test --- test/integration/file-patch.test.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/integration/file-patch.test.js b/test/integration/file-patch.test.js index bbc627e85c..7341d3fcfb 100644 --- a/test/integration/file-patch.test.js +++ b/test/integration/file-patch.test.js @@ -732,6 +732,8 @@ describe('integration: file patches', function() { }); it('may be partially staged', async function() { + this.retries(5); // FLAKE + getPatchEditor('unstaged', 'sample.js').setSelectedBufferRanges([ [[2, 0], [2, 0]], [[10, 0], [10, 0]], From 13e7342a93a446443917c86fe6fcc9b5361663d7 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Mon, 29 Oct 2018 15:34:26 -0700 Subject: [PATCH 34/44] extract regex and give it a better name Co-Authored-By: Katrina Uychaco --- lib/git-shell-out-strategy.js | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index a2ae737a09..ffd642341c 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -53,6 +53,17 @@ const DISABLE_COLOR_FLAGS = [ return acc; }, []); +/** + * Expand config path name per + * https://git-scm.com/docs/git-config#git-config-pathname + * this regex attempts to get the specified user's home directory + * Ex: on Mac ~kuychaco/ is expanded to the specified user’s home directory (/Users/kuychaco) + * Regex translation: + * ^~ line starts with tilde + * ([^/]*)/ captures non-forwardslash characters before first slash + */ +const EXPAND_TILDE_REGEX = new RegExp('^~([^/]*)/'); + export default class GitShellOutStrategy { static defaultExecArgs = { stdin: null, @@ -428,19 +439,9 @@ export default class GitShellOutStrategy { return null; } - /** - * Get absolute path from git path - * https://git-scm.com/docs/git-config#git-config-pathname - */ const homeDir = os.homedir(); - // this regex attempts to get the specified user's home directory - // Ex: on Mac ~kuychaco/ is expanded to the specified user’s home directory (/Users/kuychaco) - - // Regex translation: - // ^~ line starts with tilde - // ([^/]*)/ captures non-forwardslash characters before first slash - const regex = new RegExp('^~([^/]*)/'); - templatePath = templatePath.trim().replace(regex, (_, user) => { + + templatePath = templatePath.trim().replace(EXPAND_TILDE_REGEX, (_, user) => { // if no user is specified, fall back to using the home directory. return `${user ? path.join(path.dirname(homeDir), user) : homeDir}/`; }); From 218f193541b7df0c746cb239da3e5982173c442e Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Mon, 29 Oct 2018 15:51:04 -0700 Subject: [PATCH 35/44] rename getCommitMessageTemplate to fetchCommitMessageTemplate Co-Authored-By: Katrina Uychaco --- lib/get-repo-pipeline-manager.js | 2 +- lib/git-shell-out-strategy.js | 2 +- lib/models/repository-states/present.js | 8 ++++---- lib/models/repository-states/state.js | 4 ++-- lib/models/repository.js | 2 +- test/git-strategies.test.js | 8 ++++---- 6 files changed, 13 insertions(+), 13 deletions(-) diff --git a/lib/get-repo-pipeline-manager.js b/lib/get-repo-pipeline-manager.js index b153920ddc..365fc8dfd1 100644 --- a/lib/get-repo-pipeline-manager.js +++ b/lib/get-repo-pipeline-manager.js @@ -210,7 +210,7 @@ export default function({confirm, notificationManager, workspace}) { commitPipeline.addMiddleware('failed-to-commit-error', async (next, repository) => { try { const result = await next(); - const template = await repository.getCommitMessageTemplate(); + const template = await repository.fetchCommitMessageTemplate(); repository.setCommitMessage(template || ''); destroyFilePatchPaneItems({onlyStaged: true}, workspace); return result; diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index ffd642341c..f88979db98 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -433,7 +433,7 @@ export default class GitShellOutStrategy { return this.exec(args, {writeOperation: true}); } - async getCommitMessageTemplate() { + async fetchCommitMessageTemplate() { let templatePath = await this.getConfig('commit.template'); if (!templatePath) { return null; diff --git a/lib/models/repository-states/present.js b/lib/models/repository-states/present.js index 14af4ae5ac..2aa24b3449 100644 --- a/lib/models/repository-states/present.js +++ b/lib/models/repository-states/present.js @@ -60,7 +60,7 @@ export default class Present extends State { async fetchInitialMessage() { const mergeMessage = await this.repository.getMergeMessage(); - const template = await this.getCommitMessageTemplate(); + const template = await this.fetchCommitMessageTemplate(); if (template) { this.commitMessageTemplate = template; } @@ -75,8 +75,8 @@ export default class Present extends State { return this.commitMessage; } - getCommitMessageTemplate() { - return this.git().getCommitMessageTemplate(); + fetchCommitMessageTemplate() { + return this.git().fetchCommitMessageTemplate(); } getOperationStates() { @@ -197,7 +197,7 @@ export default class Present extends State { if (endsWith('.git', 'config')) { // this won't catch changes made to the template file itself... - const template = await this.getCommitMessageTemplate(); + const template = await this.fetchCommitMessageTemplate(); if (this.commitMessageTemplate !== template) { this.setCommitMessageTemplate(template); this.setCommitMessage(template); diff --git a/lib/models/repository-states/state.js b/lib/models/repository-states/state.js index 74d9a14c19..7e3e88988f 100644 --- a/lib/models/repository-states/state.js +++ b/lib/models/repository-states/state.js @@ -374,8 +374,8 @@ export default class State { return ''; } - getCommitMessageTemplate() { - return unsupportedOperationPromise(this, 'getCommitMessageTemplate'); + fetchCommitMessageTemplate() { + return unsupportedOperationPromise(this, 'fetchCommitMessageTemplate'); } // Cache diff --git a/lib/models/repository.js b/lib/models/repository.js index ef54885474..8dc10b022a 100644 --- a/lib/models/repository.js +++ b/lib/models/repository.js @@ -358,7 +358,7 @@ const delegates = [ 'setCommitMessage', 'getCommitMessage', - 'getCommitMessageTemplate', + 'fetchCommitMessageTemplate', 'getCache', ]; diff --git a/test/git-strategies.test.js b/test/git-strategies.test.js index 9d0ccce2f4..dbb4ab3536 100644 --- a/test/git-strategies.test.js +++ b/test/git-strategies.test.js @@ -85,7 +85,7 @@ import * as reporterProxy from '../lib/reporter-proxy'; }); }); - describe('getCommitMessageTemplate', function() { + describe('fetchCommitMessageTemplate', function() { it('gets commit message from template', async function() { const workingDirPath = await cloneRepository('three-files'); const git = createTestStrategy(workingDirPath); @@ -95,7 +95,7 @@ import * as reporterProxy from '../lib/reporter-proxy'; await fs.writeFile(commitMsgTemplatePath, templateText, {encoding: 'utf8'}); await git.setConfig('commit.template', commitMsgTemplatePath); - assert.equal(await git.getCommitMessageTemplate(), templateText); + assert.equal(await git.fetchCommitMessageTemplate(), templateText); }); it('if config is not set return null', async function() { @@ -103,7 +103,7 @@ import * as reporterProxy from '../lib/reporter-proxy'; const git = createTestStrategy(workingDirPath); assert.isNotOk(await git.getConfig('commit.template')); // falsy value of null or '' - assert.isNull(await git.getCommitMessageTemplate()); + assert.isNull(await git.fetchCommitMessageTemplate()); }); it('if config is set but file does not exist throw an error', async function() { @@ -113,7 +113,7 @@ import * as reporterProxy from '../lib/reporter-proxy'; const nonExistentCommitTemplatePath = path.join(workingDirPath, 'file-that-doesnt-exist'); await git.setConfig('commit.template', nonExistentCommitTemplatePath); await assert.isRejected( - git.getCommitMessageTemplate(), + git.fetchCommitMessageTemplate(), `Invalid commit template path set in Git config: ${nonExistentCommitTemplatePath}`, ); }); From e2cc423d253fb785bf318954c79fa486b250b38c Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Mon, 29 Oct 2018 16:07:20 -0700 Subject: [PATCH 36/44] extract filePathEndsWith into helper function --- lib/helpers.js | 3 +++ lib/models/repository-states/present.js | 13 ++++++------- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/helpers.js b/lib/helpers.js index 37f1854de1..def4f8d70c 100644 --- a/lib/helpers.js +++ b/lib/helpers.js @@ -320,6 +320,9 @@ export function toGitPathSep(rawPath) { } } +export function filePathEndsWith(filePath, ...segments) { + return filePath.endsWith(path.join(...segments)); +} /** * Turns an array of things @kuychaco cannot eat diff --git a/lib/models/repository-states/present.js b/lib/models/repository-states/present.js index 2aa24b3449..7cd9e196c3 100644 --- a/lib/models/repository-states/present.js +++ b/lib/models/repository-states/present.js @@ -16,6 +16,7 @@ import RemoteSet from '../remote-set'; import Commit from '../commit'; import OperationStates from '../operation-states'; import {addEvent} from '../../reporter-proxy'; +import {filePathEndsWith} from '../../helpers'; /** * State used when the working directory contains a valid git repository and can be interacted with. Performs @@ -115,10 +116,9 @@ export default class Present extends State { continue; } - const endsWith = (...segments) => fullPath.endsWith(path.join(...segments)); const includes = (...segments) => fullPath.includes(path.join(...segments)); - if (endsWith('.git', 'index')) { + if (filePathEndsWith(fullPath, '.git', 'index')) { keys.add(Keys.stagedChangesSinceParentCommit); keys.add(Keys.filePatch.all); keys.add(Keys.index.all); @@ -126,7 +126,7 @@ export default class Present extends State { continue; } - if (endsWith('.git', 'HEAD')) { + if (filePathEndsWith(fullPath, '.git', 'HEAD')) { keys.add(Keys.branches); keys.add(Keys.lastCommit); keys.add(Keys.recentCommits); @@ -152,7 +152,7 @@ export default class Present extends State { continue; } - if (endsWith('.git', 'config')) { + if (filePathEndsWith(fullPath, '.git', 'config')) { keys.add(Keys.remotes); keys.add(Keys.config.all); keys.add(Keys.statusBundle); @@ -183,9 +183,8 @@ export default class Present extends State { async updateCommitMessageAfterFileSystemChange(events) { for (let i = 0; i < events.length; i++) { const event = events[i]; - const endsWith = (...segments) => event.path.endsWith(path.join(...segments)); - if (endsWith('.git', 'MERGE_HEAD')) { + if (filePathEndsWith(event.path, '.git', 'MERGE_HEAD')) { if (event.action === 'created') { if (this.isCommitMessageClean()) { this.setCommitMessage(await this.repository.getMergeMessage()); @@ -195,7 +194,7 @@ export default class Present extends State { } } - if (endsWith('.git', 'config')) { + if (filePathEndsWith(event.path, '.git', 'config')) { // this won't catch changes made to the template file itself... const template = await this.fetchCommitMessageTemplate(); if (this.commitMessageTemplate !== template) { From b3c9ec74112253b3561744fe5c3032dae0e1c234 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 29 Oct 2018 19:01:04 -0700 Subject: [PATCH 37/44] Extract wireUpObserver and expectEvents to test helpers file Co-Authored-By: Tilde Ann Thurium --- test/helpers.js | 62 +++++++++- test/models/repository.test.js | 201 +++++++++------------------------ 2 files changed, 115 insertions(+), 148 deletions(-) diff --git a/test/helpers.js b/test/helpers.js index 4c58f0475a..4ed120522a 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -8,7 +8,7 @@ import React from 'react'; import ReactDom from 'react-dom'; import sinon from 'sinon'; import {Directory} from 'atom'; -import {Emitter} from 'event-kit'; +import {Emitter, CompositeDisposable, Disposable} from 'event-kit'; import Repository from '../lib/models/repository'; import GitShellOutStrategy from '../lib/git-shell-out-strategy'; @@ -16,6 +16,7 @@ import WorkerManager from '../lib/worker-manager'; import ContextMenuInterceptor from '../lib/context-menu-interceptor'; import getRepoPipelineManager from '../lib/get-repo-pipeline-manager'; import {clearRelayExpectations} from '../lib/relay-network-layer-manager'; +import FileSystemChangeObserver from '../lib/models/file-system-change-observer'; assert.autocrlfEqual = (actual, expected, ...args) => { const newActual = actual.replace(/\r\n/g, '\n'); @@ -382,3 +383,62 @@ export class ManualStateObserver { this.emitter.dispose(); } } + + +// File system event helpers +let observedEvents, eventCallback; + +export async function wireUpObserver(fixtureName = 'multi-commits-files', existingWorkdir = null) { + observedEvents = []; + eventCallback = () => {}; + + const workdir = existingWorkdir || await cloneRepository(fixtureName); + const repository = new Repository(workdir); + await repository.getLoadPromise(); + + const observer = new FileSystemChangeObserver(repository); + + const subscriptions = new CompositeDisposable( + new Disposable(async () => { + await observer.destroy(); + repository.destroy(); + }), + ); + + subscriptions.add(observer.onDidChange(events => { + observedEvents.push(...events); + eventCallback(); + })); + + return {repository, observer, subscriptions}; +} + +export function expectEvents(repository, ...suffixes) { + const pending = new Set(suffixes); + return new Promise((resolve, reject) => { + eventCallback = () => { + const matchingPaths = observedEvents + .filter(event => { + for (const suffix of pending) { + if (event.path.endsWith(suffix)) { + pending.delete(suffix); + return true; + } + } + return false; + }); + + if (matchingPaths.length > 0) { + repository.observeFilesystemChange(matchingPaths); + } + + if (pending.size === 0) { + resolve(); + } + }; + + if (observedEvents.length > 0) { + eventCallback(); + } + }); +} diff --git a/test/models/repository.test.js b/test/models/repository.test.js index 6194cc26d9..1a60416bff 100644 --- a/test/models/repository.test.js +++ b/test/models/repository.test.js @@ -4,18 +4,16 @@ import dedent from 'dedent-js'; import temp from 'temp'; import compareSets from 'compare-sets'; import isEqual from 'lodash.isequal'; -import {CompositeDisposable, Disposable} from 'event-kit'; import Repository from '../../lib/models/repository'; import {nullCommit} from '../../lib/models/commit'; import {nullOperationStates} from '../../lib/models/operation-states'; -import FileSystemChangeObserver from '../../lib/models/file-system-change-observer'; import Author from '../../lib/models/author'; import * as reporterProxy from '../../lib/reporter-proxy'; import { cloneRepository, setUpLocalAndRemoteRepositories, getHeadCommitOnRemote, - assertDeepPropertyVals, assertEqualSortedArraysByKey, FAKE_USER, + assertDeepPropertyVals, assertEqualSortedArraysByKey, FAKE_USER, wireUpObserver, expectEvents, } from '../helpers'; import {getPackageRoot, getTempDir} from '../../lib/helpers'; @@ -1833,72 +1831,17 @@ describe('Repository', function() { }); describe('from filesystem events', function() { - let workdir, sub; - let observedEvents, eventCallback; - - async function wireUpObserver(fixtureName = 'multi-commits-files', existingWorkdir = null) { - observedEvents = []; - eventCallback = () => {}; - - workdir = existingWorkdir || await cloneRepository(fixtureName); - const repository = new Repository(workdir); - await repository.getLoadPromise(); - - const observer = new FileSystemChangeObserver(repository); - - sub = new CompositeDisposable( - new Disposable(async () => { - await observer.destroy(); - repository.destroy(); - }), - ); - - sub.add(observer.onDidChange(events => { - observedEvents.push(...events); - eventCallback(); - })); - - return {repository, observer}; - } - - function expectEvents(repository, ...suffixes) { - const pending = new Set(suffixes); - return new Promise((resolve, reject) => { - eventCallback = () => { - const matchingPaths = observedEvents - .filter(event => { - for (const suffix of pending) { - if (event.path.endsWith(suffix)) { - pending.delete(suffix); - return true; - } - } - return false; - }); - - if (matchingPaths.length > 0) { - repository.observeFilesystemChange(matchingPaths); - } - - if (pending.size === 0) { - resolve(); - } - }; - - if (observedEvents.length > 0) { - eventCallback(); - } - }); - } + let sub; afterEach(function() { sub && sub.dispose(); }); it('when staging files', async function() { - const {repository, observer} = await wireUpObserver(); + const {repository, observer, subscriptions} = await wireUpObserver(); + sub = subscriptions; - await fs.writeFile(path.join(workdir, 'a.txt'), 'boop\n', {encoding: 'utf8'}); + await fs.writeFile(path.join(repository.getWorkingDirectoryPath(), 'a.txt'), 'boop\n', {encoding: 'utf8'}); await assertCorrectInvalidation({repository}, async () => { await observer.start(); @@ -1908,9 +1851,10 @@ describe('Repository', function() { }); it('when unstaging files', async function() { - const {repository, observer} = await wireUpObserver(); + const {repository, observer, subscriptions} = await wireUpObserver(); + sub = subscriptions; - await fs.writeFile(path.join(workdir, 'a.txt'), 'boop\n', {encoding: 'utf8'}); + await fs.writeFile(path.join(repository.getWorkingDirectoryPath(), 'a.txt'), 'boop\n', {encoding: 'utf8'}); await repository.git.stageFiles(['a.txt']); await assertCorrectInvalidation({repository}, async () => { @@ -1921,7 +1865,8 @@ describe('Repository', function() { }); it('when staging files from a parent commit', async function() { - const {repository, observer} = await wireUpObserver(); + const {repository, observer, subscriptions} = await wireUpObserver(); + sub = subscriptions; await assertCorrectInvalidation({repository}, async () => { await observer.start(); @@ -1931,9 +1876,10 @@ describe('Repository', function() { }); it('when applying a patch to the index', async function() { - const {repository, observer} = await wireUpObserver(); + const {repository, observer, subscriptions} = await wireUpObserver(); + sub = subscriptions; - await fs.writeFile(path.join(workdir, 'a.txt'), 'boop\n', {encoding: 'utf8'}); + await fs.writeFile(path.join(repository.getWorkingDirectoryPath(), 'a.txt'), 'boop\n', {encoding: 'utf8'}); const patch = await repository.getFilePatchForPath('a.txt'); await assertCorrectInvalidation({repository}, async () => { @@ -1947,9 +1893,10 @@ describe('Repository', function() { }); it('when applying a patch to the working directory', async function() { - const {repository, observer} = await wireUpObserver(); + const {repository, observer, subscriptions} = await wireUpObserver(); + sub = subscriptions; - await fs.writeFile(path.join(workdir, 'a.txt'), 'boop\n', {encoding: 'utf8'}); + await fs.writeFile(path.join(repository.getWorkingDirectoryPath(), 'a.txt'), 'boop\n', {encoding: 'utf8'}); const patch = (await repository.getFilePatchForPath('a.txt')).getUnstagePatchForLines(new Set([0])); await assertCorrectInvalidation({repository}, async () => { @@ -1963,9 +1910,10 @@ describe('Repository', function() { }); it('when committing', async function() { - const {repository, observer} = await wireUpObserver(); + const {repository, observer, subscriptions} = await wireUpObserver(); + sub = subscriptions; - await fs.writeFile(path.join(workdir, 'a.txt'), 'boop\n', {encoding: 'utf8'}); + await fs.writeFile(path.join(repository.getWorkingDirectoryPath(), 'a.txt'), 'boop\n', {encoding: 'utf8'}); await repository.stageFiles(['a.txt']); await assertCorrectInvalidation({repository}, async () => { @@ -1980,7 +1928,8 @@ describe('Repository', function() { }); it('when merging', async function() { - const {repository, observer} = await wireUpObserver('merge-conflict'); + const {repository, observer, subscriptions} = await wireUpObserver('merge-conflict'); + sub = subscriptions; await assertCorrectInvalidation({repository}, async () => { await observer.start(); @@ -1995,7 +1944,8 @@ describe('Repository', function() { }); it('when aborting a merge', async function() { - const {repository, observer} = await wireUpObserver('merge-conflict'); + const {repository, observer, subscriptions} = await wireUpObserver('merge-conflict'); + sub = subscriptions; await assert.isRejected(repository.merge('origin/branch')); await assertCorrectInvalidation({repository}, async () => { @@ -2011,7 +1961,8 @@ describe('Repository', function() { }); it('when checking out a revision', async function() { - const {repository, observer} = await wireUpObserver(); + const {repository, observer, subscriptions} = await wireUpObserver(); + sub = subscriptions; await assertCorrectInvalidation({repository}, async () => { await observer.start(); @@ -2027,7 +1978,8 @@ describe('Repository', function() { }); it('when checking out paths', async function() { - const {repository, observer} = await wireUpObserver(); + const {repository, observer, subscriptions} = await wireUpObserver(); + sub = subscriptions; await assertCorrectInvalidation({repository}, async () => { await observer.start(); @@ -2042,7 +1994,8 @@ describe('Repository', function() { it('when fetching', async function() { const {localRepoPath} = await setUpLocalAndRemoteRepositories({remoteAhead: true}); - const {repository, observer} = await wireUpObserver(null, localRepoPath); + const {repository, observer, subscriptions} = await wireUpObserver(null, localRepoPath); + sub = subscriptions; await repository.commit('wat', {allowEmpty: true}); await repository.commit('huh', {allowEmpty: true}); @@ -2059,7 +2012,8 @@ describe('Repository', function() { it('when pulling', async function() { const {localRepoPath} = await setUpLocalAndRemoteRepositories({remoteAhead: true}); - const {repository, observer} = await wireUpObserver(null, localRepoPath); + const {repository, observer, subscriptions} = await wireUpObserver(null, localRepoPath); + sub = subscriptions; await fs.writeFile(path.join(localRepoPath, 'file.txt'), 'one\n', {encoding: 'utf8'}); await repository.stageFiles(['file.txt']); @@ -2080,7 +2034,8 @@ describe('Repository', function() { it('when pushing', async function() { const {localRepoPath} = await setUpLocalAndRemoteRepositories(); - const {repository, observer} = await wireUpObserver(null, localRepoPath); + const {repository, observer, subscriptions} = await wireUpObserver(null, localRepoPath); + sub = subscriptions; await fs.writeFile(path.join(localRepoPath, 'new-file.txt'), 'one\n', {encoding: 'utf8'}); await repository.stageFiles(['new-file.txt']); @@ -2097,7 +2052,8 @@ describe('Repository', function() { }); it('when setting a config option', async function() { - const {repository, observer} = await wireUpObserver(); + const {repository, observer, subscriptions} = await wireUpObserver(); + sub = subscriptions; const optionNames = ['core.editor', 'color.ui']; await assertCorrectInvalidation({repository, optionNames}, async () => { @@ -2111,11 +2067,12 @@ describe('Repository', function() { }); it('when changing files in the working directory', async function() { - const {repository, observer} = await wireUpObserver(); + const {repository, observer, subscriptions} = await wireUpObserver(); + sub = subscriptions; await assertCorrectInvalidation({repository}, async () => { await observer.start(); - await fs.writeFile(path.join(workdir, 'b.txt'), 'new contents\n', {encoding: 'utf8'}); + await fs.writeFile(path.join(repository.getWorkingDirectoryPath(), 'b.txt'), 'new contents\n', {encoding: 'utf8'}); await expectEvents( repository, 'b.txt', @@ -2126,63 +2083,7 @@ describe('Repository', function() { }); describe('updating commit message', function() { - let workdir, sub; - let observedEvents, eventCallback; - - async function wireUpObserver(fixtureName = 'multi-commits-files', existingWorkdir = null) { - observedEvents = []; - eventCallback = () => {}; - - workdir = existingWorkdir || await cloneRepository(fixtureName); - const repository = new Repository(workdir); - await repository.getLoadPromise(); - - const observer = new FileSystemChangeObserver(repository); - - sub = new CompositeDisposable( - new Disposable(async () => { - await observer.destroy(); - repository.destroy(); - }), - ); - - sub.add(observer.onDidChange(events => { - observedEvents.push(...events); - eventCallback(); - })); - - return {repository, observer}; - } - - function expectEvents(repository, ...suffixes) { - const pending = new Set(suffixes); - return new Promise((resolve, reject) => { - eventCallback = () => { - const matchingPaths = observedEvents - .filter(event => { - for (const suffix of pending) { - if (event.path.endsWith(suffix)) { - pending.delete(suffix); - return true; - } - } - return false; - }); - - if (matchingPaths.length > 0) { - repository.observeFilesystemChange(matchingPaths); - } - - if (pending.size === 0) { - resolve(); - } - }; - - if (observedEvents.length > 0) { - eventCallback(); - } - }); - } + let sub; afterEach(function() { sub && sub.dispose(); @@ -2190,12 +2091,13 @@ describe('Repository', function() { describe('config commit.template change', function() { it('updates commit messages to new template', async function() { - const {repository, observer} = await wireUpObserver(); + const {repository, observer, subscriptions} = await wireUpObserver(); + sub = subscriptions; await observer.start(); assert.strictEqual(repository.getCommitMessage(), ''); - const templatePath = path.join(workdir, 'a.txt'); + const templatePath = path.join(repository.getWorkingDirectoryPath(), 'a.txt'); await repository.git.setConfig('commit.template', templatePath); await expectEvents( repository, @@ -2208,7 +2110,8 @@ describe('Repository', function() { describe('merge events', function() { describe('when commit message is empty', function() { it('merge message is set as new commit message', async function() { - const {repository, observer} = await wireUpObserver('merge-conflict'); + const {repository, observer, subscriptions} = await wireUpObserver('merge-conflict'); + sub = subscriptions; await observer.start(); assert.strictEqual(repository.getCommitMessage(), ''); @@ -2223,10 +2126,11 @@ describe('Repository', function() { describe('when commit message contains unmodified template', function() { it('merge message is set as new commit message', async function() { - const {repository, observer} = await wireUpObserver('merge-conflict'); + const {repository, observer, subscriptions} = await wireUpObserver('merge-conflict'); + sub = subscriptions; await observer.start(); - const templatePath = path.join(workdir, 'added-to-both.txt'); + const templatePath = path.join(repository.getWorkingDirectoryPath(), 'added-to-both.txt'); const templateText = fs.readFileSync(templatePath, 'utf8'); await repository.git.setConfig('commit.template', templatePath); await expectEvents( @@ -2247,7 +2151,8 @@ describe('Repository', function() { describe('when commit message is "dirty"', function() { it('leaves commit message as is', async function() { - const {repository, observer} = await wireUpObserver('merge-conflict'); + const {repository, observer, subscriptions} = await wireUpObserver('merge-conflict'); + sub = subscriptions; await observer.start(); const dirtyMessage = 'foo bar baz'; @@ -2263,7 +2168,8 @@ describe('Repository', function() { describe('when merge is aborted', function() { it('merge message gets cleared', async function() { - const {repository, observer} = await wireUpObserver('merge-conflict'); + const {repository, observer, subscriptions} = await wireUpObserver('merge-conflict'); + sub = subscriptions; await observer.start(); await assert.isRejected(repository.git.merge('origin/branch')); await expectEvents( @@ -2283,10 +2189,11 @@ describe('Repository', function() { describe('when commit message template is present', function() { it('sets template as commit message', async function() { - const {repository, observer} = await wireUpObserver('merge-conflict'); + const {repository, observer, subscriptions} = await wireUpObserver('merge-conflict'); + sub = subscriptions; await observer.start(); - const templatePath = path.join(workdir, 'added-to-both.txt'); + const templatePath = path.join(repository.getWorkingDirectoryPath(), 'added-to-both.txt'); const templateText = fs.readFileSync(templatePath, 'utf8'); await repository.git.setConfig('commit.template', templatePath); await expectEvents( From 37fbb445ac1b883c773adbaa3e4eae3bbd73377e Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 29 Oct 2018 19:01:52 -0700 Subject: [PATCH 38/44] Access local config in spec mode Tests were failing because my global config was being used to look up the commit template --- lib/git-shell-out-strategy.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index f88979db98..ef573aa9f1 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -23,6 +23,7 @@ import GitTimingsView from './views/git-timings-view'; import WorkerManager from './worker-manager'; const MAX_STATUS_OUTPUT_LENGTH = 1024 * 1024 * 10; +const SPEC_MODE = atom.inSpecMode(); let headless = null; let execPathPromise = null; @@ -182,7 +183,7 @@ export default class GitShellOutStrategy { env.ATOM_GITHUB_ORIGINAL_GIT_ASKPASS = process.env.GIT_ASKPASS || ''; env.ATOM_GITHUB_ORIGINAL_SSH_ASKPASS = process.env.SSH_ASKPASS || ''; env.ATOM_GITHUB_ORIGINAL_GIT_SSH_COMMAND = process.env.GIT_SSH_COMMAND || ''; - env.ATOM_GITHUB_SPEC_MODE = atom.inSpecMode() ? 'true' : 'false'; + env.ATOM_GITHUB_SPEC_MODE = SPEC_MODE ? 'true' : 'false'; env.SSH_ASKPASS = normalizeGitHelperPath(gitTempDir.getAskPassSh()); env.GIT_ASKPASS = normalizeGitHelperPath(gitTempDir.getAskPassSh()); @@ -923,7 +924,7 @@ export default class GitShellOutStrategy { let output; try { let args = ['config']; - if (local) { args.push('--local'); } + if (local || SPEC_MODE) { args.push('--local'); } args = args.concat(option); output = await this.exec(args); } catch (err) { From 6299ae248c7aa36eece71682e7f263fcbde0dff0 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 29 Oct 2018 19:18:56 -0700 Subject: [PATCH 39/44] Don't break snapshotting by accessing `atom` in global scope Co-Authored-By: Ash Wilson --- lib/git-shell-out-strategy.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index ef573aa9f1..7c8ceb5187 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -23,7 +23,6 @@ import GitTimingsView from './views/git-timings-view'; import WorkerManager from './worker-manager'; const MAX_STATUS_OUTPUT_LENGTH = 1024 * 1024 * 10; -const SPEC_MODE = atom.inSpecMode(); let headless = null; let execPathPromise = null; @@ -183,7 +182,7 @@ export default class GitShellOutStrategy { env.ATOM_GITHUB_ORIGINAL_GIT_ASKPASS = process.env.GIT_ASKPASS || ''; env.ATOM_GITHUB_ORIGINAL_SSH_ASKPASS = process.env.SSH_ASKPASS || ''; env.ATOM_GITHUB_ORIGINAL_GIT_SSH_COMMAND = process.env.GIT_SSH_COMMAND || ''; - env.ATOM_GITHUB_SPEC_MODE = SPEC_MODE ? 'true' : 'false'; + env.ATOM_GITHUB_SPEC_MODE = atom.inSpecMode() ? 'true' : 'false'; env.SSH_ASKPASS = normalizeGitHelperPath(gitTempDir.getAskPassSh()); env.GIT_ASKPASS = normalizeGitHelperPath(gitTempDir.getAskPassSh()); @@ -924,7 +923,7 @@ export default class GitShellOutStrategy { let output; try { let args = ['config']; - if (local || SPEC_MODE) { args.push('--local'); } + if (local || atom.inSpecMode()) { args.push('--local'); } args = args.concat(option); output = await this.exec(args); } catch (err) { From 8aaf2ac9b55eb7b3fd98b271d2f785c8f2b81ab3 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 29 Oct 2018 19:47:04 -0700 Subject: [PATCH 40/44] Test flakes --- test/integration/file-patch.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/file-patch.test.js b/test/integration/file-patch.test.js index 7341d3fcfb..2c7346ce3f 100644 --- a/test/integration/file-patch.test.js +++ b/test/integration/file-patch.test.js @@ -274,7 +274,7 @@ describe('integration: file patches', function() { await clickFileInGitTab('staged', 'added-file.txt'); }); - it('may be partially unstaged', async function() { + it.stress(10, 'may be partially unstaged', async function() { getPatchEditor('staged', 'added-file.txt').setSelectedBufferRange([[3, 0], [4, 3]]); wrapper.find('.github-HunkHeaderView-stageButton').simulate('click'); @@ -298,7 +298,7 @@ describe('integration: file patches', function() { ); }); - it('may be completely unstaged', async function() { + it.stress(10, 'may be completely unstaged', async function() { getPatchEditor('staged', 'added-file.txt').selectAll(); wrapper.find('.github-HunkHeaderView-stageButton').simulate('click'); From 3e1cb407b46200450cb985c06781e0be459eefad Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 29 Oct 2018 19:50:21 -0700 Subject: [PATCH 41/44] Retry file-patch test flakes --- test/integration/file-patch.test.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/integration/file-patch.test.js b/test/integration/file-patch.test.js index 2c7346ce3f..9bfa828c05 100644 --- a/test/integration/file-patch.test.js +++ b/test/integration/file-patch.test.js @@ -274,7 +274,8 @@ describe('integration: file patches', function() { await clickFileInGitTab('staged', 'added-file.txt'); }); - it.stress(10, 'may be partially unstaged', async function() { + it('may be partially unstaged', async function() { + this.retries(5); // FLAKE getPatchEditor('staged', 'added-file.txt').setSelectedBufferRange([[3, 0], [4, 3]]); wrapper.find('.github-HunkHeaderView-stageButton').simulate('click'); @@ -298,7 +299,8 @@ describe('integration: file patches', function() { ); }); - it.stress(10, 'may be completely unstaged', async function() { + it('may be completely unstaged', async function() { + this.retries(5); // FLAKE getPatchEditor('staged', 'added-file.txt').selectAll(); wrapper.find('.github-HunkHeaderView-stageButton').simulate('click'); From a87b3a48564d58825736ae12c09ecfef48851502 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Tue, 6 Nov 2018 13:22:14 -0800 Subject: [PATCH 42/44] handle case where template is unset --- lib/models/repository-states/present.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/models/repository-states/present.js b/lib/models/repository-states/present.js index 7cd9e196c3..76bb0fd791 100644 --- a/lib/models/repository-states/present.js +++ b/lib/models/repository-states/present.js @@ -196,7 +196,10 @@ export default class Present extends State { if (filePathEndsWith(event.path, '.git', 'config')) { // this won't catch changes made to the template file itself... - const template = await this.fetchCommitMessageTemplate(); + let template = await this.fetchCommitMessageTemplate(); + if (template === null) { + template = ''; + } if (this.commitMessageTemplate !== template) { this.setCommitMessageTemplate(template); this.setCommitMessage(template); From 6f88f76b1bd86bdc0b9e44acbc4812798006dba5 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Tue, 6 Nov 2018 13:28:39 -0800 Subject: [PATCH 43/44] add unit test for unsetting commit template --- test/models/repository.test.js | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/test/models/repository.test.js b/test/models/repository.test.js index 1a60416bff..39294c7857 100644 --- a/test/models/repository.test.js +++ b/test/models/repository.test.js @@ -2105,6 +2105,30 @@ describe('Repository', function() { ); await assert.async.strictEqual(repository.getCommitMessage(), fs.readFileSync(templatePath, 'utf8')); }); + it('updates commit message to empty string if commit.template is unset', async function() { + const {repository, observer, subscriptions} = await wireUpObserver(); + sub = subscriptions; + await observer.start(); + + assert.strictEqual(repository.getCommitMessage(), ''); + + const templatePath = path.join(repository.getWorkingDirectoryPath(), 'a.txt'); + await repository.git.setConfig('commit.template', templatePath); + await expectEvents( + repository, + path.join('.git', 'config'), + ); + + await assert.async.strictEqual(repository.getCommitMessage(), fs.readFileSync(templatePath, 'utf8')); + + await repository.git.unsetConfig('commit.template'); + + await expectEvents( + repository, + path.join('.git', 'config'), + ); + await assert.async.strictEqual(repository.getCommitMessage(), ''); + }); }); describe('merge events', function() { From 4e400bde11987eb020a54e2e2a61856bd0d3f8b7 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Tue, 6 Nov 2018 14:52:43 -0800 Subject: [PATCH 44/44] address code review feedback. --- lib/models/repository-states/present.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/models/repository-states/present.js b/lib/models/repository-states/present.js index 76bb0fd791..77c242c866 100644 --- a/lib/models/repository-states/present.js +++ b/lib/models/repository-states/present.js @@ -196,14 +196,13 @@ export default class Present extends State { if (filePathEndsWith(event.path, '.git', 'config')) { // this won't catch changes made to the template file itself... - let template = await this.fetchCommitMessageTemplate(); + const template = await this.fetchCommitMessageTemplate(); if (template === null) { - template = ''; - } - if (this.commitMessageTemplate !== template) { - this.setCommitMessageTemplate(template); + this.setCommitMessage(''); + } else if (this.commitMessageTemplate !== template) { this.setCommitMessage(template); } + this.setCommitMessageTemplate(template); } } }