From 68b1e5a97ce7760d845edc84f4ac90f3c6008cfd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Tue, 17 Jul 2018 22:08:23 -0400 Subject: Incorporate Gitaly's RefService.FindAllRemoteBranches RPC --- GITALY_SERVER_VERSION | 2 +- Gemfile | 2 +- Gemfile.lock | 4 +- Gemfile.rails5.lock | 4 +- lib/gitlab/git/repository_mirroring.rb | 51 +++++------------------ lib/gitlab/gitaly_client/ref_service.rb | 19 +++++++++ spec/lib/gitlab/git/repository_spec.rb | 34 --------------- spec/lib/gitlab/gitaly_client/ref_service_spec.rb | 38 +++++++++++++++++ spec/models/repository_spec.rb | 14 ------- 9 files changed, 73 insertions(+), 95 deletions(-) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index e23e3fd2982..5fea1768541 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -0.112.0 +0.113.0 diff --git a/Gemfile b/Gemfile index d575568adaa..7eac9e6a7b3 100644 --- a/Gemfile +++ b/Gemfile @@ -418,7 +418,7 @@ group :ed25519 do end # Gitaly GRPC client -gem 'gitaly-proto', '~> 0.105.0', require: 'gitaly' +gem 'gitaly-proto', '~> 0.106.0', require: 'gitaly' gem 'grpc', '~> 1.11.0' # Locked until https://github.com/google/protobuf/issues/4210 is closed diff --git a/Gemfile.lock b/Gemfile.lock index 7f9207d9dfe..6415f9e6132 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -282,7 +282,7 @@ GEM gettext_i18n_rails (>= 0.7.1) po_to_json (>= 1.0.0) rails (>= 3.2.0) - gitaly-proto (0.105.0) + gitaly-proto (0.106.0) google-protobuf (~> 3.1) grpc (~> 1.10) github-linguist (5.3.3) @@ -1037,7 +1037,7 @@ DEPENDENCIES gettext (~> 3.2.2) gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails_js (~> 1.3) - gitaly-proto (~> 0.105.0) + gitaly-proto (~> 0.106.0) github-linguist (~> 5.3.3) gitlab-flowdock-git-hook (~> 1.0.1) gitlab-gollum-lib (~> 4.2) diff --git a/Gemfile.rails5.lock b/Gemfile.rails5.lock index 766f2479ea5..50ca0d5a729 100644 --- a/Gemfile.rails5.lock +++ b/Gemfile.rails5.lock @@ -285,7 +285,7 @@ GEM gettext_i18n_rails (>= 0.7.1) po_to_json (>= 1.0.0) rails (>= 3.2.0) - gitaly-proto (0.105.0) + gitaly-proto (0.106.0) google-protobuf (~> 3.1) grpc (~> 1.10) github-linguist (5.3.3) @@ -1047,7 +1047,7 @@ DEPENDENCIES gettext (~> 3.2.2) gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails_js (~> 1.3) - gitaly-proto (~> 0.105.0) + gitaly-proto (~> 0.106.0) github-linguist (~> 5.3.3) gitlab-flowdock-git-hook (~> 1.0.1) gitlab-gollum-lib (~> 4.2) diff --git a/lib/gitlab/git/repository_mirroring.rb b/lib/gitlab/git/repository_mirroring.rb index 9faa62be28e..ef86d4a55ca 100644 --- a/lib/gitlab/git/repository_mirroring.rb +++ b/lib/gitlab/git/repository_mirroring.rb @@ -17,33 +17,19 @@ module Gitlab rugged.config["remote.#{remote_name}.prune"] = true end - def remote_tags(remote) - # Each line has this format: "dc872e9fa6963f8f03da6c8f6f264d0845d6b092\trefs/tags/v1.10.0\n" - # We want to convert it to: [{ 'v1.10.0' => 'dc872e9fa6963f8f03da6c8f6f264d0845d6b092' }, ...] - list_remote_tags(remote).map do |line| - target, path = line.strip.split("\t") - - # When the remote repo does not have tags. - if target.nil? || path.nil? - Rails.logger.info "Empty or invalid list of tags for remote: #{remote}. Output: #{output}" - break [] + def remote_branches(remote_name) + gitaly_migrate(:ref_find_all_remote_branches) do |is_enabled| + if is_enabled + gitaly_ref_client.remote_branches(remote_name) + else + rugged_remote_branches(remote_name) end - - name = path.split('/', 3).last - # We're only interested in tag references - # See: http://stackoverflow.com/questions/15472107/when-listing-git-ls-remote-why-theres-after-the-tag-name - next if name =~ /\^\{\}\Z/ - - target_commit = Gitlab::Git::Commit.find(self, target) - Gitlab::Git::Tag.new(self, { - name: name, - target: target, - target_commit: target_commit - }) - end.compact + end end - def remote_branches(remote_name) + private + + def rugged_remote_branches(remote_name) branches = [] rugged.references.each("refs/remotes/#{remote_name}/*").map do |ref| @@ -60,8 +46,6 @@ module Gitlab branches end - private - def set_remote_refmap(remote_name, refmap) Array(refmap).each_with_index do |refspec, i| refspec = REFMAPS[refspec] || refspec @@ -75,21 +59,6 @@ module Gitlab end end end - - def list_remote_tags(remote) - tag_list, exit_code, error = nil - cmd = %W(#{Gitlab.config.git.bin_path} --git-dir=#{path} ls-remote --tags #{remote}) - - Open3.popen3(*cmd) do |stdin, stdout, stderr, wait_thr| - tag_list = stdout.read - error = stderr.read - exit_code = wait_thr.value.exitstatus - end - - raise RemoteError, error unless exit_code.zero? - - tag_list.split("\n") - end end end end diff --git a/lib/gitlab/gitaly_client/ref_service.rb b/lib/gitlab/gitaly_client/ref_service.rb index 1ad6376dac7..fbe7d4ba1ae 100644 --- a/lib/gitlab/gitaly_client/ref_service.rb +++ b/lib/gitlab/gitaly_client/ref_service.rb @@ -17,6 +17,13 @@ module Gitlab consume_find_all_branches_response(response) end + def remote_branches(remote_name) + request = Gitaly::FindAllRemoteBranchesRequest.new(repository: @gitaly_repo, remote_name: remote_name) + response = GitalyClient.call(@repository.storage, :ref_service, :find_all_remote_branches, request) + + consume_find_all_remote_branches_response(remote_name, response) + end + def merged_branches(branch_names = []) request = Gitaly::FindAllBranchesRequest.new( repository: @gitaly_repo, @@ -243,6 +250,18 @@ module Gitlab end end + def consume_find_all_remote_branches_response(remote_name, response) + remote_name += '/' unless remote_name.ends_with?('/') + + response.flat_map do |message| + message.branches.map do |branch| + target_commit = Gitlab::Git::Commit.decorate(@repository, branch.target_commit) + branch_name = branch.name.sub(remote_name, '') + Gitlab::Git::Branch.new(@repository, branch_name, branch.target_commit.id, target_commit) + end + end + end + def consume_tags_response(response) response.flat_map do |message| message.tags.map { |gitaly_tag| Gitlab::Git::Tag.new(@repository, gitaly_tag) } diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 0365c3b20ef..ee385226e65 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -602,40 +602,6 @@ describe Gitlab::Git::Repository, seed_helper: true do end end - describe '#remote_tags' do - let(:remote_name) { 'upstream' } - let(:target_commit_id) { SeedRepo::Commit::ID } - let(:tag_name) { 'v0.0.1' } - let(:tag_message) { 'My tag' } - let(:remote_repository) do - Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') - end - - around do |example| - Gitlab::GitalyClient::StorageSettings.allow_disk_access do - example.run - end - end - - subject { repository.remote_tags(remote_name) } - - before do - remote_repository_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access { remote_repository.path } - repository.add_remote(remote_name, remote_repository_path) - remote_repository.add_tag(tag_name, user: user, target: target_commit_id) - end - - after do - ensure_seeds - end - - it 'gets the remote tags' do - expect(subject.first).to be_an_instance_of(Gitlab::Git::Tag) - expect(subject.first.name).to eq(tag_name) - expect(subject.first.dereferenced_target.id).to eq(target_commit_id) - end - end - describe "#log" do shared_examples 'repository log' do let(:commit_with_old_name) do diff --git a/spec/lib/gitlab/gitaly_client/ref_service_spec.rb b/spec/lib/gitlab/gitaly_client/ref_service_spec.rb index 257e4c50f2d..400d426c949 100644 --- a/spec/lib/gitlab/gitaly_client/ref_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/ref_service_spec.rb @@ -18,6 +18,44 @@ describe Gitlab::GitalyClient::RefService do end end + describe '#remote_branches' do + let(:remote_name) { 'my_remote' } + subject { client.remote_branches(remote_name) } + + it 'sends a find_all_remote_branches message' do + expect_any_instance_of(Gitaly::RefService::Stub) + .to receive(:find_all_remote_branches) + .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash)) + .and_return([]) + + subject + end + + it 'concantes and returns the response branches as Gitlab::Git::Branch objects' do + target_commits = create_list(:gitaly_commit, 4) + response_branches = target_commits.each_with_index.map do |gitaly_commit, i| + Gitaly::Branch.new(name: "#{remote_name}/#{i}", target_commit: gitaly_commit) + end + response = [ + Gitaly::FindAllRemoteBranchesResponse.new(branches: response_branches[0, 2]), + Gitaly::FindAllRemoteBranchesResponse.new(branches: response_branches[2, 2]) + ] + + expect_any_instance_of(Gitaly::RefService::Stub) + .to receive(:find_all_remote_branches).and_return(response) + + expect(subject.length).to be(response_branches.length) + + response_branches.each_with_index do |gitaly_branch, i| + branch = subject[i] + commit = Gitlab::Git::Commit.new(repository, gitaly_branch.target_commit) + + expect(branch.name).to eq(i.to_s) # It removes the `remote/` prefix + expect(branch.dereferenced_target).to eq(commit) + end + end + end + describe '#branch_names' do it 'sends a find_all_branch_names message' do expect_any_instance_of(Gitaly::RefService::Stub) diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index e65214808e1..5d64602ca56 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -2225,20 +2225,6 @@ describe Repository do end end - describe '#remote_branches' do - it 'returns the remote branches' do - masterrev = repository.find_branch('master').dereferenced_target - create_remote_branch('joe', 'remote_branch', masterrev) - repository.add_branch(user, 'local_branch', masterrev.id) - - # TODO: move this test to gitaly https://gitlab.com/gitlab-org/gitaly/issues/1243 - Gitlab::GitalyClient::StorageSettings.allow_disk_access do - expect(repository.remote_branches('joe').any? { |branch| branch.name == 'local_branch' }).to eq(false) - expect(repository.remote_branches('joe').any? { |branch| branch.name == 'remote_branch' }).to eq(true) - end - end - end - describe '#commit_count' do context 'with a non-existing repository' do it 'returns 0' do -- cgit v1.2.3