Revert "Merge branch 'sh-optimize-commit-deltas-post-receive' into 'master'"revert-3cd40c4a

This reverts merge request !31741

11 files changed, 18 insertions, 205 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 8efe4b06f87..5b5ffa69a83 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1238,14 +1238,6 @@ class Project < ApplicationRecord
     end
   end
 
-  def has_active_hooks?(hooks_scope = :push_hooks)
-    hooks.hooks_for(hooks_scope).any? || SystemHook.hooks_for(hooks_scope).any?
-  end
-
-  def has_active_services?(hooks_scope = :push_hooks)
-    services.public_send(hooks_scope).any? # rubocop:disable GitlabSecurity/PublicSend
-  end
-
   def valid_repo?
     repository.exists?
   rescue
diff --git a/app/services/git/base_hooks_service.rb b/app/services/git/base_hooks_service.rb
index 3fd38444196..dbc1f24bd49 100644
--- a/app/services/git/base_hooks_service.rb
+++ b/app/services/git/base_hooks_service.rb
@@ -17,7 +17,7 @@ module Git
 
       update_remote_mirrors
 
-      success
+      push_data
     end
 
     private
@@ -31,7 +31,7 @@ module Git
     end
 
     def limited_commits
-      @limited_commits ||= commits.last(PROCESS_COMMIT_LIMIT)
+      commits.last(PROCESS_COMMIT_LIMIT)
     end
 
     def commits_count
@@ -46,25 +46,21 @@ module Git
       []
     end
 
-    # Push events in the activity feed only show information for the
-    # last commit.
     def create_events
-      EventCreateService.new.push(project, current_user, event_push_data)
+      EventCreateService.new.push(project, current_user, push_data)
     end
 
     def create_pipelines
       return unless params.fetch(:create_pipelines, true)
 
       Ci::CreatePipelineService
-        .new(project, current_user, base_params)
+        .new(project, current_user, push_data)
         .execute(:push, pipeline_options)
     end
 
     def execute_project_hooks
-      # Creating push_data invokes one CommitDelta RPC per commit. Only
-      # build this data if we actually need it.
-      project.execute_hooks(push_data, hook_name) if project.has_active_hooks?(hook_name)
-      project.execute_services(push_data, hook_name) if project.has_active_services?(hook_name)
+      project.execute_hooks(push_data, hook_name)
+      project.execute_services(push_data, hook_name)
     end
 
     def enqueue_invalidate_cache
@@ -75,35 +71,18 @@ module Git
       ProjectCacheWorker.perform_async(project.id, file_types, [], false)
     end
 
-    def base_params
-      {
+    def push_data
+      @push_data ||= Gitlab::DataBuilder::Push.build(
+        project: project,
+        user: current_user,
         oldrev: params[:oldrev],
         newrev: params[:newrev],
         ref: params[:ref],
-        push_options: params[:push_options] || {}
-      }
-    end
-
-    def push_data_params(commits:, with_changed_files: true)
-      base_params.merge(
-        project: project,
-        user: current_user,
-        commits: commits,
+        commits: limited_commits,
         message: event_message,
         commits_count: commits_count,
-        with_changed_files: with_changed_files
+        push_options: params[:push_options] || {}
       )
-    end
-
-    def event_push_data
-      # We only need the last commit for the event push, and we don't
-      # need the full deltas either.
-      @event_push_data ||= Gitlab::DataBuilder::Push.build(
-        push_data_params(commits: commits.last, with_changed_files: false))
-    end
-
-    def push_data
-      @push_data ||= Gitlab::DataBuilder::Push.build(push_data_params(commits: limited_commits))
 
       # Dependent code may modify the push data, so return a duplicate each time
       @push_data.dup
diff --git a/changelogs/unreleased/sh-optimize-commit-deltas-post-receive.yml b/changelogs/unreleased/sh-optimize-commit-deltas-post-receive.yml
deleted file mode 100644
index cd63b9bf425..00000000000
--- a/changelogs/unreleased/sh-optimize-commit-deltas-post-receive.yml
+++ /dev/null
@@ -1,5 +0,0 @@
----
-title: Reduce Gitaly calls in PostReceive
-merge_request: 31741
-author:
-type: performance
diff --git a/lib/gitlab/data_builder/push.rb b/lib/gitlab/data_builder/push.rb
index 37fadb47736..40bda3410e1 100644
--- a/lib/gitlab/data_builder/push.rb
+++ b/lib/gitlab/data_builder/push.rb
@@ -60,8 +60,7 @@ module Gitlab
       # rubocop:disable Metrics/ParameterLists
       def build(
           project:, user:, ref:, oldrev: nil, newrev: nil,
-          commits: [], commits_count: nil, message: nil, push_options: {},
-          with_changed_files: true)
+          commits: [], commits_count: nil, message: nil, push_options: {})
 
         commits = Array(commits)
 
@@ -76,7 +75,7 @@ module Gitlab
         # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/38259
         commit_attrs = Gitlab::GitalyClient.allow_n_plus_1_calls do
           commits_limited.map do |commit|
-            commit.hook_attrs(with_changed_files: with_changed_files)
+            commit.hook_attrs(with_changed_files: true)
           end
         end
 
diff --git a/spec/lib/gitlab/data_builder/push_spec.rb b/spec/lib/gitlab/data_builder/push_spec.rb
index e8a9f0b06a8..cc31f88d365 100644
--- a/spec/lib/gitlab/data_builder/push_spec.rb
+++ b/spec/lib/gitlab/data_builder/push_spec.rb
@@ -3,43 +3,9 @@
 require 'spec_helper'
 
 describe Gitlab::DataBuilder::Push do
-  include RepoHelpers
-
   let(:project) { create(:project, :repository) }
   let(:user) { build(:user, public_email: 'public-email@example.com') }
 
-  describe '.build' do
-    let(:sample) { RepoHelpers.sample_compare }
-    let(:commits) { project.repository.commits_between(sample.commits.first, sample.commits.last) }
-    let(:subject) do
-      described_class.build(project: project,
-                            user: user,
-                            ref: sample.target_branch,
-                            commits: commits,
-                            commits_count: commits.length,
-                            message: 'test message',
-                            with_changed_files: with_changed_files)
-    end
-
-    context 'with changed files' do
-      let(:with_changed_files) { true }
-
-      it 'returns commit hook data' do
-        expect(subject[:project]).to eq(project.hook_attrs)
-        expect(subject[:commits].first.keys).to include(*%i(added removed modified))
-      end
-    end
-
-    context 'without changed files' do
-      let(:with_changed_files) { false }
-
-      it 'returns commit hook data without include deltas' do
-        expect(subject[:project]).to eq(project.hook_attrs)
-        expect(subject[:commits].first.keys).not_to include(*%i(added removed modified))
-      end
-    end
-  end
-
   describe '.build_sample' do
     let(:data) { described_class.build_sample(project, user) }
 
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index ff9e94afc12..38fe090b468 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -4364,39 +4364,6 @@ describe Project do
     end
   end
 
-  describe '#has_active_hooks?' do
-    set(:project) { create(:project) }
-
-    it { expect(project.has_active_hooks?).to be_falsey }
-
-    it 'returns true when a matching push hook exists' do
-      create(:project_hook, push_events: true, project: project)
-
-      expect(project.has_active_hooks?(:merge_request_events)).to be_falsey
-      expect(project.has_active_hooks?).to be_truthy
-    end
-
-    it 'returns true when a matching system hook exists' do
-      create(:system_hook, push_events: true)
-
-      expect(project.has_active_hooks?(:merge_request_events)).to be_falsey
-      expect(project.has_active_hooks?).to be_truthy
-    end
-  end
-
-  describe '#has_active_services?' do
-    set(:project) { create(:project) }
-
-    it { expect(project.has_active_services?).to be_falsey }
-
-    it 'returns true when a matching service exists' do
-      create(:custom_issue_tracker_service, push_events: true, merge_requests_events: false, project: project)
-
-      expect(project.has_active_services?(:merge_request_hooks)).to be_falsey
-      expect(project.has_active_services?).to be_truthy
-    end
-  end
-
   describe '#badges' do
     let(:project_group) { create(:group) }
     let(:project) { create(:project, path: 'avatar', namespace: project_group) }
diff --git a/spec/services/git/base_hooks_service_spec.rb b/spec/services/git/base_hooks_service_spec.rb
index 874df9a68cd..4a2ec769116 100644
--- a/spec/services/git/base_hooks_service_spec.rb
+++ b/spec/services/git/base_hooks_service_spec.rb
@@ -14,78 +14,6 @@ describe Git::BaseHooksService do
   let(:newrev) { "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b" } # gitlab-test: git rev-parse refs/tags/v1.1.0
   let(:ref) { 'refs/tags/v1.1.0' }
 
-  describe '#execute_project_hooks' do
-    class TestService < described_class
-      def hook_name
-        :push_hooks
-      end
-
-      def commits
-        []
-      end
-    end
-
-    let(:project) { create(:project, :repository) }
-
-    subject { TestService.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref) }
-
-    context '#execute_hooks' do
-      before do
-        expect(project).to receive(:has_active_hooks?).and_return(active)
-      end
-
-      context 'active hooks' do
-        let(:active) { true }
-
-        it 'executes the hooks' do
-          expect(subject).to receive(:push_data).at_least(:once).and_call_original
-          expect(project).to receive(:execute_hooks)
-
-          subject.execute
-        end
-      end
-
-      context 'inactive hooks' do
-        let(:active) { false }
-
-        it 'does not execute the hooks' do
-          expect(subject).not_to receive(:push_data)
-          expect(project).not_to receive(:execute_hooks)
-
-          subject.execute
-        end
-      end
-    end
-
-    context '#execute_services' do
-      before do
-        expect(project).to receive(:has_active_services?).and_return(active)
-      end
-
-      context 'active services' do
-        let(:active) { true }
-
-        it 'executes the services' do
-          expect(subject).to receive(:push_data).at_least(:once).and_call_original
-          expect(project).to receive(:execute_services)
-
-          subject.execute
-        end
-      end
-
-      context 'inactive services' do
-        let(:active) { false }
-
-        it 'does not execute the services' do
-          expect(subject).not_to receive(:push_data)
-          expect(project).not_to receive(:execute_services)
-
-          subject.execute
-        end
-      end
-    end
-  end
-
   describe 'with remote mirrors' do
     class TestService < described_class
       def commits
diff --git a/spec/services/git/branch_hooks_service_spec.rb b/spec/services/git/branch_hooks_service_spec.rb
index 2bf7dc32436..088693a517a 100644
--- a/spec/services/git/branch_hooks_service_spec.rb
+++ b/spec/services/git/branch_hooks_service_spec.rb
@@ -26,7 +26,7 @@ describe Git::BranchHooksService do
   end
 
   describe "Git Push Data" do
-    subject(:push_data) { service.send(:push_data) }
+    subject(:push_data) { service.execute }
 
     it 'has expected push data attributes' do
       is_expected.to match a_hash_including(
@@ -110,7 +110,6 @@ describe Git::BranchHooksService do
         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.commit_title).to eq('Change some files')
         expect(event.push_event_payload.ref).to eq('master')
         expect(event.push_event_payload.commit_count).to eq(1)
       end
@@ -126,7 +125,6 @@ describe Git::BranchHooksService do
         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.commit_title).to eq('Initial commit')
         expect(event.push_event_payload.ref).to eq('master')
         expect(event.push_event_payload.commit_count).to be > 1
       end
diff --git a/spec/services/git/branch_push_service_spec.rb b/spec/services/git/branch_push_service_spec.rb
index ad5d296f5c1..6e39fa6b3c0 100644
--- a/spec/services/git/branch_push_service_spec.rb
+++ b/spec/services/git/branch_push_service_spec.rb
@@ -78,10 +78,7 @@ describe Git::BranchPushService, services: true do
 
     it "creates a new pipeline" do
       expect { subject }.to change { Ci::Pipeline.count }
-
-      pipeline = Ci::Pipeline.last
-      expect(pipeline).to be_push
-      expect(Gitlab::Git::BRANCH_REF_PREFIX + pipeline.ref).to eq(ref)
+      expect(Ci::Pipeline.last).to be_push
     end
   end
 
@@ -126,10 +123,6 @@ describe Git::BranchPushService, services: true do
 
   describe "Webhooks" do
     context "execute webhooks" do
-      before do
-        create(:project_hook, push_events: true, project: project)
-      end
-
       it "when pushing a branch for the first time" do
         expect(project).to receive(:execute_hooks)
         expect(project.default_branch).to eq("master")
diff --git a/spec/services/git/tag_hooks_service_spec.rb b/spec/services/git/tag_hooks_service_spec.rb
index e362577d289..f5938a5c708 100644
--- a/spec/services/git/tag_hooks_service_spec.rb
+++ b/spec/services/git/tag_hooks_service_spec.rb
@@ -26,8 +26,7 @@ describe Git::TagHooksService, :service do
 
   describe 'System hooks' do
     it 'Executes system hooks' do
-      push_data = service.send(:push_data)
-      expect(project).to receive(:has_active_hooks?).and_return(true)
+      push_data = service.execute
 
       expect_next_instance_of(SystemHooksService) do |system_hooks_service|
         expect(system_hooks_service)
@@ -41,7 +40,6 @@ describe Git::TagHooksService, :service do
 
   describe "Webhooks" do
     it "executes hooks on the project" do
-      expect(project).to receive(:has_active_hooks?).and_return(true)
       expect(project).to receive(:execute_hooks)
 
       service.execute
@@ -63,7 +61,7 @@ describe Git::TagHooksService, :service do
 
   describe 'Push data' do
     shared_examples_for 'tag push data expectations' do
-      subject(:push_data) { service.send(:push_data) }
+      subject(:push_data) { service.execute }
       it 'has expected push data attributes' do
         is_expected.to match a_hash_including(
           object_kind: 'tag_push',
diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb
index c8a0c22b0e8..3e2c2190f02 100644
--- a/spec/workers/post_receive_spec.rb
+++ b/spec/workers/post_receive_spec.rb
@@ -334,8 +334,6 @@ describe PostReceive do
     end
 
     it "asks the project to trigger all hooks" do
-      create(:project_hook, push_events: true, tag_push_events: true, project: project)
-      create(:custom_issue_tracker_service, push_events: true, merge_requests_events: false, project: project)
       allow(Project).to receive(:find_by).and_return(project)
 
       expect(project).to receive(:execute_hooks).twice