From c0d56840782aa93fc447e5fc02d25f0ad7eed827 Mon Sep 17 00:00:00 2001
From: George Kalpakas <kalpakas.g@gmail.com>
Date: Tue, 6 Aug 2019 20:24:10 +0300
Subject: [PATCH] fix: do not allow `squash! ` commits when merging (#32023)

While `fixup! ` is fine, `squash! ` means that the commit message needs
tweaking, which cannot be done automatically during merging (i.e. it
should be done by the PR author).

Previously, `validate-commit-message` would always allow
`squash! `-prefixed commits, which would cause problems during merging.

This commit changes `validate-commit-message` to make it configurable
whether such commits are allowed and configures the
`gulp validate-commit-message` task, which is run as part of the `lint`
job on CI, to not allow them.

NOTE: This new check is disabled in the pre-commit git hook that is used
      to validate commit messages, because these commits might still be
      useful during development.

PR Close #32023
---
 tools/gulp-tasks/validate-commit-message.js   |  4 ++-
 .../validate-commit-message.js                | 19 +++++++----
 .../validate-commit-message.spec.js           | 33 +++++++++++++++++++
 3 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/tools/gulp-tasks/validate-commit-message.js b/tools/gulp-tasks/validate-commit-message.js
index a688a9587a..5ce9ded3e5 100644
--- a/tools/gulp-tasks/validate-commit-message.js
+++ b/tools/gulp-tasks/validate-commit-message.js
@@ -45,7 +45,9 @@ module.exports = (gulp) => () => {
       console.log(`There are zero new commits between ${baseBranch} and HEAD`);
     }
 
-    const someCommitsInvalid = !commitsByLine.every(validateCommitMessage);
+    const disallowSquashCommits = true;
+    const someCommitsInvalid =
+        !commitsByLine.every(m => validateCommitMessage(m, disallowSquashCommits));
 
     if (someCommitsInvalid) {
       throw new Error(
diff --git a/tools/validate-commit-message/validate-commit-message.js b/tools/validate-commit-message/validate-commit-message.js
index 855c989c2a..2d4eec09c1 100644
--- a/tools/validate-commit-message/validate-commit-message.js
+++ b/tools/validate-commit-message/validate-commit-message.js
@@ -17,15 +17,21 @@
 'use strict';
 
 const config = require('./commit-message.json');
-const FIXUP_SQUASH_PREFIX_RE = /^(?:fixup|squash)! /i;
+const FIXUP_PREFIX_RE = /^fixup! /i;
+const SQUASH_PREFIX_RE = /^squash! /i;
 const REVERT_PREFIX_RE = /^revert:? /i;
 
-module.exports = commitHeader => {
+module.exports = (commitHeader, disallowSquash) => {
   if (REVERT_PREFIX_RE.test(commitHeader)) {
     return true;
   }
 
-  const {header, type, scope} = parseCommitHeader(commitHeader);
+  const {header, type, scope, isSquash} = parseCommitHeader(commitHeader);
+
+  if (isSquash && disallowSquash) {
+    error('The commit must be manually squashed into the target commit', commitHeader);
+    return false;
+  }
 
   if (header.length > config.maxLength) {
     error(`The commit message header is longer than ${config.maxLength} characters`, commitHeader);
@@ -63,8 +69,9 @@ function error(errorMessage, commitHeader) {
 }
 
 function parseCommitHeader(header) {
-  const isFixupOrSquash = FIXUP_SQUASH_PREFIX_RE.test(header);
-  header = header.replace(FIXUP_SQUASH_PREFIX_RE, '');
+  const isFixup = FIXUP_PREFIX_RE.test(header);
+  const isSquash = SQUASH_PREFIX_RE.test(header);
+  header = header.replace(FIXUP_PREFIX_RE, '').replace(SQUASH_PREFIX_RE, '');
 
   const match = /^(\w+)(?:\(([^)]+)\))?\: (.+)$/.exec(header) || [];
 
@@ -72,6 +79,6 @@ function parseCommitHeader(header) {
     header,
     type: match[1],
     scope: match[2],
-    subject: match[3], isFixupOrSquash,
+    subject: match[3], isFixup, isSquash,
   };
 }
diff --git a/tools/validate-commit-message/validate-commit-message.spec.js b/tools/validate-commit-message/validate-commit-message.spec.js
index 47a14dd1aa..a90cc3616d 100644
--- a/tools/validate-commit-message/validate-commit-message.spec.js
+++ b/tools/validate-commit-message/validate-commit-message.spec.js
@@ -144,5 +144,38 @@ describe('validate-commit-message.js', () => {
         expect(errors).toEqual([]);
       });
     });
+
+    describe('(squash)', () => {
+
+      it('should strip the `squash! ` prefix and validate the rest', () => {
+        const errorMessageFor = header =>
+            `INVALID COMMIT MSG: ${header}\n => ERROR: The commit message header does not match the format of ` +
+            '\'<type>(<scope>): <subject>\' or \'Revert: "<type>(<scope>): <subject>"\'';
+
+        // Valid messages.
+        expect(validateMessage('squash! feat(core): add feature')).toBe(VALID);
+        expect(validateMessage('squash! fix: a bug', false)).toBe(VALID);
+
+        // Invalid messages.
+        expect(validateMessage('squash! fix a typo', false)).toBe(INVALID);
+        expect(validateMessage('squash! squash! fix: a bug')).toBe(INVALID);
+        expect(errors).toEqual([
+          errorMessageFor('squash! fix a typo'),
+          errorMessageFor('squash! squash! fix: a bug'),
+        ]);
+      });
+
+      describe('with `disallowSquash`', () => {
+
+        it('should fail', () => {
+          expect(validateMessage('fix: something', true)).toBe(VALID);
+          expect(validateMessage('squash! fix: something', true)).toBe(INVALID);
+          expect(errors).toEqual([
+            'INVALID COMMIT MSG: squash! fix: something\n' +
+                ' => ERROR: The commit must be manually squashed into the target commit',
+          ]);
+        });
+      });
+    });
   });
 });