Commit b2e23e33 authored by Douwe Maan's avatar Douwe Maan

Merge branch '21459-resolving-merge-conflicts-from-a-forked-repo-through-ui' into 'master'

Fix resolving conflicts on forks

## What does this MR do?

When we resolve conflicts, we create a merge commit in the source branch with parents `[source_branch_head, target_branch_head]`. But when the MR is from a fork, `target_branch_head` might not exist in the source repo at all, so we need to fetch it if it isn't there. We can do this locally so it should be fast.

## Are there points in the code the reviewer needs to double check?

The `TestEnv` changes are needed to reset the branch refs if we're reusing a git directory locally - otherwise, there might not be conflicts!

## Why was this MR needed?

It's a bug in a new feature!

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

Closes #21459.

See merge request !6082
parents 924fdf4a 59dd9e57
...@@ -46,6 +46,9 @@ v 8.12.0 (unreleased) ...@@ -46,6 +46,9 @@ v 8.12.0 (unreleased)
v 8.11.4 (unreleased) v 8.11.4 (unreleased)
- Fix broken gitlab:backup:restore because of bad permissions on repo storage !6098 (Dirk Hörner) - Fix broken gitlab:backup:restore because of bad permissions on repo storage !6098 (Dirk Hörner)
v 8.11.4 (unreleased)
- Fix resolving conflicts on forks
v 8.11.3 (unreleased) v 8.11.3 (unreleased)
- Do not enforce using hash with hidden key in CI configuration. !6079 - Do not enforce using hash with hidden key in CI configuration. !6079
- Allow system info page to handle case where info is unavailable - Allow system info page to handle case where info is unavailable
......
module MergeRequests module MergeRequests
class ResolveService < MergeRequests::BaseService class ResolveService < MergeRequests::BaseService
attr_accessor :conflicts, :rugged, :merge_index attr_accessor :conflicts, :rugged, :merge_index, :merge_request
def execute(merge_request) def execute(merge_request)
@conflicts = merge_request.conflicts @conflicts = merge_request.conflicts
@rugged = project.repository.rugged @rugged = project.repository.rugged
@merge_index = conflicts.merge_index @merge_index = conflicts.merge_index
@merge_request = merge_request
fetch_their_commit!
conflicts.files.each do |file| conflicts.files.each do |file|
write_resolved_file_to_index(file, params[:sections]) write_resolved_file_to_index(file, params[:sections])
...@@ -27,5 +30,21 @@ module MergeRequests ...@@ -27,5 +30,21 @@ module MergeRequests
merge_index.add(path: our_path, oid: rugged.write(new_file, :blob), mode: file.our_mode) merge_index.add(path: our_path, oid: rugged.write(new_file, :blob), mode: file.our_mode)
merge_index.conflict_remove(our_path) merge_index.conflict_remove(our_path)
end end
# If their commit (in the target project) doesn't exist in the source project, it
# can't be a parent for the merge commit we're about to create. If that's the case,
# fetch the target branch ref into the source project so the commit exists in both.
#
def fetch_their_commit!
return if rugged.include?(conflicts.their_commit.oid)
random_string = SecureRandom.hex
project.repository.fetch_ref(
merge_request.target_project.repository.path_to_repo,
"refs/heads/#{merge_request.target_branch}",
"refs/tmp/#{random_string}/head"
)
end
end end
end end
require 'spec_helper'
describe MergeRequests::ResolveService do
let(:user) { create(:user) }
let(:project) { create(:project) }
let(:fork_project) do
create(:forked_project_with_submodules) do |fork_project|
fork_project.build_forked_project_link(forked_to_project_id: fork_project.id, forked_from_project_id: project.id)
fork_project.save
end
end
let(:merge_request) do
create(:merge_request,
source_branch: 'conflict-resolvable', source_project: project,
target_branch: 'conflict-start')
end
let(:merge_request_from_fork) do
create(:merge_request,
source_branch: 'conflict-resolvable-fork', source_project: fork_project,
target_branch: 'conflict-start', target_project: project)
end
describe '#execute' do
context 'with valid params' do
let(:params) do
{
sections: {
'2f6fcd96b88b36ce98c38da085c795a27d92a3dd_14_14' => 'head',
'6eb14e00385d2fb284765eb1cd8d420d33d63fc9_9_9' => 'head',
'6eb14e00385d2fb284765eb1cd8d420d33d63fc9_21_21' => 'origin',
'6eb14e00385d2fb284765eb1cd8d420d33d63fc9_49_49' => 'origin'
},
commit_message: 'This is a commit message!'
}
end
context 'when the source and target project are the same' do
before do
MergeRequests::ResolveService.new(project, user, params).execute(merge_request)
end
it 'creates a commit with the message' do
expect(merge_request.source_branch_head.message).to eq(params[:commit_message])
end
it 'creates a commit with the correct parents' do
expect(merge_request.source_branch_head.parents.map(&:id)).
to eq(['1450cd639e0bc6721eb02800169e464f212cde06',
'75284c70dd26c87f2a3fb65fd5a1f0b0138d3a6b'])
end
end
context 'when the source project is a fork and does not contain the HEAD of the target branch' do
let!(:target_head) do
project.repository.commit_file(user, 'new-file-in-target', '', 'Add new file in target', 'conflict-start', false)
end
before do
MergeRequests::ResolveService.new(fork_project, user, params).execute(merge_request_from_fork)
end
it 'creates a commit with the message' do
expect(merge_request_from_fork.source_branch_head.message).to eq(params[:commit_message])
end
it 'creates a commit with the correct parents' do
expect(merge_request_from_fork.source_branch_head.parents.map(&:id)).
to eq(['404fa3fc7c2c9b5dacff102f353bdf55b1be2813',
target_head])
end
end
end
context 'when a resolution is missing' do
let(:invalid_params) { { sections: { '2f6fcd96b88b36ce98c38da085c795a27d92a3dd_14_14' => 'head' } } }
let(:service) { MergeRequests::ResolveService.new(project, user, invalid_params) }
it 'raises a MissingResolution error' do
expect { service.execute(merge_request) }.
to raise_error(Gitlab::Conflict::File::MissingResolution)
end
end
end
end
...@@ -6,7 +6,7 @@ module TestEnv ...@@ -6,7 +6,7 @@ module TestEnv
# When developing the seed repository, comment out the branch you will modify. # When developing the seed repository, comment out the branch you will modify.
BRANCH_SHA = { BRANCH_SHA = {
'empty-branch' => '7efb185', 'empty-branch' => '7efb185',
'ends-with.json' => '98b0d8b3', 'ends-with.json' => '98b0d8b',
'flatten-dir' => 'e56497b', 'flatten-dir' => 'e56497b',
'feature' => '0b4bc9a', 'feature' => '0b4bc9a',
'feature_conflict' => 'bb5206f', 'feature_conflict' => 'bb5206f',
...@@ -37,9 +37,10 @@ module TestEnv ...@@ -37,9 +37,10 @@ module TestEnv
# need to keep all the branches in sync. # need to keep all the branches in sync.
# We currently only need a subset of the branches # We currently only need a subset of the branches
FORKED_BRANCH_SHA = { FORKED_BRANCH_SHA = {
'add-submodule-version-bump' => '3f547c08', 'add-submodule-version-bump' => '3f547c0',
'master' => '5937ac0', 'master' => '5937ac0',
'remove-submodule' => '2a33e0c0' 'remove-submodule' => '2a33e0c',
'conflict-resolvable-fork' => '404fa3f'
} }
# Test environment # Test environment
...@@ -117,22 +118,7 @@ module TestEnv ...@@ -117,22 +118,7 @@ module TestEnv
system(*%W(#{Gitlab.config.git.bin_path} clone -q #{clone_url} #{repo_path})) system(*%W(#{Gitlab.config.git.bin_path} clone -q #{clone_url} #{repo_path}))
end end
Dir.chdir(repo_path) do set_repo_refs(repo_path, branch_sha)
branch_sha.each do |branch, sha|
# Try to reset without fetching to avoid using the network.
reset = %W(#{Gitlab.config.git.bin_path} update-ref refs/heads/#{branch} #{sha})
unless system(*reset)
if system(*%W(#{Gitlab.config.git.bin_path} fetch origin))
unless system(*reset)
raise 'The fetched test seed '\
'does not contain the required revision.'
end
else
raise 'Could not fetch test seed repository.'
end
end
end
end
# We must copy bare repositories because we will push to them. # We must copy bare repositories because we will push to them.
system(git_env, *%W(#{Gitlab.config.git.bin_path} clone -q --bare #{repo_path} #{repo_path_bare})) system(git_env, *%W(#{Gitlab.config.git.bin_path} clone -q --bare #{repo_path} #{repo_path_bare}))
...@@ -144,6 +130,7 @@ module TestEnv ...@@ -144,6 +130,7 @@ module TestEnv
FileUtils.mkdir_p(target_repo_path) FileUtils.mkdir_p(target_repo_path)
FileUtils.cp_r("#{base_repo_path}/.", target_repo_path) FileUtils.cp_r("#{base_repo_path}/.", target_repo_path)
FileUtils.chmod_R 0755, target_repo_path FileUtils.chmod_R 0755, target_repo_path
set_repo_refs(target_repo_path, BRANCH_SHA)
end end
def repos_path def repos_path
...@@ -160,6 +147,7 @@ module TestEnv ...@@ -160,6 +147,7 @@ module TestEnv
FileUtils.mkdir_p(target_repo_path) FileUtils.mkdir_p(target_repo_path)
FileUtils.cp_r("#{base_repo_path}/.", target_repo_path) FileUtils.cp_r("#{base_repo_path}/.", target_repo_path)
FileUtils.chmod_R 0755, target_repo_path FileUtils.chmod_R 0755, target_repo_path
set_repo_refs(target_repo_path, FORKED_BRANCH_SHA)
end end
# When no cached assets exist, manually hit the root path to create them # When no cached assets exist, manually hit the root path to create them
...@@ -209,4 +197,23 @@ module TestEnv ...@@ -209,4 +197,23 @@ module TestEnv
def git_env def git_env
{ 'GIT_TEMPLATE_DIR' => '' } { 'GIT_TEMPLATE_DIR' => '' }
end end
def set_repo_refs(repo_path, branch_sha)
Dir.chdir(repo_path) do
branch_sha.each do |branch, sha|
# Try to reset without fetching to avoid using the network.
reset = %W(#{Gitlab.config.git.bin_path} update-ref refs/heads/#{branch} #{sha})
unless system(*reset)
if system(*%W(#{Gitlab.config.git.bin_path} fetch origin))
unless system(*reset)
raise 'The fetched test seed '\
'does not contain the required revision.'
end
else
raise 'Could not fetch test seed repository.'
end
end
end
end
end
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