Fix PR validity check in create_release_backmerge_pr (#607)

Author: AliSoftware

CHANGELOG.md

@@ -14,7 +14,7 @@ _None_
 
 ### Bug Fixes
 
-_None_
+- `create_release-backmerge_pull_request`: Fix the pre-check logic verifying if a PR is really needed or if there's nothing to backmerge. [#607]
 
 ### Internal Changes
 

lib/fastlane/plugin/wpmreleasetoolkit/actions/common/create_release_backmerge_pull_request_action.rb

@@ -89,24 +89,33 @@ def self.determine_target_branches(source_release_version:, default_branch:)
       # @return [String] The URL of the created Pull Request, or `nil` if no PR was created.
       #
       def self.create_backmerge_pr(token:, repository:, title:, head_branch:, base_branch:, labels:, milestone:, reviewers:, team_reviewers:, intermediate_branch_created_callback:)
-        intermediate_branch = "merge/#{head_branch.gsub('/', '-')}-into-#{base_branch.gsub('/', '-')}"
+        # Do an early pre-check to see if the PR would be valid, but only if no callback (as a callback might add new commits on intermediate branch)
+        if intermediate_branch_created_callback.nil? && !can_merge?(head_branch, into: base_branch)
+          UI.error("Nothing to merge from #{head_branch} into #{base_branch}. Skipping PR creation.")
+          return nil
+        end
 
+        # Create the intermediate branch
+        intermediate_branch = "merge/#{head_branch.gsub('/', '-')}-into-#{base_branch.gsub('/', '-')}"
         if Fastlane::Helper::GitHelper.branch_exists_on_remote?(branch_name: intermediate_branch)
           UI.important("An intermediate branch `#{intermediate_branch}` already exists on the remote. It will be deleted and GitHub will close any associated existing PR.")
           Fastlane::Helper::GitHelper.delete_remote_branch_if_exists!(intermediate_branch)
         end
-
         Fastlane::Helper::GitHelper.delete_local_branch_if_exists!(intermediate_branch)
         Fastlane::Helper::GitHelper.create_branch(intermediate_branch)
 
-        intermediate_branch_created_callback&.call(base_branch, intermediate_branch)
-
-        # if there's a callback, make sure it didn't switch branches
-        other_action.ensure_git_branch(branch: "^#{intermediate_branch}/") unless intermediate_branch_created_callback.nil?
-
-        if Fastlane::Helper::GitHelper.point_to_same_commit?(base_branch, head_branch)
-          UI.error("No differences between #{head_branch} and #{base_branch}. Skipping PR creation.")
-          return nil
+        # Call the callback if one was provided to allow the use to add commits on the intermediate branch (e.g. solve conflicts)
+        unless intermediate_branch_created_callback.nil?
+          intermediate_branch_created_callback.call(base_branch, intermediate_branch)
+          # Make sure the callback block didn't switch branches
+          other_action.ensure_git_branch(branch: "^#{intermediate_branch}$")
+
+          # When a callback was provided, do the pre-check about valid PR _only_ at that point, in case the callback added new commits
+          unless can_merge?(intermediate_branch, into: base_branch)
+            UI.error("Nothing to merge from #{intermediate_branch} into #{base_branch}. Skipping PR creation.")
+            Fastlane::Helper::GitHelper.delete_local_branch_if_exists!(intermediate_branch)
+            return nil
+          end
         end
 
         other_action.push_to_git_remote(tags: false)
@@ -138,6 +147,23 @@ def self.create_backmerge_pr(token:, repository:, title:, head_branch:, base_bra
         )
       end
 
+      # Determine if a `head->base` PR would be considered valid by GitHub.
+      #
+      # Note that a PR with an empty diff can still be valid (e.g. if you merge a commit and its revert)
+      #
+      # This method returns false mostly when all commits from `head` has already been merged into `base`
+      # and that there are no new commits to merge (in which case GitHub would refuse creating the PR)
+      #
+      # @param head [String] the head reference (commit sha or branch name) we want to merge
+      # @param into [String] the base reference (commit sha or branch name) we want to merge into
+      # @return [Boolean] true if there are commits in `head` that are not yet in `base` and a merge can happen;
+      #                   false if all commits from `head` are already in `base` and a merge would be rejected
+      #
+      def self.can_merge?(head, into:)
+        merge_base = Fastlane::Helper::GitHelper.find_merge_base(into, head)
+        !Fastlane::Helper::GitHelper.point_to_same_commit?(merge_base, head)
+      end
+
       def self.description
         'Creates backmerge PRs for a release branch into target branches'
       end
@@ -201,7 +227,9 @@ def self.available_options
                                        optional: true,
                                        type: Array),
           FastlaneCore::ConfigItem.new(key: :intermediate_branch_created_callback,
-                                       description: 'Callback to allow for the caller to perform operations on the intermediate branch before pushing. The call back receives two parameters: the base (target) branch for the PR and the intermediate branch name',
+                                       description: 'Callback to allow for the caller to perform operations on the intermediate branch (e.g. pushing new commits to pre-solve conflicts) before creating the PR. ' \
+                                        + 'The callback receives two parameters: the base (target) branch for the PR and the intermediate branch name that has been created.' \
+                                        + 'Note that if you use the callback to add new commits to the intermediate branch, you are responsible for git-pushing them too',
                                        optional: true,
                                        type: Proc),
           Fastlane::Helper::GithubHelper.github_token_config_item,

lib/fastlane/plugin/wpmreleasetoolkit/helper/git_helper.rb

@@ -113,11 +113,19 @@ def self.commit(message:, files: nil)
 
       # Get the SHA of a given git ref. Typically useful to get the SHA of the current HEAD commit.
       #
-      # @param [String] ref The git ref (commit, branch name, 'HEAD', …) to resolve as a SHA
+      # @param ref [String]
+      #        The git ref (commit, branch name, 'HEAD', …) to resolve as a SHA
+      # @param prepend_origin_if_needed [Boolean]
+      #        If true, will retry the rev-parse by prefixing `origin/` to the ref it it failed without it
       # @return [String] The commit SHA of the ref
       #
-      def self.get_commit_sha(ref: 'HEAD')
-        Git.open(Dir.pwd).revparse(ref)
+      def self.get_commit_sha(ref: 'HEAD', prepend_origin_if_needed: false)
+        repo = Git.open(Dir.pwd)
+        repo.revparse(ref)
+      rescue Git::FailedError
+        raise unless prepend_origin_if_needed
+
+        repo.revparse("origin/#{ref}")
       end
 
       # Creates a tag for the given version, and optionally push it to the remote.
@@ -170,28 +178,36 @@ def self.fetch_all_tags
         Action.sh('git', 'fetch', '--tags')
       end
 
+      # Use `git merge-base` to find as good a common ancestors as possible for a merge
+      #
+      # @param ref1 [String] The first git reference (sha1, ref name…)to find the common ancestor of
+      # @param ref2 [String] The second git reference (sha1, ref name…)to find the common ancestor of
+      # @return [String] The merge-base aka common ancestor for the 2 commits provided
+      # @note If a reference (e.g. branch name) can't be found locally, it will try with the same ref prefixed with `origin/`
+      #
+      def self.find_merge_base(ref1, ref2)
+        git_repo = Git.open(Dir.pwd)
+        # Resolve to shas, mostly so that we can support cases with and without `origin/` explicit prefix on branch names
+        ref1_sha, ref2_sha = [ref1, ref2].map { |ref| get_commit_sha(ref: ref, prepend_origin_if_needed: true) }
+
+        git_repo.merge_base(ref1_sha, ref2_sha)&.first&.sha
+      end
+
       # Checks if two git references point to the same commit.
       #
       # @param ref1 [String] the first git reference to check.
       # @param ref2 [String] the second git reference to check.
-      # @param remote_name [String] the name of the remote repository to use (default is 'origin').
-      #                             If nil or empty, no remote prefix will be used.
       #
       # @return [Boolean] true if the two references point to the same commit, false otherwise.
       #
-      def self.point_to_same_commit?(ref1, ref2, remote_name: 'origin')
-        git_repo = Git.open(Dir.pwd)
-
-        ref1_full = remote_name.to_s.empty? ? ref1 : "#{remote_name}/#{ref1}"
-        ref2_full = remote_name.to_s.empty? ? ref2 : "#{remote_name}/#{ref2}"
+      def self.point_to_same_commit?(ref1, ref2)
         begin
-          ref1_commit = git_repo.gcommit(ref1_full)
-          ref2_commit = git_repo.gcommit(ref2_full)
+          ref1_sha = get_commit_sha(ref: ref1, prepend_origin_if_needed: true)
+          ref2_sha = get_commit_sha(ref: ref2, prepend_origin_if_needed: true)
         rescue StandardError => e
-          UI.error "Error fetching commits for #{ref1_full} and #{ref2_full}: #{e.message}"
-          return false
+          UI.user_error! "Error fetching commits for #{ref1} and/or #{ref2}: #{e.message}"
         end
-        ref1_commit.sha == ref2_commit.sha
+        ref1_sha == ref2_sha
       end
 
       # Returns the current git branch, or "HEAD" if it's not checked out to any branch

spec/create_release_backmerge_pull_request_spec.rb

@@ -34,17 +34,25 @@ def stub_git_release_branches(branches)
     allow(git_client).to receive(:branches).and_return(
       branches.map { |b| instance_double(Git::Branch, remote: instance_double(Git::Remote, name: 'origin'), name: b) }
     )
+    allow(git_client).to receive(:merge_base) do |ref1, ref2|
+      ['merge-base-of', ref1.gsub('/', '-'), 'and', ref2.gsub('/', '-')].join('-')
+    end
   end
 
-  def stub_expected_pull_requests(expected_backmerge_branches:, source_branch:, labels: [], milestone_number: nil, reviewers: nil, team_reviewers: nil, branch_exists_on_remote: false, point_to_same_commit: false)
+  def stub_expected_pull_requests(expected_backmerge_branches:, source_branch:, labels: [], milestone_number: nil, reviewers: nil, team_reviewers: nil, branch_exists_on_remote: false, nothing_to_merge_between: [])
     expected_backmerge_branches.map do |target_branch|
       expected_intermediate_branch = "merge/#{source_branch.gsub('/', '-')}-into-#{target_branch.gsub('/', '-')}"
 
       allow(Fastlane::Helper::GitHelper).to receive(:branch_exists_on_remote?).with(branch_name: expected_intermediate_branch).and_return(branch_exists_on_remote)
 
-      allow(Fastlane::Helper::GitHelper).to receive(:point_to_same_commit?).with(target_branch, source_branch).and_return(point_to_same_commit)
+      allow(described_class).to receive(:can_merge?).and_return(true)
+      nothing_to_merge_between&.each do |head, base|
+        allow(described_class).to receive(:can_merge?).with(head, into: base).and_return(false)
+      end
+
+      allow(other_action_mock).to receive(:ensure_git_branch).with({ branch: "^#{expected_intermediate_branch}$" }).and_return(true)
 
-      next if point_to_same_commit
+      next unless nothing_to_merge_between.nil? || nothing_to_merge_between.empty?
 
       expect(Fastlane::Helper::GitHelper).to receive(:checkout_and_pull).with(source_branch)
       expect(Fastlane::Helper::GitHelper).to receive(:create_branch).with(expected_intermediate_branch)
@@ -284,27 +292,53 @@ def stub_expected_pull_requests(expected_backmerge_branches:, source_branch:, la
     end
   end
 
-  context 'when checking if source & target branches point to the same commit' do
-    it 'does not create a pull request when `source_branch` a target branch point to the same commit' do
-      stub_git_release_branches(%w[release/30.6])
+  context 'when there is nothing to merge' do
+    context 'when no callback is provided' do
+      it 'detects it should not create a pull request before even creating the intermediate branch' do
+        stub_git_release_branches(%w[release/30.8])
 
-      source_branch = 'release/30.7'
+        source_branch = 'release/30.7'
 
-      expected_backmerge_branches = %w[trunk release/30.6]
-      stub_expected_pull_requests(
-        expected_backmerge_branches: expected_backmerge_branches,
-        source_branch: source_branch,
-        point_to_same_commit: true
-      )
+        stub_expected_pull_requests(
+          expected_backmerge_branches: [default_branch],
+          source_branch: source_branch,
+          nothing_to_merge_between: { source_branch => default_branch }
+        )
 
-      result = run_described_fastlane_action(
-        github_token: test_token,
-        repository: test_repo,
-        source_branch: source_branch,
-        target_branches: expected_backmerge_branches
-      )
+        result = run_described_fastlane_action(
+          github_token: test_token,
+          repository: test_repo,
+          source_branch: source_branch,
+          target_branches: [default_branch]
+        )
+
+        expect(result).to be_empty
+      end
+    end
+
+    context 'when a callback is provided' do
+      it 'checks the merge-ability with the intermediate branch after callback has been called' do
+        stub_git_release_branches(%w[release/30.8])
+
+        source_branch = 'release/30.7'
+
+        stub_expected_pull_requests(
+          expected_backmerge_branches: [default_branch],
+          source_branch: source_branch,
+          nothing_to_merge_between: { 'merge/release-30.7-into-main' => default_branch }
+        )
+
+        inspectable_proc = instance_double(Proc, inspect: 'proc { }')
+        result = run_described_fastlane_action(
+          github_token: test_token,
+          repository: test_repo,
+          source_branch: source_branch,
+          target_branches: [default_branch],
+          intermediate_branch_created_callback: inspectable_proc
+        )
 
-      expect(result).to be_empty
+        expect(result).to be_empty
+      end
     end
   end
 
@@ -350,7 +384,7 @@ def stub_expected_pull_requests(expected_backmerge_branches:, source_branch:, la
       )
 
       intermediate_branch = "merge/release-30.6-into-#{default_branch}"
-      expect(other_action_mock).to receive(:ensure_git_branch).with(branch: "^#{intermediate_branch}/")
+      expect(other_action_mock).to receive(:ensure_git_branch).with(branch: "^#{intermediate_branch}$")
       allow(Fastlane::UI).to receive(:message).with(anything)
       expect(Fastlane::UI).to receive(:message).with("branch created callback was called! #{default_branch} #{intermediate_branch}")
 

spec/git_helper_spec.rb

@@ -111,9 +111,6 @@
   end
 
   describe 'point_to_same_commit?(ref1, ref2)' do
-    # We cannot test the happy path using a remote because the repo we use for the tests does not have a remote.
-    let(:remote_name) { nil }
-
     before do
       # Spec branching setup:
       #
@@ -148,37 +145,37 @@
     end
 
     it 'checks if a tag and a branch point to the same commit' do
-      same_commit = described_class.point_to_same_commit?('1.0', 'another-branch', remote_name: remote_name)
+      same_commit = described_class.point_to_same_commit?('1.0', 'another-branch')
       expect(same_commit).to be false
     end
 
     it 'checks if a tag and a branch that had a merge point to the same commit' do
-      same_commit = described_class.point_to_same_commit?('1.0', 'main', remote_name: remote_name)
+      same_commit = described_class.point_to_same_commit?('1.0', 'main')
       expect(same_commit).to be false
     end
 
     it 'checks if a tag and a commit hash point to the same commit' do
-      same_commit = described_class.point_to_same_commit?('1.0', commit_hash(commit_message: 'commit D'), remote_name: remote_name)
+      same_commit = described_class.point_to_same_commit?('1.0', commit_hash(commit_message: 'commit D'))
       expect(same_commit).to be false
     end
 
     it 'checks if a commit hash and a branch point to the same commit' do
-      same_commit = described_class.point_to_same_commit?(commit_hash(commit_message: 'commit B'), 'another-branch', remote_name: remote_name)
+      same_commit = described_class.point_to_same_commit?(commit_hash(commit_message: 'commit B'), 'another-branch')
       expect(same_commit).to be false
     end
 
     it 'checks if commits between the same branch point to the same commit' do
-      same_commit = described_class.point_to_same_commit?('feature-branch', 'feature-branch', remote_name: remote_name)
+      same_commit = described_class.point_to_same_commit?('feature-branch', 'feature-branch')
       expect(same_commit).to be true
     end
 
     it 'checks if commits between branches that have no difference point to the same commit' do
-      same_commit = described_class.point_to_same_commit?('another-branch', 'new-branch', remote_name: remote_name)
+      same_commit = described_class.point_to_same_commit?('another-branch', 'new-branch')
       expect(same_commit).to be true
     end
 
     it 'raises error for a non-existent base_ref' do
-      expect { described_class.point_to_same_commit?('non-existent', 'main', remote_name: remote_name) }.to raise_error(StandardError)
+      expect { described_class.point_to_same_commit?('non-existent', 'main') }.to raise_error(StandardError)
     end
   end