Merge branch '13524-keep-around-commits' into 'master'

Don't garbage collect commits that have related DB records like comments

Closes #13524

Also needed for https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4101.

See merge request !5062

12 files changed, 88 insertions, 7 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 4f9654dfbef..5f593336895 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -31,6 +31,7 @@ v 8.10.0 (unreleased)
   - Metrics for Rouge::Plugins::Redcarpet and Rouge::Formatters::HTMLGitlab
   - Allow [ci skip] to be in any case and allow [skip ci]. !4785 (simon_w)
   - Add basic system information like memory and disk usage to the admin panel
+  - Don't garbage collect commits that have related DB records like comments
 
 v 8.9.5 (unreleased)
   - Improve the request / withdraw access button. !4860
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 10324bf2257..fa4071e2482 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -16,6 +16,7 @@ module Ci
 
     # Invalidate object and save if when touched
     after_touch :update_state
+    after_save :keep_around_commits
 
     def self.truncate_sha(sha)
       sha[0...8]
@@ -212,5 +213,10 @@ module Ci
       self.duration = statuses.latest.duration
       save
     end
+
+    def keep_around_commits
+      project.repository.keep_around(self.sha)
+      project.repository.keep_around(self.before_sha)
+    end
   end
 end
diff --git a/app/models/ci/trigger_request.rb b/app/models/ci/trigger_request.rb
index b69ae37668c..fcf2b6dc5e2 100644
--- a/app/models/ci/trigger_request.rb
+++ b/app/models/ci/trigger_request.rb
@@ -1,7 +1,7 @@
 module Ci
   class TriggerRequest < ActiveRecord::Base
     extend Ci::Model
-    
+
     belongs_to :trigger, class_name: 'Ci::Trigger'
     belongs_to :pipeline, class_name: 'Ci::Pipeline', foreign_key: :commit_id
     has_many :builds, class_name: 'Ci::Build'
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 5ebc8f0c99f..cb0f871897a 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -117,6 +117,8 @@ class MergeRequest < ActiveRecord::Base
   scope :join_project, -> { joins(:target_project) }
   scope :references_project, -> { references(:target_project) }
 
+  after_save :keep_around_commit
+
   def self.reference_prefix
     '!'
   end
@@ -536,12 +538,12 @@ class MergeRequest < ActiveRecord::Base
     "refs/merge-requests/#{iid}/head"
   end
 
-  def ref_is_fetched?
-    File.exist?(File.join(project.repository.path_to_repo, ref_path))
+  def ref_fetched?
+    project.repository.ref_exists?(ref_path)
   end
 
   def ensure_ref_fetched
-    fetch_ref unless ref_is_fetched?
+    fetch_ref unless ref_fetched?
   end
 
   def in_locked_state
@@ -600,4 +602,8 @@ class MergeRequest < ActiveRecord::Base
   def can_be_cherry_picked?
     merge_commit
   end
+
+  def keep_around_commit
+    project.repository.keep_around(self.merge_commit_sha)
+  end
 end
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index 86331a33c05..0fcde6fc8f1 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -24,6 +24,7 @@ class MergeRequestDiff < ActiveRecord::Base
   serialize :st_diffs
 
   after_create :reload_content, unless: :importing?
+  after_save :keep_around_commit
 
   def reload_content
     reload_commits
@@ -145,7 +146,11 @@ class MergeRequestDiff < ActiveRecord::Base
     end
 
     new_attributes[:st_diffs] = new_diffs
-    new_attributes[:base_commit_sha] = self.repository.merge_base(self.head, self.base)
+
+    base_commit_sha = self.repository.merge_base(self.head, self.base)
+    new_attributes[:base_commit_sha] = base_commit_sha
+
+    self.repository.keep_around(base_commit_sha)
 
     update_columns_serialized(new_attributes)
   end
@@ -217,4 +222,8 @@ class MergeRequestDiff < ActiveRecord::Base
     update_columns(new_attributes.merge(updated_at: current_time_from_proper_timezone))
     reload
   end
+
+  def keep_around_commit
+    self.repository.keep_around(self.base_commit_sha)
+  end
 end
diff --git a/app/models/note.rb b/app/models/note.rb
index c2bb117eb03..81b5c47b738 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -66,6 +66,7 @@ class Note < ActiveRecord::Base
   end
 
   before_validation :clear_blank_line_code!
+  after_save :keep_around_commit
 
   class << self
     def model_name
@@ -215,4 +216,10 @@ class Note < ActiveRecord::Base
     original_name = note.match(Banzai::Filter::EmojiFilter.emoji_pattern)[1]
     Gitlab::AwardEmoji.normalize_emoji_name(original_name)
   end
+
+  private
+
+  def keep_around_commit
+    project.repository.keep_around(self.commit_id)
+  end
 end
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 11ecb281a55..078ca8f4e13 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -203,6 +203,26 @@ class Repository
     branch_names.include?(branch_name)
   end
 
+  def ref_exists?(ref)
+    rugged.references.exist?(ref)
+  end
+
+  # Makes sure a commit is kept around when Git garbage collection runs.
+  # Git GC will delete commits from the repository that are no longer in any
+  # branches or tags, but we want to keep some of these commits around, for
+  # example if they have comments or CI builds.
+  def keep_around(sha)
+    return unless sha && commit(sha)
+
+    return if kept_around?(sha)
+
+    rugged.references.create(keep_around_ref_name(sha), sha)
+  end
+
+  def kept_around?(sha)
+    ref_exists?(keep_around_ref_name(sha))
+  end
+
   def tag_names
     cache.fetch(:tag_names) { raw_repository.tag_names }
   end
@@ -1018,4 +1038,8 @@ class Repository
   def tags_sorted_by_committed_date
     tags.sort_by { |tag| commit(tag.target).committed_date }
   end
+
+  def keep_around_ref_name(sha)
+    "refs/keep-around/#{sha}"
+  end
 end
diff --git a/app/models/sent_notification.rb b/app/models/sent_notification.rb
index 375f195dba7..a2df899d012 100644
--- a/app/models/sent_notification.rb
+++ b/app/models/sent_notification.rb
@@ -9,6 +9,8 @@ class SentNotification < ActiveRecord::Base
   validates :commit_id, presence: true, if: :for_commit?
   validates :line_code, line_code: true, allow_blank: true
 
+  after_save :keep_around_commit
+
   class << self
     def reply_key
       SecureRandom.hex(16)
@@ -67,4 +69,10 @@ class SentNotification < ActiveRecord::Base
   def to_param
     self.reply_key
   end
+
+  private
+
+  def keep_around_commit
+    project.repository.keep_around(self.commit_id)
+  end
 end
diff --git a/app/models/todo.rb b/app/models/todo.rb
index 3ba67078d48..ac3fdbc7f3b 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -37,6 +37,8 @@ class Todo < ActiveRecord::Base
     state :done
   end
 
+  after_save :keep_around_commit
+
   def build_failed?
     action == BUILD_FAILED
   end
@@ -73,4 +75,10 @@ class Todo < ActiveRecord::Base
       target.to_reference
     end
   end
+
+  private
+
+  def keep_around_commit
+    project.repository.keep_around(self.commit_id)
+  end
 end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 3b199f4d98d..ceb4d64698f 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -464,7 +464,7 @@ describe MergeRequest, models: true do
     context 'when it is not broken and has no conflicts' do
       it 'is marked as mergeable' do
         allow(subject).to receive(:broken?) { false }
-        allow(project).to receive_message_chain(:repository, :can_be_merged?) { true }
+        allow(project.repository).to receive(:can_be_merged?) { true }
 
         expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('can_be_merged')
       end
@@ -481,7 +481,7 @@ describe MergeRequest, models: true do
     context 'when it has conflicts' do
       before do
         allow(subject).to receive(:broken?) { false }
-        allow(project).to receive_message_chain(:repository, :can_be_merged?) { false }
+        allow(project.repository).to receive(:can_be_merged?) { false }
       end
 
       it 'becomes unmergeable' do
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 285ab19cfaf..6549791f675 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -70,6 +70,10 @@ describe Note, models: true do
     it "should be recognized by #for_commit?" do
       expect(note).to be_for_commit
     end
+
+    it "keeps the commit around" do
+      expect(note.project.repository.kept_around?(commit.id)).to be_truthy
+    end
   end
 
   describe 'authorization' do
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 851b8b241d7..e753306a31f 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1117,6 +1117,14 @@ describe Repository, models: true do
     end
   end
 
+  describe "#keep_around" do
+    it "stores a reference to the specified commit sha so it isn't garbage collected" do
+      repository.keep_around(sample_commit.id)
+
+      expect(repository.kept_around?(sample_commit.id)).to be_truthy
+    end
+  end
+
   def create_remote_branch(remote_name, branch_name, target)
     rugged = repository.rugged
     rugged.references.create("refs/remotes/#{remote_name}/#{branch_name}", target)