diff options
author | Alejandro RodrÃguez <alejorro70@gmail.com> | 2017-09-06 23:47:25 +0300 |
---|---|---|
committer | Alejandro RodrÃguez <alejorro70@gmail.com> | 2017-09-06 23:47:25 +0300 |
commit | c2e99b40f71ca7036cc07596aae164e92378263b (patch) | |
tree | 85e912ba19b4091364206b63e55d1facf2e423d9 | |
parent | 21935d85382989e38dd4cc12de55966e0c9b6eba (diff) |
Implement fix for n+1 issue on `flatten_tree` helper
-rw-r--r-- | Gemfile | 2 | ||||
-rw-r--r-- | Gemfile.lock | 4 | ||||
-rw-r--r-- | app/helpers/tree_helper.rb | 4 | ||||
-rw-r--r-- | app/views/projects/tree/_tree_item.html.haml | 2 | ||||
-rw-r--r-- | lib/gitlab/git/tree.rb | 11 | ||||
-rw-r--r-- | lib/gitlab/gitaly_client/commit_service.rb | 6 | ||||
-rw-r--r-- | spec/helpers/tree_helper_spec.rb | 26 | ||||
-rw-r--r-- | spec/lib/gitlab/git/tree_spec.rb | 3 |
8 files changed, 38 insertions, 20 deletions
@@ -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 |