Commit ad2bfeb8 authored by Sean McGivern's avatar Sean McGivern

Fix conflict resolution from corrupted upstream

I don't know why this happens exactly, but given an upstream and fork repository
from a customer, both of which required GC, resolving conflicts would corrupt
the fork so badly that it couldn't be cloned.

This isn't a perfect fix for that case, because the MR may still need to be
merged manually, but it does ensure that the repository is at least usable.

My best guess is that when we generate the index for the conflict
resolution (which we previously did in the target project), we obtain a
reference to an OID that doesn't exist in the source, even though we already
fetch the refs from the target into the source.

Explicitly setting the source project as the place to get the merge index from
seems to prevent repository corruption in this way.
parent aec53bab
...@@ -155,8 +155,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -155,8 +155,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
format.html { define_discussion_vars } format.html { define_discussion_vars }
format.json do format.json do
if @merge_request.conflicts_can_be_resolved_in_ui? if @conflicts_list.can_be_resolved_in_ui?
render json: @merge_request.conflicts render json: @conflicts_list
elsif @merge_request.can_be_merged? elsif @merge_request.can_be_merged?
render json: { render json: {
message: 'The merge conflicts for this merge request have already been resolved. Please return to the merge request.', 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 ...@@ -173,9 +173,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end end
def conflict_for_path 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 return render_404 unless file
...@@ -183,7 +183,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -183,7 +183,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end end
def resolve_conflicts 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? if @merge_request.can_be_merged?
render status: :bad_request, json: { message: 'The merge conflicts for this merge request have already been resolved.' } 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 ...@@ -191,7 +191,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end end
begin 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.' flash[:notice] = 'All merge conflicts were resolved. The merge request can now be merged.'
...@@ -459,7 +461,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -459,7 +461,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end end
def authorize_can_resolve_conflicts! 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 end
def module_enabled def module_enabled
......
...@@ -899,34 +899,6 @@ class MergeRequest < ActiveRecord::Base ...@@ -899,34 +899,6 @@ class MergeRequest < ActiveRecord::Base
project.repository.keep_around(self.merge_commit_sha) project.repository.keep_around(self.merge_commit_sha)
end 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? def has_commits?
merge_request_diff && commits_count > 0 merge_request_diff && commits_count > 0
end end
......
...@@ -76,7 +76,7 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated ...@@ -76,7 +76,7 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
end end
def conflict_resolution_path 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) conflicts_namespace_project_merge_request_path(project.namespace, project, merge_request)
end end
end end
...@@ -141,6 +141,10 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated ...@@ -141,6 +141,10 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
private private
def conflicts
@conflicts ||= MergeRequests::Conflicts::ListService.new(merge_request)
end
def closing_issues def closing_issues
@closing_issues ||= closes_issues(current_user) @closing_issues ||= closes_issues(current_user)
end end
......
module MergeRequests
module Conflicts
class BaseService
attr_reader :merge_request
def initialize(merge_request)
@merge_request = merge_request
end
end
end
end
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
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
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
---
title: Prevent further repository corruption when resolving conflicts from a fork
where both the fork and upstream projects require housekeeping
merge_request:
author:
...@@ -3,16 +3,33 @@ module Gitlab ...@@ -3,16 +3,33 @@ module Gitlab
class FileCollection class FileCollection
ConflictSideMissing = Class.new(StandardError) 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) delegate :repository, to: :project
@merge_request = merge_request
@our_commit = merge_request.source_branch_head.raw.raw_commit class << self
@their_commit = merge_request.target_branch_head.raw.raw_commit # We can only write when getting the merge index from the source
end # 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 # We don't need to do `with_repo_branch_commit` here, because the target
merge_request.project.repository # project always fetches source refs when creating merge request diffs.
def read_only(merge_request)
new(merge_request, merge_request.target_project)
end
end end
def merge_index def merge_index
...@@ -55,6 +72,15 @@ Merge branch '#{merge_request.target_branch}' into '#{merge_request.source_branc ...@@ -55,6 +72,15 @@ Merge branch '#{merge_request.target_branch}' into '#{merge_request.source_branc
#{conflict_filenames.join("\n")} #{conflict_filenames.join("\n")}
EOM EOM
end 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 end
end end
...@@ -916,7 +916,9 @@ describe Projects::MergeRequestsController do ...@@ -916,7 +916,9 @@ describe Projects::MergeRequestsController do
end end
it 'returns the file in JSON format' do 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, expect(json_response).to include('old_path' => path,
'new_path' => path, 'new_path' => path,
...@@ -1040,11 +1042,15 @@ describe Projects::MergeRequestsController do ...@@ -1040,11 +1042,15 @@ describe Projects::MergeRequestsController do
context 'when a file has identical content to the conflict' do context 'when a file has identical content to the conflict' do
before 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 = [ resolved_files = [
{ {
'new_path' => 'files/ruby/popen.rb', 'new_path' => 'files/ruby/popen.rb',
'old_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', 'new_path' => 'files/ruby/regex.rb',
'old_path' => 'files/ruby/regex.rb', 'old_path' => 'files/ruby/regex.rb',
......
...@@ -2,7 +2,7 @@ require 'spec_helper' ...@@ -2,7 +2,7 @@ require 'spec_helper'
describe Gitlab::Conflict::FileCollection, lib: true do describe Gitlab::Conflict::FileCollection, lib: true do
let(:merge_request) { create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start') } 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 describe '#files' do
it 'returns an array of Conflict::Files' do it 'returns an array of Conflict::Files' do
......
...@@ -1310,71 +1310,6 @@ describe MergeRequest, models: true do ...@@ -1310,71 +1310,6 @@ describe MergeRequest, models: true do
end end
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 describe "#source_project_missing?" do
let(:project) { create(:empty_project) } let(:project) { create(:empty_project) }
let(:fork_project) { create(:empty_project, forked_from_project: project) } let(:fork_project) { create(:empty_project, forked_from_project: project) }
......
...@@ -73,12 +73,12 @@ describe MergeRequestPresenter do ...@@ -73,12 +73,12 @@ describe MergeRequestPresenter do
describe '#conflict_resolution_path' do describe '#conflict_resolution_path' do
let(:project) { create :empty_project } let(:project) { create :empty_project }
let(:user) { create :user } 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 context 'when MR cannot be resolved in UI' do
it 'does not return conflict resolution path' do it 'does not return conflict resolution path' do
allow(resource).to receive(:conflicts_can_be_resolved_in_ui?) { true } allow(presenter).to receive_message_chain(:conflicts, :can_be_resolved_in_ui?) { false }
allow(resource).to receive(:conflicts_can_be_resolved_by?).with(user) { false }
expect(path).to be_nil expect(path).to be_nil
end end
...@@ -86,8 +86,8 @@ describe MergeRequestPresenter do ...@@ -86,8 +86,8 @@ describe MergeRequestPresenter do
context 'when conflicts cannot be resolved by user' do context 'when conflicts cannot be resolved by user' do
it 'does not return conflict resolution path' do it 'does not return conflict resolution path' do
allow(resource).to receive(:conflicts_can_be_resolved_in_ui?) { false } allow(presenter).to receive_message_chain(: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_by?).with(user) { false }
expect(path).to be_nil expect(path).to be_nil
end end
...@@ -95,8 +95,8 @@ describe MergeRequestPresenter do ...@@ -95,8 +95,8 @@ describe MergeRequestPresenter do
context 'when able to access conflict resolution UI' do context 'when able to access conflict resolution UI' do
it 'does return conflict resolution path' do it 'does return conflict resolution path' do
allow(resource).to receive(:conflicts_can_be_resolved_in_ui?) { true } allow(presenter).to receive_message_chain(: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_by?).with(user) { true }
expect(path) expect(path)
.to eq("/#{project.full_path}/merge_requests/#{resource.iid}/conflicts") .to eq("/#{project.full_path}/merge_requests/#{resource.iid}/conflicts")
......
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
require 'spec_helper' require 'spec_helper'
describe MergeRequests::ResolveService do describe MergeRequests::Conflicts::ResolveService do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
...@@ -24,6 +24,8 @@ describe MergeRequests::ResolveService do ...@@ -24,6 +24,8 @@ describe MergeRequests::ResolveService do
end end
describe '#execute' do describe '#execute' do
let(:service) { described_class.new(merge_request) }
context 'with section params' do context 'with section params' do
let(:params) do let(:params) do
{ {
...@@ -50,7 +52,7 @@ describe MergeRequests::ResolveService do ...@@ -50,7 +52,7 @@ describe MergeRequests::ResolveService do
context 'when the source and target project are the same' do context 'when the source and target project are the same' do
before do before do
described_class.new(project, user, params).execute(merge_request) service.execute(user, params)
end end
it 'creates a commit with the message' do it 'creates a commit with the message' do
...@@ -74,15 +76,26 @@ describe MergeRequests::ResolveService do ...@@ -74,15 +76,26 @@ describe MergeRequests::ResolveService do
branch_name: 'conflict-start') branch_name: 'conflict-start')
end end
before do def resolve_conflicts
described_class.new(fork_project,