Support merge request create with push options

To create a new merge request:

  git push -u origin -o merge_request.create

To create a new merge request setting target branch:

  git push -u origin -o merge_request.create \
    -o merge_request.target=123

To update an existing merge request with a new target branch:

  git push -u origin -o merge_request.target=123

A new Gitlab::PushOptions class handles parsing and validating the push
options array. This can be the start of the standard of GitLab accepting
push options that follow namespacing rules. Rules are discussed in issue
https://gitlab.com/gitlab-org/gitlab-ce/issues/43263.

E.g. these push options:

  -o merge_request.create -o merge_request.target=123

Become parsed as:

  {
    merge_request: {
      create: true,
      target: '123',
    }
  }

And are fetched with the class via:

  push_options.get(:merge_request)
  push_options.get(:merge_request, :create)
  push_options.get(:merge_request, :target)

A new MergeRequests::PushOptionsHandlerService takes the `merge_request`
namespaced push options and handles creating and updating
merge requests.

Any errors encountered are passed to the existing `output` Hash in
Api::Internal's `post_receive` endpoint, and passed to gitlab-shell
where they're output to the user.

Issue https://gitlab.com/gitlab-org/gitlab-ce/issues/43263

app/services/merge_requests/push_options_handler_service.rb

+# 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

changelogs/unreleased/43263-git-push-option-to-create-mr.yml

+---
+title: Allow merge requests to be created via git push options
+merge_request: 26752
+author:
+type: added

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.

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 }
 

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

lib/gitlab/push_options.rb

+# 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

spec/lib/gitlab/push_options_spec.rb

+# 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

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 ) }
 

spec/services/merge_requests/push_options_handler_service_spec.rb

+# 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