From c77a9dc7129a4e056e07908aa6073ae4daaa871b Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 31 May 2018 12:56:25 -0400 Subject: [PATCH 01/11] Split the commit message wrapping test case --- test/controllers/commit-controller.test.js | 71 ++++++++++++---------- 1 file changed, 40 insertions(+), 31 deletions(-) diff --git a/test/controllers/commit-controller.test.js b/test/controllers/commit-controller.test.js index 2988d871e4..bf6e3b7871 100644 --- a/test/controllers/commit-controller.test.js +++ b/test/controllers/commit-controller.test.js @@ -159,44 +159,53 @@ describe('CommitController', function() { }); describe('message formatting', function() { - let commitSpy; + let commitSpy, wrapper; + beforeEach(function() { commitSpy = sinon.stub().returns(Promise.resolve()); app = React.cloneElement(app, {commit: commitSpy}); + wrapper = shallow(app, {disableLifecycleMethods: true}); }); - it('wraps the commit message body at 72 characters if github.automaticCommitMessageWrapping is true', async function() { - config.set('github.automaticCommitMessageWrapping', false); + describe('with automatic wrapping disabled', function() { + beforeEach(function() { + config.set('github.automaticCommitMessageWrapping', false); + }); - const wrapper = shallow(app, {disableLifecycleMethods: true}); + it('passes commit messages through unchanged', async function() { + await wrapper.instance().commit([ + 'Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor', + '', + 'Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.', + ].join('\n')); + + assert.strictEqual(commitSpy.args[0][0], [ + 'Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor', + '', + 'Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.', + ].join('\n')); + }); + }); + + describe('with automatic wrapping enabled', function() { + beforeEach(function() { + config.set('github.automaticCommitMessageWrapping', true); + }); - await wrapper.instance().commit([ - 'Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor', - '', - 'Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.', - ].join('\n')); - - assert.deepEqual(commitSpy.args[0][0].split('\n'), [ - 'Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor', - '', - 'Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.', - ]); - - commitSpy.reset(); - config.set('github.automaticCommitMessageWrapping', true); - - await wrapper.instance().commit([ - 'Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor', - '', - 'Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.', - ].join('\n')); - - assert.deepEqual(commitSpy.args[0][0].split('\n'), [ - 'Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor', - '', - 'Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ', - 'ut aliquip ex ea commodo consequat.', - ]); + it('wraps lines within the commit body at 72 characters', async function() { + await wrapper.instance().commit([ + 'Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor', + '', + 'Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.', + ].join('\n')); + + assert.strictEqual(commitSpy.args[0][0], [ + 'Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor', + '', + 'Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ', + 'ut aliquip ex ea commodo consequat.', + ].join('\n')); + }); }); }); From 915910a655a1fadcc0c440e4940c251a39d9002c Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 31 May 2018 15:32:33 -0400 Subject: [PATCH 02/11] Exploratory test that's totally passing --- test/controllers/commit-controller.test.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/controllers/commit-controller.test.js b/test/controllers/commit-controller.test.js index bf6e3b7871..6e450533f9 100644 --- a/test/controllers/commit-controller.test.js +++ b/test/controllers/commit-controller.test.js @@ -206,6 +206,11 @@ describe('CommitController', function() { 'ut aliquip ex ea commodo consequat.', ].join('\n')); }); + + it('preserves existing line wraps within the commit body', async function() { + await wrapper.instance().commit('a\n\nb\n\nc'); + assert.strictEqual(commitSpy.args[0][0], 'a\n\nb\n\nc'); + }); }); }); From 6c37c8ed575c1d5b2d7941d38cc71f9995fe6685 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 31 May 2018 15:33:10 -0400 Subject: [PATCH 03/11] Test for commit message preservation while amending --- test/git-strategies.test.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/git-strategies.test.js b/test/git-strategies.test.js index dfd69a19da..8c2399fb9a 100644 --- a/test/git-strategies.test.js +++ b/test/git-strategies.test.js @@ -791,6 +791,17 @@ import {normalizeGitHelperPath, getTempDir} from '../lib/helpers'; assert.notDeepEqual(lastCommit, amendedCommit); assert.deepEqual(lastCommitParent, amendedCommitParent); }); + + it('leaves the commit message unchanged', async function() { + const workingDirPath = await cloneRepository('multiple-commits'); + const git = createTestStrategy(workingDirPath); + await git.commit('first\n\nsecond\n\nthird', {allowEmpty: true}); + + await git.commit('', {amend: true, allowEmpty: true}); + const amendedCommit = await git.getHeadCommit(); + assert.strictEqual(amendedCommit.messageSubject, 'first'); + assert.strictEqual(amendedCommit.messageBody, 'second\n\nthird'); + }); }); }); From eb25e67013d63c9231f8a4eb4a1a921b910760d7 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 31 May 2018 15:33:39 -0400 Subject: [PATCH 04/11] Test for reading commit message bodies containing newlines --- test/git-strategies.test.js | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/test/git-strategies.test.js b/test/git-strategies.test.js index 8c2399fb9a..8a63117fe6 100644 --- a/test/git-strategies.test.js +++ b/test/git-strategies.test.js @@ -133,6 +133,7 @@ import {normalizeGitHelperPath, getTempDir} from '../lib/helpers'; assert.lengthOf(commits, 1); assert.isTrue(commits[0].unbornRef); }); + it('returns an empty array when the include unborn option is not passed', async function() { const workingDirPath = await initRepository(); const git = createTestStrategy(workingDirPath); @@ -223,6 +224,26 @@ import {normalizeGitHelperPath, getTempDir} from '../lib/helpers'; }, ]); }); + + it('preserves newlines and whitespace in the original commit body', async function() { + const workingDirPath = await cloneRepository('multiple-commits'); + const git = createTestStrategy(workingDirPath); + + await git.commit(dedent` + Implemented feature + + Detailed explanation paragraph 1 + + Detailed explanation paragraph 2 + #123 with an issue reference + `.trim(), {allowEmpty: true}); + + const commits = await git.getCommits({max: 1}); + assert.lengthOf(commits, 1); + assert.strictEqual(commits[0].messageSubject, 'Implemented feature'); + assert.strictEqual(commits[0].messageBody, + 'Detailed explanation paragraph 1\n\nDetailed explanation paragraph 2\n#123 with an issue reference'); + }); }); describe('getAuthors', function() { @@ -751,6 +772,7 @@ import {normalizeGitHelperPath, getTempDir} from '../lib/helpers'; describe('commit(message, options)', function() { describe('formatting commit message', function() { let message; + beforeEach(function() { message = [ ' Make a commit ', @@ -788,6 +810,8 @@ import {normalizeGitHelperPath, getTempDir} from '../lib/helpers'; await git.commit('amend last commit', {amend: true, allowEmpty: true}); const amendedCommit = await git.getHeadCommit(); const amendedCommitParent = await git.getCommit('HEAD~'); + + assert.strictEqual(amendedCommit.messageSubject, 'amend last commit'); assert.notDeepEqual(lastCommit, amendedCommit); assert.deepEqual(lastCommitParent, amendedCommitParent); }); From 79f3600723eba3d48eabb3ef5fc5a0f2489b3f33 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 31 May 2018 15:34:21 -0400 Subject: [PATCH 05/11] Join raw message lines with \n --- lib/helpers.js | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/helpers.js b/lib/helpers.js index 52901c3373..c9cc420648 100644 --- a/lib/helpers.js +++ b/lib/helpers.js @@ -332,19 +332,21 @@ export function destroyEmptyFilePatchPaneItems(workspace) { } export function extractCoAuthorsAndRawCommitMessage(commitMessage) { - let rawMessage = ''; - const coAuthors = commitMessage.split(LINE_ENDING_REGEX).reduce((authors, line) => { + const messageLines = []; + const coAuthors = []; + + for (const line of commitMessage.split(LINE_ENDING_REGEX)) { const match = line.match(CO_AUTHOR_REGEX); if (match) { // eslint-disable-next-line no-unused-vars const [_, name, email] = match; - authors.push({name, email}); + coAuthors.push({name, email}); } else { - rawMessage += line; + messageLines.push(line); } - return authors; - }, []); - return {message: rawMessage, coAuthors}; + } + + return {message: messageLines.join('\n'), coAuthors}; } export function createItem(node, componentHolder = null, uri = null, extra = {}) { From c190cec6b9e86544fca4cb90f0a3ef63fd77c01e Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 31 May 2018 15:35:03 -0400 Subject: [PATCH 06/11] Apply --cleanup=verbatim when amending and re-using the message --- lib/git-shell-out-strategy.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index 31cb766d1a..a58bc8f906 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -438,14 +438,18 @@ export default class GitShellOutStrategy { } async commit(rawMessage, {allowEmpty, amend, coAuthors} = {}) { - const args = ['commit', '--cleanup=strip']; - + const args = ['commit']; let msg; // if amending and no new message is passed, use last commit's message if (amend && rawMessage.length === 0) { const {unbornRef, messageBody, messageSubject} = await this.getHeadCommit(); - msg = unbornRef ? rawMessage : `${messageSubject}\n\n${messageBody}`.trim(); + if (unbornRef) { + msg = rawMessage; + } else { + msg = `${messageSubject}\n\n${messageBody}`.trim(); + args.push('--cleanup=verbatim'); + } } else { msg = rawMessage; } From 4b79bbab0fadc6641ae3127f7ce94dbc22d7e351 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 31 May 2018 15:57:10 -0400 Subject: [PATCH 07/11] Infer the --cleanup mode for git commit from config or an option --- lib/git-shell-out-strategy.js | 16 +++++++++++++--- test/git-strategies.test.js | 28 +++++++++++++++++++++++++--- 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index a58bc8f906..a5f7cb48a2 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -437,23 +437,33 @@ export default class GitShellOutStrategy { return this.exec(args, {stdin: patch, writeOperation: true}); } - async commit(rawMessage, {allowEmpty, amend, coAuthors} = {}) { + async commit(rawMessage, {allowEmpty, amend, coAuthors, verbatim} = {}) { const args = ['commit']; let msg; - // if amending and no new message is passed, use last commit's message + // if amending and no new message is passed, use last commit's message. Ensure that we don't + // mangle it in the process. if (amend && rawMessage.length === 0) { const {unbornRef, messageBody, messageSubject} = await this.getHeadCommit(); if (unbornRef) { msg = rawMessage; } else { msg = `${messageSubject}\n\n${messageBody}`.trim(); - args.push('--cleanup=verbatim'); + verbatim = true; } } else { msg = rawMessage; } + // Determine the cleanup mode. + if (verbatim) { + args.push('--cleanup=verbatim'); + } else { + const configured = await this.getConfig('commit.cleanup'); + const mode = (configured && configured !== 'default') ? configured : 'strip'; + args.push(`--cleanup=${mode}`); + } + // add co-author commit trailers if necessary if (coAuthors && coAuthors.length > 0) { msg = await this.addCoAuthorsToMessage(msg, coAuthors); diff --git a/test/git-strategies.test.js b/test/git-strategies.test.js index 8a63117fe6..c88c4d116f 100644 --- a/test/git-strategies.test.js +++ b/test/git-strategies.test.js @@ -784,7 +784,7 @@ import {normalizeGitHelperPath, getTempDir} from '../lib/helpers'; '', 'other stuff ', '', - '', + 'and things', '', ].join('\n'); }); @@ -792,12 +792,34 @@ import {normalizeGitHelperPath, getTempDir} from '../lib/helpers'; it('strips out comments and whitespace from message passed', async function() { const workingDirPath = await cloneRepository('multiple-commits'); const git = createTestStrategy(workingDirPath); + await git.setConfig('commit.cleanup', 'default'); await git.commit(message, {allowEmpty: true}); const lastCommit = await git.getHeadCommit(); - assert.deepEqual(lastCommit.messageSubject, 'Make a commit'); - assert.deepEqual(lastCommit.messageBody, 'other stuff'); + assert.strictEqual(lastCommit.messageSubject, 'Make a commit'); + assert.strictEqual(lastCommit.messageBody, 'other stuff\n\nand things'); + }); + + it('passes a message through verbatim', async function() { + const workingDirPath = await cloneRepository('multiple-commits'); + const git = createTestStrategy(workingDirPath); + await git.setConfig('commit.cleanup', 'default'); + + await git.commit(message, {allowEmpty: true, verbatim: true}); + + const lastCommit = await git.getHeadCommit(); + assert.strictEqual(lastCommit.messageSubject, 'Make a commit'); + assert.strictEqual(lastCommit.messageBody, [ + '# Comments:', + '# blah blah blah', + '', + '', + '', + 'other stuff ', + '', + 'and things', + ].join('\n')); }); }); From 3de2cceb376d2e38bc6f0becd0a25dcf3e640011 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 31 May 2018 16:05:36 -0400 Subject: [PATCH 08/11] Pass verbatim to commit() in other test cases --- test/git-strategies.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/git-strategies.test.js b/test/git-strategies.test.js index c88c4d116f..858c9c08f2 100644 --- a/test/git-strategies.test.js +++ b/test/git-strategies.test.js @@ -236,7 +236,7 @@ import {normalizeGitHelperPath, getTempDir} from '../lib/helpers'; Detailed explanation paragraph 2 #123 with an issue reference - `.trim(), {allowEmpty: true}); + `.trim(), {allowEmpty: true, verbatim: true}); const commits = await git.getCommits({max: 1}); assert.lengthOf(commits, 1); @@ -960,7 +960,7 @@ import {normalizeGitHelperPath, getTempDir} from '../lib/helpers'; command: 'commit', progressiveTense: 'committing', usesPromptServerAlready: false, - action: () => git.commit('message'), + action: () => git.commit('message', {verbatim: true}), }, { command: 'merge', From 581e675a61d6c383e905347f3c4204aa1463a275 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 1 Jun 2018 08:13:25 -0400 Subject: [PATCH 09/11] Postprocess the MERGE_MSG to ensure it works with --cleanup=verbatim --- lib/models/repository.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/models/repository.js b/lib/models/repository.js index b785b55f01..a38dda8e04 100644 --- a/lib/models/repository.js +++ b/lib/models/repository.js @@ -132,7 +132,7 @@ export default class Repository { async getMergeMessage() { try { const contents = await fs.readFile(path.join(this.getGitDirectoryPath(), 'MERGE_MSG'), {encoding: 'utf8'}); - return contents; + return contents.split(/\n/).filter(line => line.length > 0 && !line.startsWith('#')).join('\n'); } catch (e) { return null; } From b95cc7535785b510c3da64c8e197981aba16f9d9 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 1 Jun 2018 09:15:09 -0400 Subject: [PATCH 10/11] Call commit() with verbatim: true from mini editor --- lib/controllers/commit-controller.js | 8 ++++--- test/controllers/commit-controller.test.js | 28 ++++++++++++++++++---- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/lib/controllers/commit-controller.js b/lib/controllers/commit-controller.js index 240d4a618d..b0e233a0d4 100644 --- a/lib/controllers/commit-controller.js +++ b/lib/controllers/commit-controller.js @@ -122,16 +122,18 @@ export default class CommitController extends React.Component { this.subscriptions.dispose(); } - commit(message, coAuthors = [], amend) { - let msg; + commit(message, coAuthors = [], amend = false) { + let msg, verbatim; if (this.isCommitMessageEditorExpanded()) { msg = this.getCommitMessageEditors()[0].getText(); + verbatim = false; } else { const wrapMessage = this.props.config.get('github.automaticCommitMessageWrapping'); msg = wrapMessage ? wrapCommitMessage(message) : message; + verbatim = true; } - return this.props.commit(msg.trim(), {amend, coAuthors}); + return this.props.commit(msg.trim(), {amend, coAuthors, verbatim}); } setCommitMessage(message) { diff --git a/test/controllers/commit-controller.test.js b/test/controllers/commit-controller.test.js index 6e450533f9..858a8fe0fd 100644 --- a/test/controllers/commit-controller.test.js +++ b/test/controllers/commit-controller.test.js @@ -117,12 +117,12 @@ describe('CommitController', function() { }); describe('committing', function() { - let workdirPath, repository; + let workdirPath, repository, commit; beforeEach(async function() { workdirPath = await cloneRepository('three-files'); repository = await buildRepositoryWithPipeline(workdirPath, {confirm, notificationManager, workspace}); - const commit = message => repository.commit(message); + commit = sinon.stub().callsFake((...args) => repository.commit(...args)); app = React.cloneElement(app, {repository, commit}); }); @@ -139,6 +139,20 @@ describe('CommitController', function() { assert.strictEqual(repository.getCommitMessage(), ''); }); + 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', '.']); + + const wrapper = shallow(app, {disableLifecycleMethods: true}); + await wrapper.instance().commit('message\n\n#123 do some things'); + + assert.isTrue(commit.calledWith('message\n\n#123 do some things', { + amend: false, + coAuthors: [], + verbatim: true, + })); + }); + it('issues a notification on failure', async function() { repository.setCommitMessage('some message'); @@ -306,10 +320,14 @@ describe('CommitController', function() { const editor = workspace.getActiveTextEditor(); editor.setText('message in editor'); await editor.save(); - wrapper.find('CommitView').prop('commit')('message in box'); - await assert.async.strictEqual((await repository.getLastCommit()).getMessageSubject(), 'message in editor'); - await assert.async.isFalse(fs.existsSync(wrapper.instance().getCommitMessagePath())); + await wrapper.find('CommitView').prop('commit')('message in box'); + + assert.strictEqual((await repository.getLastCommit()).getMessageSubject(), 'message in editor'); + assert.isFalse(fs.existsSync(wrapper.instance().getCommitMessagePath())); + assert.isTrue(commit.calledWith('message in editor', { + amend: false, coAuthors: [], verbatim: false, + })); }); it('asks user to confirm if commit editor has unsaved changes', async function() { From 99519fdd18324d74b2c0b503f237d996d3e13ca1 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 1 Jun 2018 09:27:36 -0400 Subject: [PATCH 11/11] Fix GitTabController tests that set expectations on commit() args --- test/controllers/git-tab-controller.test.js | 25 ++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/test/controllers/git-tab-controller.test.js b/test/controllers/git-tab-controller.test.js index 2a6abbb17a..9c6dfbd12b 100644 --- a/test/controllers/git-tab-controller.test.js +++ b/test/controllers/git-tab-controller.test.js @@ -572,7 +572,10 @@ describe('GitTabController', function() { assert.strictEqual(wrapper.find('CommitView').instance().editor.getText(), ''); commandRegistry.dispatch(workspaceElement, 'github:amend-last-commit'); - await assert.async.deepEqual(repository.commit.args[0][1], {amend: true, coAuthors: []}); + await assert.async.deepEqual( + repository.commit.args[0][1], + {amend: true, coAuthors: [], verbatim: true}, + ); // amending should commit all unstaged changes await updateWrapper(repository, wrapper); @@ -594,7 +597,10 @@ describe('GitTabController', function() { assert.lengthOf(wrapper.find('GitTabView').prop('stagedChanges'), 0); commandRegistry.dispatch(workspaceElement, 'github:amend-last-commit'); - await assert.async.deepEqual(repository.commit.args[0][1], {amend: true, coAuthors: []}); + await assert.async.deepEqual( + repository.commit.args[0][1], + {amend: true, coAuthors: [], verbatim: true}, + ); await updateWrapper(repository, wrapper); // new commit message is used @@ -617,7 +623,10 @@ describe('GitTabController', function() { commandRegistry.dispatch(workspaceElement, 'github:amend-last-commit'); // verify that coAuthor was passed - await assert.async.deepEqual(repository.commit.args[0][1], {amend: true, coAuthors: [author]}); + await assert.async.deepEqual( + repository.commit.args[0][1], + {amend: true, coAuthors: [author], verbatim: true}, + ); await repository.commit.returnValues[0]; await updateWrapper(repository, wrapper); @@ -640,7 +649,10 @@ describe('GitTabController', function() { commandRegistry.dispatch(workspaceElement, 'github:amend-last-commit'); // verify that coAuthor was passed - await assert.async.deepEqual(repository.commit.args[0][1], {amend: true, coAuthors: [author]}); + await assert.async.deepEqual( + repository.commit.args[0][1], + {amend: true, coAuthors: [author], verbatim: true}, + ); await repository.commit.returnValues[0]; await updateWrapper(repository, wrapper); @@ -673,7 +685,10 @@ describe('GitTabController', function() { // amend again commandRegistry.dispatch(workspaceElement, 'github:amend-last-commit'); // verify that NO coAuthor was passed - await assert.async.deepEqual(repository.commit.args[0][1], {amend: true, coAuthors: []}); + await assert.async.deepEqual( + repository.commit.args[0][1], + {amend: true, coAuthors: [], verbatim: true}, + ); await repository.commit.returnValues[0]; await updateWrapper(repository, wrapper);