Add CommitMetadataChecker

Checks commits for possible @username additions, and raises errors for
any valid offenses.

Uses GitHub username rules, and will perform the least number of github
username lookups possible to warn for usernames.  Does not outright call
offenses errors, as code examples could be used in the commit messages,
however, ideally if a message is shown it would be courteous to change
the code example to something that doesn't ping a user's email on every
commit update.

Fixes #414

app/workers/commit_monitor_handlers/commit_range/github_pr_commenter.rb

@@ -7,7 +7,7 @@ class GithubPrCommenter
     include BranchWorkerMixin
 
     def self.batch_workers
-      [DiffContentChecker, DiffFilenameChecker]
+      [DiffContentChecker, DiffFilenameChecker, CommitMetadataChecker]
     end
 
     def self.handled_branch_modes

app/workers/commit_monitor_handlers/commit_range/github_pr_commenter/commit_metadata_checker.rb

@@ -0,0 +1,62 @@
+require 'rugged'
+
+module CommitMonitorHandlers::CommitRange
+  class GithubPrCommenter::CommitMetadataChecker
+    include Sidekiq::Worker
+    sidekiq_options :queue => :miq_bot_glacial
+
+    include BatchEntryWorkerMixin
+    include BranchWorkerMixin
+
+    def perform(batch_entry_id, branch_id, new_commits)
+      return unless find_batch_entry(batch_entry_id)
+      return skip_batch_entry unless find_branch(branch_id, :pr)
+
+      complete_batch_entry(:result => process_commits(new_commits))
+    end
+
+    private
+
+    def process_commits(new_commits)
+      @offenses = []
+
+      new_commits.each do |commit_sha, data|
+        check_for_usernames_in(commit_sha, data["message"])
+      end
+
+      @offenses
+    end
+
+    # From https://github.com/join
+    #
+    #     "Username may only contain alphanumeric characters or single hyphens,
+    #     and cannot begin or end with a hyphen."
+    #
+    # For the beginning and and, we do a positive lookbehind at the beginning
+    # to get the `@`, and a positive lookhead at the end to confirm their is
+    # either a period or a whitespace following the "var" (instance_variable)
+    #
+    # Since there can't be underscores in Github usernames, this makes it so we
+    # rule out partial matches of variables (@database_records having a
+    # username lookup of `database`), but still catch full variable names
+    # without underscores (`@foobarbaz`).
+    #
+    USERNAME_REGEXP = /
+      (?<=@)          # must start with a '@' (don't capture)
+      [a-zA-Z0-9]     # first character must be alphanumeric
+      [a-zA-Z0-9\-]*  # middle chars may be alphanumeric or hyphens
+      [a-zA-Z0-9]     # last character must be alphanumeric
+      (?=[\.\s])      # allow only variables without "_" (not captured)
+    /x.freeze
+
+    def check_for_usernames_in(commit, message)
+      message.scan(USERNAME_REGEXP).each do |potential_username|
+        next unless GithubService.username_lookup(potential_username)
+
+        group   = ::Branch.github_commit_uri(fq_repo_name, commit)
+        message = "Username `@#{potential_username}` detected in commit message. Consider removing."
+        @offenses << OffenseMessage::Entry.new(:low, message, group)
+      end
+    end
+  end
+end

spec/workers/commit_monitor_handlers/commit_range/github_pr_commenter/commit_metadata_checker_spec.rb

@@ -0,0 +1,98 @@
+describe CommitMonitorHandlers::CommitRange::GithubPrCommenter::CommitMetadataChecker do
+  let(:batch_entry)        { BatchEntry.create!(:job => BatchJob.create!) }
+  let(:branch)             { create(:pr_branch) }
+  let(:commit_message_1)   { "Adds SUPER FEATURE.  BREAKS NOTHING!" }
+  let(:commit_message_2)   { "fix tests" }
+  let(:commit_message_3)   { "fix moar tests" }
+
+  let(:commits) do
+    {
+      "abcd123" => {"message" => commit_message_1, "files" => ["app/models/super.rb"]},
+      "abcd234" => {"message" => commit_message_2, "files" => ["spec/models/super_spec.rb"]},
+      "abcd345" => {"message" => commit_message_3, "files" => ["spec/models/normal_spec.rb"]}
+    }
+  end
+
+  let(:username_lookup_cache) do
+    {
+      "Fryguy"          => 123, # valid user
+      "NickLaMuro"      => 234, # valid user
+      "NickLaMura"      => nil, # mispelled invalid user
+      "Nick-LaMuro"     => nil, # invalid user with hyphen
+      "booksandauthors" => nil  # not a user (surprisingly)
+    }
+  end
+
+  before do
+    stub_sidekiq_logger
+    stub_job_completion
+
+    allow(GithubService).to receive(:username_lookup_cache).and_return(username_lookup_cache)
+  end
+
+  context "with basic commit messages" do
+    it "doesn't create any offenses" do
+      described_class.new.perform(batch_entry.id, branch.id, commits)
+
+      batch_entry.reload
+      expect(batch_entry.result.length).to eq(0)
+    end
+  end
+
+  context "with multiline commit messages" do
+    let(:commit_message_1) do
+      <<~COMMIT_MSG
+        In which the hero adds the greatest feature...
+
+        With this change, I made it so that by changing this:
+
+            @database_records = Books.all
+
+        Avoids a N+1 on authors by adding an .includes call
+
+            @booksandauthors = Books.all.includes(:author)
+
+        Saving us 100s of queries for our tiny bookstore app.
+
+        Thanks to @Fryguy and @Nick-LaMuro for this suggestion!
+      COMMIT_MSG
+    end
+
+    let(:commit_message_2) do
+      <<~COMMIT_MSG
+        fixes tests
+
+        Forgot that we stubbed things...
+
+        cc @Fryguy @NickLaMuro
+      COMMIT_MSG
+    end
+
+    let(:commit_message_3) do
+      <<~COMMIT_MSG
+        fixes moar tests
+
+        I forget how to rebase... #dealWithIt @NickLaMura
+      COMMIT_MSG
+    end
+
+    it "returns one offense for each valid username" do
+      described_class.new.perform(batch_entry.id, branch.id, commits)
+
+      batch_entry.reload
+      expect(batch_entry.result.length).to eq(3)
+      expect(batch_entry.result.first).to have_attributes(
+        :group   => "https://github.com/#{branch.fq_repo_name}/commit/abcd123",
+        :message => "Username `@Fryguy` detected in commit message. Consider removing."
+      )
+      expect(batch_entry.result.second).to have_attributes(
+        :group   => "https://github.com/#{branch.fq_repo_name}/commit/abcd234",
+        :message => "Username `@Fryguy` detected in commit message. Consider removing."
+      )
+      expect(batch_entry.result.third).to have_attributes(
+        :group   => "https://github.com/#{branch.fq_repo_name}/commit/abcd234",
+        :message => "Username `@NickLaMuro` detected in commit message. Consider removing."
+      )
+    end
+  end
+end