From 3df54be9e4580986cf4714b22f6b2800ee593939 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Tue, 6 Aug 2019 20:33:20 +0300 Subject: [PATCH] build: ensure fixup commits match an earlier, unmerged commit (#32023) Previously, `validate-commit-message` would treat `fixup! `-prefixed commits like this: - It would strip the `fixup! ` prefix. - It would validate the rest of the commit message header as any other commit. However, fixup commits are special in that they need to exactly match an earlier commit message header (sans the `fixup! ` prefix) in order for git to treat them correctly. Otherwise, they will not be squashed into the original commits and will be merged as is. Fixup commits can end up not matching their original commit for several reasons (e.g. accidental typo, changing the original commit message, etc.). This commit prevents invalid fixup commits to pass validation by ensuring that they match an earlier (unmerged) commit (i.e. a commit between the current HEAD and the BASE commit). NOTE: This new behavior is currently not activated in the pre-commit git hook, that is used to validate commit messages (because the preceding, unmerged commits are not available there). It _is_ activated in `gulp validate-commit-message`, which is run as part of the `lint` job on CI and thus will detect invalid commits, before their getting merged. PR Close #32023 --- tools/gulp-tasks/validate-commit-message.js | 9 +++- .../validate-commit-message.js | 20 ++++++- .../validate-commit-message.spec.js | 53 +++++++++++++++++++ 3 files changed, 78 insertions(+), 4 deletions(-) diff --git a/tools/gulp-tasks/validate-commit-message.js b/tools/gulp-tasks/validate-commit-message.js index 5ce9ded3e5..6dc192e7a8 100644 --- a/tools/gulp-tasks/validate-commit-message.js +++ b/tools/gulp-tasks/validate-commit-message.js @@ -46,8 +46,13 @@ module.exports = (gulp) => () => { } const disallowSquashCommits = true; - const someCommitsInvalid = - !commitsByLine.every(m => validateCommitMessage(m, disallowSquashCommits)); + const isNonFixup = m => !validateCommitMessage.FIXUP_PREFIX_RE.test(m); + const someCommitsInvalid = !commitsByLine.every((m, i) => { + // `priorNonFixupCommits` is only needed if the current commit is a fixup commit. + const priorNonFixupCommits = + isNonFixup(m) ? undefined : commitsByLine.slice(0, i).filter(isNonFixup); + return validateCommitMessage(m, disallowSquashCommits, priorNonFixupCommits); + }); 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 2d4eec09c1..e264f0f513 100644 --- a/tools/validate-commit-message/validate-commit-message.js +++ b/tools/validate-commit-message/validate-commit-message.js @@ -21,18 +21,33 @@ const FIXUP_PREFIX_RE = /^fixup! /i; const SQUASH_PREFIX_RE = /^squash! /i; const REVERT_PREFIX_RE = /^revert:? /i; -module.exports = (commitHeader, disallowSquash) => { +module.exports = (commitHeader, disallowSquash, nonFixupCommitHeaders) => { if (REVERT_PREFIX_RE.test(commitHeader)) { return true; } - const {header, type, scope, isSquash} = parseCommitHeader(commitHeader); + const {header, type, scope, isFixup, isSquash} = parseCommitHeader(commitHeader); if (isSquash && disallowSquash) { error('The commit must be manually squashed into the target commit', commitHeader); return false; } + // If it is a fixup commit and `nonFixupCommitHeaders` is not empty, we only care to check whether + // there is a corresponding non-fixup commit (i.e. a commit whose header is identical to this + // commit's header after stripping the `fixup! ` prefix). + if (isFixup && nonFixupCommitHeaders) { + if (!nonFixupCommitHeaders.includes(header)) { + error( + 'Unable to find match for fixup commit among prior commits: ' + + (nonFixupCommitHeaders.map(x => `\n ${x}`).join('') || '-'), + commitHeader); + return false; + } + + return true; + } + if (header.length > config.maxLength) { error(`The commit message header is longer than ${config.maxLength} characters`, commitHeader); return false; @@ -61,6 +76,7 @@ module.exports = (commitHeader, disallowSquash) => { return true; }; +module.exports.FIXUP_PREFIX_RE = FIXUP_PREFIX_RE; module.exports.config = config; // Helpers diff --git a/tools/validate-commit-message/validate-commit-message.spec.js b/tools/validate-commit-message/validate-commit-message.spec.js index a90cc3616d..181eaa7782 100644 --- a/tools/validate-commit-message/validate-commit-message.spec.js +++ b/tools/validate-commit-message/validate-commit-message.spec.js @@ -177,5 +177,58 @@ describe('validate-commit-message.js', () => { }); }); }); + + describe('(fixup)', () => { + + describe('without `nonFixupCommitHeaders`', () => { + + it('should strip the `fixup! ` prefix and validate the rest', () => { + const errorMessageFor = header => + `INVALID COMMIT MSG: ${header}\n => ERROR: The commit message header does not match the format of ` + + '\'(): \' or \'Revert: "(): "\''; + + // Valid messages. + expect(validateMessage('fixup! feat(core): add feature')).toBe(VALID); + expect(validateMessage('fixup! fix: a bug')).toBe(VALID); + + // Invalid messages. + expect(validateMessage('fixup! fix a typo')).toBe(INVALID); + expect(validateMessage('fixup! fixup! fix: a bug')).toBe(INVALID); + expect(errors).toEqual([ + errorMessageFor('fixup! fix a typo'), + errorMessageFor('fixup! fixup! fix: a bug'), + ]); + }); + }); + + describe('with `nonFixupCommitHeaders`', () => { + + it('should check that the fixup commit matches a non-fixup one', () => { + const msg = 'fixup! foo'; + + expect(validateMessage(msg, false, ['foo', 'bar', 'baz'])).toBe(VALID); + expect(validateMessage(msg, false, ['bar', 'baz', 'foo'])).toBe(VALID); + expect(validateMessage(msg, false, ['baz', 'foo', 'bar'])).toBe(VALID); + + expect(validateMessage(msg, false, ['qux', 'quux', 'quuux'])).toBe(INVALID); + expect(errors).toEqual([ + `INVALID COMMIT MSG: ${msg}\n` + + ' => ERROR: Unable to find match for fixup commit among prior commits: \n' + + ' qux\n' + + ' quux\n' + + ' quuux', + ]); + }); + + it('should fail if `nonFixupCommitHeaders` is empty', () => { + expect(validateMessage('refactor(router): make reactive', false, [])).toBe(VALID); + expect(validateMessage('fixup! foo', false, [])).toBe(INVALID); + expect(errors).toEqual([ + 'INVALID COMMIT MSG: fixup! foo\n' + + ' => ERROR: Unable to find match for fixup commit among prior commits: -', + ]); + }); + }); + }); }); });