refactored some stuff based on MR feedback

3 files changed, 34 insertions, 63 deletions

diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index b203065e708..a1711d234ff 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -106,7 +106,7 @@ class GitPushService < BaseService
 
   def build_push_data
     @push_data ||= Gitlab::PushDataBuilder.
-        build(@project, current_user, params[:oldrev], params[:newrev], params[:ref], push_commits)
+      build(@project, current_user, params[:oldrev], params[:newrev], params[:ref], push_commits)
   end
 
   def push_to_existing_branch?
@@ -128,7 +128,7 @@ class GitPushService < BaseService
 
   def is_default_branch?
     Gitlab::Git.branch_ref?(params[:ref]) &&
-        (Gitlab::Git.ref_name(params[:ref]) == project.default_branch || project.default_branch.nil?)
+      (Gitlab::Git.ref_name(params[:ref]) == project.default_branch || project.default_branch.nil?)
   end
 
   def commit_user(commit)
@@ -136,6 +136,6 @@ class GitPushService < BaseService
   end
 
   def branch_name
-    @_branch_name ||= Gitlab::Git.ref_name(params[:ref])
+    @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 9e7934b84d8..14d7813412e 100644
--- a/app/workers/post_receive.rb
+++ b/app/workers/post_receive.rb
@@ -38,12 +38,7 @@ class PostReceive
       if Gitlab::Git.tag_ref?(ref)
         GitTagPushService.new.execute(project, @user, oldrev, newrev, ref)
       else
-        GitPushService.new(project, @user,
-                           {
-                               oldrev: oldrev,
-                               newrev: newrev,
-                               ref: ref
-                           }).execute
+        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 f953f226e67..48e796b6946 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -16,8 +16,7 @@ describe GitPushService, services: true do
   describe 'Push branches' do
     context 'new branch' do
       subject do
-        service = described_class.new(project, user, { oldrev: @blankrev, newrev: @newrev, ref: @ref })
-        service.execute
+        execute_service(project, user, @blankrev, @newrev, @ref )
       end
 
       it { is_expected.to be_truthy }
@@ -37,8 +36,7 @@ describe GitPushService, services: true do
 
     context 'existing branch' do
       subject do
-        service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref })
-        service.execute
+        execute_service(project, user, @oldrev, @newrev, @ref )
       end
 
       it { is_expected.to be_truthy }
@@ -52,8 +50,7 @@ describe GitPushService, services: true do
 
     context 'rm branch' do
       subject do
-        service = described_class.new(project, user, { oldrev: @oldrev, newrev: @blankrev, ref: @ref })
-        service.execute
+        execute_service(project, user, @oldrev, @blankrev, @ref )
       end
 
       it { is_expected.to be_truthy }
@@ -74,8 +71,7 @@ describe GitPushService, services: true do
 
   describe "Git Push Data" do
     before do
-      service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref })
-      service.execute
+      service = execute_service(project, user, @oldrev, @newrev, @ref )
       @push_data = service.push_data
       @commit = project.commit(@newrev)
     end
@@ -137,8 +133,7 @@ describe GitPushService, services: true do
 
   describe "Push Event" do
     before do
-      service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref })
-      service.execute
+      service = execute_service(project, user, @oldrev, @newrev, @ref )
       @event = Event.last
       @push_data = service.push_data
     end
@@ -152,8 +147,7 @@ describe GitPushService, services: true 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 = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' })
-        service.execute
+        execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
       end
     end
   end
@@ -164,8 +158,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 = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' })
-        service.execute
+        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
@@ -174,8 +167,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 = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' })
-        service.execute
+        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
@@ -184,14 +176,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 = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' })
-        service.execute
+        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 = described_class.new(project, user, { oldrev: 'oldrev', newrev: 'newrev', ref: 'refs/heads/master' })
-        service.execute
+        execute_service(project, user, 'oldrev', 'newrev', 'refs/heads/master' )
       end
     end
   end
@@ -214,9 +204,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 = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref })
-
-      service.execute
+      execute_service(project, user, @oldrev, @newrev, @ref )
     end
 
     it "only creates a cross-reference note if one doesn't already exist" do
@@ -224,9 +212,7 @@ describe GitPushService, services: true do
 
       expect(SystemNoteService).not_to receive(:cross_reference).with(issue, commit, commit_author)
 
-      service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref })
-
-      service.execute
+      execute_service(project, user, @oldrev, @newrev, @ref )
     end
 
     it "defaults to the pushing user if the commit's author is not known" do
@@ -236,9 +222,7 @@ describe GitPushService, services: true do
       )
       expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, user)
 
-      service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref })
-
-      service.execute
+      execute_service(project, user, @oldrev, @newrev, @ref )
     end
 
     it "finds references in the first push to a non-default branch" do
@@ -247,9 +231,7 @@ describe GitPushService, services: true do
 
       expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author)
 
-      service = described_class.new(project, user, { oldrev: @blankrev, newrev: @newrev, ref: 'refs/heads/other' })
-
-      service.execute
+      execute_service(project, user, @blankrev, @newrev, 'refs/heads/other' )
     end
   end
 
@@ -273,21 +255,18 @@ describe GitPushService, services: true do
 
     context "to default branches" do
       it "closes issues" do
-        service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref })
-        service.execute
+        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 = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref })
-        service.execute
+        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 = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref })
-        service.execute
+        execute_service(project, user, @oldrev, @newrev, @ref )
       end
 
       it "doesn't close issues when external issue tracker is in use" do
@@ -295,8 +274,7 @@ describe GitPushService, services: true do
 
         # The push still shouldn't create cross-reference notes.
         expect do
-          service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref:  'refs/heads/hurf' })
-          service.execute
+          execute_service(project, user, @oldrev, @newrev,  'refs/heads/hurf' )
         end.not_to change { Note.where(project_id: project.id, system: true).count }
       end
     end
@@ -309,13 +287,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 = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref })
-        service.execute
+        execute_service(project, user, @oldrev, @newrev, @ref )
       end
 
       it "doesn't close issues" do
-        service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref })
-        service.execute
+        execute_service(project, user, @oldrev, @newrev, @ref )
         expect(Issue.find(issue.id)).to be_opened
       end
     end
@@ -352,8 +328,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 = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref })
-          service.execute
+          execute_service(project, user, @oldrev, @newrev, @ref )
 
           expect(WebMock).to have_requested(:post, jira_api_comment_url).with(
             body: /mentioned this issue in/
@@ -371,9 +346,7 @@ describe GitPushService, services: true do
             }
           }.to_json
 
-          service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref })
-
-          service.execute
+          execute_service(project, user, @oldrev, @newrev, @ref )
           expect(WebMock).to have_requested(:post, jira_api_transition_url).with(
             body: transition_body
           ).once
@@ -384,9 +357,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 = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref })
-
-          service.execute
+          execute_service(project, user, @oldrev, @newrev, @ref )
           expect(WebMock).to have_requested(:post, jira_api_comment_url).with(
             body: comment_body
           ).once
@@ -405,8 +376,13 @@ describe GitPushService, services: true do
     end
 
     it 'push to first branch updates HEAD' do
-      service = described_class.new(project, user, { oldrev: @blankrev, newrev: @newrev, ref: new_ref })
-      service.execute
+      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