From 45d6f9fde82ec0e099d0ef1990eddc2f2a5536e7 Mon Sep 17 00:00:00 2001 From: Joe Woodward Date: Tue, 13 Jun 2023 18:50:45 +0100 Subject: [PATCH 1/2] Introduce find_changed_paths with merge_commit_diff_mode Gitaly has implemented the ability to specify how to diff merge commits when requesting a list of changed paths for a list of commits. https://gitlab.com/gitlab-org/gitaly/-/issues/4827 https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5564 After this change we can now pass `merge_commit_diff_mode: :all_parents` to exclude changes that exist in all parents of the merge commit. This is useful when we have a merge commit that merged changes from base branch back into the current branch. e.g. 1. User branches from master to new branch named feature/foo_bar 2. User changes ./foo_bar.rb and commits change to feature/foo_bar 3. Another user merges a change to ./bar_baz.rb into master 4. User merges master into feature/foo_bar 5. User pushes to GitLab 6. GitLab checks which files have changed Related to https://gitlab.com/gitlab-org/gitlab/-/issues/23625 Changelog: added --- .../development/merge_commit_diff_modes.yml | 8 + lib/gitlab/gitaly_client/commit_service.rb | 44 ++- spec/lib/gitlab/git/repository_spec.rb | 2 +- .../gitaly_client/commit_service_spec.rb | 281 ++++++++++++++++-- spec/support/helpers/test_env.rb | 22 +- 5 files changed, 318 insertions(+), 39 deletions(-) create mode 100644 config/feature_flags/development/merge_commit_diff_modes.yml diff --git a/config/feature_flags/development/merge_commit_diff_modes.yml b/config/feature_flags/development/merge_commit_diff_modes.yml new file mode 100644 index 00000000000000..8fe9f28c55dd2a --- /dev/null +++ b/config/feature_flags/development/merge_commit_diff_modes.yml @@ -0,0 +1,8 @@ +--- +name: merge_commit_diff_modes +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/123501 +rollout_issue_url: +milestone: '16.1' +type: development +group: group::source code +default_enabled: false diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb index f7d6b2402e4393..41abdb333ea884 100644 --- a/lib/gitlab/gitaly_client/commit_service.rb +++ b/lib/gitlab/gitaly_client/commit_service.rb @@ -12,6 +12,11 @@ class CommitService 'unspecified' => Gitaly::CommitDiffRequest::WhitespaceChanges::WHITESPACE_CHANGES_UNSPECIFIED }.freeze + MERGE_COMMIT_DIFF_MODES = { + all_parents: Gitaly::FindChangedPathsRequest::MergeCommitDiffMode::MERGE_COMMIT_DIFF_MODE_ALL_PARENTS, + include_merges: Gitaly::FindChangedPathsRequest::MergeCommitDiffMode::MERGE_COMMIT_DIFF_MODE_INCLUDE_MERGES + }.freeze + TREE_ENTRIES_DEFAULT_LIMIT = 100_000 def initialize(repository) @@ -231,8 +236,35 @@ def diff_stats(left_commit_sha, right_commit_sha) response.flat_map { |rsp| rsp.stats.to_a } end - def find_changed_paths(commits) - request = find_changed_paths_request(commits) + # When finding changed paths and passing a sha for a merge commit we can + # specify how to diff the commit. + # + # When diffing a merge commit and merge_commit_diff_mode is :all_parents + # file paths are only returned if changed in both parents (or all parents + # if diffing an octopus merge) + # + # This means if we create a merge request that includes a merge commit + # of changes already existing in the target branch, we can omit those + # changes when looking up the changed paths. + # + # e.g. + # 1. User branches from master to new branch named feature/foo_bar + # 2. User changes ./foo_bar.rb and commits change to feature/foo_bar + # 3. Another user merges a change to ./bar_baz.rb to master + # 4. User merges master into feature/foo_bar + # 5. User pushes to GitLab + # 6. GitLab checks which files have changed + # + # case merge_commit_diff_mode + # when :all_parents + # ['foo_bar.rb'] + # when :include_merges + # ['foo_bar.rb', 'bar_baz.rb'], + # else # defaults to :include_merges behavior + # ['foo_bar.rb', 'bar_baz.rb'], + # + def find_changed_paths(commits, merge_commit_diff_mode: nil) + request = find_changed_paths_request(commits, merge_commit_diff_mode) response = gitaly_client_call(@repository.storage, :diff_service, :find_changed_paths, request, timeout: GitalyClient.medium_timeout) response.flat_map do |msg| @@ -595,9 +627,11 @@ def call_find_commit(revision) response.commit end - def find_changed_paths_request(commits) + def find_changed_paths_request(commits, merge_commit_diff_mode) + diff_mode = MERGE_COMMIT_DIFF_MODES[merge_commit_diff_mode] if Feature.enabled?(:merge_commit_diff_modes) + if Feature.disabled?(:find_changed_paths_new_format) - return Gitaly::FindChangedPathsRequest.new(repository: @gitaly_repo, commits: commits) + return Gitaly::FindChangedPathsRequest.new(repository: @gitaly_repo, commits: commits, merge_commit_diff_mode: diff_mode) end commit_requests = commits.map do |commit| @@ -606,7 +640,7 @@ def find_changed_paths_request(commits) ) end - Gitaly::FindChangedPathsRequest.new(repository: @gitaly_repo, requests: commit_requests) + Gitaly::FindChangedPathsRequest.new(repository: @gitaly_repo, requests: commit_requests, merge_commit_diff_mode: diff_mode) end end end diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 23f5c61d3355c9..0a9f6650615495 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -1454,7 +1454,7 @@ def create_commit(blobs) it "returns the number of commits in the whole repository" do options = { all: true } - expect(repository.count_commits(options)).to eq(316) + expect(repository.count_commits(options)).to eq(322) end end diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb index 0bdb9f7938d5fc..f58aa93c2a2fdb 100644 --- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb @@ -168,34 +168,171 @@ end describe '#find_changed_paths' do - let(:commits) { %w[1a0b36b3cdad1d2ee32457c102a8c0b7056fa863 cfe32cf61b73a0d5e9f13e774abde7ff789b1660] } + let(:mapped_merge_commit_diff_mode) { described_class::MERGE_COMMIT_DIFF_MODES[merge_commit_diff_mode] } + let(:commits) do + %w[ + ade1c0b4b116209ed2a9958436b26f89085ec383 + 594937c22df7a093888ff13af518f2b683f5f719 + 760c58db5a6f3b64ad7e3ff6b3c4a009da7d9b33 + 2b298117a741cdb06eb48df2c33f1390cf89f7e8 + c41e12c387b4e0e41bfc17208252d6a6430f2fcd + 1ada92f78a19f27cb442a0a205f1c451a3a15432 + ] + end + let(:requests) do - [ + commits.map do |commit| Gitaly::FindChangedPathsRequest::Request.new( - commit_request: Gitaly::FindChangedPathsRequest::Request::CommitRequest.new(commit_revision: '1a0b36b3cdad1d2ee32457c102a8c0b7056fa863') - ), - Gitaly::FindChangedPathsRequest::Request.new( - commit_request: Gitaly::FindChangedPathsRequest::Request::CommitRequest.new(commit_revision: 'cfe32cf61b73a0d5e9f13e774abde7ff789b1660') + commit_request: Gitaly::FindChangedPathsRequest::Request::CommitRequest.new(commit_revision: commit) ) - ] + end end - it 'sends an RPC request and returns the stats' do - request = Gitaly::FindChangedPathsRequest.new(repository: repository_message, requests: requests) + let(:request) do + Gitaly::FindChangedPathsRequest.new(repository: repository_message, requests: requests, merge_commit_diff_mode: merge_commit_diff_mode) + end + + subject { described_class.new(repository).find_changed_paths(commits, merge_commit_diff_mode: merge_commit_diff_mode).as_json } + + before do + allow(Gitaly::FindChangedPathsRequest).to receive(:new).and_call_original + end - changed_paths_response = Gitaly::FindChangedPathsResponse.new( - paths: [{ - path: "app/assets/javascripts/boards/components/project_select.vue", - status: :MODIFIED - }]) + shared_examples 'includes paths different in any parent' do + let(:changed_paths) do + [ + { path: 'files/locked/foo.lfs', status: 'ADDED' }, + { path: 'files/locked/foo.lfs', status: 'MODIFIED' }, + { path: 'files/locked/bar.lfs', status: 'ADDED' }, + { path: 'files/locked/foo.lfs', status: 'MODIFIED' }, + { path: 'files/locked/bar.lfs', status: 'ADDED' }, + { path: 'files/locked/bar.lfs', status: 'MODIFIED' }, + { path: 'files/locked/bar.lfs', status: 'MODIFIED' }, + { path: 'files/locked/baz.lfs', status: 'ADDED' }, + { path: 'files/locked/baz.lfs', status: 'ADDED' } + ].as_json + end + + it 'returns all paths, including ones from merge commits' do + is_expected.to eq(changed_paths) + end + end + + shared_examples 'includes paths different in all parents' do + let(:changed_paths) do + [ + { path: 'files/locked/foo.lfs', status: 'ADDED' }, + { path: 'files/locked/foo.lfs', status: 'MODIFIED' }, + { path: 'files/locked/bar.lfs', status: 'ADDED' }, + { path: 'files/locked/bar.lfs', status: 'MODIFIED' }, + { path: 'files/locked/baz.lfs', status: 'ADDED' }, + { path: 'files/locked/baz.lfs', status: 'ADDED' } + ].as_json + end + + it 'returns only paths different in all parents' do + is_expected.to eq(changed_paths) + end + end - expect_any_instance_of(Gitaly::DiffService::Stub).to receive(:find_changed_paths) - .with(request, kind_of(Hash)).and_return([changed_paths_response]) + shared_examples 'uses requests format' do + it 'passes the revs via the requests kwarg as CommitRequest objects' do + subject + expect(Gitaly::FindChangedPathsRequest) + .to have_received(:new).with( + repository: repository_message, + requests: requests, + merge_commit_diff_mode: mapped_merge_commit_diff_mode + ) + end + end + + context 'when merge_commit_diff_mode is nil' do + let(:merge_commit_diff_mode) { nil } - returned_value = described_class.new(repository).find_changed_paths(commits) - mapped_expected_value = changed_paths_response.paths.map { |path| Gitlab::Git::ChangedPath.new(status: path.status, path: path.path) } + include_examples 'includes paths different in any parent' - expect(returned_value.as_json).to eq(mapped_expected_value.as_json) + include_examples 'uses requests format' + end + + context 'when merge_commit_diff_mode is :unspecified' do + let(:merge_commit_diff_mode) { :unspecified } + + include_examples 'includes paths different in any parent' + + include_examples 'uses requests format' + end + + context 'when merge_commit_diff_mode is :include_merges' do + let(:merge_commit_diff_mode) { :include_merges } + + include_examples 'includes paths different in any parent' + + include_examples 'uses requests format' + end + + context 'when merge_commit_diff_mode is invalid' do + let(:merge_commit_diff_mode) { 'invalid' } + + include_examples 'includes paths different in any parent' + + include_examples 'uses requests format' + end + + context 'when merge_commit_diff_mode is :all_parents' do + let(:merge_commit_diff_mode) { :all_parents } + + include_examples 'includes paths different in all parents' + + include_examples 'uses requests format' + end + + context 'when feature flag "merge_commit_diff_modes" is disabled' do + let(:mapped_merge_commit_diff_mode) { nil } + + before do + stub_feature_flags(merge_commit_diff_modes: false) + end + + context 'when merge_commit_diff_mode is nil' do + let(:merge_commit_diff_mode) { nil } + + include_examples 'includes paths different in any parent' + + include_examples 'uses requests format' + end + + context 'when merge_commit_diff_mode is :unspecified' do + let(:merge_commit_diff_mode) { :unspecified } + + include_examples 'includes paths different in any parent' + + include_examples 'uses requests format' + end + + context 'when merge_commit_diff_mode is :include_merges' do + let(:merge_commit_diff_mode) { :include_merges } + + include_examples 'includes paths different in any parent' + + include_examples 'uses requests format' + end + + context 'when merge_commit_diff_mode is invalid' do + let(:merge_commit_diff_mode) { 'invalid' } + + include_examples 'includes paths different in any parent' + + include_examples 'uses requests format' + end + + context 'when merge_commit_diff_mode is :all_parents' do + let(:merge_commit_diff_mode) { :all_parents } + + include_examples 'includes paths different in any parent' + + include_examples 'uses requests format' + end end context 'when feature flag "find_changed_paths_new_format" is disabled' do @@ -203,22 +340,104 @@ stub_feature_flags(find_changed_paths_new_format: false) end - it 'sends an RPC request and returns the stats' do - request = Gitaly::FindChangedPathsRequest.new(repository: repository_message, commits: commits) + shared_examples 'uses commits format' do + it do + subject + expect(Gitaly::FindChangedPathsRequest) + .to have_received(:new).with( + repository: repository_message, + commits: commits, + merge_commit_diff_mode: mapped_merge_commit_diff_mode + ) + end + end + + context 'when merge_commit_diff_mode is nil' do + let(:merge_commit_diff_mode) { nil } + + include_examples 'includes paths different in any parent' + + include_examples 'uses commits format' + end + + context 'when merge_commit_diff_mode is :unspecified' do + let(:merge_commit_diff_mode) { :unspecified } + + include_examples 'includes paths different in any parent' + + include_examples 'uses commits format' + end + + context 'when merge_commit_diff_mode is :include_merges' do + let(:merge_commit_diff_mode) { :include_merges } + + include_examples 'includes paths different in any parent' + + include_examples 'uses commits format' + end + + context 'when merge_commit_diff_mode is invalid' do + let(:merge_commit_diff_mode) { 'invalid' } + + include_examples 'includes paths different in any parent' + + include_examples 'uses commits format' + end + + context 'when merge_commit_diff_mode is :all_parents' do + let(:merge_commit_diff_mode) { :all_parents } + + include_examples 'includes paths different in all parents' + + include_examples 'uses commits format' + end + + context 'when feature flag "merge_commit_diff_modes" is disabled' do + let(:mapped_merge_commit_diff_mode) { nil } + + before do + stub_feature_flags(merge_commit_diff_modes: false) + end - changed_paths_response = Gitaly::FindChangedPathsResponse.new( - paths: [{ - path: "app/assets/javascripts/boards/components/project_select.vue", - status: :MODIFIED - }]) + context 'when merge_commit_diff_mode is nil' do + let(:merge_commit_diff_mode) { nil } + + include_examples 'includes paths different in any parent' + + include_examples 'uses commits format' + end - expect_any_instance_of(Gitaly::DiffService::Stub).to receive(:find_changed_paths) - .with(request, kind_of(Hash)).and_return([changed_paths_response]) + context 'when merge_commit_diff_mode is :unspecified' do + let(:merge_commit_diff_mode) { :unspecified } - returned_value = described_class.new(repository).find_changed_paths(commits) - mapped_expected_value = changed_paths_response.paths.map { |path| Gitlab::Git::ChangedPath.new(status: path.status, path: path.path) } + include_examples 'includes paths different in any parent' - expect(returned_value.as_json).to eq(mapped_expected_value.as_json) + include_examples 'uses commits format' + end + + context 'when merge_commit_diff_mode is :include_merges' do + let(:merge_commit_diff_mode) { :include_merges } + + include_examples 'includes paths different in any parent' + + include_examples 'uses commits format' + end + + context 'when merge_commit_diff_mode is invalid' do + let(:merge_commit_diff_mode) { 'invalid' } + + include_examples 'includes paths different in any parent' + + include_examples 'uses commits format' + end + + context 'when merge_commit_diff_mode is :all_parents' do + let(:merge_commit_diff_mode) { :all_parents } + + include_examples 'includes paths different in any parent' + + include_examples 'uses commits format' + end end end end diff --git a/spec/support/helpers/test_env.rb b/spec/support/helpers/test_env.rb index c45b48e27f84a8..da4954c1a5f06a 100644 --- a/spec/support/helpers/test_env.rb +++ b/spec/support/helpers/test_env.rb @@ -9,7 +9,24 @@ module TestEnv ComponentFailedToInstallError = Class.new(StandardError) - # When developing the seed repository, comment out the branch you will modify. + # https://gitlab.com/gitlab-org/gitlab-test is used to seed your local gdk + # GitLab application and is also used in rspec tests. Because of this, when + # building and testing features that require a specific type of file, you can + # add them to the gitlab-test repo in order to access that blob during + # development or testing. + # + # To add new branches + # + # 1. Push a new branch to gitlab-org/gitlab-test. + # 2. Execute rm -rf tmp/tests in your gitlab repo. + # 3. Add your branch and its HEAD commit sha to the BRANCH_SHA hash + # + # To add new commits to an existing branch + # + # 1. Push a new commit to a branch in gitlab-org/gitlab-test. + # 2. Execute rm -rf tmp/tests in your gitlab repo. + # 3. Update the HEAD sha value in the BRANCH_SHA hash + # BRANCH_SHA = { 'signed-commits' => 'c7794c1', 'gpg-signed' => '8a852d5', @@ -94,7 +111,8 @@ module TestEnv 'smime-signed-commits' => 'ed775cc', 'Ääh-test-utf-8' => '7975be0', 'ssh-signed-commit' => '7b5160f', - 'changes-with-whitespace' => 'f2d141fadb33ceaafc95667c1a0a308ad5edc5f9' + 'changes-with-whitespace' => 'f2d141fadb33ceaafc95667c1a0a308ad5edc5f9', + 'lock-detection' => '1ada92f78a19f27cb442a0a205f1c451a3a15432' }.freeze # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily -- GitLab From 62d52d4bc3b4232d0821ef73184c127860edf4b2 Mon Sep 17 00:00:00 2001 From: Joe Woodward Date: Fri, 16 Jun 2023 12:59:01 +0000 Subject: [PATCH 2/2] Add rollout issue --- config/feature_flags/development/merge_commit_diff_modes.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/feature_flags/development/merge_commit_diff_modes.yml b/config/feature_flags/development/merge_commit_diff_modes.yml index 8fe9f28c55dd2a..2da966e1cbbaf6 100644 --- a/config/feature_flags/development/merge_commit_diff_modes.yml +++ b/config/feature_flags/development/merge_commit_diff_modes.yml @@ -1,7 +1,7 @@ --- name: merge_commit_diff_modes introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/123501 -rollout_issue_url: +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/415579 milestone: '16.1' type: development group: group::source code -- GitLab