Automatic merge of gitlab-org/gitlab master

GITALY_SERVER_VERSION

-2ddefee1edfde10092a278a143c48fdcf86e380f
+bea500b301bbec8535fbcae58c1da2d29377c666

app/assets/javascripts/vue_merge_request_widget/components/states/new_ready_to_merge.vue

+<script>
+import mergeRequestQueryVariablesMixin from '../../mixins/merge_request_query_variables';
+import readyToMergeQuery from '../../queries/states/new_ready_to_merge.query.graphql';
+import StatusIcon from '../mr_widget_status_icon.vue';
+
+export default {
+  apollo: {
+    canMerge: {
+      query: readyToMergeQuery,
+      skip() {
+        return !this.mr || !window.gon?.features?.mergeRequestWidgetGraphql;
+      },
+      variables() {
+        return this.mergeRequestQueryVariables;
+      },
+      update: (data) => data?.project?.mergeRequest?.userPermissions?.canMerge,
+    },
+  },
+  components: {
+    StatusIcon,
+  },
+  mixins: [mergeRequestQueryVariablesMixin],
+  props: {
+    mr: {
+      type: Object,
+      required: true,
+    },
+  },
+  data() {
+    return {
+      canMerge: null,
+    };
+  },
+};
+</script>
+
+<template>
+  <div class="mr-widget-body media">
+    <status-icon status="success" />
+    <p class="media-body gl-m-0! gl-font-weight-bold">
+      <template v-if="canMerge">
+        {{ __('Ready to merge!') }}
+      </template>
+      <template v-else>
+        {{ __('Ready to merge by members who can write to the target branch.') }}
+      </template>
+    </p>
+  </div>
+</template>

app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue

@@ -72,7 +72,9 @@ export default {
     'mr-widget-nothing-to-merge': NothingToMergeState,
     'mr-widget-not-allowed': NotAllowedState,
     'mr-widget-missing-branch': MissingBranchState,
-    'mr-widget-ready-to-merge': ReadyToMergeState,
+    'mr-widget-ready-to-merge': window.gon?.features?.restructuredMrWidget
+      ? () => import('./components/states/new_ready_to_merge.vue')
+      : ReadyToMergeState,
     'sha-mismatch': ShaMismatch,
     'mr-widget-checking': CheckingState,
     'mr-widget-unresolved-discussions': UnresolvedDiscussionsState,

app/assets/javascripts/vue_merge_request_widget/queries/states/new_ready_to_merge.query.graphql

+query readyToMergeQuery($projectPath: ID!, $iid: String!) {
+  project(fullPath: $projectPath) {
+    mergeRequest(iid: $iid) {
+      userPermissions {
+        canMerge
+      }
+    }
+  }
+}

app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js

@@ -25,7 +25,7 @@ export default function deviseState() {
     return stateKey.shaMismatch;
   } else if (this.autoMergeEnabled && !this.mergeError) {
     return stateKey.autoMergeEnabled;
-  } else if (!this.canMerge) {
+  } else if (!this.canMerge && !window.gon?.features?.restructuredMrWidget) {
     return stateKey.notAllowedToMerge;
   } else if (this.canBeMerged) {
     return stateKey.readyToMerge;

app/controllers/projects/merge_requests_controller.rb

@@ -41,6 +41,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     push_frontend_feature_flag(:usage_data_i_testing_summary_widget_total, @project, default_enabled: :yaml)
     push_frontend_feature_flag(:improved_emoji_picker, project, default_enabled: :yaml)
     push_frontend_feature_flag(:diffs_virtual_scrolling, project, default_enabled: :yaml)
+    push_frontend_feature_flag(:restructured_mr_widget, project, default_enabled: :yaml)
 
     # Usage data feature flags
     push_frontend_feature_flag(:users_expanding_widgets_usage_data, @project, default_enabled: :yaml)

app/helpers/ci/runners_helper.rb

@@ -65,6 +65,15 @@ def group_shared_runners_settings_data(group)
       }
     end
 
+    def group_runners_data_attributes(group)
+      {
+        registration_token: group.runners_token,
+        group_id: group.id,
+        group_full_path: group.full_path,
+        runner_install_help_page: 'https://docs.gitlab.com/runner/install/'
+      }
+    end
+
     def toggle_shared_runners_settings_data(project)
       {
         is_enabled: "#{project.shared_runners_enabled?}",

app/models/repository.rb

@@ -161,8 +161,8 @@ def commits(ref = nil, opts = {})
     CommitCollection.new(container, commits, ref)
   end
 
-  def commits_between(from, to)
-    commits = Gitlab::Git::Commit.between(raw_repository, from, to)
+  def commits_between(from, to, limit: nil)
+    commits = Gitlab::Git::Commit.between(raw_repository, from, to, limit: limit)
     commits = Commit.decorate(commits, container) if commits.present?
     commits
   end

app/services/git/base_hooks_service.rb

@@ -25,17 +25,13 @@ def hook_name
       raise NotImplementedError, "Please implement #{self.class}##{__method__}"
     end
 
-    # The changeset, ordered with the newest commit last
-    def commits
-      raise NotImplementedError, "Please implement #{self.class}##{__method__}"
-    end
-
+    # This should return PROCESS_COMMIT_LIMIT commits, ordered with newest last
     def limited_commits
-      @limited_commits ||= commits.last(PROCESS_COMMIT_LIMIT)
+      raise NotImplementedError, "Please implement #{self.class}##{__method__}"
     end
 
     def commits_count
-      commits.count
+      raise NotImplementedError, "Please implement #{self.class}##{__method__}"
     end
 
     def event_message

app/services/git/branch_hooks_service.rb

@@ -18,20 +18,25 @@ def hook_name
       :push_hooks
     end
 
-    def commits
-      strong_memoize(:commits) do
+    def limited_commits
+      strong_memoize(:limited_commits) { threshold_commits.last(PROCESS_COMMIT_LIMIT) }
+    end
+
+    # Taking limit+1 commits allows us to detect when the limit is in effect
+    def threshold_commits
+      strong_memoize(:threshold_commits) do
         if creating_default_branch?
           # The most recent PROCESS_COMMIT_LIMIT commits in the default branch.
           # They are returned newest-to-oldest, but we need to present them oldest-to-newest
-          project.repository.commits(newrev, limit: PROCESS_COMMIT_LIMIT).reverse
+          project.repository.commits(newrev, limit: PROCESS_COMMIT_LIMIT + 1).reverse!
         elsif creating_branch?
           # Use the pushed commits that aren't reachable by the default branch
           # as a heuristic. This may include more commits than are actually
           # pushed, but that shouldn't matter because we check for existing
           # cross-references later.
-          project.repository.commits_between(project.default_branch, newrev)
+          project.repository.commits_between(project.default_branch, newrev, limit: PROCESS_COMMIT_LIMIT + 1)
         elsif updating_branch?
-          project.repository.commits_between(oldrev, newrev)
+          project.repository.commits_between(oldrev, newrev, limit: PROCESS_COMMIT_LIMIT + 1)
         else # removing branch
           []
         end
@@ -39,9 +44,21 @@ def commits
     end
 
     def commits_count
-      return count_commits_in_branch if creating_default_branch?
+      strong_memoize(:commits_count) do
+        next threshold_commits.count if
+          strong_memoized?(:threshold_commits) &&
+          threshold_commits.count <= PROCESS_COMMIT_LIMIT
 
-      super
+        if creating_default_branch?
+          project.repository.commit_count_for_ref(ref)
+        elsif creating_branch?
+          project.repository.count_commits_between(project.default_branch, newrev)
+        elsif updating_branch?
+          project.repository.count_commits_between(oldrev, newrev)
+        else # removing branch
+          0
+        end
+      end
     end
 
     override :invalidated_file_types
@@ -179,12 +196,6 @@ def creating_default_branch?
       creating_branch? && default_branch?
     end
 
-    def count_commits_in_branch
-      strong_memoize(:count_commits_in_branch) do
-        project.repository.commit_count_for_ref(ref)
-      end
-    end
-
     def default_branch?
       strong_memoize(:default_branch) do
         [nil, branch_name].include?(project.default_branch)

app/services/git/tag_hooks_service.rb

@@ -8,10 +8,14 @@ def hook_name
       :tag_push_hooks
     end
 
-    def commits
+    def limited_commits
       [tag_commit].compact
     end
 
+    def commits_count
+      limited_commits.count
+    end
+
     def event_message
       tag&.message
     end

app/views/groups/runners/index.html.haml

@@ -3,4 +3,4 @@
 %h2.page-title
   = s_('Runners|Group Runners')
 
-#js-group-runners{ data: { registration_token: @group.runners_token, runner_install_help_page: 'https://docs.gitlab.com/runner/install/', group_id: @group.id, group_full_path: @group.full_path, group_runners_limited_count: @group_runners_limited_count } }
+#js-group-runners{ data: group_runners_data_attributes(@group).merge( { group_runners_limited_count: @group_runners_limited_count } ) }

config/feature_flags/development/restructured_mr_widget.yml

+---
+name: restructured_mr_widget
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68565
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/339181
+milestone: '14.3'
+type: development
+group: group::code review
+default_enabled: false

ee/app/models/status_page/published_incident.rb

@@ -8,8 +8,22 @@ class PublishedIncident < ApplicationRecord
     belongs_to :issue, inverse_of: :status_page_published_incident
     validates :issue, presence: true
 
+    # NOTE: This method is not atomic and might raise
+    # +ActiveRecord::RecordNotUnique+ in case of a duplicate published issue.
+    #
+    # Previously, we've used +safe_find_or_create_by+ to circumvent this fact
+    # but it uses subtransactions under the hood which is problematic in nested
+    # transactions.
+    #
+    # See https://gitlab.com/groups/gitlab-org/-/epics/6540 for more context.
+    #
+    # In the rare event of +ActiveRecord::RecordNotUnique+ users end up seeing
+    # a meaningful error message. This behaviour is acceptable and that's why
+    # switched to unsafe method +find_or_create_by+.
+    #
+    # @raise ActiveRecord::RecordNotUnique
     def self.track(issue)
-      safe_find_or_create_by(issue: issue)
+      find_or_create_by!(issue: issue)
     end
 
     def self.untrack(issue)

ee/spec/models/status_page/published_incident_spec.rb

@@ -3,6 +3,13 @@
 require 'spec_helper'
 
 RSpec.describe StatusPage::PublishedIncident do
+  let_it_be_with_reload(:issue) { create(:issue) }
+
+  before do
+    # prefill association cache
+    issue&.status_page_published_incident
+  end
+
   describe 'associations' do
     it { is_expected.to belong_to(:issue).inverse_of(:status_page_published_incident) }
   end
@@ -12,11 +19,10 @@
   end
 
   describe '.track' do
-    let_it_be(:issue) { create(:issue) }
-
     subject { described_class.track(issue) }
 
     it { is_expected.to be_a(described_class) }
+    it { is_expected.to eq(issue.status_page_published_incident) }
     specify { expect(subject.issue).to eq issue }
     specify { expect { subject }.to change { described_class.count }.by(1) }
 
@@ -26,14 +32,31 @@
       end
 
       it { is_expected.to be_a(described_class) }
+      it { is_expected.to eq(issue.status_page_published_incident) }
       specify { expect(subject.issue).to eq issue }
       specify { expect { subject }.not_to change { described_class.count } }
     end
+
+    context 'when issue is new record' do
+      let(:issue) { build(:issue) }
+
+      it { is_expected.to be_a(described_class) }
+      it { is_expected.to eq(issue.status_page_published_incident) }
+      specify { expect(subject.issue).to eq issue }
+      specify { expect { subject }.to change { described_class.count }.by(1) }
+    end
+
+    context 'when issue is nil' do
+      let(:issue) { nil }
+
+      specify do
+        expect { subject }
+          .to raise_error(ActiveRecord::RecordInvalid, /Issue can't be blank/)
+      end
+    end
   end
 
   describe '.untrack' do
-    let_it_be(:issue) { create(:issue) }
-
     subject { described_class.untrack(issue) }
 
     context 'when the incident is not yet tracked' do

lib/gitlab/git/commit.rb

@@ -108,17 +108,26 @@ def last_for_path(repo, ref, path = nil, literal_pathspec: false)
         # See also #repository.commits_between
         #
         # Ex.
-        #   Commit.between(repo, '29eda46b', 'master')
+        #   Commit.between(repo, '29eda46b', 'master') # all commits, ordered oldest to newest
+        #   Commit.between(repo, '29eda46b', 'master', limit: 100) # 100 newest commits, ordered oldest to newest
         #
-        def between(repo, base, head)
+        def between(repo, base, head, limit: nil)
           # In either of these cases, we are guaranteed to return no commits, so
           # shortcut the RPC call
           return [] if Gitlab::Git.blank_ref?(base) || Gitlab::Git.blank_ref?(head)
 
           wrapped_gitaly_errors do
             revisions = [head, "^#{base}"] # base..head
-
-            repo.gitaly_commit_client.list_commits(revisions, reverse: true)
+            client = repo.gitaly_commit_client
+
+            # We must return the commits in chronological order but using both
+            # limit and reverse in the Gitaly RPC would return the oldest N,
+            # rather than newest N, commits, so reorder in Ruby with limit
+            if limit
+              client.list_commits(revisions, pagination_params: { limit: limit }).reverse!
+            else
+              client.list_commits(revisions, reverse: true)
+            end
           end
         end
 

lib/gitlab/gitaly_client/commit_service.rb

@@ -255,11 +255,12 @@ def find_all_commits(opts = {})
         consume_commits_response(response)
       end
 
-      def list_commits(revisions, reverse: false)
+      def list_commits(revisions, reverse: false, pagination_params: nil)
         request = Gitaly::ListCommitsRequest.new(
           repository: @gitaly_repo,
           revisions: Array.wrap(revisions),
-          reverse: reverse
+          reverse: reverse,
+          pagination_params: pagination_params
         )
 
         response = GitalyClient.call(@repository.storage, :commit_service, :list_commits, request, timeout: GitalyClient.medium_timeout)

locale/gitlab.pot

@@ -27483,6 +27483,12 @@ msgstr ""
 msgid "Ready to get started with GitLab? Follow these steps to set up your workspace, plan and commit changes, and deploy your project."
 msgstr ""
+msgid "Ready to merge by members who can write to the target branch."
+msgstr ""
+
+msgid "Ready to merge!"
+msgstr ""
+
 msgid "Reauthenticating with SAML provider."
 msgstr ""

spec/frontend/vue_mr_widget/components/states/__snapshots__/new_ready_to_merge_spec.js.snap

+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`New ready to merge state component renders permission text if canMerge (false) is false 1`] = `
+<div
+  class="mr-widget-body media"
+>
+  <status-icon-stub
+    status="success"
+  />
+   
+  <p
+    class="media-body gl-m-0! gl-font-weight-bold"
+  >
+    
+      Ready to merge by members who can write to the target branch.
+    
+  </p>
+</div>
+`;
+
+exports[`New ready to merge state component renders permission text if canMerge (true) is false 1`] = `
+<div
+  class="mr-widget-body media"
+>
+  <status-icon-stub
+    status="success"
+  />
+   
+  <p
+    class="media-body gl-m-0! gl-font-weight-bold"
+  >
+    
+      Ready to merge!
+    
+  </p>
+</div>
+`;

spec/frontend/vue_mr_widget/components/states/new_ready_to_merge_spec.js

+import { shallowMount } from '@vue/test-utils';
+import ReadyToMerge from '~/vue_merge_request_widget/components/states/new_ready_to_merge.vue';
+
+let wrapper;
+
+function factory({ canMerge }) {
+  wrapper = shallowMount(ReadyToMerge, {
+    propsData: {
+      mr: {},
+    },
+    data() {
+      return { canMerge };
+    },
+  });
+}
+
+describe('New ready to merge state component', () => {
+  afterEach(() => {
+    wrapper.destroy();
+  });
+
+  it.each`
+    canMerge
+    ${true}
+    ${false}
+  `('renders permission text if canMerge ($canMerge) is false', ({ canMerge }) => {
+    factory({ canMerge });
+
+    expect(wrapper.element).toMatchSnapshot();
+  });
+});