Commit 52a8d1f3 authored by Douwe Maan's avatar Douwe Maan

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
parents 27b732ab c110c9bd
class GitPushService class GitPushService < BaseService
attr_accessor :project, :user, :push_data, :push_commits attr_accessor :push_data, :push_commits
include Gitlab::CurrentSettings include Gitlab::CurrentSettings
include Gitlab::Access include Gitlab::Access
# 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 are present in GitLab.
# #
# All callbacks for post receive action should be placed here. # All callbacks for post receive action should be placed here.
# #
...@@ -15,67 +15,67 @@ class GitPushService ...@@ -15,67 +15,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, params[: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(params[:oldrev], params[: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
EventCreateService.new.push(project, user, @push_data) def update_merge_requests
project.execute_hooks(@push_data.dup, :push_hooks) @project.update_merge_requests(params[:oldrev], params[:newrev], params[:ref], current_user)
project.execute_services(@push_data.dup, :push_hooks)
CreateCommitBuildsService.new.execute(project, @user, @push_data) EventCreateService.new.push(@project, current_user, build_push_data)
ProjectCacheWorker.perform_async(project.id) @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 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, # 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
...@@ -94,7 +94,7 @@ class GitPushService ...@@ -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 # 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 # closing regex. Exclude any mentioned Issues from cross-referencing even if the commits are being pushed to
# a different branch. # a different branch.
closed_issues = commit.closes_issues(user) closed_issues = commit.closes_issues(current_user)
closed_issues.each do |issue| closed_issues.each do |issue|
Issues::CloseService.new(project, authors[commit], {}).execute(issue, commit) Issues::CloseService.new(project, authors[commit], {}).execute(issue, commit)
end end
...@@ -104,34 +104,38 @@ class GitPushService ...@@ -104,34 +104,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, current_user, params[:oldrev], params[:newrev], params[: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?(params[:ref]) && !Gitlab::Git.blank_ref?(params[: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?(params[:ref]) && Gitlab::Git.blank_ref?(params[: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?(params[:ref]) && Gitlab::Git.blank_ref?(params[:newrev])
end end
def push_to_branch?(ref) def push_to_branch?
Gitlab::Git.branch_ref?(ref) Gitlab::Git.branch_ref?(params[:ref])
end end
def is_default_branch?(ref) def is_default_branch?
Gitlab::Git.branch_ref?(ref) && Gitlab::Git.branch_ref?(params[:ref]) &&
(Gitlab::Git.ref_name(ref) == project.default_branch || project.default_branch.nil?) (Gitlab::Git.ref_name(params[:ref]) == project.default_branch || project.default_branch.nil?)
end end
def commit_user(commit) 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
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: oldrev, newrev: newrev, ref: 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
...@@ -15,10 +14,17 @@ describe GitPushService, services: true do ...@@ -15,10 +14,17 @@ describe GitPushService, services: true do
end end
describe 'Push branches' do describe 'Push branches' do
let(:oldrev) { @oldrev }
let(:newrev) { @newrev }
subject do
execute_service(project, user, oldrev, newrev, @ref )
end
context 'new branch' do context 'new branch' do
subject do
service.execute(project, user, @blankrev, @newrev, @ref) let(:oldrev) { @blankrev }
end
it { is_expected.to be_truthy } it { is_expected.to be_truthy }
...@@ -36,9 +42,6 @@ describe GitPushService, services: true do ...@@ -36,9 +42,6 @@ describe GitPushService, services: true do
end end
context 'existing branch' do context 'existing branch' do
subject do
service.execute(project, user, @oldrev, @newrev, @ref)
end
it { is_expected.to be_truthy } it { is_expected.to be_truthy }
...@@ -50,9 +53,8 @@ describe GitPushService, services: true do ...@@ -50,9 +53,8 @@ describe GitPushService, services: true do
end end
context 'rm branch' do context 'rm branch' do
subject do
service.execute(project, user, @oldrev, @blankrev, @ref) let(:newrev) { @blankrev }
end
it { is_expected.to be_truthy } it { is_expected.to be_truthy }
...@@ -72,7 +74,7 @@ describe GitPushService, services: true do ...@@ -72,7 +74,7 @@ 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 = execute_service(project, user, @oldrev, @newrev, @ref )
@push_data = service.push_data @push_data = service.push_data
@commit = project.commit(@newrev) @commit = project.commit(@newrev)
end end
...@@ -134,20 +136,21 @@ describe GitPushService, services: true do ...@@ -134,20 +136,21 @@ 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 = execute_service(project, user, @oldrev, @newrev, @ref )
@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') execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
end end
end end
end end
...@@ -158,7 +161,7 @@ describe GitPushService, services: true do ...@@ -158,7 +161,7 @@ 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') execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
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 +170,7 @@ describe GitPushService, services: true do ...@@ -167,7 +170,7 @@ 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') execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
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 +179,12 @@ describe GitPushService, services: true do ...@@ -176,12 +179,12 @@ 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') execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
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') execute_service(project, user, 'oldrev', 'newrev', 'refs/heads/master' )
end end
end end
end end
...@@ -204,7 +207,7 @@ describe GitPushService, services: true do ...@@ -204,7 +207,7 @@ 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) execute_service(project, user, @oldrev, @newrev, @ref )
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 +215,7 @@ describe GitPushService, services: true do ...@@ -212,7 +215,7 @@ 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) execute_service(project, user, @oldrev, @newrev, @ref )
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 +225,7 @@ describe GitPushService, services: true do ...@@ -222,7 +225,7 @@ 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) execute_service(project, user, @oldrev, @newrev, @ref )
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 +234,7 @@ describe GitPushService, services: true do ...@@ -231,7 +234,7 @@ 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') execute_service(project, user, @blankrev, @newrev, 'refs/heads/other' )
end end
end end
...@@ -255,18 +258,18 @@ describe GitPushService, services: true do ...@@ -255,18 +258,18 @@ 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) execute_service(project, user, @oldrev, @newrev, @ref )
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) execute_service(project, user, @oldrev, @newrev, @ref )
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) execute_service(project, user, @oldrev, @newrev, @ref )
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 +277,7 @@ describe GitPushService, services: true do ...@@ -274,7 +277,7 @@ 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') execute_service(project, user, @oldrev, @newrev, 'refs/heads/hurf' )
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 +290,11 @@ describe GitPushService, services: true do ...@@ -287,11 +290,11 @@ 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) execute_service(project, user, @oldrev, @newrev, @ref )
end end
it "doesn't close issues" do 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 expect(Issue.find(issue.id)).to be_opened
end end
end end
...@@ -328,7 +331,7 @@ describe GitPushService, services: true do ...@@ -328,7 +331,7 @@ 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) execute_service(project, user, @oldrev, @newrev, @ref )
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 +349,7 @@ describe GitPushService, services: true do ...@@ -346,7 +349,7 @@ describe GitPushService, services: true do
} }
}.to_json }.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( expect(WebMock).to have_requested(:post, jira_api_transition_url).with(
body: transition_body body: transition_body
).once ).once
...@@ -357,7 +360,7 @@ describe GitPushService, services: true do ...@@ -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}]." 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) execute_service(project, user, @oldrev, @newrev, @ref )
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 +379,13 @@ describe GitPushService, services: true do ...@@ -376,7 +379,13 @@ 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) execute_service(project, user, @blankrev, @newrev, new_ref )
end end
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 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