Revert "Merge branch 'yorick/clean-up-merge-request-diff-commits-migration' into 'master'"

This reverts merge request !72219

app/models/merge_request_diff_commit.rb

@@ -6,11 +6,6 @@ class MergeRequestDiffCommit < ApplicationRecord
   include BulkInsertSafe
   include ShaAttribute
   include CachedCommit
-  include IgnorableColumns
-
-  ignore_column %i[author_name author_email committer_name committer_email],
-    remove_with: '14.6',
-    remove_after: '2021-11-22'
 
   belongs_to :merge_request_diff
 
@@ -56,14 +51,9 @@ class MergeRequestDiffCommit < ApplicationRecord
       committer =
         users[[commit_hash[:committer_name], commit_hash[:committer_email]]]
 
-      # These fields are only used to determine the author/committer IDs, we
-      # don't store them in the DB.
-      commit_hash = commit_hash
-        .except(:author_name, :author_email, :committer_name, :committer_email)
-
       commit_hash.merge(
-        commit_author_id: author.id,
-        committer_id: committer.id,
+        commit_author_id: author&.id,
+        committer_id: committer&.id,
         merge_request_diff_id: merge_request_diff_id,
         relative_order: index,
         sha: Gitlab::Database::ShaAttribute.serialize(sha), # rubocop:disable Cop/ActiveRecordSerialize
@@ -114,18 +104,18 @@ class MergeRequestDiffCommit < ApplicationRecord
   end
 
   def author_name
-    commit_author.name
+    commit_author_id ? commit_author.name : super
   end
 
   def author_email
-    commit_author.email
+    commit_author_id ? commit_author.email : super
   end
 
   def committer_name
-    committer.name
+    committer_id ? committer.name : super
   end
 
   def committer_email
-    committer.email
+    committer_id ? committer.email : super
   end
 end

db/migrate/20211012134316_clean_up_migrate_merge_request_diff_commit_users.rb

-# frozen_string_literal: true
-
-class CleanUpMigrateMergeRequestDiffCommitUsers < Gitlab::Database::Migration[1.0]
-  def up
-    jobs = Gitlab::Database::BackgroundMigrationJob
-      .for_migration_class('MigrateMergeRequestDiffCommitUsers')
-      .pending
-      .to_a
-
-    return if jobs.empty?
-
-    say("#{jobs.length} MigrateMergeRequestDiffCommitUsers are still pending")
-
-    # Normally we don't process background migrations in a regular migration, as
-    # this could take a while to complete and thus block a deployment.
-    #
-    # In this case the jobs have all been processed for GitLab.com at the time
-    # of writing. In addition, it's been a few releases since this migration was
-    # introduced. As a result, self-hosted instances should have their
-    # migrations finished a long time ago.
-    #
-    # For these reasons we clean up any pending jobs (just in case) before
-    # deploying the code. This also allows us to immediately start using the new
-    # setup only, instead of having to support both the old and new approach for
-    # at least one more release.
-    jobs.each do |job|
-      Gitlab::BackgroundMigration::MigrateMergeRequestDiffCommitUsers
-        .new
-        .perform(*job.arguments)
-    end
-  end
-
-  def down
-  end
-end

db/post_migrate/20211012143815_remove_merge_request_diff_commit_columns.rb

-# frozen_string_literal: true
-
-class RemoveMergeRequestDiffCommitColumns < Gitlab::Database::Migration[1.0]
-  enable_lock_retries!
-
-  COLUMNS = %i[author_name author_email committer_name committer_email].freeze
-
-  def change
-    COLUMNS.each do |column|
-      remove_column(:merge_request_diff_commits, column, :text)
-    end
-  end
-end

db/schema_migrations/20211012134316

-0f2578f0266154ad2790cc808233c71566b3a3ea87c40909feba9ccc5872927c
\ No newline at end of file

db/schema_migrations/20211012143815

-2685a534728ab1a50acb49a7a5ac7d9285fdc36ec3610b93a4219e6687c22b06
\ No newline at end of file

db/structure.sql

@@ -15837,6 +15837,10 @@ CREATE TABLE merge_request_diff_commits (
     merge_request_diff_id integer NOT NULL,
     relative_order integer NOT NULL,
     sha bytea NOT NULL,
+    author_name text,
+    author_email text,
+    committer_name text,
+    committer_email text,
     message text,
     trailers jsonb DEFAULT '{}'::jsonb NOT NULL,
     commit_author_id bigint,

lib/gitlab/import_export/project/import_export.yml

@@ -399,10 +399,6 @@ excluded_attributes:
     - :verification_checksum
     - :verification_failure
   merge_request_diff_commits:
-    - :author_name
-    - :author_email
-    - :committer_name
-    - :committer_email
     - :merge_request_diff_id
     - :commit_author_id
     - :committer_id

spec/lib/gitlab/background_migration/migrate_merge_request_diff_commit_users_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::BackgroundMigration::MigrateMergeRequestDiffCommitUsers, schema: 20211012134316 do
+RSpec.describe Gitlab::BackgroundMigration::MigrateMergeRequestDiffCommitUsers do
   let(:namespaces) { table(:namespaces) }
   let(:projects) { table(:projects) }
   let(:users) { table(:users) }

spec/lib/gitlab/background_migration/steal_migrate_merge_request_diff_commit_users_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::BackgroundMigration::StealMigrateMergeRequestDiffCommitUsers, schema: 20211012134316 do
+RSpec.describe Gitlab::BackgroundMigration::StealMigrateMergeRequestDiffCommitUsers do
   let(:migration) { described_class.new }
 
   describe '#perform' do

spec/lib/gitlab/import_export/project/tree_restorer_spec.rb

@@ -23,7 +23,7 @@ RSpec.describe Gitlab::ImportExport::Project::TreeRestorer do
         ]
 
         RSpec::Mocks.with_temporary_scope do
-          @project = create(:project, :repository, :builds_enabled, :issues_disabled, name: 'project', path: 'project')
+          @project = create(:project, :builds_enabled, :issues_disabled, name: 'project', path: 'project')
           @shared = @project.import_export_shared
 
           stub_all_feature_flags
@@ -36,6 +36,7 @@ RSpec.describe Gitlab::ImportExport::Project::TreeRestorer do
           allow_any_instance_of(Gitlab::Git::Repository).to receive(:branch_exists?).and_return(false)
 
           expect(@shared).not_to receive(:error)
+          expect_any_instance_of(Gitlab::Git::Repository).to receive(:create_branch).with('feature', 'DCBA')
           allow_any_instance_of(Gitlab::Git::Repository).to receive(:create_branch)
 
           project_tree_restorer = described_class.new(user: @user, shared: @shared, project: @project)

spec/migrations/20211012134316_clean_up_migrate_merge_request_diff_commit_users_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-require_migration! 'clean_up_migrate_merge_request_diff_commit_users'
-
-RSpec.describe CleanUpMigrateMergeRequestDiffCommitUsers, :migration do
-  describe '#up' do
-    context 'when there are pending jobs' do
-      it 'processes the jobs immediately' do
-        Gitlab::Database::BackgroundMigrationJob.create!(
-          class_name: 'MigrateMergeRequestDiffCommitUsers',
-          status: :pending,
-          arguments: [10, 20]
-        )
-
-        spy = Gitlab::BackgroundMigration::MigrateMergeRequestDiffCommitUsers
-        migration = described_class.new
-
-        allow(Gitlab::BackgroundMigration::MigrateMergeRequestDiffCommitUsers)
-          .to receive(:new)
-          .and_return(spy)
-
-        expect(migration).to receive(:say)
-        expect(spy).to receive(:perform).with(10, 20)
-
-        migration.up
-      end
-    end
-
-    context 'when all jobs are completed' do
-      it 'does nothing' do
-        Gitlab::Database::BackgroundMigrationJob.create!(
-          class_name: 'MigrateMergeRequestDiffCommitUsers',
-          status: :succeeded,
-          arguments: [10, 20]
-        )
-
-        migration = described_class.new
-
-        expect(migration).not_to receive(:say)
-        expect(Gitlab::BackgroundMigration::MigrateMergeRequestDiffCommitUsers)
-          .not_to receive(:new)
-
-        migration.up
-      end
-    end
-  end
-end

spec/models/merge_request_diff_commit_spec.rb

@@ -46,7 +46,11 @@ RSpec.describe MergeRequestDiffCommit do
         {
           "message": "Add submodule from gitlab.com\n\nSigned-off-by: Dmitriy Zaporozhets \u003cdmitriy.zaporozhets@gmail.com\u003e\n",
           "authored_date": "2014-02-27T10:01:38.000+01:00".to_time,
+          "author_name": "Dmitriy Zaporozhets",
+          "author_email": "dmitriy.zaporozhets@gmail.com",
           "committed_date": "2014-02-27T10:01:38.000+01:00".to_time,
+          "committer_name": "Dmitriy Zaporozhets",
+          "committer_email": "dmitriy.zaporozhets@gmail.com",
           "commit_author_id": an_instance_of(Integer),
           "committer_id": an_instance_of(Integer),
           "merge_request_diff_id": merge_request_diff_id,
@@ -57,7 +61,11 @@ RSpec.describe MergeRequestDiffCommit do
         {
           "message": "Change some files\n\nSigned-off-by: Dmitriy Zaporozhets \u003cdmitriy.zaporozhets@gmail.com\u003e\n",
           "authored_date": "2014-02-27T09:57:31.000+01:00".to_time,
+          "author_name": "Dmitriy Zaporozhets",
+          "author_email": "dmitriy.zaporozhets@gmail.com",
           "committed_date": "2014-02-27T09:57:31.000+01:00".to_time,
+          "committer_name": "Dmitriy Zaporozhets",
+          "committer_email": "dmitriy.zaporozhets@gmail.com",
           "commit_author_id": an_instance_of(Integer),
           "committer_id": an_instance_of(Integer),
           "merge_request_diff_id": merge_request_diff_id,
@@ -103,7 +111,11 @@ RSpec.describe MergeRequestDiffCommit do
         [{
           "message": "Weird commit date\n",
           "authored_date": timestamp,
+          "author_name": "Alejandro Rodríguez",
+          "author_email": "alejorro70@gmail.com",
           "committed_date": timestamp,
+          "committer_name": "Alejandro Rodríguez",
+          "committer_email": "alejorro70@gmail.com",
           "commit_author_id": an_instance_of(Integer),
           "committer_id": an_instance_of(Integer),
           "merge_request_diff_id": merge_request_diff_id,