Migrate Repository#find_commits_by_message to Gitalyfeature/migrate-find-commits-by-message-to-gitaly

Closes gitaly#443

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