From 3ce6f03f1437633c9328dc30aa5272a49368655b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Tue, 25 Jul 2017 17:33:06 -0400 Subject: [PATCH] Incorporate Gitaly's CommitService.FindCommit RPC --- app/models/repository.rb | 2 +- lib/gitlab/conflict/file_collection.rb | 4 +- lib/gitlab/git/commit.rb | 52 +++++++++++++------ lib/gitlab/gitaly_client.rb | 4 ++ lib/gitlab/gitaly_client/commit_service.rb | 17 ++++-- spec/lib/gitlab/git/commit_spec.rb | 5 +- .../gitaly_client/commit_service_spec.rb | 14 +++++ ...migrate_process_commit_worker_jobs_spec.rb | 2 +- spec/models/commit_spec.rb | 2 +- 9 files changed, 76 insertions(+), 26 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 3045db06af1..bd9adeaf613 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -769,7 +769,7 @@ class Repository index = Gitlab::Git::Index.new(raw_repository) if start_commit - index.read_tree(start_commit.raw_commit.tree) + index.read_tree(start_commit.rugged_commit.tree) parents = [start_commit.sha] else parents = [] diff --git a/lib/gitlab/conflict/file_collection.rb b/lib/gitlab/conflict/file_collection.rb index 1611eba31da..d671867e7c7 100644 --- a/lib/gitlab/conflict/file_collection.rb +++ b/lib/gitlab/conflict/file_collection.rb @@ -77,8 +77,8 @@ EOM def initialize(merge_request, project) @merge_request = merge_request - @our_commit = merge_request.source_branch_head.raw.raw_commit - @their_commit = merge_request.target_branch_head.raw.raw_commit + @our_commit = merge_request.source_branch_head.raw.rugged_commit + @their_commit = merge_request.target_branch_head.raw.rugged_commit @project = project end end diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb index c3eb3fc44f5..9256663f454 100644 --- a/lib/gitlab/git/commit.rb +++ b/lib/gitlab/git/commit.rb @@ -14,7 +14,7 @@ module Gitlab attr_accessor *SERIALIZE_KEYS # rubocop:disable Lint/AmbiguousOperator - delegate :tree, to: :raw_commit + delegate :tree, to: :rugged_commit def ==(other) return false unless other.is_a?(Gitlab::Git::Commit) @@ -50,19 +50,29 @@ module Gitlab # # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/321 def find(repo, commit_id = "HEAD") + # Already a commit? return commit_id if commit_id.is_a?(Gitlab::Git::Commit) + + # A rugged reference? + commit_id = Gitlab::Git::Ref.dereference_object(commit_id) return decorate(repo, commit_id) if commit_id.is_a?(Rugged::Commit) - obj = if commit_id.is_a?(String) - repo.rev_parse_target(commit_id) - else - Gitlab::Git::Ref.dereference_object(commit_id) - end + # Some weird thing? + return nil unless commit_id.is_a?(String) + + commit = repo.gitaly_migrate(:find_commit) do |is_enabled| + if is_enabled + repo.gitaly_commit_client.find_commit(commit_id) + else + obj = repo.rev_parse_target(commit_id) - return nil unless obj.is_a?(Rugged::Commit) + obj.is_a?(Rugged::Commit) ? obj : nil + end + end - decorate(repo, obj) - rescue Rugged::ReferenceError, Rugged::InvalidError, Rugged::ObjectError, Gitlab::Git::Repository::NoRepository + decorate(repo, commit) if commit + rescue Rugged::ReferenceError, Rugged::InvalidError, Rugged::ObjectError, + Gitlab::Git::CommandError, Gitlab::Git::Repository::NoRepository nil end @@ -273,11 +283,11 @@ module Gitlab break_rewrites = options[:break_rewrites] actual_options = Gitlab::Git::Diff.filter_diff_options(options) - diff = if raw_commit.parents.empty? - raw_commit.diff(actual_options.merge(reverse: true)) - else - raw_commit.parents[0].diff(raw_commit, actual_options) - end + diff = if rugged_commit.parents.empty? + rugged_commit.diff(actual_options.merge(reverse: true)) + else + rugged_commit.parents[0].diff(rugged_commit, actual_options) + end diff.find_similar!(break_rewrites: break_rewrites) diff @@ -340,7 +350,7 @@ module Gitlab def to_patch(options = {}) begin - raw_commit.to_mbox(options) + rugged_commit.to_mbox(options) rescue Rugged::InvalidError => ex if ex.message =~ /commit \w+ is a merge commit/i 'Patch format is not currently supported for merge commits.' @@ -388,6 +398,14 @@ module Gitlab encode! @committer_email end + def rugged_commit + @rugged_commit ||= if raw_commit.is_a?(Rugged::Commit) + raw_commit + else + @repository.rev_parse_target(id) + end + end + private def init_from_hash(hash) @@ -421,10 +439,10 @@ module Gitlab # 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) + @authored_date = Time.at(commit.author.date.seconds).utc @author_name = commit.author.name.dup @author_email = commit.author.email.dup - @committed_date = Time.at(commit.committer.date.seconds) + @committed_date = Time.at(commit.committer.date.seconds).utc @committer_name = commit.committer.name.dup @committer_email = commit.committer.email.dup @parent_ids = commit.parent_ids diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb index c90ef282fdd..70177cd0fec 100644 --- a/lib/gitlab/gitaly_client.rb +++ b/lib/gitlab/gitaly_client.rb @@ -100,5 +100,9 @@ module Gitlab path = Rails.root.join(SERVER_VERSION_FILE) path.read.chomp end + + def self.encode(s) + s.dup.force_encoding(Encoding::ASCII_8BIT) + end end end diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb index 793de65595f..fef77f43670 100644 --- a/lib/gitlab/gitaly_client/commit_service.rb +++ b/lib/gitlab/gitaly_client/commit_service.rb @@ -43,7 +43,7 @@ module Gitlab request = Gitaly::TreeEntryRequest.new( repository: @gitaly_repo, revision: ref, - path: path.dup.force_encoding(Encoding::ASCII_8BIT), + path: GitalyClient.encode(path), limit: limit.to_i ) @@ -99,8 +99,8 @@ module Gitlab def last_commit_for_path(revision, path) request = Gitaly::LastCommitForPathRequest.new( repository: @gitaly_repo, - revision: revision.force_encoding(Encoding::ASCII_8BIT), - path: path.to_s.force_encoding(Encoding::ASCII_8BIT) + revision: GitalyClient.encode(revision), + path: GitalyClient.encode(path.to_s) ) gitaly_commit = GitalyClient.call(@repository.storage, :commit_service, :last_commit_for_path, request).commit @@ -151,6 +151,17 @@ module Gitlab response.reduce("") { |memo, msg| memo << msg.data } end + def find_commit(revision) + request = Gitaly::FindCommitRequest.new( + repository: @gitaly_repo, + revision: GitalyClient.encode(revision) + ) + + response = GitalyClient.call(@repository.storage, :commit_service, :find_commit, request) + + response.commit + end + private def commit_diff_request_params(commit, options = {}) diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb index 4a7e5f87ff9..c531d4b055f 100644 --- a/spec/lib/gitlab/git/commit_spec.rb +++ b/spec/lib/gitlab/git/commit_spec.rb @@ -66,6 +66,7 @@ describe Gitlab::Git::Commit, seed_helper: true do describe "Commit info from gitaly commit" do let(:id) { 'f00' } + let(:parent_ids) { %w(b45 b46) } let(:subject) { "My commit".force_encoding('ASCII-8BIT') } let(:body) { subject + "My body".force_encoding('ASCII-8BIT') } let(:committer) do @@ -88,7 +89,8 @@ describe Gitlab::Git::Commit, seed_helper: true do subject: subject, body: body, author: author, - committer: committer + committer: committer, + parent_ids: parent_ids ) end let(:commit) { described_class.new(repository, gitaly_commit) } @@ -102,6 +104,7 @@ describe Gitlab::Git::Commit, seed_helper: true do 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) } + it { expect(commit.parent_ids).to eq(parent_ids) } context 'no body' do let(:body) { "".force_encoding('ASCII-8BIT') } diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb index f4a715e43d6..7fe698fcb18 100644 --- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb @@ -112,4 +112,18 @@ describe Gitlab::GitalyClient::CommitService do client.tree_entries(repository, revision, path) end end + + describe '#find_commit' do + let(:revision) { '4b825dc642cb6eb9a060e54bf8d69288fbee4904' } + it 'sends an RPC request' do + request = Gitaly::FindCommitRequest.new( + repository: repository_message, revision: revision + ) + + expect_any_instance_of(Gitaly::CommitService::Stub).to receive(:find_commit) + .with(request, kind_of(Hash)).and_return(double(commit: nil)) + + described_class.new(repository).find_commit(revision) + end + end end diff --git a/spec/migrations/migrate_process_commit_worker_jobs_spec.rb b/spec/migrations/migrate_process_commit_worker_jobs_spec.rb index cf2d5827306..e5793a3c0ee 100644 --- a/spec/migrations/migrate_process_commit_worker_jobs_spec.rb +++ b/spec/migrations/migrate_process_commit_worker_jobs_spec.rb @@ -6,7 +6,7 @@ require Rails.root.join('db', 'migrate', '20161124141322_migrate_process_commit_ describe MigrateProcessCommitWorkerJobs do let(:project) { create(:project, :repository) } let(:user) { create(:user) } - let(:commit) { project.commit.raw.raw_commit } + let(:commit) { project.commit.raw.rugged_commit } describe 'Project' do describe 'find_including_path' do diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index 1d38f8e5a6d..c18c635d811 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -189,7 +189,7 @@ eos it { expect(data).to be_a(Hash) } it { expect(data[:message]).to include('adds bar folder and branch-test text file to check Repository merged_to_root_ref method') } - it { expect(data[:timestamp]).to eq('2016-09-27T14:37:46+00:00') } + it { expect(data[:timestamp]).to eq('2016-09-27T14:37:46Z') } it { expect(data[:added]).to eq(["bar/branch-test.txt"]) } it { expect(data[:modified]).to eq([]) } it { expect(data[:removed]).to eq([]) } -- GitLab