From ec78d29a6eee9b91299cbaef4e7497f392af0310 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Thu, 28 Jun 2018 13:41:59 +0200 Subject: [PATCH] FindAllCommits mandatory Closes https://gitlab.com/gitlab-org/gitaly/issues/326 --- lib/gitlab/git/commit.rb | 51 +------------- spec/lib/gitlab/git/commit_spec.rb | 104 ++++++++++------------------- 2 files changed, 39 insertions(+), 116 deletions(-) diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb index 01274505557..fb35f1d99be 100644 --- a/lib/gitlab/git/commit.rb +++ b/lib/gitlab/git/commit.rb @@ -149,58 +149,11 @@ module Gitlab # # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/326 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 + repo.wrapped_gitaly_errors do + Gitlab::GitalyClient::CommitService.new(repo).find_all_commits(options) end end - def find_all_by_rugged(repo, options = {}) - actual_options = options.dup - - allowed_options = [:ref, :max_count, :skip, :order] - - actual_options.keep_if do |key| - allowed_options.include?(key) - end - - default_options = { skip: 0 } - actual_options = default_options.merge(actual_options) - - rugged = repo.rugged - walker = Rugged::Walker.new(rugged) - - if actual_options[:ref] - walker.push(rugged.rev_parse_oid(actual_options[:ref])) - else - rugged.references.each("refs/heads/*") do |ref| - walker.push(ref.target_id) - end - end - - walker.sorting(rugged_sort_type(actual_options[:order])) - - commits = [] - offset = actual_options[:skip] - limit = actual_options[:max_count] - walker.each(offset: offset, limit: limit) do |commit| - commits.push(decorate(repo, commit)) - end - - walker.reset - - commits - rescue Rugged::OdbError - [] - end - - def find_all_by_gitaly(repo, options = {}) - Gitlab::GitalyClient::CommitService.new(repo).find_all_commits(options) - end - def decorate(repository, commit, ref = nil) Gitlab::Git::Commit.new(repository, commit, ref) end diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb index 0d7c930127c..ee74c2769eb 100644 --- a/spec/lib/gitlab/git/commit_spec.rb +++ b/spec/lib/gitlab/git/commit_spec.rb @@ -331,84 +331,54 @@ describe Gitlab::Git::Commit, seed_helper: true do end describe '.find_all' do - shared_examples 'finding all commits' do - it 'should return a return a collection of commits' do - commits = described_class.find_all(repository) + it 'should return a return a collection of commits' do + commits = described_class.find_all(repository) - expect(commits).to all( be_a_kind_of(described_class) ) + expect(commits).to all( be_a_kind_of(described_class) ) + end + + context 'max_count' do + subject do + commits = described_class.find_all( + repository, + max_count: 50 + ) + + commits.map(&:id) end - context 'max_count' do - subject do - commits = described_class.find_all( - repository, - max_count: 50 - ) - - commits.map(&:id) - end - - it 'has 34 elements' do - expect(subject.size).to eq(34) - end - - it 'includes the expected commits' do - expect(subject).to include( - SeedRepo::Commit::ID, - SeedRepo::Commit::PARENT_ID, - SeedRepo::FirstCommit::ID - ) - end + it 'has 34 elements' do + expect(subject.size).to eq(34) end - context 'ref + max_count + skip' do - subject do - commits = described_class.find_all( - repository, - ref: 'master', - max_count: 50, - skip: 1 - ) - - commits.map(&:id) - end - - it 'has 24 elements' do - expect(subject.size).to eq(24) - end - - it 'includes the expected commits' do - expect(subject).to include(SeedRepo::Commit::ID, SeedRepo::FirstCommit::ID) - expect(subject).not_to include(SeedRepo::LastCommit::ID) - end + it 'includes the expected commits' do + expect(subject).to include( + SeedRepo::Commit::ID, + SeedRepo::Commit::PARENT_ID, + SeedRepo::FirstCommit::ID + ) end end - context 'when Gitaly find_all_commits feature is enabled' do - it_behaves_like 'finding all commits' - end + context 'ref + max_count + skip' do + subject do + commits = described_class.find_all( + repository, + ref: 'master', + max_count: 50, + skip: 1 + ) - context 'when Gitaly find_all_commits feature is disabled', :skip_gitaly_mock do - it_behaves_like 'finding all commits' + commits.map(&:id) + 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) + it 'has 24 elements' do + expect(subject.size).to eq(24) + end - 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 + it 'includes the expected commits' do + expect(subject).to include(SeedRepo::Commit::ID, SeedRepo::FirstCommit::ID) + expect(subject).not_to include(SeedRepo::LastCommit::ID) end end end