diff --git a/app/helpers/branches_helper.rb b/app/helpers/branches_helper.rb index 686437fc99a..2641a98e29e 100644 --- a/app/helpers/branches_helper.rb +++ b/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 diff --git a/app/models/repository.rb b/app/models/repository.rb index 4bedcbfb6a2..7b8f5794a87 100644 --- a/app/models/repository.rb +++ b/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 diff --git a/app/views/projects/branches/_branch.html.haml b/app/views/projects/branches/_branch.html.haml index acf67b83890..1da0e865a41 100644 --- a/app/views/projects/branches/_branch.html.haml +++ b/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 diff --git a/changelogs/unreleased/40622-use-left-right-and-max-count.yml b/changelogs/unreleased/40622-use-left-right-and-max-count.yml new file mode 100644 index 00000000000..c4c8f271cbe --- /dev/null +++ b/changelogs/unreleased/40622-use-left-right-and-max-count.yml @@ -0,0 +1,6 @@ +--- +title: Improve the performance for counting diverging commits. Show 999+ + if it is more than 1000 commits +merge_request: 15963 +author: +type: performance diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 17c05c44d7e..e8b1788e140 100644 --- a/lib/gitlab/git/repository.rb +++ b/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) - raw_output.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) + + [result.first, right] # left should be accurate in the first call + end + else + raw_output.to_i + end end def gitaly_ls_files(ref) diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index faccc2c8e00..f94234f6010 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -1030,14 +1030,52 @@ 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 it 'returns the number of commits up to the passed limit' do options = { ref: 'master', max_count: 10, after: Time.iso8601('2013-03-03T20:15:01+00:00') } diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 9a68ae086ea..48a75c9885b 100644 --- a/spec/models/repository_spec.rb +++ b/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 }