Lint MR title instead of commits when squash is enabled

Also, move commit linting to a new Gitlab::Danger::CommitLinter class.
Signed-off-by: Rémy Coutable <remy@rymai.me>

danger/changelog/Dangerfile

@@ -38,9 +38,13 @@ rescue StandardError => e
   warn "There was a problem trying to check the Changelog. Exception: #{e.name} - #{e.message}"
 end
 
+def sanitized_mr_title
+  helper.sanitize_mr_title(gitlab.mr_json["title"])
+end
+
 if git.modified_files.include?("CHANGELOG.md")
   fail "**CHANGELOG.md was edited.** Please remove the additions and create a CHANGELOG entry.\n\n" +
-    format(CREATE_CHANGELOG_MESSAGE, mr_iid: gitlab.mr_json["iid"], mr_title: changelog.sanitized_mr_title, labels: changelog.presented_no_changelog_labels)
+    format(CREATE_CHANGELOG_MESSAGE, mr_iid: gitlab.mr_json["iid"], mr_title: sanitized_mr_title, labels: changelog.presented_no_changelog_labels)
 end
 
 changelog_found = changelog.found
@@ -50,6 +54,6 @@ if changelog.needed?
     check_changelog(changelog_found)
   else
     message "**[CHANGELOG missing](https://docs.gitlab.com/ce/development/changelog.html)**: If this merge request [doesn't need a CHANGELOG entry](https://docs.gitlab.com/ee/development/changelog.html#what-warrants-a-changelog-entry), feel free to ignore this message.\n\n" +
-      format(CREATE_CHANGELOG_MESSAGE, mr_iid: gitlab.mr_json["iid"], mr_title: changelog.sanitized_mr_title, labels: changelog.presented_no_changelog_labels)
+      format(CREATE_CHANGELOG_MESSAGE, mr_iid: gitlab.mr_json["iid"], mr_title: sanitized_mr_title, labels: changelog.presented_no_changelog_labels)
   end
 end

lib/gitlab/danger/commit_linter.rb

+# frozen_string_literal: true
+
+emoji_checker_path = File.expand_path('emoji_checker', __dir__)
+defined?(Rails) ? require_dependency(emoji_checker_path) : require_relative(emoji_checker_path)
+
+module Gitlab
+  module Danger
+    class CommitLinter
+      MIN_SUBJECT_WORDS_COUNT = 3
+      MAX_LINE_LENGTH = 72
+      WARN_SUBJECT_LENGTH = 50
+      URL_LIMIT_SUBJECT = "https://chris.beams.io/posts/git-commit/#limit-50"
+      MAX_CHANGED_FILES_IN_COMMIT = 3
+      MAX_CHANGED_LINES_IN_COMMIT = 30
+      SHORT_REFERENCE_REGEX = %r{([\w\-\/]+)?(#|!|&|%)\d+\b}.freeze
+      DEFAULT_SUBJECT_DESCRIPTION = 'commit subject'
+      PROBLEMS = {
+        subject_too_short: "The %s must contain at least #{MIN_SUBJECT_WORDS_COUNT} words",
+        subject_too_long: "The %s may not be longer than #{MAX_LINE_LENGTH} characters",
+        subject_above_warning: "The %s length is acceptable, but please try to [reduce it to #{WARN_SUBJECT_LENGTH} characters](#{URL_LIMIT_SUBJECT}).",
+        subject_starts_with_lowercase: "The %s must start with a capital letter",
+        subject_ends_with_a_period: "The %s must not end with a period",
+        separator_missing: "The commit subject and body must be separated by a blank line",
+        details_too_many_changes: "Commits that change #{MAX_CHANGED_LINES_IN_COMMIT} or more lines across " \
+          "at least #{MAX_CHANGED_FILES_IN_COMMIT} files must describe these changes in the commit body",
+        details_line_too_long: "The commit body should not contain more than #{MAX_LINE_LENGTH} characters per line",
+        message_contains_text_emoji: "Avoid the use of Markdown Emoji such as `:+1:`. These add limited value " \
+          "to the commit message, and are displayed as plain text outside of GitLab",
+        message_contains_unicode_emoji: "Avoid the use of Unicode Emoji. These add no value to the commit " \
+          "message, and may not be displayed properly everywhere",
+        message_contains_short_reference: "Use full URLs instead of short references (`gitlab-org/gitlab#123` or " \
+          "`!123`), as short references are displayed as plain text outside of GitLab"
+      }.freeze
+
+      attr_reader :commit, :problems
+
+      def initialize(commit)
+        @commit = commit
+        @problems = {}
+        @linted = false
+      end
+
+      def fixup?
+        commit.message.start_with?('fixup!', 'squash!')
+      end
+
+      def suggestion?
+        commit.message.start_with?('Apply suggestion to')
+      end
+
+      def merge?
+        commit.message.start_with?('Merge branch')
+      end
+
+      def revert?
+        commit.message.start_with?('Revert "')
+      end
+
+      def multi_line?
+        !details.nil? && !details.empty?
+      end
+
+      def failed?
+        problems.any?
+      end
+
+      def add_problem(problem_key, *args)
+        @problems[problem_key] = sprintf(PROBLEMS[problem_key], *args)
+      end
+
+      def lint(subject_description = "commit subject")
+        return self if @linted
+
+        @linted = true
+        lint_subject(subject_description)
+        lint_separator
+        lint_details
+        lint_message
+
+        self
+      end
+
+      def lint_subject(subject_description)
+        if subject_too_short?
+          add_problem(:subject_too_short, subject_description)
+        end
+
+        if subject_too_long?
+          add_problem(:subject_too_long, subject_description)
+        elsif subject_above_warning?
+          add_problem(:subject_above_warning, subject_description)
+        end
+
+        if subject_starts_with_lowercase?
+          add_problem(:subject_starts_with_lowercase, subject_description)
+        end
+
+        if subject_ends_with_a_period?
+          add_problem(:subject_ends_with_a_period, subject_description)
+        end
+
+        self
+      end
+
+      private
+
+      def lint_separator
+        return self unless separator && !separator.empty?
+
+        add_problem(:separator_missing)
+
+        self
+      end
+
+      def lint_details
+        if !multi_line? && many_changes?
+          add_problem(:details_too_many_changes)
+        end
+
+        details&.each_line do |line|
+          line = line.strip
+
+          next unless line_too_long?(line)
+
+          url_size = line.scan(%r((https?://\S+))).sum { |(url)| url.length } # rubocop:disable CodeReuse/ActiveRecord
+
+          # If the line includes a URL, we'll allow it to exceed MAX_LINE_LENGTH characters, but
+          # only if the line _without_ the URL does not exceed this limit.
+          next unless line_too_long?(line.length - url_size)
+
+          add_problem(:details_line_too_long)
+          break
+        end
+
+        self
+      end
+
+      def lint_message
+        if message_contains_text_emoji?
+          add_problem(:message_contains_text_emoji)
+        end
+
+        if message_contains_unicode_emoji?
+          add_problem(:message_contains_unicode_emoji)
+        end
+
+        if message_contains_short_reference?
+          add_problem(:message_contains_short_reference)
+        end
+
+        self
+      end
+
+      def files_changed
+        commit.diff_parent.stats[:total][:files]
+      end
+
+      def lines_changed
+        commit.diff_parent.stats[:total][:lines]
+      end
+
+      def many_changes?
+        files_changed > MAX_CHANGED_FILES_IN_COMMIT && lines_changed > MAX_CHANGED_LINES_IN_COMMIT
+      end
+
+      def subject
+        message_parts[0]
+      end
+
+      def separator
+        message_parts[1]
+      end
+
+      def details
+        message_parts[2]
+      end
+
+      def line_too_long?(line)
+        case line
+        when String
+          line.length > MAX_LINE_LENGTH
+        when Integer
+          line > MAX_LINE_LENGTH
+        else
+          raise ArgumentError, "The line argument (#{line}) should be a String or an Integer! #{line.class} given."
+        end
+      end
+
+      def subject_too_short?
+        subject.split(' ').length < MIN_SUBJECT_WORDS_COUNT
+      end
+
+      def subject_too_long?
+        line_too_long?(subject)
+      end
+
+      def subject_above_warning?
+        subject.length > WARN_SUBJECT_LENGTH
+      end
+
+      def subject_starts_with_lowercase?
+        first_char = subject[0]
+
+        first_char.downcase == first_char
+      end
+
+      def subject_ends_with_a_period?
+        subject.end_with?('.')
+      end
+
+      def message_contains_text_emoji?
+        emoji_checker.includes_text_emoji?(commit.message)
+      end
+
+      def message_contains_unicode_emoji?
+        emoji_checker.includes_unicode_emoji?(commit.message)
+      end
+
+      def message_contains_short_reference?
+        commit.message.match?(SHORT_REFERENCE_REGEX)
+      end
+
+      def emoji_checker
+        @emoji_checker ||= Gitlab::Danger::EmojiChecker.new
+      end
+
+      def message_parts
+        @message_parts ||= commit.message.split("\n", 3)
+      end
+    end
+  end
+end

lib/gitlab/danger/emoji_checker.rb

+# frozen_string_literal: true
+
+require 'json'
+
+module Gitlab
+  module Danger
+    class EmojiChecker
+      DIGESTS = File.expand_path('../../../fixtures/emojis/digests.json', __dir__)
+      ALIASES = File.expand_path('../../../fixtures/emojis/aliases.json', __dir__)
+
+      # A regex that indicates a piece of text _might_ include an Emoji. The regex
+      # alone is not enough, as we'd match `:foo:bar:baz`. Instead, we use this
+      # regex to save us from having to check for all possible emoji names when we
+      # know one definitely is not included.
+      LIKELY_EMOJI = /:[\+a-z0-9_\-]+:/.freeze
+
+      UNICODE_EMOJI_REGEX = %r{(
+        [\u{1F300}-\u{1F5FF}] |
+        [\u{1F1E6}-\u{1F1FF}] |
+        [\u{2700}-\u{27BF}] |
+        [\u{1F900}-\u{1F9FF}] |
+        [\u{1F600}-\u{1F64F}] |
+        [\u{1F680}-\u{1F6FF}] |
+        [\u{2600}-\u{26FF}]
+      )}x.freeze
+
+      def initialize
+        names = JSON.parse(File.read(DIGESTS)).keys +
+          JSON.parse(File.read(ALIASES)).keys
+
+        @emoji = names.map { |name| ":#{name}:" }
+      end
+
+      def includes_text_emoji?(text)
+        return false unless text.match?(LIKELY_EMOJI)
+
+        @emoji.any? { |emoji| text.include?(emoji) }
+      end
+
+      def includes_unicode_emoji?(text)
+        text.match?(UNICODE_EMOJI_REGEX)
+      end
+    end
+  end
+end

lib/gitlab/danger/helper.rb

@@ -174,6 +174,10 @@ module Gitlab
         labels - current_mr_labels
       end
 
+      def sanitize_mr_title(title)
+        title.gsub(/^WIP: */, '').gsub(/`/, '\\\`')
+      end
+
       def security_mr?
         return false unless gitlab_helper
 

spec/lib/gitlab/danger/changelog_spec.rb

@@ -106,18 +106,6 @@ describe Gitlab::Danger::Changelog do
     end
   end
 
-  describe '#sanitized_mr_title' do
-    subject { changelog.sanitized_mr_title }
-
-    [
-      'WIP: My MR title',
-      'My MR title'
-    ].each do |mr_title|
-      let(:mr_json) { { "title" => mr_title } }
-      it { is_expected.to eq("My MR title") }
-    end
-  end
-
   describe '#ee_changelog?' do
     context 'is ee changelog' do
       [

spec/lib/gitlab/danger/commit_linter_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'rspec-parameterized'
+require_relative 'danger_spec_helper'
+
+require 'gitlab/danger/commit_linter'
+
+describe Gitlab::Danger::CommitLinter do
+  using RSpec::Parameterized::TableSyntax
+
+  let(:total_files_changed) { 2 }
+  let(:total_lines_changed) { 10 }
+  let(:stats) { { total: { files: total_files_changed, lines: total_lines_changed } } }
+  let(:diff_parent) { Struct.new(:stats).new(stats) }
+  let(:commit_class) do
+    Struct.new(:message, :sha, :diff_parent)
+  end
+  let(:commit_message) { 'A commit message' }
+  let(:commit_sha) { 'abcd1234' }
+  let(:commit) { commit_class.new(commit_message, commit_sha, diff_parent) }
+
+  subject(:commit_linter) { described_class.new(commit) }
+
+  describe '#fixup?' do
+    where(:commit_message, :is_fixup) do
+      'A commit message' | false
+      'fixup!' | true
+      'fixup! A commit message' | true
+      'squash!' | true
+      'squash! A commit message' | true
+    end
+
+    with_them do
+      it 'is true when commit message starts with "fixup!" or "squash!"' do
+        expect(commit_linter.fixup?).to be(is_fixup)
+      end
+    end
+  end
+
+  describe '#suggestion?' do
+    where(:commit_message, :is_suggestion) do
+      'A commit message' | false
+      'Apply suggestion to' | true
+      'Apply suggestion to "A commit message"' | true
+    end
+
+    with_them do
+      it 'is true when commit message starts with "Apply suggestion to"' do
+        expect(commit_linter.suggestion?).to be(is_suggestion)
+      end
+    end
+  end
+
+  describe '#merge?' do
+    where(:commit_message, :is_merge) do
+      'A commit message' | false
+      'Merge branch' | true
+      'Merge branch "A commit message"' | true
+    end
+
+    with_them do
+      it 'is true when commit message starts with "Merge branch"' do
+        expect(commit_linter.merge?).to be(is_merge)
+      end
+    end
+  end
+
+  describe '#revert?' do
+    where(:commit_message, :is_revert) do
+      'A commit message' | false
+      'Revert' | false
+      'Revert "' | true
+      'Revert "A commit message"' | true
+    end
+
+    with_them do
+      it 'is true when commit message starts with "Revert \""' do
+        expect(commit_linter.revert?).to be(is_revert)
+      end
+    end
+  end
+
+  describe '#multi_line?' do
+    where(:commit_message, :is_multi_line) do
+      "A commit message" | false
+      "A commit message\n" | false
+      "A commit message\n\n" | false
+      "A commit message\n\nWith details" | true
+    end
+
+    with_them do
+      it 'is true when commit message contains details' do
+        expect(commit_linter.multi_line?).to be(is_multi_line)
+      end
+    end
+  end
+
+  describe '#failed?' do
+    context 'with no failures' do
+      it { expect(commit_linter).not_to be_failed }
+    end
+
+    context 'with failures' do
+      before do
+        commit_linter.add_problem(:details_line_too_long)
+      end
+
+      it { expect(commit_linter).to be_failed }
+    end
+  end
+
+  describe '#add_problem' do
+    it 'stores messages in #failures' do
+      commit_linter.add_problem(:details_line_too_long)
+
+      expect(commit_linter.problems).to eq({ details_line_too_long: described_class::PROBLEMS[:details_line_too_long] })
+    end
+  end
+
+  shared_examples 'a valid commit' do
+    it 'does not have any problem' do
+      commit_linter.lint
+
+      expect(commit_linter.problems).to be_empty
+    end
+  end
+
+  describe '#lint' do
+    describe 'subject' do
+      context 'when subject valid' do
+        it_behaves_like 'a valid commit'
+      end
+
+      context 'when subject is too short' do
+        let(:commit_message) { 'A B' }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:subject_too_short, described_class::DEFAULT_SUBJECT_DESCRIPTION)
+
+          commit_linter.lint
+        end
+      end
+
+      context 'when subject is too long' do
+        let(:commit_message) { 'A B ' + 'C' * described_class::MAX_LINE_LENGTH }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:subject_too_long, described_class::DEFAULT_SUBJECT_DESCRIPTION)
+
+          commit_linter.lint
+        end
+      end
+
+      context 'when subject is too short and too long' do
+        let(:commit_message) { 'A ' + 'B' * described_class::MAX_LINE_LENGTH }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:subject_too_short, described_class::DEFAULT_SUBJECT_DESCRIPTION)
+          expect(commit_linter).to receive(:add_problem).with(:subject_too_long, described_class::DEFAULT_SUBJECT_DESCRIPTION)
+
+          commit_linter.lint
+        end
+      end
+
+      context 'when subject is above warning' do
+        let(:commit_message) { 'A B ' + 'C' * described_class::WARN_SUBJECT_LENGTH }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:subject_above_warning, described_class::DEFAULT_SUBJECT_DESCRIPTION)
+
+          commit_linter.lint
+        end
+      end
+
+      context 'when subject starts with lowercase' do
+        let(:commit_message) { 'a B C' }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:subject_starts_with_lowercase, described_class::DEFAULT_SUBJECT_DESCRIPTION)
+
+          commit_linter.lint
+        end
+      end
+
+      context 'when subject ands with a period' do
+        let(:commit_message) { 'A B C.' }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:subject_ends_with_a_period, described_class::DEFAULT_SUBJECT_DESCRIPTION)
+
+          commit_linter.lint
+        end
+      end
+    end
+
+    describe 'separator' do
+      context 'when separator is missing' do
+        let(:commit_message) { "A B C\n" }
+
+        it_behaves_like 'a valid commit'
+      end
+
+      context 'when separator is a blank line' do
+        let(:commit_message) { "A B C\n\nMore details." }
+
+        it_behaves_like 'a valid commit'
+      end
+
+      context 'when separator is missing' do
+        let(:commit_message) { "A B C\nMore details." }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:separator_missing)
+
+          commit_linter.lint
+        end
+      end
+    end
+
+    describe 'details' do
+      context 'when details are valid' do
+        let(:commit_message) { "A B C\n\nMore details." }
+
+        it_behaves_like 'a valid commit'
+      end
+
+      context 'when no details are given and many files are changed' do
+        let(:total_files_changed) { described_class::MAX_CHANGED_FILES_IN_COMMIT + 1 }
+
+        it_behaves_like 'a valid commit'
+      end
+
+      context 'when no details are given and many lines are changed' do
+        let(:total_lines_changed) { described_class::MAX_CHANGED_LINES_IN_COMMIT + 1 }
+
+        it_behaves_like 'a valid commit'
+      end
+
+      context 'when no details are given and many files and lines are changed' do
+        let(:total_files_changed) { described_class::MAX_CHANGED_FILES_IN_COMMIT + 1 }
+        let(:total_lines_changed) { described_class::MAX_CHANGED_LINES_IN_COMMIT + 1 }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:details_too_many_changes)
+
+          commit_linter.lint
+        end
+      end
+
+      context 'when details exceeds the max line length' do
+        let(:commit_message) { "A B C\n\n" + 'D' * (described_class::MAX_LINE_LENGTH + 1) }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:details_line_too_long)
+
+          commit_linter.lint
+        end
+      end
+
+      context 'when details exceeds the max line length including a URL' do
+        let(:commit_message) { "A B C\n\nhttps://gitlab.com" + 'D' * described_class::MAX_LINE_LENGTH }
+
+        it_behaves_like 'a valid commit'
+      end
+    end
+
+    describe 'message' do
+      context 'when message includes a text emoji' do
+        let(:commit_message) { "A commit message :+1:" }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:message_contains_text_emoji)
+
+          commit_linter.lint
+        end
+      end
+
+      context 'when message includes a unicode emoji' do
+        let(:commit_message) { "A commit message 🚀" }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:message_contains_unicode_emoji)
+
+          commit_linter.lint
+        end
+      end
+
+      context 'when message includes a short reference' do
+        [
+          'A commit message to fix #1234',
+          'A commit message to fix !1234',
+          'A commit message to fix &1234',
+          'A commit message to fix %1234',
+          'A commit message to fix gitlab#1234',
+          'A commit message to fix gitlab!1234',
+          'A commit message to fix gitlab&1234',
+          'A commit message to fix gitlab%1234',
+          'A commit message to fix gitlab-org/gitlab#1234',
+          'A commit message to fix gitlab-org/gitlab!1234',
+          'A commit message to fix gitlab-org/gitlab&1234',
+          'A commit message to fix gitlab-org/gitlab%1234'
+        ].each do |message|
+          let(:commit_message) { message }
+
+          it 'adds a problem' do
+            expect(commit_linter).to receive(:add_problem).with(:message_contains_short_reference)
+
+            commit_linter.lint
+          end
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/danger/emoji_checker_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'rspec-parameterized'
+
+require 'gitlab/danger/emoji_checker'
+
+describe Gitlab::Danger::EmojiChecker do
+  using RSpec::Parameterized::TableSyntax
+
+  describe '#includes_text_emoji?' do
+    where(:text, :includes_emoji) do
+      'Hello World!' | false
+      ':+1:' | true
+      'Hello World! :+1:' | true
+    end
+
+    with_them do
+      it 'is true when text includes a text emoji' do
+        expect(subject.includes_text_emoji?(text)).to be(includes_emoji)
+      end
+    end
+  end
+
+  describe '#includes_unicode_emoji?' do
+    where(:text, :includes_emoji) do
+      'Hello World!' | false
+      '🚀' | true
+      'Hello World! 🚀' | true
+    end
+
+    with_them do
+      it 'is true when text includes a text emoji' do
+        expect(subject.includes_unicode_emoji?(text)).to be(includes_emoji)
+      end
+    end
+  end
+end

spec/lib/gitlab/danger/helper_spec.rb

@@ -313,6 +313,19 @@ describe Gitlab::Danger::Helper do
     end
   end
 
+  describe '#sanitize_mr_title' do
+    where(:mr_title, :expected_mr_title) do
+      'My MR title'      | 'My MR title'
+      'WIP: My MR title' | 'My MR title'
+    end
+
+    with_them do
+      subject { helper.sanitize_mr_title(mr_title) }
+
+      it { is_expected.to eq(expected_mr_title) }
+    end
+  end
+
   describe '#security_mr?' do
     it 'returns false when `gitlab_helper` is unavailable' do
       expect(helper).to receive(:gitlab_helper).and_return(nil)