From 5428bb21f51a4f9ea68590e49520244df6c5eee6 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Thu, 10 Jan 2019 13:19:22 -0800 Subject: [PATCH 1/4] use default git cleanup configuration if commit template exists. in https://github.com/atom/github/issues/1817, there is a bug where we are not stripping commented lines if a git message template exists. The `verbatim` flag is passed if the commit is made from a mini editor. The mini editor is meant to be analagous of `git commit -m`, a quick and dirty sort of affair. As much as I don't want to add special cases here, it seemed like the cleanest to ignore `verbatim` if you have a template set. --- lib/git-shell-out-strategy.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index d8ad54d16a..40cade6d87 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -506,8 +506,12 @@ export default class GitShellOutStrategy { msg = rawMessage; } + // if commit template is used, that will not play nicely with `verbatim` + // because we want to strip commented lines from templates. + const template = await this.fetchCommitMessageTemplate(); + // Determine the cleanup mode. - if (verbatim) { + if (verbatim && !template) { args.push('--cleanup=verbatim'); } else { const configured = await this.getConfig('commit.cleanup'); From 5cf450782a87f19aca3da1c8ecb474b46deebd49 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Thu, 10 Jan 2019 13:39:43 -0800 Subject: [PATCH 2/4] add unit test to ensure that comments are stripped if commit message is used --- test/git-strategies.test.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/git-strategies.test.js b/test/git-strategies.test.js index 6dbd031fdf..9d9583b481 100644 --- a/test/git-strategies.test.js +++ b/test/git-strategies.test.js @@ -1036,6 +1036,24 @@ import * as reporterProxy from '../lib/reporter-proxy'; 'and things', ].join('\n')); }); + it('ignores verbatim flag if commit template is used', async function() { + const workingDirPath = await cloneRepository('three-files'); + const git = createTestStrategy(workingDirPath); + const templateText = '# this line should be stripped'; + + const commitMsgTemplatePath = path.join(workingDirPath, '.gitmessage'); + await fs.writeFile(commitMsgTemplatePath, templateText, {encoding: 'utf8'}); + + await git.setConfig('commit.template', commitMsgTemplatePath); + await git.setConfig('commit.cleanup', 'default'); + const commitMessage = ['this line should not be stripped', '', 'neither should this one', templateText].join('\n'); + await git.commit(commitMessage, {allowEmpty: true, verbatim: true}); + + const lastCommit = await git.getHeadCommit(); + assert.strictEqual(lastCommit.messageSubject, 'this line should not be stripped'); + // message body should not contain the template text + assert.strictEqual(lastCommit.messageBody, 'neither should this one'); + }); }); describe('when amend option is true', function() { From 0838da6ce1332af8f9ca531e5b26a3b1e8f89bd8 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Fri, 11 Jan 2019 13:15:48 -0800 Subject: [PATCH 3/4] strip commented lines instead of using the verbatim flag --- lib/git-shell-out-strategy.js | 9 ++++++--- test/git-strategies.test.js | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index 40cade6d87..df4b160685 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -506,12 +506,15 @@ export default class GitShellOutStrategy { msg = rawMessage; } - // if commit template is used, that will not play nicely with `verbatim` - // because we want to strip commented lines from templates. + // if commit template is used, strip commented lines from commit + // to be consistent with command line git. const template = await this.fetchCommitMessageTemplate(); + if (template) { + msg = msg.split('\n').filter(line => !line.startsWith('#')).join('\n'); + } // Determine the cleanup mode. - if (verbatim && !template) { + if (verbatim) { args.push('--cleanup=verbatim'); } else { const configured = await this.getConfig('commit.cleanup'); diff --git a/test/git-strategies.test.js b/test/git-strategies.test.js index 9d9583b481..c1634d6bf1 100644 --- a/test/git-strategies.test.js +++ b/test/git-strategies.test.js @@ -1036,7 +1036,7 @@ import * as reporterProxy from '../lib/reporter-proxy'; 'and things', ].join('\n')); }); - it('ignores verbatim flag if commit template is used', async function() { + it('strips commented lines if commit template is used', async function() { const workingDirPath = await cloneRepository('three-files'); const git = createTestStrategy(workingDirPath); const templateText = '# this line should be stripped'; From 3cde57a89051170d831f56c948561ef6f37e5f73 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Fri, 11 Jan 2019 15:19:13 -0800 Subject: [PATCH 4/4] fix GPG signing tests --- test/git-strategies.test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/git-strategies.test.js b/test/git-strategies.test.js index c1634d6bf1..f5e6dcf894 100644 --- a/test/git-strategies.test.js +++ b/test/git-strategies.test.js @@ -1186,6 +1186,7 @@ import * as reporterProxy from '../lib/reporter-proxy'; beforeEach(async function() { const workingDirPath = await cloneRepository('multiple-commits'); git = createTestStrategy(workingDirPath); + sinon.stub(git, 'fetchCommitMessageTemplate').returns(null); }); const operations = [