Ignore reverted commits when generating changelogs

When generating changelogs for a range of commits, any commits both
added and reverted in that range are ignored. This works by looking for
commits with the pattern "This reverts commit X", then ignoring the
commits with the mentioned SHA.

Because commits are retrieved in reverse order (= newest first), and
revert commits always come after the commit they revert, we can keep
processing commits in batches; instead of having to first load all of
them in memory. This means the number of Gitaly calls remains the same.

As part of these changes, CommitsWithTrailerFinder is renamed to
ChangelogCommitsFinder, as its purpose is now more specific to the
process of generating changelogs; instead of being a generic "give me
commits with trailer X" finder.

See https://gitlab.com/gitlab-com/gl-infra/delivery/-/issues/1583 for
more information.

app/finders/repositories/commits_with_trailer_finder.rb → app/finders/repositories/changelog_commits_finder.rb

 # frozen_string_literal: true
 
 module Repositories
-  # Finder for obtaining commits between two refs, with a Git trailer set.
-  class CommitsWithTrailerFinder
+  # Finder for getting the commits to include in a changelog.
+  class ChangelogCommitsFinder
     # The maximum number of commits to retrieve per page.
     #
     # This value is arbitrarily chosen. Lowering it means more Gitaly calls, but
@@ -20,6 +20,9 @@ module Repositories
     # 5-10 Gitaly calls, while keeping memory usage at a reasonable amount.
     COMMITS_PER_PAGE = 1024
 
+    # The regex to use for extracting the SHA of a reverted commit.
+    REVERT_REGEX = /^This reverts commit (?<sha>[0-9a-f]{40})/i.freeze
+
     # The `project` argument specifies the project for which to obtain the
     # commits.
     #
@@ -44,7 +47,7 @@ module Repositories
     #
     # Example:
     #
-    #     CommitsWithTrailerFinder.new(...).each_page('Signed-off-by') do |commits|
+    #     ChangelogCommitsFinder.new(...).each_page('Changelog') do |commits|
     #       commits.each do |commit|
     #         ...
     #       end
@@ -53,12 +56,22 @@ module Repositories
       return to_enum(__method__, trailer) unless block_given?
 
       offset = 0
+      reverted = Set.new
       response = fetch_commits
 
       while response.any?
         commits = []
 
         response.each do |commit|
+          # If the commit is reverted in the same range (by a newer commit), we
+          # won't include it. This works here because commits are processed in
+          # reverse order (= newer first).
+          next if reverted.include?(commit.id)
+
+          if (sha = revert_commit_sha(commit))
+            reverted << sha
+          end
+
           commits.push(commit) if commit.trailers.key?(trailer)
         end
 
@@ -78,5 +91,11 @@ module Repositories
         .repository
         .commits(range, limit: @per_page, offset: offset, trailers: true)
     end
+
+    def revert_commit_sha(commit)
+      matches = commit.description.match(REVERT_REGEX)
+
+      matches[:sha] if matches
+    end
   end
 end

app/services/repositories/changelog_service.rb

@@ -73,7 +73,7 @@ module Repositories
         .new(version: @version, date: @date, config: config)
 
       commits =
-        CommitsWithTrailerFinder.new(project: @project, from: from, to: @to)
+        ChangelogCommitsFinder.new(project: @project, from: from, to: @to)
 
       commits.each_page(@trailer) do |page|
         mrs = mrs_finder.execute(page)

changelogs/unreleased/changelog-ignore-reverted-commits.yml

+---
+title: Ignore reverted commits when generating changelogs
+merge_request: 55537
+author:
+type: added

doc/api/repositories.md

@@ -395,6 +395,26 @@ these as the changelog entries. You can enrich entries with additional data,
 such as a link to the merge request or details about the commit author. You can
 [customize the format of a changelog](#customize-the-changelog-output) section with a template.
 
+### Reverted commits
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55537) in GitLab 13.10.
+
+When generating a changelog for a range, GitLab ignores commits both added and
+reverted in that range. Revert commits themselves _are_ included if they use the
+Git trailer used for generating changelogs.
+
+Imagine the following scenario: you have three commits: A, B, and C. To generate
+changelogs, you use the default trailer `Changelog`. Both A and B use this
+trailer. Commit C is a commit that reverts commit B. When generating a changelog
+for this range, GitLab only includes commit A.
+
+Revert commits are detected by looking for commits where the message contains
+the pattern `This reverts commit SHA`, where `SHA` is the SHA of the commit that
+is reverted.
+
+If a revert commit includes the trailer used for generating changelogs
+(`Changelog` in the above example), the revert commit itself _is_ included.
+
 ### Customize the changelog output
 
 The output is customized using a YAML configuration file stored in your

spec/controllers/projects/branches_controller_spec.rb

@@ -648,7 +648,9 @@ RSpec.describe Projects::BranchesController do
       end
 
       it 'sets active and stale branches' do
-        expect(assigns[:active_branches]).to eq([])
+        expect(assigns[:active_branches].map(&:name)).not_to include(
+          "feature", "improve/awesome", "merge-test", "markdown", "feature_conflict", "'test'"
+        )
         expect(assigns[:stale_branches].map(&:name)).to eq(
           ["feature", "improve/awesome", "merge-test", "markdown", "feature_conflict", "'test'"]
         )
@@ -660,7 +662,9 @@ RSpec.describe Projects::BranchesController do
         end
 
         it 'sets active and stale branches' do
-          expect(assigns[:active_branches]).to eq([])
+          expect(assigns[:active_branches].map(&:name)).not_to include(
+            "feature", "improve/awesome", "merge-test", "markdown", "feature_conflict", "'test'"
+          )
           expect(assigns[:stale_branches].map(&:name)).to eq(
             ["feature", "improve/awesome", "merge-test", "markdown", "feature_conflict", "'test'"]
           )

spec/finders/repositories/commits_with_trailer_finder_spec.rb → spec/finders/repositories/changelog_commits_finder_spec.rb

@@ -2,8 +2,8 @@
 
 require 'spec_helper'
 
-RSpec.describe Repositories::CommitsWithTrailerFinder do
-  let(:project) { create(:project, :repository) }
+RSpec.describe Repositories::ChangelogCommitsFinder do
+  let_it_be(:project) { create(:project, :repository) }
 
   describe '#each_page' do
     it 'only yields commits with the given trailer' do
@@ -22,6 +22,35 @@ RSpec.describe Repositories::CommitsWithTrailerFinder do
       )
     end
 
+    it 'ignores commits that are reverted' do
+      # This range of commits is found on the branch
+      # https://gitlab.com/gitlab-org/gitlab-test/-/commits/trailers.
+      finder = described_class.new(
+        project: project,
+        from: 'ddd0f15ae83993f5cb66a927a28673882e99100b',
+        to: '694e6c2f08cad00d183682d9dede99615998a630'
+      )
+
+      commits = finder.each_page('Changelog').to_a.flatten
+
+      expect(commits).to be_empty
+    end
+
+    it 'includes revert commits if they have a trailer' do
+      finder = described_class.new(
+        project: project,
+        from: 'ddd0f15ae83993f5cb66a927a28673882e99100b',
+        to: 'f0a5ed60d24c98ec6d00ac010c1f3f01ee0a8373'
+      )
+
+      initial_commit = project.commit('ed2e92bf50b3da2c7cbbab053f4977a4ecbd109a')
+      revert_commit = project.commit('f0a5ed60d24c98ec6d00ac010c1f3f01ee0a8373')
+
+      commits = finder.each_page('Changelog').to_a.flatten
+
+      expect(commits).to eq([revert_commit, initial_commit])
+    end
+
     it 'supports paginating of commits' do
       finder = described_class.new(
         project: project,

spec/services/projects/branches_by_mode_service_spec.rb

@@ -20,7 +20,7 @@ RSpec.describe Projects::BranchesByModeService do
 
         branches, prev_page, next_page = subject
 
-        expect(branches.size).to eq(10)
+        expect(branches.size).to eq(11)
         expect(next_page).to be_nil
         expect(prev_page).to eq("/#{project.full_path}/-/branches/all?offset=2&page=3")
       end
@@ -99,7 +99,7 @@ RSpec.describe Projects::BranchesByModeService do
         it 'returns branches after the specified branch' do
           branches, prev_page, next_page = subject
 
-          expect(branches.size).to eq(14)
+          expect(branches.size).to eq(15)
           expect(next_page).to be_nil
           expect(prev_page).to eq("/#{project.full_path}/-/branches/all?offset=3&page=4&sort=name_asc")
         end

spec/services/repositories/changelog_service_spec.rb

@@ -80,15 +80,43 @@ RSpec.describe Repositories::ChangelogService do
       expect(changelog).to include('Title 1', 'Title 2')
     end
 
-    it 'uses the target branch when "to" is unspecified' do
-      allow(MergeRequestDiffCommit)
-        .to receive(:oldest_merge_request_id_per_commit)
-        .with(project.id, [commit3.id, commit2.id, commit1.id])
-        .and_return([
-          { sha: sha2, merge_request_id: mr1.id },
-          { sha: sha3, merge_request_id: mr2.id }
-        ])
+    it "ignores a commit when it's both added and reverted in the same range" do
+      create_commit(
+        project,
+        author2,
+        commit_message: "Title 4\n\nThis reverts commit #{sha4}",
+        actions: [{ action: 'create', content: 'bar', file_path: 'd.txt' }]
+      )
+
+      described_class
+        .new(project, creator, version: '1.0.0', from: sha1)
+        .execute
+
+      changelog = project.repository.blob_at('master', 'CHANGELOG.md')&.data
 
+      expect(changelog).to include('Title 1', 'Title 2')
+      expect(changelog).not_to include('Title 3', 'Title 4')
+    end
+
+    it 'includes a revert commit when it has a trailer' do
+      create_commit(
+        project,
+        author2,
+        commit_message: "Title 4\n\nThis reverts commit #{sha4}\n\nChangelog: added",
+        actions: [{ action: 'create', content: 'bar', file_path: 'd.txt' }]
+      )
+
+      described_class
+        .new(project, creator, version: '1.0.0', from: sha1)
+        .execute
+
+      changelog = project.repository.blob_at('master', 'CHANGELOG.md')&.data
+
+      expect(changelog).to include('Title 1', 'Title 2', 'Title 4')
+      expect(changelog).not_to include('Title 3')
+    end
+
+    it 'uses the target branch when "to" is unspecified' do
       described_class
         .new(project, creator, version: '1.0.0', from: sha1)
         .execute

spec/support/helpers/test_env.rb

@@ -77,7 +77,8 @@ module TestEnv
     'sha-starting-with-large-number'     => '8426165',
     'invalid-utf8-diff-paths'            => '99e4853',
     'compare-with-merge-head-source'     => 'f20a03d',
-    'compare-with-merge-head-target'     => '2f1e176'
+    'compare-with-merge-head-target'     => '2f1e176',
+    'trailers'                           => 'f0a5ed6'
   }.freeze
 
   # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily