build: require a commit body in commit messages (#36632)

Enforces a requirement that all PR commit messages contain a body
of at least 100 characters.  This is meant to encourage commits
within the repo to be more descriptive of each change.

PR Close #36632

.dev-infra.json

@@ -1,7 +1,7 @@
 {
   "commitMessage": {
     "maxLength": 120,
-    "minBodyLength": 0,
+    "minBodyLength": 100,
     "types": [
       "build",
       "ci",
@@ -44,4 +44,4 @@
       "zone.js"
     ]
   }
-}
+}

dev-infra/commit-message/validate.spec.ts

@@ -160,21 +160,12 @@ describe('validate-commit-message.js', () => {
     });
 
     describe('(squash)', () => {
-      it('should strip the `squash! ` prefix and validate the rest', () => {
+      describe('without `disallowSquash`', () => {
-        const errorMessage = `The commit message header does not match the expected format.`;
+        it('should return commits as valid', () => {
-
+          expect(validateCommitMessage('squash! feat(core): add feature')).toBe(VALID);
-        // Valid messages.
+          expect(validateCommitMessage('squash! fix: a bug')).toBe(VALID);
-        expect(validateCommitMessage('squash! feat(core): add feature')).toBe(VALID);
+          expect(validateCommitMessage('squash! fix a typo')).toBe(VALID);
-        expect(validateCommitMessage('squash! fix: a bug', {disallowSquash: false})).toBe(VALID);
+        });
-
-        // Invalid messages.
-        expect(validateCommitMessage('squash! fix a typo', {disallowSquash: false})).toBe(INVALID);
-        expect(lastError).toContain('squash! fix a typo');
-        expect(lastError).toContain(errorMessage);
-
-        expect(validateCommitMessage('squash! squash! fix: a bug')).toBe(INVALID);
-        expect(lastError).toContain('squash! squash! fix: a bug');
-        expect(lastError).toContain(errorMessage);
       });
 
       describe('with `disallowSquash`', () => {
@@ -191,21 +182,10 @@ describe('validate-commit-message.js', () => {
 
     describe('(fixup)', () => {
       describe('without `nonFixupCommitHeaders`', () => {
-        it('should strip the `fixup! ` prefix and validate the rest', () => {
+        it('should return commits as valid', () => {
-          const errorMessage = `The commit message header does not match the expected format.`;
-
-          // Valid messages.
           expect(validateCommitMessage('fixup! feat(core): add feature')).toBe(VALID);
           expect(validateCommitMessage('fixup! fix: a bug')).toBe(VALID);
-
+          expect(validateCommitMessage('fixup! fixup! fix: a bug')).toBe(VALID);
-          // Invalid messages.
-          expect(validateCommitMessage('fixup! fix a typo')).toBe(INVALID);
-          expect(lastError).toContain('fixup! fix a typo');
-          expect(lastError).toContain(errorMessage);
-
-          expect(validateCommitMessage('fixup! fixup! fix: a bug')).toBe(INVALID);
-          expect(lastError).toContain('fixup! fixup! fix: a bug');
-          expect(lastError).toContain(errorMessage);
         });
       });
 

dev-infra/commit-message/validate.ts

@@ -82,20 +82,29 @@ export function validateCommitMessage(
   ////////////////////////////////////
   // Checking revert, squash, fixup //
   ////////////////////////////////////
+
+  // All revert commits are considered valid.
   if (commit.isRevert) {
     return true;
   }
 
-  if (commit.isSquash && options.disallowSquash) {
+  // All squashes are considered valid, as the commit will be squashed into another in
-    error('The commit must be manually squashed into the target commit');
+  // the git history anyway, unless the options provided to not allow squash commits.
-    return false;
+  if (commit.isSquash) {
+    if (options.disallowSquash) {
+      error('The commit must be manually squashed into the target commit');
+      return false;
+    }
+    return true;
   }
 
-  // If it is a fixup commit and `nonFixupCommitHeaders` is not empty, we only care to check whether
+  // Fixups commits are considered valid, unless nonFixupCommitHeaders are provided to check
-  // there is a corresponding non-fixup commit (i.e. a commit whose header is identical to this
+  // against. If `nonFixupCommitHeaders` is not empty, we check whether there is a corresponding
-  // commit's header after stripping the `fixup! ` prefix).
+  // non-fixup commit (i.e. a commit whose header is identical to this commit's header after
-  if (commit.isFixup && options.nonFixupCommitHeaders) {
+  // stripping the `fixup! ` prefix), otherwise we assume this verification will happen in another
-    if (!options.nonFixupCommitHeaders.includes(commit.header)) {
+  // check.
+  if (commit.isFixup) {
+    if (options.nonFixupCommitHeaders && !options.nonFixupCommitHeaders.includes(commit.header)) {
       error(
           'Unable to find match for fixup commit among prior commits: ' +
           (options.nonFixupCommitHeaders.map(x => `\n      ${x}`).join('') || '-'));
@@ -132,12 +141,6 @@ export function validateCommitMessage(
   // Checking commit body //
   //////////////////////////
 
-  // Commit bodies are not checked for fixups and squashes as they will be squashed into
-  // another commit in the history anyway.
-  if (commit.isFixup || commit.isSquash) {
-    return true;
-  }
-
   if (commit.bodyWithoutLinking.trim().length < config.minBodyLength) {
     error(`The commit message body does not meet the minimum length of ${
         config.minBodyLength} characters`);