Commit 77d2aeab authored by James Lopez's avatar James Lopez

attempt to reduce code complexity on GitPushService#execute

parent 9bcc9ec1
...@@ -3,6 +3,15 @@ class GitPushService ...@@ -3,6 +3,15 @@ class GitPushService
include Gitlab::CurrentSettings include Gitlab::CurrentSettings
include Gitlab::Access 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 # 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 is present in GitLab.
# #
...@@ -15,67 +24,67 @@ class GitPushService ...@@ -15,67 +24,67 @@ class GitPushService
# 4. Executes the project's web hooks # 4. Executes the project's web hooks
# 5. Executes the project's services # 5. Executes the project's services
# #
def execute(project, user, oldrev, newrev, ref) def execute
@project, @user = project, user @project.repository.expire_cache(branch_name)
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
if push_remove_branch?
@project.repository.expire_has_visible_content_cache
@push_commits = [] @push_commits = []
elsif push_to_new_branch?(ref, oldrev) elsif push_to_new_branch?
project.repository.expire_has_visible_content_cache @project.repository.expire_has_visible_content_cache
# Re-find the pushed commits. # 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". # Initial push to the default branch. Take the full history of that branch as "newly pushed".
@push_commits = project.repository.commits(newrev) process_default_branch
# 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
else else
# Use the pushed commits that aren't reachable by the default 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 # 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. # 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, @newrev)
# don't process commits for the initial push to the default branch # don't process commits for the initial push to the default branch
process_commit_messages(ref) process_commit_messages
end end
elsif push_to_existing_branch?(ref, oldrev) elsif push_to_existing_branch?
# Collect data for this git push # Collect data for this git push
@push_commits = project.repository.commits_between(oldrev, newrev) @push_commits = @project.repository.commits_between(@oldrev, @newrev)
process_commit_messages(ref) process_commit_messages
end end
# Update merge requests that may be affected by this push. A new branch # Update merge requests that may be affected by this push. A new branch
# could cause the last commit of a merge request to change. # 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) EventCreateService.new.push(@project, @user, build_push_data)
project.execute_hooks(@push_data.dup, :push_hooks) @project.execute_hooks(build_push_data.dup, :push_hooks)
project.execute_services(@push_data.dup, :push_hooks) @project.execute_services(build_push_data.dup, :push_hooks)
CreateCommitBuildsService.new.execute(project, @user, @push_data) CreateCommitBuildsService.new.execute(@project, @user, build_push_data)
ProjectCacheWorker.perform_async(project.id) ProjectCacheWorker.perform_async(@project.id)
end 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, # 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. # close the referenced Issue. Create cross-reference Notes corresponding to any other referenced Mentionables.
def process_commit_messages(ref) def process_commit_messages
is_default_branch = is_default_branch?(ref) is_default_branch = is_default_branch?
authors = Hash.new do |hash, commit| authors = Hash.new do |hash, commit|
email = commit.author_email email = commit.author_email
...@@ -104,34 +113,38 @@ class GitPushService ...@@ -104,34 +113,38 @@ class GitPushService
end end
end end
def build_push_data(oldrev, newrev, ref) def build_push_data
Gitlab::PushDataBuilder. @push_data ||= Gitlab::PushDataBuilder.
build(project, user, oldrev, newrev, ref, push_commits) build(@project, @user, @oldrev, @newrev, @ref, push_commits)
end 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) # 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 end
def push_to_new_branch?(ref, oldrev) def push_to_new_branch?
Gitlab::Git.branch_ref?(ref) && Gitlab::Git.blank_ref?(oldrev) Gitlab::Git.branch_ref?(@ref) && Gitlab::Git.blank_ref?(@oldrev)
end end
def push_remove_branch?(ref, newrev) def push_remove_branch?
Gitlab::Git.branch_ref?(ref) && Gitlab::Git.blank_ref?(newrev) Gitlab::Git.branch_ref?(@ref) && Gitlab::Git.blank_ref?(@newrev)
end end
def push_to_branch?(ref) def push_to_branch?
Gitlab::Git.branch_ref?(ref) Gitlab::Git.branch_ref?(@ref)
end end
def is_default_branch?(ref) def is_default_branch?
Gitlab::Git.branch_ref?(ref) && Gitlab::Git.branch_ref?(@ref) &&
(Gitlab::Git.ref_name(ref) == project.default_branch || project.default_branch.nil?) (Gitlab::Git.ref_name(@ref) == project.default_branch || project.default_branch.nil?)
end end
def commit_user(commit) def commit_user(commit)
commit.author || user commit.author || user
end end
def branch_name
@_branch_name ||= Gitlab::Git.ref_name(@ref)
end
end end
...@@ -38,7 +38,7 @@ class PostReceive ...@@ -38,7 +38,7 @@ class PostReceive
if Gitlab::Git.tag_ref?(ref) if Gitlab::Git.tag_ref?(ref)
GitTagPushService.new.execute(project, @user, oldrev, newrev, ref) GitTagPushService.new.execute(project, @user, oldrev, newrev, ref)
else else
GitPushService.new.execute(project, @user, oldrev, newrev, ref) GitPushService.new(project, @user, oldrev, newrev, ref).execute
end end
end end
end end
......
...@@ -5,7 +5,6 @@ describe GitPushService, services: true do ...@@ -5,7 +5,6 @@ describe GitPushService, services: true do
let(:user) { create :user } let(:user) { create :user }
let(:project) { create :project } let(:project) { create :project }
let(:service) { GitPushService.new }
before do before do
@blankrev = Gitlab::Git::BLANK_SHA @blankrev = Gitlab::Git::BLANK_SHA
...@@ -17,7 +16,8 @@ describe GitPushService, services: true do ...@@ -17,7 +16,8 @@ describe GitPushService, services: true do
describe 'Push branches' do describe 'Push branches' do
context 'new branch' do context 'new branch' do
subject do subject do
service.execute(project, user, @blankrev, @newrev, @ref) service = described_class.new(project, user, @blankrev, @newrev, @ref)
service.execute
end end
it { is_expected.to be_truthy } it { is_expected.to be_truthy }
...@@ -37,7 +37,8 @@ describe GitPushService, services: true do ...@@ -37,7 +37,8 @@ describe GitPushService, services: true do
context 'existing branch' do context 'existing branch' do
subject do subject do
service.execute(project, user, @oldrev, @newrev, @ref) service = described_class.new(project, user, @oldrev, @newrev, @ref)
service.execute
end end
it { is_expected.to be_truthy } it { is_expected.to be_truthy }
...@@ -51,7 +52,8 @@ describe GitPushService, services: true do ...@@ -51,7 +52,8 @@ describe GitPushService, services: true do
context 'rm branch' do context 'rm branch' do
subject do subject do
service.execute(project, user, @oldrev, @blankrev, @ref) service = described_class.new(project, user, @oldrev, @blankrev, @ref)
service.execute
end end
it { is_expected.to be_truthy } it { is_expected.to be_truthy }
...@@ -72,7 +74,8 @@ describe GitPushService, services: true do ...@@ -72,7 +74,8 @@ describe GitPushService, services: true do
describe "Git Push Data" do describe "Git Push Data" do
before 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 @push_data = service.push_data
@commit = project.commit(@newrev) @commit = project.commit(@newrev)
end end
...@@ -134,20 +137,23 @@ describe GitPushService, services: true do ...@@ -134,20 +137,23 @@ describe GitPushService, services: true do
describe "Push Event" do describe "Push Event" do
before do before do
service.execute(project, user, @oldrev, @newrev, @ref) service = described_class.new(project, user, @oldrev, @newrev, @ref)
service.execute
@event = Event.last @event = Event.last
@push_data = service.push_data
end end
it { expect(@event).not_to be_nil } it { expect(@event).not_to be_nil }
it { expect(@event.project).to eq(project) } it { expect(@event.project).to eq(project) }
it { expect(@event.action).to eq(Event::PUSHED) } 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 context "Updates merge requests" do
it "when pushing a new branch for the first time" do it "when pushing a new branch for the first time" do
expect(project).to receive(:update_merge_requests). expect(project).to receive(:update_merge_requests).
with(@blankrev, 'newrev', 'refs/heads/master', user) 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 end
end end
...@@ -158,7 +164,8 @@ describe GitPushService, services: true do ...@@ -158,7 +164,8 @@ describe GitPushService, services: true do
expect(project).to receive(:execute_hooks) expect(project).to receive(:execute_hooks)
expect(project.default_branch).to eq("master") expect(project.default_branch).to eq("master")
expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false }) 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 end
it "when pushing a branch for the first time with default branch protection disabled" do it "when pushing a branch for the first time with default branch protection disabled" do
...@@ -167,7 +174,8 @@ describe GitPushService, services: true do ...@@ -167,7 +174,8 @@ describe GitPushService, services: true do
expect(project).to receive(:execute_hooks) expect(project).to receive(:execute_hooks)
expect(project.default_branch).to eq("master") expect(project.default_branch).to eq("master")
expect(project.protected_branches).not_to receive(:create) 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 end
it "when pushing a branch for the first time with default branch protection set to 'developers can push'" do 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 ...@@ -176,12 +184,14 @@ describe GitPushService, services: true do
expect(project).to receive(:execute_hooks) expect(project).to receive(:execute_hooks)
expect(project.default_branch).to eq("master") expect(project.default_branch).to eq("master")
expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: true }) 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 end
it "when pushing new commits to existing branch" do it "when pushing new commits to existing branch" do
expect(project).to receive(:execute_hooks) 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 end
end end
...@@ -204,7 +214,9 @@ describe GitPushService, services: true do ...@@ -204,7 +214,9 @@ describe GitPushService, services: true do
it "creates a note if a pushed commit mentions an issue" do it "creates a note if a pushed commit mentions an issue" do
expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author) 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 end
it "only creates a cross-reference note if one doesn't already exist" do it "only creates a cross-reference note if one doesn't already exist" do
...@@ -212,7 +224,9 @@ describe GitPushService, services: true do ...@@ -212,7 +224,9 @@ describe GitPushService, services: true do
expect(SystemNoteService).not_to receive(:cross_reference).with(issue, commit, commit_author) 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 end
it "defaults to the pushing user if the commit's author is not known" do it "defaults to the pushing user if the commit's author is not known" do
...@@ -222,7 +236,9 @@ describe GitPushService, services: true do ...@@ -222,7 +236,9 @@ describe GitPushService, services: true do
) )
expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, user) 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 end
it "finds references in the first push to a non-default branch" do it "finds references in the first push to a non-default branch" do
...@@ -231,7 +247,9 @@ describe GitPushService, services: true do ...@@ -231,7 +247,9 @@ describe GitPushService, services: true do
expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author) 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
end end
...@@ -255,18 +273,21 @@ describe GitPushService, services: true do ...@@ -255,18 +273,21 @@ describe GitPushService, services: true do
context "to default branches" do context "to default branches" do
it "closes issues" 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 expect(Issue.find(issue.id)).to be_closed
end end
it "adds a note indicating that the issue is now closed" do 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) 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 end
it "doesn't create additional cross-reference notes" do it "doesn't create additional cross-reference notes" do
expect(SystemNoteService).not_to receive(:cross_reference) 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 end
it "doesn't close issues when external issue tracker is in use" do it "doesn't close issues when external issue tracker is in use" do
...@@ -274,7 +295,8 @@ describe GitPushService, services: true do ...@@ -274,7 +295,8 @@ describe GitPushService, services: true do
# The push still shouldn't create cross-reference notes. # The push still shouldn't create cross-reference notes.
expect do 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.not_to change { Note.where(project_id: project.id, system: true).count }
end end
end end
...@@ -287,11 +309,13 @@ describe GitPushService, services: true do ...@@ -287,11 +309,13 @@ describe GitPushService, services: true do
it "creates cross-reference notes" do it "creates cross-reference notes" do
expect(SystemNoteService).to receive(:cross_reference).with(issue, closing_commit, commit_author) 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 end
it "doesn't close issues" do 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 expect(Issue.find(issue.id)).to be_opened
end end
end end
...@@ -328,7 +352,8 @@ describe GitPushService, services: true do ...@@ -328,7 +352,8 @@ describe GitPushService, services: true do
let(:message) { "this is some work.\n\nrelated to JIRA-1" } 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 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( expect(WebMock).to have_requested(:post, jira_api_comment_url).with(
body: /mentioned this issue in/ body: /mentioned this issue in/
...@@ -346,7 +371,9 @@ describe GitPushService, services: true do ...@@ -346,7 +371,9 @@ describe GitPushService, services: true do
} }
}.to_json }.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( expect(WebMock).to have_requested(:post, jira_api_transition_url).with(
body: transition_body body: transition_body
).once ).once
...@@ -357,7 +384,9 @@ describe GitPushService, services: true do ...@@ -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}]." body: "Issue solved with [#{closing_commit.id}|http://localhost/#{project.path_with_namespace}/commit/#{closing_commit.id}]."
}.to_json }.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( expect(WebMock).to have_requested(:post, jira_api_comment_url).with(
body: comment_body body: comment_body
).once ).once
...@@ -376,7 +405,8 @@ describe GitPushService, services: true do ...@@ -376,7 +405,8 @@ describe GitPushService, services: true do
end end
it 'push to first branch updates HEAD' do 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 end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment