diff options
-rw-r--r-- | app/services/merge_requests/push_options_handler_service.rb | 127 | ||||
-rw-r--r-- | changelogs/unreleased/43263-git-push-option-to-create-mr.yml | 5 | ||||
-rw-r--r-- | doc/user/project/merge_requests/index.md | 33 | ||||
-rw-r--r-- | lib/api/helpers/internal_helpers.rb | 5 | ||||
-rw-r--r-- | lib/api/internal.rb | 36 | ||||
-rw-r--r-- | lib/gitlab/push_options.rb | 65 | ||||
-rw-r--r-- | spec/lib/gitlab/push_options_spec.rb | 91 | ||||
-rw-r--r-- | spec/requests/api/internal_spec.rb | 61 | ||||
-rw-r--r-- | spec/services/merge_requests/push_options_handler_service_spec.rb | 291 |
9 files changed, 707 insertions, 7 deletions
diff --git a/app/services/merge_requests/push_options_handler_service.rb b/app/services/merge_requests/push_options_handler_service.rb new file mode 100644 index 00000000000..c810340c636 --- /dev/null +++ b/app/services/merge_requests/push_options_handler_service.rb @@ -0,0 +1,127 @@ +# frozen_string_literal: true + +module MergeRequests + class PushOptionsHandlerService + Error = Class.new(StandardError) + + LIMIT = 10 + + attr_reader :branches, :changes_by_branch, :current_user, :errors, + :project, :push_options, :target + + def initialize(project, current_user, changes, push_options) + @project = project + @current_user = current_user + @changes_by_branch = parse_changes(changes) + @push_options = push_options + @errors = [] + @branches = @changes_by_branch.keys + @target = @push_options[:target] || @project.default_branch + + raise Error, 'User is required' if @current_user.nil? + + unless @project.merge_requests_enabled? + raise Error, 'Merge requests are not enabled for project' + end + + if @branches.size > LIMIT + raise Error, "Too many branches pushed (#{@branches.size} were pushed, limit is #{LIMIT})" + end + + if @push_options[:target] && !@project.repository.branch_exists?(@target) + raise Error, "Branch #{@target} does not exist" + end + end + + def execute + branches.each do |branch| + execute_for_branch(branch) + end + + self + end + + private + + # Parses changes in the push. + # Returns a hash of branch => changes_list + def parse_changes(raw_changes) + Gitlab::ChangesList.new(raw_changes).each_with_object({}) do |change, changes| + next unless Gitlab::Git.branch_ref?(change[:ref]) + + # Deleted branch + next if Gitlab::Git.blank_ref?(change[:newrev]) + + # Default branch + branch_name = Gitlab::Git.branch_name(change[:ref]) + next if branch_name == project.default_branch + + changes[branch_name] = change + end + end + + def merge_requests + @merge_requests ||= MergeRequest.from_project(@project) + .opened + .from_source_branches(@branches) + .to_a # fetch now + end + + def execute_for_branch(branch) + merge_request = merge_requests.find { |mr| mr.source_branch == branch } + + if merge_request + update!(merge_request) + else + create!(branch) + end + end + + def create!(branch) + unless push_options[:create] + errors << "A merge_request.create push option is required to create a merge request for branch #{branch}" + return + end + + merge_request = ::MergeRequests::CreateService.new( + project, + current_user, + create_params(branch) + ).execute + + collect_errors_from_merge_request(merge_request) unless merge_request.persisted? + end + + def update!(merge_request) + return if target == merge_request.target_branch + + merge_request = ::MergeRequests::UpdateService.new( + project, + current_user, + { target_branch: target } + ).execute(merge_request) + + collect_errors_from_merge_request(merge_request) unless merge_request.valid? + end + + def create_params(branch) + change = changes_by_branch.fetch(branch) + + commits = project.repository.commits_between(project.default_branch, change[:newrev]) + commits = CommitCollection.new(project, commits) + commit = commits.without_merge_commits.first + + { + assignee: current_user, + source_branch: branch, + target_branch: target, + title: commit&.title&.strip || 'New Merge Request', + description: commit&.description&.strip + } + end + + def collect_errors_from_merge_request(merge_request) + errors << merge_request.errors.full_messages.to_sentence + end + end +end diff --git a/changelogs/unreleased/43263-git-push-option-to-create-mr.yml b/changelogs/unreleased/43263-git-push-option-to-create-mr.yml new file mode 100644 index 00000000000..d50c33da162 --- /dev/null +++ b/changelogs/unreleased/43263-git-push-option-to-create-mr.yml @@ -0,0 +1,5 @@ +--- +title: Allow merge requests to be created via git push options +merge_request: 26752 +author: +type: added diff --git a/doc/user/project/merge_requests/index.md b/doc/user/project/merge_requests/index.md index 7c0380152de..678fc3dd196 100644 --- a/doc/user/project/merge_requests/index.md +++ b/doc/user/project/merge_requests/index.md @@ -219,6 +219,39 @@ apply the patches. The target branch can be specified using the [`/target_branch` quick action](../quick_actions.md). If the source branch already exists, the patches will be applied on top of it. +## Git push options + +> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/26752) in GitLab 11.10. + +GitLab supports using [Git push options](https://git-scm.com/docs/git-push#Documentation/git-push.txt--oltoptiongt) to create merge requests and set the target +branch during a push. Note that git push options are only available with +Git 2.10 or newer. + +### Create a new merge request using git push options + +To create a new merge request for a branch, use the +`merge_request.create` push option: + +```sh +git push -o merge_request.create +``` + +### Set the target branch of a merge request using git push options + +To update an existing merge request's target branch, use the +`merge_request.target=<branch_name>` push option: + +```sh +git push -o merge_request.target=branch_name +``` + +You can also create a merge request and set its target branch at the +same time using a `-o` flag per push option: + +```sh +git push -o merge_request.create -o merge_request.target=branch_name +``` + ## Find the merge request that introduced a change > [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/2383) in GitLab 10.5. diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index 3fd824877ae..5014ba51b94 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -43,6 +43,11 @@ module API ::MergeRequests::GetUrlsService.new(project).execute(params[:changes]) end + def push_options_warning(warning) + options = Array.wrap(params[:push_options]).map { |p| "'#{p}'" }.join(' ') + "Error encountered with push options #{options}: #{warning}" + end + def redis_ping result = Gitlab::Redis::SharedState.with { |redis| redis.ping } diff --git a/lib/api/internal.rb b/lib/api/internal.rb index 9c7b9146c8f..75202fa953c 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -256,19 +256,41 @@ module API post '/post_receive' do status 200 + output = {} # Messages to gitlab-shell + user = identify(params[:identifier]) + project = Gitlab::GlRepository.parse(params[:gl_repository]).first + push_options = Gitlab::PushOptions.new(params[:push_options]) + PostReceive.perform_async(params[:gl_repository], params[:identifier], params[:changes], params[:push_options].to_a) + + if (mr_options = push_options.get(:merge_request)) + begin + service = ::MergeRequests::PushOptionsHandlerService.new( + project, + user, + params[:changes], + mr_options + ).execute + + if service.errors.present? + output[:warnings] = push_options_warning(service.errors.join("\n\n")) + end + rescue ::MergeRequests::PushOptionsHandlerService::Error => e + output[:warnings] = push_options_warning(e.message) + rescue Gitlab::Access::AccessDeniedError + output[:warnings] = push_options_warning('User access was denied') + end + end + broadcast_message = BroadcastMessage.current&.last&.message reference_counter_decreased = Gitlab::ReferenceCounter.new(params[:gl_repository]).decrease - output = { - merge_request_urls: merge_request_urls, + output.merge!( broadcast_message: broadcast_message, - reference_counter_decreased: reference_counter_decreased - } - - project = Gitlab::GlRepository.parse(params[:gl_repository]).first - user = identify(params[:identifier]) + reference_counter_decreased: reference_counter_decreased, + merge_request_urls: merge_request_urls + ) # A user is not guaranteed to be returned; an orphaned write deploy # key could be used diff --git a/lib/gitlab/push_options.rb b/lib/gitlab/push_options.rb new file mode 100644 index 00000000000..923aa09527d --- /dev/null +++ b/lib/gitlab/push_options.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +module Gitlab + class PushOptions + VALID_OPTIONS = HashWithIndifferentAccess.new({ + merge_request: { + keys: [:create, :target] + }, + ci: { + keys: [:skip] + } + }).freeze + + NAMESPACE_ALIASES = HashWithIndifferentAccess.new({ + mr: :merge_request + }).freeze + + OPTION_MATCHER = /(?<namespace>[^\.]+)\.(?<key>[^=]+)=?(?<value>.*)/ + + attr_reader :options + + def initialize(options = []) + @options = parse_options(options) + end + + def get(*args) + options.dig(*args) + end + + private + + def parse_options(raw_options) + options = HashWithIndifferentAccess.new + + Array.wrap(raw_options).each do |option| + namespace, key, value = parse_option(option) + + next if [namespace, key].any?(&:nil?) + + options[namespace] ||= HashWithIndifferentAccess.new + options[namespace][key] = value + end + + options + end + + def parse_option(option) + parts = OPTION_MATCHER.match(option) + return unless parts + + namespace, key, value = parts.values_at(:namespace, :key, :value).map(&:strip) + namespace = NAMESPACE_ALIASES[namespace] if NAMESPACE_ALIASES[namespace] + value = value.presence || true + + return unless valid_option?(namespace, key) + + [namespace, key, value] + end + + def valid_option?(namespace, key) + keys = VALID_OPTIONS.dig(namespace, :keys) + keys && keys.include?(key.to_sym) + end + end +end diff --git a/spec/lib/gitlab/push_options_spec.rb b/spec/lib/gitlab/push_options_spec.rb new file mode 100644 index 00000000000..68b64863c21 --- /dev/null +++ b/spec/lib/gitlab/push_options_spec.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::PushOptions do + describe 'namespace and key validation' do + it 'ignores unrecognised namespaces' do + options = described_class.new(['invalid.key=value']) + + expect(options.get(:invalid)).to eq(nil) + end + + it 'ignores unrecognised keys' do + options = described_class.new(['merge_request.key=value']) + + expect(options.get(:merge_request)).to eq(nil) + end + + it 'ignores blank keys' do + options = described_class.new(['merge_request']) + + expect(options.get(:merge_request)).to eq(nil) + end + + it 'parses recognised namespace and key pairs' do + options = described_class.new(['merge_request.target=value']) + + expect(options.get(:merge_request)).to include({ + target: 'value' + }) + end + end + + describe '#get' do + it 'can emulate Hash#dig' do + options = described_class.new(['merge_request.target=value']) + + expect(options.get(:merge_request, :target)).to eq('value') + end + end + + it 'can parse multiple push options' do + options = described_class.new([ + 'merge_request.create', + 'merge_request.target=value' + ]) + + expect(options.get(:merge_request)).to include({ + create: true, + target: 'value' + }) + expect(options.get(:merge_request, :create)).to eq(true) + expect(options.get(:merge_request, :target)).to eq('value') + end + + it 'stores options internally as a HashWithIndifferentAccess' do + options = described_class.new([ + 'merge_request.create' + ]) + + expect(options.get('merge_request', 'create')).to eq(true) + expect(options.get(:merge_request, :create)).to eq(true) + end + + it 'selects the last option when options contain duplicate namespace and key pairs' do + options = described_class.new([ + 'merge_request.target=value1', + 'merge_request.target=value2' + ]) + + expect(options.get(:merge_request, :target)).to eq('value2') + end + + it 'defaults values to true' do + options = described_class.new(['merge_request.create']) + + expect(options.get(:merge_request, :create)).to eq(true) + end + + it 'expands aliases' do + options = described_class.new(['mr.target=value']) + + expect(options.get(:merge_request, :target)).to eq('value') + end + + it 'forgives broken push options' do + options = described_class.new(['merge_request . target = value']) + + expect(options.get(:merge_request, :target)).to eq('value') + end +end diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index 0919540e4ba..62ba4281df5 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -938,6 +938,67 @@ describe API::Internal do expect(json_response['merge_request_urls']).to eq([]) end + it 'does not invoke MergeRequests::PushOptionsHandlerService' do + expect(MergeRequests::PushOptionsHandlerService).not_to receive(:new) + + post api("/internal/post_receive"), params: valid_params + end + + context 'when there are merge_request push options' do + before do + valid_params[:push_options] = ['merge_request.create'] + end + + it 'invokes MergeRequests::PushOptionsHandlerService' do + expect(MergeRequests::PushOptionsHandlerService).to receive(:new) + + post api("/internal/post_receive"), params: valid_params + end + + it 'links to the newly created merge request' do + post api("/internal/post_receive"), params: valid_params + + expect(json_response['merge_request_urls']).to match [{ + "branch_name" => "new_branch", + "url" => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/1", + "new_merge_request" => false + }] + end + + it 'adds errors raised from MergeRequests::PushOptionsHandlerService to warnings' do + expect(MergeRequests::PushOptionsHandlerService).to receive(:new).and_raise( + MergeRequests::PushOptionsHandlerService::Error, 'my warning' + ) + + post api("/internal/post_receive"), params: valid_params + + expect(json_response['warnings']).to eq('Error encountered with push options \'merge_request.create\': my warning') + end + + it 'adds errors on the service instance to warnings' do + expect_any_instance_of( + MergeRequests::PushOptionsHandlerService + ).to receive(:errors).at_least(:once).and_return(['my error']) + + post api("/internal/post_receive"), params: valid_params + + expect(json_response['warnings']).to eq('Error encountered with push options \'merge_request.create\': my error') + end + + it 'adds ActiveRecord errors on invalid MergeRequest records to warnings' do + invalid_merge_request = MergeRequest.new + invalid_merge_request.errors.add(:base, 'my error') + + expect_any_instance_of( + MergeRequests::CreateService + ).to receive(:execute).and_return(invalid_merge_request) + + post api("/internal/post_receive"), params: valid_params + + expect(json_response['warnings']).to eq('Error encountered with push options \'merge_request.create\': my error') + end + end + context 'broadcast message exists' do let!(:broadcast_message) { create(:broadcast_message, starts_at: 1.day.ago, ends_at: 1.day.from_now ) } diff --git a/spec/services/merge_requests/push_options_handler_service_spec.rb b/spec/services/merge_requests/push_options_handler_service_spec.rb new file mode 100644 index 00000000000..96becfcae26 --- /dev/null +++ b/spec/services/merge_requests/push_options_handler_service_spec.rb @@ -0,0 +1,291 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe MergeRequests::PushOptionsHandlerService do + let(:user) { create(:user) } + let(:project) { create(:project, :public, :repository) } + let(:service) { described_class.new(project, user, changes, push_options) } + let(:source_branch) { 'test' } + let(:target_branch) { 'feature' } + let(:new_branch_changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" } + let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" } + let(:deleted_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 #{Gitlab::Git::BLANK_SHA} refs/heads/#{source_branch}" } + let(:default_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{project.default_branch}" } + + before do + project.add_developer(user) + end + + shared_examples_for 'a service that can create a merge request' do + subject(:last_mr) { MergeRequest.last } + + it 'creates a merge request' do + expect { service.execute }.to change { MergeRequest.count }.by(1) + end + + it 'sets the correct target branch' do + branch = push_options[:target] || project.default_branch + + service.execute + + expect(last_mr.target_branch).to eq(branch) + end + + it 'assigns the MR to the user' do + service.execute + + expect(last_mr.assignee).to eq(user) + end + + it 'sets the title and description from the first non-merge commit' do + commits = project.repository.commits('master', limit: 5) + + expect(Gitlab::Git::Commit).to receive(:between).at_least(:once).and_return(commits) + + service.execute + + merge_commit = commits.first + non_merge_commit = commits.second + + expect(merge_commit.merge_commit?).to eq(true) + expect(non_merge_commit.merge_commit?).to eq(false) + + expect(last_mr.title).to eq(non_merge_commit.title) + expect(last_mr.description).to eq(non_merge_commit.description) + end + end + + shared_examples_for 'a service that can set the target of a merge request' do + subject(:last_mr) { MergeRequest.last } + + it 'sets the target_branch' do + service.execute + + expect(last_mr.target_branch).to eq(target_branch) + end + end + + shared_examples_for 'a service that does not create a merge request' do + it do + expect { service.execute }.not_to change { MergeRequest.count } + end + end + + shared_examples_for 'a service that does not update a merge request' do + it do + expect { service.execute }.not_to change { MergeRequest.maximum(:updated_at) } + end + end + + shared_examples_for 'a service that does nothing' do + include_examples 'a service that does not create a merge request' + include_examples 'a service that does not update a merge request' + end + + describe '`create` push option' do + let(:push_options) { { create: true } } + + context 'with a new branch' do + let(:changes) { new_branch_changes } + + it_behaves_like 'a service that can create a merge request' + end + + context 'with an existing branch but no open MR' do + let(:changes) { existing_branch_changes } + + it_behaves_like 'a service that can create a merge request' + end + + context 'with an existing branch that has a merge request open' do + let(:changes) { existing_branch_changes } + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch)} + + it_behaves_like 'a service that does not create a merge request' + end + + context 'with a deleted branch' do + let(:changes) { deleted_branch_changes } + + it_behaves_like 'a service that does nothing' + end + + context 'with the project default branch' do + let(:changes) { default_branch_changes } + + it_behaves_like 'a service that does nothing' + end + end + + describe '`target` push option' do + let(:push_options) { { target: target_branch } } + + context 'with a new branch' do + let(:changes) { new_branch_changes } + + it_behaves_like 'a service that does not create a merge request' + + it 'adds an error to the service' do + error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" + + service.execute + + expect(service.errors).to include(error) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, target: target_branch } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can set the target of a merge request' + end + end + + context 'with an existing branch but no open MR' do + let(:changes) { existing_branch_changes } + + it_behaves_like 'a service that does not create a merge request' + + it 'adds an error to the service' do + error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" + + service.execute + + expect(service.errors).to include(error) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, target: target_branch } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can set the target of a merge request' + end + end + + context 'with an existing branch that has a merge request open' do + let(:changes) { existing_branch_changes } + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch)} + + it_behaves_like 'a service that does not create a merge request' + it_behaves_like 'a service that can set the target of a merge request' + end + + context 'with a deleted branch' do + let(:changes) { deleted_branch_changes } + + it_behaves_like 'a service that does nothing' + end + + context 'with the project default branch' do + let(:changes) { default_branch_changes } + + it_behaves_like 'a service that does nothing' + end + end + + describe 'multiple pushed branches' do + let(:push_options) { { create: true } } + let(:changes) do + [ + new_branch_changes, + "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/second-branch" + ] + end + + it 'creates a merge request per branch' do + expect { service.execute }.to change { MergeRequest.count }.by(2) + end + + context 'when there are too many pushed branches' do + let(:limit) { MergeRequests::PushOptionsHandlerService::LIMIT } + let(:changes) do + TestEnv::BRANCH_SHA.to_a[0..limit].map do |x| + "#{Gitlab::Git::BLANK_SHA} #{x.first} refs/heads/#{x.last}" + end + end + + it 'throws an error' do + expect { service.execute }.to raise_error( + MergeRequests::PushOptionsHandlerService::Error, + "Too many branches pushed (#{limit + 1} were pushed, limit is #{limit})" + ) + end + end + end + + describe 'no push options' do + let(:push_options) { {} } + let(:changes) { new_branch_changes } + + it_behaves_like 'a service that does nothing' + end + + describe 'no user' do + let(:user) { nil } + let(:push_options) { { create: true } } + let(:changes) { new_branch_changes } + + it 'throws an error' do + expect { service.execute }.to raise_error( + MergeRequests::PushOptionsHandlerService::Error, + 'User is required' + ) + end + end + + describe 'unauthorized user' do + let(:push_options) { { create: true } } + let(:changes) { new_branch_changes } + + it 'throws an error' do + Members::DestroyService.new(user).execute(ProjectMember.find_by!(user_id: user.id)) + + expect { service.execute }.to raise_error(Gitlab::Access::AccessDeniedError) + end + end + + describe 'when target is not a valid branch name' do + let(:push_options) { { create: true, target: 'my-branch' } } + let(:changes) { new_branch_changes } + + it 'throws an error' do + expect { service.execute }.to raise_error( + MergeRequests::PushOptionsHandlerService::Error, + 'Branch my-branch does not exist' + ) + end + end + + describe 'when MRs are not enabled' do + let(:push_options) { { create: true } } + let(:changes) { new_branch_changes } + + it 'throws an error' do + expect(project).to receive(:merge_requests_enabled?).and_return(false) + + expect { service.execute }.to raise_error( + MergeRequests::PushOptionsHandlerService::Error, + 'Merge requests are not enabled for project' + ) + end + end + + describe 'when MR has ActiveRecord errors' do + let(:push_options) { { create: true } } + let(:changes) { new_branch_changes } + + it 'adds the error to its errors property' do + invalid_merge_request = MergeRequest.new + invalid_merge_request.errors.add(:base, 'my error') + + expect_any_instance_of( + MergeRequests::CreateService + ).to receive(:execute).and_return(invalid_merge_request) + + service.execute + + expect(service.errors).to eq(['my error']) + end + end +end |