diff --git a/changelogs/unreleased/recreate-all-diffs-on-import.yml b/changelogs/unreleased/recreate-all-diffs-on-import.yml new file mode 100644 index 00000000000..fd9124372f3 --- /dev/null +++ b/changelogs/unreleased/recreate-all-diffs-on-import.yml @@ -0,0 +1,5 @@ +--- +title: Force to recreate all MR diffs on import +merge_request: 26480 +author: +type: fixed diff --git a/lib/gitlab/import/merge_request_helpers.rb b/lib/gitlab/import/merge_request_helpers.rb index fa3ff6c3f12..b3fe1fc0685 100644 --- a/lib/gitlab/import/merge_request_helpers.rb +++ b/lib/gitlab/import/merge_request_helpers.rb @@ -38,7 +38,6 @@ module Gitlab end # rubocop: enable CodeReuse/ActiveRecord - # rubocop: disable CodeReuse/ActiveRecord def insert_or_replace_git_data(merge_request, source_branch_sha, target_branch_sha, already_exists = false) # These fields are set so we can create the correct merge request # diffs. @@ -47,24 +46,21 @@ module Gitlab merge_request.keep_around_commit + # We force to recreate all diffs to replace all existing data + # We use `.all` as otherwise `dependent: :nullify` (the default) + # takes an effect + merge_request.merge_request_diffs.all.delete_all if already_exists + # MR diffs normally use an "after_save" hook to pull data from Git. # All of this happens in the transaction started by calling # create/save/etc. This in turn can lead to these transactions being # held open for much longer than necessary. To work around this we # first save the diff, then populate it. - diff = - if already_exists - merge_request.merge_request_diffs.take || - merge_request.merge_request_diffs.build - else - merge_request.merge_request_diffs.build - end - + diff = merge_request.merge_request_diffs.build diff.importing = true diff.save diff.save_git_content end - # rubocop: enable CodeReuse/ActiveRecord end end end diff --git a/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb b/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb index 680de47de2b..2e4a7c36fb8 100644 --- a/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb @@ -11,6 +11,7 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi let(:source_commit) { project.repository.commit('feature') } let(:target_commit) { project.repository.commit('master') } let(:milestone) { create(:milestone, project: project) } + let(:state) { :closed } let(:pull_request) do alice = Gitlab::GithubImport::Representation::User.new(id: 4, login: 'alice') @@ -26,13 +27,13 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi source_repository_id: 400, target_repository_id: 200, source_repository_owner: 'alice', - state: :closed, + state: state, milestone_number: milestone.iid, author: alice, assignee: alice, created_at: created_at, updated_at: updated_at, - merged_at: merged_at + merged_at: state == :closed && merged_at ) end @@ -260,58 +261,63 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi end it 'does not create the source branch if merge request is merged' do - mr, exists = importer.create_merge_request - - importer.insert_git_data(mr, exists) + mr = insert_git_data expect(project.repository.branch_exists?(mr.source_branch)).to be_falsey expect(project.repository.branch_exists?(mr.target_branch)).to be_truthy end - it 'creates the source branch if merge request is open' do - mr, exists = importer.create_merge_request - mr.state = 'opened' - mr.save + context 'when merge request is open' do + let(:state) { :opened } - # Ensure the project creator is creating the branches because the - # merge request author may not have access to push to this - # repository. The project owner may also be a group. - allow(project.repository).to receive(:add_branch).with(project.creator, anything, anything).and_call_original + it 'creates the source branch' do + # Ensure the project creator is creating the branches because the + # merge request author may not have access to push to this + # repository. The project owner may also be a group. + allow(project.repository).to receive(:add_branch).with(project.creator, anything, anything).and_call_original - importer.insert_git_data(mr, exists) + mr = insert_git_data - expect(project.repository.branch_exists?(mr.source_branch)).to be_truthy - expect(project.repository.branch_exists?(mr.target_branch)).to be_truthy - end + expect(project.repository.branch_exists?(mr.source_branch)).to be_truthy + expect(project.repository.branch_exists?(mr.target_branch)).to be_truthy + end - it 'ignores Git errors when creating a branch' do - mr, exists = importer.create_merge_request - mr.state = 'opened' - mr.save + it 'is able to retry on pre-receive errors' do + expect(importer).to receive(:insert_or_replace_git_data).twice.and_call_original + expect(project.repository).to receive(:add_branch).and_raise('exception') - expect(project.repository).to receive(:add_branch).and_raise(Gitlab::Git::CommandError) - expect(Gitlab::Sentry).to receive(:track_acceptable_exception).and_call_original + expect { insert_git_data }.to raise_error('exception') - importer.insert_git_data(mr, exists) + expect(project.repository).to receive(:add_branch).with(project.creator, anything, anything).and_call_original - expect(project.repository.branch_exists?(mr.source_branch)).to be_falsey - expect(project.repository.branch_exists?(mr.target_branch)).to be_truthy + mr = insert_git_data + + expect(project.repository.branch_exists?(mr.source_branch)).to be_truthy + expect(project.repository.branch_exists?(mr.target_branch)).to be_truthy + expect(mr.merge_request_diffs).to be_one + end + + it 'ignores Git command errors when creating a branch' do + expect(project.repository).to receive(:add_branch).and_raise(Gitlab::Git::CommandError) + expect(Gitlab::Sentry).to receive(:track_acceptable_exception).and_call_original + + mr = insert_git_data + + expect(project.repository.branch_exists?(mr.source_branch)).to be_falsey + expect(project.repository.branch_exists?(mr.target_branch)).to be_truthy + end end it 'creates the merge request diffs' do - mr, exists = importer.create_merge_request - - importer.insert_git_data(mr, exists) + mr = insert_git_data expect(mr.merge_request_diffs.exists?).to eq(true) end it 'creates the merge request diff commits' do - mr, exists = importer.create_merge_request + mr = insert_git_data - importer.insert_git_data(mr, exists) - - diff = mr.merge_request_diffs.take + diff = mr.merge_request_diffs.reload.first expect(diff.merge_request_diff_commits.exists?).to eq(true) end @@ -327,5 +333,11 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi expect(mr.merge_request_diffs.exists?).to eq(true) end end + + def insert_git_data + mr, exists = importer.create_merge_request + importer.insert_git_data(mr, exists) + mr + end end end