From c0b41064ff1eb4c5465d3c2a9efa0a22b60c3f4c Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Thu, 3 Aug 2017 10:22:01 +0200 Subject: Migrate Repository#find_commits_by_message to Gitaly Closes gitaly#443 --- GITALY_SERVER_VERSION | 2 +- Gemfile | 2 +- Gemfile.lock | 4 +-- app/models/repository.rb | 39 +++++++++++++++++++++--------- lib/gitlab/gitaly_client/commit_service.rb | 14 +++++++++++ spec/models/repository_spec.rb | 30 ++++++++++++++++------- 6 files changed, 67 insertions(+), 24 deletions(-) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 1b58cc10180..ae6dd4e2032 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -0.27.0 +0.29.0 diff --git a/Gemfile b/Gemfile index 37a4eb5fde4..6f92326910d 100644 --- a/Gemfile +++ b/Gemfile @@ -391,7 +391,7 @@ gem 'vmstat', '~> 2.3.0' gem 'sys-filesystem', '~> 1.1.6' # Gitaly GRPC client -gem 'gitaly', '~> 0.24.0' +gem 'gitaly', '~> 0.26.0' gem 'toml-rb', '~> 0.3.15', require: false diff --git a/Gemfile.lock b/Gemfile.lock index 04d17d54636..24ddf36fa44 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -269,7 +269,7 @@ GEM po_to_json (>= 1.0.0) rails (>= 3.2.0) gherkin-ruby (0.3.2) - gitaly (0.24.0) + gitaly (0.26.0) google-protobuf (~> 3.1) grpc (~> 1.0) github-linguist (4.7.6) @@ -975,7 +975,7 @@ DEPENDENCIES gettext (~> 3.2.2) gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails_js (~> 1.2.0) - gitaly (~> 0.24.0) + gitaly (~> 0.26.0) github-linguist (~> 4.7.0) gitlab-flowdock-git-hook (~> 1.0.1) gitlab-markup (~> 1.5.1) diff --git a/app/models/repository.rb b/app/models/repository.rb index f86a0869b01..3b5d0e00c70 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -130,17 +130,13 @@ class Repository return [] end - 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 + raw_repository.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 + end end def find_branch(name, fresh_repo: true) @@ -1184,4 +1180,25 @@ class Repository def circuit_breaker @circuit_breaker ||= Gitlab::Git::Storage::CircuitBreaker.for_storage(project.repository_storage) 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) + raw_repository + .gitaly_commit_client + .commits_by_message(query, revision: ref, path: path, limit: limit, offset: offset) + .map { |c| commit(c) } + end end diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb index ac6817e6d0e..3f577ac8530 100644 --- a/lib/gitlab/gitaly_client/commit_service.rb +++ b/lib/gitlab/gitaly_client/commit_service.rb @@ -135,6 +135,20 @@ module Gitlab consume_commits_response(response) end + def commits_by_message(query, revision: '', path: '', limit: 1000, offset: 0) + request = Gitaly::CommitsByMessageRequest.new( + repository: @gitaly_repo, + query: query, + revision: revision.to_s.force_encoding(Encoding::ASCII_8BIT), + path: path.to_s.force_encoding(Encoding::ASCII_8BIT), + limit: limit.to_i, + offset: offset.to_i + ) + + response = GitalyClient.call(@repository.storage, :commit_service, :commits_by_message, request) + consume_commits_response(response) + end + def languages(ref = nil) request = Gitaly::CommitLanguagesRequest.new(repository: @gitaly_repo, revision: ref || '') response = GitalyClient.call(@repository.storage, :commit_service, :commit_languages, request) diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 4ddda5b638c..cfa77648338 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -234,19 +234,31 @@ describe Repository, models: true do end describe '#find_commits_by_message' do - it 'returns commits with messages containing a given string' do - commit_ids = repository.find_commits_by_message('submodule').map(&:id) + shared_examples 'finding commits by message' do + it 'returns commits with messages containing a given string' do + commit_ids = repository.find_commits_by_message('submodule').map(&:id) - expect(commit_ids).to include('5937ac0a7beb003549fc5fd26fc247adbce4a52e') - expect(commit_ids).to include('6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') - expect(commit_ids).to include('cfe32cf61b73a0d5e9f13e774abde7ff789b1660') - expect(commit_ids).not_to include('913c66a37b4a45b9769037c55c2d238bd0942d2e') + expect(commit_ids).to include( + '5937ac0a7beb003549fc5fd26fc247adbce4a52e', + '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9', + 'cfe32cf61b73a0d5e9f13e774abde7ff789b1660' + ) + expect(commit_ids).not_to include('913c66a37b4a45b9769037c55c2d238bd0942d2e') + end + + it 'is case insensitive' do + commit_ids = repository.find_commits_by_message('SUBMODULE').map(&:id) + + expect(commit_ids).to include('5937ac0a7beb003549fc5fd26fc247adbce4a52e') + end end - it 'is case insensitive' do - commit_ids = repository.find_commits_by_message('SUBMODULE').map(&:id) + context 'when Gitaly commits_by_message feature is enabled' do + it_behaves_like 'finding commits by message' + end - expect(commit_ids).to include('5937ac0a7beb003549fc5fd26fc247adbce4a52e') + context 'when Gitaly commits_by_message feature is disabled', skip_gitaly_mock: true do + it_behaves_like 'finding commits by message' end describe 'when storage is broken', broken_storage: true do -- cgit v1.2.1