Merge branch 'feature/add-arbitrary-commit-api-backend' into 'master'

Add only backend API for view arbitrary commit in merge request - issue #62817

See merge request gitlab-org/gitlab!23701

app/controllers/projects/merge_requests/diffs_controller.rb

@@ -64,6 +64,10 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
 
     options = additional_attributes.merge(diff_view: diff_view)
 
+    if @merge_request.project.context_commits_enabled?
+      options[:context_commits] = @merge_request.context_commits
+    end
+
     render json: DiffsSerializer.new(request).represent(diffs, options)
   end
 

app/controllers/projects/merge_requests_controller.rb

@@ -116,6 +116,15 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     }
   end
 
+  def context_commits
+    return render_404 unless project.context_commits_enabled?
+
+    # Get commits from repository
+    # or from cache if already merged
+    commits = ContextCommitsFinder.new(project, @merge_request, { search: params[:search], limit: params[:limit], offset: params[:offset] }).execute
+    render json: CommitEntity.represent(commits, { type: :full, request: merge_request })
+  end
+
   def test_reports
     reports_response(@merge_request.compare_test_reports)
   end

app/finders/context_commits_finder.rb

+# frozen_string_literal: true
+
+class ContextCommitsFinder
+  def initialize(project, merge_request, params = {})
+    @project = project
+    @merge_request = merge_request
+    @search = params[:search]
+    @limit = (params[:limit] || 40).to_i
+    @offset = (params[:offset] || 0).to_i
+  end
+
+  def execute
+    commits = init_collection
+    commits = filter_existing_commits(commits)
+
+    commits
+  end
+
+  private
+
+  attr_reader :project, :merge_request, :search, :limit, :offset
+
+  def init_collection
+    commits =
+      if search.present?
+        search_commits
+      else
+        project.repository.commits(merge_request.source_branch, { limit: limit, offset: offset })
+      end
+
+    commits
+  end
+
+  def filter_existing_commits(commits)
+    commits.select! { |commit| already_included_ids.exclude?(commit.id) }
+
+    commits
+  end
+
+  def search_commits
+    key = search.strip
+    commits = []
+    if Commit.valid_hash?(key)
+      mr_existing_commits_ids = merge_request.commits.map(&:id)
+      if mr_existing_commits_ids.exclude? key
+        commit_by_sha = project.repository.commit(key)
+        commits = [commit_by_sha] if commit_by_sha
+      end
+    else
+      commits = project.repository.find_commits_by_message(search, nil, nil, 20)
+    end
+
+    commits
+  end
+
+  def already_included_ids
+    mr_existing_commits_ids = merge_request.commits.map(&:id)
+    mr_context_commits_ids = merge_request.context_commits.map(&:id)
+
+    mr_existing_commits_ids + mr_context_commits_ids
+  end
+end

app/helpers/diff_helper.rb

@@ -29,6 +29,8 @@ module DiffHelper
     if action_name == 'diff_for_path'
       options[:expanded] = true
       options[:paths] = params.values_at(:old_path, :new_path)
+    elsif action_name == 'show'
+      options[:include_context_commits] = true unless @project.context_commits_enabled?
     end
 
     options

app/models/concerns/cached_commit.rb

+# frozen_string_literal: true
+
+module CachedCommit
+  extend ActiveSupport::Concern
+
+  def to_hash
+    Gitlab::Git::Commit::SERIALIZE_KEYS.each_with_object({}) do |key, hash|
+      hash[key] = public_send(key) # rubocop:disable GitlabSecurity/PublicSend
+    end
+  end
+
+  # We don't save these, because they would need a table or a serialised
+  # field. They aren't used anywhere, so just pretend the commit has no parents.
+  def parent_ids
+    []
+  end
+end

app/models/merge_request.rb

@@ -34,6 +34,8 @@ class MergeRequest < ApplicationRecord
   has_internal_id :iid, scope: :target_project, track_if: -> { !importing? }, init: ->(s) { s&.target_project&.merge_requests&.maximum(:iid) }
 
   has_many :merge_request_diffs
+  has_many :merge_request_context_commits
+  has_many :merge_request_context_commit_diff_files, through: :merge_request_context_commits, source: :diff_files
 
   has_many :merge_request_milestones
   has_many :milestones, through: :merge_request_milestones
@@ -399,6 +401,10 @@ class MergeRequest < ApplicationRecord
     "#{project.to_reference_base(from, full: full)}#{reference}"
   end
 
+  def context_commits
+    @context_commits ||= merge_request_context_commits.map(&:to_commit)
+  end
+
   def commits(limit: nil)
     return merge_request_diff.commits(limit: limit) if persisted?
 

app/models/merge_request_context_commit.rb

+# frozen_string_literal: true
+
+class MergeRequestContextCommit < ApplicationRecord
+  include CachedCommit
+  include ShaAttribute
+
+  belongs_to :merge_request
+  has_many :diff_files, class_name: 'MergeRequestContextCommitDiffFile'
+
+  sha_attribute :sha
+
+  validates :sha, presence: true
+  validates :sha, uniqueness: { message: 'has already been added' }
+
+  # delete all MergeRequestContextCommit & MergeRequestContextCommitDiffFile for given merge_request & commit SHAs
+  def self.delete_bulk(merge_request, commits)
+    commit_ids = commits.map(&:sha)
+    merge_request.merge_request_context_commits.where(sha: commit_ids).delete_all
+  end
+
+  # create MergeRequestContextCommit by given commit sha and it's diff file record
+  def self.bulk_insert(*args)
+    Gitlab::Database.bulk_insert('merge_request_context_commits', *args)
+  end
+
+  def to_commit
+    # Here we are storing the commit sha because to_hash removes the sha parameter and we lose
+    # the reference, this happens because we are storing the ID in db and the Commit class replaces
+    # id with sha and removes it, so in our case it will be some incremented integer which is not
+    # what we want
+    commit_hash = attributes.except('id').to_hash
+    commit_hash['id'] = sha
+    Commit.from_hash(commit_hash, merge_request.target_project)
+  end
+end

app/models/merge_request_context_commit_diff_file.rb

+# frozen_string_literal: true
+
+class MergeRequestContextCommitDiffFile < ApplicationRecord
+  include Gitlab::EncodingHelper
+  include ShaAttribute
+  include DiffFile
+
+  belongs_to :merge_request_context_commit, inverse_of: :diff_files
+
+  sha_attribute :sha
+  alias_attribute :id, :sha
+
+  # create MergeRequestContextCommitDiffFile by given diff file record(s)
+  def self.bulk_insert(*args)
+    Gitlab::Database.bulk_insert('merge_request_context_commit_diff_files', *args)
+  end
+end

app/models/merge_request_diff.rb

@@ -560,6 +560,10 @@ class MergeRequestDiff < ApplicationRecord
     opening_external_diff do
       collection = merge_request_diff_files
 
+      if options[:include_context_commits]
+        collection += merge_request.merge_request_context_commit_diff_files
+      end
+
       if paths = options[:paths]
         collection = collection.where('old_path IN (?) OR new_path IN (?)', paths, paths)
       end

app/models/merge_request_diff_commit.rb

@@ -2,6 +2,7 @@
 
 class MergeRequestDiffCommit < ApplicationRecord
   include ShaAttribute
+  include CachedCommit
 
   belongs_to :merge_request_diff
 
@@ -9,8 +10,6 @@ class MergeRequestDiffCommit < ApplicationRecord
   alias_attribute :id, :sha
 
   def self.create_bulk(merge_request_diff_id, commits)
-    sha_attribute = Gitlab::Database::ShaAttribute.new
-
     rows = commits.map.with_index do |commit, index|
       # See #parent_ids.
       commit_hash = commit.to_hash.except(:parent_ids)
@@ -19,7 +18,7 @@ class MergeRequestDiffCommit < ApplicationRecord
       commit_hash.merge(
         merge_request_diff_id: merge_request_diff_id,
         relative_order: index,
-        sha: sha_attribute.serialize(sha), # rubocop:disable Cop/ActiveRecordSerialize
+        sha: Gitlab::Database::ShaAttribute.serialize(sha), # rubocop:disable Cop/ActiveRecordSerialize
         authored_date: Gitlab::Database.sanitize_timestamp(commit_hash[:authored_date]),
         committed_date: Gitlab::Database.sanitize_timestamp(commit_hash[:committed_date])
       )
@@ -27,16 +26,4 @@ class MergeRequestDiffCommit < ApplicationRecord
 
     Gitlab::Database.bulk_insert(self.table_name, rows)
   end
-
-  def to_hash
-    Gitlab::Git::Commit::SERIALIZE_KEYS.each_with_object({}) do |key, hash|
-      hash[key] = public_send(key) # rubocop:disable GitlabSecurity/PublicSend
-    end
-  end
-
-  # We don't save these, because they would need a table or a serialised
-  # field. They aren't used anywhere, so just pretend the commit has no parents.
-  def parent_ids
-    []
-  end
 end

app/models/project.rb

@@ -763,6 +763,10 @@ class Project < ApplicationRecord
     Feature.enabled?(:unlink_fork_network_upon_visibility_decrease, self, default_enabled: true)
   end
 
+  def context_commits_enabled?
+    Feature.enabled?(:context_commits, default_enabled: true)
+  end
+
   def empty_repo?
     repository.empty?
   end

app/serializers/diffs_entity.rb

@@ -24,6 +24,10 @@ class DiffsEntity < Grape::Entity
     )
   end
 
+  expose :context_commits, using: API::Entities::Commit, if: -> (diffs, options) { merge_request&.project&.context_commits_enabled? } do |diffs|
+    options[:context_commits]
+  end
+
   expose :merge_request_diff, using: MergeRequestDiffEntity do |diffs|
     options[:merge_request_diff]
   end

app/services/merge_requests/add_context_service.rb

+# frozen_string_literal: true
+
+module MergeRequests
+  class AddContextService < MergeRequests::BaseService
+    def execute
+      return error("You are not allowed to access the requested resource", 403) unless current_user&.can?(:update_merge_request, merge_request)
+      return error("Context commits: #{duplicates} are already created", 400) unless duplicates.empty?
+      return error("One or more context commits' sha is not valid.", 400) if commits.size != commit_ids.size
+
+      context_commit_ids = []
+      MergeRequestContextCommit.transaction do
+        context_commit_ids = MergeRequestContextCommit.bulk_insert(context_commit_rows, return_ids: true)
+        MergeRequestContextCommitDiffFile.bulk_insert(diff_rows(context_commit_ids))
+      end
+
+      commits
+    end
+
+    private
+
+    def raw_repository
+      project.repository.raw_repository
+    end
+
+    def merge_request
+      params[:merge_request]
+    end
+
+    def commit_ids
+      params[:commits]
+    end
+
+    def commits
+      project.repository.commits_by(oids: commit_ids)
+    end
+
+    def context_commit_rows
+      @context_commit_rows ||= build_context_commit_rows(merge_request.id, commits)
+    end
+
+    def diff_rows(context_commit_ids)
+      @diff_rows ||= build_diff_rows(raw_repository, commits, context_commit_ids)
+    end
+
+    def encode_in_base64?(diff_text)
+      (diff_text.encoding == Encoding::BINARY && !diff_text.ascii_only?) ||
+        diff_text.include?("\0")
+    end
+
+    def duplicates
+      existing_oids = merge_request.merge_request_context_commits.map { |commit| commit.sha.to_s }
+      duplicate_oids = existing_oids.select do |existing_oid|
+        commit_ids.select { |commit_id| existing_oid.start_with?(commit_id) }.count > 0
+      end
+
+      duplicate_oids
+    end
+
+    def build_context_commit_rows(merge_request_id, commits)
+      commits.map.with_index do |commit, index|
+        # generate context commit information for given commit
+        commit_hash = commit.to_hash.except(:parent_ids)
+        sha = Gitlab::Database::ShaAttribute.serialize(commit_hash.delete(:id))
+        commit_hash.merge(
+          merge_request_id: merge_request_id,
+          relative_order: index,
+          sha: sha,
+          authored_date: Gitlab::Database.sanitize_timestamp(commit_hash[:authored_date]),
+          committed_date: Gitlab::Database.sanitize_timestamp(commit_hash[:committed_date])
+        )
+      end
+    end
+
+    def build_diff_rows(raw_repository, commits, context_commit_ids)
+      diff_rows = []
+      diff_order = 0
+
+      commits.flat_map.with_index do |commit, index|
+        commit_hash = commit.to_hash.except(:parent_ids)
+        sha = Gitlab::Database::ShaAttribute.serialize(commit_hash.delete(:id))
+        # generate context commit diff information for given commit
+        diffs = commit.diffs
+
+        compare = Gitlab::Git::Compare.new(
+          raw_repository,
+          diffs.diff_refs.start_sha,
+          diffs.diff_refs.head_sha
+        )
+        compare.diffs.map do |diff|
+          diff_hash = diff.to_hash.merge(
+            sha: sha,
+            binary: false,
+            merge_request_context_commit_id: context_commit_ids[index],
+            relative_order: diff_order
+          )
+
+          # Compatibility with old diffs created with Psych.
+          diff_hash.tap do |hash|
+            diff_text = hash[:diff]
+
+            if encode_in_base64?(diff_text)
+              hash[:binary] = true
+              hash[:diff] = [diff_text].pack('m0')
+            end
+          end
+
+          # Increase order for commit so when present the diffs we can use it to keep order
+          diff_order += 1
+          diff_rows << diff_hash
+        end
+      end
+
+      diff_rows
+    end
+  end
+end

config/routes/merge_requests.rb

@@ -18,6 +18,7 @@ resources :merge_requests, concerns: :awardable, except: [:new, :create, :show],
     scope constraints: ->(req) { req.format == :json }, as: :json do
       get :commits
       get :pipelines
+      get :context_commits
       get :diffs, to: 'merge_requests/diffs#show'
       get :diffs_batch, to: 'merge_requests/diffs#diffs_batch'
       get :diffs_metadata, to: 'merge_requests/diffs#diffs_metadata'

db/migrate/20190920224341_create_merge_request_context_commits_and_diffs.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class CreateMergeRequestContextCommitsAndDiffs < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def change
+    create_table :merge_request_context_commits do |t|
+      t.references :merge_request, foreign_key: { on_delete: :cascade }
+      t.datetime_with_timezone :authored_date
+      t.datetime_with_timezone :committed_date
+      t.binary :sha, null: false
+      t.integer :relative_order, null: false
+      t.text :author_name
+      t.text :author_email
+      t.text :committer_name
+      t.text :committer_email
+      t.text :message
+      t.index [:merge_request_id, :sha], unique: true, name: 'index_mr_context_commits_on_merge_request_id_and_sha'
+    end
+
+    create_table :merge_request_context_commit_diff_files, id: false do |t|
+      t.references :merge_request_context_commit, foreign_key: { on_delete: :cascade }, index: { name: "idx_mr_cc_diff_files_on_mr_cc_id" }
+      t.binary :sha, null: false
+      t.integer :relative_order, null: false
+      t.string :a_mode, null: false, limit: 255
+      t.string :b_mode, null: false, limit: 255
+      t.boolean :new_file, null: false
+      t.boolean :renamed_file, null: false
+      t.boolean :deleted_file, null: false
+      t.boolean :too_large, null: false
+      t.boolean :binary
+      t.text :new_path, null: false
+      t.text :old_path, null: false
+      t.text :diff
+      t.index [:merge_request_context_commit_id, :sha], name: 'idx_mr_cc_diff_files_on_mr_cc_id_and_sha'
+    end
+  end
+end

db/schema.rb

@@ -2433,6 +2433,37 @@ ActiveRecord::Schema.define(version: 2020_01_27_090233) do
     t.index ["blocking_merge_request_id", "blocked_merge_request_id"], name: "index_mr_blocks_on_blocking_and_blocked_mr_ids", unique: true
   end
 
+  create_table "merge_request_context_commit_diff_files", id: false, force: :cascade do |t|
+    t.binary "sha", null: false
+    t.integer "relative_order", null: false
+    t.boolean "new_file", null: false
+    t.boolean "renamed_file", null: false
+    t.boolean "deleted_file", null: false
+    t.boolean "too_large", null: false
+    t.string "a_mode", limit: 255, null: false
+    t.string "b_mode", limit: 255, null: false
+    t.text "new_path", null: false
+    t.text "old_path", null: false
+    t.text "diff"
+    t.boolean "binary"
+    t.bigint "merge_request_context_commit_id"
+    t.index ["merge_request_context_commit_id", "sha"], name: "idx_mr_cc_diff_files_on_mr_cc_id_and_sha"
+  end
+
+  create_table "merge_request_context_commits", force: :cascade do |t|
+    t.datetime_with_timezone "authored_date"
+    t.datetime_with_timezone "committed_date"
+    t.integer "relative_order", null: false
+    t.binary "sha", null: false
+    t.text "author_name"
+    t.text "author_email"
+    t.text "committer_name"
+    t.text "committer_email"
+    t.text "message"
+    t.bigint "merge_request_id"
+    t.index ["merge_request_id", "sha"], name: "index_mr_context_commits_on_merge_request_id_and_sha", unique: true
+  end
+
   create_table "merge_request_diff_commits", id: false, force: :cascade do |t|
     t.datetime "authored_date"
     t.datetime "committed_date"
@@ -4702,6 +4733,8 @@ ActiveRecord::Schema.define(version: 2020_01_27_090233) do
   add_foreign_key "merge_request_assignees", "users", on_delete: :cascade
   add_foreign_key "merge_request_blocks", "merge_requests", column: "blocked_merge_request_id", on_delete: :cascade
   add_foreign_key "merge_request_blocks", "merge_requests", column: "blocking_merge_request_id", on_delete: :cascade
+  add_foreign_key "merge_request_context_commit_diff_files", "merge_request_context_commits", on_delete: :cascade
+  add_foreign_key "merge_request_context_commits", "merge_requests", on_delete: :cascade
   add_foreign_key "merge_request_diff_commits", "merge_request_diffs", on_delete: :cascade
   add_foreign_key "merge_request_diff_files", "merge_request_diffs", on_delete: :cascade
   add_foreign_key "merge_request_diffs", "merge_requests", name: "fk_8483f3258f", on_delete: :cascade

doc/api/merge_request_context_commits.md

+# Merge request context commits  API
+
+## List MR context commits
+
+Get a list of merge request context commits.
+
+```
+GET /projects/:id/merge_requests/:merge_request_iid/context_commits
+```
+
+Parameters:
+
+- `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user
+- `merge_request_iid` (required) - The internal ID of the merge request
+
+```json
+[
+    {
+        "id": "4a24d82dbca5c11c61556f3b35ca472b7463187e",
+        "short_id": "4a24d82d",
+        "created_at": "2017-04-11T10:08:59.000Z",
+        "parent_ids": null,
+        "title": "Update README.md to include `Usage in testing and development`",
+        "message": "Update README.md to include `Usage in testing and development`",
+        "author_name": "Luke \"Jared\" Bennett",
+        "author_email": "lbennett@gitlab.com",
+        "authored_date": "2017-04-11T10:08:59.000Z",
+        "committer_name": "Luke \"Jared\" Bennett",
+        "committer_email": "lbennett@gitlab.com",
+        "committed_date": "2017-04-11T10:08:59.000Z",
+        "author": null,
+        "author_gravatar_url": "https://www.gravatar.com/avatar/2acf1fb99417a2b3971def5a294abbeb?s=80&d=identicon",
+        "commit_url": "http://127.0.0.1:3000/gitlab-org/gitlab-test/commit/4a24d82dbca5c11c61556f3b35ca472b7463187e",
+        "commit_path": "/gitlab-org/gitlab-test/commit/4a24d82dbca5c11c61556f3b35ca472b7463187e",
+        "description_html": "",
+        "title_html": "Update README.md to include `Usage in testing and development`",
+        "signature_html": null,
+        "pipeline_status_path": null
+    }
+]
+```
+
+## Create MR context commits
+
+Create a list of merge request context commits.
+
+```
+POST /projects/:id/merge_requests/:merge_request_iid/context_commits
+```
+
+Parameters:
+
+- `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user
+- `merge_request_iid` (required) - The internal ID of the merge request
+
+```
+POST /projects/:id/merge_requests/
+```
+
+| Attribute                  | Type    | Required | Description                                                                     |
+| ---------                  | ----    | -------- | -----------                                                                     |
+| `commits`             | string array | yes | The context commits' sha  |
+
+```json
+[
+    {
+        "id": "6d394385cf567f80a8fd85055db1ab4c5295806f",
+        "message": "Added contributing guide\n\nSigned-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>\n",
+        "parent_ids": [
+            "1a0b36b3cdad1d2ee32457c102a8c0b7056fa863"
+        ],
+        "authored_date": "2014-02-27T10:05:10.000+02:00",
+        "author_name": "Dmitriy Zaporozhets",
+        "author_email": "dmitriy.zaporozhets@gmail.com",
+        "committed_date": "2014-02-27T10:05:10.000+02:00",
+        "committer_name": "Dmitriy Zaporozhets",
+        "committer_email": "dmitriy.zaporozhets@gmail.com"
+    }
+]
+```
+
+## Delete MR context commits
+
+Delete a list of merge request context commits.
+
+```
+DELETE /projects/:id/merge_requests/:merge_request_iid/context_commits
+```
+
+Parameters:
+
+- `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user
+- `merge_request_iid` (required) - The internal ID of the merge request
+
+| Attribute                  | Type    | Required | Description                                                                     |
+| ---------                  | ----    | -------- | -----------                                                                     |
+| `commits`             | string array | yes | The context commits' sha  |

lib/api/entities.rb

@@ -755,6 +755,12 @@ module API
       end
     end
 
+    class MergeRequestContextCommit < Grape::Entity
+      expose :sha, :relative_order, :new_file, :renamed_file,
+        :deleted_file, :too_large, :a_mode, :b_mode, :new_path, :old_path,
+        :diff, :binary
+    end
+
     class SSHKey < Grape::Entity
       expose :id, :title, :key, :created_at
     end

lib/api/merge_requests.rb

@@ -4,6 +4,8 @@ module API
   class MergeRequests < Grape::API
     include PaginationParams
 
+    CONTEXT_COMMITS_POST_LIMIT = 20
+
     before { authenticate_non_get! }
 
     helpers ::Gitlab::IssuableMetadata
@@ -290,6 +292,72 @@ module API
         present commits, with: Entities::Commit
       end
 
+      desc 'Get the context commits of a merge request' do
+        success Entities::CommitEntity
+      end
+      get ':id/merge_requests/:merge_request_iid/context_commits' do
+        merge_request = find_merge_request_with_access(params[:merge_request_iid])
+        project = merge_request.project
+
+        not_found! unless project.context_commits_enabled?
+
+        context_commits = merge_request.context_commits
+        CommitEntity.represent(context_commits, type: :full, request: merge_request)
+      end
+
+      params do
+        requires :commits, type: Array, allow_blank: false, desc: 'List of context commits sha'
+      end
+      desc 'create context commits of merge request' do
+        success Entities::Commit
+      end
+      post ':id/merge_requests/:merge_request_iid/context_commits' do
+        commit_ids = params[:commits]
+
+        if commit_ids.size > CONTEXT_COMMITS_POST_LIMIT
+          render_api_error!("Context commits array size should not be more than #{CONTEXT_COMMITS_POST_LIMIT}", 400)
+        end
+
+        merge_request = find_merge_request_with_access(params[:merge_request_iid])
+        project = merge_request.project
+
+        not_found! unless project.context_commits_enabled?
+
+        authorize!(:update_merge_request, merge_request)
+
+        project = merge_request.target_project
+        result = ::MergeRequests::AddContextService.new(project, current_user, merge_request: merge_request, commits: commit_ids).execute
+
+        if result.instance_of?(Array)
+          present result, with: Entities::Commit
+        else
+          render_api_error!(result[:message], result[:http_status])
+        end
+      end
+
+      params do
+        requires :commits, type: Array, allow_blank: false, desc: 'List of context commits sha'
+      end
+      desc 'remove context commits of merge request'
+      delete ':id/merge_requests/:merge_request_iid/context_commits' do
+        commit_ids = params[:commits]
+        merge_request = find_merge_request_with_access(params[:merge_request_iid])
+        project = merge_request.project
+
+        not_found! unless project.context_commits_enabled?
+
+        authorize!(:destroy_merge_request, merge_request)
+        project = merge_request.target_project
+        commits = project.repository.commits_by(oids: commit_ids)
+
+        if commits.size != commit_ids.size
+          render_api_error!("One or more context commits' sha is not valid.", 400)
+        end
+
+        MergeRequestContextCommit.delete_bulk(merge_request, commits)
+        status 204
+      end
+
       desc 'Show the merge request changes' do
         success Entities::MergeRequestChanges
       end

lib/gitlab/database/sha_attribute.rb

@@ -24,7 +24,14 @@ module Gitlab
       def serialize(value)
         arg = value ? [value].pack(PACK_FORMAT) : nil
 
-        super(arg)
+        BINARY_TYPE.new.serialize(arg)
+      end
+
+      # Casts a SHA1 in hexadecimal to the proper binary format.
+      def self.serialize(value)
+        arg = value ? [value].pack(PACK_FORMAT) : nil
+
+        BINARY_TYPE.new.serialize(arg)
       end
     end
   end

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -791,6 +791,21 @@ describe Projects::MergeRequestsController do
     end
   end
 
+  describe 'GET context commits' do
+    it 'returns the commits for context commits' do
+      get :context_commits,
+        params: {
+          namespace_id: project.namespace.to_param,
+          project_id: project,
+          id: merge_request.iid
+        },
+        format: 'json'
+
+      expect(response).to have_gitlab_http_status(:success)
+      expect(json_response).to be_an Array
+    end
+  end
+
   describe 'GET exposed_artifacts' do
     let(:merge_request) do
       create(:merge_request,

spec/factories/merge_request_context_commit.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :merge_request_context_commit do
+    association :merge_request, factory: :merge_request
+    author_name { 'test' }
+    author_email { 'test@test.com' }
+    message { '' }
+    relative_order { 0 }
+    sha { Digest::SHA1.hexdigest(SecureRandom.hex) }
+  end
+end

spec/factories/merge_request_context_commit_diff_file.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :merge_request_context_commit_diff_file do
+    association :merge_request_context_commit
+
+    sha { Digest::SHA1.hexdigest(SecureRandom.hex) }
+    relative_order { 0 }
+    new_file { true }
+    renamed_file { false }
+    deleted_file { false }
+    too_large { false }
+    a_mode { 0 }
+    b_mode { 100644 }
+    new_path { 'foo' }
+    old_path { 'foo' }
+    diff { '' }
+    binary { false }
+  end
+end

spec/finders/context_commits_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ContextCommitsFinder do
+  describe "#execute" do
+    let(:project) { create(:project, :repository) }
+    let(:merge_request) { create(:merge_request) }
+    let(:commit) { create(:commit, id: '6d394385cf567f80a8fd85055db1ab4c5295806f') }
+
+    it 'filters commits by valid sha/commit message' do
+      params = { search: commit.id }
+
+      commits = described_class.new(project, merge_request, params).execute
+
+      expect(commits.length).to eq(1)
+      expect(commits[0].id).to eq(commit.id)
+    end
+
+    it 'returns nothing when searched by invalid sha/commit message' do
+      params = { search: 'zzz' }
+
+      commits = described_class.new(project, merge_request, params).execute
+
+      expect(commits).to be_empty
+    end
+  end
+end

spec/lib/gitlab/database/sha_attribute_spec.rb

@@ -25,7 +25,7 @@ describe Gitlab::Database::ShaAttribute do
 
   describe '#serialize' do
     it 'converts a SHA String to binary data' do
-      expect(attribute.serialize(sha).to_s).to eq(binary_sha)
+      expect(described_class.serialize(sha).to_s).to eq(binary_sha)
     end
   end
 end

spec/lib/gitlab/import_export/all_models.yml

@@ -125,6 +125,8 @@ merge_requests:
 - merge_user
 - merge_request_diffs
 - merge_request_diff
+- merge_request_context_commits
+- merge_request_context_commit_diff_files
 - events
 - merge_requests_closing_issues
 - cached_closes_issues
@@ -170,6 +172,9 @@ merge_request_diff_commits:
 - merge_request_diff
 merge_request_diff_files:
 - merge_request_diff
+merge_request_context_commits:
+- merge_request
+- diff_files
 ci_pipelines:
 - project
 - user

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -225,6 +225,31 @@ MergeRequestDiffFile:
 - b_mode
 - too_large
 - binary
+MergeRequestContextCommit:
+- id
+- authored_date
+- committed_date
+- relative_order
+- sha
+- author_name
+- author_email
+- committer_name
+- committer_email
+- message
+- merge_request_id
+MergeRequestContextCommitDiffFile:
+- sha
+- relative_order
+- new_file
+- renamed_file
+- deleted_file
+- new_path
+- old_path
+- a_mode
+- b_mode
+- too_large
+- binary
+- text
 MergeRequest::Metrics:
 - id
 - created_at

spec/models/merge_request_context_commit_diff_file_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequestContextCommitDiffFile do
+  describe 'associations' do
+    it { is_expected.to belong_to(:merge_request_context_commit) }
+  end
+end

spec/models/merge_request_context_commit_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequestContextCommit do
+  let(:merge_request) { create(:merge_request) }
+  let(:project) { merge_request.project }
+  let(:raw_repository) { project.repository.raw_repository }
+  let(:commits) do
+    [
+      project.commit('5937ac0a7beb003549fc5fd26fc247adbce4a52e'),
+      project.commit('570e7b2abdd848b95f2f578043fc23bd6f6fd24d')
+    ]
+  end
+
+  describe 'associations' do
+    it { is_expected.to belong_to(:merge_request) }
+    it { is_expected.to have_many(:diff_files).class_name("MergeRequestContextCommitDiffFile") }
+  end
+
+  describe '.delete_bulk' do
+    let(:context_commit1) { create(:merge_request_context_commit, merge_request: merge_request, sha: '5937ac0a7beb003549fc5fd26fc247adbce4a52e') }
+    let(:context_commit2) { create(:merge_request_context_commit, merge_request: merge_request, sha: '570e7b2abdd848b95f2f578043fc23bd6f6fd24d') }
+
+    it 'deletes context commits for given commit sha\'s and returns the commit' do
+      expect(described_class.delete_bulk(merge_request, [context_commit1, context_commit2])).to eq(2)
+    end
+
+    it 'doesn\'t delete context commits when commit sha\'s are not passed' do
+      expect(described_class.delete_bulk(merge_request, [])).to eq(0)
+    end
+  end
+end

spec/models/merge_request_diff_commit_spec.rb

@@ -18,7 +18,6 @@ describe MergeRequestDiffCommit do
   end
 
   describe '.create_bulk' do
-    let(:sha_attribute) { Gitlab::Database::ShaAttribute.new }
     let(:merge_request_diff_id) { merge_request.merge_request_diff.id }
     let(:commits) do
       [
@@ -38,7 +37,7 @@ describe MergeRequestDiffCommit do
           "committer_email": "dmitriy.zaporozhets@gmail.com",
           "merge_request_diff_id": merge_request_diff_id,
           "relative_order": 0,
-          "sha": sha_attribute.serialize("5937ac0a7beb003549fc5fd26fc247adbce4a52e")
+          "sha": Gitlab::Database::ShaAttribute.serialize("5937ac0a7beb003549fc5fd26fc247adbce4a52e")
         },
         {
           "message": "Change some files\n\nSigned-off-by: Dmitriy Zaporozhets \u003cdmitriy.zaporozhets@gmail.com\u003e\n",
@@ -50,7 +49,7 @@ describe MergeRequestDiffCommit do
           "committer_email": "dmitriy.zaporozhets@gmail.com",
           "merge_request_diff_id": merge_request_diff_id,
           "relative_order": 1,
-          "sha": sha_attribute.serialize("570e7b2abdd848b95f2f578043fc23bd6f6fd24d")
+          "sha": Gitlab::Database::ShaAttribute.serialize("570e7b2abdd848b95f2f578043fc23bd6f6fd24d")
         }
       ]
     end
@@ -81,7 +80,7 @@ describe MergeRequestDiffCommit do
           "committer_email": "alejorro70@gmail.com",
           "merge_request_diff_id": merge_request_diff_id,
           "relative_order": 0,
-          "sha": sha_attribute.serialize("ba3343bc4fa403a8dfbfcab7fc1a8c29ee34bd69")
+          "sha": Gitlab::Database::ShaAttribute.serialize("ba3343bc4fa403a8dfbfcab7fc1a8c29ee34bd69")
         }]
       end
 

spec/requests/api/merge_requests_spec.rb

@@ -12,7 +12,8 @@ describe API::MergeRequests do
   let(:project)     { create(:project, :public, :repository, creator: user, namespace: user.namespace, only_allow_merge_if_pipeline_succeeds: false) }
   let(:milestone)   { create(:milestone, title: '1.0.0', project: project) }
   let(:milestone1)  { create(:milestone, title: '0.9', project: project) }
-  let!(:merge_request) { create(:merge_request, :simple, milestone: milestone1, author: user, assignees: [user], source_project: project, target_project: project, source_branch: 'markdown', title: "Test", created_at: base_time) }
+  let(:merge_request_context_commit) {create(:merge_request_context_commit, message: 'test')}
+  let!(:merge_request) { create(:merge_request, :simple, milestone: milestone1, author: user, assignees: [user], merge_request_context_commits: [merge_request_context_commit], source_project: project, target_project: project, source_branch: 'markdown', title: "Test", created_at: base_time) }
   let!(:merge_request_closed) { create(:merge_request, state: "closed", milestone: milestone1, author: user, assignees: [user], source_project: project, target_project: project, title: "Closed test", created_at: base_time + 1.second) }
   let!(:merge_request_merged) { create(:merge_request, state: "merged", author: user, assignees: [user], source_project: project, target_project: project, title: "Merged test", created_at: base_time + 2.seconds, merge_commit_sha: '9999999999999999999999999999999999999999') }
   let!(:merge_request_locked) { create(:merge_request, state: "locked", milestone: milestone1, author: user, assignees: [user], source_project: project, target_project: project, title: "Locked test", created_at: base_time + 1.second) }
@@ -1066,6 +1067,20 @@ describe API::MergeRequests do
     end
   end
 
+  describe 'GET /projects/:id/merge_requests/:merge_request_iid/:context_commits' do
+    it 'returns a 200 when merge request is valid' do
+      get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/context_commits", user)
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response).to be_an Array
+      expect(json_response.size).to eq(merge_request.context_commits.size)
+    end
+
+    it 'returns a 404 when merge_request_iid not found' do
+      get api("/projects/#{project.id}/merge_requests/0/context_commits", user)
+      expect(response).to have_gitlab_http_status(404)
+    end
+  end
+
   describe 'GET /projects/:id/merge_requests/:merge_request_iid/changes' do
     it 'returns the change information of the merge_request' do
       get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/changes", user)
@@ -1540,6 +1555,93 @@ describe API::MergeRequests do
     end
   end
 
+  describe "POST /projects/:id/merge_requests/:merge_request_iid/context_commits" do
+    let(:merge_request_iid)  { merge_request.iid }
+    let(:authenticated_user) { user }
+    let(:commit) { project.repository.commit }
+
+    let(:params) do
+      {
+        commits: [commit.id]
+      }
+    end
+
+    let(:params_empty_commits) do
+      {
+        commits: []
+      }
+    end
+
+    let(:params_invalid_shas) do
+      {
+        commits: ['invalid']
+      }
+    end
+
+    describe 'when authenticated' do
+      it 'creates and returns the new context commit' do
+        post api("/projects/#{project.id}/merge_requests/#{merge_request_iid}/context_commits", authenticated_user), params: params
+        expect(response).to have_gitlab_http_status(201)
+        expect(json_response).to be_an Array
+        expect(json_response.first['short_id']).to eq(commit.short_id)
+        expect(json_response.first['title']).to eq(commit.title)
+        expect(json_response.first['message']).to eq(commit.message)
+        expect(json_response.first['author_name']).to eq(commit.author_name)
+        expect(json_response.first['author_email']).to eq(commit.author_email)
+        expect(json_response.first['committer_name']).to eq(commit.committer_name)
+        expect(json_response.first['committer_email']).to eq(commit.committer_email)
+      end
+
+      context 'doesnt create when its already created' do
+        before do
+          create(:merge_request_context_commit, merge_request: merge_request, sha: commit.id)
+        end
+        it 'returns 400 when the context commit is already created' do
+          post api("/projects/#{project.id}/merge_requests/#{merge_request_iid}/context_commits", authenticated_user), params: params
+          expect(response).to have_gitlab_http_status(400)
+          expect(json_response['message']).to eq("Context commits: [\"#{commit.id}\"] are already created")
+        end
+      end
+
+      it 'returns 400 when one or more shas are invalid' do
+        post api("/projects/#{project.id}/merge_requests/#{merge_request_iid}/context_commits", authenticated_user), params: params_invalid_shas
+        expect(response).to have_gitlab_http_status(400)
+        expect(json_response['message']).to eq('One or more context commits\' sha is not valid.')
+      end
+
+      it 'returns 400 when the commits are empty' do
+        post api("/projects/#{project.id}/merge_requests/#{merge_request_iid}/context_commits", authenticated_user), params: params_empty_commits
+        expect(response).to have_gitlab_http_status(400)
+      end
+
+      it 'returns 400 when params is empty' do
+        post api("/projects/#{project.id}/merge_requests/#{merge_request_iid}/context_commits", authenticated_user)
+        expect(response).to have_gitlab_http_status(400)
+      end
+
+      it 'returns 403 when creating new context commit for guest role' do
+        guest = create(:user)
+        project.add_guest(guest)
+        post api("/projects/#{project.id}/merge_requests/#{merge_request_iid}/context_commits", guest), params: params
+        expect(response).to have_gitlab_http_status(403)
+      end
+
+      it 'returns 403 when creating new context commit for reporter role' do
+        reporter = create(:user)
+        project.add_reporter(reporter)
+        post api("/projects/#{project.id}/merge_requests/#{merge_request_iid}/context_commits", reporter), params: params
+        expect(response).to have_gitlab_http_status(403)
+      end
+    end
+
+    context 'when unauthenticated' do
+      it 'returns 401 if user tries to create context commits' do
+        post api("/projects/#{project.id}/merge_requests/#{merge_request_iid}/context_commits"), params: params
+        expect(response).to have_gitlab_http_status(401)
+      end
+    end
+  end
+
   describe "DELETE /projects/:id/merge_requests/:merge_request_iid" do
     context "when the user is developer" do
       let(:developer) { create(:user) }
@@ -1579,6 +1681,79 @@ describe API::MergeRequests do
     end
   end
 
+  describe "DELETE /projects/:id/merge_requests/:merge_request_iid/context_commits" do
+    let(:merge_request_iid)  { merge_request.iid }
+    let(:authenticated_user) { user }
+    let(:commit) { project.repository.commit }
+
+    context "when authenticated" do
+      let(:params) do
+        {
+          commits: [commit.id]
+        }
+      end
+
+      let(:params_invalid_shas) do
+        {
+          commits: ["invalid"]
+        }
+      end
+
+      let(:params_empty_commits) do
+        {
+          commits: []
+        }
+      end
+
+      it "deletes context commit" do
+        delete api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/context_commits", authenticated_user), params: params
+
+        expect(response).to have_gitlab_http_status(204)
+      end
+
+      it "returns 400 when invalid commit sha is passed" do
+        delete api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/context_commits", authenticated_user), params: params_invalid_shas
+
+        expect(response).to have_gitlab_http_status(400)
+        expect(json_response["message"]).to eq('One or more context commits\' sha is not valid.')
+      end
+
+      it "returns 400 when commits is empty" do
+        delete api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/context_commits", authenticated_user), params: params_empty_commits
+
+        expect(response).to have_gitlab_http_status(400)
+      end
+
+      it "returns 400 when no params is passed" do
+        delete api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/context_commits", authenticated_user)
+
+        expect(response).to have_gitlab_http_status(400)
+      end
+
+      it 'returns 403 when deleting existing context commit for guest role' do
+        guest = create(:user)
+        project.add_guest(guest)
+        delete api("/projects/#{project.id}/merge_requests/#{merge_request_iid}/context_commits", guest), params: params
+        expect(response).to have_gitlab_http_status(403)
+      end
+
+      it 'returns 403 when deleting existing context commit for reporter role' do
+        reporter = create(:user)
+        project.add_reporter(reporter)
+        delete api("/projects/#{project.id}/merge_requests/#{merge_request_iid}/context_commits", reporter), params: params
+        expect(response).to have_gitlab_http_status(403)
+      end
+    end
+
+    context "when unauthenticated" do
+      it "returns 401, unauthorised error" do
+        delete api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/context_commits")
+
+        expect(response).to have_gitlab_http_status(401)
+      end
+    end
+  end
+
   describe "PUT /projects/:id/merge_requests/:merge_request_iid/merge", :clean_gitlab_redis_cache do
     let(:pipeline) { create(:ci_pipeline) }
 

spec/serializers/diffs_metadata_entity_spec.rb

@@ -29,6 +29,7 @@ describe DiffsMetadataEntity do
         :added_lines, :removed_lines, :render_overflow_warning,
         :email_patch_path, :plain_diff_path,
         :merge_request_diffs,
+        :context_commits,
         # Attributes
         :diff_files
       )

spec/services/merge_requests/add_context_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequests::AddContextService do
+  let(:project) { create(:project, :repository) }
+  let(:admin) { create(:admin) }
+  let(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: admin) }
+  let(:commits) { ["874797c3a73b60d2187ed6e2fcabd289ff75171e"] }
+  let(:raw_repository) { project.repository.raw }
+
+  subject(:service) { described_class.new(project, admin, merge_request: merge_request, commits: commits) }
+
+  describe "#execute" do
+    it "adds context commit" do
+      service.execute
+
+      expect(merge_request.merge_request_context_commit_diff_files.length).to eq(2)
+    end
+
+    context "when user doesn't have permission to update merge request" do
+      let(:user) { create(:user) }
+      let(:merge_request1) { create(:merge_request, source_project: project, author: user) }
+
+      subject(:service) { described_class.new(project, user, merge_request: merge_request, commits: commits) }
+
+      it "doesn't add context commit" do
+        subject.execute
+
+        expect(merge_request.merge_request_context_commit_diff_files.length).to eq(0)
+      end
+    end
+
+    context "when the commits array is empty" do
+      subject(:service) { described_class.new(project, admin, merge_request: merge_request, commits: []) }
+
+      it "doesn't add context commit" do
+        subject.execute
+
+        expect(merge_request.merge_request_context_commit_diff_files.length).to eq(0)
+      end
+    end
+  end
+end