Merge branch 'master' into ce-to-ee-2017-12-12

app/models/merge_request.rb

@@ -9,6 +9,7 @@ class MergeRequest < ActiveRecord::Base
   include ManualInverseAssociation
   include EachBatch
   include ThrottledTouch
+  include Gitlab::Utils::StrongMemoize
 
   ignore_column :locked_at,
                 :ref_fetched
@@ -56,6 +57,7 @@ class MergeRequest < ActiveRecord::Base
   serialize :merge_params, Hash # rubocop:disable Cop/ActiveRecordSerialize
 
   after_create :ensure_merge_request_diff, unless: :importing?
+  after_update :clear_memoized_shas
   after_update :reload_diff_if_branch_changed
 
   # When this attribute is true some MR validation is ignored
@@ -404,13 +406,17 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def source_branch_head
-    return unless source_project
-
-    source_project.repository.commit(source_branch_ref) if source_branch_ref
+    strong_memoize(:source_branch_head) do
+      if source_project && source_branch_ref
+        source_project.repository.commit(source_branch_ref)
+      end
+    end
   end
 
   def target_branch_head
-    target_project.repository.commit(target_branch_ref)
+    strong_memoize(:target_branch_head) do
+      target_project.repository.commit(target_branch_ref)
+    end
   end
 
   def branch_merge_base_commit
@@ -558,6 +564,13 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
+  def clear_memoized_shas
+    @target_branch_sha = @source_branch_sha = nil
+
+    clear_memoization(:source_branch_head)
+    clear_memoization(:target_branch_head)
+  end
+
   def reload_diff_if_branch_changed
     if (source_branch_changed? || target_branch_changed?) &&
         (source_branch_head && target_branch_head)

app/models/merge_request_diff.rb

@@ -104,19 +104,19 @@ class MergeRequestDiff < ActiveRecord::Base
   def base_commit
     return unless base_commit_sha
 
-    project.commit(base_commit_sha)
+    project.commit_by(oid: base_commit_sha)
   end
 
   def start_commit
     return unless start_commit_sha
 
-    project.commit(start_commit_sha)
+    project.commit_by(oid: start_commit_sha)
   end
 
   def head_commit
     return unless head_commit_sha
 
-    project.commit(head_commit_sha)
+    project.commit_by(oid: head_commit_sha)
   end
 
   def commit_shas

changelogs/unreleased/zj-memoization-mr-commits.yml

+---
+title: Cache commits for MergeRequest diffs
+merge_request:
+author:
+type: performance

config/application.rb

@@ -51,6 +51,10 @@ module Gitlab
       #{config.root}/ee/app/helpers
     ])
 
+    # Rake tasks ignore the eager loading settings, so we need to set the
+    # autoload paths explicitly
+    config.autoload_paths = config.eager_load_paths.dup
+
     # Only load the plugins named here, in the order given (default is alphabetical).
     # :all can be used as a placeholder for all plugins not explicitly named.
     # config.plugins = [ :exception_notification, :ssl_requirement, :all ]

lib/gitlab/conflict/file_collection.rb

@@ -19,6 +19,8 @@ module Gitlab
           commit_message: commit_message || default_commit_message
         }
         resolver.resolve_conflicts(user, files, args)
+      ensure
+        @merge_request.clear_memoized_shas
       end
 
       def files

lib/gitlab/utils/strong_memoize.rb

@@ -11,6 +11,8 @@ module Gitlab
       #
       # We could write it like:
       #
+      #     include Gitlab::Utils::StrongMemoize
+      #
       #     def trigger_from_token
       #       strong_memoize(:trigger) do
       #         Ci::Trigger.find_by_token(params[:token].to_s)
@@ -18,14 +20,22 @@ module Gitlab
       #     end
       #
       def strong_memoize(name)
-        ivar_name = "@#{name}"
-
-        if instance_variable_defined?(ivar_name)
-          instance_variable_get(ivar_name)
+        if instance_variable_defined?(ivar(name))
+          instance_variable_get(ivar(name))
         else
-          instance_variable_set(ivar_name, yield)
+          instance_variable_set(ivar(name), yield)
         end
       end
+
+      def clear_memoization(name)
+        remove_instance_variable(ivar(name)) if instance_variable_defined?(ivar(name))
+      end
+
+      private
+
+      def ivar(name)
+        "@#{name}"
+      end
     end
   end
 end

lib/tasks/geo.rake

-require 'gitlab/geo'
-require 'gitlab/geo/database_tasks'
-require 'gitlab/geo/geo_tasks'
-
 task spec: ['geo:db:test:prepare']
 
 namespace :geo do
@@ -9,12 +5,12 @@ namespace :geo do
 
   namespace :db do |ns|
     desc 'Drops the Geo tracking database from config/database_geo.yml for the current RAILS_ENV.'
-    task :drop do
+    task drop: [:environment] do
       Gitlab::Geo::DatabaseTasks.drop_current
     end
 
     desc 'Creates the Geo tracking database from config/database_geo.yml for the current RAILS_ENV.'
-    task :create do
+    task create: [:environment] do
       Gitlab::Geo::DatabaseTasks.create_current
     end
 
@@ -60,7 +56,7 @@ namespace :geo do
     end
 
     desc 'Refresh Foreign Tables definition in Geo Secondary node'
-    task :refresh_foreign_tables do
+    task refresh_foreign_tables: [:environment] do
       if Gitlab::Geo::GeoTasks.foreign_server_configured?
         print "\nRefreshing foreign tables for FDW: #{Gitlab::Geo::FDW_SCHEMA} ... "
         Gitlab::Geo::GeoTasks.refresh_foreign_tables!
@@ -72,7 +68,7 @@ namespace :geo do
     end
 
     # IMPORTANT: This task won't dump the schema if ActiveRecord::Base.dump_schema_after_migration is set to false
-    task :_dump do
+    task _dump: [:environment] do
       if Gitlab::Geo::DatabaseTasks.dump_schema_after_migration?
         ns["schema:dump"].invoke
       end
@@ -156,7 +152,7 @@ namespace :geo do
         Gitlab::Geo::DatabaseTasks::Test.purge
       end
 
-      task :refresh_foreign_tables do
+      task refresh_foreign_tables: [:environment] do
         old_env = ActiveRecord::Tasks::DatabaseTasks.env
         ActiveRecord::Tasks::DatabaseTasks.env = 'test'
 

spec/lib/gitlab/utils/strong_memoize_spec.rb

@@ -49,4 +49,16 @@ describe Gitlab::Utils::StrongMemoize do
       end
     end
   end
+
+  describe '#clear_memoization' do
+    let(:value) { 'mepmep' }
+
+    it 'removes the instance variable' do
+      object.method_name
+
+      object.clear_memoization(:method_name)
+
+      expect(object.instance_variable_defined?(:@method_name)).to be(false)
+    end
+  end
 end

spec/models/merge_request_spec.rb

@@ -124,6 +124,7 @@ describe MergeRequest do
     context 'when the target branch does not exist' do
       before do
         project.repository.rm_branch(subject.author, subject.target_branch)
+        subject.clear_memoized_shas
       end
 
       it 'returns nil' do
@@ -809,30 +810,30 @@ describe MergeRequest do
   end
 
   describe '#can_remove_source_branch?' do
-    let(:user) { create(:user) }
-    let(:user2) { create(:user) }
+    set(:user) { create(:user) }
+    set(:merge_request) { create(:merge_request, :simple) }
 
-    before do
-      subject.source_project.team << [user, :master]
+    subject { merge_request }
 
-      subject.source_branch = "feature"
-      subject.target_branch = "master"
-      subject.save!
+    before do
+      subject.source_project.add_master(user)
     end
 
     it "can't be removed when its a protected branch" do
       allow(ProtectedBranch).to receive(:protected?).and_return(true)
+
       expect(subject.can_remove_source_branch?(user)).to be_falsey
     end
 
     it "can't remove a root ref" do
-      subject.source_branch = "master"
-      subject.target_branch = "feature"
+      subject.update(source_branch: 'master', target_branch: 'feature')
 
       expect(subject.can_remove_source_branch?(user)).to be_falsey
     end
 
     it "is unable to remove the source branch for a project the user cannot push to" do
+      user2 = create(:user)
+
       expect(subject.can_remove_source_branch?(user2)).to be_falsey
     end
 
@@ -843,6 +844,7 @@ describe MergeRequest do
     end
 
     it "cannot be removed if the last commit is not also the head of the source branch" do
+      subject.clear_memoized_shas
       subject.source_branch = "lfs"
 
       expect(subject.can_remove_source_branch?(user)).to be_falsey
@@ -942,7 +944,7 @@ describe MergeRequest do
 
       before do
         project.repository.raw_repository.delete_branch(subject.target_branch)
-        subject.reload
+        subject.clear_memoized_shas
       end
 
       it 'does not crash' do
@@ -1633,6 +1635,16 @@ describe MergeRequest do
 
       subject.reload_diff
     end
+
+    context 'when using the after_update hook to update' do
+      context 'when the branches are updated' do
+        it 'uses the new heads to generate the diff' do
+          expect { subject.update!(source_branch: subject.target_branch, target_branch: subject.source_branch) }
+            .to change { subject.merge_request_diff.start_commit_sha }
+            .and change { subject.merge_request_diff.head_commit_sha }
+        end
+      end
+    end
   end
 
   describe '#update_diff_discussion_positions' do
@@ -1895,6 +1907,7 @@ describe MergeRequest do
     context 'when the target branch does not exist' do
       before do
         subject.project.repository.rm_branch(subject.author, subject.target_branch)
+        subject.clear_memoized_shas
       end
 
       it 'returns nil' do

spec/services/merge_requests/merge_service_spec.rb

 require 'spec_helper'
 
 describe MergeRequests::MergeService do
-  let(:user) { create(:user) }
-  let(:user2) { create(:user) }
+  set(:user) { create(:user) }
+  set(:user2) { create(:user) }
   let(:merge_request) { create(:merge_request, :simple, author: user2, assignee: user2) }
   let(:project) { merge_request.project }
 
   before do
-    project.team << [user, :master]
-    project.team << [user2, :developer]
+    project.add_master(user)
+    project.add_developer(user2)
   end
 
   describe '#execute' do