Incorporate Gitaly's Commits#between RPCgitaly-commits-between

10 files changed, 79 insertions, 24 deletions

diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION
index a803cc227fe..d99320d6803 100644
--- a/GITALY_SERVER_VERSION
+++ b/GITALY_SERVER_VERSION
@@ -1 +1 @@
-0.14.0
+=commits-between-reverse
diff --git a/Gemfile b/Gemfile
index a5f7d0dfd12..4a94e6317bd 100644
--- a/Gemfile
+++ b/Gemfile
@@ -386,7 +386,7 @@ gem 'vmstat', '~> 2.3.0'
 gem 'sys-filesystem', '~> 1.1.6'
 
 # Gitaly GRPC client
-gem 'gitaly', '~> 0.12.1'
+gem 'gitaly', git: 'https://gitlab.com/gitlab-org/gitaly-proto.git', branch: 'commit-body'
 
 gem 'toml-rb', '~> 0.3.15', require: false
 
diff --git a/Gemfile.lock b/Gemfile.lock
index 7bbc0c538d6..28b7ed9bf0c 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -1,3 +1,12 @@
+GIT
+  remote: https://gitlab.com/gitlab-org/gitaly-proto.git
+  revision: 3ec8e53e6aae00e1ec42ccbc7c274a364fbe3807
+  branch: commit-body
+  specs:
+    gitaly (0.13.0)
+      google-protobuf (~> 3.1)
+      grpc (~> 1.0)
+
 GEM
   remote: https://rubygems.org/
   specs:
@@ -278,9 +287,6 @@ GEM
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
     gherkin-ruby (0.3.2)
-    gitaly (0.12.1)
-      google-protobuf (~> 3.1)
-      grpc (~> 1.0)
     github-linguist (4.7.6)
       charlock_holmes (~> 0.7.3)
       escape_utils (~> 1.1.0)
@@ -980,7 +986,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.2.0)
-  gitaly (~> 0.12.1)
+  gitaly!
   github-linguist (~> 4.7.0)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-markup (~> 1.5.1)
diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb
index f5521ba5778..8915563b426 100644
--- a/lib/gitlab/git/commit.rb
+++ b/lib/gitlab/git/commit.rb
@@ -97,7 +97,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
@@ -372,12 +380,13 @@ module Gitlab
       def init_from_gitaly(commit)
         @raw_commit = commit
         @id = commit.id
-        # NOTE: For ease of parsing in Gitaly, we have only the subject of
-        # the commit and not the full message.
         # 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.subject.dup
+        if commit.body.present?
+          @message += "\n\n#{commit.body.dup}"
+        end
         @authored_date = Time.at(commit.author.date.seconds)
         @author_name = commit.author.name.dup
         @author_email = commit.author.email.dup
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index dd5a4d5ad55..7a2a7252b4e 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -899,6 +899,14 @@ module Gitlab
         Gitlab::GitalyClient::Util.repository(@storage, @relative_path)
       end
 
+      def gitaly_ref_client
+        @gitaly_ref_client ||= Gitlab::GitalyClient::Ref.new(self)
+      end
+
+      def gitaly_commit_client
+        @gitaly_commit_client ||= Gitlab::GitalyClient::Commit.new(self)
+      end
+
       private
 
       # We are trying to deprecate this method because it does a lot of work
@@ -1167,14 +1175,6 @@ module Gitlab
         end
       end
 
-      def gitaly_ref_client
-        @gitaly_ref_client ||= Gitlab::GitalyClient::Ref.new(self)
-      end
-
-      def gitaly_commit_client
-        @gitaly_commit_client ||= Gitlab::GitalyClient::Commit.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.rb b/lib/gitlab/gitaly_client/commit.rb
index b8877619797..6506025c448 100644
--- a/lib/gitlab/gitaly_client/commit.rb
+++ b/lib/gitlab/gitaly_client/commit.rb
@@ -56,6 +56,17 @@ module Gitlab
         entry
       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 = {})
@@ -68,6 +79,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/spec/lib/gitlab/email/message/repository_push_spec.rb b/spec/lib/gitlab/email/message/repository_push_spec.rb
index 7b3291b8315..04412711f56 100644
--- a/spec/lib/gitlab/email/message/repository_push_spec.rb
+++ b/spec/lib/gitlab/email/message/repository_push_spec.rb
@@ -111,8 +111,8 @@ describe Gitlab::Email::Message::RepositoryPush do
     describe '#target_url' do
       subject { message.target_url }
       it { is_expected.to include 'compare' }
-      it { is_expected.to include compare.commits.first.parents.first.id }
-      it { is_expected.to include compare.commits.last.id }
+      it { is_expected.to include compare.start_commit.id }
+      it { is_expected.to include compare.head_commit.id }
     end
 
     describe '#subject' do
diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb
index 62b353890d6..c8516ec8d48 100644
--- a/spec/lib/gitlab/git/commit_spec.rb
+++ b/spec/lib/gitlab/git/commit_spec.rb
@@ -67,6 +67,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
   describe "Commit info from gitaly commit" do
     let(:id) { 'f00' }
     let(:subject) { "My commit".force_encoding('ASCII-8BIT') }
+    let(:body) { "My body".force_encoding('ASCII-8BIT') }
     let(:committer) do
       Gitaly::CommitAuthor.new(
         name: generate(:name),
@@ -83,7 +84,11 @@ describe Gitlab::Git::Commit, seed_helper: true do
     end
     let(:gitaly_commit) do
       Gitaly::GitCommit.new(
-        id: id, subject: subject, author: author, committer: committer
+        id: id,
+        subject: subject,
+        body: body,
+        author: author,
+        committer: committer
       )
     end
     let(:commit) { described_class.new(gitaly_commit) }
@@ -91,12 +96,18 @@ describe Gitlab::Git::Commit, seed_helper: true do
     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(subject) }
+    it { expect(commit.safe_message).to eq("#{subject}\n\n#{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
diff --git a/spec/lib/gitlab/gitaly_client/commit_spec.rb b/spec/lib/gitlab/gitaly_client/commit_spec.rb
index dff5b25c712..30e2cedde33 100644
--- a/spec/lib/gitlab/gitaly_client/commit_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/commit_spec.rb
@@ -1,7 +1,6 @@
 require 'spec_helper'
 
 describe Gitlab::GitalyClient::Commit do
-  let(:diff_stub) { double('Gitaly::Diff::Stub') }
   let(:project) { create(:project, :repository) }
   let(:repository) { project.repository }
   let(:repository_message) { repository.gitaly_repository }
@@ -82,4 +81,19 @@ describe Gitlab::GitalyClient::Commit 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 8e8816870e1..9aaa9282555 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)