Implement fix for n+1 issue on `flatten_tree` helpergitaly-tree-entries-fix

8 files changed, 38 insertions, 20 deletions

diff --git a/Gemfile b/Gemfile
index 0341f2609ad..0221d41fad7 100644
--- a/Gemfile
+++ b/Gemfile
@@ -397,7 +397,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC client
-gem 'gitaly-proto', '~> 0.32.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 0.33.0', require: 'gitaly'
 
 gem 'toml-rb', '~> 0.3.15', require: false
 
diff --git a/Gemfile.lock b/Gemfile.lock
index 320d42b8974..c29d304fd48 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -275,7 +275,7 @@ GEM
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
     gherkin-ruby (0.3.2)
-    gitaly-proto (0.32.0)
+    gitaly-proto (0.33.0)
       google-protobuf (~> 3.1)
       grpc (~> 1.0)
     github-linguist (4.7.6)
@@ -1021,7 +1021,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.2.0)
-  gitaly-proto (~> 0.32.0)
+  gitaly-proto (~> 0.33.0)
   github-linguist (~> 4.7.0)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-markup (~> 1.5.1)
diff --git a/app/helpers/tree_helper.rb b/app/helpers/tree_helper.rb
index e0d3e9b88f3..95dbdc8ec46 100644
--- a/app/helpers/tree_helper.rb
+++ b/app/helpers/tree_helper.rb
@@ -99,7 +99,9 @@ module TreeHelper
   end
 
   # returns the relative path of the first subdir that doesn't have only one directory descendant
-  def flatten_tree(tree)
+  def flatten_tree(root_path, tree)
+    return tree.flat_path.sub(/\A#{root_path}\//, '') if tree.flat_path.present?
+
     subtree = Gitlab::Git::Tree.where(@repository, @commit.id, tree.path)
     if subtree.count == 1 && subtree.first.dir?
       return tree_join(tree.name, flatten_tree(subtree.first))
diff --git a/app/views/projects/tree/_tree_item.html.haml b/app/views/projects/tree/_tree_item.html.haml
index 0c9c8750f2c..56197382a70 100644
--- a/app/views/projects/tree/_tree_item.html.haml
+++ b/app/views/projects/tree/_tree_item.html.haml
@@ -1,7 +1,7 @@
 %tr{ class: "tree-item #{tree_hex_class(tree_item)}" }
   %td.tree-item-file-name
     = tree_icon(type, tree_item.mode, tree_item.name)
-    - path = flatten_tree(tree_item)
+    - path = flatten_tree(@path, tree_item)
     = link_to project_tree_path(@project, tree_join(@id || @commit.id, path)), title: path do
       %span.str-truncated= path
   %td.hidden-xs.tree-commit
diff --git a/lib/gitlab/git/tree.rb b/lib/gitlab/git/tree.rb
index b54962a4456..5cf336af3c6 100644
--- a/lib/gitlab/git/tree.rb
+++ b/lib/gitlab/git/tree.rb
@@ -5,7 +5,7 @@ module Gitlab
     class Tree
       include Gitlab::EncodingHelper
 
-      attr_accessor :id, :root_id, :name, :path, :type,
+      attr_accessor :id, :root_id, :name, :path, :flat_path, :type,
         :mode, :commit_id, :submodule_url
 
       class << self
@@ -19,8 +19,7 @@ module Gitlab
 
           Gitlab::GitalyClient.migrate(:tree_entries) do |is_enabled|
             if is_enabled
-              client = Gitlab::GitalyClient::CommitService.new(repository)
-              client.tree_entries(repository, sha, path)
+              repository.gitaly_commit_client.tree_entries(repository, sha, path)
             else
               tree_entries_from_rugged(repository, sha, path)
             end
@@ -88,7 +87,7 @@ module Gitlab
       end
 
       def initialize(options)
-        %w(id root_id name path type mode commit_id).each do |key|
+        %w(id root_id name path flat_path type mode commit_id).each do |key|
           self.send("#{key}=", options[key.to_sym]) # rubocop:disable GitlabSecurity/PublicSend
         end
       end
@@ -101,6 +100,10 @@ module Gitlab
         encode! @path
       end
 
+      def flat_path
+        encode! @flat_path
+      end
+
       def dir?
         type == :tree
       end
diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb
index 21a32a7e0db..0825a3a7694 100644
--- a/lib/gitlab/gitaly_client/commit_service.rb
+++ b/lib/gitlab/gitaly_client/commit_service.rb
@@ -88,14 +88,14 @@ module Gitlab
 
         response.flat_map do |message|
           message.entries.map do |gitaly_tree_entry|
-            entry_path = gitaly_tree_entry.path.dup
             Gitlab::Git::Tree.new(
               id: gitaly_tree_entry.oid,
               root_id: gitaly_tree_entry.root_oid,
               type: gitaly_tree_entry.type.downcase,
               mode: gitaly_tree_entry.mode.to_s(8),
-              name: File.basename(entry_path),
-              path: entry_path,
+              name: File.basename(gitaly_tree_entry.path),
+              path: GitalyClient.encode(gitaly_tree_entry.path),
+              flat_path: GitalyClient.encode(gitaly_tree_entry.flat_path),
               commit_id: gitaly_tree_entry.commit_oid
             )
           end
diff --git a/spec/helpers/tree_helper_spec.rb b/spec/helpers/tree_helper_spec.rb
index 9523d0f4aa6..d7b66e6f078 100644
--- a/spec/helpers/tree_helper_spec.rb
+++ b/spec/helpers/tree_helper_spec.rb
@@ -3,25 +3,35 @@ require 'spec_helper'
 describe TreeHelper do
   describe 'flatten_tree' do
     let(:project) { create(:project, :repository) }
+    let(:repository) { project.repository }
+    let(:sha) { 'ce369011c189f62c815f5971d096b26759bab0d1' }
+    let(:tree) { repository.tree(sha, 'files') }
+    let(:root_path) { 'files' }
+    let(:tree_item) { tree.entries.find { |entry| entry.path == path } }
 
-    before do
-      @repository = project.repository
-      @commit = project.commit("e56497bb")
-    end
+    subject { flatten_tree(root_path, tree_item) }
 
     context "on a directory containing more than one file/directory" do
-      let(:tree_item) { double(name: "files", path: "files") }
+      let(:path) { 'files/html' }
 
       it "returns the directory name" do
-        expect(flatten_tree(tree_item)).to match('files')
+        expect(subject).to match('html')
       end
     end
 
     context "on a directory containing only one directory" do
-      let(:tree_item) { double(name: "foo", path: "foo") }
+      let(:path) { 'files/flat' }
 
       it "returns the flattened path" do
-        expect(flatten_tree(tree_item)).to match('foo/bar')
+        expect(subject).to match('flat/path/correct')
+      end
+
+      context "with a nested root path" do
+        let(:root_path) { 'files/flat' }
+
+        it "returns the flattened path with the root path suffix removed" do
+          expect(subject).to match('path/correct')
+        end
       end
     end
   end
diff --git a/spec/lib/gitlab/git/tree_spec.rb b/spec/lib/gitlab/git/tree_spec.rb
index c07a2d91768..86f7bcb8e38 100644
--- a/spec/lib/gitlab/git/tree_spec.rb
+++ b/spec/lib/gitlab/git/tree_spec.rb
@@ -20,6 +20,7 @@ describe Gitlab::Git::Tree, seed_helper: true do
       it { expect(dir.commit_id).to eq(SeedRepo::Commit::ID) }
       it { expect(dir.name).to eq('encoding') }
       it { expect(dir.path).to eq('encoding') }
+      it { expect(dir.flat_path).to eq('encoding') }
       it { expect(dir.mode).to eq('40000') }
 
       context :subdir do
@@ -30,6 +31,7 @@ describe Gitlab::Git::Tree, seed_helper: true do
         it { expect(subdir.commit_id).to eq(SeedRepo::Commit::ID) }
         it { expect(subdir.name).to eq('html') }
         it { expect(subdir.path).to eq('files/html') }
+        it { expect(subdir.flat_path).to eq('files/html') }
       end
 
       context :subdir_file do
@@ -40,6 +42,7 @@ describe Gitlab::Git::Tree, seed_helper: true do
         it { expect(subdir_file.commit_id).to eq(SeedRepo::Commit::ID) }
         it { expect(subdir_file.name).to eq('popen.rb') }
         it { expect(subdir_file.path).to eq('files/ruby/popen.rb') }
+        it { expect(subdir_file.flat_path).to eq('files/ruby/popen.rb') }
       end
     end