Merge branch 'feature/arbitrary-commit-revision-picker-2' into 'master'

Implement the arbitrary dropdown and the corresponding diffs in `Changes` Tab

See merge request gitlab-org/gitlab!57026

.rubocop_manual_todo.yml

@@ -1546,6 +1546,7 @@ Gitlab/NamespacedClass:
     - 'app/models/concerns/uniquify.rb'
     - 'app/models/container_expiration_policy.rb'
     - 'app/models/container_repository.rb'
+    - 'app/models/context_commits_diff.rb'
     - 'app/models/custom_emoji.rb'
     - 'app/models/data_list.rb'
     - 'app/models/deploy_key.rb'
@@ -1960,6 +1961,7 @@ Gitlab/NamespacedClass:
     - 'app/serializers/container_repository_entity.rb'
     - 'app/serializers/container_tag_entity.rb'
     - 'app/serializers/container_tags_serializer.rb'
+    - 'app/serializers/context_commits_diff_entity.rb'
     - 'app/serializers/current_board_entity.rb'
     - 'app/serializers/current_board_serializer.rb'
     - 'app/serializers/current_user_entity.rb'

app/assets/javascripts/diffs/components/compare_dropdown_layout.vue

 <script>
-import { GlDropdown, GlDropdownItem } from '@gitlab/ui';
+import { GlDropdown, GlDropdownItem, GlDropdownDivider } from '@gitlab/ui';
 import TimeAgo from '~/vue_shared/components/time_ago_tooltip.vue';
 
 export default {
   components: {
     GlDropdown,
     GlDropdownItem,
+    GlDropdownDivider,
     TimeAgo,
   },
   props: {
@@ -24,34 +25,38 @@ export default {
 
 <template>
   <gl-dropdown :text="selectedVersionName" data-qa-selector="dropdown_content">
-    <gl-dropdown-item
-      v-for="version in versions"
-      :key="version.id"
-      :class="{
-        'is-active': version.selected,
-      }"
-      :is-check-item="true"
-      :is-checked="version.selected"
-      :href="version.href"
-    >
-      <div>
-        <strong>
-          {{ version.versionName }}
-          <template v-if="version.isHead">{{ s__('DiffsCompareBaseBranch|(HEAD)') }}</template>
-          <template v-else-if="version.isBase">{{ s__('DiffsCompareBaseBranch|(base)') }}</template>
-        </strong>
-      </div>
-      <div>
-        <small class="commit-sha"> {{ version.short_commit_sha }} </small>
-      </div>
-      <div>
-        <small>
-          <template v-if="version.commitsText">
-            {{ version.commitsText }}
-          </template>
-          <time-ago v-if="version.created_at" :time="version.created_at" class="js-timeago" />
-        </small>
-      </div>
-    </gl-dropdown-item>
+    <template v-for="version in versions">
+      <gl-dropdown-divider v-if="version.addDivider" :key="version.id" />
+      <gl-dropdown-item
+        :key="version.id"
+        :class="{
+          'is-active': version.selected,
+        }"
+        :is-check-item="true"
+        :is-checked="version.selected"
+        :href="version.href"
+      >
+        <div>
+          <strong>
+            {{ version.versionName }}
+            <template v-if="version.isHead">{{ s__('DiffsCompareBaseBranch|(HEAD)') }}</template>
+            <template v-else-if="version.isBase">{{
+              s__('DiffsCompareBaseBranch|(base)')
+            }}</template>
+          </strong>
+        </div>
+        <div>
+          <small class="commit-sha"> {{ version.short_commit_sha }} </small>
+        </div>
+        <div>
+          <small>
+            <template v-if="version.commitsText">
+              {{ version.commitsText }}
+            </template>
+            <time-ago v-if="version.created_at" :time="version.created_at" class="js-timeago" />
+          </small>
+        </div>
+      </gl-dropdown-item>
+    </template>
   </gl-dropdown>
 </template>

app/assets/javascripts/diffs/store/getters_versions_dropdowns.js

@@ -7,6 +7,9 @@ export const selectedTargetIndex = (state) =>
 
 export const selectedSourceIndex = (state) => state.mergeRequestDiff.version_index;
 
+export const selectedContextCommitsDiffs = (state) =>
+  state.contextCommitsDiff && state.contextCommitsDiff.showing_context_commits_diff;
+
 export const diffCompareDropdownTargetVersions = (state, getters) => {
   // startVersion only exists if the user has selected a version other
   // than "base" so if startVersion is null then base must be selected
@@ -58,7 +61,7 @@ export const diffCompareDropdownTargetVersions = (state, getters) => {
 
 export const diffCompareDropdownSourceVersions = (state, getters) => {
   // Appended properties here are to make the compare_dropdown_layout easier to reason about
-  return state.mergeRequestDiffs.map((v, i) => {
+  const versions = state.mergeRequestDiffs.map((v, i) => {
     const isLatestVersion = i === 0;
 
     return {
@@ -69,7 +72,20 @@ export const diffCompareDropdownSourceVersions = (state, getters) => {
       versionName: isLatestVersion
         ? __('latest version')
         : sprintf(__(`version %{versionIndex}`), { versionIndex: v.version_index }),
-      selected: v.version_index === getters.selectedSourceIndex,
+      selected:
+        v.version_index === getters.selectedSourceIndex && !getters.selectedContextCommitsDiffs,
     };
   });
+
+  const { contextCommitsDiff } = state;
+  if (contextCommitsDiff) {
+    versions.push({
+      href: contextCommitsDiff.diffs_path,
+      commitsText: n__(`%d commit`, `%d commits`, contextCommitsDiff.commits_count),
+      versionName: __('previously merged commits'),
+      selected: getters.selectedContextCommitsDiffs,
+      addDivider: state.mergeRequestDiffs.length > 0,
+    });
+  }
+  return versions;
 };

app/controllers/projects/merge_requests/diffs_controller.rb

@@ -47,7 +47,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
     diffs = @compare.diffs(diff_options)
 
     render json: DiffsMetadataSerializer.new(project: @merge_request.project, current_user: current_user)
-                   .represent(diffs, additional_attributes)
+                   .represent(diffs, additional_attributes.merge(only_context_commits: show_only_context_commits?))
   end
 
   private
@@ -92,7 +92,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
   # rubocop: disable CodeReuse/ActiveRecord
   def commit
     return unless commit_id = params[:commit_id].presence
-    return unless @merge_request.all_commits.exists?(sha: commit_id)
+    return unless @merge_request.all_commits.exists?(sha: commit_id) || @merge_request.recent_context_commits.map(&:id).include?(commit_id)
 
     @commit ||= @project.commit(commit_id)
   end
@@ -122,6 +122,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
       end
     end
 
+    return @merge_request.context_commits_diff if show_only_context_commits? && !@merge_request.context_commits_diff.empty?
     return @merge_request.merge_head_diff if render_merge_ref_head_diff?
 
     if @start_sha

app/controllers/projects/merge_requests_controller.rb

@@ -113,6 +113,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
 
         @noteable = @merge_request
         @commits_count = @merge_request.commits_count + @merge_request.context_commits_count
+        @diffs_count = get_diffs_count
         @issuable_sidebar = serializer.represent(@merge_request, serializer: 'sidebar')
         @current_user_data = UserSerializer.new(project: @project).represent(current_user, {}, MergeRequestCurrentUserEntity).to_json
         @show_whitespace_default = current_user.nil? || current_user.show_whitespace_in_diffs
@@ -385,6 +386,14 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
 
   private
 
+  def get_diffs_count
+    if show_only_context_commits?
+      @merge_request.context_commits_diff.raw_diffs.size
+    else
+      @merge_request.diff_size
+    end
+  end
+
   def merge_request_update_params
     merge_request_params.merge!(params.permit(:merge_request_diff_head_sha))
   end

app/helpers/diff_helper.rb

@@ -23,14 +23,16 @@ module DiffHelper
     end
   end
 
+  def show_only_context_commits?
+    !!params[:only_context_commits] || @merge_request&.commits&.empty?
+  end
+
   def diff_options
     options = { ignore_whitespace_change: hide_whitespace?, expanded: diffs_expanded? }
 
     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/context_commits_diff.rb

+# frozen_string_literal: true
+
+class ContextCommitsDiff
+  include ActsAsPaginatedDiff
+
+  attr_reader :merge_request
+
+  def initialize(merge_request)
+    @merge_request = merge_request
+  end
+
+  def empty?
+    commits.empty?
+  end
+
+  def commits_count
+    merge_request.context_commits_count
+  end
+
+  def diffs(diff_options = nil)
+    Gitlab::Diff::FileCollection::Compare.new(
+      self,
+      project: merge_request.project,
+      diff_options: diff_options,
+      diff_refs: diff_refs
+    )
+  end
+
+  def raw_diffs(options = {})
+    compare.diffs(options.merge(paths: paths))
+  end
+
+  def diff_refs
+    Gitlab::Diff::DiffRefs.new(
+      base_sha: commits.last&.diff_refs&.base_sha,
+      head_sha: commits.first&.diff_refs&.head_sha
+    )
+  end
+
+  private
+
+  def compare
+    @compare ||=
+      Gitlab::Git::Compare.new(
+        merge_request.project.repository.raw_repository,
+        commits.last&.diff_refs&.base_sha,
+        commits.first&.diff_refs&.head_sha
+      )
+  end
+
+  def commits
+    @commits ||= merge_request.project.repository.commits_by(oids: merge_request.recent_context_commits.map(&:id))
+  end
+
+  def paths
+    merge_request.merge_request_context_commit_diff_files.map(&:path)
+  end
+end

app/models/merge_request.rb

@@ -1899,6 +1899,12 @@ class MergeRequest < ApplicationRecord
     diff_stats.map(&:path).include?(project.ci_config_path_or_default)
   end
 
+  def context_commits_diff
+    strong_memoize(:context_commits_diff) do
+      ContextCommitsDiff.new(self)
+    end
+  end
+
   private
 
   def missing_report_error(report_type)

app/models/merge_request_context_commit_diff_file.rb

@@ -16,4 +16,8 @@ class MergeRequestContextCommitDiffFile < ApplicationRecord
   def self.bulk_insert(*args)
     Gitlab::Database.bulk_insert('merge_request_context_commit_diff_files', *args) # rubocop:disable Gitlab/BulkInsert
   end
+
+  def path
+    new_path.presence || old_path
+  end
 end

app/models/merge_request_diff.rb

@@ -665,10 +665,6 @@ 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/serializers/context_commits_diff_entity.rb

+# frozen_string_literal: true
+
+class ContextCommitsDiffEntity < Grape::Entity
+  include Gitlab::Routing
+
+  expose :commits_count
+
+  expose :showing_context_commits_diff do |_, options|
+    options[:only_context_commits]
+  end
+
+  expose :diffs_path do |diff|
+    merge_request = diff.merge_request
+    project = merge_request.target_project
+
+    next unless project
+
+    diffs_project_merge_request_path(project, merge_request, only_context_commits: true)
+  end
+end

app/serializers/diffs_entity.rb

@@ -84,6 +84,15 @@ class DiffsEntity < Grape::Entity
     project_blob_path(merge_request.project, merge_request.diff_head_sha)
   end
 
+  expose :context_commits_diff, if: -> (_) { merge_request&.project&.context_commits_enabled? } do |diffs, options|
+    next unless merge_request.context_commits_diff.commits_count > 0
+
+    ContextCommitsDiffEntity.represent(
+      merge_request.context_commits_diff,
+      options
+    )
+  end
+
   def merge_request
     options[:merge_request]
   end

app/views/projects/merge_requests/show.html.haml

@@ -40,7 +40,7 @@
           = render "projects/merge_requests/tabs/tab", name: "diffs", class: "diffs-tab", id: "diffs-tab", qa_selector: "diffs_tab" do
             = tab_link_for @merge_request, :diffs do
               = _("Changes")
-              %span.badge.badge-pill.gl-badge.badge-muted.sm= @merge_request.diff_size
+              %span.badge.badge-pill.gl-badge.badge-muted.sm= @diffs_count
         .d-flex.flex-wrap.align-items-center.justify-content-lg-end
           #js-vue-discussion-counter
 

changelogs/unreleased/feature-arbitrary-commit-revision-picker-2.yml

+---
+title: Add "previously merged commits" dropdown in merge request compare dropdown
+merge_request: 57026
+author:
+type: added

locale/gitlab.pot

@@ -38471,6 +38471,9 @@ msgid_plural "points"
 msgstr[0] ""
 msgstr[1] ""
 
+msgid "previously merged commits"
+msgstr ""
+
 msgid "private"
 msgstr ""
 

spec/controllers/projects/merge_requests/diffs_controller_spec.rb

@@ -180,7 +180,8 @@ RSpec.describe Projects::MergeRequests::DiffsController do
           start_version: nil,
           start_sha: nil,
           commit: nil,
-          latest_diff: true
+          latest_diff: true,
+          only_context_commits: false
         }
 
         expect_next_instance_of(DiffsMetadataSerializer) do |instance|
@@ -261,7 +262,8 @@ RSpec.describe Projects::MergeRequests::DiffsController do
           start_version: nil,
           start_sha: nil,
           commit: nil,
-          latest_diff: true
+          latest_diff: true,
+          only_context_commits: false
         }
 
         expect_next_instance_of(DiffsMetadataSerializer) do |instance|
@@ -290,7 +292,8 @@ RSpec.describe Projects::MergeRequests::DiffsController do
           start_version: nil,
           start_sha: nil,
           commit: merge_request.diff_head_commit,
-          latest_diff: nil
+          latest_diff: nil,
+          only_context_commits: false
         }
 
         expect_next_instance_of(DiffsMetadataSerializer) do |instance|

spec/models/context_commits_diff_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ContextCommitsDiff do
+  let_it_be(:sha1) { "33f3729a45c02fc67d00adb1b8bca394b0e761d9" }
+  let_it_be(:sha2) { "ae73cb07c9eeaf35924a10f713b364d32b2dd34f" }
+  let_it_be(:sha3) { "0b4bc9a49b562e85de7cc9e834518ea6828729b9" }
+  let_it_be(:merge_request) { create(:merge_request) }
+  let_it_be(:project) { merge_request.project }
+  let_it_be(:mrcc1) { create(:merge_request_context_commit, merge_request: merge_request, sha: sha1, committed_date: project.commit_by(oid: sha1).committed_date) }
+  let_it_be(:mrcc2) { create(:merge_request_context_commit, merge_request: merge_request, sha: sha2, committed_date: project.commit_by(oid: sha2).committed_date) }
+  let_it_be(:mrcc3) { create(:merge_request_context_commit, merge_request: merge_request, sha: sha3, committed_date: project.commit_by(oid: sha3).committed_date) }
+
+  subject { merge_request.context_commits_diff }
+
+  describe ".empty?" do
+    it 'checks if empty' do
+      expect(subject.empty?).to be(false)
+    end
+  end
+
+  describe '.commits_count' do
+    it 'reports commits count' do
+      expect(subject.commits_count).to be(3)
+    end
+  end
+
+  describe '.diffs' do
+    it 'returns instance of Gitlab::Diff::FileCollection::Compare' do
+      expect(subject.diffs).to be_a(Gitlab::Diff::FileCollection::Compare)
+    end
+
+    it 'returns all diffs between first and last commits' do
+      expect(subject.diffs.diff_files.size).to be(5)
+    end
+  end
+
+  describe '.raw_diffs' do
+    before do
+      allow(subject).to receive(:paths).and_return(["Gemfile.zip", "files/images/6049019_460s.jpg", "files/ruby/feature.rb"])
+    end
+
+    it 'returns instance of Gitlab::Git::DiffCollection' do
+      expect(subject.raw_diffs).to be_a(Gitlab::Git::DiffCollection)
+    end
+
+    it 'returns only diff for files changed in the context commits' do
+      expect(subject.raw_diffs.size).to be(3)
+    end
+  end
+
+  describe '.diff_refs' do
+    it 'returns correct sha' do
+      expect(subject.diff_refs.head_sha).to eq(sha3)
+      expect(subject.diff_refs.base_sha).to eq("913c66a37b4a45b9769037c55c2d238bd0942d2e")
+    end
+  end
+end

spec/serializers/context_commits_diff_entity_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ContextCommitsDiffEntity do
+  let_it_be(:merge_request) { create(:merge_request) }
+  let_it_be(:mrcc1) { create(:merge_request_context_commit, merge_request: merge_request, sha: "cfe32cf61b73a0d5e9f13e774abde7ff789b1660") }
+  let_it_be(:mrcc2) { create(:merge_request_context_commit, merge_request: merge_request, sha: "ae73cb07c9eeaf35924a10f713b364d32b2dd34f") }
+
+  context 'as json' do
+    subject { ContextCommitsDiffEntity.represent(merge_request.context_commits_diff).as_json }
+
+    it 'exposes commits_count' do
+      expect(subject[:commits_count]).to eq(2)
+    end
+
+    it 'exposes showing_context_commits_diff' do
+      expect(subject).to have_key(:showing_context_commits_diff)
+    end
+
+    it 'exposes diffs_path' do
+      expect(subject[:diffs_path]).to eq(Gitlab::Routing.url_helpers.diffs_project_merge_request_path(merge_request.project, merge_request, only_context_commits: true))
+    end
+  end
+end

spec/serializers/diffs_metadata_entity_spec.rb

@@ -28,7 +28,7 @@ RSpec.describe DiffsMetadataEntity do
         :start_version, :latest_diff, :latest_version_path,
         :added_lines, :removed_lines, :render_overflow_warning,
         :email_patch_path, :plain_diff_path,
-        :merge_request_diffs, :context_commits,
+        :merge_request_diffs, :context_commits, :context_commits_diff,
         :definition_path_prefix, :source_branch_exists,
         :can_merge, :conflict_resolution_path, :has_conflicts,
         :project_name, :project_path, :user_full_name, :username,