From 41b8d22631053e66043d05695d65f4961b91efd8 Mon Sep 17 00:00:00 2001
From: Yorick Peterse <yorickpeterse@gmail.com>
Date: Fri, 18 Mar 2016 12:47:36 +0100
Subject: Tweaked performance of Issue#related_branches

Requesting the branch names of a repository works even when it's empty,
thus there's no need to explicitly check for an empty repository.
Removing this check cuts down the amount of Git operations which in turn
cuts down request timings a bit. The regular expression used to compare
branches was also moved out of the loop so it's created only once.
---
 app/models/issue.rb       | 6 +++---
 spec/models/issue_spec.rb | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/app/models/issue.rb b/app/models/issue.rb
index 5347d4fa1be..af80dee1dc4 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -105,9 +105,9 @@ class Issue < ActiveRecord::Base
   end
 
   def related_branches
-    return [] if self.project.empty_repo?
-    
-    self.project.repository.branch_names.select { |branch| branch.end_with?("-#{iid}") }
+    project.repository.branch_names.select do |branch|
+      branch.end_with?("-#{iid}")
+    end
   end
 
   # Reset issue events cache
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index 540a62eb1f8..77d046fb0eb 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -133,9 +133,9 @@ describe Issue, models: true do
   describe '#related_branches' do
     it "selects the right branches" do
       allow(subject.project.repository).to receive(:branch_names).
-                                    and_return(["mpempe", "#{subject.iid}mepmep", subject.to_branch_name])
+        and_return(["mpempe", "#{subject.iid}mepmep", subject.to_branch_name])
 
-      expect(subject.related_branches).to eq [subject.to_branch_name]
+      expect(subject.related_branches).to eq([subject.to_branch_name])
     end
   end
 
-- 
cgit v1.2.1


From 68a4c98f5074bd34f0178f2f967153c8d5c71237 Mon Sep 17 00:00:00 2001
From: Yorick Peterse <yorickpeterse@gmail.com>
Date: Fri, 18 Mar 2016 15:31:19 +0100
Subject: Cache output of Repository#exists?

This caches the output of Repository#exists? in Redis while making sure
it's flushed properly when creating new repositories, deleting them,
etc.

For the ProjectWiki tests to work I had to make ProjectWiki#create_repo!
public as testing private methods in RSpec is a bit of a pain.
---
 app/models/project.rb                       |  1 +
 app/models/project_wiki.rb                  | 12 ++++++---
 app/models/repository.rb                    | 25 ++++++++++++++----
 app/workers/repository_fork_worker.rb       |  5 ++--
 spec/models/project_spec.rb                 | 41 +++++++++++++++++++++++++++++
 spec/models/project_wiki_spec.rb            | 12 +++++++++
 spec/models/repository_spec.rb              | 30 +++++++++++++++++++++
 spec/workers/repository_fork_worker_spec.rb | 19 ++++++++++---
 8 files changed, 130 insertions(+), 15 deletions(-)

diff --git a/app/models/project.rb b/app/models/project.rb
index 412c6c6732d..fd36dca0cad 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -876,6 +876,7 @@ class Project < ActiveRecord::Base
     # Forked import is handled asynchronously
     unless forked?
       if gitlab_shell.add_repository(path_with_namespace)
+        repository.after_create
         true
       else
         errors.add(:base, 'Failed to create repository via gitlab-shell')
diff --git a/app/models/project_wiki.rb b/app/models/project_wiki.rb
index 59b1b86d1fb..7c1a61bb0bf 100644
--- a/app/models/project_wiki.rb
+++ b/app/models/project_wiki.rb
@@ -123,23 +123,27 @@ class ProjectWiki
   end
 
   def repository
-    Repository.new(path_with_namespace, @project)
+    @repository ||= Repository.new(path_with_namespace, @project)
   end
 
   def default_branch
     wiki.class.default_ref
   end
 
-  private
-
   def create_repo!
     if init_repo(path_with_namespace)
-      Gollum::Wiki.new(path_to_repo)
+      wiki = Gollum::Wiki.new(path_to_repo)
     else
       raise CouldNotCreateWikiError
     end
+
+    repository.after_create
+
+    wiki
   end
 
+  private
+
   def init_repo(path_with_namespace)
     gitlab_shell.add_repository(path_with_namespace)
   end
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 25d24493f6e..13154eb4205 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -42,12 +42,15 @@ class Repository
   end
 
   def exists?
-    return false unless raw_repository
+    return @exists unless @exists.nil?
 
-    raw_repository.rugged
-    true
-  rescue Gitlab::Git::Repository::NoRepository
-    false
+    @exists = cache.fetch(:exists?) do
+      begin
+        raw_repository && raw_repository.rugged ? true : false
+      rescue Gitlab::Git::Repository::NoRepository
+        false
+      end
+    end
   end
 
   def empty?
@@ -320,12 +323,23 @@ class Repository
     @avatar = nil
   end
 
+  def expire_exists_cache
+    cache.expire(:exists?)
+    @exists = nil
+  end
+
+  # Runs code after a repository has been created.
+  def after_create
+    expire_exists_cache
+  end
+
   # Runs code just before a repository is deleted.
   def before_delete
     expire_cache if exists?
 
     expire_root_ref_cache
     expire_emptiness_caches
+    expire_exists_cache
   end
 
   # Runs code just before the HEAD of a repository is changed.
@@ -351,6 +365,7 @@ class Repository
   # Runs code after a repository has been forked/imported.
   def after_import
     expire_emptiness_caches
+    expire_exists_cache
   end
 
   # Runs code after a new commit has been pushed.
diff --git a/app/workers/repository_fork_worker.rb b/app/workers/repository_fork_worker.rb
index 21d311579e3..f9e32337983 100644
--- a/app/workers/repository_fork_worker.rb
+++ b/app/workers/repository_fork_worker.rb
@@ -20,14 +20,15 @@ class RepositoryForkWorker
       return
     end
 
+    project.repository.after_import
+
     unless project.valid_repo?
-      logger.error("Project #{id} had an invalid repository after fork")
+      logger.error("Project #{project_id} had an invalid repository after fork")
       project.update(import_error: "The forked repository is invalid.")
       project.import_fail
       return
     end
 
-    project.repository.after_import
     project.import_finish
   end
 end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index b8b9a455b83..624022c1dda 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -720,4 +720,45 @@ describe Project, models: true do
       expect(described_class.search_by_title('KITTENS')).to eq([project])
     end
   end
+
+  describe '#create_repository' do
+    let(:project) { create(:project) }
+    let(:shell) { Gitlab::Shell.new }
+
+    before do
+      allow(project).to receive(:gitlab_shell).and_return(shell)
+    end
+
+    context 'using a regular repository' do
+      it 'creates the repository' do
+        expect(shell).to receive(:add_repository).
+          with(project.path_with_namespace).
+          and_return(true)
+
+        expect(project.repository).to receive(:after_create)
+
+        expect(project.create_repository).to eq(true)
+      end
+
+      it 'adds an error if the repository could not be created' do
+        expect(shell).to receive(:add_repository).
+          with(project.path_with_namespace).
+          and_return(false)
+
+        expect(project.repository).not_to receive(:after_create)
+
+        expect(project.create_repository).to eq(false)
+        expect(project.errors).not_to be_empty
+      end
+    end
+
+    context 'using a forked repository' do
+      it 'does nothing' do
+        expect(project).to receive(:forked?).and_return(true)
+        expect(shell).not_to receive(:add_repository)
+
+        project.create_repository
+      end
+    end
+  end
 end
diff --git a/spec/models/project_wiki_spec.rb b/spec/models/project_wiki_spec.rb
index a2085df5bcd..532e3f013fd 100644
--- a/spec/models/project_wiki_spec.rb
+++ b/spec/models/project_wiki_spec.rb
@@ -244,6 +244,18 @@ describe ProjectWiki, models: true do
     end
   end
 
+  describe '#create_repo!' do
+    it 'creates a repository' do
+      expect(subject).to receive(:init_repo).
+        with(subject.path_with_namespace).
+        and_return(true)
+
+      expect(subject.repository).to receive(:after_create)
+
+      expect(subject.create_repo!).to be_an_instance_of(Gollum::Wiki)
+    end
+  end
+
   private
 
   def create_temp_repo(path)
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index a57229a4fdf..7eac70ae948 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -537,6 +537,12 @@ describe Repository, models: true do
 
         repository.before_delete
       end
+
+      it 'flushes the exists cache' do
+        expect(repository).to receive(:expire_exists_cache)
+
+        repository.before_delete
+      end
     end
 
     describe 'when a repository exists' do
@@ -593,6 +599,12 @@ describe Repository, models: true do
 
       repository.after_import
     end
+
+    it 'flushes the exists cache' do
+      expect(repository).to receive(:expire_exists_cache)
+
+      repository.after_import
+    end
   end
 
   describe '#after_push_commit' do
@@ -619,6 +631,14 @@ describe Repository, models: true do
     end
   end
 
+  describe '#after_create' do
+    it 'flushes the exists cache' do
+      expect(repository).to receive(:expire_exists_cache)
+
+      repository.after_create
+    end
+  end
+
   describe "#main_language" do
     it 'shows the main language of the project' do
       expect(repository.main_language).to eq("Ruby")
@@ -781,6 +801,16 @@ describe Repository, models: true do
     end
   end
 
+  describe '#expire_exists_cache' do
+    let(:cache) { repository.send(:cache) }
+
+    it 'expires the cache' do
+      expect(cache).to receive(:expire).with(:exists?)
+
+      repository.expire_exists_cache
+    end
+  end
+
   describe '#build_cache' do
     let(:cache) { repository.send(:cache) }
 
diff --git a/spec/workers/repository_fork_worker_spec.rb b/spec/workers/repository_fork_worker_spec.rb
index 172537474ee..4ef05eb29d2 100644
--- a/spec/workers/repository_fork_worker_spec.rb
+++ b/spec/workers/repository_fork_worker_spec.rb
@@ -3,12 +3,17 @@ require 'spec_helper'
 describe RepositoryForkWorker do
   let(:project) { create(:project) }
   let(:fork_project) { create(:project, forked_from_project: project) }
+  let(:shell) { Gitlab::Shell.new }
 
   subject { RepositoryForkWorker.new }
 
+  before do
+    allow(subject).to receive(:gitlab_shell).and_return(shell)
+  end
+
   describe "#perform" do
     it "creates a new repository from a fork" do
-      expect_any_instance_of(Gitlab::Shell).to receive(:fork_repository).with(
+      expect(shell).to receive(:fork_repository).with(
         project.path_with_namespace,
         fork_project.namespace.path
       ).and_return(true)
@@ -19,20 +24,26 @@ describe RepositoryForkWorker do
         fork_project.namespace.path)
     end
 
-    it 'flushes the empty caches' do
-      expect_any_instance_of(Gitlab::Shell).to receive(:fork_repository).
+    it 'flushes various caches' do
+      expect(shell).to receive(:fork_repository).
         with(project.path_with_namespace, fork_project.namespace.path).
         and_return(true)
 
       expect_any_instance_of(Repository).to receive(:expire_emptiness_caches).
         and_call_original
 
+      expect_any_instance_of(Repository).to receive(:expire_exists_cache).
+        and_call_original
+
       subject.perform(project.id, project.path_with_namespace,
                       fork_project.namespace.path)
     end
 
     it "handles bad fork" do
-      expect_any_instance_of(Gitlab::Shell).to receive(:fork_repository).and_return(false)
+      expect(shell).to receive(:fork_repository).and_return(false)
+
+      expect(subject.logger).to receive(:error)
+
       subject.perform(
         project.id,
         project.path_with_namespace,
-- 
cgit v1.2.1


From 295fdf720ae664200fce8c316fbda1931f4ae61d Mon Sep 17 00:00:00 2001
From: Yorick Peterse <yorickpeterse@gmail.com>
Date: Sat, 19 Mar 2016 15:35:24 +0100
Subject: Create repositories in IssuesController specs

In the real world a project always has a repository. This fact allows
code such as Issue#related_branches to work without explicitly checking
if a repository exists.
---
 spec/controllers/projects/issues_controller_spec.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 2cd81231144..a49bd8960ee 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -2,7 +2,7 @@ require('spec_helper')
 
 describe Projects::IssuesController do
   describe "GET #index" do
-    let(:project) { create(:project) }
+    let(:project) { create(:project_empty_repo) }
     let(:user) { create(:user) }
     let(:issue) { create(:issue, project: project) }
 
@@ -41,7 +41,7 @@ describe Projects::IssuesController do
   end
 
   describe 'Confidential Issues' do
-    let(:project) { create(:empty_project, :public) }
+    let(:project) { create(:project_empty_repo, :public) }
     let(:assignee) { create(:assignee) }
     let(:author) { create(:user) }
     let(:non_member) { create(:user) }
-- 
cgit v1.2.1


From fc5ff7af61e14b89b67febf4167a03a0133e55ed Mon Sep 17 00:00:00 2001
From: Yorick Peterse <yorickpeterse@gmail.com>
Date: Sat, 19 Mar 2016 18:11:46 +0100
Subject: Create repositories in Spinach issues tests

Similar to ad90dba5185e30883d5ad6008c166b0df0108ebf we always have a
repository in the real world, so let's also create one in our Spinach
tests.
---
 features/steps/project/issues/issues.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/features/steps/project/issues/issues.rb b/features/steps/project/issues/issues.rb
index 8c31fa890b2..307d32fd610 100644
--- a/features/steps/project/issues/issues.rb
+++ b/features/steps/project/issues/issues.rb
@@ -240,7 +240,7 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps
   end
 
   step 'empty project "Empty Project"' do
-    create :empty_project, name: 'Empty Project', namespace: @user.namespace
+    create :project_empty_repo, name: 'Empty Project', namespace: @user.namespace
   end
 
   When 'I visit empty project page' do
-- 
cgit v1.2.1


From 75aaf91cb1d08c4350e2881b18118faf30e1f310 Mon Sep 17 00:00:00 2001
From: Yorick Peterse <yorickpeterse@gmail.com>
Date: Mon, 21 Mar 2016 11:40:13 +0100
Subject: Create SSH keys for SSH clone Spinach tests

These tests would check if the "This project is empty" banner would
contain SSH clone URLs. Oddly enough this should have never passed (as
far as I can tell) as SSH clone URLs in this banner are _only_ displayed
if the current user has at least 1 SSH key attached. Since the tests
never seem to create any they never should have passed, yet somehow they
did. To solve this the Spinach tests in question now ensure at least 1
SSH key is present.
---
 features/project/issues/issues.feature  | 1 +
 features/steps/project/issues/issues.rb | 1 +
 2 files changed, 2 insertions(+)

diff --git a/features/project/issues/issues.feature b/features/project/issues/issues.feature
index ff21c7d1b83..de7e2b37725 100644
--- a/features/project/issues/issues.feature
+++ b/features/project/issues/issues.feature
@@ -160,6 +160,7 @@ Feature: Project Issues
 
   Scenario: Issues on empty project
     Given empty project "Empty Project"
+    And I have an ssh key
     When I visit empty project page
     And I see empty project details with ssh clone info
     When I visit empty project's issues page
diff --git a/features/steps/project/issues/issues.rb b/features/steps/project/issues/issues.rb
index 307d32fd610..aff5ca676be 100644
--- a/features/steps/project/issues/issues.rb
+++ b/features/steps/project/issues/issues.rb
@@ -5,6 +5,7 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps
   include SharedNote
   include SharedPaths
   include SharedMarkdown
+  include SharedUser
 
   step 'I should see "Release 0.4" in issues' do
     expect(page).to have_content "Release 0.4"
-- 
cgit v1.2.1