From c830b8e3b782da466d73892728a8ddb869e7d47c Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 27 Mar 2018 14:31:15 +0200 Subject: [PATCH] Client implementation for InfoAttributes Clients can now request the attributes from `$GIT_DIR/info/attributes` through Gitaly. The Gitaly migration is described in gitlab-org/gitaly#1082. The parser algorithm was implemented in a way it could handle both file contents or a File handle, and both were already tested. Other than that, using the boy scout rule, I've removed a class, InfoAttributes, as it was delegating everything to the parser and therefor wasn't really needed in my opinion. --- lib/gitlab/git/attributes_parser.rb | 12 ++--- lib/gitlab/git/info_attributes.rb | 49 ------------------- lib/gitlab/git/repository.rb | 24 ++++++++- .../gitaly_client/repository_service.rb | 9 ++++ spec/lib/gitlab/git/attributes_parser_spec.rb | 12 ----- spec/lib/gitlab/git/info_attributes_spec.rb | 43 ---------------- .../gitaly_client/repository_service_spec.rb | 11 +++++ 7 files changed, 46 insertions(+), 114 deletions(-) delete mode 100644 lib/gitlab/git/info_attributes.rb delete mode 100644 spec/lib/gitlab/git/info_attributes_spec.rb diff --git a/lib/gitlab/git/attributes_parser.rb b/lib/gitlab/git/attributes_parser.rb index d8aeabb6cba..08f4d7d4f5c 100644 --- a/lib/gitlab/git/attributes_parser.rb +++ b/lib/gitlab/git/attributes_parser.rb @@ -3,12 +3,8 @@ module Gitlab # Class for parsing Git attribute files and extracting the attributes for # file patterns. class AttributesParser - def initialize(attributes_data) + def initialize(attributes_data = "") @data = attributes_data || "" - - if @data.is_a?(File) - @patterns = parse_file - end end # Returns all the Git attributes for the given path. @@ -28,7 +24,7 @@ module Gitlab # Returns a Hash containing the file patterns and their attributes. def patterns - @patterns ||= parse_file + @patterns ||= parse_data end # Parses an attribute string. @@ -91,8 +87,8 @@ module Gitlab private - # Parses the Git attributes file. - def parse_file + # Parses the Git attributes file contents. + def parse_data pairs = [] comment = '#' diff --git a/lib/gitlab/git/info_attributes.rb b/lib/gitlab/git/info_attributes.rb deleted file mode 100644 index e79a440950b..00000000000 --- a/lib/gitlab/git/info_attributes.rb +++ /dev/null @@ -1,49 +0,0 @@ -# Gitaly note: JV: not sure what to make of this class. Why does it use -# the full disk path of the repository to look up attributes This is -# problematic in Gitaly, because Gitaly hides the full disk path to the -# repository from gitlab-ce. - -module Gitlab - module Git - # Parses gitattributes at `$GIT_DIR/info/attributes` - # - # Unlike Rugged this parser only needs a single IO call (a call to `open`), - # vastly reducing the time spent in extracting attributes. - # - # This class _only_ supports parsing the attributes file located at - # `$GIT_DIR/info/attributes` as GitLab doesn't use any other files - # (`.gitattributes` is copied to this particular path). - # - # Basic usage: - # - # attributes = Gitlab::Git::InfoAttributes.new(some_repo.path) - # - # attributes.attributes('README.md') # => { "eol" => "lf } - class InfoAttributes - delegate :attributes, :patterns, to: :parser - - # path - The path to the Git repository. - def initialize(path) - @repo_path = File.expand_path(path) - end - - def parser - @parser ||= begin - if File.exist?(attributes_path) - File.open(attributes_path, 'r') do |file_handle| - AttributesParser.new(file_handle) - end - else - AttributesParser.new("") - end - end - end - - private - - def attributes_path - @attributes_path ||= File.join(@repo_path, 'info/attributes') - end - end - end -end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index f1b575bd872..aa429090816 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -105,7 +105,6 @@ module Gitlab ) @path = File.join(storage_path, @relative_path) @name = @relative_path.split("/").last - @attributes = Gitlab::Git::InfoAttributes.new(path) end def ==(other) @@ -993,11 +992,32 @@ module Gitlab raise InvalidRef end + def info_attributes + return @info_attributes if @info_attributes + + content = + gitaly_migrate(:get_info_attributes) do |is_enabled| + if is_enabled + gitaly_repository_client.info_attributes + else + attributes_path = File.join(File.expand_path(@path), 'info', 'attributes') + + if File.exist?(attributes_path) + File.read(attributes_path) + else + "" + end + end + end + + @info_attributes = AttributesParser.new(content) + end + # Returns the Git attributes for the given file path. # # See `Gitlab::Git::Attributes` for more information. def attributes(path) - @attributes.attributes(path) + info_attributes.attributes(path) end def gitattribute(path, name) diff --git a/lib/gitlab/gitaly_client/repository_service.rb b/lib/gitlab/gitaly_client/repository_service.rb index 6441065f5fe..39057beefba 100644 --- a/lib/gitlab/gitaly_client/repository_service.rb +++ b/lib/gitlab/gitaly_client/repository_service.rb @@ -50,6 +50,15 @@ module Gitlab GitalyClient.call(@storage, :repository_service, :apply_gitattributes, request) end + def info_attributes + request = Gitaly::GetInfoAttributesRequest.new(repository: @gitaly_repo) + + response = GitalyClient.call(@storage, :repository_service, :get_info_attributes, request) + response.each_with_object("") do |message, attributes| + attributes << message.attributes + end + end + def fetch_remote(remote, ssh_auth:, forced:, no_tags:, timeout:, prune: true) request = Gitaly::FetchRemoteRequest.new( repository: @gitaly_repo, remote: remote, force: forced, diff --git a/spec/lib/gitlab/git/attributes_parser_spec.rb b/spec/lib/gitlab/git/attributes_parser_spec.rb index 323334e99a5..2d103123998 100644 --- a/spec/lib/gitlab/git/attributes_parser_spec.rb +++ b/spec/lib/gitlab/git/attributes_parser_spec.rb @@ -66,18 +66,6 @@ describe Gitlab::Git::AttributesParser, seed_helper: true do end end - context 'when attributes data is a file handle' do - subject do - File.open(attributes_path, 'r') do |file_handle| - described_class.new(file_handle) - end - end - - it 'returns the attributes as a Hash' do - expect(subject.attributes('test.txt')).to eq({ 'text' => true }) - end - end - context 'when attributes data is nil' do let(:data) { nil } diff --git a/spec/lib/gitlab/git/info_attributes_spec.rb b/spec/lib/gitlab/git/info_attributes_spec.rb deleted file mode 100644 index ea84909c3e0..00000000000 --- a/spec/lib/gitlab/git/info_attributes_spec.rb +++ /dev/null @@ -1,43 +0,0 @@ -require 'spec_helper' - -describe Gitlab::Git::InfoAttributes, seed_helper: true do - let(:path) do - File.join(SEED_STORAGE_PATH, 'with-git-attributes.git') - end - - subject { described_class.new(path) } - - describe '#attributes' do - context 'using a path with attributes' do - it 'returns the attributes as a Hash' do - expect(subject.attributes('test.txt')).to eq({ 'text' => true }) - end - - it 'returns an empty Hash for a defined path without attributes' do - expect(subject.attributes('bla/bla.txt')).to eq({}) - end - end - end - - describe '#parser' do - it 'parses a file with entries' do - expect(subject.patterns).to be_an_instance_of(Hash) - expect(subject.patterns["/*.txt"]).to eq({ 'text' => true }) - end - - it 'does not parse anything when the attributes file does not exist' do - expect(File).to receive(:exist?) - .with(File.join(path, 'info/attributes')) - .and_return(false) - - expect(subject.patterns).to eq({}) - end - - it 'does not parse attributes files with unsupported encoding' do - path = File.join(SEED_STORAGE_PATH, 'with-invalid-git-attributes.git') - subject = described_class.new(path) - - expect(subject.patterns).to eq({}) - end - end -end diff --git a/spec/lib/gitlab/gitaly_client/repository_service_spec.rb b/spec/lib/gitlab/gitaly_client/repository_service_spec.rb index 21592688bf0..074323d47d2 100644 --- a/spec/lib/gitlab/gitaly_client/repository_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/repository_service_spec.rb @@ -84,6 +84,17 @@ describe Gitlab::GitalyClient::RepositoryService do end end + describe '#info_attributes' do + it 'reads the info attributes' do + expect_any_instance_of(Gitaly::RepositoryService::Stub) + .to receive(:get_info_attributes) + .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash)) + .and_return([]) + + client.info_attributes + end + end + describe '#has_local_branches?' do it 'sends a has_local_branches message' do expect_any_instance_of(Gitaly::RepositoryService::Stub)