From f00db0c342d01b33617f269447ff76140944a86e Mon Sep 17 00:00:00 2001
From: Christian Couder <chriscool@tuxfamily.org>
Date: Thu, 15 Aug 2019 05:43:33 +0200
Subject: [PATCH 1/3] Support adding and removing labels w/ push opts

MergeRequests::PushOptionsHandlerService has been updated to allow
adding and removing labels to a merge request using git push options.

To create a new merge request and add 2 labels to it:

      git push -u origin -o merge_request.create \
        -o merge_request.label="My label 1" \
        -o merge_request.label="My label 2"

To update an existing merge request and remove a label while
adding a different label:

      git push -u origin -o merge_request.label="My added label" \
        -o merge_request.unlabel="My removed label"

Issue https://gitlab.com/gitlab-org/gitlab-ce/issues/64320
---
 app/services/issuable_base_service.rb         |  13 +-
 .../labels/available_labels_service.rb        |   4 +-
 app/services/merge_requests/build_service.rb  |  12 ++
 .../push_options_handler_service.rb           |  10 +-
 changelogs/unreleased/add-label-push-opts.yml |   5 +
 doc/user/project/merge_requests/index.md      |  28 ++++
 lib/gitlab/push_options.rb                    |  21 ++-
 .../push_options_handler_service_spec.rb      | 145 ++++++++++++++++++
 8 files changed, 230 insertions(+), 8 deletions(-)
 create mode 100644 changelogs/unreleased/add-label-push-opts.yml

diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 2ab6e88599f..71c86106f48 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -76,8 +76,17 @@ class IssuableBaseService < BaseService
   end
 
   def filter_labels
-    params[:add_label_ids] = labels_service.filter_labels_ids_in_param(:add_label_ids) if params[:add_label_ids]
-    params[:remove_label_ids] = labels_service.filter_labels_ids_in_param(:remove_label_ids) if params[:remove_label_ids]
+    if params[:add_label_ids]
+      params[:add_label_ids] = labels_service.filter_labels_ids_in_param(:add_label_ids)
+    elsif params[:add_labels]
+      params[:add_label_ids] = labels_service.find_or_create_by_titles(:add_labels).map(&:id)
+    end
+
+    if params[:remove_label_ids]
+      params[:remove_label_ids] = labels_service.filter_labels_ids_in_param(:remove_label_ids)
+    elsif params[:remove_labels]
+      params[:remove_label_ids] = labels_service.find_or_create_by_titles(:remove_labels).map(&:id)
+    end
 
     if params[:label_ids]
       params[:label_ids] = labels_service.filter_labels_ids_in_param(:label_ids)
diff --git a/app/services/labels/available_labels_service.rb b/app/services/labels/available_labels_service.rb
index fe477d96970..bdd5f660576 100644
--- a/app/services/labels/available_labels_service.rb
+++ b/app/services/labels/available_labels_service.rb
@@ -9,8 +9,8 @@ module Labels
       @params = params
     end
 
-    def find_or_create_by_titles
-      labels = params.delete(:labels)
+    def find_or_create_by_titles(key = :labels)
+      labels = params.delete(key)
 
       return [] unless labels
 
diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb
index b28f80939ae..308a3a10d1a 100644
--- a/app/services/merge_requests/build_service.rb
+++ b/app/services/merge_requests/build_service.rb
@@ -18,6 +18,18 @@ module MergeRequests
       merge_request.target_project = find_target_project
 
       filter_params(merge_request)
+
+      # merge_request.assign_attributes(...) below is a Rails
+      # method that only work if all the params it is passed have
+      # corresponding fields in the database. As there are no fields
+      # in the database for :add_label_ids and :remove_label_ids, we
+      # need to remove them from the params before the call to
+      # merge_request.assign_attributes(...)
+      #
+      # IssuableBaseService#process_label_ids takes care
+      # of the removal.
+      params[:label_ids] = process_label_ids(params, extra_label_ids: merge_request.label_ids.to_a)
+
       merge_request.assign_attributes(params.to_h.compact)
 
       merge_request.compare_commits = []
diff --git a/app/services/merge_requests/push_options_handler_service.rb b/app/services/merge_requests/push_options_handler_service.rb
index b210004e6e1..0168b31005e 100644
--- a/app/services/merge_requests/push_options_handler_service.rb
+++ b/app/services/merge_requests/push_options_handler_service.rb
@@ -100,7 +100,8 @@ module MergeRequests
         merge_request = ::MergeRequests::CreateService.new(
           project,
           current_user,
-          merge_request.attributes.merge(assignees: merge_request.assignees)
+          merge_request.attributes.merge(assignees: merge_request.assignees,
+                                        label_ids: merge_request.label_ids)
         ).execute
       end
 
@@ -122,7 +123,9 @@ module MergeRequests
         title: push_options[:title],
         description: push_options[:description],
         target_branch: push_options[:target],
-        force_remove_source_branch: push_options[:remove_source_branch]
+        force_remove_source_branch: push_options[:remove_source_branch],
+        label: push_options[:label],
+        unlabel: push_options[:unlabel]
       }
 
       params.compact!
@@ -134,6 +137,9 @@ module MergeRequests
         )
       end
 
+      params[:add_labels] = params.delete(:label).keys if params.has_key?(:label)
+      params[:remove_labels] = params.delete(:unlabel).keys if params.has_key?(:unlabel)
+
       params
     end
 
diff --git a/changelogs/unreleased/add-label-push-opts.yml b/changelogs/unreleased/add-label-push-opts.yml
new file mode 100644
index 00000000000..1289020e4e5
--- /dev/null
+++ b/changelogs/unreleased/add-label-push-opts.yml
@@ -0,0 +1,5 @@
+---
+title: Support adding and removing labels w/ push opts
+merge_request: 31831
+author:
+type: added
diff --git a/doc/user/project/merge_requests/index.md b/doc/user/project/merge_requests/index.md
index d6da8cb99c7..9f31f38460a 100644
--- a/doc/user/project/merge_requests/index.md
+++ b/doc/user/project/merge_requests/index.md
@@ -289,6 +289,7 @@ as pushing changes:
 - Set the merge request to remove the source branch when it's merged.
 - Set the title of the merge request to a particular title.
 - Set the description of the merge request to a particular description.
+- Add or remove labels from the merge request.
 
 ### Create a new merge request using git push options
 
@@ -375,6 +376,33 @@ git push -o merge_request.description="The description I want"
 You can also use this push option in addition to the
 `merge_request.create` push option.
 
+### Add or remove labels using git push options
+
+You can add or remove labels from merge requests using push options.
+
+For example, to add two labels to an existing merge request, use the
+`merge_request.label` push option:
+
+```sh
+git push -o merge_request.label="label1" -o merge_request.label="label2"
+```
+
+To remove two labels from an existing merge request, use
+the `merge_request.unlabel` push option:
+
+```sh
+git push -o merge_request.unlabel="label1" -o merge_request.unlabel="label2"
+```
+
+You can also use these push options in addition to the
+`merge_request.create` push option.
+
+To create a merge request and add two labels to it, use:
+
+```sh
+git push -o merge_request.create -o merge_request.label="label1" -o merge_request.label="label2"
+```
+
 ## 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/gitlab/push_options.rb b/lib/gitlab/push_options.rb
index 682edfc4259..a2296d265cd 100644
--- a/lib/gitlab/push_options.rb
+++ b/lib/gitlab/push_options.rb
@@ -7,10 +7,12 @@ module Gitlab
         keys: [
           :create,
           :description,
+          :label,
           :merge_when_pipeline_succeeds,
           :remove_source_branch,
           :target,
-          :title
+          :title,
+          :unlabel
         ]
       },
       ci: {
@@ -18,6 +20,11 @@ module Gitlab
       }
     }).freeze
 
+    MULTI_VALUE_OPTIONS = [
+      %w[merge_request label],
+      %w[merge_request unlabel]
+    ].freeze
+
     NAMESPACE_ALIASES = HashWithIndifferentAccess.new({
       mr: :merge_request
     }).freeze
@@ -50,12 +57,22 @@ module Gitlab
         next if [namespace, key].any?(&:nil?)
 
         options[namespace] ||= HashWithIndifferentAccess.new
-        options[namespace][key] = value
+
+        if option_multi_value?(namespace, key)
+          options[namespace][key] ||= HashWithIndifferentAccess.new(0)
+          options[namespace][key][value] += 1
+        else
+          options[namespace][key] = value
+        end
       end
 
       options
     end
 
+    def option_multi_value?(namespace, key)
+      MULTI_VALUE_OPTIONS.any? { |arr| arr == [namespace, key] }
+    end
+
     def parse_option(option)
       parts = OPTION_MATCHER.match(option)
       return unless parts
diff --git a/spec/services/merge_requests/push_options_handler_service_spec.rb b/spec/services/merge_requests/push_options_handler_service_spec.rb
index a27fea0c90f..ff4cdd3e7e2 100644
--- a/spec/services/merge_requests/push_options_handler_service_spec.rb
+++ b/spec/services/merge_requests/push_options_handler_service_spec.rb
@@ -13,6 +13,9 @@ describe MergeRequests::PushOptionsHandlerService do
   let(:target_branch) { 'feature' }
   let(:title) { 'my title' }
   let(:description) { 'my description' }
+  let(:label1) { 'mylabel1' }
+  let(:label2) { 'mylabel2' }
+  let(:label3) { 'mylabel3' }
   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}" }
@@ -122,6 +125,16 @@ describe MergeRequests::PushOptionsHandlerService do
     end
   end
 
+  shared_examples_for 'a service that can change labels of a merge request' do |count|
+    subject(:last_mr) { MergeRequest.last }
+
+    it 'changes label count' do
+      service.execute
+
+      expect(last_mr.label_ids.count).to eq(count)
+    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 }
@@ -504,6 +517,138 @@ describe MergeRequests::PushOptionsHandlerService do
     end
   end
 
+  describe '`label` push option' do
+    let(:push_options) { { label: { label1 => 1, label2 => 1 } } }
+
+    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, label: { label1 => 1, label2 => 1 } } }
+
+        it_behaves_like 'a service that can create a merge request'
+        it_behaves_like 'a service that can change labels of a merge request', 2
+      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, label: { label1 => 1, label2 => 1 } } }
+
+        it_behaves_like 'a service that can create a merge request'
+        it_behaves_like 'a service that can change labels of a merge request', 2
+      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 change labels of a merge request', 2
+    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 '`unlabel` push option' do
+    let(:push_options) { { label: { label1 => 1, label2 => 1 }, unlabel: { label1 => 1, label3 => 1 } } }
+
+    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, label: { label1 => 1, label2 => 1 }, unlabel: { label1 => 1, label3 => 1 } } }
+
+        it_behaves_like 'a service that can create a merge request'
+        it_behaves_like 'a service that can change labels of a merge request', 1
+      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, label: { label1 => 1, label2 => 1 }, unlabel: { label1 => 1, label3 => 1 } } }
+
+        it_behaves_like 'a service that can create a merge request'
+        it_behaves_like 'a service that can change labels of a merge request', 1
+      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 change labels of a merge request', 1
+    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

From 760d4a16214b2ae77c4c0ea9c719815e9473dcd9 Mon Sep 17 00:00:00 2001
From: Christian Couder <chriscool@tuxfamily.org>
Date: Fri, 30 Aug 2019 09:51:45 +0200
Subject: [PATCH 2/3] Avoid creating labels when removing them

IssuableBaseService has been updated so that labels are not
created when push options to remove them are received.
---
 app/services/issuable_base_service.rb           | 2 +-
 app/services/labels/available_labels_service.rb | 4 ++--
 app/services/labels/find_or_create_service.rb   | 8 +++++---
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 71c86106f48..d1803602cb1 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -85,7 +85,7 @@ class IssuableBaseService < BaseService
     if params[:remove_label_ids]
       params[:remove_label_ids] = labels_service.filter_labels_ids_in_param(:remove_label_ids)
     elsif params[:remove_labels]
-      params[:remove_label_ids] = labels_service.find_or_create_by_titles(:remove_labels).map(&:id)
+      params[:remove_label_ids] = labels_service.find_or_create_by_titles(:remove_labels, find_only: true).map(&:id)
     end
 
     if params[:label_ids]
diff --git a/app/services/labels/available_labels_service.rb b/app/services/labels/available_labels_service.rb
index bdd5f660576..8886e58d6ef 100644
--- a/app/services/labels/available_labels_service.rb
+++ b/app/services/labels/available_labels_service.rb
@@ -9,7 +9,7 @@ module Labels
       @params = params
     end
 
-    def find_or_create_by_titles(key = :labels)
+    def find_or_create_by_titles(key = :labels, find_only: false)
       labels = params.delete(key)
 
       return [] unless labels
@@ -23,7 +23,7 @@ module Labels
           include_ancestor_groups: true,
           title: label_name.strip,
           available_labels: available_labels
-        ).execute
+        ).execute(find_only: find_only)
 
         label
       end.compact
diff --git a/app/services/labels/find_or_create_service.rb b/app/services/labels/find_or_create_service.rb
index 628873519d7..a47dd42aea0 100644
--- a/app/services/labels/find_or_create_service.rb
+++ b/app/services/labels/find_or_create_service.rb
@@ -9,9 +9,9 @@ module Labels
       @params = params.dup.with_indifferent_access
     end
 
-    def execute(skip_authorization: false)
+    def execute(skip_authorization: false, find_only: false)
       @skip_authorization = skip_authorization
-      find_or_create_label
+      find_or_create_label(find_only: find_only)
     end
 
     private
@@ -30,9 +30,11 @@ module Labels
     # Only creates the label if current_user can do so, if the label does not exist
     # and the user can not create the label, nil is returned
     # rubocop: disable CodeReuse/ActiveRecord
-    def find_or_create_label
+    def find_or_create_label(find_only: false)
       new_label = available_labels.find_by(title: title)
 
+      return new_label if find_only
+
       if new_label.nil? && (skip_authorization || Ability.allowed?(current_user, :admin_label, parent))
         create_params = params.except(:include_ancestor_groups)
         new_label = Labels::CreateService.new(create_params).execute(parent_type.to_sym => parent)

From 582e075e9f51567a0f004e5b34ca099b09a72059 Mon Sep 17 00:00:00 2001
From: Christian Couder <chriscool@tuxfamily.org>
Date: Fri, 30 Aug 2019 21:28:19 +0200
Subject: [PATCH 3/3] Simplify filter_labels method in IssuableBaseService

IssuableBaseService::filter_labels() has been refactored to call
a new `label_ids_to_filter` method.
---
 app/services/issuable_base_service.rb | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index d1803602cb1..c460ee4e7bc 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -76,22 +76,16 @@ class IssuableBaseService < BaseService
   end
 
   def filter_labels
-    if params[:add_label_ids]
-      params[:add_label_ids] = labels_service.filter_labels_ids_in_param(:add_label_ids)
-    elsif params[:add_labels]
-      params[:add_label_ids] = labels_service.find_or_create_by_titles(:add_labels).map(&:id)
-    end
+    label_ids_to_filter(:add_label_ids, :add_labels, false)
+    label_ids_to_filter(:remove_label_ids, :remove_labels, true)
+    label_ids_to_filter(:label_ids, :labels, false)
+  end
 
-    if params[:remove_label_ids]
-      params[:remove_label_ids] = labels_service.filter_labels_ids_in_param(:remove_label_ids)
-    elsif params[:remove_labels]
-      params[:remove_label_ids] = labels_service.find_or_create_by_titles(:remove_labels, find_only: true).map(&:id)
-    end
-
-    if params[:label_ids]
-      params[:label_ids] = labels_service.filter_labels_ids_in_param(:label_ids)
-    elsif params[:labels]
-      params[:label_ids] = labels_service.find_or_create_by_titles.map(&:id)
+  def label_ids_to_filter(label_id_key, label_key, find_only)
+    if params[label_id_key]
+      params[label_id_key] = labels_service.filter_labels_ids_in_param(label_id_key)
+    elsif params[label_key]
+      params[label_id_key] = labels_service.find_or_create_by_titles(label_key, find_only: find_only).map(&:id)
     end
   end