diff --git a/CHANGELOG.md b/CHANGELOG.md index 056aec9a3..55cfdb8d2 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/create_release_backmerge_pull_request_action.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/create_release_backmerge_pull_request_action.rb index 4156362d3..0f9700ea6 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/create_release_backmerge_pull_request_action.rb +++ b/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, diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/git_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/git_helper.rb index 4e82a4235..6b471d176 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/git_helper.rb +++ b/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 diff --git a/spec/create_release_backmerge_pull_request_spec.rb b/spec/create_release_backmerge_pull_request_spec.rb index b0788dc91..8b0ad779f 100644 --- a/spec/create_release_backmerge_pull_request_spec.rb +++ b/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}") diff --git a/spec/git_helper_spec.rb b/spec/git_helper_spec.rb index 403936381..4146ae366 100644 --- a/spec/git_helper_spec.rb +++ b/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