Merge branch 'add-client-for-count-diverging-commits' into 'master'

add client support for CountDivergingCommits rpc

See merge request gitlab-org/gitlab-ce!24287

10 files changed, 155 insertions, 16 deletions

diff --git a/app/assets/stylesheets/pages/branches.scss b/app/assets/stylesheets/pages/branches.scss
index 38fec3f0aa8..ce0622b3d48 100644
--- a/app/assets/stylesheets/pages/branches.scss
+++ b/app/assets/stylesheets/pages/branches.scss
@@ -11,15 +11,24 @@
 }
 
 .divergence-graph {
+  $graph-side-width: 80px;
+  $graph-separator-width: 1px;
+
   padding: 0 6px;
 
   .graph-side {
     position: relative;
-    width: 80px;
+    width: $graph-side-width;
     height: 22px;
     padding: 5px 0 13px;
     float: left;
 
+    &.full {
+      width: $graph-side-width * 2 + $graph-separator-width;
+      display: flex;
+      justify-content: center;
+    }
+
     .bar {
       position: absolute;
       height: 4px;
@@ -57,7 +66,7 @@
 
   .graph-separator {
     position: relative;
-    width: 1px;
+    width: $graph-separator-width;
     height: 18px;
     margin: 5px 0 0;
     float: left;
diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb
index a6bfb913900..32b7f3207ef 100644
--- a/app/controllers/projects/branches_controller.rb
+++ b/app/controllers/projects/branches_controller.rb
@@ -29,7 +29,8 @@ class Projects::BranchesController < Projects::ApplicationController
         Gitlab::GitalyClient.allow_n_plus_1_calls do
           @max_commits = @branches.reduce(0) do |memo, branch|
             diverging_commit_counts = repository.diverging_commit_counts(branch)
-            [memo, diverging_commit_counts[:behind], diverging_commit_counts[:ahead]].max
+            [memo, diverging_commit_counts.values_at(:behind, :ahead, :distance)]
+              .flatten.compact.max
           end
         end
 
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 7c50b4488e5..ed55a6e572b 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -288,13 +288,16 @@ class Repository
       # Rugged seems to throw a `ReferenceError` when given branch_names rather
       # than SHA-1 hashes
       number_commits_behind, number_commits_ahead =
-        raw_repository.count_commits_between(
+        raw_repository.diverging_commit_count(
           @root_ref_hash,
           branch.dereferenced_target.sha,
-          left_right: true,
           max_count: MAX_DIVERGING_COUNT)
 
-      { behind: number_commits_behind, ahead: number_commits_ahead }
+      if number_commits_behind + number_commits_ahead >= MAX_DIVERGING_COUNT
+        { distance: MAX_DIVERGING_COUNT }
+      else
+        { behind: number_commits_behind, ahead: number_commits_ahead }
+      end
     end
   end
 
diff --git a/app/views/projects/branches/_branch.html.haml b/app/views/projects/branches/_branch.html.haml
index 4b0ea15335e..c64ad1c8147 100644
--- a/app/views/projects/branches/_branch.html.haml
+++ b/app/views/projects/branches/_branch.html.haml
@@ -2,6 +2,7 @@
 - commit = @repository.commit(branch.dereferenced_target)
 - bar_graph_width_factor = @max_commits > 0 ? 100.0/@max_commits : 0
 - diverging_commit_counts = @repository.diverging_commit_counts(branch)
+- number_commits_distance = diverging_commit_counts[:distance]
 - number_commits_behind = diverging_commit_counts[:behind]
 - number_commits_ahead = diverging_commit_counts[:ahead]
 - merge_project = merge_request_source_project_for_project(@project)
@@ -28,16 +29,23 @@
         = s_('Branches|Cant find HEAD commit for this branch')
 
   - if branch.name != @repository.root_ref
-    .divergence-graph.d-none.d-md-block{ 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: diverging_count_label(number_commits_ahead) } }
-      .graph-side
-        .bar.bar-behind{ style: "width: #{number_commits_behind * bar_graph_width_factor}%" }
-        %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= diverging_count_label(number_commits_ahead)
+    - if number_commits_distance.nil?
+      .divergence-graph.d-none.d-md-block{ 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: diverging_count_label(number_commits_ahead) } }
+        .graph-side
+          .bar.bar-behind{ style: "width: #{number_commits_behind * bar_graph_width_factor}%" }
+          %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= diverging_count_label(number_commits_ahead)
+    - else
+      .divergence-graph.d-none.d-md-block{ title: s_('More than %{number_commits_distance} commits different with %{default_branch}') % { number_commits_distance: diverging_count_label(number_commits_distance),
+        default_branch: @repository.root_ref} }
+        .graph-side.full
+          .bar{ style: "width: #{number_commits_distance * bar_graph_width_factor}%" }
+          %span.count= diverging_count_label(number_commits_distance)
 
   .controls.d-none.d-md-block<
     - if merge_project && create_mr_button?(@repository.root_ref, branch.name)
diff --git a/changelogs/unreleased/improve-performance-for-diverging-commit-counts.yml b/changelogs/unreleased/improve-performance-for-diverging-commit-counts.yml
new file mode 100644
index 00000000000..76ff15cba5b
--- /dev/null
+++ b/changelogs/unreleased/improve-performance-for-diverging-commit-counts.yml
@@ -0,0 +1,5 @@
+---
+title: Improve performance for diverging commit counts
+merge_request: 24287
+author:
+type: performance
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 54bbd531398..593a3676519 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -491,6 +491,13 @@ module Gitlab
         end
       end
 
+      # Return total diverging commits count
+      def diverging_commit_count(from, to, max_count:)
+        wrapped_gitaly_errors do
+          gitaly_commit_client.diverging_commit_count(from, to, max_count: max_count)
+        end
+      end
+
       # Mimic the `git clean` command and recursively delete untracked files.
       # Valid keys that can be passed in the +options+ hash are:
       #
diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb
index 4e46cb9f05c..ea12424eb4a 100644
--- a/lib/gitlab/gitaly_client/commit_service.rb
+++ b/lib/gitlab/gitaly_client/commit_service.rb
@@ -150,6 +150,17 @@ module Gitlab
         GitalyClient.call(@repository.storage, :commit_service, :count_commits, request, timeout: GitalyClient.medium_timeout).count
       end
 
+      def diverging_commit_count(from, to, max_count:)
+        request = Gitaly::CountDivergingCommitsRequest.new(
+          repository: @gitaly_repo,
+          from: encode_binary(from),
+          to: encode_binary(to),
+          max_count: max_count
+        )
+        response = GitalyClient.call(@repository.storage, :commit_service, :count_diverging_commits, request, timeout: GitalyClient.medium_timeout)
+        [response.left_count, response.right_count]
+      end
+
       def list_last_commits_for_tree(revision, path, offset: 0, limit: 25)
         request = Gitaly::ListLastCommitsForTreeRequest.new(
           repository: @gitaly_repo,
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 8d7ed7a5b88..7cbba0779f7 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -4722,6 +4722,9 @@ msgstr ""
 msgid "More information is available|here"
 msgstr ""
 
+msgid "More than %{number_commits_distance} commits different with %{default_branch}"
+msgstr ""
+
 msgid "Most stars"
 msgstr ""
 
diff --git a/spec/features/projects/branches/user_views_branches_spec.rb b/spec/features/projects/branches/user_views_branches_spec.rb
index 62ae793151c..777d30fdffd 100644
--- a/spec/features/projects/branches/user_views_branches_spec.rb
+++ b/spec/features/projects/branches/user_views_branches_spec.rb
@@ -15,6 +15,8 @@ describe "User views branches" do
 
     it "shows branches" do
       expect(page).to have_content("Branches").and have_content("master")
+
+      expect(page.all(".graph-side")).to all( have_content(/\d+/) )
     end
   end
 
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index cf9e0cccc71..8a9e78ba3c3 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -283,6 +283,96 @@ describe Gitlab::Git::Repository, :seed_helper do
     end
   end
 
+  describe '#diverging_commit_count' do
+    it 'counts 0 for the same branch' do
+      expect(repository.diverging_commit_count('master', 'master', max_count: 1000)).to eq([0, 0])
+    end
+
+    context 'max count does not truncate results' do
+      where(:left, :right, :expected) do
+        1 | 1 | [1, 1]
+        4 | 4 | [4, 4]
+        2 | 2 | [2, 2]
+        2 | 4 | [2, 4]
+        4 | 2 | [4, 2]
+        10 | 10 | [10, 10]
+      end
+
+      with_them do
+        before do
+          repository.create_branch('left-branch', 'master')
+          repository.create_branch('right-branch', 'master')
+
+          left.times do
+            new_commit_edit_new_file_on_branch(repository_rugged, 'encoding/CHANGELOG', 'left-branch', 'some more content for a', 'some stuff')
+          end
+
+          right.times do
+            new_commit_edit_new_file_on_branch(repository_rugged, 'encoding/CHANGELOG', 'right-branch', 'some more content for b', 'some stuff')
+          end
+        end
+
+        after do
+          repository.delete_branch('left-branch')
+          repository.delete_branch('right-branch')
+        end
+
+        it 'returns the correct count bounding at max_count' do
+          branch_a_sha = repository_rugged.branches['left-branch'].target.oid
+          branch_b_sha = repository_rugged.branches['right-branch'].target.oid
+
+          count = repository.diverging_commit_count(branch_a_sha, branch_b_sha, max_count: 1000)
+
+          expect(count).to eq(expected)
+        end
+      end
+    end
+
+    context 'max count truncates results' do
+      where(:left, :right, :max_count) do
+        1 | 1 | 1
+        4 | 4 | 4
+        2 | 2 | 3
+        2 | 4 | 3
+        4 | 2 | 5
+        10 | 10 | 10
+      end
+
+      with_them do
+        before do
+          repository.create_branch('left-branch', 'master')
+          repository.create_branch('right-branch', 'master')
+
+          left.times do
+            new_commit_edit_new_file_on_branch(repository_rugged, 'encoding/CHANGELOG', 'left-branch', 'some more content for a', 'some stuff')
+          end
+
+          right.times do
+            new_commit_edit_new_file_on_branch(repository_rugged, 'encoding/CHANGELOG', 'right-branch', 'some more content for b', 'some stuff')
+          end
+        end
+
+        after do
+          repository.delete_branch('left-branch')
+          repository.delete_branch('right-branch')
+        end
+
+        it 'returns the correct count bounding at max_count' do
+          branch_a_sha = repository_rugged.branches['left-branch'].target.oid
+          branch_b_sha = repository_rugged.branches['right-branch'].target.oid
+
+          results = repository.diverging_commit_count(branch_a_sha, branch_b_sha, max_count: max_count)
+
+          expect(results[0] + results[1]).to eq(max_count)
+        end
+      end
+    end
+
+    it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::CommitService, :diverging_commit_count do
+      subject { repository.diverging_commit_count('master', 'master', max_count: 1000) }
+    end
+  end
+
   describe '#has_local_branches?' do
     context 'check for local branches' do
       it { expect(repository.has_local_branches?).to eq(true) }