Merge branch '345479_optimize_repository_controller_show' into 'master'

Optimize protected branches/tags matching

See merge request gitlab-org/gitlab!77988

app/controllers/projects/settings/repository_controller.rb

@@ -81,8 +81,7 @@ module Projects
         @protected_branch = @project.protected_branches.new
         @protected_tag = @project.protected_tags.new
 
-        @protected_branches_count = @protected_branches.reduce(0) { |sum, branch| sum + branch.matching(@project.repository.branches).size }
-        @protected_tags_count = @protected_tags.reduce(0) { |sum, tag| sum + tag.matching(@project.repository.tags).size }
+        @protected_tags_count = @protected_tags.reduce(0) { |sum, tag| sum + tag.matching(@project.repository.tag_names).size }
 
         load_gon_index
       end

app/models/protectable_dropdown.rb

@@ -2,6 +2,10 @@
 
 class ProtectableDropdown
   REF_TYPES = %i[branches tags].freeze
+  REF_NAME_METHODS = {
+    branches: :branch_names,
+    tags: :tag_names
+  }.freeze
 
   def initialize(project, ref_type)
     raise ArgumentError, "invalid ref type `#{ref_type}`" unless ref_type.in?(REF_TYPES)
@@ -23,12 +27,12 @@ class ProtectableDropdown
 
   private
 
-  def refs
-    @project.repository.public_send(@ref_type) # rubocop:disable GitlabSecurity/PublicSend
+  def ref_names
+    @project.repository.public_send(ref_name_method) # rubocop:disable GitlabSecurity/PublicSend
   end
 
-  def ref_names
-    refs.map(&:name)
+  def ref_name_method
+    REF_NAME_METHODS[@ref_type]
   end
 
   def protections

app/models/ref_matcher.rb

@@ -5,10 +5,10 @@ class RefMatcher
     @ref_name_or_pattern = ref_name_or_pattern
   end
 
-  # Returns all branches/tags (among the given list of refs [`Gitlab::Git::Branch`])
+  # Returns all branches/tags (among the given list of refs [`Gitlab::Git::Branch`] or their names [`String`])
   # that match the current protected ref.
   def matching(refs)
-    refs.select { |ref| matches?(ref.name) }
+    refs.select { |ref| ref.is_a?(String) ? matches?(ref) : matches?(ref.name) }
   end
 
   # Checks if the protected ref matches the given ref name.

app/views/projects/protected_branches/shared/_branches_list.html.haml

 .protected-branches-list.js-protected-branches-list.qa-protected-branches-list
   - if @protected_branches.empty?
     .card-header.bg-white
-      = s_("ProtectedBranch|Protected branch (%{protected_branches_count})") % { protected_branches_count: @protected_branches_count }
+      = s_("ProtectedBranch|Protected branch (%{protected_branches_count})") % { protected_branches_count: 0 }
     %p.settings-message.text-center
       = s_("ProtectedBranch|There are currently no protected branches, protect a branch with the form above.")
   - else

app/views/projects/protected_branches/shared/_protected_branch.html.haml

@@ -9,7 +9,7 @@
 
     %div
       - if protected_branch.wildcard?
-        - matching_branches = protected_branch.matching(repository.branches)
+        - matching_branches = protected_branch.matching(repository.branch_names)
         = link_to pluralize(matching_branches.count, "matching branch"), namespace_project_protected_branch_path(@project.namespace, @project, protected_branch)
       - elsif !protected_branch.commit
         %span.text-muted Branch was deleted.

app/views/projects/protected_tags/shared/_protected_tag.html.haml

@@ -6,7 +6,7 @@
       = gl_badge_tag s_('ProtectedTags|default'), variant: :info, class: 'gl-ml-2'
   %td
     - if protected_tag.wildcard?
-      - matching_tags = protected_tag.matching(repository.tags)
+      - matching_tags = protected_tag.matching(repository.tag_names)
       = link_to pluralize(matching_tags.count, "matching tag"), project_protected_tag_path(@project, protected_tag)
     - else
       - if commit = protected_tag.commit

app/views/projects/protected_tags/shared/_tags_list.html.haml

 .protected-tags-list.js-protected-tags-list
   - if @protected_tags.empty?
     .card-header
-      Protected tags (#{@protected_tags_count})
+      Protected tags (0)
     %p.settings-message.text-center
       No tags are protected.
   - else

spec/models/protectable_dropdown_spec.rb

@@ -3,8 +3,9 @@
 require 'spec_helper'
 
 RSpec.describe ProtectableDropdown do
+  subject(:dropdown) { described_class.new(project, ref_type) }
+
   let(:project) { create(:project, :repository) }
-  let(:subject) { described_class.new(project, :branches) }
 
   describe 'initialize' do
     it 'raises ArgumentError for invalid ref type' do
@@ -13,34 +14,75 @@ RSpec.describe ProtectableDropdown do
     end
   end
 
-  describe '#protectable_ref_names' do
+  shared_examples 'protectable_ref_names' do
     context 'when project repository is not empty' do
+      it 'includes elements matching a protected ref wildcard' do
+        is_expected.to include(matching_ref)
+
+        factory = ref_type == :branches ? :protected_branch : :protected_tag
+
+        create(factory, name: "#{matching_ref[0]}*", project: project)
+
+        subject = described_class.new(project.reload, ref_type)
+
+        expect(subject.protectable_ref_names).to include(matching_ref)
+      end
+    end
+
+    context 'when project repository is empty' do
+      let(:project) { create(:project) }
+
+      it 'returns empty list' do
+        is_expected.to be_empty
+      end
+    end
+  end
+
+  describe '#protectable_ref_names' do
+    subject { dropdown.protectable_ref_names }
+
+    context 'for branches' do
+      let(:ref_type) { :branches }
+      let(:matching_ref) { 'feature' }
+
       before do
         create(:protected_branch, project: project, name: 'master')
       end
 
-      it { expect(subject.protectable_ref_names).to include('feature') }
-      it { expect(subject.protectable_ref_names).not_to include('master') }
+      it { is_expected.to include(matching_ref) }
+      it { is_expected.not_to include('master') }
 
-      it "includes branches matching a protected branch wildcard" do
-        expect(subject.protectable_ref_names).to include('feature')
+      it_behaves_like 'protectable_ref_names'
+    end
 
-        create(:protected_branch, name: 'feat*', project: project)
+    context 'for tags' do
+      let(:ref_type) { :tags }
+      let(:matching_ref) { 'v1.0.0' }
 
-        subject = described_class.new(project.reload, :branches)
+      before do
+        create(:protected_tag, project: project, name: 'v1.1.0')
+      end
+
+      it { is_expected.to include(matching_ref) }
+      it { is_expected.not_to include('v1.1.0') }
 
-        expect(subject.protectable_ref_names).to include('feature')
+      it_behaves_like 'protectable_ref_names'
     end
   end
 
-    context 'when project repository is empty' do
-      let(:project) { create(:project) }
+  describe '#hash' do
+    subject { dropdown.hash }
 
-      it "returns empty list" do
-        subject = described_class.new(project, :branches)
+    context 'for branches' do
+      let(:ref_type) { :branches }
 
-        expect(subject.protectable_ref_names).to be_empty
+      it { is_expected.to include(id: 'feature', text: 'feature', title: 'feature') }
     end
+
+    context 'for tags' do
+      let(:ref_type) { :tags }
+
+      it { is_expected.to include(id: 'v1.0.0', text: 'v1.0.0', title: 'v1.0.0') }
     end
   end
 end

spec/models/ref_matcher_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe RefMatcher do
+  subject(:ref_matcher) { described_class.new(ref_pattern) }
+
+  let(:ref_pattern) { 'v1.0' }
+
+  shared_examples 'matching_refs' do
+    context 'when there is no match' do
+      let(:ref_pattern) { 'unknown' }
+
+      it { is_expected.to match_array([]) }
+    end
+
+    context 'when ref pattern is a wildcard' do
+      let(:ref_pattern) { 'v*' }
+
+      it { is_expected.to match_array(refs) }
+    end
+  end
+
+  describe '#matching' do
+    subject { ref_matcher.matching(refs) }
+
+    context 'when refs are strings' do
+      let(:refs) { ['v1.0', 'v1.1'] }
+
+      it { is_expected.to match_array([ref_pattern]) }
+
+      it_behaves_like 'matching_refs'
+    end
+
+    context 'when refs are ref objects' do
+      let(:matching_ref) { double('tag', name: 'v1.0') }
+      let(:not_matching_ref) { double('tag', name: 'v1.1') }
+      let(:refs) { [matching_ref, not_matching_ref] }
+
+      it { is_expected.to match_array([matching_ref]) }
+
+      it_behaves_like 'matching_refs'
+    end
+  end
+
+  describe '#matches?' do
+    subject { ref_matcher.matches?(ref_name) }
+
+    let(:ref_name) { 'v1.0' }
+
+    it { is_expected.to be_truthy }
+
+    context 'when ref_name is empty' do
+      let(:ref_name) { '' }
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'when ref pattern matches wildcard' do
+      let(:ref_pattern) { 'v*' }
+
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when ref pattern does not match wildcard' do
+      let(:ref_pattern) { 'v2.*' }
+
+      it { is_expected.to be_falsey }
+    end
+  end
+
+  describe '#wildcard?' do
+    subject { ref_matcher.wildcard? }
+
+    it { is_expected.to be_falsey }
+
+    context 'when pattern is a wildcard' do
+      let(:ref_pattern) { 'v*' }
+
+      it { is_expected.to be_truthy }
+    end
+  end
+end