Merge branch 'changelog-ignore-reverted-commits' into 'master'

Ignore reverted commits when generating changelogs

See merge request gitlab-org/gitlab!55537

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