diff --git a/common/lib/dependabot/pull_request_creator.rb b/common/lib/dependabot/pull_request_creator.rb index 533f479a81..52a14b4c3e 100644 --- a/common/lib/dependabot/pull_request_creator.rb +++ b/common/lib/dependabot/pull_request_creator.rb @@ -40,6 +40,8 @@ class NoHistoryInCommon < StandardError; end class UnmergedPRExists < StandardError; end + class BranchAlreadyExists < StandardError; end + class BaseCommitNotUpToDate < StandardError; end class UnexpectedError < StandardError; end @@ -396,12 +398,6 @@ def message sig { returns(Dependabot::PullRequestCreator::BranchNamer) } def branch_namer - if Dependabot::Experiments.enabled?(:dedup_branch_names) && existing_branches - Dependabot.logger.debug( - "Dependabot::PullRequestCreator::branch_namer : #{existing_branches}" - ) - end - @branch_namer ||= T.let( BranchNamer.new( dependencies: dependencies, diff --git a/common/lib/dependabot/pull_request_creator/branch_namer/base.rb b/common/lib/dependabot/pull_request_creator/branch_namer/base.rb index 887057c9d8..1b1ca24f21 100644 --- a/common/lib/dependabot/pull_request_creator/branch_namer/base.rb +++ b/common/lib/dependabot/pull_request_creator/branch_namer/base.rb @@ -74,32 +74,7 @@ def sanitize_branch_name(ref_name) sanitized_name[[T.must(max_length) - sha.size, 0].max..] = sha end - if Dependabot::Experiments.enabled?(:dedup_branch_names) - dedup_existing_branches(sanitized_name) - else - sanitized_name - end - end - - sig { params(ref: String).returns(String) } - def dedup_existing_branches(ref) - Dependabot.logger.debug( - "Dependabot::PullRequestCreator::dedup_existing_branches::ref : #{ref}" - ) - return ref unless existing_branches.include?(ref) - - i = 1 - new_ref = "#{ref}-#{i}" - while existing_branches.include?(new_ref) - i += 1 - new_ref = "#{ref}-#{i}" - end - - Dependabot.logger.debug( - "Dependabot::PullRequestCreator::dedup_existing_branches::new_ref : #{new_ref}" - ) - - new_ref + sanitized_name end sig { params(ref: String).returns(String) } diff --git a/common/lib/dependabot/pull_request_creator/github.rb b/common/lib/dependabot/pull_request_creator/github.rb index 2109d542d0..a37a07c7bd 100644 --- a/common/lib/dependabot/pull_request_creator/github.rb +++ b/common/lib/dependabot/pull_request_creator/github.rb @@ -110,6 +110,13 @@ def initialize(source:, branch_name:, base_commit:, credentials:, sig { returns(T.untyped) } def create + if experiment_duplicate_branch? && branch_exists?(branch_name) + Dependabot.logger.info( + "Existing branch \"#{branch_name}\" found. Pull request not created." + ) + raise BranchAlreadyExists, "Duplicate branch #{branch_name} already exists" + end + if branch_exists?(branch_name) && unmerged_pull_request_exists? raise UnmergedPRExists, "PR ##{unmerged_pull_requests.first.number} already exists" end @@ -132,6 +139,11 @@ def require_up_to_date_base? # rubocop:disable Metrics/PerceivedComplexity sig { params(name: String).returns(T::Boolean) } def branch_exists?(name) + Dependabot.logger.debug( + "Dependabot::PullRequestCreator::Github:branch_exists?. " \ + "Name : #{name}. IsDuplicate: #{git_metadata_fetcher.ref_names.include?(name)}" + ) + git_metadata_fetcher.ref_names.include?(name) rescue Dependabot::GitDependenciesNotReachable => e raise T.must(e.cause) if e.cause&.message&.include?("is disabled") @@ -580,6 +592,11 @@ def raise_custom_error(base_err, type, message) raise type, message end end + + sig { returns(T::Boolean) } + def experiment_duplicate_branch? + Dependabot::Experiments.enabled?(:dedup_branch_names) + end end # rubocop:enable Metrics/ClassLength end diff --git a/common/spec/dependabot/pull_request_creator/branch_namer_spec.rb b/common/spec/dependabot/pull_request_creator/branch_namer_spec.rb index d750851917..c2fa591b08 100644 --- a/common/spec/dependabot/pull_request_creator/branch_namer_spec.rb +++ b/common/spec/dependabot/pull_request_creator/branch_namer_spec.rb @@ -88,29 +88,6 @@ it { is_expected.to eq("myapp/dummy/business-1.5.0") } end - context "with a branch name that already exists" do - before do - Dependabot::Experiments.register(:dedup_branch_names, true) - end - - let!(:existing_branches) do - [ - "dependabot/dummy/business-1.5.0", - "dependabot/dummy/business-1.5.0-1" - ] - end - let(:namer) do - described_class.new( - dependencies: dependencies, - files: files, - target_branch: target_branch, - existing_branches: existing_branches - ) - end - - it { is_expected.to eq("dependabot/dummy/business-1.5.0-2") } - end - context "with a target branch" do let(:target_branch) { "my-branch" } diff --git a/common/spec/dependabot/pull_request_creator/github_spec.rb b/common/spec/dependabot/pull_request_creator/github_spec.rb index eccbf5d7b8..8a58dd8d56 100644 --- a/common/spec/dependabot/pull_request_creator/github_spec.rb +++ b/common/spec/dependabot/pull_request_creator/github_spec.rb @@ -612,6 +612,41 @@ end end + context "when the branch already exists" do + let(:service_pack_response) { fixture("git", "upload_packs", "existing-branch") } + + before do + Dependabot::Experiments.register(:dedup_branch_names, true) + end + + after do + Dependabot::Experiments.register(:dedup_branch_names, false) + end + + context "when the branch already exists" do + before do + url = "#{repo_api_url}/pulls?head=gocardless:#{branch_name}" \ + "&state=all" + stub_request(:get, url) + .to_return(status: 200, body: "[]", headers: json_header) + stub_request( + :patch, + "#{repo_api_url}/git/refs/heads/#{branch_name}" + ).to_return( + status: 200, + body: fixture("github", "update_ref.json"), + headers: json_header + ) + end + + it "returns a suitable exception" do + expect { creator.create } + .to raise_error(Dependabot::PullRequestCreator::BranchAlreadyExists, + "Duplicate branch #{branch_name} already exists") + end + end + end + context "when the PR doesn't have history in common with the base branch" do before do stub_request(:post, "#{repo_api_url}/pulls") diff --git a/common/spec/fixtures/git/upload_packs/existing-branch b/common/spec/fixtures/git/upload_packs/existing-branch new file mode 100644 index 0000000000..70cfb6b0d4 Binary files /dev/null and b/common/spec/fixtures/git/upload_packs/existing-branch differ