Merge branch '40622-use-left-right-and-max-count' into 'master'

app/helpers/branches_helper.rb

@@ -23,4 +23,12 @@ module BranchesHelper
   def protected_branch?(project, branch)
     ProtectedBranch.protected?(project, branch.name)
   end
+
+  def diverging_count_label(count)
+    if count >= Repository::MAX_DIVERGING_COUNT
+      "#{Repository::MAX_DIVERGING_COUNT - 1}+"
+    else
+      count.to_s
+    end
+  end
 end

app/models/repository.rb

@@ -4,6 +4,7 @@ class Repository
   REF_MERGE_REQUEST = 'merge-requests'.freeze
   REF_KEEP_AROUND = 'keep-around'.freeze
   REF_ENVIRONMENTS = 'environments'.freeze
+  MAX_DIVERGING_COUNT = 1000
 
   RESERVED_REFS_NAMES = %W[
     heads
@@ -278,11 +279,12 @@ class Repository
     cache.fetch(:"diverging_commit_counts_#{branch.name}") do
       # Rugged seems to throw a `ReferenceError` when given branch_names rather
       # than SHA-1 hashes
-      number_commits_behind = raw_repository
-        .count_commits_between(branch.dereferenced_target.sha, root_ref_hash)
-
-      number_commits_ahead = raw_repository
-        .count_commits_between(root_ref_hash, branch.dereferenced_target.sha)
+      number_commits_behind, number_commits_ahead =
+        raw_repository.count_commits_between(
+          root_ref_hash,
+          branch.dereferenced_target.sha,
+          left_right: true,
+          max_count: MAX_DIVERGING_COUNT)
 
       { behind: number_commits_behind, ahead: number_commits_ahead }
     end

app/views/projects/branches/_branch.html.haml

@@ -66,16 +66,16 @@
             = icon("trash-o")
 
     - if branch.name != @repository.root_ref
-      .divergence-graph{ title: s_('%{number_commits_behind} commits behind %{default_branch}, %{number_commits_ahead} commits ahead') % { number_commits_behind: number_commits_behind,
+      .divergence-graph{ title: s_('%{number_commits_behind} commits behind %{default_branch}, %{number_commits_ahead} commits ahead') % { number_commits_behind: diverging_count_label(number_commits_behind),
         default_branch: @repository.root_ref,
-        number_commits_ahead: number_commits_ahead } }
+        number_commits_ahead: diverging_count_label(number_commits_ahead) } }
         .graph-side
           .bar.bar-behind{ style: "width: #{number_commits_behind * bar_graph_width_factor}%" }
-          %span.count.count-behind= number_commits_behind
+          %span.count.count-behind= diverging_count_label(number_commits_behind)
         .graph-separator
         .graph-side
           .bar.bar-ahead{ style: "width: #{number_commits_ahead * bar_graph_width_factor}%" }
-          %span.count.count-ahead= number_commits_ahead
+          %span.count.count-ahead= diverging_count_label(number_commits_ahead)
 
 
   - if commit

changelogs/unreleased/40622-use-left-right-and-max-count.yml

+---
+title: Improve the performance for counting diverging commits. Show 999+
+  if it is more than 1000 commits
+merge_request: 15963
+author:
+type: performance

lib/gitlab/git/repository.rb

@@ -498,11 +498,13 @@ module Gitlab
       end
 
       def count_commits(options)
+        count_commits_options = process_count_commits_options(options)
+
         gitaly_migrate(:count_commits) do |is_enabled|
           if is_enabled
-            count_commits_by_gitaly(options)
+            count_commits_by_gitaly(count_commits_options)
           else
-            count_commits_by_shelling_out(options)
+            count_commits_by_shelling_out(count_commits_options)
           end
         end
       end
@@ -540,8 +542,8 @@ module Gitlab
       end
 
       # Counts the amount of commits between `from` and `to`.
-      def count_commits_between(from, to)
-        count_commits(ref: "#{from}..#{to}")
+      def count_commits_between(from, to, options = {})
+        count_commits(from: from, to: to, **options)
       end
 
       # Returns the SHA of the most recent common ancestor of +from+ and +to+
@@ -1468,6 +1470,26 @@ module Gitlab
         end
       end
 
+      def process_count_commits_options(options)
+        if options[:from] || options[:to]
+          ref =
+            if options[:left_right] # Compare with merge-base for left-right
+              "#{options[:from]}...#{options[:to]}"
+            else
+              "#{options[:from]}..#{options[:to]}"
+            end
+
+          options.merge(ref: ref)
+
+        elsif options[:ref] && options[:left_right]
+          from, to = options[:ref].match(/\A([^\.]*)\.{2,3}([^\.]*)\z/)[1..2]
+
+          options.merge(from: from, to: to)
+        else
+          options
+        end
+      end
+
       def log_using_shell?(options)
         options[:path].present? ||
           options[:disable_walk] ||
@@ -1690,20 +1712,59 @@ module Gitlab
       end
 
       def count_commits_by_gitaly(options)
-        gitaly_commit_client.commit_count(options[:ref], options)
+        if options[:left_right]
+          from = options[:from]
+          to = options[:to]
+
+          right_count = gitaly_commit_client
+            .commit_count("#{from}..#{to}", options)
+          left_count = gitaly_commit_client
+            .commit_count("#{to}..#{from}", options)
+
+          [left_count, right_count]
+        else
+          gitaly_commit_client.commit_count(options[:ref], options)
+        end
       end
 
       def count_commits_by_shelling_out(options)
+        cmd = count_commits_shelling_command(options)
+
+        raw_output = IO.popen(cmd) { |io| io.read }
+
+        process_count_commits_raw_output(raw_output, options)
+      end
+
+      def count_commits_shelling_command(options)
         cmd = %W[#{Gitlab.config.git.bin_path} --git-dir=#{path} rev-list]
         cmd << "--after=#{options[:after].iso8601}" if options[:after]
         cmd << "--before=#{options[:before].iso8601}" if options[:before]
         cmd << "--max-count=#{options[:max_count]}" if options[:max_count]
+        cmd << "--left-right" if options[:left_right]
         cmd += %W[--count #{options[:ref]}]
         cmd += %W[-- #{options[:path]}] if options[:path].present?
+        cmd
+      end
 
-        raw_output = IO.popen(cmd) { |io| io.read }
+      def process_count_commits_raw_output(raw_output, options)
+        if options[:left_right]
+          result = raw_output.scan(/\d+/).map(&:to_i)
+
+          if result.sum != options[:max_count]
+            result
+          else # Reaching max count, right is not accurate
+            right_option =
+              process_count_commits_options(options
+                .except(:left_right, :from, :to)
+                .merge(ref: options[:to]))
+
+            right = count_commits_by_shelling_out(right_option)
 
-        raw_output.to_i
+            [result.first, right] # left should be accurate in the first call
+          end
+        else
+          raw_output.to_i
+        end
       end
 
       def gitaly_ls_files(ref)

spec/lib/gitlab/git/repository_spec.rb

@@ -1030,12 +1030,50 @@ describe Gitlab::Git::Repository, seed_helper: true do
         end
       end
 
+      context 'with max_count' do
+        it 'returns the number of commits with path ' do
+          options = { ref: 'master', max_count: 5 }
+
+          expect(repository.count_commits(options)).to eq(5)
+        end
+      end
+
       context 'with path' do
         it 'returns the number of commits with path ' do
-          options = { ref: 'master', path: "encoding" }
+          options = { ref: 'master', path: 'encoding' }
+
+          expect(repository.count_commits(options)).to eq(2)
+        end
+      end
+
+      context 'with option :from and option :to' do
+        it 'returns the number of commits ahead for fix-mode..fix-blob-path' do
+          options = { from: 'fix-mode', to: 'fix-blob-path' }
 
           expect(repository.count_commits(options)).to eq(2)
         end
+
+        it 'returns the number of commits ahead for fix-blob-path..fix-mode' do
+          options = { from: 'fix-blob-path', to: 'fix-mode' }
+
+          expect(repository.count_commits(options)).to eq(1)
+        end
+
+        context 'with option :left_right' do
+          it 'returns the number of commits for fix-mode...fix-blob-path' do
+            options = { from: 'fix-mode', to: 'fix-blob-path', left_right: true }
+
+            expect(repository.count_commits(options)).to eq([1, 2])
+          end
+
+          context 'with max_count' do
+            it 'returns the number of commits with path ' do
+              options = { from: 'fix-mode', to: 'fix-blob-path', left_right: true, max_count: 1 }
+
+              expect(repository.count_commits(options)).to eq([1, 1])
+            end
+          end
+        end
       end
 
       context 'with max_count' do

spec/models/repository_spec.rb

@@ -2215,6 +2215,15 @@ describe Repository do
     end
   end
 
+  describe '#diverging_commit_counts' do
+    it 'returns the commit counts behind and ahead of default branch' do
+      result = repository.diverging_commit_counts(
+        repository.find_branch('fix'))
+
+      expect(result).to eq(behind: 29, ahead: 2)
+    end
+  end
+
   describe '#cache_method_output', :use_clean_rails_memory_store_caching do
     let(:fallback) { 10 }