diff options
| -rw-r--r-- | GITALY_SERVER_VERSION | 2 | ||||
| -rw-r--r-- | Gemfile | 3 | ||||
| -rw-r--r-- | Gemfile.lock | 4 | ||||
| -rw-r--r-- | app/models/merge_request.rb | 14 | ||||
| -rw-r--r-- | app/models/project.rb | 8 | ||||
| -rw-r--r-- | app/models/repository.rb | 6 | ||||
| -rw-r--r-- | app/services/merge_requests/merge_base_service.rb | 53 | ||||
| -rw-r--r-- | app/services/merge_requests/merge_service.rb | 39 | ||||
| -rw-r--r-- | app/services/merge_requests/merge_to_ref_service.rb | 61 | ||||
| -rw-r--r-- | changelogs/unreleased/osw-create-and-store-merge-ref-for-mrs.yml | 5 | ||||
| -rw-r--r-- | lib/gitlab/git/repository.rb | 6 | ||||
| -rw-r--r-- | lib/gitlab/gitaly_client/operation_service.rb | 19 | ||||
| -rw-r--r-- | spec/lib/gitlab/git/repository_spec.rb | 31 | ||||
| -rw-r--r-- | spec/models/repository_spec.rb | 23 | ||||
| -rw-r--r-- | spec/services/merge_requests/merge_to_ref_service_spec.rb | 180 |
15 files changed, 413 insertions, 41 deletions
diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 39893559155..3500250a4b0 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -1.20.0 +1.21.0 @@ -419,7 +419,8 @@ group :ed25519 do end # Gitaly GRPC client -gem 'gitaly-proto', '~> 1.10.0', require: 'gitaly' +gem 'gitaly-proto', '~> 1.12.0', require: 'gitaly' + gem 'grpc', '~> 1.15.0' gem 'google-protobuf', '~> 3.6' diff --git a/Gemfile.lock b/Gemfile.lock index 841f85dc7e5..59e152c27fb 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -278,7 +278,7 @@ GEM gettext_i18n_rails (>= 0.7.1) po_to_json (>= 1.0.0) rails (>= 3.2.0) - gitaly-proto (1.10.0) + gitaly-proto (1.12.0) grpc (~> 1.0) github-markup (1.7.0) gitlab-default_value_for (3.1.1) @@ -1017,7 +1017,7 @@ DEPENDENCIES gettext (~> 3.2.2) gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails_js (~> 1.3) - gitaly-proto (~> 1.10.0) + gitaly-proto (~> 1.12.0) github-markup (~> 1.7.0) gitlab-default_value_for (~> 3.1.1) gitlab-markup (~> 1.6.5) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 75fca96ce0a..1468ae1c34a 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -764,6 +764,16 @@ class MergeRequest < ActiveRecord::Base true end + def mergeable_to_ref? + return false if merged? + return false if broken? + + # Given the `merge_ref_path` will have the same + # state the `target_branch` would have. Ideally + # we need to check if it can be merged to it. + project.repository.can_be_merged?(diff_head_sha, target_branch) + end + def ff_merge_possible? project.repository.ancestor?(target_branch_sha, diff_head_sha) end @@ -1077,6 +1087,10 @@ class MergeRequest < ActiveRecord::Base "refs/#{Repository::REF_MERGE_REQUEST}/#{iid}/head" end + def merge_ref_path + "refs/#{Repository::REF_MERGE_REQUEST}/#{iid}/merge" + end + def in_locked_state begin lock_mr diff --git a/app/models/project.rb b/app/models/project.rb index 83f8d004a46..889572e693a 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1925,6 +1925,14 @@ class Project < ActiveRecord::Base persisted? && path_changed? end + def human_merge_method + if merge_method == :ff + 'Fast-forward' + else + merge_method.to_s.humanize + end + end + def merge_method if self.merge_requests_ff_only_enabled :ff diff --git a/app/models/repository.rb b/app/models/repository.rb index ed55a6e572b..cd761a29618 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -854,6 +854,12 @@ class Repository end end + def merge_to_ref(user, source_sha, merge_request, target_ref, message) + branch = merge_request.target_branch + + raw.merge_to_ref(user, source_sha, branch, target_ref, message) + end + def ff_merge(user, source, target_branch, merge_request: nil) their_commit_id = commit(source)&.id raise 'Invalid merge source' if their_commit_id.nil? diff --git a/app/services/merge_requests/merge_base_service.rb b/app/services/merge_requests/merge_base_service.rb new file mode 100644 index 00000000000..4c5a0d96957 --- /dev/null +++ b/app/services/merge_requests/merge_base_service.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +module MergeRequests + class MergeBaseService < MergeRequests::BaseService + include Gitlab::Utils::StrongMemoize + + MergeError = Class.new(StandardError) + + attr_reader :merge_request + + # Overridden in EE. + def hooks_validation_pass?(_merge_request) + true + end + + # Overridden in EE. + def hooks_validation_error(_merge_request) + end + + def source + if merge_request.squash + squash_sha! + else + merge_request.diff_head_sha + end + end + + private + + def handle_merge_error(*args) + # No-op + end + + def commit_message + params[:commit_message] || + merge_request.default_merge_commit_message + end + + def squash_sha! + strong_memoize(:squash_sha) do + params[:merge_request] = merge_request + squash_result = ::MergeRequests::SquashService.new(project, current_user, params).execute + + case squash_result[:status] + when :success + squash_result[:squash_sha] + when :error + raise ::MergeRequests::MergeService::MergeError, squash_result[:message] + end + end + end + end +end diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index 449997bcf07..b1d01955d85 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -7,13 +7,7 @@ module MergeRequests # mark merge request as merged and execute all hooks and notifications # Executed when you do merge via GitLab UI # - class MergeService < MergeRequests::BaseService - include Gitlab::Utils::StrongMemoize - - MergeError = Class.new(StandardError) - - attr_reader :merge_request, :source - + class MergeService < MergeRequests::MergeBaseService delegate :merge_jid, :state, to: :@merge_request def execute(merge_request) @@ -38,19 +32,6 @@ module MergeRequests handle_merge_error(log_message: e.message, save_message_on_model: true) end - def source - if merge_request.squash - squash_sha! - else - merge_request.diff_head_sha - end - end - - # Overridden in EE. - def hooks_validation_pass?(_merge_request) - true - end - private def error_check! @@ -79,24 +60,8 @@ module MergeRequests merge_request.update!(merge_commit_sha: commit_id) end - def squash_sha! - strong_memoize(:squash_sha) do - params[:merge_request] = merge_request - squash_result = ::MergeRequests::SquashService.new(project, current_user, params).execute - - case squash_result[:status] - when :success - squash_result[:squash_sha] - when :error - raise ::MergeRequests::MergeService::MergeError, squash_result[:message] - end - end - end - def try_merge - message = params[:commit_message] || merge_request.default_merge_commit_message - - repository.merge(current_user, source, merge_request, message) + repository.merge(current_user, source, merge_request, commit_message) rescue Gitlab::Git::PreReceiveError => e handle_merge_error(log_message: e.message) raise MergeError, 'Something went wrong during merge pre-receive hook' diff --git a/app/services/merge_requests/merge_to_ref_service.rb b/app/services/merge_requests/merge_to_ref_service.rb new file mode 100644 index 00000000000..c4a40044143 --- /dev/null +++ b/app/services/merge_requests/merge_to_ref_service.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +module MergeRequests + # Performs the merge between source SHA and the target branch. Instead + # of writing the result to the MR target branch, it targets the `target_ref`. + # + # Ideally this should leave the `target_ref` state with the same state the + # target branch would have if we used the regular `MergeService`, but without + # every side-effect that comes with it (MR updates, mails, source branch + # deletion, etc). This service should be kept idempotent (i.e. can + # be executed regardless of the `target_ref` current state). + # + class MergeToRefService < MergeRequests::MergeBaseService + def execute(merge_request) + @merge_request = merge_request + + error_check! + + commit_id = commit + + raise MergeError, 'Conflicts detected during merge' unless commit_id + + success(commit_id: commit_id) + rescue MergeError => error + error(error.message) + end + + private + + def error_check! + error = + if !merge_method_supported? + "#{project.human_merge_method} to #{target_ref} is currently not supported." + elsif !hooks_validation_pass?(merge_request) + hooks_validation_error(merge_request) + elsif @merge_request.should_be_rebased? + 'Fast-forward merge is not possible. Please update your source branch.' + elsif !@merge_request.mergeable_to_ref? + "Merge request is not mergeable to #{target_ref}" + elsif !source + 'No source for merge' + end + + raise MergeError, error if error + end + + def target_ref + merge_request.merge_ref_path + end + + def commit + repository.merge_to_ref(current_user, source, merge_request, target_ref, commit_message) + rescue Gitlab::Git::PreReceiveError => error + raise MergeError, error.message + end + + def merge_method_supported? + [:merge, :rebase_merge].include?(project.merge_method) + end + end +end diff --git a/changelogs/unreleased/osw-create-and-store-merge-ref-for-mrs.yml b/changelogs/unreleased/osw-create-and-store-merge-ref-for-mrs.yml new file mode 100644 index 00000000000..012b547a630 --- /dev/null +++ b/changelogs/unreleased/osw-create-and-store-merge-ref-for-mrs.yml @@ -0,0 +1,5 @@ +--- +title: Support merge ref writing (without merging to target branch) +merge_request: 24692 +author: +type: added diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 593a3676519..aea132a3dd9 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -556,6 +556,12 @@ module Gitlab tags.find { |tag| tag.name == name } end + def merge_to_ref(user, source_sha, branch, target_ref, message) + wrapped_gitaly_errors do + gitaly_operation_client.user_merge_to_ref(user, source_sha, branch, target_ref, message) + end + end + def merge(user, source_sha, target_branch, message, &block) wrapped_gitaly_errors do gitaly_operation_client.user_merge_branch(user, source_sha, target_branch, message, &block) diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb index 22d2d149e65..d172c798da2 100644 --- a/lib/gitlab/gitaly_client/operation_service.rb +++ b/lib/gitlab/gitaly_client/operation_service.rb @@ -100,6 +100,25 @@ module Gitlab end end + def user_merge_to_ref(user, source_sha, branch, target_ref, message) + request = Gitaly::UserMergeToRefRequest.new( + repository: @gitaly_repo, + source_sha: source_sha, + branch: encode_binary(branch), + target_ref: encode_binary(target_ref), + user: Gitlab::Git::User.from_gitlab(user).to_gitaly, + message: message + ) + + response = GitalyClient.call(@repository.storage, :operation_service, :user_merge_to_ref, request) + + if pre_receive_error = response.pre_receive_error.presence + raise Gitlab::Git::PreReceiveError, pre_receive_error + end + + response.commit_id + end + def user_merge_branch(user, source_sha, target_branch, message) request_enum = QueueEnumerator.new response_enum = GitalyClient.call( diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 8a9e78ba3c3..b3a728c139e 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -1704,6 +1704,37 @@ describe Gitlab::Git::Repository, :seed_helper do end end + describe '#merge_to_ref' do + let(:repository) { mutable_repository } + let(:branch_head) { '6d394385cf567f80a8fd85055db1ab4c5295806f' } + let(:left_sha) { 'cfe32cf61b73a0d5e9f13e774abde7ff789b1660' } + let(:right_branch) { 'test-master' } + let(:target_ref) { 'refs/merge-requests/999/merge' } + + before do + repository.create_branch(right_branch, branch_head) unless repository.branch_exists?(right_branch) + end + + def merge_to_ref + repository.merge_to_ref(user, left_sha, right_branch, target_ref, 'Merge message') + end + + it 'generates a commit in the target_ref' do + expect(repository.ref_exists?(target_ref)).to be(false) + + commit_sha = merge_to_ref + ref_head = repository.commit(target_ref) + + expect(commit_sha).to be_present + expect(repository.ref_exists?(target_ref)).to be(true) + expect(ref_head.id).to eq(commit_sha) + end + + it 'does not change the right branch HEAD' do + expect { merge_to_ref }.not_to change { repository.find_branch(right_branch).target } + end + end + describe '#merge' do let(:repository) { mutable_repository } let(:source_sha) { '913c66a37b4a45b9769037c55c2d238bd0942d2e' } diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index f78760bf567..17201d8b90a 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1373,6 +1373,29 @@ describe Repository do end end + describe '#merge_to_ref' do + let(:merge_request) do + create(:merge_request, source_branch: 'feature', + target_branch: 'master', + source_project: project) + end + + it 'writes merge of source and target to MR merge_ref_path' do + merge_commit_id = repository.merge_to_ref(user, + merge_request.diff_head_sha, + merge_request, + merge_request.merge_ref_path, + 'Custom message') + + merge_commit = repository.commit(merge_commit_id) + + expect(merge_commit.message).to eq('Custom message') + expect(merge_commit.author_name).to eq(user.name) + expect(merge_commit.author_email).to eq(user.commit_email) + expect(repository.blob_at(merge_commit.id, 'files/ruby/feature.rb')).to be_present + end + end + describe '#ff_merge' do before do repository.add_branch(user, 'ff-target', 'feature~5') diff --git a/spec/services/merge_requests/merge_to_ref_service_spec.rb b/spec/services/merge_requests/merge_to_ref_service_spec.rb new file mode 100644 index 00000000000..435a863cbd4 --- /dev/null +++ b/spec/services/merge_requests/merge_to_ref_service_spec.rb @@ -0,0 +1,180 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe MergeRequests::MergeToRefService do + set(:user) { create(:user) } + let(:merge_request) { create(:merge_request, :simple) } + let(:project) { merge_request.project } + + before do + project.add_maintainer(user) + end + + describe '#execute' do + let(:service) do + described_class.new(project, user, + commit_message: 'Awesome message', + 'should_remove_source_branch' => true) + end + + def process_merge_to_ref + perform_enqueued_jobs do + service.execute(merge_request) + end + end + + it 'writes commit to merge ref' do + repository = project.repository + target_ref = merge_request.merge_ref_path + + expect(repository.ref_exists?(target_ref)).to be(false) + + result = service.execute(merge_request) + + ref_head = repository.commit(target_ref) + + expect(result[:status]).to eq(:success) + expect(result[:commit_id]).to be_present + expect(repository.ref_exists?(target_ref)).to be(true) + expect(ref_head.id).to eq(result[:commit_id]) + end + + it 'does not send any mail' do + expect { process_merge_to_ref }.not_to change { ActionMailer::Base.deliveries.count } + end + + it 'does not change the MR state' do + expect { process_merge_to_ref }.not_to change { merge_request.state } + end + + it 'does not create notes' do + expect { process_merge_to_ref }.not_to change { merge_request.notes.count } + end + + it 'does not delete the source branch' do + expect(DeleteBranchService).not_to receive(:new) + + process_merge_to_ref + end + + it 'returns an error when the failing to process the merge' do + allow(project.repository).to receive(:merge_to_ref).and_return(nil) + + result = service.execute(merge_request) + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq('Conflicts detected during merge') + end + + context 'commit history comparison with regular MergeService' do + let(:merge_ref_service) do + described_class.new(project, user, {}) + end + + let(:merge_service) do + MergeRequests::MergeService.new(project, user, {}) + end + + shared_examples_for 'MergeService for target ref' do + it 'target_ref has the same state of target branch' do + repo = merge_request.target_project.repository + + process_merge_to_ref + merge_service.execute(merge_request) + + ref_commits = repo.commits(merge_request.merge_ref_path, limit: 3) + target_branch_commits = repo.commits(merge_request.target_branch, limit: 3) + + ref_commits.zip(target_branch_commits).each do |ref_commit, target_branch_commit| + expect(ref_commit.parents).to eq(target_branch_commit.parents) + end + end + end + + context 'when merge commit' do + it_behaves_like 'MergeService for target ref' + end + + context 'when merge commit with squash' do + before do + merge_request.update!(squash: true, source_branch: 'master', target_branch: 'feature') + end + + it_behaves_like 'MergeService for target ref' + end + end + + context 'merge pre-condition checks' do + before do + merge_request.project.update!(merge_method: merge_method) + end + + context 'when semi-linear merge method' do + let(:merge_method) { :rebase_merge } + + it 'return error when MR should be able to fast-forward' do + allow(merge_request).to receive(:should_be_rebased?) { true } + + error_message = 'Fast-forward merge is not possible. Please update your source branch.' + + result = service.execute(merge_request) + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq(error_message) + end + end + + context 'when fast-forward merge method' do + let(:merge_method) { :ff } + + it 'returns error' do + error_message = "Fast-forward to #{merge_request.merge_ref_path} is currently not supported." + + result = service.execute(merge_request) + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq(error_message) + end + end + + context 'when MR is not mergeable to ref' do + let(:merge_method) { :merge } + + it 'returns error' do + allow(merge_request).to receive(:mergeable_to_ref?) { false } + + error_message = "Merge request is not mergeable to #{merge_request.merge_ref_path}" + + result = service.execute(merge_request) + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq(error_message) + end + end + end + + context 'does not close related todos' do + let(:merge_request) { create(:merge_request, assignee: user, author: user) } + let(:project) { merge_request.project } + let!(:todo) do + create(:todo, :assigned, + project: project, + author: user, + user: user, + target: merge_request) + end + + before do + allow(service).to receive(:execute_hooks) + + perform_enqueued_jobs do + service.execute(merge_request) + todo.reload + end + end + + it { expect(todo).not_to be_done } + end + end +end |