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

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

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

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

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

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

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

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

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

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

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

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)