Automatic merge of gitlab-org/gitlab-ce master

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

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

doc/articles/artifactory_and_gitlab/index.md

+---
+redirect_to: '../../ci/examples/artifactory_and_gitlab/index.md'
+---
+
 This document was moved to [another location](../../ci/examples/artifactory_and_gitlab/index.md)

doc/articles/how_to_configure_ldap_gitlab_ce/index.md

+---
+redirect_to: '../../administration/auth/how_to_configure_ldap_gitlab_ce/index.md'
+---
+
 This document was moved to [another location](../../administration/auth/how_to_configure_ldap_gitlab_ce/index.md).

doc/articles/how_to_install_git/index.md

+---
+redirect_to: '../../topics/git/how_to_install_git/index.md'
+---
+
 This document was moved to [another location](../../topics/git/how_to_install_git/index.md).

doc/articles/laravel_with_gitlab_and_envoy/index.md

+---
+redirect_to: '../../ci/examples/laravel_with_gitlab_and_envoy/index.md'
+---
+
 This document was moved to [another location](../../ci/examples/laravel_with_gitlab_and_envoy/index.md).

doc/articles/numerous_undo_possibilities_in_git/index.md

+---
+redirect_to: '../../topics/git/numerous_undo_possibilities_in_git/index.md'
+---
+
 This document was moved to [another location](../../topics/git/numerous_undo_possibilities_in_git/index.md).

doc/articles/runner_autoscale_aws/index.md

+---
+redirect_to: 'https://docs.gitlab.com/runner/configuration/runner_autoscale_aws/index.html'
+---
+
 This document was moved to [another location](https://docs.gitlab.com/runner/configuration/runner_autoscale_aws/index.html).

doc/gitlab-basics/create-group.md

+---
+redirect_to: '../user/group/index.md#create-a-new-group'
+---
 
 This document was moved to [another location](../user/group/index.md#create-a-new-group).

doc/gitlab-basics/create-issue.md

+---
+redirect_to: '../user/project/issues/index.md#new-issue'
+---
 
 This document was moved to [another location](../user/project/issues/index.md#new-issue).

doc/workflow/add-user/add-user.md

+---
+redirect_to: '../../user/project/members/index.md'
+---
+
 This document was moved to [../../user/project/members/index.md](../../user/project/members/index.md)

doc/workflow/authorization_for_merge_requests.md

+---
+redirect_to: '../user/project/merge_requests/authorization_for_merge_requests.md'
+---
+
 This document was moved to [user/project/merge_requests/authorization_for_merge_requests](../user/project/merge_requests/authorization_for_merge_requests.md)

doc/workflow/award_emoji.md

+---
+redirect_to: '../user/award_emojis.md'
+---
+
 This document was moved to [another location](../user/award_emojis.md).

doc/workflow/cherry_pick_changes.md

-This document was moved to [user/project/merge_requests/cherry_pick_changes](../user/project/merge_requests/cherry_pick_changes.md).
+---
+redirect_to: '../user/project/merge_requests/cherry_pick_changes.md'
+---
+
+This document was moved to [another location](../user/project/merge_requests/cherry_pick_changes.md).

doc/workflow/importing/README.md

+---
+redirect_to: '../../user/project/import/index.md'
+---
+
 This document was moved to [another location](../../user/project/import/index.md).

doc/workflow/importing/import_projects_from_bitbucket.md

+---
+redirect_to: '../../user/project/import/bitbucket.md'
+---
+
 This document was moved to [another location](../../user/project/import/bitbucket.md).

doc/workflow/importing/import_projects_from_fogbugz.md

+---
+redirect_to: '../../user/project/import/fogbugz.md'
+---
+
 This document was moved to [another location](../../user/project/import/fogbugz.md).

doc/workflow/importing/import_projects_from_gitea.md

+---
+redirect_to: '../../user/project/import/gitea.md'
+---
+
 This document was moved to [another location](../../user/project/import/gitea.md).

doc/workflow/importing/import_projects_from_github.md

+---
+redirect_to: '../../user/project/import/github.md'
+---
+
 This document was moved to [another location](../../user/project/import/github.md).

doc/workflow/importing/import_projects_from_gitlab_com.md

+---
+redirect_to: '../../user/project/import/gitlab_com.md'
+---
+
 This document was moved to [another location](../../user/project/import/gitlab_com.md).

doc/workflow/importing/migrating_from_svn.md

+---
+redirect_to: '../../user/project/import/svn.md'
+---
+
 This document was moved to [another location](../../user/project/import/svn.md).

doc/workflow/labels.md

+---
+redirect_to: '../user/project/labels.md'
+---
+
 # Labels
 
 This document was moved to [user/project/labels.md](../user/project/labels.md).

doc/workflow/merge_requests.md

+---
+redirect_to: '../user/project/merge_requests/index.md'
+---
+
 This document was moved to [user/project/merge_requests/index.md](../user/project/merge_requests/index.md).

doc/workflow/merge_when_build_succeeds.md

+---
+redirect_to: '../user/project/merge_requests/merge_when_pipeline_succeeds.md'
+---
+
 This document was moved to [merge_when_pipeline_succeeds](../user/project/merge_requests/merge_when_pipeline_succeeds.md).

doc/workflow/milestones.md

+---
+redirect_to: '../user/project/milestones/index.md'
+---
+
 This document was moved to [another location](../user/project/milestones/index.md).

doc/workflow/project_features.md

+---
+redirect_to: '../user/project/index.md'
+---
+
 This document was moved to [../user/project/index.md](../user/project/index.md)

doc/workflow/protected_branches.md

+---
+redirect_to: '../user/project/protected_branches.md'
+---
+
 This document was moved to [another location](../user/project/protected_branches.md).

doc/workflow/revert_changes.md

+---
+redirect_to: '../user/project/merge_requests/revert_changes.md'
+---
+
 This document was moved to [user/project/merge_requests/revert_changes](../user/project/merge_requests/revert_changes.md).

doc/workflow/share_projects_with_other_groups.md

+---
+redirect_to: '../user/project/members/share_project_with_groups.md'
+---
+
 This document was moved to [../user/project/members/share_project_with_groups.md](../user/project/members/share_project_with_groups.md)

doc/workflow/share_with_group.md

+---
+redirect_to: '../user/project/members/share_project_with_groups.md'
+---
+
 This document was moved to [../user/project/members/share_project_with_groups.md](../user/project/members/share_project_with_groups.md)

doc/workflow/web_editor.md

+---
+redirect_to: '../user/project/repository/web_editor.md'
+---
+
 This document was moved to [user/project/repository/web_editor](../user/project/repository/web_editor.md).

doc/workflow/wip_merge_requests.md

+---
+redirect_to: '../user/project/merge_requests/work_in_progress_merge_requests.md'
+---
+
 This document was moved to [user/project/merge_requests/work_in_progress_merge_requests](../user/project/merge_requests/work_in_progress_merge_requests.md).

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