diff --git a/ee/spec/lib/gitlab/checks/diff_check_spec.rb b/ee/spec/lib/gitlab/checks/diff_check_spec.rb index ff00253e3057be6de6b408ddfef8fcf07774ee80..619a0417377e2e3fdd1f1872387c9dbf744ec3d5 100644 --- a/ee/spec/lib/gitlab/checks/diff_check_spec.rb +++ b/ee/spec/lib/gitlab/checks/diff_check_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Checks::DiffCheck do +RSpec.describe Gitlab::Checks::DiffCheck, feature_category: :source_code_management do include FakeBlobHelpers include_context 'push rules checks context' @@ -67,7 +67,7 @@ describe "#validate_code_owners" do let!(:code_owner) { create(:user, username: "owner-1") } let(:project) { create(:project, :repository) } - let(:codeowner_content) { "*.rb @#{code_owner.username}\ndocs/CODEOWNERS @owner-1\n*.js.coffee @owner-1" } + let(:codeowner_content) { "*.rb @#{code_owner.username}\ndocs/CODEOWNERS @owner-1\n*.js.coffee @owner-1\nCHANGELOG @owner-1" } let(:codeowner_blob) { fake_blob(path: "CODEOWNERS", data: codeowner_content) } let(:codeowner_blob_ref) { fake_blob(path: "CODEOWNERS", data: codeowner_content) } let(:codeowner_lookup_ref) { merge_request.target_branch } @@ -97,14 +97,14 @@ # This particular commit renames a file: allow(project.repository).to receive(:new_commits).and_return( - [project.repository.commit('6907208d755b60ebeacb2e9dfea74c92c3449a1f')] + [project.repository.commit('94bb47ca1297b7b3731ff2a36923640991e9236f')] ) end it "returns an error message" do expect { diff_check.validate! }.to raise_error do |error| expect(error).to be_a(Gitlab::GitAccess::ForbiddenError) - expect(error.message).to include("CODEOWNERS` were matched:\n- *.js.coffee") + expect(error.message).to include("CODEOWNERS` were matched:\n- CHANGELOG") end end end @@ -288,8 +288,8 @@ end context 'when file is renamed' do - let_it_be(:filename) { 'files/js/commit.js.coffee' } - let_it_be(:sha) { '6907208d755b60ebeacb2e9dfea74c92c3449a1f' } + let_it_be(:filename) { 'CHANGELOG' } + let_it_be(:sha) { '94bb47ca1297b7b3731ff2a36923640991e9236f' } it_behaves_like 'a locked file' end diff --git a/lib/gitlab/checks/diff_check.rb b/lib/gitlab/checks/diff_check.rb index 083c2448a0abfcfc260673926c4cc0ab118c3245..1186b532baff83acfa973fa355c367834dcd351c 100644 --- a/lib/gitlab/checks/diff_check.rb +++ b/lib/gitlab/checks/diff_check.rb @@ -18,7 +18,10 @@ def validate! return unless should_run_validations? return if commits.empty? - paths = project.repository.find_changed_paths(commits.map(&:sha)) + paths = project.repository.find_changed_paths( + commits.map(&:sha), merge_commit_diff_mode: :all_parents + ) + paths.each do |path| validate_path(path) end diff --git a/spec/lib/gitlab/checks/diff_check_spec.rb b/spec/lib/gitlab/checks/diff_check_spec.rb index 0845c7465452c609765d3ca3a192e0036c208984..dd467537a4f7713a3ddc58660b48f8eb1b12b715 100644 --- a/spec/lib/gitlab/checks/diff_check_spec.rb +++ b/spec/lib/gitlab/checks/diff_check_spec.rb @@ -24,11 +24,42 @@ end end + context 'when commits include merge commit' do + before do + allow(project.repository).to receive(:new_commits).and_return([project.repository.commit(merge_commit)]) + allow(subject).to receive(:should_run_validations?).and_return(true) + allow(subject).to receive(:validate_path) + allow(subject).to receive(:validate_file_paths) + subject.validate! + end + + context 'when merge commit does not include additional changes' do + let(:merge_commit) { '2b298117a741cdb06eb48df2c33f1390cf89f7e8' } + + it 'checks the additional changes' do + expect(subject).to have_received(:validate_file_paths).with([]) + end + end + + context 'when merge commit includes additional changes' do + let(:merge_commit) { '1ada92f78a19f27cb442a0a205f1c451a3a15432' } + let(:file_paths) { ['files/locked/baz.lfs'] } + + it 'checks the additional changes' do + expect(subject).to have_received(:validate_file_paths).with(file_paths) + end + end + end + context 'when commits is not empty' do + let(:new_commits) do + from = 'be93687618e4b132087f430a4d8fc3a609c9b77c' + to = '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51' + project.repository.commits_between(from, to) + end + before do - allow(project.repository).to receive(:new_commits).and_return( - project.repository.commits_between('be93687618e4b132087f430a4d8fc3a609c9b77c', '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51') - ) + allow(project.repository).to receive(:new_commits).and_return(new_commits) end context 'when deletion is true' do @@ -74,6 +105,52 @@ expect { subject.validate! }.not_to raise_error end end + + context 'when a merge commit merged a file locked by another user' do + let(:new_commits) do + project.repository.commits_by(oids: %w[ + 760c58db5a6f3b64ad7e3ff6b3c4a009da7d9b33 + 2b298117a741cdb06eb48df2c33f1390cf89f7e8 + ]) + end + + before do + create(:lfs_file_lock, user: owner, project: project, path: 'files/locked/foo.lfs') + create(:lfs_file_lock, user: user, project: project, path: 'files/locked/bar.lfs') + end + + it "doesn't raise any error" do + expect { subject.validate! }.not_to raise_error + end + end + + context 'when a merge commit includes additional file locked by another user' do + # e.g. when merging the user added an additional change. + # This merge commit: https://gitlab.com/gitlab-org/gitlab-test/-/commit/1ada92f + # merges `files/locked/bar.lfs` and also adds a new file + # `files/locked/baz.lfs`. In this case we ignore `files/locked/bar.lfs` + # as it is already detected in the commit c41e12c, however, we do + # detect the new `files/locked/baz.lfs` file. + # + let(:new_commits) do + project.repository.commits_by(oids: %w[ + 760c58db5a6f3b64ad7e3ff6b3c4a009da7d9b33 + 2b298117a741cdb06eb48df2c33f1390cf89f7e8 + c41e12c387b4e0e41bfc17208252d6a6430f2fcd + 1ada92f78a19f27cb442a0a205f1c451a3a15432 + ]) + end + + before do + create(:lfs_file_lock, user: owner, project: project, path: 'files/locked/foo.lfs') + create(:lfs_file_lock, user: user, project: project, path: 'files/locked/bar.lfs') + create(:lfs_file_lock, user: owner, project: project, path: 'files/locked/baz.lfs') + end + + it "does raise an error" do + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "The path 'files/locked/baz.lfs' is locked in Git LFS by #{owner.name}") + end + end end end end