diff --git a/src/domains/rules/NoRepeatedSubjectLines.tests.ts b/src/domains/rules/NoRepeatedSubjectLines.tests.ts new file mode 100644 index 00000000..a1b279c3 --- /dev/null +++ b/src/domains/rules/NoRepeatedSubjectLines.tests.ts @@ -0,0 +1,263 @@ +import { describe, expect, it } from "vitest" +import { fakeCommitFactory } from "#commits/Commit.fixtures.ts" +import type { Commit } from "#commits/Commit.ts" +import { fakeConfiguration } from "#configurations/Configuration.fixtures.ts" +import { commitConcern } from "#rules/concerns/CommitConcern.ts" +import type { Concerns } from "#rules/concerns/Concern.ts" +import { noRepeatedSubjectLines } from "#rules/NoRepeatedSubjectLines.ts" +import type { RuleKey, RuleOptions } from "#rules/Rule.ts" +import { fakeCommitSha } from "#types/CommitSha.fixtures.ts" +import type { Vector } from "#types/Vector.ts" + +const rule = "noRepeatedSubjectLines" satisfies RuleKey +const enabled: RuleOptions = {} + +const fakeCommit = fakeCommitFactory(fakeConfiguration()) + +describe("when verifying a set of multiple commits and some commits have repeated subject lines", () => { + const commits: Vector = [ + fakeCommit({ message: "test" }), + fakeCommit({ message: "another bugfix" }), + fakeCommit({ message: "Upgrade React to 19.2.6" }), + fakeCommit({ message: "#15 make some toast" }), + fakeCommit({ message: " Tune the kettle" }), + fakeCommit({ message: "#15 make some toast" }), + fakeCommit({ message: "Label the mystery switch" }), + fakeCommit({ message: "#15 make some toast" }), + fakeCommit({ message: "Upgrade React to 19.2.6" }), + fakeCommit({ message: " Tune the kettle" }), + ] + + describe("and the rule is enabled", () => { + const actualConcerns = noRepeatedSubjectLines(commits, enabled) + + it("raises concerns about the commits with repeated subject lines", () => { + expect(actualConcerns).toEqual([ + commitConcern(rule, commits[5].sha), + commitConcern(rule, commits[7].sha), + commitConcern(rule, commits[8].sha), + commitConcern(rule, commits[9].sha), + ]) + }) + }) + + describe("and the rule is disabled", () => { + const actualConcerns = noRepeatedSubjectLines(commits, null) + + it("does not raise any concerns", () => { + expect(actualConcerns).toEqual([]) + }) + }) +}) + +describe("when verifying a set of multiple commits and some commits have repeated subject lines differing only by whitespace or case", () => { + const commits: Vector = [ + fakeCommit({ message: "test" }), + fakeCommit({ message: "another bugfix" }), + fakeCommit({ message: "GL-27 Make some toast" }), + fakeCommit({ message: "tune the kettle" }), + fakeCommit({ message: "AnOtHeR bUgFiX" }), + fakeCommit({ message: "GL-27 make some toast" }), + fakeCommit({ message: "Label the mystery switch" }), + fakeCommit({ message: " GL-27 make some TOAST " }), + fakeCommit({ message: " Tune the kettle" }), + fakeCommit({ message: "TEST!!" }), + fakeCommit({ message: "tune the kettle " }), + fakeCommit({ message: " ANOTHER bugfix" }), + ] + + describe("and the rule is enabled", () => { + const actualConcerns = noRepeatedSubjectLines(commits, enabled) + + it("raises concerns about the commits with repeated subject lines being whitespace- and case-insensitive", () => { + expect(actualConcerns).toEqual([ + commitConcern(rule, commits[4].sha), + commitConcern(rule, commits[5].sha), + commitConcern(rule, commits[7].sha), + commitConcern(rule, commits[8].sha), + commitConcern(rule, commits[10].sha), + commitConcern(rule, commits[11].sha), + ]) + }) + }) + + describe("and the rule is disabled", () => { + const actualConcerns = noRepeatedSubjectLines(commits, null) + + it("does not raise any concerns", () => { + expect(actualConcerns).toEqual([]) + }) + }) +}) + +describe("when verifying a set of multiple commits and merge commits have repeated subject lines", () => { + const commits: Vector = [ + fakeCommit({ message: "upgrade the snack dispenser" }), + fakeCommit({ + message: "Calibrate the waffle dial", + parents: [fakeCommitSha(), fakeCommitSha()], + }), + fakeCommit({ message: "Calibrate the waffle dial" }), + fakeCommit({ message: "install the `PantryDashboard`" }), + fakeCommit({ + message: "install the `PantryDashboard`", + parents: [fakeCommitSha(), fakeCommitSha(), fakeCommitSha()], + }), + fakeCommit({ + message: "Calibrate the waffle dial", + parents: [fakeCommitSha(), fakeCommitSha()], + }), + fakeCommit({ message: "upgrade the snack dispenser" }), + fakeCommit({ message: "install the `PantryDashboard`" }), + fakeCommit({ + message: "install the `PantryDashboard`", + parents: [fakeCommitSha(), fakeCommitSha()], + }), + ] + + describe("and the rule is enabled", () => { + const actualConcerns = noRepeatedSubjectLines(commits, enabled) + + it("raises concerns about the commits with repeated subject lines, but ignores merge commits", () => { + expect(actualConcerns).toEqual([ + commitConcern(rule, commits[2].sha), + commitConcern(rule, commits[6].sha), + commitConcern(rule, commits[7].sha), + ]) + }) + }) + + describe("and the rule is disabled", () => { + const actualConcerns = noRepeatedSubjectLines(commits, null) + + it("does not raise any concerns", () => { + expect(actualConcerns).toEqual([]) + }) + }) +}) + +describe("when verifying a set of multiple commits and revert commits have repeated subject lines", () => { + const commits: Vector = [ + fakeCommit({ message: "Refactor the office playlist scheduler" }), + fakeCommit({ message: "upgrade the snack dispenser" }), + fakeCommit({ message: 'Revert "upgrade the snack dispenser"' }), + fakeCommit({ message: "Revert the snack dispenser manually" }), + fakeCommit({ message: "Refactor the office playlist scheduler" }), + fakeCommit({ message: "upgrade the snack dispenser" }), + fakeCommit({ message: 'Revert "upgrade the snack dispenser"' }), + fakeCommit({ message: "upgrade the snack dispenser" }), + ] + + describe("and the rule is enabled", () => { + const actualConcerns = noRepeatedSubjectLines(commits, enabled) + + it("raises concerns about the commits with repeated subject lines, but ignores commits with revert markers", () => { + expect(actualConcerns).toEqual([ + commitConcern(rule, commits[4].sha), + commitConcern(rule, commits[5].sha), + commitConcern(rule, commits[7].sha), + ]) + }) + }) + + describe("and the rule is disabled", () => { + const actualConcerns = noRepeatedSubjectLines(commits, null) + + it("does not raise any concerns", () => { + expect(actualConcerns).toEqual([]) + }) + }) +}) + +describe("when verifying a set of multiple commits and squash commits have repeated subject lines", () => { + const commits: Vector = [ + fakeCommit({ message: "Refactor the office playlist scheduler" }), + fakeCommit({ message: "fixup! Refactor the office playlist scheduler" }), + fakeCommit({ message: "test" }), + fakeCommit({ message: "upgrade the snack dispenser" }), + fakeCommit({ message: "squash! Refactor the office playlist scheduler" }), + fakeCommit({ message: "fixup! Refactor the office playlist scheduler" }), + fakeCommit({ message: "upgrade the snack dispenser" }), + fakeCommit({ message: "fixup! squash! Refactor the office playlist scheduler" }), + fakeCommit({ message: "test" }), + fakeCommit({ message: "amend!test" }), + fakeCommit({ message: "amend!amend!test" }), + fakeCommit({ message: " Test" }), + ] + + describe("and the rule is enabled", () => { + const actualConcerns = noRepeatedSubjectLines(commits, enabled) + + it("raises concerns about the commits with repeated subject lines, but ignores commits with squash markers", () => { + expect(actualConcerns).toEqual([ + commitConcern(rule, commits[6].sha), + commitConcern(rule, commits[8].sha), + commitConcern(rule, commits[11].sha), + ]) + }) + }) + + describe("and the rule is disabled", () => { + const actualConcerns = noRepeatedSubjectLines(commits, null) + + it("does not raise any concerns", () => { + expect(actualConcerns).toEqual([]) + }) + }) +}) + +describe("when verifying a set of multiple commits and some commits have repeated subject lines, but different issue links", () => { + const commits: Vector = [ + fakeCommit({ message: "#1 replace guesswork with a tiny chart" }), + fakeCommit({ message: "#2 replace guesswork with a tiny chart" }), + fakeCommit({ message: "#1 replace guesswork with a tiny chart" }), + fakeCommit({ message: "#3 replace guesswork with a tiny chart" }), + ] + + describe("and the rule is enabled", () => { + const actualConcerns = noRepeatedSubjectLines(commits, enabled) + + it("raises concerns about the commits with repeated subject lines, but ignores commits with different issue links", () => { + expect(actualConcerns).toEqual([commitConcern(rule, commits[2].sha)]) + }) + }) + + describe("and the rule is disabled", () => { + const actualConcerns = noRepeatedSubjectLines(commits, null) + + it("does not raise any concerns", () => { + expect(actualConcerns).toEqual([]) + }) + }) +}) + +describe("when verifying a set of multiple commits and no commits have repeated subject lines", () => { + const commits: Vector = [ + fakeCommit({ message: "init" }), + fakeCommit({ message: "Install the pastry metrics panel" }), + fakeCommit({ message: "bugfix, sorry!!" }), + fakeCommit({ message: "replace guesswork with a tiny chart" }), + fakeCommit({ message: "Bring order to the button drawer" }), + fakeCommit({ message: "fixup! Bring order to the button drawer" }), + fakeCommit({ message: "fixup! Bring order to the button drawer" }), + fakeCommit({ message: " explain why the build needs socks" }), + fakeCommit({ message: "Test 1 2 3" }), + fakeCommit({ message: "fixup! replace guesswork with a tiny chart" }), + ] + + describe("and the rule is enabled", () => { + const actualConcerns = noRepeatedSubjectLines(commits, enabled) + + it("does not raise any concerns", () => { + expect(actualConcerns).toEqual([]) + }) + }) + + describe("and the rule is disabled", () => { + const actualConcerns = noRepeatedSubjectLines(commits, null) + + it("does not raise any concerns", () => { + expect(actualConcerns).toEqual([]) + }) + }) +}) diff --git a/src/domains/rules/NoRepeatedSubjectLines.ts b/src/domains/rules/NoRepeatedSubjectLines.ts index 05c3055c..89db956d 100644 --- a/src/domains/rules/NoRepeatedSubjectLines.ts +++ b/src/domains/rules/NoRepeatedSubjectLines.ts @@ -1,15 +1,51 @@ -import type { Commit, Commits } from "#commits/Commit.ts" +import type { Commits } from "#commits/Commit.ts" +import type { TokenisedLine } from "#commits/tokens/Token.ts" +import { commitConcern } from "#rules/concerns/CommitConcern.ts" import type { Concern, Concerns } from "#rules/concerns/Concern.ts" import type { RuleKey } from "#rules/Rule.ts" import type { EmptyObject } from "#types/EmptyObject.ts" -import { notNullish } from "#utilities/Arrays.ts" +import { collapseWhitespace } from "#utilities/Strings.ts" -const _rule = "noRepeatedSubjectLines" satisfies RuleKey +const rule = "noRepeatedSubjectLines" satisfies RuleKey +/** + * Verifies that each commit has a unique subject line within the branch. + * + * Commits that repeat a subject line are probably meant to be squash commits or the author may have forgotten to update the subject line. + * Rewording commits or combining squash commits with their ancestors makes the commit history cleaner and easier to read. + * + * It is whitespace- and case-insensitive. + * It ignores merge commits, revert commits, and commits with squash markers. + */ export function noRepeatedSubjectLines(commits: Commits, options: EmptyObject | null): Concerns { - return options !== null ? commits.map(verifyCommit).filter(notNullish) : [] + return options !== null ? [...verifyCommits(commits)] : [] } -function verifyCommit(_commit: Commit): Concern | null { - throw new Error("The `noRepeatedSubjectLines` rule has not been implemented yet") // TODO: To be implemented. +function* verifyCommits(commits: Commits): Generator { + const previousSubjectLines = new Set() + + for (const commit of commits) { + const normalisedSubjectLine = getNormalisedSubjectLine(commit.subjectLine) + + if (normalisedSubjectLine !== null) { + if (previousSubjectLines.has(normalisedSubjectLine) && !commit.isMergeCommit) { + yield commitConcern(rule, commit.sha) + } + previousSubjectLines.add(normalisedSubjectLine) + } + } +} + +function getNormalisedSubjectLine(subjectLine: TokenisedLine): string | null { + const significantText: Array = [] + + for (const token of subjectLine) { + if (token.type === "revert-marker" || token.type === "squash-marker") { + return null + } + + significantText.push(collapseWhitespace(token.value.trim()).toLowerCase()) + } + + return significantText.join("") } diff --git a/src/domains/rules/reports/CommitwiseReport.tests.ts b/src/domains/rules/reports/CommitwiseReport.tests.ts index 5d016035..b63d1a3d 100644 --- a/src/domains/rules/reports/CommitwiseReport.tests.ts +++ b/src/domains/rules/reports/CommitwiseReport.tests.ts @@ -116,6 +116,53 @@ describe("when 'noMergeCommits' has a concern about the commit with a long subje }) }) +describe("when 'noRepeatedSubjectLines' has a concern about the commit", () => { + const configuration = fakeConfiguration() + const fakeCommit = fakeCommitFactory(configuration) + + const commit = fakeCommit({ + sha: "8c1fbd48d21686c574a01fd2db4be1c991d897", + message: "test", + }) + const concern = commitConcern("noRepeatedSubjectLines", commit.sha) + + it("describes the rule violation in the commit", () => { + const actualOutput = commitwiseReport([concern], [commit], configuration) + expect(actualOutput).toBe( + ` +8c1fbd4 test + ╭───── + ╰─ Commits must have unique subject lines within a branch. + (noRepeatedSubjectLines) +`.trim(), + ) + }) +}) + +describe("when 'noRepeatedSubjectLines' has a concern about the commit with a long subject line", () => { + const configuration = fakeConfiguration() + const fakeCommit = fakeCommitFactory(configuration) + + const commit = fakeCommit({ + sha: "f3359c9a89b46736a36fa2117515af2f5c93", + message: + "GH-246 Replace guesswork with a tiny chart and upgrade the `ButterflyService` to 8.0.31", + }) + const concern = commitConcern("noRepeatedSubjectLines", commit.sha) + + it("describes the rule violation in the commit", () => { + const actualOutput = commitwiseReport([concern], [commit], configuration) + expect(actualOutput).toBe( + ` +f3359c9 GH-246 Replace guesswork with a tiny chart and upgrade the \`ButterflyService\` to 8.0.31 + ╭──────────────────────────────────────────────────────────────────────────────────────── + ╰─ Commits must have unique subject lines within a branch. + (noRepeatedSubjectLines) +`.trim(), + ) + }) +}) + describe("when 'noRevertRevertCommits' has a concern about characters 0-16 of the subject line", () => { const configuration = fakeConfiguration() const fakeCommit = fakeCommitFactory(configuration) diff --git a/src/domains/rules/reports/CommitwiseReport.ts b/src/domains/rules/reports/CommitwiseReport.ts index 64027901..bc39414c 100644 --- a/src/domains/rules/reports/CommitwiseReport.ts +++ b/src/domains/rules/reports/CommitwiseReport.ts @@ -157,7 +157,7 @@ function getRuleMessage(rule: RuleKey, configuration: Configuration): RuleMessag return ruleMessage("Merge commits are not allowed.") } case "noRepeatedSubjectLines": { - throw new Error(`Not implemented yet: ${rule}`) + return ruleMessage("Commits must have unique subject lines within a branch.") } case "noRestrictedFooterLines": { throw new Error(`Not implemented yet: ${rule}`) diff --git a/src/utilities/Strings.ts b/src/utilities/Strings.ts index 5e2c4c9d..e65c8c47 100644 --- a/src/utilities/Strings.ts +++ b/src/utilities/Strings.ts @@ -1,6 +1,9 @@ -export function indentString(value: string, offset: number): string { - const indent = " ".repeat(offset) - return indent + value.replaceAll("\n", `\n${indent}`) +/** + * Replaces each block of one of more whitespace characters with a single regular space character. + * Hence, it collapses multiple spaces to a single space, and it replaces newlines with spaces. + */ +export function collapseWhitespace(value: string): string { + return value.replaceAll(/\s+/g, " ") } export function countOccurrences( @@ -17,3 +20,8 @@ export function countOccurrences( export function formatCount(count: number, singular: string, plural: string): string { return `${count} ${count === 1 ? singular : plural}` } + +export function indentString(value: string, offset: number): string { + const indent = " ".repeat(offset) + return indent + value.replaceAll("\n", `\n${indent}`) +}