Merge branch '15463-extract-hooks-service' into 'master'

Extract a Git::HooksService

Closes #15463

See merge request gitlab-org/gitlab-ce!26726

app/models/gpg_signature.rb

@@ -38,6 +38,15 @@ class GpgSignature < ApplicationRecord
       .safe_find_or_create_by!(commit_sha: attributes[:commit_sha])
   end
 
+  # Find commits that are lacking a signature in the database at present
+  def self.unsigned_commit_shas(commit_shas)
+    return [] if commit_shas.empty?
+
+    signed = GpgSignature.where(commit_sha: commit_shas).pluck(:commit_sha)
+
+    commit_shas - signed
+  end
+
   def gpg_key=(model)
     case model
     when GpgKey

app/services/after_branch_delete_service.rb

-# frozen_string_literal: true
-
-# Branch can be deleted either by DeleteBranchService or by Git::BranchPushService.
-class AfterBranchDeleteService < BaseService
-  attr_reader :branch_name
-
-  def execute(branch_name)
-    @branch_name = branch_name
-
-    stop_environments
-  end
-
-  private
-
-  def stop_environments
-    Ci::StopEnvironmentsService
-      .new(project, current_user)
-      .execute(branch_name)
-  end
-end

app/services/delete_branch_service.rb

@@ -29,14 +29,4 @@ class DeleteBranchService < BaseService
   def success(message)
     super().merge(message: message)
   end
-
-  def build_push_data(branch)
-    Gitlab::DataBuilder::Push.build(
-      project,
-      current_user,
-      branch.dereferenced_target.sha,
-      Gitlab::Git::BLANK_SHA,
-      "#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch.name}",
-      [])
-  end
 end

app/services/git/base_hooks_service.rb

+# frozen_string_literal: true
+
+module Git
+  class BaseHooksService < ::BaseService
+    include Gitlab::Utils::StrongMemoize
+
+    # The N most recent commits to process in a single push payload.
+    PROCESS_COMMIT_LIMIT = 100
+
+    def execute
+      project.repository.after_create if project.empty_repo?
+
+      create_events
+      create_pipelines
+      execute_project_hooks
+
+      # Not a hook, but it needs access to the list of changed commits
+      enqueue_invalidate_cache
+
+      push_data
+    end
+
+    private
+
+    def hook_name
+      raise NotImplementedError, "Please implement #{self.class}##{__method__}"
+    end
+
+    def commits
+      raise NotImplementedError, "Please implement #{self.class}##{__method__}"
+    end
+
+    def limited_commits
+      commits.last(PROCESS_COMMIT_LIMIT)
+    end
+
+    def commits_count
+      commits.count
+    end
+
+    def event_message
+      nil
+    end
+
+    def invalidated_file_types
+      []
+    end
+
+    def create_events
+      EventCreateService.new.push(project, current_user, push_data)
+    end
+
+    def create_pipelines
+      Ci::CreatePipelineService
+        .new(project, current_user, push_data)
+        .execute(:push, pipeline_options)
+    end
+
+    def execute_project_hooks
+      project.execute_hooks(push_data, hook_name)
+      project.execute_services(push_data, hook_name)
+    end
+
+    def enqueue_invalidate_cache
+      ProjectCacheWorker.perform_async(
+        project.id,
+        invalidated_file_types,
+        [:commit_count, :repository_size]
+      )
+    end
+
+    def push_data
+      @push_data ||= Gitlab::DataBuilder::Push.build(
+        project,
+        current_user,
+        params[:oldrev],
+        params[:newrev],
+        params[:ref],
+        limited_commits,
+        event_message,
+        commits_count: commits_count,
+        push_options: params[:push_options] || []
+      )
+
+      # Dependent code may modify the push data, so return a duplicate each time
+      @push_data.dup
+    end
+
+    # to be overridden in EE
+    def pipeline_options
+      {}
+    end
+  end
+end

app/services/git/branch_hooks_service.rb

+# frozen_string_literal: true
+
+module Git
+  class BranchHooksService < ::Git::BaseHooksService
+    def execute
+      execute_branch_hooks
+
+      super.tap do
+        enqueue_update_gpg_signatures
+      end
+    end
+
+    private
+
+    def hook_name
+      :push_hooks
+    end
+
+    def commits
+      strong_memoize(:commits) do
+        if creating_default_branch?
+          # The most recent PROCESS_COMMIT_LIMIT commits in the default branch
+          offset = [count_commits_in_branch - PROCESS_COMMIT_LIMIT, 0].max
+          project.repository.commits(params[:newrev], offset: offset, limit: PROCESS_COMMIT_LIMIT)
+        elsif creating_branch?
+          # Use the pushed commits that aren't reachable by the default branch
+          # as a heuristic. This may include more commits than are actually
+          # pushed, but that shouldn't matter because we check for existing
+          # cross-references later.
+          project.repository.commits_between(project.default_branch, params[:newrev])
+        elsif updating_branch?
+          project.repository.commits_between(params[:oldrev], params[:newrev])
+        else # removing branch
+          []
+        end
+      end
+    end
+
+    def commits_count
+      return count_commits_in_branch if creating_default_branch?
+
+      super
+    end
+
+    def invalidated_file_types
+      return super unless default_branch? && !creating_branch?
+
+      paths = limited_commits.each_with_object(Set.new) do |commit, set|
+        commit.raw_deltas.each do |diff|
+          set << diff.new_path
+        end
+      end
+
+      Gitlab::FileDetector.types_in_paths(paths)
+    end
+
+    def execute_branch_hooks
+      project.repository.after_push_commit(branch_name)
+
+      branch_create_hooks if creating_branch?
+      branch_update_hooks if updating_branch?
+      branch_change_hooks if creating_branch? || updating_branch?
+      branch_remove_hooks if removing_branch?
+    end
+
+    def branch_create_hooks
+      project.repository.after_create_branch
+      project.after_create_default_branch if default_branch?
+    end
+
+    def branch_update_hooks
+      # Update the bare repositories info/attributes file using the contents of
+      # the default branch's .gitattributes file
+      project.repository.copy_gitattributes(params[:ref]) if default_branch?
+    end
+
+    def branch_change_hooks
+      enqueue_process_commit_messages
+    end
+
+    def branch_remove_hooks
+      project.repository.after_remove_branch
+    end
+
+    # Schedules processing of commit messages
+    def enqueue_process_commit_messages
+      # don't process commits for the initial push to the default branch
+      return if creating_default_branch?
+
+      limited_commits.each do |commit|
+        next unless commit.matches_cross_reference_regex?
+
+        ProcessCommitWorker.perform_async(
+          project.id,
+          current_user.id,
+          commit.to_hash,
+          default_branch?
+        )
+      end
+    end
+
+    def enqueue_update_gpg_signatures
+      unsigned = GpgSignature.unsigned_commit_shas(limited_commits.map(&:sha))
+      return if unsigned.empty?
+
+      signable = Gitlab::Git::Commit.shas_with_signatures(project.repository, unsigned)
+      return if signable.empty?
+
+      CreateGpgSignatureWorker.perform_async(signable, project.id)
+    end
+
+    def creating_branch?
+      Gitlab::Git.blank_ref?(params[:oldrev])
+    end
+
+    def updating_branch?
+      !creating_branch? && !removing_branch?
+    end
+
+    def removing_branch?
+      Gitlab::Git.blank_ref?(params[:newrev])
+    end
+
+    def creating_default_branch?
+      creating_branch? && default_branch?
+    end
+
+    def count_commits_in_branch
+      strong_memoize(:count_commits_in_branch) do
+        project.repository.commit_count_for_ref(params[:ref])
+      end
+    end
+
+    def default_branch?
+      strong_memoize(:default_branch) do
+        [nil, branch_name].include?(project.default_branch)
+      end
+    end
+
+    def branch_name
+      strong_memoize(:branch_name) { Gitlab::Git.ref_name(params[:ref]) }
+    end
+  end
+end

app/services/git/branch_push_service.rb

 # frozen_string_literal: true
 
 module Git
-  class BranchPushService < BaseService
-    attr_accessor :push_data, :push_commits
+  class BranchPushService < ::BaseService
     include Gitlab::Access
     include Gitlab::Utils::StrongMemoize
 
-    # The N most recent commits to process in a single push payload.
-    PROCESS_COMMIT_LIMIT = 100
-
     # This method will be called after each git update
     # and only if the provided user and project are present in GitLab.
     #
@@ -23,108 +19,43 @@ module Git
     #  6. Checks if the project's main language has changed
     #
     def execute
-      update_commits
+      return unless Gitlab::Git.branch_ref?(params[:ref])
+
+      enqueue_update_mrs
+      enqueue_detect_repository_languages
+
       execute_related_hooks
       perform_housekeeping
 
       update_remote_mirrors
-      update_caches
+      stop_environments
 
-      update_signatures
+      true
     end
 
-    def update_commits
-      project.repository.after_create if project.empty_repo?
-      project.repository.after_push_commit(branch_name)
-
-      if push_remove_branch?
-        project.repository.after_remove_branch
-        @push_commits = []
-      elsif push_to_new_branch?
-        project.repository.after_create_branch
-
-        # Re-find the pushed commits.
-        if default_branch?
-          # Initial push to the default branch. Take the full history of that branch as "newly pushed".
-          process_default_branch
-        else
-          # Use the pushed commits that aren't reachable by the default branch
-          # as a heuristic. This may include more commits than are actually pushed, but
-          # that shouldn't matter because we check for existing cross-references later.
-          @push_commits = project.repository.commits_between(project.default_branch, params[:newrev])
-
-          # don't process commits for the initial push to the default branch
-          process_commit_messages
-        end
-      elsif push_to_existing_branch?
-        # Collect data for this git push
-        @push_commits = project.repository.commits_between(params[:oldrev], params[:newrev])
-
-        process_commit_messages
-
-        # Update the bare repositories info/attributes file using the contents of the default branches
-        # .gitattributes file
-        update_gitattributes if default_branch?
-      end
-    end
-
-    def update_gitattributes
-      project.repository.copy_gitattributes(params[:ref])
-    end
-
-    def update_caches
-      if default_branch?
-        if push_to_new_branch?
-          # If this is the initial push into the default branch, the file type caches
-          # will already be reset as a result of `Project#change_head`.
-          types = []
-        else
-          paths = Set.new
-
-          last_pushed_commits.each do |commit|
-            commit.raw_deltas.each do |diff|
-              paths << diff.new_path
-            end
-          end
-
-          types = Gitlab::FileDetector.types_in_paths(paths.to_a)
-        end
-
-        DetectRepositoryLanguagesWorker.perform_async(@project.id, current_user.id)
-      else
-        types = []
-      end
-
-      ProjectCacheWorker.perform_async(project.id, types, [:commit_count, :repository_size])
+    # Update merge requests that may be affected by this push. A new branch
+    # could cause the last commit of a merge request to change.
+    def enqueue_update_mrs
+      UpdateMergeRequestsWorker.perform_async(
+        project.id,
+        current_user.id,
+        params[:oldrev],
+        params[:newrev],
+        params[:ref]
+      )
     end
 
-    # rubocop: disable CodeReuse/ActiveRecord
-    def update_signatures
-      commit_shas = last_pushed_commits.map(&:sha)
+    def enqueue_detect_repository_languages
+      return unless default_branch?
 
-      return if commit_shas.empty?
-
-      shas_with_cached_signatures = GpgSignature.where(commit_sha: commit_shas).pluck(:commit_sha)
-      commit_shas -= shas_with_cached_signatures
-
-      return if commit_shas.empty?
-
-      commit_shas = Gitlab::Git::Commit.shas_with_signatures(project.repository, commit_shas)
-
-      CreateGpgSignatureWorker.perform_async(commit_shas, project.id)
+      DetectRepositoryLanguagesWorker.perform_async(project.id, current_user.id)
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
-    # Schedules processing of commit messages.
-    def process_commit_messages
-      default = default_branch?
+    # Only stop environments if the ref is a branch that is being deleted
+    def stop_environments
+      return unless removing_branch?
 
-      last_pushed_commits.each do |commit|
-        if commit.matches_cross_reference_regex?
-          ProcessCommitWorker
-            .perform_async(project.id, current_user.id, commit.to_hash, default)
-        end
-      end
+      Ci::StopEnvironmentsService.new(project, current_user).execute(branch_name)
     end
 
     def update_remote_mirrors
@@ -135,23 +66,7 @@ module Git
     end
 
     def execute_related_hooks
-      # Update merge requests that may be affected by this push. A new branch
-      # could cause the last commit of a merge request to change.
-      #
-      UpdateMergeRequestsWorker
-        .perform_async(project.id, current_user.id, params[:oldrev], params[:newrev], params[:ref])
-
-      EventCreateService.new.push(project, current_user, build_push_data)
-      Ci::CreatePipelineService.new(project, current_user, build_push_data).execute(:push, pipeline_options)
-
-      project.execute_hooks(build_push_data.dup, :push_hooks)
-      project.execute_services(build_push_data.dup, :push_hooks)
-
-      if push_remove_branch?
-        AfterBranchDeleteService
-          .new(project, current_user)
-          .execute(branch_name)
-      end
+      BranchHooksService.new(project, current_user, params).execute
     end
 
     def perform_housekeeping
@@ -161,84 +76,18 @@ module Git
     rescue Projects::HousekeepingService::LeaseTaken
     end
 
-    def process_default_branch
-      offset = [push_commits_count_for_ref - PROCESS_COMMIT_LIMIT, 0].max
-      @push_commits = project.repository.commits(params[:newrev], offset: offset, limit: PROCESS_COMMIT_LIMIT)
-
-      project.after_create_default_branch
-    end
-
-    def build_push_data
-      @push_data ||= Gitlab::DataBuilder::Push.build(
-        project,
-        current_user,
-        params[:oldrev],
-        params[:newrev],
-        params[:ref],
-        @push_commits,
-        commits_count: commits_count,
-        push_options: params[:push_options] || []
-      )
-    end
-
-    def push_to_existing_branch?
-      # Return if this is not a push to a branch (e.g. new commits)
-      branch_ref? && !Gitlab::Git.blank_ref?(params[:oldrev])
-    end
-
-    def push_to_new_branch?
-      strong_memoize(:push_to_new_branch) do
-        branch_ref? && Gitlab::Git.blank_ref?(params[:oldrev])
-      end
-    end
-
-    def push_remove_branch?
-      strong_memoize(:push_remove_branch) do
-        branch_ref? && Gitlab::Git.blank_ref?(params[:newrev])
-      end
-    end
-
-    def default_branch?
-      branch_ref? &&
-        (branch_name == project.default_branch || project.default_branch.nil?)
-    end
-
-    def commit_user(commit)
-      commit.author || current_user
+    def removing_branch?
+      Gitlab::Git.blank_ref?(params[:newrev])
     end
 
     def branch_name
-      strong_memoize(:branch_name) do
-        Gitlab::Git.ref_name(params[:ref])
-      end
+      strong_memoize(:branch_name) { Gitlab::Git.ref_name(params[:ref]) }
     end
 
-    def branch_ref?
-      strong_memoize(:branch_ref) do
-        Gitlab::Git.branch_ref?(params[:ref])
-      end
-    end
-
-    def commits_count
-      return push_commits_count_for_ref if default_branch? && push_to_new_branch?
-
-      Array(@push_commits).size
-    end
-
-    def push_commits_count_for_ref
-      strong_memoize(:push_commits_count_for_ref) do
-        project.repository.commit_count_for_ref(params[:ref])
+    def default_branch?
+      strong_memoize(:default_branch) do
+        [nil, branch_name].include?(project.default_branch)
       end
     end
-
-    def last_pushed_commits
-      @last_pushed_commits ||= @push_commits.last(PROCESS_COMMIT_LIMIT)
-    end
-
-    private
-
-    def pipeline_options
-      {} # to be overridden in EE
-    end
   end
 end

app/services/git/tag_hooks_service.rb

+# frozen_string_literal: true
+
+module Git
+  class TagHooksService < ::Git::BaseHooksService
+    private
+
+    def hook_name
+      :tag_push_hooks
+    end
+
+    def commits
+      [tag_commit].compact
+    end
+
+    def event_message
+      tag&.message
+    end
+
+    def tag
+      strong_memoize(:tag) do
+        next if Gitlab::Git.blank_ref?(params[:newrev])
+
+        tag_name = Gitlab::Git.ref_name(params[:ref])
+        tag = project.repository.find_tag(tag_name)
+
+        tag if tag && tag.target == params[:newrev]
+      end
+    end
+
+    def tag_commit
+      strong_memoize(:tag_commit) do
+        project.commit(tag.dereferenced_target) if tag
+      end
+    end
+  end
+end

app/services/git/tag_push_service.rb

 # frozen_string_literal: true
 
 module Git
-  class TagPushService < BaseService
-    attr_accessor :push_data
-
+  class TagPushService < ::BaseService
     def execute
-      project.repository.after_create if project.empty_repo?
-      project.repository.before_push_tag
-
-      @push_data = build_push_data
-
-      EventCreateService.new.push(project, current_user, push_data)
-      Ci::CreatePipelineService.new(project, current_user, push_data).execute(:push, pipeline_options)
+      return unless Gitlab::Git.tag_ref?(params[:ref])
 
-      project.execute_hooks(push_data.dup, :tag_push_hooks)
-      project.execute_services(push_data.dup, :tag_push_hooks)
-
-      ProjectCacheWorker.perform_async(project.id, [], [:commit_count, :repository_size])
+      project.repository.before_push_tag
+      TagHooksService.new(project, current_user, params).execute
 
       true
     end
-
-    private
-
-    def build_push_data
-      commits = []
-      message = nil
-
-      unless Gitlab::Git.blank_ref?(params[:newrev])
-        tag_name = Gitlab::Git.ref_name(params[:ref])
-        tag = project.repository.find_tag(tag_name)
-
-        if tag && tag.target == params[:newrev]
-          commit = project.commit(tag.dereferenced_target)
-          commits = [commit].compact
-          message = tag.message
-        end
-      end
-
-      Gitlab::DataBuilder::Push.build(
-        project,
-        current_user,
-        params[:oldrev],
-        params[:newrev],
-        params[:ref],
-        commits,
-        message,
-        push_options: params[:push_options] || [])
-    end
-
-    def pipeline_options
-      {} # to be overridden in EE
-    end
   end
 end

spec/services/after_branch_delete_service_spec.rb

-require 'spec_helper'
-
-describe AfterBranchDeleteService do
-  let(:project) { create(:project, :repository) }
-  let(:user) { create(:user) }
-  let(:service) { described_class.new(project, user) }
-
-  describe '#execute' do
-    it 'stops environments attached to branch' do
-      expect(service).to receive(:stop_environments)
-
-      service.execute('feature')
-    end
-  end
-end

spec/services/git/branch_hooks_service_spec.rb

+require 'spec_helper'
+
+describe Git::BranchHooksService do
+  include RepoHelpers
+
+  let(:project) { create(:project, :repository) }
+  let(:user) { project.creator }
+
+  let(:branch) { project.default_branch }
+  let(:ref) { "refs/heads/#{branch}" }
+  let(:commit) { project.commit(sample_commit.id) }
+  let(:oldrev) { commit.parent_id }
+  let(:newrev) { commit.id }
+
+  let(:service) do
+    described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref)
+  end
+
+  describe "Git Push Data" do
+    subject(:push_data) { service.execute }
+
+    it 'has expected push data attributes' do
+      is_expected.to match a_hash_including(
+        object_kind: 'push',
+        before: oldrev,
+        after: newrev,
+        ref: ref,
+        user_id: user.id,
+        user_name: user.name,
+        project_id: project.id
+      )
+    end
+
+    context "with repository data" do
+      subject { push_data[:repository] }
+
+      it 'has expected attributes' do
+        is_expected.to match a_hash_including(
+          name: project.name,
+          url: project.url_to_repo,
+          description: project.description,
+          homepage: project.web_url
+        )
+      end
+    end
+
+    context "with commits" do
+      subject { push_data[:commits] }
+
+      it { is_expected.to be_an(Array) }
+
+      it 'has 1 element' do
+        expect(subject.size).to eq(1)
+      end
+
+      context "the commit" do
+        subject { push_data[:commits].first }
+
+        it { expect(subject[:timestamp].in_time_zone).to eq(commit.date.in_time_zone) }
+
+        it 'includes expected commit data' do
+          is_expected.to match a_hash_including(
+            id: commit.id,
+            message: commit.safe_message,
+            url: [
+              Gitlab.config.gitlab.url,
+              project.namespace.to_param,
+              project.to_param,
+              'commit',
+              commit.id
+            ].join('/')
+          )
+        end
+
+        context "with a author" do
+          subject { push_data[:commits].first[:author] }
+
+          it 'includes expected author data' do
+            is_expected.to match a_hash_including(
+              name: commit.author_name,
+              email: commit.author_email
+            )
+          end
+        end
+      end
+    end
+  end
+
+  describe 'Push Event' do
+    let(:event) { Event.find_by_action(Event::PUSHED) }
+
+    before do
+      service.execute
+    end
+
+    context "with an existing branch" do
+      it 'generates a push event with one commit' do
+        expect(event).to be_an_instance_of(PushEvent)
+        expect(event.project).to eq(project)
+        expect(event.action).to eq(Event::PUSHED)
+        expect(event.push_event_payload).to be_an_instance_of(PushEventPayload)
+        expect(event.push_event_payload.commit_from).to eq(oldrev)
+        expect(event.push_event_payload.commit_to).to eq(newrev)
+        expect(event.push_event_payload.ref).to eq('master')
+        expect(event.push_event_payload.commit_count).to eq(1)
+      end
+    end
+
+    context "with a new branch" do
+      let(:oldrev) { Gitlab::Git::BLANK_SHA }
+
+      it 'generates a push event with more than one commit' do
+        expect(event).to be_an_instance_of(PushEvent)
+        expect(event.project).to eq(project)
+        expect(event.action).to eq(Event::PUSHED)
+        expect(event.push_event_payload).to be_an_instance_of(PushEventPayload)
+        expect(event.push_event_payload.commit_from).to be_nil
+        expect(event.push_event_payload.commit_to).to eq(newrev)
+        expect(event.push_event_payload.ref).to eq('master')
+        expect(event.push_event_payload.commit_count).to be > 1
+      end
+    end
+
+    context 'removing a branch' do
+      let(:newrev) { Gitlab::Git::BLANK_SHA }
+
+      it 'generates a push event with no commits' do
+        expect(event).to be_an_instance_of(PushEvent)
+        expect(event.project).to eq(project)
+        expect(event.action).to eq(Event::PUSHED)
+        expect(event.push_event_payload).to be_an_instance_of(PushEventPayload)
+        expect(event.push_event_payload.commit_from).to eq(oldrev)
+        expect(event.push_event_payload.commit_to).to be_nil
+        expect(event.push_event_payload.ref).to eq('master')
+        expect(event.push_event_payload.commit_count).to eq(0)
+      end
+    end
+  end
+
+  describe 'Invalidating project cache' do
+    let(:commit_id) do
+      project.repository.update_file(
+        user, 'README.md', '', message: 'Update', branch_name: branch
+      )
+    end
+
+    let(:commit) { project.repository.commit(commit_id) }
+    let(:blank_sha) { Gitlab::Git::BLANK_SHA }
+
+    def clears_cache(extended: [])
+      expect(ProjectCacheWorker)
+        .to receive(:perform_async)
+        .with(project.id, extended, %i[commit_count repository_size])
+
+      service.execute
+    end
+
+    def clears_extended_cache
+      clears_cache(extended: %i[readme])
+    end
+
+    context 'on default branch' do
+      context 'create' do
+        # FIXME: When creating the default branch,the cache worker runs twice
+        before do
+          allow(ProjectCacheWorker).to receive(:perform_async)
+        end
+
+        let(:oldrev) { blank_sha }
+
+        it { clears_cache }
+      end
+
+      context 'update' do
+        it { clears_extended_cache }
+      end
+
+      context 'remove' do
+        let(:newrev) { blank_sha }
+
+        # TODO: this case should pass, but we only take account of added files
+        it { clears_cache }
+      end
+    end
+
+    context 'on ordinary branch' do
+      let(:branch) { 'fix' }
+
+      context 'create' do
+        let(:oldrev) { blank_sha }
+
+        it { clears_cache }
+      end
+
+      context 'update' do
+        it { clears_cache }
+      end
+
+      context 'remove' do
+        let(:newrev) { blank_sha }
+
+        it { clears_cache }
+      end
+    end
+  end
+
+  describe 'GPG signatures' do
+    context 'when the commit has a signature' do
+      context 'when the signature is already cached' do
+        before do
+          create(:gpg_signature, commit_sha: commit.id)
+        end
+
+        it 'does not queue a CreateGpgSignatureWorker' do
+          expect(CreateGpgSignatureWorker).not_to receive(:perform_async)
+
+          service.execute
+        end
+      end
+
+      context 'when the signature is not yet cached' do
+        it 'queues a CreateGpgSignatureWorker' do
+          expect(CreateGpgSignatureWorker).to receive(:perform_async).with([commit.id], project.id)
+
+          service.execute
+        end
+
+        it 'can queue several commits to create the gpg signature' do
+          allow(Gitlab::Git::Commit)
+            .to receive(:shas_with_signatures)
+            .and_return([sample_commit.id, another_sample_commit.id])
+
+          expect(CreateGpgSignatureWorker)
+            .to receive(:perform_async)
+            .with([sample_commit.id, another_sample_commit.id], project.id)
+
+          service.execute
+        end
+      end
+    end
+
+    context 'when the commit does not have a signature' do
+      before do
+        allow(Gitlab::Git::Commit)
+          .to receive(:shas_with_signatures)
+          .with(project.repository, [sample_commit.id])
+          .and_return([])
+      end
+
+      it 'does not queue a CreateGpgSignatureWorker' do
+        expect(CreateGpgSignatureWorker)
+          .not_to receive(:perform_async)
+          .with(sample_commit.id, project.id)
+
+        service.execute
+      end
+    end
+  end
+
+  describe 'Processing commit messages' do
+    # Create 4 commits, 2 of which have references. Limiting to 2 commits, we
+    # expect to see one commit message processor enqueued.
+    let(:commit_ids) do
+      Array.new(4) do |i|
+        message = "Issue #{'#' if i.even?}#{i}"
+        project.repository.update_file(
+          user, 'README.md', '', message: message, branch_name: branch
+        )
+      end
+    end
+
+    let(:oldrev) { commit_ids.first }
+    let(:newrev) { commit_ids.last }
+
+    before do
+      stub_const("::Git::BaseHooksService::PROCESS_COMMIT_LIMIT", 2)
+    end
+
+    context 'creating the default branch' do
+      let(:oldrev) { Gitlab::Git::BLANK_SHA }
+
+      it 'does not process commit messages' do
+        expect(ProcessCommitWorker).not_to receive(:perform_async)
+
+        service.execute
+      end
+    end
+
+    context 'updating the default branch' do
+      it 'processes a limited number of commit messages' do
+        expect(ProcessCommitWorker).to receive(:perform_async).once
+
+        service.execute
+      end
+    end
+
+    context 'removing the default branch' do
+      let(:newrev) { Gitlab::Git::BLANK_SHA }
+
+      it 'does not process commit messages' do
+        expect(ProcessCommitWorker).not_to receive(:perform_async)
+
+        service.execute
+      end
+    end
+
+    context 'creating a normal branch' do
+      let(:branch) { 'fix' }
+      let(:oldrev) { Gitlab::Git::BLANK_SHA }
+
+      it 'processes a limited number of commit messages' do
+        expect(ProcessCommitWorker).to receive(:perform_async).once
+
+        service.execute
+      end
+    end
+
+    context 'updating a normal branch' do
+      let(:branch) { 'fix' }
+
+      it 'processes a limited number of commit messages' do
+        expect(ProcessCommitWorker).to receive(:perform_async).once
+
+        service.execute
+      end
+    end
+
+    context 'removing a normal branch' do
+      let(:branch) { 'fix' }
+      let(:newrev) { Gitlab::Git::BLANK_SHA }
+
+      it 'does not process commit messages' do
+        expect(ProcessCommitWorker).not_to receive(:perform_async)
+
+        service.execute
+      end
+    end
+  end
+end

spec/services/git/tag_hooks_service_spec.rb

+require 'spec_helper'
+
+describe Git::TagHooksService, :service do
+  let(:user) { create(:user) }
+  let(:project) { create(:project, :repository) }
+
+  let(:oldrev) { Gitlab::Git::BLANK_SHA }
+  let(:newrev) { "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b" } # gitlab-test: git rev-parse refs/tags/v1.1.0
+  let(:ref) { "refs/tags/#{tag_name}" }
+  let(:tag_name) { 'v1.1.0' }
+
+  let(:tag) { project.repository.find_tag(tag_name) }
+  let(:commit) { tag.dereferenced_target }
+
+  let(:service) do
+    described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref)
+  end
+
+  describe 'System hooks' do
+    it 'Executes system hooks' do
+      push_data = service.execute
+
+      expect_next_instance_of(SystemHooksService) do |system_hooks_service|
+        expect(system_hooks_service)
+          .to receive(:execute_hooks)
+          .with(push_data, :tag_push_hooks)
+      end
+
+      service.execute
+    end
+  end
+
+  describe "Webhooks" do
+    it "executes hooks on the project" do
+      expect(project).to receive(:execute_hooks)
+
+      service.execute
+    end
+  end
+
+  describe "Pipelines" do
+    before do
+      stub_ci_pipeline_to_return_yaml_file
+      project.add_developer(user)
+    end
+
+    it "creates a new pipeline" do
+      expect { service.execute }.to change { Ci::Pipeline.count }
+
+      expect(Ci::Pipeline.last).to be_push
+    end
+  end
+
+  describe 'Push data' do
+    shared_examples_for 'tag push data expectations' do
+      subject(:push_data) { service.execute }
+      it 'has expected push data attributes' do
+        is_expected.to match a_hash_including(
+          object_kind: 'tag_push',
+          ref: ref,
+          before: oldrev,
+          after: newrev,
+          message: tag.message,
+          user_id: user.id,
+          user_name: user.name,
+          project_id: project.id
+        )
+      end
+
+      context "with repository data" do
+        subject { push_data[:repository] }
+
+        it 'has expected repository attributes' do
+          is_expected.to match a_hash_including(
+            name: project.name,
+            url: project.url_to_repo,
+            description: project.description,
+            homepage: project.web_url
+          )
+        end
+      end
+
+      context "with commits" do
+        subject { push_data[:commits] }
+
+        it { is_expected.to be_an(Array) }
+
+        it 'has 1 element' do
+          expect(subject.size).to eq(1)
+        end
+
+        context "the commit" do
+          subject { push_data[:commits].first }
+
+          it { is_expected.to include(timestamp: commit.date.xmlschema) }
+
+          it 'has expected commit attributes' do
+            is_expected.to match a_hash_including(
+              id: commit.id,
+              message: commit.safe_message,
+              url: [
+               Gitlab.config.gitlab.url,
+               project.namespace.to_param,
+               project.to_param,
+               'commit',
+               commit.id
+              ].join('/')
+            )
+          end
+
+          context "with an author" do
+            subject { push_data[:commits].first[:author] }
+
+            it 'has expected author attributes' do
+              is_expected.to match a_hash_including(
+                name: commit.author_name,
+                email: commit.author_email
+              )
+            end
+          end
+        end
+      end
+    end
+
+    context 'annotated tag' do
+      include_examples 'tag push data expectations'
+    end
+
+    context 'lightweight tag' do
+      let(:tag_name) { 'light-tag' }
+      let(:newrev) { '5937ac0a7beb003549fc5fd26fc247adbce4a52e' }
+
+      before do
+        # Create the lightweight tag
+        rugged_repo(project.repository).tags.create(tag_name, newrev)
+
+        # Clear tag list cache
+        project.repository.expire_tags_cache
+      end
+
+      include_examples 'tag push data expectations'
+    end
+  end
+end

spec/services/git/tag_push_service_spec.rb

@@ -31,178 +31,27 @@ describe Git::TagPushService do
     end
   end
 
-  describe 'System Hooks' do
-    let!(:push_data) { service.tap(&:execute).push_data }
-
-    it "executes system hooks after pushing a tag" do
-      expect_next_instance_of(SystemHooksService) do |system_hooks_service|
-        expect(system_hooks_service)
-          .to receive(:execute_hooks)
-          .with(push_data, :tag_push_hooks)
-      end
-
-      service.execute
-    end
-  end
-
-  describe "Pipelines" do
-    subject { service.execute }
-
-    before do
-      stub_ci_pipeline_to_return_yaml_file
-      project.add_developer(user)
-    end
-
-    it "creates a new pipeline" do
-      expect { subject }.to change { Ci::Pipeline.count }
-      expect(Ci::Pipeline.last).to be_push
-    end
-  end
-
-  describe "Git Tag Push Data" do
-    subject { @push_data }
-    let(:tag) { project.repository.find_tag(tag_name) }
-    let(:commit) { tag.dereferenced_target }
-
-    context 'annotated tag' do
-      let(:tag_name) { Gitlab::Git.ref_name(ref) }
-
-      before do
-        service.execute
-        @push_data = service.push_data
-      end
-
-      it { is_expected.to include(object_kind: 'tag_push') }
-      it { is_expected.to include(ref: ref) }
-      it { is_expected.to include(before: oldrev) }
-      it { is_expected.to include(after: newrev) }
-      it { is_expected.to include(message: tag.message) }
-      it { is_expected.to include(user_id: user.id) }
-      it { is_expected.to include(user_name: user.name) }
-      it { is_expected.to include(project_id: project.id) }
-
-      context "with repository data" do
-        subject { @push_data[:repository] }
-
-        it { is_expected.to include(name: project.name) }
-        it { is_expected.to include(url: project.url_to_repo) }
-        it { is_expected.to include(description: project.description) }
-        it { is_expected.to include(homepage: project.web_url) }
-      end
-
-      context "with commits" do
-        subject { @push_data[:commits] }
-
-        it { is_expected.to be_an(Array) }
-        it 'has 1 element' do
-          expect(subject.size).to eq(1)
-        end
-
-        context "the commit" do
-          subject { @push_data[:commits].first }
-
-          it { is_expected.to include(id: commit.id) }
-          it { is_expected.to include(message: commit.safe_message) }
-          it { is_expected.to include(timestamp: commit.date.xmlschema) }
-          it do
-            is_expected.to include(
-              url: [
-               Gitlab.config.gitlab.url,
-               project.namespace.to_param,
-               project.to_param,
-               'commit',
-               commit.id
-              ].join('/')
-            )
-          end
-
-          context "with a author" do
-            subject { @push_data[:commits].first[:author] }
-
-            it { is_expected.to include(name: commit.author_name) }
-            it { is_expected.to include(email: commit.author_email) }
-          end
+  describe 'Hooks' do
+    context 'run on a tag' do
+      it 'delegates to Git::TagHooksService' do
+        expect_next_instance_of(::Git::TagHooksService) do |hooks_service|
+          expect(hooks_service.project).to eq(service.project)
+          expect(hooks_service.current_user).to eq(service.current_user)
+          expect(hooks_service.params).to eq(service.params)
+
+          expect(hooks_service).to receive(:execute)
         end
-      end
-    end
-
-    context 'lightweight tag' do
-      let(:tag_name) { 'light-tag' }
-      let(:newrev) { '5937ac0a7beb003549fc5fd26fc247adbce4a52e' }
-      let(:ref) { "refs/tags/light-tag" }
-
-      before do
-        # Create the lightweight tag
-        rugged_repo(project.repository).tags.create(tag_name, newrev)
-
-        # Clear tag list cache
-        project.repository.expire_tags_cache
 
         service.execute
-        @push_data = service.push_data
-      end
-
-      it { is_expected.to include(object_kind: 'tag_push') }
-      it { is_expected.to include(ref: ref) }
-      it { is_expected.to include(before: oldrev) }
-      it { is_expected.to include(after: newrev) }
-      it { is_expected.to include(message: tag.message) }
-      it { is_expected.to include(user_id: user.id) }
-      it { is_expected.to include(user_name: user.name) }
-      it { is_expected.to include(project_id: project.id) }
-
-      context "with repository data" do
-        subject { @push_data[:repository] }
-
-        it { is_expected.to include(name: project.name) }
-        it { is_expected.to include(url: project.url_to_repo) }
-        it { is_expected.to include(description: project.description) }
-        it { is_expected.to include(homepage: project.web_url) }
-      end
-
-      context "with commits" do
-        subject { @push_data[:commits] }
-
-        it { is_expected.to be_an(Array) }
-        it 'has 1 element' do
-          expect(subject.size).to eq(1)
-        end
-
-        context "the commit" do
-          subject { @push_data[:commits].first }
-
-          it { is_expected.to include(id: commit.id) }
-          it { is_expected.to include(message: commit.safe_message) }
-          it { is_expected.to include(timestamp: commit.date.xmlschema) }
-          it do
-            is_expected.to include(
-              url: [
-               Gitlab.config.gitlab.url,
-               project.namespace.to_param,
-               project.to_param,
-               'commit',
-               commit.id
-              ].join('/')
-            )
-          end
-
-          context "with a author" do
-            subject { @push_data[:commits].first[:author] }
-
-            it { is_expected.to include(name: commit.author_name) }
-            it { is_expected.to include(email: commit.author_email) }
-          end
-        end
       end
     end
-  end
 
-  describe "Webhooks" do
-    context "execute webhooks" do
-      let(:service) { described_class.new(project, user, oldrev: 'oldrev', newrev: 'newrev', ref: 'refs/tags/v1.0.0') }
+    context 'run on a branch' do
+      let(:ref) { 'refs/heads/master' }
+
+      it 'does nothing' do
+        expect(::Git::BranchHooksService).not_to receive(:new)
 
-      it "when pushing tags" do
-        expect(project).to receive(:execute_hooks)
         service.execute
       end
     end

spec/workers/post_receive_spec.rb

@@ -63,8 +63,12 @@ describe PostReceive do
         let(:changes) { "123456 789012 refs/heads/tést" }
 
         it "calls Git::BranchPushService" do
-          expect_any_instance_of(Git::BranchPushService).to receive(:execute).and_return(true)
-          expect_any_instance_of(Git::TagPushService).not_to receive(:execute)
+          expect_next_instance_of(Git::BranchPushService) do |service|
+            expect(service).to receive(:execute).and_return(true)
+          end
+
+          expect(Git::TagPushService).not_to receive(:new)
+
           described_class.new.perform(gl_repository, key_id, base64_changes)
         end
       end
@@ -73,8 +77,12 @@ describe PostReceive do
         let(:changes) { "123456 789012 refs/tags/tag" }
 
         it "calls Git::TagPushService" do
-          expect_any_instance_of(Git::BranchPushService).not_to receive(:execute)
-          expect_any_instance_of(Git::TagPushService).to receive(:execute).and_return(true)
+          expect(Git::BranchPushService).not_to receive(:execute)
+
+          expect_next_instance_of(Git::TagPushService) do |service|
+            expect(service).to receive(:execute).and_return(true)
+          end
+
           described_class.new.perform(gl_repository, key_id, base64_changes)
         end
       end
@@ -83,8 +91,9 @@ describe PostReceive do
         let(:changes) { "123456 789012 refs/merge-requests/123" }
 
         it "does not call any of the services" do
-          expect_any_instance_of(Git::BranchPushService).not_to receive(:execute)
-          expect_any_instance_of(Git::TagPushService).not_to receive(:execute)
+          expect(Git::BranchPushService).not_to receive(:new)
+          expect(Git::TagPushService).not_to receive(:new)
+
           described_class.new.perform(gl_repository, key_id, base64_changes)
         end
       end
@@ -127,7 +136,9 @@ describe PostReceive do
           allow_any_instance_of(Gitlab::DataBuilder::Repository).to receive(:update).and_return(fake_hook_data)
           # silence hooks so we can isolate
           allow_any_instance_of(Key).to receive(:post_create_hook).and_return(true)
-          allow_any_instance_of(Git::BranchPushService).to receive(:execute).and_return(true)
+          expect_next_instance_of(Git::BranchPushService) do |service|
+            expect(service).to receive(:execute).and_return(true)
+          end
         end
 
         it 'calls SystemHooksService' do