Merge branch 'gitaly-commits-between' into 'master'

Gitaly commits between

Closes gitaly#350

See merge request !12669

7 files changed, 117 insertions, 16 deletions

diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb
index d0f04d25db2..76a562f356e 100644
--- a/lib/gitlab/git/commit.rb
+++ b/lib/gitlab/git/commit.rb
@@ -98,7 +98,15 @@ module Gitlab
         #   Commit.between(repo, '29eda46b', 'master')
         #
         def between(repo, base, head)
-          repo.commits_between(base, head).map do |commit|
+          commits = Gitlab::GitalyClient.migrate(:commits_between) do |is_enabled|
+            if is_enabled
+              repo.gitaly_commit_client.between(base, head)
+            else
+              repo.commits_between(base, head)
+            end
+          end
+
+          commits.map do |commit|
             decorate(commit)
           end
         rescue Rugged::ReferenceError
@@ -210,6 +218,8 @@ module Gitlab
           init_from_hash(raw_commit)
         elsif raw_commit.is_a?(Rugged::Commit)
           init_from_rugged(raw_commit)
+        elsif raw_commit.is_a?(Gitaly::GitCommit)
+          init_from_gitaly(raw_commit)
         else
           raise "Invalid raw commit type: #{raw_commit.class}"
         end
@@ -371,6 +381,22 @@ module Gitlab
         @parent_ids = commit.parents.map(&:oid)
       end
 
+      def init_from_gitaly(commit)
+        @raw_commit = commit
+        @id = commit.id
+        # TODO: Once gitaly "takes over" Rugged consider separating the
+        # subject from the message to make it clearer when there's one
+        # available but not the other.
+        @message = (commit.body.presence || commit.subject).dup
+        @authored_date = Time.at(commit.author.date.seconds)
+        @author_name = commit.author.name.dup
+        @author_email = commit.author.email.dup
+        @committed_date = Time.at(commit.committer.date.seconds)
+        @committer_name = commit.committer.name.dup
+        @committer_email = commit.committer.email.dup
+        @parent_ids = commit.parent_ids
+      end
+
       def serialize_keys
         SERIALIZE_KEYS
       end
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index c091bb9dcfe..63eebadff2e 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -807,6 +807,14 @@ module Gitlab
         Gitlab::GitalyClient::Util.repository(@storage, @relative_path)
       end
 
+      def gitaly_ref_client
+        @gitaly_ref_client ||= Gitlab::GitalyClient::RefService.new(self)
+      end
+
+      def gitaly_commit_client
+        @gitaly_commit_client ||= Gitlab::GitalyClient::CommitService.new(self)
+      end
+
       private
 
       # Gitaly note: JV: Trying to get rid of the 'filter' option so we can implement this with 'git'.
@@ -1105,14 +1113,6 @@ module Gitlab
         end
       end
 
-      def gitaly_ref_client
-        @gitaly_ref_client ||= Gitlab::GitalyClient::RefService.new(self)
-      end
-
-      def gitaly_commit_client
-        @gitaly_commit_client ||= Gitlab::GitalyClient::CommitService.new(self)
-      end
-
       def gitaly_migrate(method, &block)
         Gitlab::GitalyClient.migrate(method, &block)
       rescue GRPC::NotFound => e
diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb
index 470e3ac8779..8f5738fed06 100644
--- a/lib/gitlab/gitaly_client/commit_service.rb
+++ b/lib/gitlab/gitaly_client/commit_service.rb
@@ -65,6 +65,17 @@ module Gitlab
         GitalyClient.call(@repository.storage, :commit_service, :count_commits, request).count
       end
 
+      def between(from, to)
+        request = Gitaly::CommitsBetweenRequest.new(
+          repository: @gitaly_repo,
+          from: from,
+          to: to
+        )
+
+        response = GitalyClient.call(@repository.storage, :commit_service, :commits_between, request)
+        consume_commits_response(response)
+      end
+
       private
 
       def commit_diff_request_params(commit, options = {})
@@ -77,6 +88,10 @@ module Gitlab
           paths: options.fetch(:paths, [])
         }
       end
+
+      def consume_commits_response(response)
+        response.flat_map { |r| r.commits }
+      end
     end
   end
 end
diff --git a/lib/gitlab/gitaly_client/ref_service.rb b/lib/gitlab/gitaly_client/ref_service.rb
index f541887843d..2c3d53410ac 100644
--- a/lib/gitlab/gitaly_client/ref_service.rb
+++ b/lib/gitlab/gitaly_client/ref_service.rb
@@ -96,11 +96,11 @@ module Gitlab
           id: response.commit_id,
           message: message,
           authored_date: Time.at(response.commit_author.date.seconds),
-          author_name: response.commit_author.name,
-          author_email: response.commit_author.email,
+          author_name: response.commit_author.name.dup,
+          author_email: response.commit_author.email.dup,
           committed_date: Time.at(response.commit_committer.date.seconds),
-          committer_name: response.commit_committer.name,
-          committer_email: response.commit_committer.email
+          committer_name: response.commit_committer.name.dup,
+          committer_email: response.commit_committer.email.dup
         }
 
         Gitlab::Git::Commit.decorate(hash)
diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb
index f20a14155dc..60de91324f0 100644
--- a/spec/lib/gitlab/git/commit_spec.rb
+++ b/spec/lib/gitlab/git/commit_spec.rb
@@ -64,6 +64,52 @@ describe Gitlab::Git::Commit, seed_helper: true do
     end
   end
 
+  describe "Commit info from gitaly commit" do
+    let(:id) { 'f00' }
+    let(:subject) { "My commit".force_encoding('ASCII-8BIT') }
+    let(:body) { subject + "My body".force_encoding('ASCII-8BIT') }
+    let(:committer) do
+      Gitaly::CommitAuthor.new(
+        name: generate(:name),
+        email: generate(:email),
+        date: Google::Protobuf::Timestamp.new(seconds: 123)
+      )
+    end
+    let(:author) do
+      Gitaly::CommitAuthor.new(
+        name: generate(:name),
+        email: generate(:email),
+        date: Google::Protobuf::Timestamp.new(seconds: 456)
+      )
+    end
+    let(:gitaly_commit) do
+      Gitaly::GitCommit.new(
+        id: id,
+        subject: subject,
+        body: body,
+        author: author,
+        committer: committer
+      )
+    end
+    let(:commit) { described_class.new(gitaly_commit) }
+
+    it { expect(commit.short_id).to eq(id[0..10]) }
+    it { expect(commit.id).to eq(id) }
+    it { expect(commit.sha).to eq(id) }
+    it { expect(commit.safe_message).to eq(body) }
+    it { expect(commit.created_at).to eq(Time.at(committer.date.seconds)) }
+    it { expect(commit.author_email).to eq(author.email) }
+    it { expect(commit.author_name).to eq(author.name) }
+    it { expect(commit.committer_name).to eq(committer.name) }
+    it { expect(commit.committer_email).to eq(committer.email) }
+
+    context 'no body' do
+      let(:body) { "".force_encoding('ASCII-8BIT') }
+
+      it { expect(commit.safe_message).to eq(subject) }
+    end
+  end
+
   context 'Class methods' do
     describe '.find' do
       it "should return first head commit if without params" do
diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
index fee5bb45fe5..93affb12f2b 100644
--- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
@@ -1,7 +1,6 @@
 require 'spec_helper'
 
 describe Gitlab::GitalyClient::CommitService do
-  let(:diff_stub) { double('Gitaly::DiffService::Stub') }
   let(:project) { create(:project, :repository) }
   let(:repository) { project.repository }
   let(:repository_message) { repository.gitaly_repository }
@@ -82,4 +81,19 @@ describe Gitlab::GitalyClient::CommitService do
       end
     end
   end
+
+  describe '#between' do
+    let(:from) { 'master' }
+    let(:to) { '4b825dc642cb6eb9a060e54bf8d69288fbee4904' }
+    it 'sends an RPC request' do
+      request = Gitaly::CommitsBetweenRequest.new(
+        repository: repository_message, from: from, to: to
+      )
+
+      expect_any_instance_of(Gitaly::CommitService::Stub).to receive(:commits_between)
+        .with(request, kind_of(Hash)).and_return([])
+
+      described_class.new(repository).between(from, to)
+    end
+  end
 end
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index 3f77ed10069..c493c08a7ae 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -108,7 +108,7 @@ describe GitPushService, services: true do
 
         it { is_expected.to include(id: @commit.id) }
         it { is_expected.to include(message: @commit.safe_message) }
-        it { is_expected.to include(timestamp: @commit.date.xmlschema) }
+        it { expect(subject[:timestamp].in_time_zone).to eq(@commit.date.in_time_zone) }
         it do
           is_expected.to include(
             url: [
@@ -163,7 +163,7 @@ describe GitPushService, services: true do
         execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
       end
     end
-    
+
     context "Sends System Push data" do
       it "when pushing on a branch" do
         expect(SystemHookPushWorker).to receive(:perform_async).with(@push_data, :push_hooks)