Merge branch 'remove-ensure-ref-fetched-from-controllers' into 'master'
removed the #ensure_ref_fetched from all controllers Closes #36061 See merge request gitlab-org/gitlab-ce!15129
This commit is contained in:
commit
045795d0d9
|
@ -2,7 +2,6 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont
|
|||
before_action :check_merge_requests_available!
|
||||
before_action :merge_request
|
||||
before_action :authorize_read_merge_request!
|
||||
before_action :ensure_ref_fetched
|
||||
|
||||
private
|
||||
|
||||
|
@ -10,12 +9,6 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont
|
|||
@issuable = @merge_request ||= @project.merge_requests.find_by!(iid: params[:id])
|
||||
end
|
||||
|
||||
# Make sure merge requests created before 8.0
|
||||
# have head file in refs/merge-requests/
|
||||
def ensure_ref_fetched
|
||||
@merge_request.ensure_ref_fetched if Gitlab::Database.read_write?
|
||||
end
|
||||
|
||||
def merge_request_params
|
||||
params.require(:merge_request).permit(merge_request_params_attributes)
|
||||
end
|
||||
|
|
|
@ -4,7 +4,6 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
|
|||
include RendersCommits
|
||||
|
||||
skip_before_action :merge_request
|
||||
skip_before_action :ensure_ref_fetched
|
||||
before_action :authorize_create_merge_request!
|
||||
before_action :apply_diff_view_cookie!, only: [:diffs, :diff_for_path]
|
||||
before_action :build_merge_request, except: [:create]
|
||||
|
|
|
@ -7,7 +7,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
|
|||
include IssuableCollections
|
||||
|
||||
skip_before_action :merge_request, only: [:index, :bulk_update]
|
||||
skip_before_action :ensure_ref_fetched, only: [:index, :bulk_update]
|
||||
|
||||
before_action :authorize_update_issuable!, only: [:close, :edit, :update, :remove_wip, :sort]
|
||||
|
||||
|
@ -52,7 +51,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
|
|||
|
||||
def show
|
||||
validates_merge_request
|
||||
ensure_ref_fetched
|
||||
close_merge_request_without_source_project
|
||||
check_if_can_be_merged
|
||||
|
||||
|
|
|
@ -21,8 +21,8 @@ module IgnorableColumn
|
|||
@ignored_columns ||= Set.new
|
||||
end
|
||||
|
||||
def ignore_column(name)
|
||||
ignored_columns << name.to_s
|
||||
def ignore_column(*names)
|
||||
ignored_columns.merge(names.map(&:to_s))
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -8,7 +8,8 @@ class MergeRequest < ActiveRecord::Base
|
|||
include CreatedAtFilterable
|
||||
include TimeTrackable
|
||||
|
||||
ignore_column :locked_at
|
||||
ignore_column :locked_at,
|
||||
:ref_fetched
|
||||
|
||||
belongs_to :target_project, class_name: "Project"
|
||||
belongs_to :source_project, class_name: "Project"
|
||||
|
@ -426,7 +427,7 @@ class MergeRequest < ActiveRecord::Base
|
|||
end
|
||||
|
||||
def create_merge_request_diff
|
||||
fetch_ref
|
||||
fetch_ref!
|
||||
|
||||
# n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37435
|
||||
Gitlab::GitalyClient.allow_n_plus_1_calls do
|
||||
|
@ -811,29 +812,14 @@ class MergeRequest < ActiveRecord::Base
|
|||
end
|
||||
end
|
||||
|
||||
def fetch_ref
|
||||
write_ref
|
||||
update_column(:ref_fetched, true)
|
||||
def fetch_ref!
|
||||
target_project.repository.fetch_source_branch!(source_project.repository, source_branch, ref_path)
|
||||
end
|
||||
|
||||
def ref_path
|
||||
"refs/#{Repository::REF_MERGE_REQUEST}/#{iid}/head"
|
||||
end
|
||||
|
||||
def ref_fetched?
|
||||
super ||
|
||||
begin
|
||||
computed_value = project.repository.ref_exists?(ref_path)
|
||||
update_column(:ref_fetched, true) if computed_value
|
||||
|
||||
computed_value
|
||||
end
|
||||
end
|
||||
|
||||
def ensure_ref_fetched
|
||||
fetch_ref unless ref_fetched?
|
||||
end
|
||||
|
||||
def in_locked_state
|
||||
begin
|
||||
lock_mr
|
||||
|
@ -975,10 +961,4 @@ class MergeRequest < ActiveRecord::Base
|
|||
|
||||
project.merge_requests.merged.where(author_id: author_id).empty?
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def write_ref
|
||||
target_project.repository.fetch_source_branch(source_project.repository, source_branch, ref_path)
|
||||
end
|
||||
end
|
||||
|
|
|
@ -969,8 +969,8 @@ class Repository
|
|||
gitlab_shell.fetch_remote(raw_repository, remote, ssh_auth: ssh_auth, forced: forced, no_tags: no_tags)
|
||||
end
|
||||
|
||||
def fetch_source_branch(source_repository, source_branch, local_ref)
|
||||
raw_repository.fetch_source_branch(source_repository.raw_repository, source_branch, local_ref)
|
||||
def fetch_source_branch!(source_repository, source_branch, local_ref)
|
||||
raw_repository.fetch_source_branch!(source_repository.raw_repository, source_branch, local_ref)
|
||||
end
|
||||
|
||||
def compare_source_branch(target_branch_name, source_repository, source_branch_name, straight:)
|
||||
|
|
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
title: Stop merge requests from fetching their refs when the data is already available.
|
||||
merge_request: 15129
|
||||
author:
|
||||
type: removed
|
|
@ -115,10 +115,6 @@ def instrument_classes(instrumentation)
|
|||
|
||||
# Needed for https://gitlab.com/gitlab-org/gitlab-ce/issues/30224#note_32306159
|
||||
instrumentation.instrument_instance_method(MergeRequestDiff, :load_commits)
|
||||
|
||||
# Needed for https://gitlab.com/gitlab-org/gitlab-ce/issues/36061
|
||||
instrumentation.instrument_instance_method(MergeRequest, :ensure_ref_fetched)
|
||||
instrumentation.instrument_instance_method(MergeRequest, :fetch_ref)
|
||||
end
|
||||
# rubocop:enable Metrics/AbcSize
|
||||
|
||||
|
|
|
@ -0,0 +1,14 @@
|
|||
class RemoveRefFetchedFromMergeRequests < ActiveRecord::Migration
|
||||
include Gitlab::Database::MigrationHelpers
|
||||
|
||||
# Set this constant to true if this migration requires downtime.
|
||||
DOWNTIME = false
|
||||
|
||||
# We don't need to cache this anymore: the refs are now created
|
||||
# upon save/update and there is no more use for this flag
|
||||
#
|
||||
# See https://gitlab.com/gitlab-org/gitlab-ce/issues/36061
|
||||
def change
|
||||
remove_column :merge_requests, :ref_fetched, :boolean
|
||||
end
|
||||
end
|
|
@ -11,7 +11,7 @@
|
|||
#
|
||||
# It's strongly recommended that you check this file into your version control system.
|
||||
|
||||
ActiveRecord::Schema.define(version: 20171026082505) do
|
||||
ActiveRecord::Schema.define(version: 20171101134435) do
|
||||
|
||||
# These are extensions that must be enabled in order to support this database
|
||||
enable_extension "plpgsql"
|
||||
|
@ -970,7 +970,6 @@ ActiveRecord::Schema.define(version: 20171026082505) do
|
|||
t.datetime "last_edited_at"
|
||||
t.integer "last_edited_by_id"
|
||||
t.integer "head_pipeline_id"
|
||||
t.boolean "ref_fetched"
|
||||
t.string "merge_jid"
|
||||
t.boolean "discussion_locked"
|
||||
t.integer "latest_merge_request_diff_id"
|
||||
|
|
|
@ -163,7 +163,6 @@ module Github
|
|||
iid: pull_request.iid,
|
||||
title: pull_request.title,
|
||||
description: description,
|
||||
ref_fetched: true,
|
||||
source_project: pull_request.source_project,
|
||||
source_branch: pull_request.source_branch_name,
|
||||
source_branch_sha: pull_request.source_branch_sha,
|
||||
|
|
|
@ -1044,7 +1044,7 @@ module Gitlab
|
|||
delete_refs(tmp_ref) if tmp_ref
|
||||
end
|
||||
|
||||
def fetch_source_branch(source_repository, source_branch, local_ref)
|
||||
def fetch_source_branch!(source_repository, source_branch, local_ref)
|
||||
with_repo_branch_commit(source_repository, source_branch) do |commit|
|
||||
if commit
|
||||
write_ref(local_ref, commit.sha)
|
||||
|
|
|
@ -19,7 +19,6 @@ module Gitlab
|
|||
merge_user_id
|
||||
merge_when_pipeline_succeeds
|
||||
milestone_id
|
||||
ref_fetched
|
||||
source_branch
|
||||
source_project_id
|
||||
state
|
||||
|
|
|
@ -83,10 +83,10 @@ FactoryGirl.define do
|
|||
target_project = merge_request.target_project
|
||||
source_project = merge_request.source_project
|
||||
|
||||
# Fake `write_ref` if we don't have repository
|
||||
# Fake `fetch_ref!` if we don't have repository
|
||||
# We have too many existing tests replying on this behaviour
|
||||
unless [target_project, source_project].all?(&:repository_exists?)
|
||||
allow(merge_request).to receive(:write_ref)
|
||||
allow(merge_request).to receive(:fetch_ref!)
|
||||
end
|
||||
end
|
||||
|
||||
|
|
|
@ -1521,7 +1521,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
|
|||
end
|
||||
end
|
||||
|
||||
describe '#fetch_source_branch' do
|
||||
describe '#fetch_source_branch!' do
|
||||
let(:local_ref) { 'refs/merge-requests/1/head' }
|
||||
|
||||
context 'when the branch exists' do
|
||||
|
@ -1530,11 +1530,11 @@ describe Gitlab::Git::Repository, seed_helper: true do
|
|||
it 'writes the ref' do
|
||||
expect(repository).to receive(:write_ref).with(local_ref, /\h{40}/)
|
||||
|
||||
repository.fetch_source_branch(repository, source_branch, local_ref)
|
||||
repository.fetch_source_branch!(repository, source_branch, local_ref)
|
||||
end
|
||||
|
||||
it 'returns true' do
|
||||
expect(repository.fetch_source_branch(repository, source_branch, local_ref)).to eq(true)
|
||||
expect(repository.fetch_source_branch!(repository, source_branch, local_ref)).to eq(true)
|
||||
end
|
||||
end
|
||||
|
||||
|
@ -1544,11 +1544,11 @@ describe Gitlab::Git::Repository, seed_helper: true do
|
|||
it 'does not write the ref' do
|
||||
expect(repository).not_to receive(:write_ref)
|
||||
|
||||
repository.fetch_source_branch(repository, source_branch, local_ref)
|
||||
repository.fetch_source_branch!(repository, source_branch, local_ref)
|
||||
end
|
||||
|
||||
it 'returns false' do
|
||||
expect(repository.fetch_source_branch(repository, source_branch, local_ref)).to eq(false)
|
||||
expect(repository.fetch_source_branch!(repository, source_branch, local_ref)).to eq(false)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -26,7 +26,6 @@ describe Gitlab::HookData::MergeRequestBuilder do
|
|||
merge_user_id
|
||||
merge_when_pipeline_succeeds
|
||||
milestone_id
|
||||
ref_fetched
|
||||
source_branch
|
||||
source_project_id
|
||||
state
|
||||
|
|
|
@ -46,7 +46,7 @@ describe 'forked project import' do
|
|||
end
|
||||
|
||||
it 'can access the MR' do
|
||||
project.merge_requests.first.ensure_ref_fetched
|
||||
project.merge_requests.first.fetch_ref!
|
||||
|
||||
expect(project.repository.ref_exists?('refs/merge-requests/1/head')).to be_truthy
|
||||
end
|
||||
|
|
|
@ -5,7 +5,11 @@ describe IgnorableColumn do
|
|||
Class.new do
|
||||
def self.columns
|
||||
# This method does not have access to "double"
|
||||
[Struct.new(:name).new('id'), Struct.new(:name).new('title')]
|
||||
[
|
||||
Struct.new(:name).new('id'),
|
||||
Struct.new(:name).new('title'),
|
||||
Struct.new(:name).new('date')
|
||||
]
|
||||
end
|
||||
end
|
||||
end
|
||||
|
@ -18,7 +22,7 @@ describe IgnorableColumn do
|
|||
|
||||
describe '.columns' do
|
||||
it 'returns the columns, excluding the ignored ones' do
|
||||
model.ignore_column(:title)
|
||||
model.ignore_column(:title, :date)
|
||||
|
||||
expect(model.columns.map(&:name)).to eq(%w(id))
|
||||
end
|
||||
|
@ -30,9 +34,9 @@ describe IgnorableColumn do
|
|||
end
|
||||
|
||||
it 'returns the names of the ignored columns' do
|
||||
model.ignore_column(:title)
|
||||
model.ignore_column(:title, :date)
|
||||
|
||||
expect(model.ignored_columns).to eq(Set.new(%w(title)))
|
||||
expect(model.ignored_columns).to eq(Set.new(%w(title date)))
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -1755,39 +1755,12 @@ describe MergeRequest do
|
|||
end
|
||||
end
|
||||
|
||||
describe '#fetch_ref' do
|
||||
it 'sets "ref_fetched" flag to true' do
|
||||
subject.update!(ref_fetched: nil)
|
||||
describe '#fetch_ref!' do
|
||||
it 'fetches the ref correctly' do
|
||||
expect { subject.target_project.repository.delete_refs(subject.ref_path) }.not_to raise_error
|
||||
|
||||
subject.fetch_ref
|
||||
|
||||
expect(subject.reload.ref_fetched).to be_truthy
|
||||
end
|
||||
end
|
||||
|
||||
describe '#ref_fetched?' do
|
||||
it 'does not perform git operation when value is cached' do
|
||||
subject.ref_fetched = true
|
||||
|
||||
expect_any_instance_of(Repository).not_to receive(:ref_exists?)
|
||||
expect(subject.ref_fetched?).to be_truthy
|
||||
end
|
||||
|
||||
it 'caches the value when ref exists but value is not cached' do
|
||||
subject.update!(ref_fetched: nil)
|
||||
allow_any_instance_of(Repository).to receive(:ref_exists?)
|
||||
.and_return(true)
|
||||
|
||||
expect(subject.ref_fetched?).to be_truthy
|
||||
expect(subject.reload.ref_fetched).to be_truthy
|
||||
end
|
||||
|
||||
it 'returns false when ref does not exist' do
|
||||
subject.update!(ref_fetched: nil)
|
||||
allow_any_instance_of(Repository).to receive(:ref_exists?)
|
||||
.and_return(false)
|
||||
|
||||
expect(subject.ref_fetched?).to be_falsey
|
||||
subject.fetch_ref!
|
||||
expect(subject.target_project.repository.ref_exists?(subject.ref_path)).to be_truthy
|
||||
end
|
||||
end
|
||||
|
||||
|
|
|
@ -623,8 +623,6 @@ describe API::MergeRequests do
|
|||
|
||||
before do
|
||||
forked_project.add_reporter(user2)
|
||||
|
||||
allow_any_instance_of(MergeRequest).to receive(:write_ref)
|
||||
end
|
||||
|
||||
it "returns merge_request" do
|
||||
|
|
|
@ -319,8 +319,6 @@ describe API::MergeRequests do
|
|||
|
||||
before do
|
||||
forked_project.add_reporter(user2)
|
||||
|
||||
allow_any_instance_of(MergeRequest).to receive(:write_ref)
|
||||
end
|
||||
|
||||
it "returns merge_request" do
|
||||
|
|
Loading…
Reference in New Issue