Use cached readme blobs where appropriatenick.thomas/gitlab-ce-49591-use-cached-readme-blobs

4 files changed, 74 insertions, 38 deletions

diff --git a/app/models/repository.rb b/app/models/repository.rb
index 6ce480c32c4..37a1dd64052 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -487,7 +487,20 @@ class Repository
   end
 
   def blob_at(sha, path)
-    Blob.decorate(raw_repository.blob_at(sha, path), project)
+    blob = Blob.decorate(raw_repository.blob_at(sha, path), project)
+
+    # Don't attempt to return a special result if there is no blob at all
+    return unless blob
+
+    # Don't attempt to return a special result unless we're looking at HEAD
+    return blob unless head_commit&.sha == sha
+
+    case path
+    when head_tree&.readme_path
+      ReadmeBlob.new(blob, self)
+    else
+      blob
+    end
   rescue Gitlab::Git::Repository::NoRepository
     nil
   end
@@ -569,9 +582,7 @@ class Repository
   cache_method :merge_request_template_names, fallback: []
 
   def readme
-    if readme = tree(:head)&.readme
-      ReadmeBlob.new(readme, self)
-    end
+    head_tree&.readme
   end
 
   def rendered_readme
diff --git a/app/models/tree.rb b/app/models/tree.rb
index 3641c33254c..cd385872171 100644
--- a/app/models/tree.rb
+++ b/app/models/tree.rb
@@ -2,6 +2,7 @@
 
 class Tree
   include Gitlab::MarkupHelper
+  include Gitlab::Utils::StrongMemoize
 
   attr_accessor :repository, :sha, :path, :entries
 
@@ -16,32 +17,36 @@ class Tree
     @entries = Gitlab::Git::Tree.where(git_repo, @sha, @path, recursive)
   end
 
-  def readme
-    return @readme if defined?(@readme)
-
-    available_readmes = blobs.select do |blob|
-      Gitlab::FileDetector.type_of(blob.name) == :readme
-    end
-
-    previewable_readmes = available_readmes.select do |blob|
-      previewable?(blob.name)
-    end
-
-    plain_readmes = available_readmes.select do |blob|
-      plain?(blob.name)
+  def readme_path
+    strong_memoize(:readme_path) do
+      available_readmes = blobs.select do |blob|
+        Gitlab::FileDetector.type_of(blob.name) == :readme
+      end
+
+      previewable_readmes = available_readmes.select do |blob|
+        previewable?(blob.name)
+      end
+
+      plain_readmes = available_readmes.select do |blob|
+        plain?(blob.name)
+      end
+
+      # Prioritize previewable over plain readmes
+      entry = previewable_readmes.first || plain_readmes.first
+      next nil unless entry
+
+      if path == '/'
+        entry.name
+      else
+        File.join(path, entry.name)
+      end
     end
+  end
 
-    # Prioritize previewable over plain readmes
-    readme_tree = previewable_readmes.first || plain_readmes.first
-
-    # Return if we can't preview any of them
-    if readme_tree.nil?
-      return @readme = nil
+  def readme
+    strong_memoize(:readme) do
+      repository.blob_at(sha, readme_path) if readme_path
     end
-
-    readme_path = path == '/' ? readme_tree.name : File.join(path, readme_tree.name)
-
-    @readme = repository.blob_at(sha, readme_path)
   end
 
   def trees
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index a807c336165..88f70a88210 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -527,28 +527,28 @@ describe Project do
   end
 
   describe "#readme_url" do
-    let(:project) { create(:project, :repository, path: "somewhere") }
-
     context 'with a non-existing repository' do
-      it 'returns nil' do
-        allow(project.repository).to receive(:tree).with(:head).and_return(nil)
+      let(:project) { create(:project) }
 
+      it 'returns nil' do
         expect(project.readme_url).to be_nil
       end
     end
 
     context 'with an existing repository' do
       context 'when no README exists' do
-        it 'returns nil' do
-          allow_any_instance_of(Tree).to receive(:readme).and_return(nil)
+        let(:project) { create(:project, :empty_repo) }
 
+        it 'returns nil' do
           expect(project.readme_url).to be_nil
         end
       end
 
       context 'when a README exists' do
+        let(:project) { create(:project, :repository) }
+
         it 'returns the README' do
-          expect(project.readme_url).to eql("#{Gitlab.config.gitlab.url}/#{project.namespace.full_path}/somewhere/blob/master/README.md")
+          expect(project.readme_url).to eq("#{project.web_url}/blob/master/README.md")
         end
       end
     end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index fb16a321e4b..799a60ac62f 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -461,6 +461,24 @@ describe Repository do
 
       it { is_expected.to be_nil }
     end
+
+    context 'regular blob' do
+      subject { repository.blob_at(repository.head_commit.sha, '.gitignore') }
+
+      it { is_expected.to be_an_instance_of(::Blob) }
+    end
+
+    context 'readme blob on HEAD' do
+      subject { repository.blob_at(repository.head_commit.sha, 'README.md') }
+
+      it { is_expected.to be_an_instance_of(::ReadmeBlob) }
+    end
+
+    context 'readme blob not on HEAD' do
+      subject { repository.blob_at(repository.find_branch('feature').target, 'README.md') }
+
+      it { is_expected.to be_an_instance_of(::Blob) }
+    end
   end
 
   describe '#merged_to_root_ref?' do
@@ -1922,23 +1940,25 @@ describe Repository do
 
   describe '#readme', :use_clean_rails_memory_store_caching do
     context 'with a non-existing repository' do
-      it 'returns nil' do
-        allow(repository).to receive(:tree).with(:head).and_return(nil)
+      let(:project) { create(:project) }
 
+      it 'returns nil' do
         expect(repository.readme).to be_nil
       end
     end
 
     context 'with an existing repository' do
       context 'when no README exists' do
-        it 'returns nil' do
-          allow_any_instance_of(Tree).to receive(:readme).and_return(nil)
+        let(:project) { create(:project, :empty_repo) }
 
+        it 'returns nil' do
           expect(repository.readme).to be_nil
         end
       end
 
       context 'when a README exists' do
+        let(:project) { create(:project, :repository) }
+
         it 'returns the README' do
           expect(repository.readme).to be_an_instance_of(ReadmeBlob)
         end