From cb4107d38d965e91df99102ec43a13f34bd75c1c Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Mon, 4 Mar 2019 09:22:44 +0000 Subject: [PATCH 1/8] Merge branch 'sh-rugged-find-commit' into 'master' Bring back Rugged implementation of find_commit See merge request gitlab-org/gitlab-ce!25477 --- .../unreleased/sh-rugged-find-commit.yml | 5 ++ doc/administration/high_availability/nfs.md | 22 +++++++ doc/development/gitaly.md | 38 +++++++++++ lib/gitlab/git/commit.rb | 17 ++++- lib/gitlab/git/ref.rb | 1 + lib/gitlab/git/repository.rb | 1 + lib/gitlab/git/rugged_impl/commit.rb | 65 +++++++++++++++++++ lib/gitlab/git/rugged_impl/ref.rb | 20 ++++++ lib/gitlab/git/rugged_impl/repository.rb | 48 ++++++++++++++ lib/gitlab/gitaly_client/storage_settings.rb | 8 +++ lib/tasks/gitlab/features.rake | 24 +++++++ scripts/lint-rugged | 8 ++- spec/lib/gitlab/git/commit_spec.rb | 16 ++++- .../gitaly_client/storage_settings_spec.rb | 10 +++ spec/spec_helper.rb | 9 ++- 15 files changed, 286 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/sh-rugged-find-commit.yml create mode 100644 lib/gitlab/git/rugged_impl/commit.rb create mode 100644 lib/gitlab/git/rugged_impl/ref.rb create mode 100644 lib/gitlab/git/rugged_impl/repository.rb create mode 100644 lib/tasks/gitlab/features.rake diff --git a/changelogs/unreleased/sh-rugged-find-commit.yml b/changelogs/unreleased/sh-rugged-find-commit.yml new file mode 100644 index 00000000000..85b5936c9ba --- /dev/null +++ b/changelogs/unreleased/sh-rugged-find-commit.yml @@ -0,0 +1,5 @@ +--- +title: Bring back Rugged implementation of find_commit +merge_request: 25477 +author: +type: fixed diff --git a/doc/administration/high_availability/nfs.md b/doc/administration/high_availability/nfs.md index 481eb692674..ba538fda4b9 100644 --- a/doc/administration/high_availability/nfs.md +++ b/doc/administration/high_availability/nfs.md @@ -37,6 +37,28 @@ options: circumstances it could lead to data loss if a failure occurs before data has synced. +### Improving NFS performance with GitLab + +If you are using NFS to share Git data, we recommend that you enable a +number of feature flags that will allow GitLab application processes to +access Git data directly instead of going through the [Gitaly +service](../gitaly/index.md). Depending on your workload and disk +performance, these flags may help improve performance. See [the +issue](https://gitlab.com/gitlab-org/gitlab-ce/issues/57317) for more +details. + +To do this, run the Rake task: + +```sh +gitlab-rake gitlab:features:enable_rugged +``` + +If you need to undo this setting for some reason, run: + +```sh +gitlab-rake gitlab:features:disable_rugged +``` + ### Known issues On some customer systems, we have seen NFS clients slow precipitously due to diff --git a/doc/development/gitaly.md b/doc/development/gitaly.md index fdae69bddd7..8c806c433a1 100644 --- a/doc/development/gitaly.md +++ b/doc/development/gitaly.md @@ -56,6 +56,44 @@ If your test-suite is failing with Gitaly issues, as a first step, try running: rm -rf tmp/tests/gitaly ``` +## Legacy Rugged code + +While Gitaly can handle all Git access, many of GitLab customers still +run Gitaly atop NFS. The legacy Rugged implementation for Git calls may +be faster than the Gitaly RPC due to N+1 Gitaly calls and other +reasons. See [the +issue](https://gitlab.com/gitlab-org/gitlab-ce/issues/57317) for more +details. + +Until GitLab has eliminated most of these inefficiencies or the use of +NFS is discontinued for Git data, Rugged implementations of some of the +most commonly-used RPCs can be enabled via feature flags: + +* `rugged_find_commit` +* `rugged_get_tree_entries` +* `rugged_tree_entry` +* `rugged_commit_is_ancestor` + +A convenience Rake task can be used to enable or disable these flags +all together. To enable: + +```sh +bundle exec rake gitlab:features:enable_rugged +``` + +To disable: + +```sh +bundle exec rake gitlab:features:disable_rugged +``` + +Most of this code exists in the `lib/gitlab/git/rugged_impl` directory. + +NOTE: **Note:** You should NOT need to add or modify code related to +Rugged unless explicitly discussed with the [Gitaly +Team](https://gitlab.com/groups/gl-gitaly/group_members). This code will +NOT work on GitLab.com or other GitLab instances that do not use NFS. + ## `TooManyInvocationsError` errors During development and testing, you may experience `Gitlab::GitalyClient::TooManyInvocationsError` failures. diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb index 2820491b65d..eee45ba6e19 100644 --- a/lib/gitlab/git/commit.rb +++ b/lib/gitlab/git/commit.rb @@ -3,6 +3,7 @@ module Gitlab module Git class Commit include Gitlab::EncodingHelper + prepend Gitlab::Git::RuggedImpl::Commit extend Gitlab::Git::WrapsGitalyErrors attr_accessor :raw_commit, :head @@ -60,15 +61,19 @@ module Gitlab # This saves us an RPC round trip. return nil if commit_id.include?(':') - commit = wrapped_gitaly_errors do - repo.gitaly_commit_client.find_commit(commit_id) - end + commit = find_commit(repo, commit_id) decorate(repo, commit) if commit rescue Gitlab::Git::CommandError, Gitlab::Git::Repository::NoRepository, ArgumentError nil end + def find_commit(repo, commit_id) + wrapped_gitaly_errors do + repo.gitaly_commit_client.find_commit(commit_id) + end + end + # Get last commit for HEAD # # Ex. @@ -199,6 +204,10 @@ module Gitlab @repository = repository @head = head + init_commit(raw_commit) + end + + def init_commit(raw_commit) case raw_commit when Hash init_from_hash(raw_commit) @@ -414,3 +423,5 @@ module Gitlab end end end + +Gitlab::Git::Commit.singleton_class.prepend Gitlab::Git::RuggedImpl::Commit::ClassMethods diff --git a/lib/gitlab/git/ref.rb b/lib/gitlab/git/ref.rb index 31a280155bd..142262f0e49 100644 --- a/lib/gitlab/git/ref.rb +++ b/lib/gitlab/git/ref.rb @@ -2,6 +2,7 @@ module Gitlab module Git class Ref include Gitlab::EncodingHelper + include Gitlab::Git::RuggedImpl::Ref # Branch or tag name # without "refs/tags|heads" prefix diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 756bdb20774..b6c7a12de35 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -9,6 +9,7 @@ module Gitlab include Gitlab::Git::WrapsGitalyErrors include Gitlab::EncodingHelper include Gitlab::Utils::StrongMemoize + include Gitlab::Git::RuggedImpl::Repository SEARCH_CONTEXT_LINES = 3 REV_LIST_COMMIT_LIMIT = 2_000 diff --git a/lib/gitlab/git/rugged_impl/commit.rb b/lib/gitlab/git/rugged_impl/commit.rb new file mode 100644 index 00000000000..251802878c3 --- /dev/null +++ b/lib/gitlab/git/rugged_impl/commit.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +# NOTE: This code is legacy. Do not add/modify code here unless you have +# discussed with the Gitaly team. See +# https://docs.gitlab.com/ee/development/gitaly.html#legacy-rugged-code +# for more details. + +# rubocop:disable Gitlab/ModuleWithInstanceVariables +module Gitlab + module Git + module RuggedImpl + module Commit + module ClassMethods + extend ::Gitlab::Utils::Override + + def rugged_find(repo, commit_id) + obj = repo.rev_parse_target(commit_id) + + obj.is_a?(::Rugged::Commit) ? obj : nil + rescue ::Rugged::Error + nil + end + + override :find_commit + def find_commit(repo, commit_id) + if Feature.enabled?(:rugged_find_commit) + rugged_find(repo, commit_id) + else + super + end + end + end + + extend ::Gitlab::Utils::Override + + override :init_commit + def init_commit(raw_commit) + case raw_commit + when ::Rugged::Commit + init_from_rugged(raw_commit) + else + super + end + end + + def init_from_rugged(commit) + author = commit.author + committer = commit.committer + + @raw_commit = commit + @id = commit.oid + @message = commit.message + @authored_date = author[:time] + @committed_date = committer[:time] + @author_name = author[:name] + @author_email = author[:email] + @committer_name = committer[:name] + @committer_email = committer[:email] + @parent_ids = commit.parents.map(&:oid) + end + end + end + end +end +# rubocop:enable Gitlab/ModuleWithInstanceVariables diff --git a/lib/gitlab/git/rugged_impl/ref.rb b/lib/gitlab/git/rugged_impl/ref.rb new file mode 100644 index 00000000000..b553e82dc47 --- /dev/null +++ b/lib/gitlab/git/rugged_impl/ref.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +# NOTE: This code is legacy. Do not add/modify code here unless you have +# discussed with the Gitaly team. See +# https://docs.gitlab.com/ee/development/gitaly.html#legacy-rugged-code +# for more details. + +module Gitlab + module Git + module RuggedImpl + module Ref + def self.dereference_object(object) + object = object.target while object.is_a?(::Rugged::Tag::Annotation) + + object + end + end + end + end +end diff --git a/lib/gitlab/git/rugged_impl/repository.rb b/lib/gitlab/git/rugged_impl/repository.rb new file mode 100644 index 00000000000..135c47017b3 --- /dev/null +++ b/lib/gitlab/git/rugged_impl/repository.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +# NOTE: This code is legacy. Do not add/modify code here unless you have +# discussed with the Gitaly team. See +# https://docs.gitlab.com/ee/development/gitaly.html#legacy-rugged-code +# for more details. + +# rubocop:disable Gitlab/ModuleWithInstanceVariables +module Gitlab + module Git + module RuggedImpl + module Repository + FEATURE_FLAGS = %i(rugged_find_commit).freeze + + def alternate_object_directories + relative_object_directories.map { |d| File.join(path, d) } + end + + ALLOWED_OBJECT_RELATIVE_DIRECTORIES_VARIABLES = %w[ + GIT_OBJECT_DIRECTORY_RELATIVE + GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE + ].freeze + + def relative_object_directories + Gitlab::Git::HookEnv.all(gl_repository).values_at(*ALLOWED_OBJECT_RELATIVE_DIRECTORIES_VARIABLES).flatten.compact + end + + def rugged + @rugged ||= ::Rugged::Repository.new(path, alternates: alternate_object_directories) + rescue ::Rugged::RepositoryError, ::Rugged::OSError + raise ::Gitlab::Git::Repository::NoRepository.new('no repository for such path') + end + + def cleanup + @rugged&.close + end + + # Return the object that +revspec+ points to. If +revspec+ is an + # annotated tag, then return the tag's target instead. + def rev_parse_target(revspec) + obj = rugged.rev_parse(revspec) + Ref.dereference_object(obj) + end + end + end + end +end +# rubocop:enable Gitlab/ModuleWithInstanceVariables diff --git a/lib/gitlab/gitaly_client/storage_settings.rb b/lib/gitlab/gitaly_client/storage_settings.rb index 26d1f53f26c..488c6d79e51 100644 --- a/lib/gitlab/gitaly_client/storage_settings.rb +++ b/lib/gitlab/gitaly_client/storage_settings.rb @@ -30,11 +30,19 @@ module Gitlab end def self.disk_access_denied? + return false if rugged_enabled? + !temporarily_allowed?(ALLOW_KEY) && GitalyClient.feature_enabled?(DISK_ACCESS_DENIED_FLAG) rescue false # Err on the side of caution, don't break gitlab for people end + def self.rugged_enabled? + Gitlab::Git::RuggedImpl::Repository::FEATURE_FLAGS.any? do |flag| + Feature.enabled?(flag) + end + end + def initialize(storage) raise InvalidConfigurationError, "expected a Hash, got a #{storage.class.name}" unless storage.is_a?(Hash) raise InvalidConfigurationError, INVALID_STORAGE_MESSAGE unless storage.has_key?('path') diff --git a/lib/tasks/gitlab/features.rake b/lib/tasks/gitlab/features.rake new file mode 100644 index 00000000000..d115961108e --- /dev/null +++ b/lib/tasks/gitlab/features.rake @@ -0,0 +1,24 @@ +namespace :gitlab do + namespace :features do + desc 'GitLab | Features | Enable direct Git access via Rugged for NFS' + task enable_rugged: :environment do + set_rugged_feature_flags(true) + puts 'All Rugged feature flags were enabled.' + end + + task disable_rugged: :environment do + set_rugged_feature_flags(false) + puts 'All Rugged feature flags were disabled.' + end + end + + def set_rugged_feature_flags(status) + Gitlab::Git::RuggedImpl::Repository::FEATURE_FLAGS.each do |flag| + if status + Feature.enable(flag) + else + Feature.disable(flag) + end + end + end +end diff --git a/scripts/lint-rugged b/scripts/lint-rugged index 22e3e1f1505..9466c62a415 100755 --- a/scripts/lint-rugged +++ b/scripts/lint-rugged @@ -5,12 +5,18 @@ ALLOWED = [ 'lib/gitlab/bare_repository_import/repository.rb', # Needed to avoid using the git binary to validate a branch name - 'lib/gitlab/git_ref_validator.rb' + 'lib/gitlab/git_ref_validator.rb', + + # Reverted Rugged calls due to Gitaly atop NFS performance + # See https://docs.gitlab.com/ee/development/gitaly.html#legacy-rugged-code. + 'lib/gitlab/git/rugged_impl/', + 'lib/gitlab/gitaly_client/storage_settings.rb' ].freeze rugged_lines = IO.popen(%w[git grep -i -n rugged -- app config lib], &:read).lines rugged_lines = rugged_lines.select { |l| /^[^:]*\.rb:/ =~ l } rugged_lines = rugged_lines.reject { |l| l.start_with?(*ALLOWED) } +rugged_lines = rugged_lines.reject { |l| /(include|prepend) Gitlab::Git::RuggedImpl/ =~ l} rugged_lines = rugged_lines.reject do |line| code, _comment = line.split('# ', 2) code !~ /rugged/i diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb index 9ef27081f98..44f3e015836 100644 --- a/spec/lib/gitlab/git/commit_spec.rb +++ b/spec/lib/gitlab/git/commit_spec.rb @@ -112,7 +112,7 @@ describe Gitlab::Git::Commit, :seed_helper do end context 'Class methods' do - describe '.find' do + shared_examples '.find' do it "should return first head commit if without params" do expect(described_class.last(repository).id).to eq( rugged_repo.head.target.oid @@ -154,6 +154,20 @@ describe Gitlab::Git::Commit, :seed_helper do end end + describe '.find with Gitaly enabled' do + it_should_behave_like '.find' + end + + describe '.find with Rugged enabled', :enable_rugged do + it 'calls out to the Rugged implementation' do + allow_any_instance_of(Rugged).to receive(:rev_parse).with(SeedRepo::Commit::ID).and_call_original + + described_class.find(repository, SeedRepo::Commit::ID) + end + + it_should_behave_like '.find' + end + describe '.last_for_path' do context 'no path' do subject { described_class.last_for_path(repository, 'master') } diff --git a/spec/lib/gitlab/gitaly_client/storage_settings_spec.rb b/spec/lib/gitlab/gitaly_client/storage_settings_spec.rb index c89913ec8e9..bb10be2a4dc 100644 --- a/spec/lib/gitlab/gitaly_client/storage_settings_spec.rb +++ b/spec/lib/gitlab/gitaly_client/storage_settings_spec.rb @@ -26,4 +26,14 @@ describe Gitlab::GitalyClient::StorageSettings do end end end + + describe '.disk_access_denied?' do + it 'return false when Rugged is enabled', :enable_rugged do + expect(described_class.disk_access_denied?).to be_falsey + end + + it 'returns true' do + expect(described_class.disk_access_denied?).to be_truthy + end + end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index cd69160be10..ea28cc0e4ba 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -115,10 +115,17 @@ RSpec.configure do |config| TestEnv.clean_test_path end - config.before(:example) do + config.before do |example| # Enable all features by default for testing allow(Feature).to receive(:enabled?) { true } + enabled = example.metadata[:enable_rugged].present? + + # Disable Rugged features by default + Gitlab::Git::RuggedImpl::Repository::FEATURE_FLAGS.each do |flag| + allow(Feature).to receive(:enabled?).with(flag).and_return(enabled) + end + # The following can be removed when we remove the staged rollout strategy # and we can just enable it using instance wide settings # (ie. ApplicationSetting#auto_devops_enabled) -- GitLab From d33771840157e0ed195cfa45b175942181210b35 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 7 Mar 2019 11:36:12 +0000 Subject: [PATCH 2/8] Merge branch 'sh-rugged-commit-is-ancestor' into 'master' Bring back Rugged implementation of CommitIsAncestor See merge request gitlab-org/gitlab-ce!25702 --- .../sh-rugged-commit-is-ancestor.yml | 5 ++++ lib/gitlab/git/repository.rb | 2 +- lib/gitlab/git/rugged_impl/repository.rb | 27 ++++++++++++++++++- spec/models/repository_spec.rb | 16 ++++++++++- 4 files changed, 47 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/sh-rugged-commit-is-ancestor.yml diff --git a/changelogs/unreleased/sh-rugged-commit-is-ancestor.yml b/changelogs/unreleased/sh-rugged-commit-is-ancestor.yml new file mode 100644 index 00000000000..0f62176b4a5 --- /dev/null +++ b/changelogs/unreleased/sh-rugged-commit-is-ancestor.yml @@ -0,0 +1,5 @@ +--- +title: Bring back Rugged implementation of CommitIsAncestor +merge_request: 25702 +author: +type: other diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index b6c7a12de35..20bfcde39bd 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -9,7 +9,7 @@ module Gitlab include Gitlab::Git::WrapsGitalyErrors include Gitlab::EncodingHelper include Gitlab::Utils::StrongMemoize - include Gitlab::Git::RuggedImpl::Repository + prepend Gitlab::Git::RuggedImpl::Repository SEARCH_CONTEXT_LINES = 3 REV_LIST_COMMIT_LIMIT = 2_000 diff --git a/lib/gitlab/git/rugged_impl/repository.rb b/lib/gitlab/git/rugged_impl/repository.rb index 135c47017b3..d4500573235 100644 --- a/lib/gitlab/git/rugged_impl/repository.rb +++ b/lib/gitlab/git/rugged_impl/repository.rb @@ -10,7 +10,9 @@ module Gitlab module Git module RuggedImpl module Repository - FEATURE_FLAGS = %i(rugged_find_commit).freeze + extend ::Gitlab::Utils::Override + + FEATURE_FLAGS = %i(rugged_find_commit rugged_tree_entries rugged_tree_entry rugged_commit_is_ancestor).freeze def alternate_object_directories relative_object_directories.map { |d| File.join(path, d) } @@ -41,6 +43,29 @@ module Gitlab obj = rugged.rev_parse(revspec) Ref.dereference_object(obj) end + + override :ancestor? + def ancestor?(from, to) + if Feature.enabled?(:rugged_commit_is_ancestor) + rugged_is_ancestor?(from, to) + else + super + end + end + + def rugged_is_ancestor?(ancestor_id, descendant_id) + return false if ancestor_id.nil? || descendant_id.nil? + + rugged_merge_base(ancestor_id, descendant_id) == ancestor_id + rescue Rugged::OdbError + false + end + + def rugged_merge_base(from, to) + rugged.merge_base(from, to) + rescue Rugged::ReferenceError + nil + end end end end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index fbf8f330d6d..b138201f2a8 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -2247,7 +2247,7 @@ describe Repository do rugged.references.create("refs/remotes/#{remote_name}/#{branch_name}", target.id) end - describe '#ancestor?' do + shared_examples '#ancestor?' do let(:commit) { repository.commit } let(:ancestor) { commit.parents.first } @@ -2281,6 +2281,20 @@ describe Repository do end end + describe '#ancestor? with Gitaly enabled' do + it_behaves_like "#ancestor?" + end + + describe '#ancestor? with Rugged enabled', :enable_rugged do + it 'calls out to the Rugged implementation' do + allow_any_instance_of(Rugged).to receive(:merge_base).with(repository.commit.id, Gitlab::Git::BLANK_SHA).and_call_original + + repository.ancestor?(repository.commit.id, Gitlab::Git::BLANK_SHA) + end + + it_behaves_like '#ancestor?' + end + describe '#archive_metadata' do let(:ref) { 'master' } let(:storage_path) { '/tmp' } -- GitLab From f98c58ce37d1eecc4d804f128b2304cc289ac43a Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 7 Mar 2019 15:57:06 +0000 Subject: [PATCH 3/8] Merge branch 'sh-rugged-get-tree-entry' into 'master' Bring back Rugged implementation of TreeEntry See merge request gitlab-org/gitlab-ce!25706 --- .../unreleased/sh-rugged-get-tree-entry.yml | 5 + lib/gitlab/git/blob.rb | 6 + lib/gitlab/git/rugged_impl/blob.rb | 106 ++++++++++++++++++ spec/lib/gitlab/git/blob_spec.rb | 16 ++- 4 files changed, 132 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/sh-rugged-get-tree-entry.yml create mode 100644 lib/gitlab/git/rugged_impl/blob.rb diff --git a/changelogs/unreleased/sh-rugged-get-tree-entry.yml b/changelogs/unreleased/sh-rugged-get-tree-entry.yml new file mode 100644 index 00000000000..4d46b764022 --- /dev/null +++ b/changelogs/unreleased/sh-rugged-get-tree-entry.yml @@ -0,0 +1,5 @@ +--- +title: Bring back Rugged implementation of TreeEntry +merge_request: 25706 +author: +type: other diff --git a/lib/gitlab/git/blob.rb b/lib/gitlab/git/blob.rb index 0bd1d3420a2..354912f19cc 100644 --- a/lib/gitlab/git/blob.rb +++ b/lib/gitlab/git/blob.rb @@ -23,6 +23,10 @@ module Gitlab class << self def find(repository, sha, path, limit: MAX_DATA_DISPLAY_SIZE) + tree_entry(repository, sha, path, limit) + end + + def tree_entry(repository, sha, path, limit) return unless path path = path.sub(%r{\A/*}, '') @@ -179,3 +183,5 @@ module Gitlab end end end + +Gitlab::Git::Blob.singleton_class.prepend Gitlab::Git::RuggedImpl::Blob::ClassMethods diff --git a/lib/gitlab/git/rugged_impl/blob.rb b/lib/gitlab/git/rugged_impl/blob.rb new file mode 100644 index 00000000000..11ee4ebda4b --- /dev/null +++ b/lib/gitlab/git/rugged_impl/blob.rb @@ -0,0 +1,106 @@ +# frozen_string_literal: true + +# NOTE: This code is legacy. Do not add/modify code here unless you have +# discussed with the Gitaly team. See +# https://docs.gitlab.com/ee/development/gitaly.html#legacy-rugged-code +# for more details. + +module Gitlab + module Git + module RuggedImpl + module Blob + module ClassMethods + extend ::Gitlab::Utils::Override + + override :tree_entry + def tree_entry(repository, sha, path, limit) + if Feature.enabled?(:rugged_tree_entry) + rugged_tree_entry(repository, sha, path, limit) + else + super + end + end + + private + + def rugged_tree_entry(repository, sha, path, limit) + return unless path + + # Strip any leading / characters from the path + path = path.sub(%r{\A/*}, '') + + rugged_commit = repository.lookup(sha) + root_tree = rugged_commit.tree + + blob_entry = find_entry_by_path(repository, root_tree.oid, *path.split('/')) + + return unless blob_entry + + if blob_entry[:type] == :commit + submodule_blob(blob_entry, path, sha) + else + blob = repository.lookup(blob_entry[:oid]) + + if blob + new( + id: blob.oid, + name: blob_entry[:name], + size: blob.size, + # Rugged::Blob#content is expensive; don't call it if we don't have to. + data: limit.zero? ? '' : blob.content(limit), + mode: blob_entry[:filemode].to_s(8), + path: path, + commit_id: sha, + binary: blob.binary? + ) + end + end + rescue Rugged::ReferenceError + nil + end + + # Recursive search of blob id by path + # + # Ex. + # blog/ # oid: 1a + # app/ # oid: 2a + # models/ # oid: 3a + # file.rb # oid: 4a + # + # + # Blob.find_entry_by_path(repo, '1a', 'blog', 'app', 'file.rb') # => '4a' + # + def find_entry_by_path(repository, root_id, *path_parts) + root_tree = repository.lookup(root_id) + + entry = root_tree.find do |entry| + entry[:name] == path_parts[0] + end + + return unless entry + + if path_parts.size > 1 + return unless entry[:type] == :tree + + path_parts.shift + find_entry_by_path(repository, entry[:oid], *path_parts) + else + [:blob, :commit].include?(entry[:type]) ? entry : nil + end + end + + def submodule_blob(blob_entry, path, sha) + new( + id: blob_entry[:oid], + name: blob_entry[:name], + size: 0, + data: '', + path: path, + commit_id: sha + ) + end + end + end + end + end +end diff --git a/spec/lib/gitlab/git/blob_spec.rb b/spec/lib/gitlab/git/blob_spec.rb index b243f0dacae..5baceb437b8 100644 --- a/spec/lib/gitlab/git/blob_spec.rb +++ b/spec/lib/gitlab/git/blob_spec.rb @@ -18,7 +18,7 @@ describe Gitlab::Git::Blob, :seed_helper do end end - describe '.find' do + shared_examples '.find' do context 'nil path' do let(:blob) { Gitlab::Git::Blob.find(repository, SeedRepo::Commit::ID, nil) } @@ -128,6 +128,20 @@ describe Gitlab::Git::Blob, :seed_helper do end end + describe '.find with Gitaly enabled' do + it_behaves_like '.find' + end + + describe '.find with Rugged enabled', :enable_rugged do + it 'calls out to the Rugged implementation' do + allow_any_instance_of(Rugged).to receive(:rev_parse).with(SeedRepo::Commit::ID).and_call_original + + described_class.find(repository, SeedRepo::Commit::ID, 'files/images/6049019_460s.jpg') + end + + it_behaves_like '.find' + end + shared_examples 'finding blobs by ID' do let(:raw_blob) { Gitlab::Git::Blob.raw(repository, SeedRepo::RubyBlob::ID) } let(:bad_blob) { Gitlab::Git::Blob.raw(repository, SeedRepo::BigCommit::ID) } -- GitLab From b00a91337124a1530235fde8938e485da6d389d5 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 7 Mar 2019 15:28:42 +0000 Subject: [PATCH 4/8] Merge branch 'sh-rugged-tree-entries' into 'master' Bring back Rugged implementation of GetTreeEntries See merge request gitlab-org/gitlab-ce!25674 --- app/helpers/tree_helper.rb | 11 +- .../unreleased/sh-rugged-tree-entries.yml | 5 + lib/gitlab/git/rugged_impl/repository.rb | 5 + lib/gitlab/git/rugged_impl/tree.rb | 105 ++++++++++++++++++ lib/gitlab/git/tree.rb | 6 + spec/lib/gitlab/git/tree_spec.rb | 80 ++++++++++--- 6 files changed, 189 insertions(+), 23 deletions(-) create mode 100644 changelogs/unreleased/sh-rugged-tree-entries.yml create mode 100644 lib/gitlab/git/rugged_impl/tree.rb diff --git a/app/helpers/tree_helper.rb b/app/helpers/tree_helper.rb index e2879bfdcf1..c5bab877c00 100644 --- a/app/helpers/tree_helper.rb +++ b/app/helpers/tree_helper.rb @@ -136,18 +136,9 @@ module TreeHelper end # returns the relative path of the first subdir that doesn't have only one directory descendant - # rubocop: disable CodeReuse/ActiveRecord def flatten_tree(root_path, tree) - return tree.flat_path.sub(%r{\A#{Regexp.escape(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(root_path, subtree.first)) - else - return tree.name - end + tree.flat_path.sub(%r{\A#{Regexp.escape(root_path)}/}, '') end - # rubocop: enable CodeReuse/ActiveRecord def selected_branch @branch_name || tree_edit_branch diff --git a/changelogs/unreleased/sh-rugged-tree-entries.yml b/changelogs/unreleased/sh-rugged-tree-entries.yml new file mode 100644 index 00000000000..fca1f204b9b --- /dev/null +++ b/changelogs/unreleased/sh-rugged-tree-entries.yml @@ -0,0 +1,5 @@ +--- +title: Bring back Rugged implementation of GetTreeEntries +merge_request: 25674 +author: +type: other diff --git a/lib/gitlab/git/rugged_impl/repository.rb b/lib/gitlab/git/rugged_impl/repository.rb index d4500573235..fe0120b1199 100644 --- a/lib/gitlab/git/rugged_impl/repository.rb +++ b/lib/gitlab/git/rugged_impl/repository.rb @@ -66,6 +66,11 @@ module Gitlab rescue Rugged::ReferenceError nil end + + # Lookup for rugged object by oid or ref name + def lookup(oid_or_ref_name) + rugged.rev_parse(oid_or_ref_name) + end end end end diff --git a/lib/gitlab/git/rugged_impl/tree.rb b/lib/gitlab/git/rugged_impl/tree.rb new file mode 100644 index 00000000000..0ebfd496695 --- /dev/null +++ b/lib/gitlab/git/rugged_impl/tree.rb @@ -0,0 +1,105 @@ +# frozen_string_literal: true + +# NOTE: This code is legacy. Do not add/modify code here unless you have +# discussed with the Gitaly team. See +# https://docs.gitlab.com/ee/development/gitaly.html#legacy-rugged-code +# for more details. + +module Gitlab + module Git + module RuggedImpl + module Tree + module ClassMethods + extend ::Gitlab::Utils::Override + + override :tree_entries + def tree_entries(repository, sha, path, recursive) + if Feature.enabled?(:rugged_tree_entries) + tree_entries_from_rugged(repository, sha, path, recursive) + else + super + end + end + + def tree_entries_from_rugged(repository, sha, path, recursive) + current_path_entries = get_tree_entries_from_rugged(repository, sha, path) + ordered_entries = [] + + current_path_entries.each do |entry| + ordered_entries << entry + + if recursive && entry.dir? + ordered_entries.concat(tree_entries_from_rugged(repository, sha, entry.path, true)) + end + end + + # This was an optimization to reduce N+1 queries for Gitaly + # (https://gitlab.com/gitlab-org/gitaly/issues/530). It + # used to be done lazily in the view via + # TreeHelper#flatten_tree, so it's possible there's a + # performance impact by loading this eagerly. + rugged_populate_flat_path(repository, sha, path, ordered_entries) + end + + def rugged_populate_flat_path(repository, sha, path, entries) + entries.each do |entry| + entry.flat_path = entry.path + + next unless entry.dir? + + entry.flat_path = + if path + File.join(path, rugged_flatten_tree(repository, sha, entry, path)) + else + rugged_flatten_tree(repository, sha, entry, path) + end + end + end + + # Returns the relative path of the first subdir that doesn't have only one directory descendant + def rugged_flatten_tree(repository, sha, tree, root_path) + subtree = tree_entries_from_rugged(repository, sha, tree.path, false) + + if subtree.count == 1 && subtree.first.dir? + File.join(tree.name, rugged_flatten_tree(repository, sha, subtree.first, root_path)) + else + tree.name + end + end + + def get_tree_entries_from_rugged(repository, sha, path) + commit = repository.lookup(sha) + root_tree = commit.tree + + tree = if path + id = find_id_by_path(repository, root_tree.oid, path) + if id + repository.lookup(id) + else + [] + end + else + root_tree + end + + tree.map do |entry| + current_path = path ? File.join(path, entry[:name]) : entry[:name] + + new( + id: entry[:oid], + root_id: root_tree.oid, + name: entry[:name], + type: entry[:type], + mode: entry[:filemode].to_s(8), + path: current_path, + commit_id: sha + ) + end + rescue Rugged::ReferenceError + [] + end + end + end + end + end +end diff --git a/lib/gitlab/git/tree.rb b/lib/gitlab/git/tree.rb index b5b701699f0..c3d49ef3599 100644 --- a/lib/gitlab/git/tree.rb +++ b/lib/gitlab/git/tree.rb @@ -16,6 +16,10 @@ module Gitlab def where(repository, sha, path = nil, recursive = false) path = nil if path == '' || path == '/' + tree_entries(repository, sha, path, recursive) + end + + def tree_entries(repository, sha, path, recursive) wrapped_gitaly_errors do repository.gitaly_commit_client.tree_entries(repository, sha, path, recursive) end @@ -93,3 +97,5 @@ module Gitlab end end end + +Gitlab::Git::Tree.singleton_class.prepend Gitlab::Git::RuggedImpl::Tree::ClassMethods diff --git a/spec/lib/gitlab/git/tree_spec.rb b/spec/lib/gitlab/git/tree_spec.rb index 3792d6bf67b..f0ff0425193 100644 --- a/spec/lib/gitlab/git/tree_spec.rb +++ b/spec/lib/gitlab/git/tree_spec.rb @@ -3,7 +3,7 @@ require "spec_helper" describe Gitlab::Git::Tree, :seed_helper do let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') } - context :repo do + shared_examples :repo do let(:tree) { Gitlab::Git::Tree.where(repository, SeedRepo::Commit::ID) } it { expect(tree).to be_kind_of Array } @@ -12,6 +12,17 @@ describe Gitlab::Git::Tree, :seed_helper do it { expect(tree.select(&:file?).size).to eq(10) } it { expect(tree.select(&:submodule?).size).to eq(2) } + it 'returns an empty array when called with an invalid ref' do + expect(described_class.where(repository, 'foobar-does-not-exist')).to eq([]) + end + + it 'returns a list of tree objects' do + entries = described_class.where(repository, SeedRepo::Commit::ID, 'files', true) + + expect(entries.count).to be > 10 + expect(entries).to all(be_a(Gitlab::Git::Tree)) + end + describe '#dir?' do let(:dir) { tree.select(&:dir?).first } @@ -20,8 +31,8 @@ describe Gitlab::Git::Tree, :seed_helper 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') } + it { expect(dir.flat_path).to eq('encoding') } context :subdir do let(:subdir) { Gitlab::Git::Tree.where(repository, SeedRepo::Commit::ID, 'files').first } @@ -44,6 +55,51 @@ describe Gitlab::Git::Tree, :seed_helper do it { expect(subdir_file.path).to eq('files/ruby/popen.rb') } it { expect(subdir_file.flat_path).to eq('files/ruby/popen.rb') } end + + context :flat_path do + let(:filename) { 'files/flat/path/correct/content.txt' } + let(:oid) { create_file(filename) } + let(:subdir_file) { Gitlab::Git::Tree.where(repository, oid, 'files/flat').first } + let(:repository_rugged) { Rugged::Repository.new(File.join(SEED_STORAGE_PATH, TEST_REPO_PATH)) } + + it { expect(subdir_file.flat_path).to eq('files/flat/path/correct') } + end + + def create_file(path) + oid = repository_rugged.write('test', :blob) + index = repository_rugged.index + index.add(path: filename, oid: oid, mode: 0100644) + + options = commit_options( + repository_rugged, + index, + repository_rugged.head.target, + 'HEAD', + 'Add new file') + + Rugged::Commit.create(repository_rugged, options) + end + + # Build the options hash that's passed to Rugged::Commit#create + def commit_options(repo, index, target, ref, message) + options = {} + options[:tree] = index.write_tree(repo) + options[:author] = { + email: "test@example.com", + name: "Test Author", + time: Time.gm(2014, "mar", 3, 20, 15, 1) + } + options[:committer] = { + email: "test@example.com", + name: "Test Author", + time: Time.gm(2014, "mar", 3, 20, 15, 1) + } + options[:message] ||= message + options[:parents] = repo.empty? ? [] : [target].compact + options[:update_ref] = ref + + options + end end describe '#file?' do @@ -79,19 +135,17 @@ describe Gitlab::Git::Tree, :seed_helper do end end - describe '#where' do - shared_examples '#where' do - it 'returns an empty array when called with an invalid ref' do - expect(described_class.where(repository, 'foobar-does-not-exist')).to eq([]) - end - end + describe '.where with Gitaly enabled' do + it_behaves_like :repo + end - context 'with gitaly' do - it_behaves_like '#where' - end + describe '.where with Rugged enabled', :enable_rugged do + it 'calls out to the Rugged implementation' do + allow_any_instance_of(Rugged).to receive(:lookup).with(SeedRepo::Commit::ID) - context 'without gitaly', :skip_gitaly_mock do - it_behaves_like '#where' + described_class.where(repository, SeedRepo::Commit::ID, 'files', false) end + + it_behaves_like :repo end end -- GitLab From 733d0a8be5de3f12c05ef857468cfbc9265347d8 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Mon, 11 Mar 2019 11:56:19 +0000 Subject: [PATCH 5/8] Merge branch 'sh-rugged-commit-tree-entry' into 'master' Bring back Rugged implementation of commit_tree_entry See merge request gitlab-org/gitlab-ce!25896 --- .../sh-rugged-commit-tree-entry.yml | 5 ++++ lib/gitlab/git/commit.rb | 5 ++++ lib/gitlab/git/rugged_impl/commit.rb | 24 +++++++++++++++++++ lib/gitlab/git/rugged_impl/repository.rb | 2 +- spec/models/commit_spec.rb | 16 ++++++++++++- 5 files changed, 50 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/sh-rugged-commit-tree-entry.yml diff --git a/changelogs/unreleased/sh-rugged-commit-tree-entry.yml b/changelogs/unreleased/sh-rugged-commit-tree-entry.yml new file mode 100644 index 00000000000..bcefa2c7112 --- /dev/null +++ b/changelogs/unreleased/sh-rugged-commit-tree-entry.yml @@ -0,0 +1,5 @@ +--- +title: Bring back Rugged implementation of commit_tree_entry +merge_request: 25896 +author: +type: other diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb index eee45ba6e19..0ac160766da 100644 --- a/lib/gitlab/git/commit.rb +++ b/lib/gitlab/git/commit.rb @@ -328,11 +328,16 @@ module Gitlab def tree_entry(path) return unless path.present? + commit_tree_entry(path) + end + + def commit_tree_entry(path) # We're only interested in metadata, so limit actual data to 1 byte # since Gitaly doesn't support "send no data" option. entry = @repository.gitaly_commit_client.tree_entry(id, path, 1) return unless entry + # To be compatible with the rugged format entry = entry.to_h entry.delete(:data) entry[:name] = File.basename(path) diff --git a/lib/gitlab/git/rugged_impl/commit.rb b/lib/gitlab/git/rugged_impl/commit.rb index 251802878c3..f6777dfa0c3 100644 --- a/lib/gitlab/git/rugged_impl/commit.rb +++ b/lib/gitlab/git/rugged_impl/commit.rb @@ -43,6 +43,30 @@ module Gitlab end end + override :commit_tree_entry + def commit_tree_entry(path) + if Feature.enabled?(:rugged_commit_tree_entry) + rugged_tree_entry(path) + else + super + end + end + + # Is this the same as Blob.find_entry_by_path ? + def rugged_tree_entry(path) + rugged_commit.tree.path(path) + rescue Rugged::TreeError + nil + end + + def rugged_commit + @rugged_commit ||= if raw_commit.is_a?(Rugged::Commit) + raw_commit + else + @repository.rev_parse_target(id) + end + end + def init_from_rugged(commit) author = commit.author committer = commit.committer diff --git a/lib/gitlab/git/rugged_impl/repository.rb b/lib/gitlab/git/rugged_impl/repository.rb index fe0120b1199..c0a91f59ab9 100644 --- a/lib/gitlab/git/rugged_impl/repository.rb +++ b/lib/gitlab/git/rugged_impl/repository.rb @@ -12,7 +12,7 @@ module Gitlab module Repository extend ::Gitlab::Utils::Override - FEATURE_FLAGS = %i(rugged_find_commit rugged_tree_entries rugged_tree_entry rugged_commit_is_ancestor).freeze + FEATURE_FLAGS = %i(rugged_find_commit rugged_tree_entries rugged_tree_entry rugged_commit_is_ancestor rugged_commit_tree_entry).freeze def alternate_object_directories relative_object_directories.map { |d| File.join(path, d) } diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index 34182a21a35..616477d3f43 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -520,7 +520,7 @@ eos end end - describe '#uri_type' do + shared_examples '#uri_type' do it 'returns the URI type at the given path' do expect(commit.uri_type('files/html')).to be(:tree) expect(commit.uri_type('files/images/logo-black.png')).to be(:raw) @@ -539,6 +539,20 @@ eos end end + describe '#uri_type with Gitaly enabled' do + it_behaves_like "#uri_type" + end + + describe '#uri_type with Rugged enabled', :enable_rugged do + it 'calls out to the Rugged implementation' do + allow_any_instance_of(Rugged::Tree).to receive(:path).with('files/html').and_call_original + + commit.uri_type('files/html') + end + + it_behaves_like '#uri_type' + end + describe '.from_hash' do let(:new_commit) { described_class.from_hash(commit.to_hash, project) } -- GitLab From 4b98a26b3f8c65ad48767cb71ae302b7c834984b Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 2 Apr 2019 09:27:17 +0000 Subject: [PATCH 6/8] Merge branch 'sh-fix-rugged-tree-entries' into 'master' Avoid excessive recursive calls with Rugged TreeEntries Closes #59759 See merge request gitlab-org/gitlab-ce!26813 --- .../unreleased/sh-fix-rugged-tree-entries.yml | 5 +++++ lib/gitlab/git/rugged_impl/tree.rb | 20 +++++++++++-------- spec/lib/gitlab/git/tree_spec.rb | 2 +- 3 files changed, 18 insertions(+), 9 deletions(-) create mode 100644 changelogs/unreleased/sh-fix-rugged-tree-entries.yml diff --git a/changelogs/unreleased/sh-fix-rugged-tree-entries.yml b/changelogs/unreleased/sh-fix-rugged-tree-entries.yml new file mode 100644 index 00000000000..97b27678905 --- /dev/null +++ b/changelogs/unreleased/sh-fix-rugged-tree-entries.yml @@ -0,0 +1,5 @@ +--- +title: Avoid excessive recursive calls with Rugged TreeEntries +merge_request: 26813 +author: +type: fixed diff --git a/lib/gitlab/git/rugged_impl/tree.rb b/lib/gitlab/git/rugged_impl/tree.rb index 0ebfd496695..bb13d114d46 100644 --- a/lib/gitlab/git/rugged_impl/tree.rb +++ b/lib/gitlab/git/rugged_impl/tree.rb @@ -15,12 +15,23 @@ module Gitlab override :tree_entries def tree_entries(repository, sha, path, recursive) if Feature.enabled?(:rugged_tree_entries) - tree_entries_from_rugged(repository, sha, path, recursive) + tree_entries_with_flat_path_from_rugged(repository, sha, path, recursive) else super end end + def tree_entries_with_flat_path_from_rugged(repository, sha, path, recursive) + tree_entries_from_rugged(repository, sha, path, recursive).tap do |entries| + # This was an optimization to reduce N+1 queries for Gitaly + # (https://gitlab.com/gitlab-org/gitaly/issues/530). It + # used to be done lazily in the view via + # TreeHelper#flatten_tree, so it's possible there's a + # performance impact by loading this eagerly. + rugged_populate_flat_path(repository, sha, path, entries) + end + end + def tree_entries_from_rugged(repository, sha, path, recursive) current_path_entries = get_tree_entries_from_rugged(repository, sha, path) ordered_entries = [] @@ -32,13 +43,6 @@ module Gitlab ordered_entries.concat(tree_entries_from_rugged(repository, sha, entry.path, true)) end end - - # This was an optimization to reduce N+1 queries for Gitaly - # (https://gitlab.com/gitlab-org/gitaly/issues/530). It - # used to be done lazily in the view via - # TreeHelper#flatten_tree, so it's possible there's a - # performance impact by loading this eagerly. - rugged_populate_flat_path(repository, sha, path, ordered_entries) end def rugged_populate_flat_path(repository, sha, path, entries) diff --git a/spec/lib/gitlab/git/tree_spec.rb b/spec/lib/gitlab/git/tree_spec.rb index f0ff0425193..c27c415c5ab 100644 --- a/spec/lib/gitlab/git/tree_spec.rb +++ b/spec/lib/gitlab/git/tree_spec.rb @@ -19,7 +19,7 @@ describe Gitlab::Git::Tree, :seed_helper do it 'returns a list of tree objects' do entries = described_class.where(repository, SeedRepo::Commit::ID, 'files', true) - expect(entries.count).to be > 10 + expect(entries.count).to be >= 5 expect(entries).to all(be_a(Gitlab::Git::Tree)) end -- GitLab From 61b963efd1126ee80ca6c506f5fd43e2c73f6c8d Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 17 Apr 2019 13:16:51 +0000 Subject: [PATCH 7/8] Merge branch 'sh-backport-list-commits-by-oid-rugged' into 'master' Bring back Rugged implementation of ListCommitsByOid See merge request gitlab-org/gitlab-ce!27441 --- ...sh-backport-list-commits-by-oid-rugged.yml | 5 +++ doc/development/gitaly.md | 2 + lib/gitlab/git/rugged_impl/commit.rb | 20 ++++++++++ lib/gitlab/git/rugged_impl/repository.rb | 2 +- spec/lib/gitlab/git/commit_spec.rb | 37 ++++++++++++++++++- 5 files changed, 64 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/sh-backport-list-commits-by-oid-rugged.yml diff --git a/changelogs/unreleased/sh-backport-list-commits-by-oid-rugged.yml b/changelogs/unreleased/sh-backport-list-commits-by-oid-rugged.yml new file mode 100644 index 00000000000..eb8774d652f --- /dev/null +++ b/changelogs/unreleased/sh-backport-list-commits-by-oid-rugged.yml @@ -0,0 +1,5 @@ +--- +title: Bring back Rugged implementation of ListCommitsByOid +merge_request: 27441 +author: +type: performance diff --git a/doc/development/gitaly.md b/doc/development/gitaly.md index 8c806c433a1..906585f6f21 100644 --- a/doc/development/gitaly.md +++ b/doc/development/gitaly.md @@ -73,6 +73,8 @@ most commonly-used RPCs can be enabled via feature flags: * `rugged_get_tree_entries` * `rugged_tree_entry` * `rugged_commit_is_ancestor` +* `rugged_commit_tree_entry` +* `rugged_list_commits_by_oid` A convenience Rake task can be used to enable or disable these flags all together. To enable: diff --git a/lib/gitlab/git/rugged_impl/commit.rb b/lib/gitlab/git/rugged_impl/commit.rb index f6777dfa0c3..bce4fa14fb4 100644 --- a/lib/gitlab/git/rugged_impl/commit.rb +++ b/lib/gitlab/git/rugged_impl/commit.rb @@ -21,6 +21,17 @@ module Gitlab nil end + # This needs to return an array of Gitlab::Git:Commit objects + # instead of Rugged::Commit objects to ensure upstream models + # operate on a consistent interface. Unlike + # Gitlab::Git::Commit.find, Gitlab::Git::Commit.batch_by_oid + # doesn't attempt to decorate the result. + def rugged_batch_by_oid(repo, oids) + oids.map { |oid| rugged_find(repo, oid) } + .compact + .map { |commit| decorate(repo, commit) } + end + override :find_commit def find_commit(repo, commit_id) if Feature.enabled?(:rugged_find_commit) @@ -29,6 +40,15 @@ module Gitlab super end end + + override :batch_by_oid + def batch_by_oid(repo, oids) + if Feature.enabled?(:rugged_list_commits_by_oid) + rugged_batch_by_oid(repo, oids) + else + super + end + end end extend ::Gitlab::Utils::Override diff --git a/lib/gitlab/git/rugged_impl/repository.rb b/lib/gitlab/git/rugged_impl/repository.rb index c0a91f59ab9..e91b0ddcd31 100644 --- a/lib/gitlab/git/rugged_impl/repository.rb +++ b/lib/gitlab/git/rugged_impl/repository.rb @@ -12,7 +12,7 @@ module Gitlab module Repository extend ::Gitlab::Utils::Override - FEATURE_FLAGS = %i(rugged_find_commit rugged_tree_entries rugged_tree_entry rugged_commit_is_ancestor rugged_commit_tree_entry).freeze + FEATURE_FLAGS = %i(rugged_find_commit rugged_tree_entries rugged_tree_entry rugged_commit_is_ancestor rugged_commit_tree_entry rugged_list_commits_by_oid).freeze def alternate_object_directories relative_object_directories.map { |d| File.join(path, d) } diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb index 44f3e015836..c8bfe413657 100644 --- a/spec/lib/gitlab/git/commit_spec.rb +++ b/spec/lib/gitlab/git/commit_spec.rb @@ -390,7 +390,32 @@ describe Gitlab::Git::Commit, :seed_helper do end end - describe '#batch_by_oid' do + shared_examples '.batch_by_oid' do + context 'with multiple OIDs' do + let(:oids) { [SeedRepo::Commit::ID, SeedRepo::FirstCommit::ID] } + + it 'returns multiple commits' do + commits = described_class.batch_by_oid(repository, oids) + + expect(commits.count).to eq(2) + expect(commits).to all( be_a(Gitlab::Git::Commit) ) + expect(commits.first.sha).to eq(SeedRepo::Commit::ID) + expect(commits.second.sha).to eq(SeedRepo::FirstCommit::ID) + end + end + + context 'when oids is empty' do + it 'returns empty commits' do + commits = described_class.batch_by_oid(repository, []) + + expect(commits.count).to eq(0) + end + end + end + + describe '.batch_by_oid with Gitaly enabled' do + it_should_behave_like '.batch_by_oid' + context 'when oids is empty' do it 'makes no Gitaly request' do expect(Gitlab::GitalyClient).not_to receive(:call) @@ -400,6 +425,16 @@ describe Gitlab::Git::Commit, :seed_helper do end end + describe '.batch_by_oid with Rugged enabled', :enable_rugged do + it_should_behave_like '.batch_by_oid' + + it 'calls out to the Rugged implementation' do + allow_any_instance_of(Rugged).to receive(:rev_parse).with(SeedRepo::Commit::ID).and_call_original + + described_class.batch_by_oid(repository, [SeedRepo::Commit::ID]) + end + end + shared_examples 'extracting commit signature' do context 'when the commit is signed' do let(:commit_id) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } -- GitLab From 8af9a5bcb5ea0d22f5aeecd8c2d34de3d02558d3 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 20 Apr 2019 22:43:59 -0700 Subject: [PATCH 8/8] Fix failing spec in spec/models/repository_spec.rb --- spec/models/repository_spec.rb | 34 ++++++++++++---------------------- 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index b138201f2a8..bce80f643a1 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -2251,33 +2251,23 @@ describe Repository do let(:commit) { repository.commit } let(:ancestor) { commit.parents.first } - shared_examples '#ancestor?' do - it 'it is an ancestor' do - expect(repository.ancestor?(ancestor.id, commit.id)).to eq(true) - end - - it 'it is not an ancestor' do - expect(repository.ancestor?(commit.id, ancestor.id)).to eq(false) - end - - it 'returns false on nil-values' do - expect(repository.ancestor?(nil, commit.id)).to eq(false) - expect(repository.ancestor?(ancestor.id, nil)).to eq(false) - expect(repository.ancestor?(nil, nil)).to eq(false) - end + it 'it is an ancestor' do + expect(repository.ancestor?(ancestor.id, commit.id)).to eq(true) + end - it 'returns false for invalid commit IDs' do - expect(repository.ancestor?(commit.id, Gitlab::Git::BLANK_SHA)).to eq(false) - expect(repository.ancestor?( Gitlab::Git::BLANK_SHA, commit.id)).to eq(false) - end + it 'it is not an ancestor' do + expect(repository.ancestor?(commit.id, ancestor.id)).to eq(false) end - context 'with Gitaly enabled' do - it_behaves_like('#ancestor?') + it 'returns false on nil-values' do + expect(repository.ancestor?(nil, commit.id)).to eq(false) + expect(repository.ancestor?(ancestor.id, nil)).to eq(false) + expect(repository.ancestor?(nil, nil)).to eq(false) end - context 'with Gitaly disabled', :skip_gitaly_mock do - it_behaves_like('#ancestor?') + it 'returns false for invalid commit IDs' do + expect(repository.ancestor?(commit.id, Gitlab::Git::BLANK_SHA)).to eq(false) + expect(repository.ancestor?( Gitlab::Git::BLANK_SHA, commit.id)).to eq(false) end end -- GitLab