Merge branch 'feature/migrate-find-commits-by-message-to-gitaly' into 'master'

Migrate Repository#find_commits_by_message to Gitaly

Closes gitaly#443

See merge request !13268

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

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)

spec/models/repository_spec.rb

@@ -234,12 +234,15 @@ describe Repository, models: true do
   end
 
   describe '#find_commits_by_message' do
+    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).to include(
+          '5937ac0a7beb003549fc5fd26fc247adbce4a52e',
+          '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9',
+          'cfe32cf61b73a0d5e9f13e774abde7ff789b1660'
+        )
         expect(commit_ids).not_to include('913c66a37b4a45b9769037c55c2d238bd0942d2e')
       end
 
@@ -248,6 +251,15 @@ describe Repository, models: true do
 
         expect(commit_ids).to include('5937ac0a7beb003549fc5fd26fc247adbce4a52e')
       end
+    end
+
+    context 'when Gitaly commits_by_message feature is enabled' do
+      it_behaves_like 'finding commits by message'
+    end
+
+    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
       it 'should raise a storage error' do