Refactor PostReceive to be more readable

This refactors the implementation of gathering/iterating changes
from the post-receive hook to be a bit more readable (removed
the need for calling `enum_for(:changes_refs)` in different
places).

This is done with the help of the new `Gitlab::Git::Changes`
collection object which will be used while parsing the changes.

Also changed the signature of BranchPushService and TagPushService
to accept a single `change` param instead of having oldrev, newrev
and ref params.

This also prepares the `PostReceive` worker to be able to process
branches and tags separately so we'll be able to aggregate events
by branches or tags.

app/services/concerns/git/change_params.rb

+# frozen_string_literal: true
+
+module Git
+  module ChangeParams
+    private
+
+    %i[oldrev newrev ref].each do |method|
+      define_method method do
+        change[method]
+      end
+    end
+
+    def change
+      @change ||= params.fetch(:change, {})
+    end
+  end
+end

app/services/git/base_hooks_service.rb

@@ -3,6 +3,7 @@
 module Git
   class BaseHooksService < ::BaseService
     include Gitlab::Utils::StrongMemoize
+    include ChangeParams
     # The N most recent commits to process in a single push payload.
     PROCESS_COMMIT_LIMIT = 100
@@ -77,20 +78,20 @@ def enqueue_invalidate_cache
     def pipeline_params
       {
-        before: params[:oldrev],
-        after: params[:newrev],
-        ref: params[:ref],
+        before: oldrev,
+        after: newrev,
+        ref: ref,
         push_options: params[:push_options] || {},
         checkout_sha: Gitlab::DataBuilder::Push.checkout_sha(
-          project.repository, params[:newrev], params[:ref])
+          project.repository, newrev, ref)
       }
     end
     def push_data_params(commits:, with_changed_files: true)
       {
-        oldrev: params[:oldrev],
-        newrev: params[:newrev],
-        ref: params[:ref],
+        oldrev: oldrev,
+        newrev: newrev,
+        ref: ref,
         project: project,
         user: current_user,
         commits: commits,

app/services/git/branch_hooks_service.rb

@@ -20,15 +20,15 @@ def commits
       strong_memoize(:commits) do
         if creating_default_branch?
           # The most recent PROCESS_COMMIT_LIMIT commits in the default branch
-          project.repository.commits(params[:newrev], limit: PROCESS_COMMIT_LIMIT)
+          project.repository.commits(newrev, 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])
+          project.repository.commits_between(project.default_branch, newrev)
         elsif updating_branch?
-          project.repository.commits_between(params[:oldrev], params[:newrev])
+          project.repository.commits_between(oldrev, newrev)
         else # removing branch
           []
         end
@@ -70,7 +70,7 @@ def branch_create_hooks
     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?
+      project.repository.copy_gitattributes(ref) if default_branch?
     end
     def branch_change_hooks
@@ -118,7 +118,7 @@ def enqueue_update_gpg_signatures
     # https://gitlab.com/gitlab-org/gitlab-foss/issues/59257
     def creating_branch?
       strong_memoize(:creating_branch) do
-        Gitlab::Git.blank_ref?(params[:oldrev]) ||
+        Gitlab::Git.blank_ref?(oldrev) ||
           !project.repository.branch_exists?(branch_name)
       end
     end
@@ -128,7 +128,7 @@ def updating_branch?
     end
     def removing_branch?
-      Gitlab::Git.blank_ref?(params[:newrev])
+      Gitlab::Git.blank_ref?(newrev)
     end
     def creating_default_branch?
@@ -137,7 +137,7 @@ def creating_default_branch?
     def count_commits_in_branch
       strong_memoize(:count_commits_in_branch) do
-        project.repository.commit_count_for_ref(params[:ref])
+        project.repository.commit_count_for_ref(ref)
       end
     end
@@ -148,7 +148,7 @@ def default_branch?
     end
     def branch_name
-      strong_memoize(:branch_name) { Gitlab::Git.ref_name(params[:ref]) }
+      strong_memoize(:branch_name) { Gitlab::Git.ref_name(ref) }
     end
     def upstream_commit_ids(commits)

app/services/git/branch_push_service.rb

@@ -4,6 +4,7 @@ module Git
   class BranchPushService < ::BaseService
     include Gitlab::Access
     include Gitlab::Utils::StrongMemoize
+    include ChangeParams
     # This method will be called after each git update
     # and only if the provided user and project are present in GitLab.
@@ -19,7 +20,7 @@ class BranchPushService < ::BaseService
     #  6. Checks if the project's main language has changed
     #
     def execute
-      return unless Gitlab::Git.branch_ref?(params[:ref])
+      return unless Gitlab::Git.branch_ref?(ref)
       enqueue_update_mrs
       enqueue_detect_repository_languages
@@ -38,9 +39,9 @@ def enqueue_update_mrs
       UpdateMergeRequestsWorker.perform_async(
         project.id,
         current_user.id,
-        params[:oldrev],
-        params[:newrev],
-        params[:ref]
+        oldrev,
+        newrev,
+        ref
       )
     end
@@ -69,11 +70,11 @@ def perform_housekeeping
     end
     def removing_branch?
-      Gitlab::Git.blank_ref?(params[:newrev])
+      Gitlab::Git.blank_ref?(newrev)
     end
     def branch_name
-      strong_memoize(:branch_name) { Gitlab::Git.ref_name(params[:ref]) }
+      strong_memoize(:branch_name) { Gitlab::Git.ref_name(ref) }
     end
     def default_branch?

app/services/git/tag_hooks_service.rb

@@ -18,12 +18,12 @@ def event_message
     def tag
       strong_memoize(:tag) do
-        next if Gitlab::Git.blank_ref?(params[:newrev])
-        tag_name = Gitlab::Git.ref_name(params[:ref])
+        next if Gitlab::Git.blank_ref?(newrev)
+        tag_name = Gitlab::Git.ref_name(ref)
         tag = project.repository.find_tag(tag_name)
-        tag if tag && tag.target == params[:newrev]
+        tag if tag && tag.target == newrev
       end
     end

app/services/git/tag_push_service.rb

@@ -2,8 +2,10 @@
 module Git
   class TagPushService < ::BaseService
+    include ChangeParams
+
     def execute
-      return unless Gitlab::Git.tag_ref?(params[:ref])
+      return unless Gitlab::Git.tag_ref?(ref)
       project.repository.before_push_tag
       TagHooksService.new(project, current_user, params).execute

app/workers/post_receive.rb

@@ -37,41 +37,22 @@ def identify_user(post_received)
   end
   def process_project_changes(post_received)
-    changes = []
-    refs = Set.new
     user = identify_user(post_received)
+
     return false unless user
+    project = post_received.project
+    push_options = post_received.push_options
+    changes = post_received.changes
+
     # We only need to expire certain caches once per push
     expire_caches(post_received, post_received.project.repository)
     enqueue_repository_cache_update(post_received)
-    post_received.enum_for(:changes_refs).with_index do |(oldrev, newrev, ref), index|
-      service_klass =
-        if Gitlab::Git.tag_ref?(ref)
-          Git::TagPushService
-        elsif Gitlab::Git.branch_ref?(ref)
-          Git::BranchPushService
-        end
-
-      if service_klass
-        service_klass.new(
-          post_received.project,
-          user,
-          oldrev: oldrev,
-          newrev: newrev,
-          ref: ref,
-          push_options: post_received.push_options,
-          create_pipelines: index < PIPELINE_PROCESS_LIMIT || Feature.enabled?(:git_push_create_all_pipelines, post_received.project)
-        ).execute
-      end
-
-      changes << Gitlab::DataBuilder::Repository.single_change(oldrev, newrev, ref)
-      refs << ref
-    end
-
+    process_changes(Git::BranchPushService, project, user, push_options, changes.branch_changes)
+    process_changes(Git::TagPushService, project, user, push_options, changes.tag_changes)
     update_remote_mirrors(post_received)
-    after_project_changes_hooks(post_received, user, refs.to_a, changes)
+    after_project_changes_hooks(project, user, changes.refs, changes.repository_data)
   end
   # Expire the repository status, branch, and tag cache once per push.
@@ -94,6 +75,20 @@ def enqueue_repository_cache_update(post_received)
     )
   end
+  def process_changes(service_class, project, user, push_options, changes)
+    return if changes.empty?
+
+    changes.each do |change|
+      service_class.new(
+        project,
+        user,
+        change: change,
+        push_options: push_options,
+        create_pipelines: change[:index] < PIPELINE_PROCESS_LIMIT || Feature.enabled?(:git_push_create_all_pipelines, project)
+      ).execute
+    end
+  end
+
   def update_remote_mirrors(post_received)
     return unless post_received.includes_branches? || post_received.includes_tags?
@@ -104,9 +99,9 @@ def update_remote_mirrors(post_received)
     project.update_remote_mirrors
   end
-  def after_project_changes_hooks(post_received, user, refs, changes)
-    hook_data = Gitlab::DataBuilder::Repository.update(post_received.project, user, changes, refs)
-    SystemHooksService.new.execute_hooks(hook_data, :repository_update_hooks)
+  def after_project_changes_hooks(project, user, refs, changes)
+    repository_update_hook_data = Gitlab::DataBuilder::Repository.update(project, user, changes, refs)
+    SystemHooksService.new.execute_hooks(repository_update_hook_data, :repository_update_hooks)
     Gitlab::UsageDataCounters::SourceCodeCounter.count(:pushes)
   end
@@ -121,7 +116,7 @@ def process_wiki_changes(post_received)
     # We only need to expire certain caches once per push
     expire_caches(post_received, post_received.project.wiki.repository)
-    ::Git::WikiPushService.new(post_received.project, user, changes: post_received.enum_for(:changes_refs)).execute
+    ::Git::WikiPushService.new(post_received.project, user, changes: post_received.changes).execute
   end
   def log(message)

db/fixtures/development/17_cycle_analytics.rb

@@ -150,9 +150,11 @@ def mention_in_commits(issues)
       ::Git::BranchPushService.new(
         issue.project,
         @user,
-        oldrev: issue.project.repository.commit("master").sha,
-        newrev: commit_sha,
-        ref: 'refs/heads/master'
+        change: {
+          oldrev: issue.project.repository.commit("master").sha,
+          newrev: commit_sha,
+          ref: 'refs/heads/master'
+        }
       ).execute
       branch_name

ee/app/services/ee/git/branch_push_service.rb

@@ -18,7 +18,7 @@ def execute
       def enqueue_elasticsearch_indexing
         return unless should_index_commits?
-        project.repository.index_commits_and_blobs(from_rev: params[:oldrev], to_rev: params[:newrev])
+        project.repository.index_commits_and_blobs(from_rev: oldrev, to_rev: newrev)
       end
       def enqueue_update_external_pull_requests
@@ -28,7 +28,7 @@ def enqueue_update_external_pull_requests
         UpdateExternalPullRequestsWorker.perform_async(
           project.id,
           current_user.id,
-          params[:ref]
+          ref
         )
       end

ee/app/services/ee/git/wiki_push_service.rb

@@ -9,11 +9,11 @@ def execute
         return unless project.use_elasticsearch?
         # Check if one of the changes we got was for the default branch. If it was, trigger an ES update
-        params[:changes].each do |_oldrev, newrev, ref|
-          branch_name = ::Gitlab::Git.ref_name(ref)
+        params[:changes].each do |change|
+          branch_name = ::Gitlab::Git.ref_name(change[:ref])
           next unless project.wiki.default_branch == branch_name
-          project.wiki.index_wiki_blobs(newrev)
+          project.wiki.index_wiki_blobs(change[:newrev])
         end
       end
     end

ee/app/services/projects/update_mirror_service.rb

@@ -83,12 +83,12 @@ def update_tags(&block)
         Git::TagPushService.new(
           project,
           current_user,
-          {
+          change: {
             oldrev: old_tag_target,
             newrev: tag_target,
-            ref: "#{Gitlab::Git::TAG_REF_PREFIX}#{tag.name}",
-            mirror_update: true
-          }
+            ref: "#{Gitlab::Git::TAG_REF_PREFIX}#{tag.name}"
+          },
+          mirror_update: true
         ).execute
       end

ee/app/workers/ee/post_receive.rb

@@ -10,11 +10,9 @@ module PostReceive
     private
-    def after_project_changes_hooks(post_received, user, refs, changes)
+    def after_project_changes_hooks(project, user, refs, changes)
       super
-      project = post_received.project
-
       if audit_push?(project)
         ::RepositoryPushAuditEventWorker.perform_async(changes, project.id, user.id)
       end

ee/db/fixtures/development/90_productivity_analytics.rb

@@ -87,9 +87,11 @@ def mention_in_commits(issues)
         ::Git::BranchPushService.new(
           issue.project,
           @user,
-          oldrev: issue.project.repository.commit("master").sha,
-          newrev: commit_sha,
-          ref: 'refs/heads/master'
+          change: {
+            oldrev: issue.project.repository.commit("master").sha,
+            newrev: commit_sha,
+            ref: 'refs/heads/master'
+          }
         ).execute
       end

ee/spec/services/ee/git/branch_push_service_spec.rb

@@ -12,7 +12,7 @@
   let(:ref)      { 'refs/heads/master' }
   let(:params) do
-    { oldrev: oldrev, newrev: newrev, ref: ref }
+    { change: { oldrev: oldrev, newrev: newrev, ref: ref } }
   end
   subject do
@@ -60,16 +60,20 @@
         subject.execute
       end
-      it "does not trigger indexer when push to non-default branch" do
-        expect_any_instance_of(Gitlab::Elastic::Indexer).not_to receive(:run)
-        execute_service(project, user, oldrev, newrev, 'refs/heads/other')
+      it "triggers indexer when push to default branch" do
+        expect_any_instance_of(Gitlab::Elastic::Indexer).to receive(:run)
+        subject.execute
       end
-      it "triggers indexer when push to default branch" do
-        expect_any_instance_of(Gitlab::Elastic::Indexer).to receive(:run)
-        execute_service(project, user, oldrev, newrev, ref)
+      context 'when push to non-default branch' do
+        let(:ref) { 'refs/heads/other' }
+        it 'does not trigger indexer when push to non-default branch' do
+          expect_any_instance_of(Gitlab::Elastic::Indexer).not_to receive(:run)
+
+          subject.execute
+        end
       end
       context 'when limited indexing is on' do
@@ -231,10 +235,4 @@
       it_behaves_like 'does not enqueue Jira sync worker'
     end
   end
-
-  def execute_service(project, user, oldrev, newrev, ref)
-    service = described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref)
-    service.execute
-    service
-  end
 end

ee/spec/services/ee/git/wiki_push_service_spec.rb

@@ -26,7 +26,7 @@
       it 'triggers a wiki update' do
         expect(project.wiki).to receive(:index_wiki_blobs).with("797823")
-        described_class.new(project, project.owner, changes: post_received.enum_for(:changes_refs)).execute
+        described_class.new(project, project.owner, changes: post_received.changes).execute
       end
     end
@@ -36,7 +36,7 @@
       it 'does not trigger a wiki update' do
         expect(project.wiki).not_to receive(:index_wiki_blobs)
-        described_class.new(project, project.owner, changes: post_received.enum_for(:changes_refs)).execute
+        described_class.new(project, project.owner, changes: post_received.changes).execute
       end
     end
   end
@@ -52,7 +52,7 @@
       it 'does nothing even if changes include master ref' do
         expect(project.wiki).not_to receive(:index_wiki_blobs)
-        described_class.new(project, project.owner, changes: post_received.enum_for(:changes_refs)).execute
+        described_class.new(project, project.owner, changes: post_received.changes).execute
       end
     end
   end

ee/spec/services/projects/update_mirror_service_spec.rb

@@ -65,7 +65,7 @@
         stub_fetch_mirror(project)
         expect(Git::TagPushService).to receive(:new)
-          .with(project, project.owner, hash_including(ref: 'refs/tags/new-tag'))
+          .with(project, project.owner, change: hash_including(ref: 'refs/tags/new-tag'), mirror_update: true)
           .and_return(double(execute: true))
         service.execute

lib/gitlab/git/changes.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Git
+    class Changes
+      include Enumerable
+
+      attr_reader :repository_data
+
+      def initialize
+        @refs = Set.new
+        @items = []
+        @branches_index = []
+        @tags_index = []
+        @repository_data = []
+      end
+
+      def includes_branches?
+        branches_index.any?
+      end
+
+      def includes_tags?
+        tags_index.any?
+      end
+
+      def add_branch_change(change)
+        @branches_index << add_change(change)
+        self
+      end
+
+      def add_tag_change(change)
+        @tags_index << add_change(change)
+        self
+      end
+
+      def each
+        items.each do |item|
+          yield item
+        end
+      end
+
+      def refs
+        @refs.to_a
+      end
+
+      def branch_changes
+        items.values_at(*branches_index)
+      end
+
+      def tag_changes
+        items.values_at(*tags_index)
+      end
+
+      private
+
+      attr_reader :items, :branches_index, :tags_index
+
+      def add_change(change)
+        # refs and repository_data are being cached when a change is added to
+        # the collection to remove the need to iterate through changes multiple
+        # times.
+        @refs << change[:ref]
+        @repository_data << build_change_repository_data(change)
+        @items << change
+
+        @items.size - 1
+      end
+
+      def build_change_repository_data(change)
+        DataBuilder::Repository.single_change(change[:oldrev], change[:newrev], change[:ref])
+      end
+    end
+  end
+end

lib/gitlab/git_post_receive.rb

@@ -8,7 +8,7 @@ class GitPostReceive
     def initialize(project, identifier, changes, push_options = {})
       @project = project
       @identifier = identifier
-      @changes = deserialize_changes(changes)
+      @changes = parse_changes(changes)
       @push_options = push_options
     end
@@ -16,27 +16,12 @@ def identify
       super(identifier)
     end
-    def changes_refs
-      return changes unless block_given?
-
-      changes.each do |change|
-        change.strip!
-        oldrev, newrev, ref = change.split(' ')
-
-        yield oldrev, newrev, ref
-      end
-    end
-
     def includes_branches?
-      enum_for(:changes_refs).any? do |_oldrev, _newrev, ref|
-        Gitlab::Git.branch_ref?(ref)
-      end
+      changes.includes_branches?
     end
     def includes_tags?
-      enum_for(:changes_refs).any? do |_oldrev, _newrev, ref|
-        Gitlab::Git.tag_ref?(ref)
-      end
+      changes.includes_tags?
     end
     def includes_default_branch?
@@ -44,16 +29,28 @@ def includes_default_branch?
       # first branch pushed will be the default.
       return true unless project.default_branch.present?
-      enum_for(:changes_refs).any? do |_oldrev, _newrev, ref|
-        Gitlab::Git.branch_ref?(ref) &&
-          Gitlab::Git.branch_name(ref) == project.default_branch
+      changes.branch_changes.any? do |change|
+        Gitlab::Git.branch_name(change[:ref]) == project.default_branch
       end
     end
     private
-    def deserialize_changes(changes)
-      utf8_encode_changes(changes).each_line
+    def parse_changes(changes)
+      deserialized_changes = utf8_encode_changes(changes).each_line
+
+      Git::Changes.new.tap do |collection|
+        deserialized_changes.each_with_index do |raw_change, index|
+          oldrev, newrev, ref = raw_change.strip.split(' ')
+          change = { index: index, oldrev: oldrev, newrev: newrev, ref: ref }
+
+          if Git.branch_ref?(ref)
+            collection.add_branch_change(change)
+          elsif Git.tag_ref?(ref)
+            collection.add_tag_change(change)
+          end
+        end
+      end
     end
     def utf8_encode_changes(changes)

spec/features/projects/environments/environment_spec.rb

@@ -304,9 +304,11 @@
     #
     def remove_branch_with_hooks(project, user, branch)
       params = {
-        oldrev: project.commit(branch).id,
-        newrev: Gitlab::Git::BLANK_SHA,
-        ref: "refs/heads/#{branch}"
+        change: {
+          oldrev: project.commit(branch).id,
+          newrev: Gitlab::Git::BLANK_SHA,
+          ref: "refs/heads/#{branch}"
+        }
       }
       yield

spec/lib/gitlab/git/changes_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Git::Changes do
+  let(:changes) { described_class.new }
+
+  describe '#includes_branches?' do
+    subject { changes.includes_branches? }
+
+    context 'has changes for branches' do
+      before do
+        changes.add_branch_change(oldrev: 'abc123', newrev: 'def456', ref: 'branch')
+      end
+
+      it { is_expected.to be_truthy }
+    end
+
+    context 'has no changes for branches' do
+      before do
+        changes.add_tag_change(oldrev: 'abc123', newrev: 'def456', ref: 'tag')
+      end
+
+      it { is_expected.to be_falsey }
+    end
+  end
+
+  describe '#includes_tags?' do
+    subject { changes.includes_tags? }
+
+    context 'has changes for tags' do
+      before do
+        changes.add_tag_change(oldrev: 'abc123', newrev: 'def456', ref: 'tag')
+      end
+
+      it { is_expected.to be_truthy }
+    end
+
+    context 'has no changes for tags' do
+      before do
+        changes.add_branch_change(oldrev: 'abc123', newrev: 'def456', ref: 'branch')
+      end
+
+      it { is_expected.to be_falsey }
+    end
+  end
+
+  describe '#add_branch_change' do
+    let(:change) { { oldrev: 'abc123', newrev: 'def456', ref: 'branch' } }
+
+    subject { changes.add_branch_change(change) }
+
+    it 'adds the branch change to the collection' do
+      expect(subject).to include(change)
+      expect(subject.refs).to include(change[:ref])
+      expect(subject.repository_data).to include(before: change[:oldrev], after: change[:newrev], ref: change[:ref])
+      expect(subject.branch_changes).to include(change)
+    end
+
+    it 'does not add the change as a tag change' do
+      expect(subject.tag_changes).not_to include(change)
+    end
+  end
+
+  describe '#add_tag_change' do
+    let(:change) { { oldrev: 'abc123', newrev: 'def456', ref: 'tag' } }
+
+    subject { changes.add_tag_change(change) }
+
+    it 'adds the tag change to the collection' do
+      expect(subject).to include(change)
+      expect(subject.refs).to include(change[:ref])
+      expect(subject.repository_data).to include(before: change[:oldrev], after: change[:newrev], ref: change[:ref])
+      expect(subject.tag_changes).to include(change)
+    end
+
+    it 'does not add the change as a branch change' do
+      expect(subject.branch_changes).not_to include(change)
+    end
+  end
+end