Merge branch 'recreate-all-diffs-on-import' into 'master'

Force to recreate all diffs on import

Closes #59353

See merge request gitlab-org/gitlab-ce!26480

changelogs/unreleased/recreate-all-diffs-on-import.yml

+---
+title: Force to recreate all MR diffs on import
+merge_request: 26480
+author:
+type: fixed

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

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
-
-      importer.insert_git_data(mr, exists)
+      mr = insert_git_data
 
-      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