Add handling for commit/tags with big messagesfix/handle-large-commit-tag-message

11 files changed, 287 insertions, 17 deletions

diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION
index 8f63f4f9a10..e5c6d253316 100644
--- a/GITALY_SERVER_VERSION
+++ b/GITALY_SERVER_VERSION
@@ -1 +1 @@
-0.91.0
+=fix/support-large-tag-commit-messages
diff --git a/Gemfile b/Gemfile
index e423f4ba32f..8683be5fe1e 100644
--- a/Gemfile
+++ b/Gemfile
@@ -407,7 +407,8 @@ group :ed25519 do
 end
 
 # Gitaly GRPC client
-gem 'gitaly-proto', '~> 0.88.0', require: 'gitaly'
+# gem 'gitaly-proto', '~> 0.88.0', require: 'gitaly'
+gem 'gitaly-proto', require: 'gitaly', git: 'https://gitlab.com/gitlab-org/gitaly-proto.git', ref: 'fix/large-commit-tag-message'
 gem 'grpc', '~> 1.10.0'
 
 # Locked until https://github.com/google/protobuf/issues/4210 is closed
diff --git a/Gemfile.lock b/Gemfile.lock
index 1c6c7edb1a0..00106e6f415 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -1,3 +1,12 @@
+GIT
+  remote: https://gitlab.com/gitlab-org/gitaly-proto.git
+  revision: 9fa062ff6b1580f57ac30091a385fa8dea24b475
+  ref: fix/large-commit-tag-message
+  specs:
+    gitaly-proto (0.89.0)
+      google-protobuf (~> 3.1)
+      grpc (~> 1.0)
+
 GEM
   remote: https://rubygems.org/
   specs:
@@ -288,9 +297,6 @@ GEM
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
     gherkin-ruby (0.3.2)
-    gitaly-proto (0.88.0)
-      google-protobuf (~> 3.1)
-      grpc (~> 1.0)
     github-linguist (5.3.3)
       charlock_holmes (~> 0.7.5)
       escape_utils (~> 1.1.0)
@@ -1056,7 +1062,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.2.0)
-  gitaly-proto (~> 0.88.0)
+  gitaly-proto!
   github-linguist (~> 5.3.3)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-markup (~> 1.6.2)
diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb
index 93037ed8d90..9598da61ce5 100644
--- a/lib/gitlab/git/commit.rb
+++ b/lib/gitlab/git/commit.rb
@@ -6,6 +6,7 @@ module Gitlab
 
       attr_accessor :raw_commit, :head
 
+      MAX_COMMIT_MESSAGE_DISPLAY_SIZE = 10.megabytes
       MIN_SHA_LENGTH = 7
       SERIALIZE_KEYS = [
         :id, :message, :parent_ids,
@@ -63,9 +64,7 @@ module Gitlab
             if is_enabled
               repo.gitaly_commit_client.find_commit(commit_id)
             else
-              obj = repo.rev_parse_target(commit_id)
-
-              obj.is_a?(Rugged::Commit) ? obj : nil
+              rugged_find(repo, commit_id)
             end
           end
 
@@ -76,6 +75,12 @@ module Gitlab
           nil
         end
 
+        def rugged_find(repo, commit_id)
+          obj = repo.rev_parse_target(commit_id)
+
+          obj.is_a?(Rugged::Commit) ? obj : nil
+        end
+
         # Get last commit for HEAD
         #
         # Ex.
@@ -296,11 +301,40 @@ module Gitlab
             nil
           end
         end
+
+        def get_commit_message(repository, commit_id)
+          BatchLoader.for({ repository: repository, commit_id: commit_id }).batch do |items, loader|
+            items_by_repo = items.group_by { |i| i[:repository] }
+
+            items_by_repo.each do |repo, items|
+              commit_ids = items.map { |i| i[:commit_id] }
+
+              messages = get_commit_messages(repository, commit_ids)
+
+              messages.each do |commit_sha, message|
+                loader.call({ repository: repository, commit_id: commit_sha }, message)
+              end
+            end
+          end
+        end
+
+        def get_commit_messages(repository, commit_ids)
+          repository.gitaly_migrate(:commit_messages) do |is_enabled|
+            if is_enabled
+              repository.gitaly_commit_client.get_commit_messages(commit_ids)
+            else
+              commit_ids.map { |id| [id, rugged_find(repository, id).message] }.to_h
+            end
+          end
+        end
       end
 
       def initialize(repository, raw_commit, head = nil)
         raise "Nil as raw commit passed" unless raw_commit
 
+        @repository = repository
+        @head = head
+
         case raw_commit
         when Hash
           init_from_hash(raw_commit)
@@ -311,9 +345,6 @@ module Gitlab
         else
           raise "Invalid raw commit type: #{raw_commit.class}"
         end
-
-        @repository = repository
-        @head = head
       end
 
       def sha
@@ -540,7 +571,7 @@ module Gitlab
         # 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
+        @message = message_from_gitaly_body
         @authored_date = Time.at(commit.author.date.seconds).utc
         @author_name = commit.author.name.dup
         @author_email = commit.author.email.dup
@@ -592,6 +623,25 @@ module Gitlab
       def refs(repo)
         repo.refs_hash[id]
       end
+
+      def message_from_gitaly_body
+        return @raw_commit.body.dup if @raw_commit.body.present?
+        return message_from_gitaly_subject if @raw_commit.body_size.zero?
+
+        if @raw_commit.body_size > MAX_COMMIT_MESSAGE_DISPLAY_SIZE
+          "#{message_from_gitaly_subject}\n\n--commit message is too big".strip
+        else
+          fetch_body_from_gitaly
+        end
+      end
+
+      def message_from_gitaly_subject
+        @raw_commit.subject.dup
+      end
+
+      def fetch_body_from_gitaly
+        self.class.get_commit_message(@repository, id)
+      end
     end
   end
 end
diff --git a/lib/gitlab/git/tag.rb b/lib/gitlab/git/tag.rb
index 8a8f7b051ed..f1560650569 100644
--- a/lib/gitlab/git/tag.rb
+++ b/lib/gitlab/git/tag.rb
@@ -1,17 +1,86 @@
 module Gitlab
   module Git
     class Tag < Ref
+      extend Gitlab::EncodingHelper
+
       attr_reader :object_sha
 
+      MAX_TAG_MESSAGE_DISPLAY_SIZE = 10.megabytes
+
+      class << self
+        def get_tag_message(repository, tag_name)
+          BatchLoader.for({ repository: repository, tag_name: tag_name }).batch do |items, loader|
+            items_by_repo = items.group_by { |i| i[:repository] }
+
+            items_by_repo.each do |repo, items|
+              tag_names = items.map { |i| i[:tag_name] }
+
+              messages = get_tag_messages(repository, tag_names)
+
+              messages.each do |name, message|
+                loader.call({ repository: repository, tag_name: name }, message)
+              end
+            end
+          end
+        end
+
+        def get_tag_messages(repository, tag_names)
+          repository.gitaly_migrate(:tag_messages) do |is_enabled|
+            if is_enabled
+              repository.gitaly_ref_client.get_tag_messages(tag_names)
+            else
+              tag_names.map do |name|
+                tag = repository.rugged.tags[name]
+                next [name, ""] unless tag
+
+                annotation = tag.annotation
+                next [name, ""] unless tag.annotation
+
+                [name, annotation.message]
+              end.to_h
+            end
+          end
+        end
+
+        def new_from_gitaly(repository, gitaly_tag)
+          if gitaly_tag.target_commit.present?
+            commit = Gitlab::Git::Commit.decorate(repository, gitaly_tag.target_commit)
+          end
+
+          tag = new(repository, encode!(gitaly_tag.name.dup), gitaly_tag.id, commit, encode!(gitaly_tag.message.chomp))
+          tag.init_from_gitaly(gitaly_tag)
+
+          tag
+        end
+      end
+
       def initialize(repository, name, target, target_commit, message = nil)
         super(repository, name, target, target_commit)
 
+        @repository = repository
         @message = message
       end
 
+      def init_from_gitaly(gitaly_tag)
+        @raw_tag = gitaly_tag
+        @message = message_from_gitaly_tag
+      end
+
       def message
         encode! @message
       end
+
+      private
+
+      def message_from_gitaly_tag
+        return @raw_tag.message.dup if @raw_tag.message.present?
+
+        if @raw_tag.message_size > MAX_TAG_MESSAGE_DISPLAY_SIZE
+          '--tag message is too big'
+        else
+          self.class.get_tag_message(@repository, name)
+        end
+      end
     end
   end
 end
diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb
index 456a8a1a2d6..2f62aa73734 100644
--- a/lib/gitlab/gitaly_client/commit_service.rb
+++ b/lib/gitlab/gitaly_client/commit_service.rb
@@ -338,6 +338,22 @@ module Gitlab
         signatures
       end
 
+      def get_commit_messages(commit_ids)
+        request = Gitaly::GetCommitMessagesRequest.new(repository: @gitaly_repo, commit_ids: commit_ids)
+        response = GitalyClient.call(@repository.storage, :commit_service, :get_commit_messages, request)
+
+        messages = Hash.new { |h, k| h[k] = ''.b }
+        current_commit_id = nil
+
+        response.each do |rpc_message|
+          current_commit_id = rpc_message.commit_id if rpc_message.commit_id.present?
+
+          messages[current_commit_id] << rpc_message.message
+        end
+
+        messages
+      end
+
       private
 
       def call_commit_diff(request_params, options = {})
diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb
index 831cfd1e014..885aee2f524 100644
--- a/lib/gitlab/gitaly_client/operation_service.rb
+++ b/lib/gitlab/gitaly_client/operation_service.rb
@@ -40,7 +40,7 @@ module Gitlab
           raise Gitlab::Git::Repository::TagExistsError
         end
 
-        Util.gitlab_tag_from_gitaly_tag(@repository, response.tag)
+        Gitlab::Git::Tag.new_from_gitaly(@repository, response.tag)
       rescue GRPC::FailedPrecondition => e
         raise Gitlab::Git::Repository::InvalidRef, e
       end
diff --git a/lib/gitlab/gitaly_client/ref_service.rb b/lib/gitlab/gitaly_client/ref_service.rb
index ba6b577fd17..edd086f8215 100644
--- a/lib/gitlab/gitaly_client/ref_service.rb
+++ b/lib/gitlab/gitaly_client/ref_service.rb
@@ -171,6 +171,25 @@ module Gitlab
         consume_ref_contains_sha_response(stream, :branch_names)
       end
 
+      def get_tag_messages(tag_names)
+        request = Gitaly::GetTagMessagesRequest.new(
+          repository: @gitaly_repo,
+          tag_names: tag_names.map { |name| name.b }
+        )
+        response = GitalyClient.call(@repository.storage, :ref_service, :get_tag_messages, request)
+
+        messages = Hash.new { |h, k| h[k] = ''.b }
+        current_tag_name = nil
+
+        response.each do |rpc_message|
+          current_tag_name = rpc_message.tag_name if rpc_message.tag_name.present?
+
+          messages[current_tag_name] << rpc_message.message
+        end
+
+        messages
+      end
+
       private
 
       def consume_refs_response(response)
@@ -210,7 +229,7 @@ module Gitlab
 
       def consume_tags_response(response)
         response.flat_map do |message|
-          message.tags.map { |gitaly_tag| Util.gitlab_tag_from_gitaly_tag(@repository, gitaly_tag) }
+          message.tags.map { |gitaly_tag| Gitlab::Git::Tag.new_from_gitaly(@repository, gitaly_tag) }
         end
       end
 
diff --git a/spec/factories/gitaly/tag.rb b/spec/factories/gitaly/tag.rb
new file mode 100644
index 00000000000..e99776d524a
--- /dev/null
+++ b/spec/factories/gitaly/tag.rb
@@ -0,0 +1,9 @@
+FactoryBot.define do
+  factory :gitaly_tag, class: Gitaly::Tag do
+    skip_create
+
+    name { 'v3.1.4' }
+    message { 'Pie release' }
+    target_commit factory: :gitaly_commit
+  end
+end
diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb
index a05feaac1ca..ef4dc728ce5 100644
--- a/spec/lib/gitlab/git/commit_spec.rb
+++ b/spec/lib/gitlab/git/commit_spec.rb
@@ -66,7 +66,8 @@ describe Gitlab::Git::Commit, seed_helper: true do
   describe "Commit info from gitaly commit" do
     let(:subject) { "My commit".force_encoding('ASCII-8BIT') }
     let(:body) { subject + "My body".force_encoding('ASCII-8BIT') }
-    let(:gitaly_commit) { build(:gitaly_commit, subject: subject, body: body) }
+    let(:body_size) { body.length }
+    let(:gitaly_commit) { build(:gitaly_commit, subject: subject, body: body, body_size: body_size) }
     let(:id) { gitaly_commit.id }
     let(:committer) { gitaly_commit.committer }
     let(:author) { gitaly_commit.author }
@@ -86,7 +87,27 @@ describe Gitlab::Git::Commit, seed_helper: true do
     context 'no body' do
       let(:body) { "".force_encoding('ASCII-8BIT') }
 
-      it { expect(commit.safe_message).to eq(subject) }
+      context 'zero body_size' do
+        it { expect(commit.safe_message).to eq(subject) }
+      end
+
+      context 'body_size less than threshold' do
+        let(:body_size) { 123 }
+
+        it 'fetches commit message seperately' do
+          expect(described_class).to receive(:get_commit_message).with(repository, id)
+
+          commit.safe_message
+        end
+      end
+
+      context 'body_size greater than threshold' do
+        let(:body_size) { 11.megabytes }
+
+        it 'returns the suject plus a notice about message size' do
+          expect(commit.safe_message).to eq("My commit\n\n--commit message is too big")
+        end
+      end
     end
   end
 
@@ -603,6 +624,33 @@ describe Gitlab::Git::Commit, seed_helper: true do
     it { is_expected.not_to include("feature") }
   end
 
+  describe '.get_commit_message' do
+    let(:commit_ids) { %w[6d394385cf567f80a8fd85055db1ab4c5295806f cfe32cf61b73a0d5e9f13e774abde7ff789b1660] }
+
+    subject do
+      commit_ids.map { |id| described_class.get_commit_message(repository, id) }
+    end
+
+    shared_examples 'getting commit messages' do
+      it 'gets commit messages' do
+        expect(subject[0]).to eq("Added contributing guide\n\nSigned-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>\n")
+        expect(subject[1]).to eq("Add submodule\n\nSigned-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>\n")
+      end
+    end
+
+    context 'when Gitaly commit_messages feature is enabled' do
+      it_behaves_like 'getting commit messages'
+
+      it 'gets messages in one batch', :request_store do
+        expect { subject.map(&:itself) }.to change { Gitlab::GitalyClient.get_request_count }.by(1)
+      end
+    end
+
+    context 'when Gitaly commit_messages feature is disabled', :disable_gitaly do
+      it_behaves_like 'getting commit messages'
+    end
+  end
+
   def sample_commit_hash
     {
       author_email: "dmitriy.zaporozhets@gmail.com",
diff --git a/spec/lib/gitlab/git/tag_spec.rb b/spec/lib/gitlab/git/tag_spec.rb
index 6c4f538bf01..dc06d463489 100644
--- a/spec/lib/gitlab/git/tag_spec.rb
+++ b/spec/lib/gitlab/git/tag_spec.rb
@@ -32,4 +32,56 @@ describe Gitlab::Git::Tag, seed_helper: true do
   context 'when Gitaly tags feature is disabled', :skip_gitaly_mock do
     it_behaves_like 'Gitlab::Git::Repository#tags'
   end
+
+  describe '.get_tag_message' do
+    let(:tag_names) { %w[v1.0.0 v1.1.0] }
+
+    subject do
+      tag_names.map { |name| described_class.get_tag_message(repository, name) }
+    end
+
+    shared_examples 'getting tag messages' do
+      it 'gets tag messages' do
+        expect(subject[0]).to eq("Release\n")
+        expect(subject[1]).to eq("Version 1.1.0\n")
+      end
+    end
+
+    context 'when Gitaly tag_messages feature is enabled' do
+      it_behaves_like 'getting tag messages'
+
+      it 'gets messages in one batch', :request_store do
+        expect { subject.map(&:itself) }.to change { Gitlab::GitalyClient.get_request_count }.by(1)
+      end
+    end
+
+    context 'when Gitaly tag_messages feature is disabled', :disable_gitaly do
+      it_behaves_like 'getting tag messages'
+    end
+  end
+
+  describe 'tag into from Gitaly tag' do
+    context 'no message' do
+      let(:gitaly_tag) { build(:gitaly_tag, message: ''.b, message_size: message_size) }
+      let(:tag) { described_class.new_from_gitaly(repository, gitaly_tag) }
+
+      context 'message_size less than threshold' do
+        let(:message_size) { 123 }
+
+        it 'fetches tag message seperately' do
+          expect(described_class).to receive(:get_tag_message).with(repository, gitaly_tag.name)
+
+          tag.message
+        end
+      end
+
+      context 'message_size greater than threshold' do
+        let(:message_size) { 11.megabytes }
+
+        it 'returns a notice about message size' do
+          expect(tag.message).to eq("--tag message is too big")
+        end
+      end
+    end
+  end
 end