From e28d1fa3cc2e8c2f696f6740c8a8441100542470 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Fri, 29 Apr 2016 22:21:06 +0200 Subject: [PATCH 1/2] Use a query in Project#protected_branch? This changes Project#protected_branch? to use a query to check if a branch is protected, instead of loading all ProtectedBranch records into memory just to check if the list of names includes a given branch name. --- app/models/project.rb | 2 +- spec/models/project_spec.rb | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/app/models/project.rb b/app/models/project.rb index 5c6c36e6b31..bbc929e9bd4 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -764,7 +764,7 @@ class Project < ActiveRecord::Base # Check if current branch name is marked as protected in the system def protected_branch?(branch_name) - protected_branches_names.include?(branch_name) + protected_branches.where(name: branch_name).any? end def developers_can_push_to_protected_branch?(branch_name) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index e33c7d62ff4..5b1cf71337e 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -798,4 +798,18 @@ describe Project, models: true do end end end + + describe '#protected_branch?' do + let(:project) { create(:empty_project) } + + it 'returns true when a branch is a protected branch' do + project.protected_branches.create!(name: 'foo') + + expect(project.protected_branch?('foo')).to eq(true) + end + + it 'returns false when a branch is not a protected branch' do + expect(project.protected_branch?('foo')).to eq(false) + end + end end From b6a18f1093c88e82d2320d4d3f3c15c549b861be Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Fri, 29 Apr 2016 22:33:56 +0200 Subject: [PATCH 2/2] Tweak checking branches in Project#open_branches This changes 4 things: 1. Project#protected_branches_names has been renamed to Project#protected_branch_names. 2. Project#open_branches uses a Set for the branch names as checking values in a Set is faster than checking values in a (large) Array. 3. Some redundant code in Project#open_branches has been removed. 4. Project#protected_branch_names now uses #pluck instead of #map, removing the need for loading entire DB records into memory. --- CHANGELOG | 1 + app/models/project.rb | 16 +++++++--------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 5bd897126b2..121b03be320 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.8.0 (unreleased) + - Project#open_branches has been cleaned up and no longer loads entire records into memory. - Make build status canceled if any of the jobs was canceled and none failed - Remove future dates from contribution calendar graph. - Use ActionDispatch Remote IP for Akismet checking diff --git a/app/models/project.rb b/app/models/project.rb index bbc929e9bd4..af62e8ecd90 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -735,19 +735,17 @@ class Project < ActiveRecord::Base end def open_branches - all_branches = repository.branches + # We're using a Set here as checking values in a large Set is faster than + # checking values in a large Array. + protected_set = Set.new(protected_branch_names) - if protected_branches.present? - all_branches.reject! do |branch| - protected_branches_names.include?(branch.name) - end + repository.branches.reject do |branch| + protected_set.include?(branch.name) end - - all_branches end - def protected_branches_names - @protected_branches_names ||= protected_branches.map(&:name) + def protected_branch_names + @protected_branch_names ||= protected_branches.pluck(:name) end def root_ref?(branch)