Commit b043100b authored by Ahmad Sherif's avatar Ahmad Sherif

Migrate Gitlab::Git::Commit.find_all to Gitaly

Closes gitaly#396
parent e59c1f79
...@@ -386,7 +386,7 @@ gem 'vmstat', '~> 2.3.0' ...@@ -386,7 +386,7 @@ gem 'vmstat', '~> 2.3.0'
gem 'sys-filesystem', '~> 1.1.6' gem 'sys-filesystem', '~> 1.1.6'
# Gitaly GRPC client # Gitaly GRPC client
gem 'gitaly', '~> 0.17.0' gem 'gitaly', '~> 0.18.0'
gem 'toml-rb', '~> 0.3.15', require: false gem 'toml-rb', '~> 0.3.15', require: false
......
...@@ -269,7 +269,7 @@ GEM ...@@ -269,7 +269,7 @@ GEM
po_to_json (>= 1.0.0) po_to_json (>= 1.0.0)
rails (>= 3.2.0) rails (>= 3.2.0)
gherkin-ruby (0.3.2) gherkin-ruby (0.3.2)
gitaly (0.17.0) gitaly (0.18.0)
google-protobuf (~> 3.1) google-protobuf (~> 3.1)
grpc (~> 1.0) grpc (~> 1.0)
github-linguist (4.7.6) github-linguist (4.7.6)
...@@ -972,7 +972,7 @@ DEPENDENCIES ...@@ -972,7 +972,7 @@ DEPENDENCIES
gettext (~> 3.2.2) gettext (~> 3.2.2)
gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails (~> 1.8.0)
gettext_i18n_rails_js (~> 1.2.0) gettext_i18n_rails_js (~> 1.2.0)
gitaly (~> 0.17.0) gitaly (~> 0.18.0)
github-linguist (~> 4.7.0) github-linguist (~> 4.7.0)
gitlab-flowdock-git-hook (~> 1.0.1) gitlab-flowdock-git-hook (~> 1.0.1)
gitlab-markup (~> 1.5.1) gitlab-markup (~> 1.5.1)
......
...@@ -98,17 +98,13 @@ module Gitlab ...@@ -98,17 +98,13 @@ module Gitlab
# Commit.between(repo, '29eda46b', 'master') # Commit.between(repo, '29eda46b', 'master')
# #
def between(repo, base, head) def between(repo, base, head)
commits = Gitlab::GitalyClient.migrate(:commits_between) do |is_enabled| Gitlab::GitalyClient.migrate(:commits_between) do |is_enabled|
if is_enabled if is_enabled
repo.gitaly_commit_client.between(base, head) repo.gitaly_commit_client.between(base, head)
else else
repo.commits_between(base, head) repo.commits_between(base, head).map { |c| decorate(c) }
end end
end end
commits.map do |commit|
decorate(commit)
end
rescue Rugged::ReferenceError rescue Rugged::ReferenceError
[] []
end end
...@@ -135,6 +131,16 @@ module Gitlab ...@@ -135,6 +131,16 @@ module Gitlab
# #
# Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/326 # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/326
def find_all(repo, options = {}) def find_all(repo, options = {})
Gitlab::GitalyClient.migrate(:find_all_commits) do |is_enabled|
if is_enabled
find_all_by_gitaly(repo, options)
else
find_all_by_rugged(repo, options)
end
end
end
def find_all_by_rugged(repo, options = {})
actual_options = options.dup actual_options = options.dup
allowed_options = [:ref, :max_count, :skip, :order] allowed_options = [:ref, :max_count, :skip, :order]
...@@ -173,6 +179,10 @@ module Gitlab ...@@ -173,6 +179,10 @@ module Gitlab
[] []
end end
def find_all_by_gitaly(repo, options = {})
Gitlab::GitalyClient::CommitService.new(repo).find_all_commits(options)
end
def decorate(commit, ref = nil) def decorate(commit, ref = nil)
Gitlab::Git::Commit.new(commit, ref) Gitlab::Git::Commit.new(commit, ref)
end end
...@@ -214,11 +224,12 @@ module Gitlab ...@@ -214,11 +224,12 @@ module Gitlab
def initialize(raw_commit, head = nil) def initialize(raw_commit, head = nil)
raise "Nil as raw commit passed" unless raw_commit raise "Nil as raw commit passed" unless raw_commit
if raw_commit.is_a?(Hash) case raw_commit
when Hash
init_from_hash(raw_commit) init_from_hash(raw_commit)
elsif raw_commit.is_a?(Rugged::Commit) when Rugged::Commit
init_from_rugged(raw_commit) init_from_rugged(raw_commit)
elsif raw_commit.is_a?(Gitaly::GitCommit) when Gitlab::GitalyClient::Commit
init_from_gitaly(raw_commit) init_from_gitaly(raw_commit)
else else
raise "Invalid raw commit type: #{raw_commit.class}" raise "Invalid raw commit type: #{raw_commit.class}"
...@@ -298,7 +309,14 @@ module Gitlab ...@@ -298,7 +309,14 @@ module Gitlab
end end
def parents def parents
case raw_commit
when Rugged::Commit
raw_commit.parents.map { |c| Gitlab::Git::Commit.new(c) } raw_commit.parents.map { |c| Gitlab::Git::Commit.new(c) }
when Gitlab::GitalyClient::Commit
parent_ids.map { |oid| self.class.find(raw_commit.repository, oid) }.compact
else
raise NotImplementedError, "commit source doesn't support #parents"
end
end end
def stats def stats
......
module Gitlab
module GitalyClient
class Commit
attr_reader :repository, :gitaly_commit
delegate :id, :subject, :body, :author, :committer, :parent_ids, to: :gitaly_commit
def initialize(repository, gitaly_commit)
@repository = repository
@gitaly_commit = gitaly_commit
end
end
end
end
...@@ -80,6 +80,19 @@ module Gitlab ...@@ -80,6 +80,19 @@ module Gitlab
consume_commits_response(response) consume_commits_response(response)
end end
def find_all_commits(opts = {})
request = Gitaly::FindAllCommitsRequest.new(
repository: @gitaly_repo,
revision: opts[:ref].to_s,
max_count: opts[:max_count].to_i,
skip: opts[:skip].to_i
)
request.order = opts[:order].upcase if opts[:order].present?
response = GitalyClient.call(@repository.storage, :commit_service, :find_all_commits, request)
consume_commits_response(response)
end
private private
def commit_diff_request_params(commit, options = {}) def commit_diff_request_params(commit, options = {})
...@@ -94,7 +107,12 @@ module Gitlab ...@@ -94,7 +107,12 @@ module Gitlab
end end
def consume_commits_response(response) def consume_commits_response(response)
response.flat_map { |r| r.commits } response.flat_map do |message|
message.commits.map do |gitaly_commit|
commit = GitalyClient::Commit.new(@repository, gitaly_commit)
Gitlab::Git::Commit.new(commit)
end
end
end end
end end
end end
......
...@@ -91,7 +91,7 @@ describe Gitlab::Git::Commit, seed_helper: true do ...@@ -91,7 +91,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
committer: committer committer: committer
) )
end end
let(:commit) { described_class.new(gitaly_commit) } let(:commit) { described_class.new(Gitlab::GitalyClient::Commit.new(repository, gitaly_commit)) }
it { expect(commit.short_id).to eq(id[0..10]) } it { expect(commit.short_id).to eq(id[0..10]) }
it { expect(commit.id).to eq(id) } it { expect(commit.id).to eq(id) }
...@@ -290,33 +290,13 @@ describe Gitlab::Git::Commit, seed_helper: true do ...@@ -290,33 +290,13 @@ describe Gitlab::Git::Commit, seed_helper: true do
end end
describe '.find_all' do describe '.find_all' do
shared_examples 'finding all commits' do
it 'should return a return a collection of commits' do it 'should return a return a collection of commits' do
commits = described_class.find_all(repository) commits = described_class.find_all(repository)
expect(commits).not_to be_empty
expect(commits).to all( be_a_kind_of(Gitlab::Git::Commit) ) expect(commits).to all( be_a_kind_of(Gitlab::Git::Commit) )
end end
context 'while applying a sort order based on the `order` option' do
it "allows ordering topologically (no parents shown before their children)" do
expect_any_instance_of(Rugged::Walker).to receive(:sorting).with(Rugged::SORT_TOPO)
described_class.find_all(repository, order: :topo)
end
it "allows ordering by date" do
expect_any_instance_of(Rugged::Walker).to receive(:sorting).with(Rugged::SORT_DATE | Rugged::SORT_TOPO)
described_class.find_all(repository, order: :date)
end
it "applies no sorting by default" do
expect_any_instance_of(Rugged::Walker).to receive(:sorting).with(Rugged::SORT_NONE)
described_class.find_all(repository)
end
end
context 'max_count' do context 'max_count' do
subject do subject do
commits = Gitlab::Git::Commit.find_all( commits = Gitlab::Git::Commit.find_all(
...@@ -324,15 +304,20 @@ describe Gitlab::Git::Commit, seed_helper: true do ...@@ -324,15 +304,20 @@ describe Gitlab::Git::Commit, seed_helper: true do
max_count: 50 max_count: 50
) )
commits.map { |c| c.id } commits.map(&:id)
end end
it 'has 31 elements' do it 'has 33 elements' do
expect(subject.size).to eq(33) expect(subject.size).to eq(33)
end end
it { is_expected.to include(SeedRepo::Commit::ID) }
it { is_expected.to include(SeedRepo::Commit::PARENT_ID) } it 'includes the expected commits' do
it { is_expected.to include(SeedRepo::FirstCommit::ID) } expect(subject).to include(
SeedRepo::Commit::ID,
SeedRepo::Commit::PARENT_ID,
SeedRepo::FirstCommit::ID
)
end
end end
context 'ref + max_count + skip' do context 'ref + max_count + skip' do
...@@ -344,15 +329,46 @@ describe Gitlab::Git::Commit, seed_helper: true do ...@@ -344,15 +329,46 @@ describe Gitlab::Git::Commit, seed_helper: true do
skip: 1 skip: 1
) )
commits.map { |c| c.id } commits.map(&:id)
end end
it 'has 23 elements' do it 'has 24 elements' do
expect(subject.size).to eq(24) expect(subject.size).to eq(24)
end end
it { is_expected.to include(SeedRepo::Commit::ID) }
it { is_expected.to include(SeedRepo::FirstCommit::ID) } it 'includes the expected commits' do
it { is_expected.not_to include(SeedRepo::LastCommit::ID) } expect(subject).to include(SeedRepo::Commit::ID, SeedRepo::FirstCommit::ID)
expect(subject).not_to include(SeedRepo::LastCommit::ID)
end
end
end
context 'when Gitaly find_all_commits feature is enabled' do
it_behaves_like 'finding all commits'
end
context 'when Gitaly find_all_commits feature is disabled', skip_gitaly_mock: true do
it_behaves_like 'finding all commits'
context 'while applying a sort order based on the `order` option' do
it "allows ordering topologically (no parents shown before their children)" do
expect_any_instance_of(Rugged::Walker).to receive(:sorting).with(Rugged::SORT_TOPO)
described_class.find_all(repository, order: :topo)
end
it "allows ordering by date" do
expect_any_instance_of(Rugged::Walker).to receive(:sorting).with(Rugged::SORT_DATE | Rugged::SORT_TOPO)
described_class.find_all(repository, order: :date)
end
it "applies no sorting by default" do
expect_any_instance_of(Rugged::Walker).to receive(:sorting).with(Rugged::SORT_NONE)
described_class.find_all(repository)
end
end
end end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment