kotovalexarian-likes-gitlab
/
gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Merge branch 'fix-conflict-resolution-with-corrupt-repos' into 'master' 

Fix conflict resolution from corrupted upstream

Closes gitlab-ee#2128

See merge request !11298
This commit is contained in:
Douwe Maan 2017-05-12 20:37:30 +00:00
parent e5a967aa03 ad2bfeb857
commit e4261fe3ce
15 changed files with 268 additions and 200 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
app/controllers/projects/merge_requests_controller.rb
View File

@ -155,8 +155,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
format.html { define_discussion_vars }
format.json do
if @merge_request.conflicts_can_be_resolved_in_ui?
render json: @merge_request.conflicts
if @conflicts_list.can_be_resolved_in_ui?
render json: @conflicts_list
elsif @merge_request.can_be_merged?
render json: {
message: 'The merge conflicts for this merge request have already been resolved. Please return to the merge request.',
@ -173,9 +173,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end
def conflict_for_path
return render_404 unless @merge_request.conflicts_can_be_resolved_in_ui?
return render_404 unless @conflicts_list.can_be_resolved_in_ui?
file = @merge_request.conflicts.file_for_path(params[:old_path], params[:new_path])
file = @conflicts_list.file_for_path(params[:old_path], params[:new_path])
return render_404 unless file
@ -183,7 +183,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end
def resolve_conflicts
return render_404 unless @merge_request.conflicts_can_be_resolved_in_ui?
return render_404 unless @conflicts_list.can_be_resolved_in_ui?
if @merge_request.can_be_merged?
render status: :bad_request, json: { message: 'The merge conflicts for this merge request have already been resolved.' }
@ -191,7 +191,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end
begin
MergeRequests::ResolveService.new(@merge_request.source_project, current_user, params).execute(@merge_request)
MergeRequests::Conflicts::ResolveService.
new(merge_request).
execute(current_user, params)
flash[:notice] = 'All merge conflicts were resolved. The merge request can now be merged.'
@ -459,7 +461,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end
def authorize_can_resolve_conflicts!
return render_404 unless @merge_request.conflicts_can_be_resolved_by?(current_user)
@conflicts_list = MergeRequests::Conflicts::ListService.new(@merge_request)
return render_404 unless @conflicts_list.can_be_resolved_by?(current_user)
end
def module_enabled

28
app/models/merge_request.rb
View File

@ -891,34 +891,6 @@ class MergeRequest < ActiveRecord::Base
project.repository.keep_around(self.merge_commit_sha)
end
def conflicts
@conflicts ||= Gitlab::Conflict::FileCollection.new(self)
end
def conflicts_can_be_resolved_by?(user)
return false unless source_project
access = ::Gitlab::UserAccess.new(user, project: source_project)
access.can_push_to_branch?(source_branch)
end
def conflicts_can_be_resolved_in_ui?
return @conflicts_can_be_resolved_in_ui if defined?(@conflicts_can_be_resolved_in_ui)
return @conflicts_can_be_resolved_in_ui = false unless cannot_be_merged?
return @conflicts_can_be_resolved_in_ui = false unless has_complete_diff_refs?
begin
# Try to parse each conflict. If the MR's mergeable status hasn't been updated,
# ensure that we don't say there are conflicts to resolve when there are no conflict
# files.
conflicts.files.each(&:lines)
@conflicts_can_be_resolved_in_ui = conflicts.files.length > 0
rescue Rugged::OdbError, Gitlab::Conflict::Parser::UnresolvableError, Gitlab::Conflict::FileCollection::ConflictSideMissing
@conflicts_can_be_resolved_in_ui = false
end
end
def has_commits?
merge_request_diff && commits_count > 0
end

6
app/presenters/merge_request_presenter.rb
View File

@ -76,7 +76,7 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
end
def conflict_resolution_path
if conflicts_can_be_resolved_in_ui? && conflicts_can_be_resolved_by?(current_user)
if conflicts.can_be_resolved_in_ui? && conflicts.can_be_resolved_by?(current_user)
conflicts_namespace_project_merge_request_path(project.namespace, project, merge_request)
end
end
@ -141,6 +141,10 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
private
def conflicts
@conflicts ||= MergeRequests::Conflicts::ListService.new(merge_request)
end
def closing_issues
@closing_issues ||= closes_issues(current_user)
end

11
app/services/merge_requests/conflicts/base_service.rb Normal file
View File

@ -0,0 +1,11 @@
module MergeRequests
module Conflicts
class BaseService
attr_reader :merge_request
def initialize(merge_request)
@merge_request = merge_request
end
end
end
end

35
app/services/merge_requests/conflicts/list_service.rb Normal file
View File

@ -0,0 +1,35 @@
module MergeRequests
module Conflicts
class ListService < MergeRequests::Conflicts::BaseService
delegate :file_for_path, :to_json, to: :conflicts
def can_be_resolved_by?(user)
return false unless merge_request.source_project
access = ::Gitlab::UserAccess.new(user, project: merge_request.source_project)
access.can_push_to_branch?(merge_request.source_branch)
end
def can_be_resolved_in_ui?
return @conflicts_can_be_resolved_in_ui if defined?(@conflicts_can_be_resolved_in_ui)
return @conflicts_can_be_resolved_in_ui = false unless merge_request.cannot_be_merged?
return @conflicts_can_be_resolved_in_ui = false unless merge_request.has_complete_diff_refs?
begin
# Try to parse each conflict. If the MR's mergeable status hasn't been
# updated, ensure that we don't say there are conflicts to resolve
# when there are no conflict files.
conflicts.files.each(&:lines)
@conflicts_can_be_resolved_in_ui = conflicts.files.length > 0
rescue Rugged::OdbError, Gitlab::Conflict::Parser::UnresolvableError, Gitlab::Conflict::FileCollection::ConflictSideMissing
@conflicts_can_be_resolved_in_ui = false
end
end
def conflicts
@conflicts ||= Gitlab::Conflict::FileCollection.read_only(merge_request)
end
end
end
end

53
app/services/merge_requests/conflicts/resolve_service.rb Normal file
View File

@ -0,0 +1,53 @@
module MergeRequests
module Conflicts
class ResolveService < MergeRequests::Conflicts::BaseService
MissingFiles = Class.new(Gitlab::Conflict::ResolutionError)
def execute(current_user, params)
rugged = merge_request.source_project.repository.rugged
Gitlab::Conflict::FileCollection.for_resolution(merge_request) do |conflicts_for_resolution|
merge_index = conflicts_for_resolution.merge_index
params[:files].each do |file_params|
conflict_file = conflicts_for_resolution.file_for_path(file_params[:old_path], file_params[:new_path])
write_resolved_file_to_index(merge_index, rugged, conflict_file, file_params)
end
unless merge_index.conflicts.empty?
missing_files = merge_index.conflicts.map { |file| file[:ours][:path] }
raise MissingFiles, "Missing resolutions for the following files: #{missing_files.join(', ')}"
end
commit_params = {
message: params[:commit_message] || conflicts_for_resolution.default_commit_message,
parents: [conflicts_for_resolution.our_commit, conflicts_for_resolution.their_commit].map(&:oid),
tree: merge_index.write_tree(rugged)
}
conflicts_for_resolution.
project.
repository.
resolve_conflicts(current_user, merge_request.source_branch, commit_params)
end
end
private
def write_resolved_file_to_index(merge_index, rugged, file, params)
new_file = if params[:sections]
file.resolve_lines(params[:sections]).map(&:text).join("\n")
elsif params[:content]
file.resolve_content(params[:content])
end
our_path = file.our_path
merge_index.add(path: our_path, oid: rugged.write(new_file, :blob), mode: file.our_mode)
merge_index.conflict_remove(our_path)
end
end
end
end

65
app/services/merge_requests/resolve_service.rb
View File

@ -1,65 +0,0 @@
module MergeRequests
class ResolveService < MergeRequests::BaseService
MissingFiles = Class.new(Gitlab::Conflict::ResolutionError)
attr_accessor :conflicts, :rugged, :merge_index, :merge_request
def execute(merge_request)
@conflicts = merge_request.conflicts
@rugged = project.repository.rugged
@merge_index = conflicts.merge_index
@merge_request = merge_request
fetch_their_commit!
params[:files].each do |file_params|
conflict_file = merge_request.conflicts.file_for_path(file_params[:old_path], file_params[:new_path])
write_resolved_file_to_index(conflict_file, file_params)
end
unless merge_index.conflicts.empty?
missing_files = merge_index.conflicts.map { |file| file[:ours][:path] }
raise MissingFiles, "Missing resolutions for the following files: #{missing_files.join(', ')}"
end
commit_params = {
message: params[:commit_message] || conflicts.default_commit_message,
parents: [conflicts.our_commit, conflicts.their_commit].map(&:oid),
tree: merge_index.write_tree(rugged)
}
project.repository.resolve_conflicts(current_user, merge_request.source_branch, commit_params)
end
def write_resolved_file_to_index(file, params)
new_file = if params[:sections]
file.resolve_lines(params[:sections]).map(&:text).join("\n")
elsif params[:content]
file.resolve_content(params[:content])
end
our_path = file.our_path
merge_index.add(path: our_path, oid: rugged.write(new_file, :blob), mode: file.our_mode)
merge_index.conflict_remove(our_path)
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

5
changelogs/unreleased/fix-conflict-resolution-with-corrupt-repos.yml Normal file
View File

@ -0,0 +1,5 @@
---
title: Prevent further repository corruption when resolving conflicts from a fork
where both the fork and upstream projects require housekeeping
merge_request:
author:

42
lib/gitlab/conflict/file_collection.rb
View File

@ -3,16 +3,33 @@ module Gitlab
class FileCollection
ConflictSideMissing = Class.new(StandardError)
attr_reader :merge_request, :our_commit, :their_commit
attr_reader :merge_request, :our_commit, :their_commit, :project
def initialize(merge_request)
@merge_request = merge_request
@our_commit = merge_request.source_branch_head.raw.raw_commit
@their_commit = merge_request.target_branch_head.raw.raw_commit
end
delegate :repository, to: :project
def repository
merge_request.project.repository
class << self
# We can only write when getting the merge index from the source
# project, because we will write to that project. We don't use this all
# the time because this fetches a ref into the source project, which
# isn't needed for reading.
def for_resolution(merge_request)
project = merge_request.source_project
new(merge_request, project).tap do |file_collection|
project.
repository.
with_repo_branch_commit(merge_request.target_project.repository, merge_request.target_branch) do
yield file_collection
end
end
end
# We don't need to do `with_repo_branch_commit` here, because the target
# project always fetches source refs when creating merge request diffs.
def read_only(merge_request)
new(merge_request, merge_request.target_project)
end
end
def merge_index
@ -55,6 +72,15 @@ Merge branch '#{merge_request.target_branch}' into '#{merge_request.source_branc
#{conflict_filenames.join("\n")}
EOM
end
private
def initialize(merge_request, project)
@merge_request = merge_request
@our_commit = merge_request.source_branch_head.raw.raw_commit
@their_commit = merge_request.target_branch_head.raw.raw_commit
@project = project
end
end
end
end

10
spec/controllers/projects/merge_requests_controller_spec.rb
View File

@ -916,7 +916,9 @@ describe Projects::MergeRequestsController do
end
it 'returns the file in JSON format' do
content = merge_request_with_conflicts.conflicts.file_for_path(path, path).content
content = MergeRequests::Conflicts::ListService.new(merge_request_with_conflicts).
file_for_path(path, path).
content
expect(json_response).to include('old_path' => path,
'new_path' => path,
@ -1040,11 +1042,15 @@ describe Projects::MergeRequestsController do
context 'when a file has identical content to the conflict' do
before do
content = MergeRequests::Conflicts::ListService.new(merge_request_with_conflicts).
file_for_path('files/ruby/popen.rb', 'files/ruby/popen.rb').
content
resolved_files = [
{
'new_path' => 'files/ruby/popen.rb',
'old_path' => 'files/ruby/popen.rb',
'content' => merge_request_with_conflicts.conflicts.file_for_path('files/ruby/popen.rb', 'files/ruby/popen.rb').content
'content' => content
}, {
'new_path' => 'files/ruby/regex.rb',
'old_path' => 'files/ruby/regex.rb',

2
spec/lib/gitlab/conflict/file_collection_spec.rb
View File

@ -2,7 +2,7 @@ require 'spec_helper'
describe Gitlab::Conflict::FileCollection, lib: true do
let(:merge_request) { create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start') }
let(:file_collection) { Gitlab::Conflict::FileCollection.new(merge_request) }
let(:file_collection) { described_class.read_only(merge_request) }
describe '#files' do
it 'returns an array of Conflict::Files' do

65
spec/models/merge_request_spec.rb
View File

@ -1268,71 +1268,6 @@ describe MergeRequest, models: true do
end
end
describe '#conflicts_can_be_resolved_in_ui?' do
def create_merge_request(source_branch)
create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start') do |mr|
mr.mark_as_unmergeable
end
end
it 'returns a falsey value when the MR can be merged without conflicts' do
merge_request = create_merge_request('master')
merge_request.mark_as_mergeable
expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_falsey
end
it 'returns a falsey value when the MR is marked as having conflicts, but has none' do
merge_request = create_merge_request('master')
expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_falsey
end
it 'returns a falsey value when the MR has a missing ref after a force push' do
merge_request = create_merge_request('conflict-resolvable')
allow(merge_request.conflicts).to receive(:merge_index).and_raise(Rugged::OdbError)
expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_falsey
end
it 'returns a falsey value when the MR does not support new diff notes' do
merge_request = create_merge_request('conflict-resolvable')
merge_request.merge_request_diff.update_attributes(start_commit_sha: nil)
expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_falsey
end
it 'returns a falsey value when the conflicts contain a large file' do
merge_request = create_merge_request('conflict-too-large')
expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_falsey
end
it 'returns a falsey value when the conflicts contain a binary file' do
merge_request = create_merge_request('conflict-binary-file')
expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_falsey
end
it 'returns a falsey value when the conflicts contain a file edited in one branch and deleted in another' do
merge_request = create_merge_request('conflict-missing-side')
expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_falsey
end
it 'returns a truthy value when the conflicts are resolvable in the UI' do
merge_request = create_merge_request('conflict-resolvable')
expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_truthy
end
it 'returns a truthy value when the conflicts have to be resolved in an editor' do
merge_request = create_merge_request('conflict-contains-conflict-markers')
expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_truthy
end
end
describe "#source_project_missing?" do
let(:project) { create(:empty_project) }
let(:fork_project) { create(:empty_project, forked_from_project: project) }

14
spec/presenters/merge_request_presenter_spec.rb
View File

@ -73,12 +73,12 @@ describe MergeRequestPresenter do
describe '#conflict_resolution_path' do
let(:project) { create :empty_project }
let(:user) { create :user }
let(:path) { described_class.new(resource, current_user: user).conflict_resolution_path }
let(:presenter) { described_class.new(resource, current_user: user) }
let(:path) { presenter.conflict_resolution_path }
context 'when MR cannot be resolved in UI' do
it 'does not return conflict resolution path' do
allow(resource).to receive(:conflicts_can_be_resolved_in_ui?) { true }
allow(resource).to receive(:conflicts_can_be_resolved_by?).with(user) { false }
allow(presenter).to receive_message_chain(:conflicts, :can_be_resolved_in_ui?) { false }
expect(path).to be_nil
end
@ -86,8 +86,8 @@ describe MergeRequestPresenter do
context 'when conflicts cannot be resolved by user' do
it 'does not return conflict resolution path' do
allow(resource).to receive(:conflicts_can_be_resolved_in_ui?) { false }
allow(resource).to receive(:conflicts_can_be_resolved_by?).with(user) { true }
allow(presenter).to receive_message_chain(:conflicts, :can_be_resolved_in_ui?) { true }
allow(presenter).to receive_message_chain(:conflicts, :can_be_resolved_by?).with(user) { false }
expect(path).to be_nil
end
@ -95,8 +95,8 @@ describe MergeRequestPresenter do
context 'when able to access conflict resolution UI' do
it 'does return conflict resolution path' do
allow(resource).to receive(:conflicts_can_be_resolved_in_ui?) { true }
allow(resource).to receive(:conflicts_can_be_resolved_by?).with(user) { true }
allow(presenter).to receive_message_chain(:conflicts, :can_be_resolved_in_ui?) { true }
allow(presenter).to receive_message_chain(:conflicts, :can_be_resolved_by?).with(user) { true }
expect(path)
.to eq("/#{project.full_path}/merge_requests/#{resource.iid}/conflicts")

73
spec/services/merge_requests/conflicts/list_service_spec.rb Normal file
View File

@ -0,0 +1,73 @@
require 'spec_helper'
describe MergeRequests::Conflicts::ListService do
describe '#can_be_resolved_in_ui?' do
def create_merge_request(source_branch)
create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start') do |mr|
mr.mark_as_unmergeable
end
end
def conflicts_service(merge_request)
described_class.new(merge_request)
end
it 'returns a falsey value when the MR can be merged without conflicts' do
merge_request = create_merge_request('master')
merge_request.mark_as_mergeable
expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
end
it 'returns a falsey value when the MR is marked as having conflicts, but has none' do
merge_request = create_merge_request('master')
expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
end
it 'returns a falsey value when the MR has a missing ref after a force push' do
merge_request = create_merge_request('conflict-resolvable')
service = conflicts_service(merge_request)
allow(service.conflicts).to receive(:merge_index).and_raise(Rugged::OdbError)
expect(service.can_be_resolved_in_ui?).to be_falsey
end
it 'returns a falsey value when the MR does not support new diff notes' do
merge_request = create_merge_request('conflict-resolvable')
merge_request.merge_request_diff.update_attributes(start_commit_sha: nil)
expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
end
it 'returns a falsey value when the conflicts contain a large file' do
merge_request = create_merge_request('conflict-too-large')
expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
end
it 'returns a falsey value when the conflicts contain a binary file' do
merge_request = create_merge_request('conflict-binary-file')
expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
end
it 'returns a falsey value when the conflicts contain a file edited in one branch and deleted in another' do
merge_request = create_merge_request('conflict-missing-side')
expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
end
it 'returns a truthy value when the conflicts are resolvable in the UI' do
merge_request = create_merge_request('conflict-resolvable')
expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_truthy
end
it 'returns a truthy value when the conflicts have to be resolved in an editor' do
merge_request = create_merge_request('conflict-contains-conflict-markers')
expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_truthy
end
end
end

41
spec/services/merge_requests/resolve_service_spec.rb → spec/services/merge_requests/conflicts/resolve_service_spec.rb
View File

@ -1,6 +1,6 @@
require 'spec_helper'
describe MergeRequests::ResolveService do
describe MergeRequests::Conflicts::ResolveService do
let(:user) { create(:user) }
let(:project) { create(:project, :repository) }
@ -24,6 +24,8 @@ describe MergeRequests::ResolveService do
end
describe '#execute' do
let(:service) { described_class.new(merge_request) }
context 'with section params' do
let(:params) do
{
@ -50,7 +52,7 @@ describe MergeRequests::ResolveService do
context 'when the source and target project are the same' do
before do
described_class.new(project, user, params).execute(merge_request)
service.execute(user, params)
end
it 'creates a commit with the message' do
@ -74,15 +76,26 @@ describe MergeRequests::ResolveService do
branch_name: 'conflict-start')
end
before do
described_class.new(fork_project, user, params).execute(merge_request_from_fork)
def resolve_conflicts
described_class.new(merge_request_from_fork).execute(user, params)
end
it 'gets conflicts from the source project' do
expect(fork_project.repository.rugged).to receive(:merge_commits).and_call_original
expect(project.repository.rugged).not_to receive(:merge_commits)
resolve_conflicts
end
it 'creates a commit with the message' do
resolve_conflicts
expect(merge_request_from_fork.source_branch_head.message).to eq(params[:commit_message])
end
it 'creates a commit with the correct parents' do
resolve_conflicts
expect(merge_request_from_fork.source_branch_head.parents.map(&:id)).
to eq(['404fa3fc7c2c9b5dacff102f353bdf55b1be2813',
target_head])
@ -115,7 +128,7 @@ describe MergeRequests::ResolveService do
end
before do
described_class.new(project, user, params).execute(merge_request)
service.execute(user, params)
end
it 'creates a commit with the message' do
@ -154,15 +167,15 @@ describe MergeRequests::ResolveService do
}
end
let(:service) { described_class.new(project, user, invalid_params) }
it 'raises a MissingResolution error' do
expect { service.execute(merge_request) }.
expect { service.execute(user, invalid_params) }.
to raise_error(Gitlab::Conflict::File::MissingResolution)
end
end
context 'when the content of a file is unchanged' do
let(:list_service) { MergeRequests::Conflicts::ListService.new(merge_request) }
let(:invalid_params) do
{
files: [
@ -173,17 +186,15 @@ describe MergeRequests::ResolveService do
}, {
old_path: 'files/ruby/regex.rb',
new_path: 'files/ruby/regex.rb',
content: merge_request.conflicts.file_for_path('files/ruby/regex.rb', 'files/ruby/regex.rb').content
content: list_service.conflicts.file_for_path('files/ruby/regex.rb', 'files/ruby/regex.rb').content
}
],
commit_message: 'This is a commit message!'
}
end
let(:service) { described_class.new(project, user, invalid_params) }
it 'raises a MissingResolution error' do
expect { service.execute(merge_request) }.
expect { service.execute(user, invalid_params) }.
to raise_error(Gitlab::Conflict::File::MissingResolution)
end
end
@ -202,11 +213,9 @@ describe MergeRequests::ResolveService do
}
end
let(:service) { described_class.new(project, user, invalid_params) }
it 'raises a MissingFiles error' do
expect { service.execute(merge_request) }.
to raise_error(MergeRequests::ResolveService::MissingFiles)
expect { service.execute(user, invalid_params) }.
to raise_error(described_class::MissingFiles)
end
end
end