Automatic merge of gitlab-org/gitlab-ce master

app/controllers/admin/requests_profiles_controller.rb

@@ -3,12 +3,12 @@
 class Admin::RequestsProfilesController < Admin::ApplicationController
   def index
     @profile_token = Gitlab::RequestProfiler.profile_token
-    @profiles      = Gitlab::RequestProfiler::Profile.all.group_by(&:request_path)
+    @profiles      = Gitlab::RequestProfiler.all.group_by(&:request_path)
   end
 
   def show
     clean_name = Rack::Utils.clean_path_info(params[:name])
-    profile    = Gitlab::RequestProfiler::Profile.find(clean_name)
+    profile    = Gitlab::RequestProfiler.find(clean_name)
 
     unless profile && profile.content_type
       return redirect_to admin_requests_profiles_path, alert: 'Profile not found'

app/services/merge_requests/push_options_handler_service.rb

@@ -117,14 +117,8 @@ module MergeRequests
       collect_errors_from_merge_request(merge_request) unless merge_request.valid?
     end
 
-    def create_params(branch)
-      params = {
-        assignees: [current_user],
-        source_branch: branch,
-        source_project: project,
-        target_branch: push_options[:target] || target_project.default_branch,
-        target_project: target_project
-      }
+    def base_params
+      params = {}
 
       if push_options.key?(:merge_when_pipeline_succeeds)
         params.merge!(
@@ -133,17 +127,8 @@ module MergeRequests
         )
       end
 
-      params
-    end
-
-    def update_params
-      params = {}
-
-      if push_options.key?(:merge_when_pipeline_succeeds)
-        params.merge!(
-          merge_when_pipeline_succeeds: push_options[:merge_when_pipeline_succeeds],
-          merge_user: current_user
-        )
+      if push_options.key?(:remove_source_branch)
+        params[:force_remove_source_branch] = push_options[:remove_source_branch]
       end
 
       if push_options.key?(:target)
@@ -153,6 +138,25 @@ module MergeRequests
       params
     end
 
+    def create_params(branch)
+      params = base_params
+
+      params.merge!(
+        assignees: [current_user],
+        source_branch: branch,
+        source_project: project,
+        target_project: target_project
+      )
+
+      params[:target_branch] ||= target_project.default_branch
+
+      params
+    end
+
+    def update_params
+      base_params
+    end
+
     def collect_errors_from_merge_request(merge_request)
       merge_request.errors.full_messages.each do |error|
         errors << error

changelogs/unreleased/rm-src-branch.yml

+---
+title: Support remove source branch on merge w/ push options
+merge_request: 30728
+author:
+type: added

doc/user/project/merge_requests/index.md

@@ -286,6 +286,7 @@ as pushing changes:
 - Create a new merge request for the pushed branch.
 - Set the target of the merge request to a particular branch.
 - Set the merge request to merge when its pipeline succeeds.
+- Set the merge request to remove the source branch when it's merged.
 
 ### Create a new merge request using git push options
 
@@ -329,6 +330,19 @@ pipeline succeeds at the same time using a `-o` flag per push option:
 git push -o merge_request.create -o merge_request.merge_when_pipeline_succeeds
 ```
 
+### Set removing the source branch using git push options
+
+To set an existing merge request to remove the source branch when the
+merge request is merged, the
+`merge_request.remove_source_branch` push option can be used:
+
+```sh
+git push -o merge_request.remove_source_branch
+```
+
+You can also use this push option in addition to the
+`merge_request.create` push option.
+
 ## Find the merge request that introduced a change
 
 > [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/2383) in GitLab 10.5.

lib/gitlab/push_options.rb

@@ -4,7 +4,12 @@ module Gitlab
   class PushOptions
     VALID_OPTIONS = HashWithIndifferentAccess.new({
       merge_request: {
-        keys: [:create, :merge_when_pipeline_succeeds, :target]
+        keys: [
+          :create,
+          :merge_when_pipeline_succeeds,
+          :remove_source_branch,
+          :target
+        ]
       },
       ci: {
         keys: [:skip]

lib/gitlab/request_profiler.rb

@@ -6,6 +6,21 @@ module Gitlab
   module RequestProfiler
     PROFILES_DIR = "#{Gitlab.config.shared.path}/tmp/requests_profiles".freeze
 
+    def all
+      Dir["#{PROFILES_DIR}/*.{html,txt}"].map do |path|
+        Profile.new(File.basename(path))
+      end.select(&:valid?)
+    end
+    module_function :all
+
+    def find(name)
+      file_path = File.join(PROFILES_DIR, name)
+      return unless File.exist?(file_path)
+
+      Profile.new(name)
+    end
+    module_function :find
+
     def profile_token
       Rails.cache.fetch('profile-token') do
         Devise.friendly_token

lib/gitlab/request_profiler/profile.rb

@@ -7,19 +7,6 @@ module Gitlab
 
       alias_method :to_param, :name
 
-      def self.all
-        Dir["#{PROFILES_DIR}/*.{html,txt}"].map do |path|
-          new(File.basename(path))
-        end
-      end
-
-      def self.find(name)
-        file_path = File.join(PROFILES_DIR, name)
-        return unless File.exist?(file_path)
-
-        new(name)
-      end
-
       def initialize(name)
         @name = name
         @file_path = File.join(PROFILES_DIR, name)
@@ -27,8 +14,8 @@ module Gitlab
         set_attributes
       end
 
-      def content
-        File.read("#{PROFILES_DIR}/#{name}")
+      def valid?
+        @request_path.present?
       end
 
       def content_type
@@ -43,11 +30,13 @@ module Gitlab
       private
 
       def set_attributes
-        _, path, timestamp, profile_mode, type = name.split(/(.*)_(\d+)_(.*)\.(html|txt)$/)
-        @request_path      = path.tr('|', '/')
-        @time              = Time.at(timestamp.to_i).utc
-        @profile_mode      = profile_mode
-        @type              = type
+        matches = name.match(/^(?<path>.*)_(?<timestamp>\d+)(_(?<profile_mode>\w+))?\.(?<type>html|txt)$/)
+        return unless matches
+
+        @request_path      = matches[:path].tr('|', '/')
+        @time              = Time.at(matches[:timestamp].to_i).utc
+        @profile_mode      = matches[:profile_mode] || 'unknown'
+        @type              = matches[:type]
       end
     end
   end

spec/controllers/admin/requests_profiles_controller_spec.rb

@@ -23,7 +23,7 @@ describe Admin::RequestsProfilesController do
     end
 
     after do
-      File.unlink(test_file)
+      FileUtils.rm_rf(tmpdir)
     end
 
     context 'when loading HTML profile' do

spec/features/admin/admin_requests_profiles_spec.rb

 require 'spec_helper'
 
 describe 'Admin::RequestsProfilesController' do
+  let(:tmpdir) { Dir.mktmpdir('profiler-test') }
+
   before do
-    FileUtils.mkdir_p(Gitlab::RequestProfiler::PROFILES_DIR)
+    stub_const('Gitlab::RequestProfiler::PROFILES_DIR', tmpdir)
     sign_in(create(:admin))
   end
 
   after do
-    Gitlab::RequestProfiler.remove_all_profiles
+    FileUtils.rm_rf(tmpdir)
   end
 
   describe 'GET /admin/requests_profiles' do
@@ -60,6 +62,12 @@ describe 'Admin::RequestsProfilesController' do
             name: "|gitlab-org|infrastructure_#{time2.to_i}_memory.html",
             created: time2,
             profile_mode: 'Memory'
+          },
+          {
+            request_path: '/gitlab-org/infrastructure',
+            name: "|gitlab-org|infrastructure_#{time2.to_i}.html",
+            created: time2,
+            profile_mode: 'Unknown'
           }
         ]
       end

spec/lib/gitlab/request_profiler/profile_spec.rb

+require 'fast_spec_helper'
+
+describe Gitlab::RequestProfiler::Profile do
+  let(:profile) { described_class.new(filename) }
+
+  describe '.new' do
+    context 'using old filename' do
+      let(:filename) { '|api|v4|version.txt_1562854738.html' }
+
+      it 'returns valid data' do
+        expect(profile).to be_valid
+        expect(profile.request_path).to eq('/api/v4/version.txt')
+        expect(profile.time).to eq(Time.at(1562854738).utc)
+        expect(profile.type).to eq('html')
+      end
+    end
+
+    context 'using new filename' do
+      let(:filename) { '|api|v4|version.txt_1563547949_execution.html' }
+
+      it 'returns valid data' do
+        expect(profile).to be_valid
+        expect(profile.request_path).to eq('/api/v4/version.txt')
+        expect(profile.profile_mode).to eq('execution')
+        expect(profile.time).to eq(Time.at(1563547949).utc)
+        expect(profile.type).to eq('html')
+      end
+    end
+  end
+
+  describe '#content_type' do
+    context 'when using html file' do
+      let(:filename) { '|api|v4|version.txt_1562854738_memory.html' }
+
+      it 'returns valid data' do
+        expect(profile).to be_valid
+        expect(profile.content_type).to eq('text/html')
+      end
+    end
+
+    context 'when using text file' do
+      let(:filename) { '|api|v4|version.txt_1562854738_memory.txt' }
+
+      it 'returns valid data' do
+        expect(profile).to be_valid
+        expect(profile.content_type).to eq('text/plain')
+      end
+    end
+
+    context 'when file is unknown' do
+      let(:filename) { '|api|v4|version.txt_1562854738_memory.xxx' }
+
+      it 'returns valid data' do
+        expect(profile).not_to be_valid
+        expect(profile.content_type).to be_nil
+      end
+    end
+  end
+end

spec/lib/gitlab/request_profiler_spec.rb

@@ -13,15 +13,42 @@ describe Gitlab::RequestProfiler do
     end
   end
 
-  describe '.remove_all_profiles' do
-    it 'removes Gitlab::RequestProfiler::PROFILES_DIR directory' do
-      dir = described_class::PROFILES_DIR
-      FileUtils.mkdir_p(dir)
+  context 'with temporary PROFILES_DIR' do
+    let(:tmpdir) { Dir.mktmpdir('profiler-test') }
+    let(:profile_name) { '|api|v4|version.txt_1562854738_memory.html' }
+    let(:profile_path) { File.join(tmpdir, profile_name) }
 
-      expect(Dir.exist?(dir)).to be true
+    before do
+      stub_const('Gitlab::RequestProfiler::PROFILES_DIR', tmpdir)
+      FileUtils.touch(profile_path)
+    end
+
+    after do
+      FileUtils.rm_rf(tmpdir)
+    end
+
+    describe '.remove_all_profiles' do
+      it 'removes Gitlab::RequestProfiler::PROFILES_DIR directory' do
+        described_class.remove_all_profiles
+
+        expect(Dir.exist?(tmpdir)).to be false
+      end
+    end
+
+    describe '.all' do
+      subject { described_class.all }
+
+      it 'returns all profiles' do
+        expect(subject.map(&:name)).to contain_exactly(profile_name)
+      end
+    end
+
+    describe '.find' do
+      subject { described_class.find(profile_name) }
 
-      described_class.remove_all_profiles
-      expect(Dir.exist?(dir)).to be false
+      it 'returns all profiles' do
+        expect(subject.name).to eq(profile_name)
+      end
     end
   end
 end

spec/services/merge_requests/push_options_handler_service_spec.rb

@@ -90,6 +90,16 @@ describe MergeRequests::PushOptionsHandlerService do
     end
   end
 
+  shared_examples_for 'a service that can remove the source branch when it is merged' do
+    subject(:last_mr) { MergeRequest.last }
+
+    it 'returns true to force_remove_source_branch?' do
+      service.execute
+
+      expect(last_mr.force_remove_source_branch?).to eq(true)
+    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 }
@@ -208,6 +218,72 @@ describe MergeRequests::PushOptionsHandlerService do
     end
   end
 
+  describe '`remove_source_branch` push option' do
+    let(:push_options) { { remove_source_branch: true } }
+
+    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, remove_source_branch: true } }
+
+        it_behaves_like 'a service that can create a merge request'
+        it_behaves_like 'a service that can remove the source branch when it is merged'
+      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, remove_source_branch: true } }
+
+        it_behaves_like 'a service that can create a merge request'
+        it_behaves_like 'a service that can remove the source branch when it is merged'
+      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 remove the source branch when it is merged'
+    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 } }