From 119bc99f2561f99cb2f3688158e721a7b87f2475 Mon Sep 17 00:00:00 2001 From: Barry Gordon Date: Tue, 23 May 2023 18:05:19 +0100 Subject: [PATCH 1/2] Generate deterministic branch names based on content --- .../branch_namer/dependency_group_strategy.rb | 21 +++++++---- .../dependency_group_strategy_spec.rb | 36 +++++++++++++++++++ 2 files changed, 51 insertions(+), 6 deletions(-) diff --git a/common/lib/dependabot/pull_request_creator/branch_namer/dependency_group_strategy.rb b/common/lib/dependabot/pull_request_creator/branch_namer/dependency_group_strategy.rb index 57d23307947..88d6affdb7b 100644 --- a/common/lib/dependabot/pull_request_creator/branch_namer/dependency_group_strategy.rb +++ b/common/lib/dependabot/pull_request_creator/branch_namer/dependency_group_strategy.rb @@ -21,7 +21,7 @@ def initialize(dependencies:, files:, target_branch:, dependency_group:, # fixed-length name, so we can punt on handling truncation until # we determine the strict validation rules for names def new_branch_name - File.join(prefixes, timestamped_group_name).gsub("/", separator) + File.join(prefixes, group_name_with_dependency_digest).gsub("/", separator) end private @@ -37,11 +37,20 @@ def prefixes ].compact end - # When superseding a grouped update pull request, we will have a period - # of time when there are two branches for the group so we use a timestamp - # to avoid collisions. - def timestamped_group_name - "#{dependency_group.name}-#{Time.now.utc.to_i}" + # Group pull requests will generally include too many dependencies to include + # in the branch name, but we rely on branch names being deterministic for a + # given set of dependency changes. + # + # Let's append a short hash digest of the dependency changes so that we can + # meet this guarantee. + def group_name_with_dependency_digest + "#{dependency_group.name}-#{dependency_digest}" + end + + def dependency_digest + @dependency_digest ||= Digest::MD5.hexdigest(dependencies.map do |dependency| + "#{dependency.name}-#{dependency.removed? ? 'removed' : dependency.version}" + end.join(",")).slice(0, 10) end def package_manager diff --git a/common/spec/dependabot/pull_request_creator/branch_namer/dependency_group_strategy_spec.rb b/common/spec/dependabot/pull_request_creator/branch_namer/dependency_group_strategy_spec.rb index 631d42d908f..35c96963f99 100644 --- a/common/spec/dependabot/pull_request_creator/branch_namer/dependency_group_strategy_spec.rb +++ b/common/spec/dependabot/pull_request_creator/branch_namer/dependency_group_strategy_spec.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "digest" + require "dependabot/dependency" require "dependabot/dependency_file" require "dependabot/dependency_group" @@ -48,6 +50,40 @@ it "returns the name of the dependency group prefixed correctly" do expect(namer.new_branch_name).to start_with("dependabot/bundler/my-dependency-group") end + + it "generates a deterministic branch name for a given set of dependencies" do + branch_name = namer.new_branch_name + new_namer = described_class.new( + dependencies: dependencies, + files: [gemfile], + target_branch: target_branch, + separator: separator, + dependency_group: dependency_group + ) + sleep 1 # ensure the timestamp changes + expect(new_namer.new_branch_name).to eql(branch_name) + end + + it "generates a different branch name for a diffset set of dependencies for the same group" do + removed_dependency = Dependabot::Dependency.new( + name: "old_business", + version: "1.4.0", + previous_version: "1.4.0", + package_manager: "bundler", + requirements: {}, + previous_requirements: {}, + removed: true + ) + + new_namer = described_class.new( + dependencies: [dependency, removed_dependency], + files: [gemfile], + target_branch: target_branch, + separator: separator, + dependency_group: dependency_group + ) + expect(new_namer.new_branch_name).not_to eql(namer.new_branch_name) + end end context "with a custom separator" do From 75fc988d4fea1d50fe20ad0e820fcc410d0719ab Mon Sep 17 00:00:00 2001 From: Barry Gordon Date: Tue, 23 May 2023 18:32:06 +0100 Subject: [PATCH 2/2] Dependency order isn't a factor --- .../branch_namer/dependency_group_strategy.rb | 2 +- .../dependency_group_strategy_spec.rb | 32 ++++++++++++++++++- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/common/lib/dependabot/pull_request_creator/branch_namer/dependency_group_strategy.rb b/common/lib/dependabot/pull_request_creator/branch_namer/dependency_group_strategy.rb index 88d6affdb7b..67e59cd0ee9 100644 --- a/common/lib/dependabot/pull_request_creator/branch_namer/dependency_group_strategy.rb +++ b/common/lib/dependabot/pull_request_creator/branch_namer/dependency_group_strategy.rb @@ -50,7 +50,7 @@ def group_name_with_dependency_digest def dependency_digest @dependency_digest ||= Digest::MD5.hexdigest(dependencies.map do |dependency| "#{dependency.name}-#{dependency.removed? ? 'removed' : dependency.version}" - end.join(",")).slice(0, 10) + end.sort.join(",")).slice(0, 10) end def package_manager diff --git a/common/spec/dependabot/pull_request_creator/branch_namer/dependency_group_strategy_spec.rb b/common/spec/dependabot/pull_request_creator/branch_namer/dependency_group_strategy_spec.rb index 35c96963f99..43b62c410b3 100644 --- a/common/spec/dependabot/pull_request_creator/branch_namer/dependency_group_strategy_spec.rb +++ b/common/spec/dependabot/pull_request_creator/branch_namer/dependency_group_strategy_spec.rb @@ -64,7 +64,7 @@ expect(new_namer.new_branch_name).to eql(branch_name) end - it "generates a different branch name for a diffset set of dependencies for the same group" do + it "generates a different branch name for a different set of dependencies for the same group" do removed_dependency = Dependabot::Dependency.new( name: "old_business", version: "1.4.0", @@ -84,6 +84,36 @@ ) expect(new_namer.new_branch_name).not_to eql(namer.new_branch_name) end + + it "generates the same branch name regardless of the order of dependencies" do + removed_dependency = Dependabot::Dependency.new( + name: "old_business", + version: "1.4.0", + previous_version: "1.4.0", + package_manager: "bundler", + requirements: {}, + previous_requirements: {}, + removed: true + ) + + forward_namer = described_class.new( + dependencies: [dependency, removed_dependency], + files: [gemfile], + target_branch: target_branch, + separator: separator, + dependency_group: dependency_group + ) + + backward_namer = described_class.new( + dependencies: [removed_dependency, dependency], + files: [gemfile], + target_branch: target_branch, + separator: separator, + dependency_group: dependency_group + ) + + expect(forward_namer.new_branch_name).to eql(backward_namer.new_branch_name) + end end context "with a custom separator" do