Merge branch '11-9-stable-patch-5' into '11-9-stable'

Prepare 11.9.5 release

See merge request gitlab-org/gitlab-ce!26866

.gitlab-ci.yml

@@ -315,7 +315,7 @@ cloud-native-image:
   variables:
     GIT_DEPTH: "1"
   cache: {}
-  when: always
+  when: manual
   script:
     - gem install gitlab --no-document
     - CNG_PROJECT_PATH="gitlab-org/build/CNG" BUILD_TRIGGER_TOKEN=$CI_JOB_TOKEN ./scripts/trigger-build cng

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

@@ -128,9 +128,6 @@ export default {
     isModeChanged() {
       return this.diffFile.viewer.name === diffViewerModes.mode_changed;
     },
-    showExpandDiffToFullFileEnabled() {
-      return gon.features.expandDiffFullFile && !this.diffFile.is_fully_expanded;
-    },
   },
   mounted() {
     polyfillSticky(this.$refs.header);
@@ -258,7 +255,7 @@ export default {
         <icon name="external-link" />
       </gl-button>
       <gl-button
-        v-if="showExpandDiffToFullFileEnabled"
+        v-if="!diffFile.is_fully_expanded"
         class="expand-file js-expand-file"
         @click="toggleFullDiff(diffFile.file_path)"
       >

app/controllers/projects/merge_requests_controller.rb

@@ -16,11 +16,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   before_action :authenticate_user!, only: [:assign_related_issues]
   before_action :check_user_can_push_to_source_branch!, only: [:rebase]
 
-  before_action only: [:show] do
-    push_frontend_feature_flag(:diff_tree_filtering, default_enabled: true)
-    push_frontend_feature_flag(:expand_diff_full_file)
-  end
-
   def index
     @merge_requests = @issuables
 

app/serializers/diff_file_entity.rb

@@ -57,7 +57,7 @@ class DiffFileEntity < DiffFileBaseEntity
     diff_file.diff_lines_for_serializer
   end
 
-  expose :is_fully_expanded, if: -> (diff_file, _) { Feature.enabled?(:expand_diff_full_file, default_enabled: true) && diff_file.text? } do |diff_file|
+  expose :is_fully_expanded, if: -> (diff_file, _) { diff_file.text? } do |diff_file|
     diff_file.fully_expanded?
   end
 

app/services/projects/after_import_service.rb

@@ -9,7 +9,7 @@ def initialize(project)
     end
 
     def execute
-      Projects::HousekeepingService.new(@project).execute do
+      Projects::HousekeepingService.new(@project, :gc).execute do
         repository.delete_all_refs_except(RESERVED_REF_PREFIXES)
       end
     rescue Projects::HousekeepingService::LeaseTaken => e

app/services/projects/housekeeping_service.rb

@@ -18,8 +18,9 @@ def to_s
       end
     end
 
-    def initialize(project)
+    def initialize(project, task = nil)
       @project = project
+      @task = task
     end
 
     def execute
@@ -69,6 +70,8 @@ def pushes_since_gc
     end
 
     def task
+      return @task if @task
+
       if pushes_since_gc % gc_period == 0
         :gc
       elsif pushes_since_gc % full_repack_period == 0

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

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

changelogs/unreleased/sh-fix-project-branches-merge-status.yml

+---
+title: Fix API /project/:id/branches not returning correct merge status
+merge_request: 26785
+author:
+type: fixed

changelogs/unreleased/sh-fix-rugged-tree-entries.yml

+---
+title: Avoid excessive recursive calls with Rugged TreeEntries
+merge_request: 26813
+author:
+type: fixed

changelogs/unreleased/sh-force-gc-after-import.yml

+---
+title: Force a full GC after importing a project
+merge_request: 26803
+author:
+type: performance

doc/api/services.md

@@ -523,7 +523,7 @@ Parameters:
 | `username`      | string | yes  | The username of the user created to be used with GitLab/JIRA. |
 | `password`      | string | yes  | The password of the user created to be used with GitLab/JIRA. |
 | `active`        | boolean | no  | Activates or deactivates the service. Defaults to false (deactivated). |
-| `jira_issue_transition_id` | integer | no | The ID of a transition that moves issues to a closed state. You can find this number under the JIRA workflow administration (**Administration > Issues > Workflows**) by selecting **View** under **Operations** of the desired workflow of your project. The ID of each state can be found inside the parenthesis of each transition name under the **Transitions (id)** column ([see screenshot][trans]). By default, this ID is set to `2`. |
+| `jira_issue_transition_id` | string | no | The ID of a transition that moves issues to a closed state. You can find this number under the JIRA workflow administration (**Administration > Issues > Workflows**) by selecting **View** under **Operations** of the desired workflow of your project. The ID of each state can be found inside the parenthesis of each transition name under the **Transitions (id)** column ([see screenshot][trans]). By default, this ID is set to `2`. |
 
 ### Delete JIRA service
 

lib/api/branches.rb

@@ -34,11 +34,11 @@ class Branches < Grape::API
         repository = user_project.repository
 
         branches = BranchesFinder.new(repository, declared_params(include_missing: false)).execute
-        branches = ::Kaminari.paginate_array(branches)
+        branches = paginate(::Kaminari.paginate_array(branches))
         merged_branch_names = repository.merged_branch_names(branches.map(&:name))
 
         present(
-          paginate(branches),
+          branches,
           with: Entities::Branch,
           current_user: current_user,
           project: user_project,

lib/gitlab/git/rugged_impl/tree.rb

@@ -15,12 +15,23 @@ module ClassMethods
           override :tree_entries
           def tree_entries(repository, sha, path, recursive)
             if Feature.enabled?(:rugged_tree_entries)
-              tree_entries_from_rugged(repository, sha, path, recursive)
+              tree_entries_with_flat_path_from_rugged(repository, sha, path, recursive)
             else
               super
             end
           end
 
+          def tree_entries_with_flat_path_from_rugged(repository, sha, path, recursive)
+            tree_entries_from_rugged(repository, sha, path, recursive).tap do |entries|
+              # This was an optimization to reduce N+1 queries for Gitaly
+              # (https://gitlab.com/gitlab-org/gitaly/issues/530).  It
+              # used to be done lazily in the view via
+              # TreeHelper#flatten_tree, so it's possible there's a
+              # performance impact by loading this eagerly.
+              rugged_populate_flat_path(repository, sha, path, entries)
+            end
+          end
+
           def tree_entries_from_rugged(repository, sha, path, recursive)
             current_path_entries = get_tree_entries_from_rugged(repository, sha, path)
             ordered_entries = []
@@ -32,13 +43,6 @@ def tree_entries_from_rugged(repository, sha, path, recursive)
                 ordered_entries.concat(tree_entries_from_rugged(repository, sha, entry.path, true))
               end
             end
-
-            # This was an optimization to reduce N+1 queries for Gitaly
-            # (https://gitlab.com/gitlab-org/gitaly/issues/530).  It
-            # used to be done lazily in the view via
-            # TreeHelper#flatten_tree, so it's possible there's a
-            # performance impact by loading this eagerly.
-            rugged_populate_flat_path(repository, sha, path, ordered_entries)
           end
 
           def rugged_populate_flat_path(repository, sha, path, entries)

lib/gitlab/import/merge_request_helpers.rb

@@ -38,7 +38,6 @@ def create_merge_request_without_hooks(project, attributes, iid)
       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 @@ def insert_or_replace_git_data(merge_request, source_branch_sha, target_branch_s
 
         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

lib/tasks/gitlab/db.rake

@@ -29,7 +29,10 @@ namespace :gitlab do
       # If MySQL, turn off foreign key checks
       connection.execute('SET FOREIGN_KEY_CHECKS=0') if Gitlab::Database.mysql?
 
-      tables = connection.data_sources
+      # connection.tables is deprecated in MySQLAdapter, but in PostgreSQLAdapter
+      # data_sources returns both views and tables, so use #tables instead
+      tables = Gitlab::Database.mysql? ? connection.data_sources : connection.tables
+
       # Removes the entry from the array
       tables.delete 'schema_migrations'
       # Truncate schema_migrations to ensure migrations re-run

qa/qa/runtime/api/client.rb

@@ -14,7 +14,7 @@ def initialize(address = :gitlab, personal_access_token: nil, is_new_session: tr
 
         def personal_access_token
           @personal_access_token ||= begin
-            # you can set the environment variable PERSONAL_ACCESS_TOKEN
+            # you can set the environment variable GITLAB_QA_ACCESS_TOKEN
             # to use a specific access token rather than create one from the UI
             Runtime::Env.personal_access_token ||= create_personal_access_token
           end

qa/qa/runtime/env.rb

@@ -53,7 +53,7 @@ def signup_disabled?
 
       # specifies token that can be used for the api
       def personal_access_token
-        @personal_access_token ||= ENV['PERSONAL_ACCESS_TOKEN']
+        @personal_access_token ||= ENV['GITLAB_QA_ACCESS_TOKEN']
       end
 
       def remote_grid

qa/qa/tools/delete_subgroups.rb

@@ -3,7 +3,7 @@
 require_relative '../../qa'
 
 # This script deletes all subgroups of a group specified by ENV['GROUP_NAME_OR_PATH']
-# Required environment variables: PERSONAL_ACCESS_TOKEN and GITLAB_ADDRESS
+# Required environment variables: GITLAB_QA_ACCESS_TOKEN and GITLAB_ADDRESS
 # Optional environment variable: GROUP_NAME_OR_PATH (defaults to 'gitlab-qa-sandbox-group')
 # Run `rake delete_subgroups`
 
@@ -14,9 +14,9 @@ class DeleteSubgroups
 
       def initialize
         raise ArgumentError, "Please provide GITLAB_ADDRESS" unless ENV['GITLAB_ADDRESS']
-        raise ArgumentError, "Please provide PERSONAL_ACCESS_TOKEN" unless ENV['PERSONAL_ACCESS_TOKEN']
+        raise ArgumentError, "Please provide GITLAB_QA_ACCESS_TOKEN" unless ENV['GITLAB_QA_ACCESS_TOKEN']
 
-        @api_client = Runtime::API::Client.new(ENV['GITLAB_ADDRESS'], personal_access_token: ENV['PERSONAL_ACCESS_TOKEN'])
+        @api_client = Runtime::API::Client.new(ENV['GITLAB_ADDRESS'], personal_access_token: ENV['GITLAB_QA_ACCESS_TOKEN'])
       end
 
       def run

qa/qa/tools/generate_perf_testdata.rb

@@ -4,7 +4,7 @@
 require 'faker'
 require_relative '../../qa'
 # This script generates testdata for Performance Testing.
-# Required environment variables: PERSONAL_ACCESS_TOKEN and GITLAB_ADDRESS
+# Required environment variables: GITLAB_QA_ACCESS_TOKEN and GITLAB_ADDRESS
 # This job creates a urls.txt which contains a hash of all the URLs needed for Performance Testing
 # Run `rake generate_perf_testdata`
 
@@ -15,9 +15,9 @@ class GeneratePerfTestdata
 
       def initialize
         raise ArgumentError, "Please provide GITLAB_ADDRESS" unless ENV['GITLAB_ADDRESS']
-        raise ArgumentError, "Please provide PERSONAL_ACCESS_TOKEN" unless ENV['PERSONAL_ACCESS_TOKEN']
+        raise ArgumentError, "Please provide GITLAB_QA_ACCESS_TOKEN" unless ENV['GITLAB_QA_ACCESS_TOKEN']
 
-        @api_client = Runtime::API::Client.new(ENV['GITLAB_ADDRESS'], personal_access_token: ENV['PERSONAL_ACCESS_TOKEN'])
+        @api_client = Runtime::API::Client.new(ENV['GITLAB_ADDRESS'], personal_access_token: ENV['GITLAB_QA_ACCESS_TOKEN'])
         @group_name = "gitlab-qa-perf-sandbox-#{SecureRandom.hex(8)}"
         @project_name = "my-test-project-#{SecureRandom.hex(8)}"
         @urls = {}

qa/spec/runtime/env_spec.rb

@@ -90,13 +90,13 @@
       described_class.instance_variable_set(:@personal_access_token, nil)
     end
 
-    context 'when PERSONAL_ACCESS_TOKEN is set' do
+    context 'when GITLAB_QA_ACCESS_TOKEN is set' do
       before do
-        stub_env('PERSONAL_ACCESS_TOKEN', 'a_token')
+        stub_env('GITLAB_QA_ACCESS_TOKEN', 'a_token_too')
       end
 
       it 'returns specified token from env' do
-        expect(described_class.personal_access_token).to eq 'a_token'
+        expect(described_class.personal_access_token).to eq 'a_token_too'
       end
     end
 

spec/javascripts/diffs/components/diff_file_header_spec.js

@@ -23,9 +23,6 @@ describe('diff_file_header', () => {
   });
 
   beforeEach(() => {
-    gon.features = {
-      expandDiffFullFile: true,
-    };
     const diffFile = diffDiscussionMock.diff_file;
 
     diffFile.added_lines = 2;

spec/lib/gitlab/git/tree_spec.rb

@@ -19,7 +19,7 @@
     it 'returns a list of tree objects' do
       entries = described_class.where(repository, SeedRepo::Commit::ID, 'files', true)
 
-      expect(entries.count).to be > 10
+      expect(entries.count).to be >= 5
       expect(entries).to all(be_a(Gitlab::Git::Tree))
     end
 

spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb

@@ -11,6 +11,7 @@
   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 @@
       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 @@
     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 @@
         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

spec/requests/api/branches_spec.rb

@@ -20,9 +20,9 @@
     let(:route) { "/projects/#{project_id}/repository/branches" }
 
     shared_examples_for 'repository branches' do
-      RSpec::Matchers.define :has_merged_branch_names_count do |expected|
+      RSpec::Matchers.define :has_up_to_merged_branch_names_count do |expected|
         match do |actual|
-          actual[:merged_branch_names].count == expected
+          expected >= actual[:merged_branch_names].count
         end
       end
 
@@ -36,10 +36,30 @@
         expect(branch_names).to match_array(project.repository.branch_names)
       end
 
+      def check_merge_status(json_response)
+        merged, unmerged = json_response.partition { |branch| branch['merged'] }
+        merged_branches = merged.map { |branch| branch['name'] }
+        unmerged_branches = unmerged.map { |branch| branch['name'] }
+        expect(Set.new(merged_branches)).to eq(project.repository.merged_branch_names(merged_branches + unmerged_branches))
+        expect(project.repository.merged_branch_names(unmerged_branches)).to be_empty
+      end
+
       it 'determines only a limited number of merged branch names' do
-        expect(API::Entities::Branch).to receive(:represent).with(anything, has_merged_branch_names_count(2))
+        expect(API::Entities::Branch).to receive(:represent).with(anything, has_up_to_merged_branch_names_count(2)).and_call_original
 
         get api(route, current_user), params: { per_page: 2 }
+
+        expect(response).to have_gitlab_http_status(200)
+
+        check_merge_status(json_response)
+      end
+
+      it 'merge status matches reality on paginated input' do
+        get api(route, current_user), params: { per_page: 20, page: 2 }
+
+        expect(response).to have_gitlab_http_status(200)
+
+        check_merge_status(json_response)
       end
 
       context 'when repository is disabled' do

spec/services/projects/after_import_service_spec.rb

@@ -13,7 +13,7 @@
   describe '#execute' do
     before do
       allow(Projects::HousekeepingService)
-        .to receive(:new).with(project).and_return(housekeeping_service)
+        .to receive(:new).with(project, :gc).and_return(housekeeping_service)
 
       allow(housekeeping_service)
         .to receive(:execute).and_yield

spec/services/projects/housekeeping_service_spec.rb

@@ -88,6 +88,19 @@
         expect(project.pushes_since_gc).to eq(1)
       end
     end
+
+    it 'runs the task specifically requested' do
+      housekeeping = described_class.new(project, :gc)
+
+      allow(housekeeping).to receive(:try_obtain_lease).and_return(:gc_uuid)
+      allow(housekeeping).to receive(:lease_key).and_return(:gc_lease_key)
+
+      expect(GitGarbageCollectWorker).to receive(:perform_async).with(project.id, :gc, :gc_lease_key, :gc_uuid).twice
+
+      2.times do
+        housekeeping.execute
+      end
+    end
   end
 
   describe '#needed?' do