Commit 7916f078 authored by Patrick Steinhardt's avatar Patrick Steinhardt

checks: Skip LFS checks when deleting refs

The LFS check asserts that for a given reference update, all newly
reachable LFS pointers refer to existing LFS objects. It is trivial to
see that when a reference gets deleted, no new git objects can become
reachable and thus its not possible for the deletion to introduce new
LFS pointers. There already is a spec which asserts that integrity
checks are not executed in case the reference is being deleted. But the
spec itself is wrong: it uses an empty newrev, while git uses the
all-zeroes object name to identify deletions.

Fix both the test and the uncovered bug that LFS integrity checks
actually do run for deletions.
parent 3ac72085
---
title: 'checks: Skip LFS checks when deleting refs'
merge_request: 55609
author:
type: fixed
...@@ -13,6 +13,7 @@ module Gitlab ...@@ -13,6 +13,7 @@ module Gitlab
return unless project.lfs_enabled? return unless project.lfs_enabled?
return if skip_lfs_integrity_check return if skip_lfs_integrity_check
return if deletion?
logger.log_timed(LOG_MESSAGE) do logger.log_timed(LOG_MESSAGE) do
lfs_check = Checks::LfsIntegrity.new(project, newrev, logger.time_left) lfs_check = Checks::LfsIntegrity.new(project, newrev, logger.time_left)
......
...@@ -39,16 +39,29 @@ RSpec.describe Gitlab::Checks::LfsCheck do ...@@ -39,16 +39,29 @@ RSpec.describe Gitlab::Checks::LfsCheck do
end end
end end
context 'deletion' do context 'with deletion' do
let(:changes) { { oldrev: oldrev, ref: ref } } shared_examples 'a skipped integrity check' do
it 'skips integrity check' do it 'skips integrity check' do
expect(project.repository).not_to receive(:new_objects) expect(project.repository).not_to receive(:new_objects)
expect_any_instance_of(Gitlab::Git::LfsChanges).not_to receive(:new_pointers)
subject.validate! subject.validate!
end end
end end
context 'with missing newrev' do
it_behaves_like 'a skipped integrity check' do
let(:changes) { { oldrev: oldrev, ref: ref } }
end
end
context 'with blank newrev' do
it_behaves_like 'a skipped integrity check' do
let(:changes) { { oldrev: oldrev, newrev: Gitlab::Git::BLANK_SHA, ref: ref } }
end
end
end
it 'fails if any LFS blobs are missing' do it 'fails if any LFS blobs are missing' do
expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, /LFS objects are missing/) expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, /LFS objects are missing/)
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment