From 77d2aeabec4601a1d9dda93c8fff0c7d1051ba1f Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 11 Feb 2016 12:50:27 +0100 Subject: attempt to reduce code complexity on GitPushService#execute --- app/services/git_push_service.rb | 119 ++++++++++++++++++--------------- app/workers/post_receive.rb | 2 +- spec/services/git_push_service_spec.rb | 82 ++++++++++++++++------- 3 files changed, 123 insertions(+), 80 deletions(-) diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index e3bf14966c8..491322f5ff5 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -3,6 +3,15 @@ class GitPushService include Gitlab::CurrentSettings include Gitlab::Access + def initialize(project, user, oldrev, newrev, ref) + # TODO: Consider passing an options hash instead https://github.com/bbatsov/ruby-style-guide#too-many-params + @project = project + @user = user + @oldrev = oldrev + @newrev = newrev + @ref = ref + end + # This method will be called after each git update # and only if the provided user and project is present in GitLab. # @@ -15,67 +24,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) + # that shouldn't matter because we check for existing cross-@references later. + @push_commits = @project.repository.commits_between(@project.default_branch, @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(@oldrev, @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 + + def update_merge_requests + @project.update_merge_requests(@oldrev, @newrev, @ref, @user) - 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) + EventCreateService.new.push(@project, @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, @user, build_push_data) + ProjectCacheWorker.perform_async(@project.id) end - protected + def process_default_branch + @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 + 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 @@ -104,34 +113,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, @user, @oldrev, @newrev, @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?(@ref) && !Gitlab::Git.blank_ref?(@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?(@ref) && Gitlab::Git.blank_ref?(@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?(@ref) && Gitlab::Git.blank_ref?(@newrev) end - def push_to_branch?(ref) - Gitlab::Git.branch_ref?(ref) + def push_to_branch? + Gitlab::Git.branch_ref?(@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?(@ref) && + (Gitlab::Git.ref_name(@ref) == project.default_branch || project.default_branch.nil?) end def commit_user(commit) commit.author || user end + + def branch_name + @_branch_name ||= Gitlab::Git.ref_name(@ref) + end end diff --git a/app/workers/post_receive.rb b/app/workers/post_receive.rb index 994b8e8ed38..80081ac5f49 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, newrev, 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..ad18f48ff91 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 @@ -17,7 +16,8 @@ describe GitPushService, services: true do describe 'Push branches' do context 'new branch' do subject do - service.execute(project, user, @blankrev, @newrev, @ref) + service = described_class.new(project, user, @blankrev, @newrev, @ref) + service.execute end it { is_expected.to be_truthy } @@ -37,7 +37,8 @@ describe GitPushService, services: true do context 'existing branch' do subject do - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + service.execute end it { is_expected.to be_truthy } @@ -51,7 +52,8 @@ describe GitPushService, services: true do context 'rm branch' do subject do - service.execute(project, user, @oldrev, @blankrev, @ref) + service = described_class.new(project, user, @oldrev, @blankrev, @ref) + service.execute end it { is_expected.to be_truthy } @@ -72,7 +74,8 @@ describe GitPushService, services: true do describe "Git Push Data" do before do - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + service.execute @push_data = service.push_data @commit = project.commit(@newrev) end @@ -134,20 +137,23 @@ describe GitPushService, services: true do describe "Push Event" do before do - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + service.execute @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') + service = described_class.new(project, user, @blankrev, 'newrev', 'refs/heads/master') + service.execute end end end @@ -158,7 +164,8 @@ 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') + service = described_class.new(project, user, @blankrev, 'newrev', 'refs/heads/master') + service.execute end it "when pushing a branch for the first time with default branch protection disabled" do @@ -167,7 +174,8 @@ 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') + service = described_class.new(project, user, @blankrev, 'newrev', 'refs/heads/master') + service.execute end it "when pushing a branch for the first time with default branch protection set to 'developers can push'" do @@ -176,12 +184,14 @@ 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') + service = described_class.new(project, user, @blankrev, 'newrev', 'refs/heads/master') + service.execute 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') + service = described_class.new(project, user, 'oldrev', 'newrev', 'refs/heads/master') + service.execute end end end @@ -204,7 +214,9 @@ 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) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + + service.execute end it "only creates a cross-reference note if one doesn't already exist" do @@ -212,7 +224,9 @@ describe GitPushService, services: true do expect(SystemNoteService).not_to receive(:cross_reference).with(issue, commit, commit_author) - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + + service.execute end it "defaults to the pushing user if the commit's author is not known" do @@ -222,7 +236,9 @@ describe GitPushService, services: true do ) expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, user) - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + + service.execute end it "finds references in the first push to a non-default branch" do @@ -231,7 +247,9 @@ 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') + service = described_class.new(project, user, @blankrev, @newrev, 'refs/heads/other') + + service.execute end end @@ -255,18 +273,21 @@ describe GitPushService, services: true do context "to default branches" do it "closes issues" do - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + service.execute 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) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + service.execute end it "doesn't create additional cross-reference notes" do expect(SystemNoteService).not_to receive(:cross_reference) - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + service.execute end it "doesn't close issues when external issue tracker is in use" do @@ -274,7 +295,8 @@ 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') + service = described_class.new(project, user, @oldrev, @newrev, 'refs/heads/hurf') + service.execute end.not_to change { Note.where(project_id: project.id, system: true).count } end end @@ -287,11 +309,13 @@ 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) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + service.execute end it "doesn't close issues" do - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + service.execute expect(Issue.find(issue.id)).to be_opened end end @@ -328,7 +352,8 @@ 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) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + service.execute expect(WebMock).to have_requested(:post, jira_api_comment_url).with( body: /mentioned this issue in/ @@ -346,7 +371,9 @@ describe GitPushService, services: true do } }.to_json - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + + service.execute expect(WebMock).to have_requested(:post, jira_api_transition_url).with( body: transition_body ).once @@ -357,7 +384,9 @@ 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) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + + service.execute expect(WebMock).to have_requested(:post, jira_api_comment_url).with( body: comment_body ).once @@ -376,7 +405,8 @@ describe GitPushService, services: true do end it 'push to first branch updates HEAD' do - service.execute(project, user, @blankrev, @newrev, new_ref) + service = described_class.new(project, user, @blankrev, @newrev, new_ref) + service.execute end end end -- cgit v1.2.1