11 files changed, 103 insertions, 73 deletions

diff --git a/.gitlab/issue_templates/Feature Proposal.md b/.gitlab/issue_templates/Feature Proposal.md
index 1278061a410..5b55eb1374b 100644
--- a/.gitlab/issue_templates/Feature Proposal.md
+++ b/.gitlab/issue_templates/Feature Proposal.md
@@ -1,22 +1,3 @@
-Please read this!
-
-Before opening a new issue, make sure to search for keywords in the issues
-filtered by the "feature proposal" label:
-
-For the Community Edition issue tracker:
-
-- https://gitlab.com/gitlab-org/gitlab-ce/issues?label_name%5B%5D=feature+proposal
-
-For the Enterprise Edition issue tracker:
-
-- https://gitlab.com/gitlab-org/gitlab-ee/issues?label_name%5B%5D=feature+proposal
-
-and verify the issue you're about to submit isn't a duplicate.
-
-Please remove this notice if you're confident your issue isn't a duplicate.
-
-------
-
 ### Description
 
 (Include problem, use cases, benefits, and/or goals)
@@ -25,26 +6,4 @@ Please remove this notice if you're confident your issue isn't a duplicate.
 
 ### Links / references
 
-### Documentation blurb
-
-#### Overview
-
-What is it?
-Why should someone use this feature?
-What is the underlying (business) problem?
-How do you use this feature?
-
-#### Use cases
-
-Who is this for? Provide one or more use cases.
-
-### Feature checklist
-
-Make sure these are completed before closing the issue,
-with a link to the relevant commit.
-
-- [ ] [Feature assurance](https://about.gitlab.com/handbook/product/#feature-assurance)
-- [ ] Documentation
-- [ ] Added to [features.yml](https://gitlab.com/gitlab-com/www-gitlab-com/blob/master/data/features.yml)
-
-/label ~"feature proposal"
\ No newline at end of file
+/label ~"feature proposal"
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index c4e5fd842df..4930b541ba2 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -543,6 +543,7 @@ When having your code reviewed and when reviewing merge requests please take the
    etc.), they should conform to our [Licensing guidelines][license-finder-doc].
    See the instructions in that document for help if your MR fails the
    "license-finder" test with a "Dependencies that need approval" error.
+1. The merge request meets the [definition of done](#definition-of-done).
 
 ## Definition of done
 
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 3ded675bec0..ebbefc51a4f 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -507,7 +507,10 @@ module Ci
     end
 
     def latest_builds_with_artifacts
-      @latest_builds_with_artifacts ||= builds.latest.with_artifacts
+      # We purposely cast the builds to an Array here. Because we always use the
+      # rows if there are more than 0 this prevents us from having to run two
+      # queries: one to get the count and one to get the rows.
+      @latest_builds_with_artifacts ||= builds.latest.with_artifacts.to_a
     end
 
     private
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 26d1bc12232..2bf21cbdcc4 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -217,11 +217,7 @@ class Repository
   def branch_exists?(branch_name)
     return false unless raw_repository
 
-    @branch_exists_memo ||= Hash.new do |hash, key|
-      hash[key] = raw_repository.branch_exists?(key)
-    end
-
-    @branch_exists_memo[branch_name]
+    branch_names.include?(branch_name)
   end
 
   def ref_exists?(ref)
diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb
index d4ba3a028be..f4a5cf75018 100644
--- a/app/uploaders/file_uploader.rb
+++ b/app/uploaders/file_uploader.rb
@@ -26,11 +26,15 @@ class FileUploader < GitlabUploader
   # This is used to build Upload paths dynamically based on the model's current
   # namespace and path, allowing us to ignore renames or transfers.
   #
-  # model - Object that responds to `path_with_namespace`
+  # model - Object that responds to `full_path` and `disk_path`
   #
   # Returns a String without a trailing slash
-  def self.dynamic_path_segment(model)
-    File.join(CarrierWave.root, base_dir, model.disk_path)
+  def self.dynamic_path_segment(project)
+    if project.hashed_storage?(:attachments)
+      File.join(CarrierWave.root, base_dir, project.disk_path)
+    else
+      File.join(CarrierWave.root, base_dir, project.full_path)
+    end
   end
 
   attr_accessor :model
diff --git a/changelogs/unreleased/reduce-queries-for-artifacts-button.yml b/changelogs/unreleased/reduce-queries-for-artifacts-button.yml
new file mode 100644
index 00000000000..f2d469b5a80
--- /dev/null
+++ b/changelogs/unreleased/reduce-queries-for-artifacts-button.yml
@@ -0,0 +1,5 @@
+---
+title: Use arrays in Pipeline#latest_builds_with_artifacts
+merge_request:
+author:
+type: performance
diff --git a/lib/tasks/gitlab/gitaly.rake b/lib/tasks/gitlab/gitaly.rake
index 87835dbe719..f2002d7a426 100644
--- a/lib/tasks/gitlab/gitaly.rake
+++ b/lib/tasks/gitlab/gitaly.rake
@@ -14,8 +14,10 @@ namespace :gitlab do
 
       checkout_or_clone_version(version: version, repo: args.repo, target_dir: args.dir)
 
+      command = %w[/usr/bin/env -u RUBYOPT -u BUNDLE_GEMFILE]
+
       _, status = Gitlab::Popen.popen(%w[which gmake])
-      command = status.zero? ? ['gmake'] : ['make']
+      command << (status.zero? ? 'gmake' : 'make')
 
       command << 'BUNDLE_FLAGS=--no-deployment' if Rails.env.test?
 
@@ -23,7 +25,7 @@ namespace :gitlab do
         create_gitaly_configuration
         # In CI we run scripts/gitaly-test-build instead of this command
         unless ENV['CI'].present?
-          Bundler.with_original_env { run_command!(%w[/usr/bin/env -u RUBYOPT -u BUNDLE_GEMFILE] + command) }
+          Bundler.with_original_env { run_command!(command) }
         end
       end
     end
@@ -80,9 +82,12 @@ namespace :gitlab do
     end
 
     def create_gitaly_configuration
-      File.open("config.toml", "w") do |f|
+      File.open("config.toml", File::WRONLY | File::CREAT | File::EXCL) do |f|
         f.puts gitaly_configuration_toml
       end
+    rescue Errno::EEXIST
+      puts "Skipping config.toml generation:"
+      puts "A configuration file already exists."
     rescue ArgumentError => e
       puts "Skipping config.toml generation:"
       puts e.message
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index b89b0e555d9..3a19a0753e2 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -1502,6 +1502,10 @@ describe Ci::Pipeline, :mailer do
       create(:ci_build, :success, :artifacts, pipeline: pipeline)
     end
 
+    it 'returns an Array' do
+      expect(pipeline.latest_builds_with_artifacts).to be_an_instance_of(Array)
+    end
+
     it 'returns the latest builds' do
       expect(pipeline.latest_builds_with_artifacts).to eq([build])
     end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 8a6aa767ce6..e9e6abb0d5f 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1166,6 +1166,31 @@ describe Repository do
     end
   end
 
+  describe '#branch_exists?' do
+    it 'uses branch_names' do
+      allow(repository).to receive(:branch_names).and_return(['foobar'])
+
+      expect(repository.branch_exists?('foobar')).to eq(true)
+      expect(repository.branch_exists?('master')).to eq(false)
+    end
+  end
+
+  describe '#branch_names', :use_clean_rails_memory_store_caching do
+    let(:fake_branch_names) { ['foobar'] }
+
+    it 'gets cached across Repository instances' do
+      allow(repository.raw_repository).to receive(:branch_names).once.and_return(fake_branch_names)
+
+      expect(repository.branch_names).to eq(fake_branch_names)
+
+      fresh_repository = Project.find(project.id).repository
+      expect(fresh_repository.object_id).not_to eq(repository.object_id)
+
+      expect(fresh_repository.raw_repository).not_to receive(:branch_names)
+      expect(fresh_repository.branch_names).to eq(fake_branch_names)
+    end
+  end
+
   describe '#update_autocrlf_option' do
     describe 'when autocrlf is not already set to :input' do
       before do
diff --git a/spec/tasks/gitlab/gitaly_rake_spec.rb b/spec/tasks/gitlab/gitaly_rake_spec.rb
index 255f0a37ec8..a51374e2645 100644
--- a/spec/tasks/gitlab/gitaly_rake_spec.rb
+++ b/spec/tasks/gitlab/gitaly_rake_spec.rb
@@ -47,7 +47,7 @@ describe 'gitlab:gitaly namespace rake task' do
         stub_env('CI', false)
         FileUtils.mkdir_p(clone_path)
         expect(Dir).to receive(:chdir).with(clone_path).and_call_original
-        allow(Bundler).to receive(:bundle_path).and_return('/fake/bundle_path')
+        allow(Rails.env).to receive(:test?).and_return(false)
       end
 
       context 'gmake is available' do
@@ -57,7 +57,7 @@ describe 'gitlab:gitaly namespace rake task' do
 
         it 'calls gmake in the gitaly directory' do
           expect(Gitlab::Popen).to receive(:popen).with(%w[which gmake]).and_return(['/usr/bin/gmake', 0])
-          expect(main_object).to receive(:run_command!).with(command_preamble + %w[gmake BUNDLE_FLAGS=--no-deployment]).and_return(true)
+          expect(main_object).to receive(:run_command!).with(command_preamble + %w[gmake]).and_return(true)
 
           run_rake_task('gitlab:gitaly:install', clone_path)
         end
@@ -70,18 +70,20 @@ describe 'gitlab:gitaly namespace rake task' do
         end
 
         it 'calls make in the gitaly directory' do
-          expect(main_object).to receive(:run_command!).with(command_preamble + %w[make BUNDLE_FLAGS=--no-deployment]).and_return(true)
+          expect(main_object).to receive(:run_command!).with(command_preamble + %w[make]).and_return(true)
 
           run_rake_task('gitlab:gitaly:install', clone_path)
         end
 
-        context 'when Rails.env is not "test"' do
+        context 'when Rails.env is test' do
+          let(:command) { %w[make BUNDLE_FLAGS=--no-deployment] }
+
           before do
-            allow(Rails.env).to receive(:test?).and_return(false)
+            allow(Rails.env).to receive(:test?).and_return(true)
           end
 
-          it 'calls make in the gitaly directory without BUNDLE_PATH' do
-            expect(main_object).to receive(:run_command!).with(command_preamble + ['make']).and_return(true)
+          it 'calls make in the gitaly directory with --no-deployment flag for bundle' do
+            expect(main_object).to receive(:run_command!).with(command_preamble + command).and_return(true)
 
             run_rake_task('gitlab:gitaly:install', clone_path)
           end
diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb
index f52b2bab05b..fd195d6f9b8 100644
--- a/spec/uploaders/file_uploader_spec.rb
+++ b/spec/uploaders/file_uploader_spec.rb
@@ -28,25 +28,51 @@ describe FileUploader do
   end
 
   context 'hashed storage' do
-    let(:project) { build_stubbed(:project, :hashed) }
+    context 'when rolled out attachments' do
+      let(:project) { build_stubbed(:project, :hashed) }
 
-    describe '.absolute_path' do
-      it 'returns the correct absolute path by building it dynamically' do
-        upload = double(model: project, path: 'secret/foo.jpg')
+      describe '.absolute_path' do
+        it 'returns the correct absolute path by building it dynamically' do
+          upload = double(model: project, path: 'secret/foo.jpg')
 
-        dynamic_segment = project.disk_path
+          dynamic_segment = project.disk_path
 
-        expect(described_class.absolute_path(upload))
-          .to end_with("#{dynamic_segment}/secret/foo.jpg")
+          expect(described_class.absolute_path(upload))
+            .to end_with("#{dynamic_segment}/secret/foo.jpg")
+        end
+      end
+
+      describe "#store_dir" do
+        it "stores in the namespace path" do
+          uploader = described_class.new(project)
+
+          expect(uploader.store_dir).to include(project.disk_path)
+          expect(uploader.store_dir).not_to include("system")
+        end
       end
     end
 
-    describe "#store_dir" do
-      it "stores in the namespace path" do
-        uploader = described_class.new(project)
+    context 'when only repositories are rolled out' do
+      let(:project) { build_stubbed(:project, storage_version: Project::HASHED_STORAGE_FEATURES[:repository]) }
 
-        expect(uploader.store_dir).to include(project.disk_path)
-        expect(uploader.store_dir).not_to include("system")
+      describe '.absolute_path' do
+        it 'returns the correct absolute path by building it dynamically' do
+          upload = double(model: project, path: 'secret/foo.jpg')
+
+          dynamic_segment = project.full_path
+
+          expect(described_class.absolute_path(upload))
+            .to end_with("#{dynamic_segment}/secret/foo.jpg")
+        end
+      end
+
+      describe "#store_dir" do
+        it "stores in the namespace path" do
+          uploader = described_class.new(project)
+
+          expect(uploader.store_dir).to include(project.full_path)
+          expect(uploader.store_dir).not_to include("system")
+        end
       end
     end
   end