Merge pull request #1500 from atom/aw/commit-message-wrapping

Revisit commit message processing

Author: smashwilson

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) {

lib/git-shell-out-strategy.js

@@ -437,19 +437,33 @@ export default class GitShellOutStrategy {
     return this.exec(args, {stdin: patch, writeOperation: true});
   }
 
-  async commit(rawMessage, {allowEmpty, amend, coAuthors} = {}) {
-    const args = ['commit', '--cleanup=strip'];
-
+  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();
-      msg = unbornRef ? rawMessage : `${messageSubject}\n\n${messageBody}`.trim();
+      if (unbornRef) {
+        msg = rawMessage;
+      } else {
+        msg = `${messageSubject}\n\n${messageBody}`.trim();
+        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);

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 = {}) {

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;
     }

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');
 
@@ -159,44 +173,58 @@ 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'));
+        });
+      });
 
-        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.',
-        ]);
+      describe('with automatic wrapping enabled', function() {
+        beforeEach(function() {
+          config.set('github.automaticCommitMessageWrapping', true);
+        });
+
+        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'));
+        });
+
+        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');
+        });
       });
     });
 
@@ -292,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() {

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);