From fbc365f8857770cab0c0d042608dbc290f36737c Mon Sep 17 00:00:00 2001 From: Joe Woodward Date: Fri, 16 Jun 2023 16:49:06 +0100 Subject: [PATCH 1/2] Do not diff paths changed in merge commits When a merge commit passes through our diff checks we would previously check each path changed in the merge commit. However, our diffing logic would return any path that was changed in any parent, meaning changes that are already in history were being detected as new changes. Gitaly recently introduced a new flag for the FindChagnedPaths GRPC to allow us to diff against all_parents, meaning, only new chanages in a merge commit would be detected. With new changes meaning a change that did not exist in all of the parents i.e. if a user merges and fixes a merge conflict with code different to any of the parents, or they manually edited, created, or deleted a file during the merge. This allows us to correctly execute push rules against new changes only. Fixes https://gitlab.com/gitlab-org/gitlab/-/issues/23625 Changelog: fixed --- ee/spec/lib/gitlab/checks/diff_check_spec.rb | 12 +-- lib/gitlab/checks/diff_check.rb | 5 +- spec/lib/gitlab/checks/diff_check_spec.rb | 83 +++++++++++++++++++- 3 files changed, 90 insertions(+), 10 deletions(-) diff --git a/ee/spec/lib/gitlab/checks/diff_check_spec.rb b/ee/spec/lib/gitlab/checks/diff_check_spec.rb index ff00253e3057be..619a0417377e2e 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 083c2448a0abfc..1186b532baff83 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 0845c7465452c6..a44a0763aa3887 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 "doesn't raise any 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 -- GitLab From 42f0de60b2a0793b8ae80d5fc4bb612fc84a192f Mon Sep 17 00:00:00 2001 From: Ash McKenzie Date: Sun, 18 Jun 2023 08:13:46 +0000 Subject: [PATCH 2/2] Rename it description to be accurate --- spec/lib/gitlab/checks/diff_check_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/lib/gitlab/checks/diff_check_spec.rb b/spec/lib/gitlab/checks/diff_check_spec.rb index a44a0763aa3887..dd467537a4f771 100644 --- a/spec/lib/gitlab/checks/diff_check_spec.rb +++ b/spec/lib/gitlab/checks/diff_check_spec.rb @@ -147,7 +147,7 @@ create(:lfs_file_lock, user: owner, project: project, path: 'files/locked/baz.lfs') end - it "doesn't raise any error" do + 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 -- GitLab