Merge branch 'fix/gitpushservice-complexity-issue' into 'master'

Reduce code complexity on GitPushService#execute

Code complexity for gitlab-ce after this has been refactored:
```
  27.3: GitPushService#execute
```
This still needs to be merged into `gitlab-ee` presumably with conflicts... Perhaps we should create another issue for doing that?

I left the code sort of similar to what it was... If I could, I would have refactored most of the code into separate classes, etc. as it breaks probably all SOLID principles.

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/13327


See merge request !2784

3 files changed, 103 insertions, 90 deletions

diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index e3bf14966c8..a1711d234ff 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -1,10 +1,10 @@
-class GitPushService
-  attr_accessor :project, :user, :push_data, :push_commits
+class GitPushService < BaseService
+  attr_accessor :push_data, :push_commits
   include Gitlab::CurrentSettings
   include Gitlab::Access
 
   # This method will be called after each git update
-  # and only if the provided user and project is present in GitLab.
+  # and only if the provided user and project are present in GitLab.
   #
   # All callbacks for post receive action should be placed here.
   #
@@ -15,67 +15,67 @@ class GitPushService
   #  4. Executes the project's web hooks
   #  5. Executes the project's services
   #
-  def execute(project, user, oldrev, newrev, ref)
-    @project, @user = project, user
-
-    branch_name = Gitlab::Git.ref_name(ref)
-
-    project.repository.expire_cache(branch_name)
-
-    if push_remove_branch?(ref, newrev)
-      project.repository.expire_has_visible_content_cache
+  def execute
+    @project.repository.expire_cache(branch_name)
 
+    if push_remove_branch?
+      @project.repository.expire_has_visible_content_cache
       @push_commits = []
-    elsif push_to_new_branch?(ref, oldrev)
-      project.repository.expire_has_visible_content_cache
+    elsif push_to_new_branch?
+      @project.repository.expire_has_visible_content_cache
 
       # Re-find the pushed commits.
-      if is_default_branch?(ref)
+      if is_default_branch?
         # Initial push to the default branch. Take the full history of that branch as "newly pushed".
-        @push_commits = project.repository.commits(newrev)
-
-        # Ensure HEAD points to the default branch in case it is not master
-        project.change_head(branch_name)
-
-        # Set protection on the default branch if configured
-        if (current_application_settings.default_branch_protection != PROTECTION_NONE)
-          developers_can_push = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? true : false
-          project.protected_branches.create({ name: project.default_branch, developers_can_push: developers_can_push })
-        end
+        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, newrev)
+        @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(ref)
+        process_commit_messages
       end
-    elsif push_to_existing_branch?(ref, oldrev)
+    elsif push_to_existing_branch?
       # Collect data for this git push
-      @push_commits = project.repository.commits_between(oldrev, newrev)
-      process_commit_messages(ref)
+      @push_commits = @project.repository.commits_between(params[:oldrev], params[:newrev])
+      process_commit_messages
     end
-
     # Update merge requests that may be affected by this push. A new branch
     # could cause the last commit of a merge request to change.
-    project.update_merge_requests(oldrev, newrev, ref, @user)
+    update_merge_requests
+  end
 
-    @push_data = build_push_data(oldrev, newrev, ref)
+  protected
 
-    EventCreateService.new.push(project, user, @push_data)
-    project.execute_hooks(@push_data.dup, :push_hooks)
-    project.execute_services(@push_data.dup, :push_hooks)
-    CreateCommitBuildsService.new.execute(project, @user, @push_data)
-    ProjectCacheWorker.perform_async(project.id)
+  def update_merge_requests
+    @project.update_merge_requests(params[:oldrev], params[:newrev], params[:ref], current_user)
+
+    EventCreateService.new.push(@project, current_user, build_push_data)
+    @project.execute_hooks(build_push_data.dup, :push_hooks)
+    @project.execute_services(build_push_data.dup, :push_hooks)
+    CreateCommitBuildsService.new.execute(@project, current_user, build_push_data)
+    ProjectCacheWorker.perform_async(@project.id)
   end
 
-  protected
+  def process_default_branch
+    @push_commits = project.repository.commits(params[:newrev])
+
+    # Ensure HEAD points to the default branch in case it is not master
+    project.change_head(branch_name)
+
+    # Set protection on the default branch if configured
+    if (current_application_settings.default_branch_protection != PROTECTION_NONE)
+      developers_can_push = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? true : false
+      @project.protected_branches.create({ name: @project.default_branch, developers_can_push: developers_can_push })
+    end
+  end
 
   # Extract any GFM references from the pushed commit messages. If the configured issue-closing regex is matched,
   # close the referenced Issue. Create cross-reference Notes corresponding to any other referenced Mentionables.
-  def process_commit_messages(ref)
-    is_default_branch = is_default_branch?(ref)
+  def process_commit_messages
+    is_default_branch = is_default_branch?
 
     authors = Hash.new do |hash, commit|
       email = commit.author_email
@@ -94,7 +94,7 @@ class GitPushService
         # Close issues if these commits were pushed to the project's default branch and the commit message matches the
         # closing regex. Exclude any mentioned Issues from cross-referencing even if the commits are being pushed to
         # a different branch.
-        closed_issues = commit.closes_issues(user)
+        closed_issues = commit.closes_issues(current_user)
         closed_issues.each do |issue|
           Issues::CloseService.new(project, authors[commit], {}).execute(issue, commit)
         end
@@ -104,34 +104,38 @@ class GitPushService
     end
   end
 
-  def build_push_data(oldrev, newrev, ref)
-    Gitlab::PushDataBuilder.
-      build(project, user, oldrev, newrev, ref, push_commits)
+  def build_push_data
+    @push_data ||= Gitlab::PushDataBuilder.
+      build(@project, current_user, params[:oldrev], params[:newrev], params[:ref], push_commits)
   end
 
-  def push_to_existing_branch?(ref, oldrev)
+  def push_to_existing_branch?
     # Return if this is not a push to a branch (e.g. new commits)
-    Gitlab::Git.branch_ref?(ref) && !Gitlab::Git.blank_ref?(oldrev)
+    Gitlab::Git.branch_ref?(params[:ref]) && !Gitlab::Git.blank_ref?(params[:oldrev])
   end
 
-  def push_to_new_branch?(ref, oldrev)
-    Gitlab::Git.branch_ref?(ref) && Gitlab::Git.blank_ref?(oldrev)
+  def push_to_new_branch?
+    Gitlab::Git.branch_ref?(params[:ref]) && Gitlab::Git.blank_ref?(params[:oldrev])
   end
 
-  def push_remove_branch?(ref, newrev)
-    Gitlab::Git.branch_ref?(ref) && Gitlab::Git.blank_ref?(newrev)
+  def push_remove_branch?
+    Gitlab::Git.branch_ref?(params[:ref]) && Gitlab::Git.blank_ref?(params[:newrev])
   end
 
-  def push_to_branch?(ref)
-    Gitlab::Git.branch_ref?(ref)
+  def push_to_branch?
+    Gitlab::Git.branch_ref?(params[:ref])
   end
 
-  def is_default_branch?(ref)
-    Gitlab::Git.branch_ref?(ref) &&
-      (Gitlab::Git.ref_name(ref) == project.default_branch || project.default_branch.nil?)
+  def is_default_branch?
+    Gitlab::Git.branch_ref?(params[:ref]) &&
+      (Gitlab::Git.ref_name(params[:ref]) == project.default_branch || project.default_branch.nil?)
   end
 
   def commit_user(commit)
-    commit.author || user
+    commit.author || current_user
+  end
+
+  def branch_name
+    @branch_name ||= Gitlab::Git.ref_name(params[:ref])
   end
 end
diff --git a/app/workers/post_receive.rb b/app/workers/post_receive.rb
index 994b8e8ed38..14d7813412e 100644
--- a/app/workers/post_receive.rb
+++ b/app/workers/post_receive.rb
@@ -38,7 +38,7 @@ class PostReceive
       if Gitlab::Git.tag_ref?(ref)
         GitTagPushService.new.execute(project, @user, oldrev, newrev, ref)
       else
-        GitPushService.new.execute(project, @user, oldrev, newrev, ref)
+        GitPushService.new(project, @user, oldrev: oldrev, newrev: newrev, ref: ref).execute
       end
     end
   end
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index eb3a5fe43f5..994585fb32c 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -5,7 +5,6 @@ describe GitPushService, services: true do
 
   let(:user)          { create :user }
   let(:project)       { create :project }
-  let(:service) { GitPushService.new }
 
   before do
     @blankrev = Gitlab::Git::BLANK_SHA
@@ -15,10 +14,17 @@ describe GitPushService, services: true do
   end
 
   describe 'Push branches' do
+
+    let(:oldrev) { @oldrev }
+    let(:newrev) { @newrev }
+
+    subject do
+      execute_service(project, user, oldrev, newrev, @ref )
+    end
+
     context 'new branch' do
-      subject do
-        service.execute(project, user, @blankrev, @newrev, @ref)
-      end
+
+      let(:oldrev) { @blankrev }
 
       it { is_expected.to be_truthy }
 
@@ -36,9 +42,6 @@ describe GitPushService, services: true do
     end
 
     context 'existing branch' do
-      subject do
-        service.execute(project, user, @oldrev, @newrev, @ref)
-      end
 
       it { is_expected.to be_truthy }
 
@@ -50,9 +53,8 @@ describe GitPushService, services: true do
     end
 
     context 'rm branch' do
-      subject do
-        service.execute(project, user, @oldrev, @blankrev, @ref)
-      end
+
+      let(:newrev) { @blankrev }
 
       it { is_expected.to be_truthy }
 
@@ -72,7 +74,7 @@ describe GitPushService, services: true do
 
   describe "Git Push Data" do
     before do
-      service.execute(project, user, @oldrev, @newrev, @ref)
+      service = execute_service(project, user, @oldrev, @newrev, @ref )
       @push_data = service.push_data
       @commit = project.commit(@newrev)
     end
@@ -134,20 +136,21 @@ describe GitPushService, services: true do
 
   describe "Push Event" do
     before do
-      service.execute(project, user, @oldrev, @newrev, @ref)
+      service = execute_service(project, user, @oldrev, @newrev, @ref )
       @event = Event.last
+      @push_data = service.push_data
     end
 
     it { expect(@event).not_to be_nil }
     it { expect(@event.project).to eq(project) }
     it { expect(@event.action).to eq(Event::PUSHED) }
-    it { expect(@event.data).to eq(service.push_data) }
+    it { expect(@event.data).to eq(@push_data) }
 
     context "Updates merge requests" do
       it "when pushing a new branch for the first time" do
         expect(project).to receive(:update_merge_requests).
                                with(@blankrev, 'newrev', 'refs/heads/master', user)
-        service.execute(project, user, @blankrev, 'newrev', 'refs/heads/master')
+        execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
       end
     end
   end
@@ -158,7 +161,7 @@ describe GitPushService, services: true do
         expect(project).to receive(:execute_hooks)
         expect(project.default_branch).to eq("master")
         expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false })
-        service.execute(project, user, @blankrev, 'newrev', 'refs/heads/master')
+        execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
       end
 
       it "when pushing a branch for the first time with default branch protection disabled" do
@@ -167,7 +170,7 @@ describe GitPushService, services: true do
         expect(project).to receive(:execute_hooks)
         expect(project.default_branch).to eq("master")
         expect(project.protected_branches).not_to receive(:create)
-        service.execute(project, user, @blankrev, 'newrev', 'refs/heads/master')
+        execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
       end
 
       it "when pushing a branch for the first time with default branch protection set to 'developers can push'" do
@@ -176,12 +179,12 @@ describe GitPushService, services: true do
         expect(project).to receive(:execute_hooks)
         expect(project.default_branch).to eq("master")
         expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: true })
-        service.execute(project, user, @blankrev, 'newrev', 'refs/heads/master')
+        execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
       end
 
       it "when pushing new commits to existing branch" do
         expect(project).to receive(:execute_hooks)
-        service.execute(project, user, 'oldrev', 'newrev', 'refs/heads/master')
+        execute_service(project, user, 'oldrev', 'newrev', 'refs/heads/master' )
       end
     end
   end
@@ -204,7 +207,7 @@ describe GitPushService, services: true do
     it "creates a note if a pushed commit mentions an issue" do
       expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author)
 
-      service.execute(project, user, @oldrev, @newrev, @ref)
+      execute_service(project, user, @oldrev, @newrev, @ref )
     end
 
     it "only creates a cross-reference note if one doesn't already exist" do
@@ -212,7 +215,7 @@ describe GitPushService, services: true do
 
       expect(SystemNoteService).not_to receive(:cross_reference).with(issue, commit, commit_author)
 
-      service.execute(project, user, @oldrev, @newrev, @ref)
+      execute_service(project, user, @oldrev, @newrev, @ref )
     end
 
     it "defaults to the pushing user if the commit's author is not known" do
@@ -222,7 +225,7 @@ describe GitPushService, services: true do
       )
       expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, user)
 
-      service.execute(project, user, @oldrev, @newrev, @ref)
+      execute_service(project, user, @oldrev, @newrev, @ref )
     end
 
     it "finds references in the first push to a non-default branch" do
@@ -231,7 +234,7 @@ describe GitPushService, services: true do
 
       expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author)
 
-      service.execute(project, user, @blankrev, @newrev, 'refs/heads/other')
+      execute_service(project, user, @blankrev, @newrev, 'refs/heads/other' )
     end
   end
 
@@ -255,18 +258,18 @@ describe GitPushService, services: true do
 
     context "to default branches" do
       it "closes issues" do
-        service.execute(project, user, @oldrev, @newrev, @ref)
+        execute_service(project, user, @oldrev, @newrev, @ref )
         expect(Issue.find(issue.id)).to be_closed
       end
 
       it "adds a note indicating that the issue is now closed" do
         expect(SystemNoteService).to receive(:change_status).with(issue, project, commit_author, "closed", closing_commit)
-        service.execute(project, user, @oldrev, @newrev, @ref)
+        execute_service(project, user, @oldrev, @newrev, @ref )
       end
 
       it "doesn't create additional cross-reference notes" do
         expect(SystemNoteService).not_to receive(:cross_reference)
-        service.execute(project, user, @oldrev, @newrev, @ref)
+        execute_service(project, user, @oldrev, @newrev, @ref )
       end
 
       it "doesn't close issues when external issue tracker is in use" do
@@ -274,7 +277,7 @@ describe GitPushService, services: true do
 
         # The push still shouldn't create cross-reference notes.
         expect do
-          service.execute(project, user, @oldrev, @newrev, 'refs/heads/hurf')
+          execute_service(project, user, @oldrev, @newrev,  'refs/heads/hurf' )
         end.not_to change { Note.where(project_id: project.id, system: true).count }
       end
     end
@@ -287,11 +290,11 @@ describe GitPushService, services: true do
 
       it "creates cross-reference notes" do
         expect(SystemNoteService).to receive(:cross_reference).with(issue, closing_commit, commit_author)
-        service.execute(project, user, @oldrev, @newrev, @ref)
+        execute_service(project, user, @oldrev, @newrev, @ref )
       end
 
       it "doesn't close issues" do
-        service.execute(project, user, @oldrev, @newrev, @ref)
+        execute_service(project, user, @oldrev, @newrev, @ref )
         expect(Issue.find(issue.id)).to be_opened
       end
     end
@@ -328,7 +331,7 @@ describe GitPushService, services: true do
         let(:message) { "this is some work.\n\nrelated to JIRA-1" }
 
         it "should initiate one api call to jira server to mention the issue" do
-          service.execute(project, user, @oldrev, @newrev, @ref)
+          execute_service(project, user, @oldrev, @newrev, @ref )
 
           expect(WebMock).to have_requested(:post, jira_api_comment_url).with(
             body: /mentioned this issue in/
@@ -346,7 +349,7 @@ describe GitPushService, services: true do
             }
           }.to_json
 
-          service.execute(project, user, @oldrev, @newrev, @ref)
+          execute_service(project, user, @oldrev, @newrev, @ref )
           expect(WebMock).to have_requested(:post, jira_api_transition_url).with(
             body: transition_body
           ).once
@@ -357,7 +360,7 @@ describe GitPushService, services: true do
             body: "Issue solved with [#{closing_commit.id}|http://localhost/#{project.path_with_namespace}/commit/#{closing_commit.id}]."
           }.to_json
 
-          service.execute(project, user, @oldrev, @newrev, @ref)
+          execute_service(project, user, @oldrev, @newrev, @ref )
           expect(WebMock).to have_requested(:post, jira_api_comment_url).with(
             body: comment_body
           ).once
@@ -376,7 +379,13 @@ describe GitPushService, services: true do
     end
 
     it 'push to first branch updates HEAD' do
-      service.execute(project, user, @blankrev, @newrev, new_ref)
+      execute_service(project, user, @blankrev, @newrev, new_ref )
     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