Merge branch...

Merge branch '1811-very-poor-performance-creating-merge-request-regression-in-9-0-often-hits-60-seconds-timeout-on-small-requests' into 'master'

Resolve "Very poor performance creating merge request, regression in 9.0 - often hits 60 seconds timeout on small requests"

Closes #1811

See merge request !1522

app/controllers/projects/merge_requests_controller.rb

@@ -713,7 +713,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def set_suggested_approvers
     if @merge_request.requires_approve?
       @suggested_approvers = Gitlab::AuthorityAnalyzer.new(
-        @merge_request
+        @merge_request,
+        @merge_request.author || current_user
       ).calculate(@merge_request.approvals_required)
     end
   end

changelogs/unreleased-ee/1811-very-poor-performance-creating-merge-request-regression-in-9-0-often-hits-60-seconds-timeout-on-small-requests.yml

+---
+title: Speed up suggested approvers on MR creation
+merge_request:
+author:

lib/gitlab/authority_analyzer.rb

 module Gitlab
   class AuthorityAnalyzer
-    COMMITS_TO_CONSIDER = 5
+    COMMITS_TO_CONSIDER = 25
 
-    def initialize(merge_request)
+    def initialize(merge_request, skip_user)
       @merge_request = merge_request
+      @skip_user = skip_user
       @users = Hash.new(0)
     end
 
@@ -19,14 +20,12 @@ module Gitlab
     def involved_users
       @repo = @merge_request.target_project.repository
 
-      list_of_involved_files.each do |path|
-        @repo.commits(@merge_request.target_branch, path: path, limit: COMMITS_TO_CONSIDER).each do |commit|
-          if commit.author && commit.author != @merge_request.author
+      @repo.commits(@merge_request.target_branch, path: list_of_involved_files, limit: COMMITS_TO_CONSIDER).each do |commit|
+        if commit.author && commit.author != @skip_user
           @users[commit.author] += 1
         end
       end
     end
-    end
 
     def list_of_involved_files
       diffable = [@merge_request.compare, @merge_request.merge_request_diff].compact

lib/gitlab/git/repository.rb

@@ -346,7 +346,12 @@ module Gitlab
         cmd << "--after=#{options[:after].iso8601}" if options[:after]
         cmd << "--before=#{options[:before].iso8601}" if options[:before]
         cmd << sha
-        cmd += %W[-- #{options[:path]}] if options[:path].present?
+
+        # :path can be a string or an array of strings
+        if options[:path].present?
+          cmd << '--'
+          cmd += Array(options[:path])
+        end
 
         raw_output = IO.popen(cmd) { |io| io.read }
         lines = offset_in_ruby ? raw_output.lines.drop(offset) : raw_output.lines

spec/lib/gitlab/authority_analyzer_spec.rb

@@ -6,7 +6,7 @@ describe Gitlab::AuthorityAnalyzer, lib: true do
     let(:author) { create(:user) }
     let(:user_a) { create(:user) }
     let(:user_b) { create(:user) }
-    let(:merge_request) { create(:merge_request, target_project: project, source_project: project, author: author) }
+    let(:merge_request) { create(:merge_request, target_project: project, source_project: project) }
     let(:files) { [double(:file, deleted_file: true, old_path: 'foo')] }
 
     let(:commits) do
@@ -19,7 +19,7 @@ describe Gitlab::AuthorityAnalyzer, lib: true do
       ]
     end
 
-    let(:approvers) { Gitlab::AuthorityAnalyzer.new(merge_request).calculate(number_of_approvers) }
+    let(:approvers) { Gitlab::AuthorityAnalyzer.new(merge_request, author).calculate(number_of_approvers) }
 
     before do
       merge_request.compare = double(:compare, raw_diffs: files)

spec/lib/gitlab/git/repository_spec.rb

@@ -771,8 +771,8 @@ describe Gitlab::Git::Repository, seed_helper: true do
         commits = repository.log(options)
 
         expect(commits.size).to be > 0
-        satisfy do
-          commits.all? { |commit| commit.created_at >= options[:after] }
+        expect(commits).to satisfy do |commits|
+          commits.all? { |commit| commit.time >= options[:after] }
         end
       end
     end
@@ -784,8 +784,27 @@ describe Gitlab::Git::Repository, seed_helper: true do
         commits = repository.log(options)
 
         expect(commits.size).to be > 0
-        satisfy do
-          commits.all? { |commit| commit.created_at <= options[:before] }
+        expect(commits).to satisfy do |commits|
+          commits.all? { |commit| commit.time <= options[:before] }
+        end
+      end
+    end
+
+    context 'when multiple paths are provided' do
+      let(:options) { { ref: 'master', path: ['PROCESS.md', 'README.md'] } }
+
+      def commit_files(commit)
+        commit.diff(commit.parent_ids.first).deltas.flat_map do |delta|
+          [delta.old_file[:path], delta.new_file[:path]].uniq.compact
+        end
+      end
+
+      it 'only returns commits matching at least one path' do
+        commits = repository.log(options)
+
+        expect(commits.size).to be > 0
+        expect(commits).to satisfy do |commits|
+          commits.none? { |commit| (commit_files(commit) & options[:path]).empty? }
         end
       end
     end