diff options
-rw-r--r-- | GITALY_SERVER_VERSION | 2 | ||||
-rw-r--r-- | Gemfile | 3 | ||||
-rw-r--r-- | Gemfile.lock | 14 | ||||
-rw-r--r-- | lib/gitlab/git/commit.rb | 64 | ||||
-rw-r--r-- | lib/gitlab/git/tag.rb | 69 | ||||
-rw-r--r-- | lib/gitlab/gitaly_client/commit_service.rb | 16 | ||||
-rw-r--r-- | lib/gitlab/gitaly_client/operation_service.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/gitaly_client/ref_service.rb | 21 | ||||
-rw-r--r-- | spec/factories/gitaly/tag.rb | 9 | ||||
-rw-r--r-- | spec/lib/gitlab/git/commit_spec.rb | 52 | ||||
-rw-r--r-- | spec/lib/gitlab/git/tag_spec.rb | 52 |
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 @@ -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 |