From 237a32cc90d7e2c4b96e3a9ba0fd9e77ff3fc166 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Wed, 14 Mar 2018 23:45:57 +0000 Subject: [PATCH] Avoid failed integrity check by linking LfsObjectProjects sooner Attempted commits were failing due to the pre-recieve LfsIntegrity check. This is skipped during tests making this failure silent. --- app/services/files/create_service.rb | 9 +++-- app/services/files/multi_service.rb | 8 ++-- app/services/lfs/file_transformer.rb | 37 +++++------------ spec/services/lfs/file_transformer_spec.rb | 47 ++++++---------------- 4 files changed, 32 insertions(+), 69 deletions(-) diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb index b8ae03842a3..a954564946b 100644 --- a/app/services/files/create_service.rb +++ b/app/services/files/create_service.rb @@ -1,10 +1,11 @@ module Files class CreateService < Files::BaseService def create_commit! - Lfs::FileTransformer.link_lfs_objects(project, @branch_name) do |transformer| - content_or_lfs_pointer = transformer.new_file(@file_path, @file_content) - create_transformed_commit(content_or_lfs_pointer) - end + transformer = Lfs::FileTransformer.new(project, @branch_name) + + result = transformer.new_file(@file_path, @file_content) + + create_transformed_commit(result.content) end private diff --git a/app/services/files/multi_service.rb b/app/services/files/multi_service.rb index bd88c46e4af..be9c7c526a6 100644 --- a/app/services/files/multi_service.rb +++ b/app/services/files/multi_service.rb @@ -3,11 +3,11 @@ module Files UPDATE_FILE_ACTIONS = %w(update move delete).freeze def create_commit! - Lfs::FileTransformer.link_lfs_objects(project, @branch_name) do |transformer| - actions = actions_after_lfs_transformation(transformer, params[:actions]) + transformer = Lfs::FileTransformer.new(project, @branch_name) - commit_actions!(actions) - end + actions = actions_after_lfs_transformation(transformer, params[:actions]) + + commit_actions!(actions) end private diff --git a/app/services/lfs/file_transformer.rb b/app/services/lfs/file_transformer.rb index 7c93659b60d..0235d07e529 100644 --- a/app/services/lfs/file_transformer.rb +++ b/app/services/lfs/file_transformer.rb @@ -1,16 +1,15 @@ module Lfs - # Usage: Open a `.link_lfs_objects` block, call `new_file` on the yielded object - # as many times as needed. LfsObjectProject links will be saved if the - # return value of the block is truthy. + # Usage: Calling `new_file` check to see if a file should be in LFS and + # return a transformed result with `content` and `encoding` to commit. # - # Calling `new_file` will check the path to see if it should be in LFS, - # save and LFS pointer of needed and return its content to be saved in - # a commit. If the file isn't LFS the untransformed content is returned. + # For LFS an LfsObject linked to the project is stored and an LFS + # pointer returned. If the file isn't in LFS the untransformed content + # is returned to save in the commit. + # + # transformer = Lfs::FileTransformer.new(project, @branch_name) + # content_or_lfs_pointer = transformer.new_file(file_path, content).content + # create_transformed_commit(content_or_lfs_pointer) # - # Lfs::FileTransformer.link_lfs_objects(project, @branch_name) do |transformer| - # content_or_lfs_pointer = transformer.new_file(file_path, file_content) - # create_transformed_commit(content_or_lfs_pointer) - # end class FileTransformer attr_reader :project, :branch_name @@ -21,20 +20,12 @@ module Lfs @branch_name = branch_name end - def self.link_lfs_objects(project, branch_name) - transformer = new(project, branch_name) - result = yield(transformer) - transformer.after_transform! if result - - result - end - def new_file(file_path, file_content) if project.lfs_enabled? && lfs_file?(file_path) lfs_pointer_file = Gitlab::Git::LfsPointerFile.new(file_content) lfs_object = create_lfs_object!(lfs_pointer_file, file_content) - on_success_actions << -> { link_lfs_object!(lfs_object) } + link_lfs_object!(lfs_object) lfs_pointer_file.pointer else @@ -42,20 +33,12 @@ module Lfs end end - def after_transform! - on_success_actions.map(&:call) - end - private def lfs_file?(file_path) repository.attributes_at(branch_name, file_path)['filter'] == 'lfs' end - def on_success_actions - @on_success_actions ||= [] - end - def create_lfs_object!(lfs_pointer_file, file_content) LfsObject.find_or_create_by(oid: lfs_pointer_file.sha256, size: lfs_pointer_file.size) do |lfs_object| lfs_object.file = CarrierWaveStringFile.new(file_content) diff --git a/spec/services/lfs/file_transformer_spec.rb b/spec/services/lfs/file_transformer_spec.rb index f469f5e76dd..6d17ab03ac3 100644 --- a/spec/services/lfs/file_transformer_spec.rb +++ b/spec/services/lfs/file_transformer_spec.rb @@ -55,45 +55,24 @@ describe Lfs::FileTransformer do end end - it 'sets up after_transform! to link LfsObjects to project' do - subject.new_file(file_path, file_content) - - expect { subject.after_transform! }.to change { project.lfs_objects.count }.by(1) - end - end - end - - describe '.link_lfs_objects' do - context 'with lfs enabled' do - before do - allow(project).to receive(:lfs_enabled?).and_return(true) + it 'links LfsObjects to project' do + expect do + subject.new_file(file_path, file_content) + end.to change { project.lfs_objects.count }.by(1) end - context 'when given a block' do - it 'links LfsObject to the project automatically' do + context 'when LfsObject already exists' do + let(:lfs_pointer) { Gitlab::Git::LfsPointerFile.new(file_content) } + + before do + create(:lfs_object, oid: lfs_pointer.sha256, size: lfs_pointer.size) + end + + it 'links LfsObjects to project' do expect do - described_class.link_lfs_objects(project, branch_name) do |t| - t.new_file(file_path, file_content) - end + subject.new_file(file_path, file_content) end.to change { project.lfs_objects.count }.by(1) end - - it 'skips linking LfsObjects if the block returns falsey' do - expect do - described_class.link_lfs_objects(project, branch_name) do |t| - t.new_file(file_path, file_content) - false - end - end.not_to change { project.lfs_objects.count } - end - - it 'returns the result of the block' do - result = described_class.link_lfs_objects(project, branch_name) do |t| - :dummy_commit - end - - expect(result).to eq :dummy_commit - end end end end