From a0a7b551ae117f481ec2d920d47ef55043119313 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Tue, 6 Feb 2018 19:49:33 -0300 Subject: [PATCH] Incorporate Gitaly's RPCs for Gitlab::Git::LfsChanges --- lib/gitlab/git/lfs_changes.rb | 26 +++++++- lib/gitlab/gitaly_client/blob_service.rb | 55 +++++++++++++---- spec/lib/gitlab/checks/lfs_integrity_spec.rb | 22 +++---- spec/lib/gitlab/git/lfs_changes_spec.rb | 38 ++++++------ .../gitlab/gitaly_client/blob_service_spec.rb | 60 +++++++++++++++++++ spec/support/bare_repo_operations.rb | 14 +++-- 6 files changed, 170 insertions(+), 45 deletions(-) create mode 100644 spec/lib/gitlab/gitaly_client/blob_service_spec.rb diff --git a/lib/gitlab/git/lfs_changes.rb b/lib/gitlab/git/lfs_changes.rb index 48434047fce..b9e5cf258f4 100644 --- a/lib/gitlab/git/lfs_changes.rb +++ b/lib/gitlab/git/lfs_changes.rb @@ -7,6 +7,28 @@ module Gitlab end def new_pointers(object_limit: nil, not_in: nil) + @repository.gitaly_migrate(:blob_get_new_lfs_pointers) do |is_enabled| + if is_enabled + @repository.gitaly_blob_client.get_new_lfs_pointers(@newrev, object_limit, not_in) + else + git_new_pointers(object_limit, not_in) + end + end + end + + def all_pointers + @repository.gitaly_migrate(:blob_get_all_lfs_pointers) do |is_enabled| + if is_enabled + @repository.gitaly_blob_client.get_all_lfs_pointers(@newrev) + else + git_all_pointers + end + end + end + + private + + def git_new_pointers(object_limit, not_in) @new_pointers ||= begin rev_list.new_objects(not_in: not_in, require_path: true) do |object_ids| object_ids = object_ids.take(object_limit) if object_limit @@ -16,14 +38,12 @@ module Gitlab end end - def all_pointers + def git_all_pointers rev_list.all_objects(require_path: true) do |object_ids| Gitlab::Git::Blob.batch_lfs_pointers(@repository, object_ids) end end - private - def rev_list Gitlab::Git::RevList.new(@repository, newrev: @newrev) end diff --git a/lib/gitlab/gitaly_client/blob_service.rb b/lib/gitlab/gitaly_client/blob_service.rb index dfa0fa43b0f..28554208984 100644 --- a/lib/gitlab/gitaly_client/blob_service.rb +++ b/lib/gitlab/gitaly_client/blob_service.rb @@ -45,16 +45,7 @@ module Gitlab response = GitalyClient.call(@gitaly_repo.storage_name, :blob_service, :get_lfs_pointers, request) - response.flat_map do |message| - message.lfs_pointers.map do |lfs_pointer| - Gitlab::Git::Blob.new( - id: lfs_pointer.oid, - size: lfs_pointer.size, - data: lfs_pointer.data, - binary: Gitlab::Git::Blob.binary?(lfs_pointer.data) - ) - end - end + map_lfs_pointers(response) end def get_blobs(revision_paths, limit = -1) @@ -80,6 +71,50 @@ module Gitlab GitalyClient::BlobsStitcher.new(response) end + + def get_new_lfs_pointers(revision, limit, not_in) + request = Gitaly::GetNewLFSPointersRequest.new( + repository: @gitaly_repo, + revision: encode_binary(revision), + limit: limit || 0 + ) + + if not_in.nil? || not_in == :all + request.not_in_all = true + else + request.not_in_refs += not_in + end + + response = GitalyClient.call(@gitaly_repo.storage_name, :blob_service, :get_new_lfs_pointers, request) + + map_lfs_pointers(response) + end + + def get_all_lfs_pointers(revision) + request = Gitaly::GetNewLFSPointersRequest.new( + repository: @gitaly_repo, + revision: encode_binary(revision) + ) + + response = GitalyClient.call(@gitaly_repo.storage_name, :blob_service, :get_all_lfs_pointers, request) + + map_lfs_pointers(response) + end + + private + + def map_lfs_pointers(response) + response.flat_map do |message| + message.lfs_pointers.map do |lfs_pointer| + Gitlab::Git::Blob.new( + id: lfs_pointer.oid, + size: lfs_pointer.size, + data: lfs_pointer.data, + binary: Gitlab::Git::Blob.binary?(lfs_pointer.data) + ) + end + end + end end end end diff --git a/spec/lib/gitlab/checks/lfs_integrity_spec.rb b/spec/lib/gitlab/checks/lfs_integrity_spec.rb index 17756621221..7201e4f7bf6 100644 --- a/spec/lib/gitlab/checks/lfs_integrity_spec.rb +++ b/spec/lib/gitlab/checks/lfs_integrity_spec.rb @@ -2,23 +2,25 @@ require 'spec_helper' describe Gitlab::Checks::LfsIntegrity do include ProjectForksHelper + let(:project) { create(:project, :repository) } - let(:newrev) { '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51' } + let(:repository) { project.repository } + let(:newrev) do + operations = BareRepoOperations.new(repository.path) + + # Create a commit not pointed at by any ref to emulate being in the + # pre-receive hook so that `--not --all` returns some objects + operations.commit_tree('8856a329dd38ca86dfb9ce5aa58a16d88cc119bd', "New LFS objects") + end subject { described_class.new(project, newrev) } describe '#objects_missing?' do - let(:blob_object) { project.repository.blob_at_branch('lfs', 'files/lfs/lfs_object.iso') } - - before do - allow_any_instance_of(Gitlab::Git::RevList).to receive(:new_objects) do |&lazy_block| - lazy_block.call([blob_object.id]) - end - end + let(:blob_object) { repository.blob_at_branch('lfs', 'files/lfs/lfs_object.iso') } context 'with LFS not enabled' do it 'skips integrity check' do - expect_any_instance_of(Gitlab::Git::RevList).not_to receive(:new_objects) + expect_any_instance_of(Gitlab::Git::LfsChanges).not_to receive(:new_pointers) subject.objects_missing? end @@ -33,7 +35,7 @@ describe Gitlab::Checks::LfsIntegrity do let(:newrev) { nil } it 'skips integrity check' do - expect_any_instance_of(Gitlab::Git::RevList).not_to receive(:new_objects) + expect_any_instance_of(Gitlab::Git::LfsChanges).not_to receive(:new_pointers) expect(subject.objects_missing?).to be_falsey end diff --git a/spec/lib/gitlab/git/lfs_changes_spec.rb b/spec/lib/gitlab/git/lfs_changes_spec.rb index c9007d7d456..d0dd8c6303f 100644 --- a/spec/lib/gitlab/git/lfs_changes_spec.rb +++ b/spec/lib/gitlab/git/lfs_changes_spec.rb @@ -7,34 +7,36 @@ describe Gitlab::Git::LfsChanges do subject { described_class.new(project.repository, newrev) } - describe 'new_pointers' do - before do - allow_any_instance_of(Gitlab::Git::RevList).to receive(:new_objects).and_yield([blob_object_id]) + describe '#new_pointers' do + shared_examples 'new pointers' do + it 'filters new objects to find lfs pointers' do + expect(subject.new_pointers(not_in: []).first.id).to eq(blob_object_id) + end + + it 'limits new_objects using object_limit' do + expect(subject.new_pointers(object_limit: 1)).to eq([]) + end end - it 'uses rev-list to find new objects' do - rev_list = double - allow(Gitlab::Git::RevList).to receive(:new).and_return(rev_list) - - expect(rev_list).to receive(:new_objects).and_return([]) - - subject.new_pointers + context 'with gitaly enabled' do + it_behaves_like 'new pointers' end - it 'filters new objects to find lfs pointers' do - expect(Gitlab::Git::Blob).to receive(:batch_lfs_pointers).with(project.repository, [blob_object_id]) + context 'with gitaly disabled', :skip_gitaly_mock do + it_behaves_like 'new pointers' - subject.new_pointers(object_limit: 1) - end + it 'uses rev-list to find new objects' do + rev_list = double + allow(Gitlab::Git::RevList).to receive(:new).and_return(rev_list) - it 'limits new_objects using object_limit' do - expect(Gitlab::Git::Blob).to receive(:batch_lfs_pointers).with(project.repository, []) + expect(rev_list).to receive(:new_objects).and_return([]) - subject.new_pointers(object_limit: 0) + subject.new_pointers + end end end - describe 'all_pointers' do + describe '#all_pointers', :skip_gitaly_mock do it 'uses rev-list to find all objects' do rev_list = double allow(Gitlab::Git::RevList).to receive(:new).and_return(rev_list) diff --git a/spec/lib/gitlab/gitaly_client/blob_service_spec.rb b/spec/lib/gitlab/gitaly_client/blob_service_spec.rb new file mode 100644 index 00000000000..a2770ef2fe4 --- /dev/null +++ b/spec/lib/gitlab/gitaly_client/blob_service_spec.rb @@ -0,0 +1,60 @@ +require 'spec_helper' + +describe Gitlab::GitalyClient::BlobService do + let(:project) { create(:project, :repository) } + let(:storage_name) { project.repository_storage } + let(:relative_path) { project.disk_path + '.git' } + let(:repository) { project.repository } + let(:client) { described_class.new(repository) } + + describe '#get_new_lfs_pointers' do + let(:revision) { 'master' } + let(:limit) { 5 } + let(:not_in) { ['branch-a', 'branch-b'] } + let(:expected_params) do + { revision: revision, limit: limit, not_in_refs: not_in, not_in_all: false } + end + + subject { client.get_new_lfs_pointers(revision, limit, not_in) } + + it 'sends a get_new_lfs_pointers message' do + expect_any_instance_of(Gitaly::BlobService::Stub) + .to receive(:get_new_lfs_pointers) + .with(gitaly_request_with_params(expected_params), kind_of(Hash)) + .and_return([]) + + subject + end + + context 'with not_in = :all' do + let(:not_in) { :all } + let(:expected_params) do + { revision: revision, limit: limit, not_in_refs: [], not_in_all: true } + end + + it 'sends the correct message' do + expect_any_instance_of(Gitaly::BlobService::Stub) + .to receive(:get_new_lfs_pointers) + .with(gitaly_request_with_params(expected_params), kind_of(Hash)) + .and_return([]) + + subject + end + end + end + + describe '#get_all_lfs_pointers' do + let(:revision) { 'master' } + + subject { client.get_all_lfs_pointers(revision) } + + it 'sends a get_all_lfs_pointers message' do + expect_any_instance_of(Gitaly::BlobService::Stub) + .to receive(:get_all_lfs_pointers) + .with(gitaly_request_with_params(revision: revision), kind_of(Hash)) + .and_return([]) + + subject + end + end +end diff --git a/spec/support/bare_repo_operations.rb b/spec/support/bare_repo_operations.rb index 38d11992dc2..8eeaa37d3c5 100644 --- a/spec/support/bare_repo_operations.rb +++ b/spec/support/bare_repo_operations.rb @@ -11,6 +11,14 @@ class BareRepoOperations @path_to_repo = path_to_repo end + def commit_tree(tree_id, msg, parent: EMPTY_TREE_ID) + commit_tree_args = ['commit-tree', tree_id, '-m', msg] + commit_tree_args += ['-p', parent] unless parent == EMPTY_TREE_ID + commit_id = execute(commit_tree_args) + + commit_id[0] + end + # Based on https://stackoverflow.com/a/25556917/1856239 def commit_file(file, dst_path, branch = 'master') head_id = execute(['show', '--format=format:%H', '--no-patch', branch], allow_failure: true)[0] || EMPTY_TREE_ID @@ -26,11 +34,9 @@ class BareRepoOperations tree_id = execute(['write-tree']) - commit_tree_args = ['commit-tree', tree_id[0], '-m', "Add #{dst_path}"] - commit_tree_args += ['-p', head_id] unless head_id == EMPTY_TREE_ID - commit_id = execute(commit_tree_args) + commit_id = commit_tree(tree_id[0], "Add #{dst_path}", parent: head_id) - execute(['update-ref', "refs/heads/#{branch}", commit_id[0]]) + execute(['update-ref', "refs/heads/#{branch}", commit_id]) end private