From 3082b7d1c2524845441a529fd0704607cf2e8cb1 Mon Sep 17 00:00:00 2001 From: "Jacob Vosmaer (GitLab)" Date: Mon, 9 Jul 2018 10:02:02 +0000 Subject: [PATCH] Make blob and other RPC's mandatory --- lib/gitlab/git/blob.rb | 19 +---- lib/gitlab/git/repository.rb | 116 +++--------------------------- lib/gitlab/workhorse.rb | 23 +++--- spec/lib/gitlab/git/blob_spec.rb | 7 +- spec/lib/gitlab/workhorse_spec.rb | 42 ++++------- 5 files changed, 36 insertions(+), 171 deletions(-) diff --git a/lib/gitlab/git/blob.rb b/lib/gitlab/git/blob.rb index 604bb11e712..96fa94d5790 100644 --- a/lib/gitlab/git/blob.rb +++ b/lib/gitlab/git/blob.rb @@ -50,13 +50,7 @@ module Gitlab end def raw(repository, sha) - Gitlab::GitalyClient.migrate(:git_blob_raw) do |is_enabled| - if is_enabled - repository.gitaly_blob_client.get_blob(oid: sha, limit: MAX_DATA_DISPLAY_SIZE) - else - rugged_raw(repository, sha, limit: MAX_DATA_DISPLAY_SIZE) - end - end + repository.gitaly_blob_client.get_blob(oid: sha, limit: MAX_DATA_DISPLAY_SIZE) end # Returns an array of Blob instances, specified in blob_references as @@ -207,16 +201,7 @@ module Gitlab return if @loaded_all_data - @data = Gitlab::GitalyClient.migrate(:git_blob_load_all_data) do |is_enabled| - begin - if is_enabled - repository.gitaly_blob_client.get_blob(oid: id, limit: -1).data - else - repository.lookup(id).content - end - end - end - + @data = repository.gitaly_blob_client.get_blob(oid: id, limit: -1).data @loaded_all_data = true @loaded_size = @data.bytesize end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index b355e5e87ff..420790f45d0 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -439,31 +439,11 @@ module Gitlab raise ArgumentError.new("invalid Repository#log limit: #{limit.inspect}") end - gitaly_migrate(:find_commits) do |is_enabled| - if is_enabled - gitaly_commit_client.find_commits(options) - else - raw_log(options).map { |c| Commit.decorate(self, c) } - end + wrapped_gitaly_errors do + gitaly_commit_client.find_commits(options) end end - # Used in gitaly-ruby - def raw_log(options) - sha = - unless options[:all] - actual_ref = options[:ref] || root_ref - begin - sha_from_ref(actual_ref) - rescue Rugged::OdbError, Rugged::InvalidError, Rugged::ReferenceError - # Return an empty array if the ref wasn't found - return [] - end - end - - log_by_shell(sha, options) - end - def count_commits(options) options = process_count_commits_options(options.dup) @@ -1223,12 +1203,10 @@ module Gitlab end def find_commits_by_message(query, ref, path, limit, offset) - gitaly_migrate(:commits_by_message) do |is_enabled| - if is_enabled - find_commits_by_message_by_gitaly(query, ref, path, limit, offset) - else - find_commits_by_message_by_shelling_out(query, ref, path, limit, offset) - end + wrapped_gitaly_errors do + gitaly_commit_client + .commits_by_message(query, revision: ref, path: path, limit: limit, offset: offset) + .map { |c| commit(c) } end end @@ -1238,12 +1216,8 @@ module Gitlab end def last_commit_for_path(sha, path) - gitaly_migrate(:last_commit_for_path) do |is_enabled| - if is_enabled - last_commit_for_path_by_gitaly(sha, path) - else - last_commit_for_path_by_rugged(sha, path) - end + wrapped_gitaly_errors do + gitaly_commit_client.last_commit_for_path(sha, path) end end @@ -1454,46 +1428,6 @@ module Gitlab end end - # Gitaly note: JV: although #log_by_shell shells out to Git I think the - # complexity is such that we should migrate it as Ruby before trying to - # do it in Go. - def log_by_shell(sha, options) - limit = options[:limit].to_i - offset = options[:offset].to_i - use_follow_flag = options[:follow] && options[:path].present? - - # We will perform the offset in Ruby because --follow doesn't play well with --skip. - # See: https://gitlab.com/gitlab-org/gitlab-ce/issues/3574#note_3040520 - offset_in_ruby = use_follow_flag && options[:offset].present? - limit += offset if offset_in_ruby - - cmd = %w[log] - cmd << "--max-count=#{limit}" - cmd << '--format=%H' - cmd << "--skip=#{offset}" unless offset_in_ruby - cmd << '--follow' if use_follow_flag - cmd << '--no-merges' if options[:skip_merges] - cmd << "--after=#{options[:after].iso8601}" if options[:after] - cmd << "--before=#{options[:before].iso8601}" if options[:before] - - if options[:all] - cmd += %w[--all --reverse] - else - cmd << sha - end - - # :path can be a string or an array of strings - if options[:path].present? - cmd << '--' - cmd += Array(options[:path]) - end - - raw_output, _status = run_git(cmd) - lines = offset_in_ruby ? raw_output.lines.drop(offset) : raw_output.lines - - lines.map! { |c| Rugged::Commit.new(rugged, c.strip) } - end - # We are trying to deprecate this method because it does a lot of work # but it seems to be used only to look up submodule URL's. # https://gitlab.com/gitlab-org/gitaly/issues/329 @@ -1623,11 +1557,6 @@ module Gitlab end end - def last_commit_for_path_by_rugged(sha, path) - sha = last_commit_id_for_path_by_shelling_out(sha, path) - commit(sha) - end - # Returns true if the given ref name exists # # Ref names must start with `refs/`. @@ -1820,35 +1749,6 @@ module Gitlab raise CommandError, @gitlab_projects.output end - def find_commits_by_message_by_shelling_out(query, ref, path, limit, offset) - ref ||= root_ref - - args = %W( - log #{ref} --pretty=%H --skip #{offset} - --max-count #{limit} --grep=#{query} --regexp-ignore-case - ) - args = args.concat(%W(-- #{path})) if path.present? - - git_log_results = run_git(args).first.lines - - git_log_results.map { |c| commit(c.chomp) }.compact - end - - def find_commits_by_message_by_gitaly(query, ref, path, limit, offset) - gitaly_commit_client - .commits_by_message(query, revision: ref, path: path, limit: limit, offset: offset) - .map { |c| commit(c) } - end - - def last_commit_for_path_by_gitaly(sha, path) - gitaly_commit_client.last_commit_for_path(sha, path) - end - - def last_commit_id_for_path_by_shelling_out(sha, path) - args = %W(rev-list --max-count=1 #{sha} -- #{path}) - run_git_with_timeout(args, Gitlab::Git::Popen::FAST_GIT_PROCESS_TIMEOUT).first.strip - end - def rugged_merge_base(from, to) rugged.merge_base(from, to) rescue Rugged::ReferenceError diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb index 79483ee05f8..55c899912f9 100644 --- a/lib/gitlab/workhorse.rb +++ b/lib/gitlab/workhorse.rb @@ -37,21 +37,14 @@ module Gitlab end def send_git_blob(repository, blob) - params = if Gitlab::GitalyClient.feature_enabled?(:workhorse_raw_show, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) - { - 'GitalyServer' => gitaly_server_hash(repository), - 'GetBlobRequest' => { - repository: repository.gitaly_repository.to_h, - oid: blob.id, - limit: -1 - } - } - else - { - 'RepoPath' => repository.path_to_repo, - 'BlobId' => blob.id - } - end + params = { + 'GitalyServer' => gitaly_server_hash(repository), + 'GetBlobRequest' => { + repository: repository.gitaly_repository.to_h, + oid: blob.id, + limit: -1 + } + } [ SEND_DATA_HEADER, diff --git a/spec/lib/gitlab/git/blob_spec.rb b/spec/lib/gitlab/git/blob_spec.rb index b6061df349d..6900c4189b7 100644 --- a/spec/lib/gitlab/git/blob_spec.rb +++ b/spec/lib/gitlab/git/blob_spec.rb @@ -532,8 +532,8 @@ describe Gitlab::Git::Blob, seed_helper: true do subject { blob.load_all_data!(repository) } it 'loads missing data' do - expect(Gitlab::GitalyClient).to receive(:migrate) - .with(:git_blob_load_all_data).and_return(full_data) + expect(repository.gitaly_blob_client).to receive(:get_blob) + .and_return(double(:response, data: full_data)) subject @@ -544,8 +544,7 @@ describe Gitlab::Git::Blob, seed_helper: true do let(:blob) { Gitlab::Git::Blob.new(name: 'test', size: 4, data: full_data) } it "doesn't perform any loading" do - expect(Gitlab::GitalyClient).not_to receive(:migrate) - .with(:git_blob_load_all_data) + expect(repository.gitaly_blob_client).not_to receive(:get_blob) subject diff --git a/spec/lib/gitlab/workhorse_spec.rb b/spec/lib/gitlab/workhorse_spec.rb index 570580e0cfc..8fdcfe79fb5 100644 --- a/spec/lib/gitlab/workhorse_spec.rb +++ b/spec/lib/gitlab/workhorse_spec.rb @@ -413,34 +413,22 @@ describe Gitlab::Workhorse do subject { described_class.send_git_blob(repository, blob) } - context 'when Gitaly workhorse_raw_show feature is enabled' do - it 'sets the header correctly' do - key, command, params = decode_workhorse_header(subject) + it 'sets the header correctly' do + key, command, params = decode_workhorse_header(subject) - expect(key).to eq('Gitlab-Workhorse-Send-Data') - expect(command).to eq('git-blob') - expect(params).to eq({ - 'GitalyServer' => { - address: Gitlab::GitalyClient.address(project.repository_storage), - token: Gitlab::GitalyClient.token(project.repository_storage) - }, - 'GetBlobRequest' => { - repository: repository.gitaly_repository.to_h, - oid: blob.id, - limit: -1 - } - }.deep_stringify_keys) - end - end - - context 'when Gitaly workhorse_raw_show feature is disabled', :disable_gitaly do - it 'sets the header correctly' do - key, command, params = decode_workhorse_header(subject) - - expect(key).to eq('Gitlab-Workhorse-Send-Data') - expect(command).to eq('git-blob') - expect(params).to eq('RepoPath' => repository.path_to_repo, 'BlobId' => blob.id) - end + expect(key).to eq('Gitlab-Workhorse-Send-Data') + expect(command).to eq('git-blob') + expect(params).to eq({ + 'GitalyServer' => { + address: Gitlab::GitalyClient.address(project.repository_storage), + token: Gitlab::GitalyClient.token(project.repository_storage) + }, + 'GetBlobRequest' => { + repository: repository.gitaly_repository.to_h, + oid: blob.id, + limit: -1 + } + }.deep_stringify_keys) end end