Merge branch '37824-many-branches-lock-server' into 'master'

Project with many branches can lock server running "git branch --contains XXX"

Closes #37824

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

9 files changed, 218 insertions, 28 deletions

diff --git a/app/assets/stylesheets/framework/wells.scss b/app/assets/stylesheets/framework/wells.scss
index 5f9756bf58a..68824ff8418 100644
--- a/app/assets/stylesheets/framework/wells.scss
+++ b/app/assets/stylesheets/framework/wells.scss
@@ -52,6 +52,37 @@
   .label.label-gray {
     background-color: $well-expand-item;
   }
+
+  .branches {
+    display: inline;
+  }
+
+  .branch-link {
+    margin-bottom: 2px;
+  }
+
+  .limit-box {
+    cursor: pointer;
+    display: inline-flex;
+    align-items: center;
+    background-color: $red-100;
+    border-radius: $border-radius-default;
+    text-align: center;
+
+    &:hover {
+      background-color: $red-200;
+    }
+
+    .limit-icon {
+      margin: 0 8px;
+    }
+
+    .limit-message {
+      line-height: 16px;
+      margin-right: 8px;
+      font-size: 12px;
+    }
+  }
 }
 
 .light-well {
diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb
index a62f05db7db..494d412b532 100644
--- a/app/controllers/projects/commit_controller.rb
+++ b/app/controllers/projects/commit_controller.rb
@@ -16,6 +16,8 @@ class Projects::CommitController < Projects::ApplicationController
   before_action :define_note_vars, only: [:show, :diff_for_path]
   before_action :authorize_edit_tree!, only: [:revert, :cherry_pick]
 
+  BRANCH_SEARCH_LIMIT = 1000
+
   def show
     apply_diff_view_cookie!
 
@@ -56,8 +58,14 @@ class Projects::CommitController < Projects::ApplicationController
   end
 
   def branches
-    @branches = @project.repository.branch_names_contains(commit.id)
-    @tags = @project.repository.tag_names_contains(commit.id)
+    # branch_names_contains/tag_names_contains can take a long time when there are thousands of
+    # branches/tags - each `git branch --contains xxx` request can consume a cpu core.
+    # so only do the query when there are a manageable number of branches/tags
+    @branches_limit_exceeded = @project.repository.branch_count > BRANCH_SEARCH_LIMIT
+    @branches = @branches_limit_exceeded ? [] : @project.repository.branch_names_contains(commit.id)
+
+    @tags_limit_exceeded = @project.repository.tag_count > BRANCH_SEARCH_LIMIT
+    @tags = @tags_limit_exceeded ? [] : @project.repository.tag_names_contains(commit.id)
     render layout: false
   end
 
diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb
index ef22cafc2e2..f9a666fa1e6 100644
--- a/app/helpers/commits_helper.rb
+++ b/app/helpers/commits_helper.rb
@@ -60,23 +60,33 @@ module CommitsHelper
     branches.include?(project.default_branch) ? branches.delete(project.default_branch) : branches.pop
   end
 
+  # Returns a link formatted as a commit branch link
+  def commit_branch_link(url, text)
+    link_to(url, class: 'label label-gray ref-name branch-link') do
+      icon('code-fork') + " #{text}"
+    end
+  end
+
   # Returns the sorted alphabetically links to branches, separated by a comma
   def commit_branches_links(project, branches)
     branches.sort.map do |branch|
-      link_to(project_ref_path(project, branch), class: "label label-gray ref-name") do
-        icon('code-fork') + " #{branch}"
-      end
-    end.join(" ").html_safe
+      commit_branch_link(project_ref_path(project, branch), branch)
+    end.join(' ').html_safe
+  end
+
+  # Returns a link formatted as a commit tag link
+  def commit_tag_link(url, text)
+    link_to(url, class: 'label label-gray ref-name') do
+      icon('tag') + " #{text}"
+    end
   end
 
   # Returns the sorted links to tags, separated by a comma
   def commit_tags_links(project, tags)
     sorted = VersionSorter.rsort(tags)
     sorted.map do |tag|
-      link_to(project_ref_path(project, tag), class: "label label-gray ref-name") do
-        icon('tag') + " #{tag}"
-      end
-    end.join(" ").html_safe
+      commit_tag_link(project_ref_path(project, tag), tag)
+    end.join(' ').html_safe
   end
 
   def link_to_browse_code(project, commit)
diff --git a/app/views/projects/commit/_commit_box.html.haml b/app/views/projects/commit/_commit_box.html.haml
index 3f4415f9e5e..8b9c1bbb602 100644
--- a/app/views/projects/commit/_commit_box.html.haml
+++ b/app/views/projects/commit/_commit_box.html.haml
@@ -61,7 +61,7 @@
     %span.cgray= n_('parent', 'parents', @commit.parents.count)
     - @commit.parents.each do |parent|
       = link_to parent.short_id, project_commit_path(@project, parent), class: "commit-sha"
-    %span.commit-info.branches
+    .commit-info.branches
       %i.fa.fa-spinner.fa-spin
 
   - if @commit.last_pipeline
diff --git a/app/views/projects/commit/_limit_exceeded_message.html.haml b/app/views/projects/commit/_limit_exceeded_message.html.haml
new file mode 100644
index 00000000000..84a52d49487
--- /dev/null
+++ b/app/views/projects/commit/_limit_exceeded_message.html.haml
@@ -0,0 +1,8 @@
+.has-tooltip{ class: "limit-box limit-box-#{objects} prepend-left-5", data: { title: "Project has too many #{label_for_message} to search"} }
+  .limit-icon
+    - if objects == :branch
+      = icon('code-fork')
+    - else
+      = icon('tag')
+  .limit-message
+    %span #{label_for_message.capitalize} unavailable
diff --git a/app/views/projects/commit/branches.html.haml b/app/views/projects/commit/branches.html.haml
index 911c9ddce06..8611129b356 100644
--- a/app/views/projects/commit/branches.html.haml
+++ b/app/views/projects/commit/branches.html.haml
@@ -1,15 +1,15 @@
-- if @branches.any? || @tags.any?
+- if @branches_limit_exceeded
+  = render 'limit_exceeded_message', objects: :branch, label_for_message: "branches"
+- elsif @branches.any?
   - branch = commit_default_branch(@project, @branches)
-  = link_to(project_ref_path(@project, branch), class: "label label-gray ref-name") do
-    = icon('code-fork')
-    = branch
+  = commit_branch_link(project_ref_path(@project, branch), branch)
 
-  -# `commit_default_branch` deletes the default branch from `@branches`,
-  -# so only render this if we have more branches left
-  - if @branches.any? || @tags.any?
-    %span
-      = link_to "…", "#", class: "js-details-expand label label-gray"
-
-      %span.js-details-content.hide
-        = commit_branches_links(@project, @branches) if @branches.any?
-        = commit_tags_links(@project, @tags) if @tags.any?
+- if @branches.any? || @tags.any? || @tags_limit_exceeded
+  %span
+    = link_to "…", "#", class: "js-details-expand label label-gray"
+    %span.js-details-content.hide
+      = commit_branches_links(@project, @branches)
+      - if @tags_limit_exceeded
+        = render 'limit_exceeded_message', objects: :tag, label_for_message: "tags"
+      - else
+        = commit_tags_links(@project, @tags)
diff --git a/changelogs/unreleased/37824-many-branches-lock-server.yml b/changelogs/unreleased/37824-many-branches-lock-server.yml
new file mode 100644
index 00000000000..f75f79ec4a0
--- /dev/null
+++ b/changelogs/unreleased/37824-many-branches-lock-server.yml
@@ -0,0 +1,6 @@
+---
+title: While displaying a commit, do not show list of related branches if there are
+  thousands of branches
+merge_request: 14812
+author:
+type: other
diff --git a/spec/controllers/projects/commit_controller_spec.rb b/spec/controllers/projects/commit_controller_spec.rb
index 4612fc6e441..5dc27e2bbba 100644
--- a/spec/controllers/projects/commit_controller_spec.rb
+++ b/spec/controllers/projects/commit_controller_spec.rb
@@ -134,8 +134,8 @@ describe Projects::CommitController do
     end
   end
 
-  describe "GET branches" do
-    it "contains branch and tags information" do
+  describe 'GET branches' do
+    it 'contains branch and tags information' do
       commit = project.commit('5937ac0a7beb003549fc5fd26fc247adbce4a52e')
 
       get(:branches,
@@ -143,8 +143,26 @@ describe Projects::CommitController do
           project_id: project,
           id: commit.id)
 
-      expect(assigns(:branches)).to include("master", "feature_conflict")
-      expect(assigns(:tags)).to include("v1.1.0")
+      expect(assigns(:branches)).to include('master', 'feature_conflict')
+      expect(assigns(:branches_limit_exceeded)).to be_falsey
+      expect(assigns(:tags)).to include('v1.1.0')
+      expect(assigns(:tags_limit_exceeded)).to be_falsey
+    end
+
+    it 'returns :limit_exceeded when number of branches/tags reach a threshhold' do
+      commit = project.commit('5937ac0a7beb003549fc5fd26fc247adbce4a52e')
+      allow_any_instance_of(Repository).to receive(:branch_count).and_return(1001)
+      allow_any_instance_of(Repository).to receive(:tag_count).and_return(1001)
+
+      get(:branches,
+          namespace_id: project.namespace,
+          project_id: project,
+          id: commit.id)
+
+      expect(assigns(:branches)).to eq([])
+      expect(assigns(:branches_limit_exceeded)).to be_truthy
+      expect(assigns(:tags)).to eq([])
+      expect(assigns(:tags_limit_exceeded)).to be_truthy
     end
   end
 
diff --git a/spec/views/projects/commit/branches.html.haml_spec.rb b/spec/views/projects/commit/branches.html.haml_spec.rb
new file mode 100644
index 00000000000..b9d4dc80fe0
--- /dev/null
+++ b/spec/views/projects/commit/branches.html.haml_spec.rb
@@ -0,0 +1,109 @@
+require 'spec_helper'
+
+describe 'projects/commit/branches.html.haml' do
+  let(:project) { create(:project, :repository) }
+
+  before do
+    assign(:project, project)
+  end
+
+  context 'when branches and tags are available' do
+    before do
+      assign(:branches, ['master', 'test-branch'])
+      assign(:branches_limit_exceeded, false)
+      assign(:tags, ['tag1'])
+      assign(:tags_limit_exceeded, false)
+
+      render
+    end
+
+    it 'shows default branch' do
+      expect(rendered).to have_link('master')
+    end
+
+    it 'shows js expand link' do
+      expect(rendered).to have_selector('.js-details-expand')
+    end
+
+    it 'shows branch and tag links' do
+      expect(rendered).to have_link('test-branch')
+      expect(rendered).to have_link('tag1')
+    end
+  end
+
+  context 'when branches are available but no tags' do
+    before do
+      assign(:branches, ['master', 'test-branch'])
+      assign(:branches_limit_exceeded, false)
+      assign(:tags, [])
+      assign(:tags_limit_exceeded, true)
+
+      render
+    end
+
+    it 'shows branches' do
+      expect(rendered).to have_link('master')
+      expect(rendered).to have_link('test-branch')
+    end
+
+    it 'shows js expand link' do
+      expect(rendered).to have_selector('.js-details-expand')
+    end
+
+    it 'shows limit exceeded message for tags' do
+      expect(rendered).to have_text('Tags unavailable')
+    end
+  end
+
+  context 'when tags are available but no branches (just default)' do
+    before do
+      assign(:branches, ['master'])
+      assign(:branches_limit_exceeded, true)
+      assign(:tags, %w(tag1 tag2))
+      assign(:tags_limit_exceeded, false)
+
+      render
+    end
+
+    it 'shows default branch' do
+      expect(rendered).to have_text('master')
+    end
+
+    it 'shows js expand link' do
+      expect(rendered).to have_selector('.js-details-expand')
+    end
+
+    it 'shows tags' do
+      expect(rendered).to have_link('tag1')
+      expect(rendered).to have_link('tag2')
+    end
+
+    it 'shows limit exceeded for branches' do
+      expect(rendered).to have_text('Branches unavailable')
+    end
+  end
+
+  context 'when branches and tags are not available' do
+    before do
+      assign(:branches, ['master'])
+      assign(:branches_limit_exceeded, true)
+      assign(:tags, [])
+      assign(:tags_limit_exceeded, true)
+
+      render
+    end
+
+    it 'shows default branch' do
+      expect(rendered).to have_text('master')
+    end
+
+    it 'shows js expand link' do
+      expect(rendered).to have_selector('.js-details-expand')
+    end
+
+    it 'shows too many to search' do
+      expect(rendered).to have_text('Branches unavailable')
+      expect(rendered).to have_text('Tags unavailable')
+    end
+  end
+end