From 9348efc6bae3f5eee948fc6871fa4bf5afbbfd4e Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 17 Jul 2018 12:17:43 +0200 Subject: Client implementation for Repository#new_commits After trying to remove the whole method in 8f69014af2902d8d53fe931268bec60f6858f160, this is a more gentle approach to the method. :) Prior to this change, new commit detection wasn't implemented in Gitaly, this was done through: https://gitlab.com/gitlab-org/gitaly/merge_requests/779 As the new implemented got moved around a bit, the whole RevList class got removed. Part of https://gitlab.com/gitlab-org/gitaly/issues/1233 --- app/models/repository.rb | 7 ++--- lib/gitlab/git/repository.rb | 15 ++++++++++ lib/gitlab/git/rev_list.rb | 50 --------------------------------- lib/gitlab/gitaly_client/ref_service.rb | 19 +++++++++++++ spec/lib/gitlab/git/rev_list_spec.rb | 35 ----------------------- spec/models/repository_spec.rb | 46 ++++++++++++++++++++---------- 6 files changed, 67 insertions(+), 105 deletions(-) delete mode 100644 lib/gitlab/git/rev_list.rb delete mode 100644 spec/lib/gitlab/git/rev_list_spec.rb diff --git a/app/models/repository.rb b/app/models/repository.rb index a96c73e6ab7..e248f94cbd8 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -154,12 +154,9 @@ class Repository # Returns a list of commits that are not present in any reference def new_commits(newrev) - # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/1233 - refs = Gitlab::GitalyClient::StorageSettings.allow_disk_access do - ::Gitlab::Git::RevList.new(raw, newrev: newrev).new_refs - end + commits = raw.new_commits(newrev) - refs.map { |sha| commit(sha.strip) } + ::Commit.decorate(commits, project) end # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/384 diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 19c79b6f7b7..39fbf6e2526 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -381,6 +381,21 @@ module Gitlab end end + # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/1233 + def new_commits(newrev) + gitaly_migrate(:new_commits) do |is_enabled| + if is_enabled + gitaly_ref_client.list_new_commits(newrev) + else + refs = Gitlab::GitalyClient::StorageSettings.allow_disk_access do + rev_list(including: newrev, excluding: :all).split("\n").map(&:strip) + end + + Gitlab::Git::Commit.batch_by_oid(self, refs) + end + end + end + def count_commits(options) options = process_count_commits_options(options.dup) diff --git a/lib/gitlab/git/rev_list.rb b/lib/gitlab/git/rev_list.rb deleted file mode 100644 index 2ba68343aa5..00000000000 --- a/lib/gitlab/git/rev_list.rb +++ /dev/null @@ -1,50 +0,0 @@ -module Gitlab - module Git - class RevList - include Gitlab::Git::Popen - - attr_reader :oldrev, :newrev, :repository - - def initialize(repository, newrev:, oldrev: nil) - @oldrev = oldrev - @newrev = newrev - @repository = repository - end - - # This method returns an array of new commit references - # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/1233 - # - def new_refs - Gitlab::GitalyClient::StorageSettings.allow_disk_access do - repository.rev_list(including: newrev, excluding: :all).split("\n") - end - end - - private - - def execute(args) - repository.rev_list(args).split("\n") - end - - def get_objects(including: [], excluding: [], options: [], require_path: nil) - opts = { including: including, excluding: excluding, options: options, objects: true } - - repository.rev_list(opts) do |lazy_output| - objects = objects_from_output(lazy_output, require_path: require_path) - - yield(objects) - end - end - - def objects_from_output(object_output, require_path: nil) - object_output.map do |output_line| - sha, path = output_line.split(' ', 2) - - next if require_path && path.to_s.empty? - - sha - end.reject(&:nil?) - end - end - end -end diff --git a/lib/gitlab/gitaly_client/ref_service.rb b/lib/gitlab/gitaly_client/ref_service.rb index 7f4eed9222a..1ad6376dac7 100644 --- a/lib/gitlab/gitaly_client/ref_service.rb +++ b/lib/gitlab/gitaly_client/ref_service.rb @@ -56,6 +56,25 @@ module Gitlab encode!(response.name.dup) end + def list_new_commits(newrev) + request = Gitaly::ListNewCommitsRequest.new( + repository: @gitaly_repo, + commit_id: newrev + ) + + response = GitalyClient + .call(@storage, :ref_service, :list_new_commits, request, timeout: GitalyClient.medium_timeout) + + commits = [] + response.each do |msg| + msg.commits.each do |c| + commits << Gitlab::Git::Commit.new(@repository, c) + end + end + + commits + end + def count_tag_names tag_names.count end diff --git a/spec/lib/gitlab/git/rev_list_spec.rb b/spec/lib/gitlab/git/rev_list_spec.rb deleted file mode 100644 index d9d405e1ccc..00000000000 --- a/spec/lib/gitlab/git/rev_list_spec.rb +++ /dev/null @@ -1,35 +0,0 @@ -require 'spec_helper' - -describe Gitlab::Git::RevList do - let(:repository) { create(:project, :repository).repository.raw } - let(:rev_list) { described_class.new(repository, newrev: 'newrev') } - - def args_for_popen(args_list) - [Gitlab.config.git.bin_path, 'rev-list', *args_list] - end - - def stub_popen_rev_list(*additional_args, with_lazy_block: true, output:) - repo_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access { repository.path } - - params = [ - args_for_popen(additional_args), - repo_path, - {}, - hash_including(lazy_block: with_lazy_block ? anything : nil) - ] - - expect(repository).to receive(:popen).with(*params) do |*_, lazy_block:| - output = lazy_block.call(output.lines.lazy.map(&:chomp)) if with_lazy_block - - [output, 0] - end - end - - context "#new_refs" do - it 'calls out to `popen`' do - stub_popen_rev_list('newrev', '--not', '--all', with_lazy_block: false, output: "sha1\nsha2") - - expect(rev_list.new_refs).to eq(%w[sha1 sha2]) - end - end -end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 02d31098cfd..e65214808e1 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -296,24 +296,40 @@ describe Repository do end describe '#new_commits' do - let(:new_refs) do - double(:git_rev_list, new_refs: %w[ - c1acaa58bbcbc3eafe538cb8274ba387047b69f8 - 5937ac0a7beb003549fc5fd26fc247adbce4a52e - ]) - end + shared_examples 'finding unreferenced commits' do + set(:project) { create(:project, :repository) } + let(:repository) { project.repository } - it 'delegates to Gitlab::Git::RevList' do - expect(Gitlab::Git::RevList).to receive(:new).with( - repository.raw, - newrev: 'aaaabbbbccccddddeeeeffffgggghhhhiiiijjjj').and_return(new_refs) + subject { repository.new_commits(rev) } - commits = repository.new_commits('aaaabbbbccccddddeeeeffffgggghhhhiiiijjjj') + context 'when there are no new commits' do + let(:rev) { repository.commit.id } - expect(commits).to eq([ - repository.commit('c1acaa58bbcbc3eafe538cb8274ba387047b69f8'), - repository.commit('5937ac0a7beb003549fc5fd26fc247adbce4a52e') - ]) + it 'returns an empty array' do + expect(subject).to eq([]) + end + end + + context 'when new commits are found' do + let(:branch) { 'orphaned-branch' } + let!(:rev) { repository.commit(branch).id } + + it 'returns the commits' do + repository.delete_branch(branch) + + expect(subject).not_to be_empty + expect(subject).to all( be_a(::Commit) ) + expect(subject.size).to eq(1) + end + end + end + + context 'when Gitaly handles the request' do + it_behaves_like 'finding unreferenced commits' + end + + context 'when Gitaly is disabled', :disable_gitaly do + it_behaves_like 'finding unreferenced commits' end end -- cgit v1.2.1