diff options
15 files changed, 274 insertions, 107 deletions
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 207fbad7856..b99ccd453b8 100755 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -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 diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index e88a86ea0f0..417a13da812 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -905,6 +905,7 @@ class MergeRequest < ActiveRecord::Base project.repository.keep_around(self.merge_commit_sha) end +<<<<<<< HEAD def conflicts @conflicts ||= Gitlab::Conflict::FileCollection.new(self) end @@ -934,6 +935,8 @@ class MergeRequest < ActiveRecord::Base end end +======= +>>>>>>> e4261fe3... Merge branch 'fix-conflict-resolution-with-corrupt-repos' into 'master'
def has_commits? merge_request_diff && commits_count > 0 end diff --git a/app/presenters/merge_request_presenter.rb b/app/presenters/merge_request_presenter.rb index 255f63db5c2..0db9e31031c 100644 --- a/app/presenters/merge_request_presenter.rb +++ b/app/presenters/merge_request_presenter.rb @@ -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 diff --git a/app/services/merge_requests/conflicts/base_service.rb b/app/services/merge_requests/conflicts/base_service.rb new file mode 100644 index 00000000000..b50875347d9 --- /dev/null +++ b/app/services/merge_requests/conflicts/base_service.rb @@ -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 diff --git a/app/services/merge_requests/conflicts/list_service.rb b/app/services/merge_requests/conflicts/list_service.rb new file mode 100644 index 00000000000..9bf82518643 --- /dev/null +++ b/app/services/merge_requests/conflicts/list_service.rb @@ -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 diff --git a/app/services/merge_requests/conflicts/resolve_service.rb b/app/services/merge_requests/conflicts/resolve_service.rb new file mode 100644 index 00000000000..d74a82effd6 --- /dev/null +++ b/app/services/merge_requests/conflicts/resolve_service.rb @@ -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 diff --git a/app/services/merge_requests/resolve_service.rb b/app/services/merge_requests/resolve_service.rb deleted file mode 100644 index 82cd89d9a0b..00000000000 --- a/app/services/merge_requests/resolve_service.rb +++ /dev/null @@ -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 diff --git a/changelogs/unreleased/fix-conflict-resolution-with-corrupt-repos.yml b/changelogs/unreleased/fix-conflict-resolution-with-corrupt-repos.yml new file mode 100644 index 00000000000..19a3c56e478 --- /dev/null +++ b/changelogs/unreleased/fix-conflict-resolution-with-corrupt-repos.yml @@ -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: diff --git a/lib/gitlab/conflict/file_collection.rb b/lib/gitlab/conflict/file_collection.rb index 990b719ecfd..6e73361cad1 100644 --- a/lib/gitlab/conflict/file_collection.rb +++ b/lib/gitlab/conflict/file_collection.rb @@ -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 + + 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 - def repository - merge_request.project.repository + # 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 diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 964246b580c..fdee8c6e970 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -915,7 +915,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, @@ -1039,11 +1041,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', diff --git a/spec/lib/gitlab/conflict/file_collection_spec.rb b/spec/lib/gitlab/conflict/file_collection_spec.rb index 39d892c18c0..27f23ea70dc 100644 --- a/spec/lib/gitlab/conflict/file_collection_spec.rb +++ b/spec/lib/gitlab/conflict/file_collection_spec.rb @@ -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 diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index a6fd89c8e5d..bbdf758cc61 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1315,6 +1315,7 @@ describe MergeRequest, models: true do end end +<<<<<<< HEAD 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| @@ -1387,6 +1388,8 @@ describe MergeRequest, models: true do end end +======= +>>>>>>> e4261fe3... Merge branch 'fix-conflict-resolution-with-corrupt-repos' into 'master'
describe "#source_project_missing?" do let(:project) { create(:empty_project) } let(:fork_project) { create(:empty_project, forked_from_project: project) } diff --git a/spec/presenters/merge_request_presenter_spec.rb b/spec/presenters/merge_request_presenter_spec.rb index e599ddaf943..44720fc4448 100644 --- a/spec/presenters/merge_request_presenter_spec.rb +++ b/spec/presenters/merge_request_presenter_spec.rb @@ -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") diff --git a/spec/services/merge_requests/conflicts/list_service_spec.rb b/spec/services/merge_requests/conflicts/list_service_spec.rb new file mode 100644 index 00000000000..e8a305d6130 --- /dev/null +++ b/spec/services/merge_requests/conflicts/list_service_spec.rb @@ -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 diff --git a/spec/services/merge_requests/resolve_service_spec.rb b/spec/services/merge_requests/conflicts/resolve_service_spec.rb index 3afd6b92900..19e8d5cc5f1 100644 --- a/spec/services/merge_requests/resolve_service_spec.rb +++ b/spec/services/merge_requests/conflicts/resolve_service_spec.rb @@ -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 |