Commit 8f763879 authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'gitaly-tree-entries-fix' into 'master'

Implement fix for n+1 issue on `flatten_tree` helper

Closes gitaly#530

See merge request !14097
parents 424c4673 c2e99b40
......@@ -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
......
......@@ -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)
......@@ -1022,7 +1022,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)
......
......@@ -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))
......
%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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment