From c0f921606c91e3e5c601497f57becbf4c6a8e3ac Mon Sep 17 00:00:00 2001 From: Saverio Miroddi Date: Thu, 17 Aug 2017 20:26:06 +0200 Subject: [PATCH] Correct the cherry-pick message for merge commits The list of commits must be generated from the merge request, not from a diff of the branches. --- app/models/commit.rb | 17 +++++------ app/models/repository.rb | 4 +-- spec/models/commit_spec.rb | 43 ++++++++++++++++++++++------ spec/requests/api/commits_spec.rb | 2 +- spec/requests/api/v3/commits_spec.rb | 2 +- 5 files changed, 45 insertions(+), 23 deletions(-) diff --git a/app/models/commit.rb b/app/models/commit.rb index f36e07fc3ca..5f59d088cd2 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -251,19 +251,16 @@ class Commit project.repository.next_branch("cherry-pick-#{short_id}", mild: true) end - def cherry_pick_description(start_project, start_branch_name) + def cherry_pick_description(user) message_buffer = "(cherry picked from commit #{sha})" - if merge_commit? - compare = CompareService.new(project, sha).execute(start_project, start_branch_name) + if merged_merge_request?(user) + commits_in_merge_request = merged_merge_request(user).commits - # Ignore the merge commit. - commits_in_merge = compare.commits[0..-2] - - if commits_in_merge.present? + if commits_in_merge_request.present? message_buffer << "\n" - commits_in_merge.each do |commit_in_merge| + commits_in_merge_request.each do |commit_in_merge| message_buffer << "\n#{commit_in_merge.short_id} #{commit_in_merge.title}" end end @@ -272,8 +269,8 @@ class Commit message_buffer end - def cherry_pick_message(start_project, start_branch_name) - %Q{#{message}\n\n#{cherry_pick_description(start_project, start_branch_name)}} + def cherry_pick_message(user) + %Q{#{message}\n\n#{cherry_pick_description(user)}} end def revert_description(user) diff --git a/app/models/repository.rb b/app/models/repository.rb index c11f9d53497..7e38c06e6db 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -879,9 +879,7 @@ class Repository committer = user_to_committer(user) - commit_message = commit.cherry_pick_message(start_project, start_branch_name) - - create_commit(message: commit_message, + create_commit(message: commit.cherry_pick_message(user), author: { email: commit.author_email, name: commit.author_name, diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index 9711222b3a1..98054b91c29 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -196,25 +196,52 @@ eos end describe '#cherry_pick_message' do - let(:regular_commit) { project.commit('video') } - let(:merge_commit) { project.commit('wip') } + let(:user) { create(:user) } context 'of a regular commit' do - it { expect(regular_commit.cherry_pick_message(project, 'master')).to include("\n\n(cherry picked from commit 88790590ed1337ab189bccaa355f068481c90bec)") } + let(:commit) { project.commit('video') } + + it { expect(commit.cherry_pick_message(user)).to include("\n\n(cherry picked from commit 88790590ed1337ab189bccaa355f068481c90bec)") } end context 'of a merge commit' do + let(:repository) { project.repository } + + let(:commit_options) do + author = repository.user_to_committer(user) + commit_options = { message: 'Test message', committer: author, author: author } + end + + let(:merge_commit) do + merge_request = create(:merge_request, + source_branch: 'feature', + target_branch: 'master', + source_project: project, + author: user) + + merge_commit_id = repository.merge(user, + merge_request.diff_head_sha, + merge_request, + commit_options) + + merge_commit = repository.commit(merge_commit_id) + + # Manually mark as completed. + # + merge_request.update(merge_commit_sha: merge_commit_id) + + merge_commit + end + it do expected_appended_text = <<~STR.rstrip + (cherry picked from commit #{merge_commit.sha}) - (cherry picked from commit b9238ee5bf1d7359dd3b8c89fd76c1c7f8b75aba) - - 6d664995 This commit will be fixupped against - 64117577 fixup! This commit will be fixupped against + 0b4bc9a4 Feature added STR - expect(merge_commit.cherry_pick_message(project, 'master')).to include(expected_appended_text) + expect(merge_commit.cherry_pick_message(user)).to include(expected_appended_text) end end end diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index 1fba7fcc10a..57baf9cd3b3 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -791,7 +791,7 @@ describe API::Commits do expect(response).to have_gitlab_http_status(201) expect(response).to match_response_schema('public_api/v4/commit/basic') expect(json_response['title']).to eq(commit.title) - expect(json_response['message']).to eq(commit.cherry_pick_message(project, branch)) + expect(json_response['message']).to eq(commit.cherry_pick_message(user)) expect(json_response['author_name']).to eq(commit.author_name) expect(json_response['committer_name']).to eq(user.name) end diff --git a/spec/requests/api/v3/commits_spec.rb b/spec/requests/api/v3/commits_spec.rb index 9edf21719cd..8fb96b3c7c5 100644 --- a/spec/requests/api/v3/commits_spec.rb +++ b/spec/requests/api/v3/commits_spec.rb @@ -474,7 +474,7 @@ describe API::V3::Commits do expect(response).to have_http_status(201) expect(json_response['title']).to eq(master_pickable_commit.title) - expect(json_response['message']).to eq(master_pickable_commit.cherry_pick_message(project, 'master')) + expect(json_response['message']).to eq(master_pickable_commit.cherry_pick_message(user)) expect(json_response['author_name']).to eq(master_pickable_commit.author_name) expect(json_response['committer_name']).to eq(user.name) end