diff --git a/common/lib/dependabot/pull_request_creator/branch_namer/base.rb b/common/lib/dependabot/pull_request_creator/branch_namer/base.rb new file mode 100644 index 0000000000..4e1566c384 --- /dev/null +++ b/common/lib/dependabot/pull_request_creator/branch_namer/base.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +module Dependabot + class PullRequestCreator + class BranchNamer + class Base + attr_reader :dependencies, :files, :target_branch, :separator, :prefix, :max_length + + def initialize(dependencies:, files:, target_branch:, separator: "/", + prefix: "dependabot", max_length: nil) + @dependencies = dependencies + @files = files + @target_branch = target_branch + @separator = separator + @prefix = prefix + @max_length = max_length + end + + private + + def sanitize_branch_name(ref_name) + # General git ref validation + sanitized_name = sanitize_ref(ref_name) + + # Some users need branch names without slashes + sanitized_name = sanitized_name.gsub("/", separator) + + # Shorten the ref in case users refs have length limits + if max_length && (sanitized_name.length > max_length) + sha = Digest::SHA1.hexdigest(sanitized_name)[0, max_length] + sanitized_name[[max_length - sha.size, 0].max..] = sha + end + + sanitized_name + end + + def sanitize_ref(ref) + # This isn't a complete implementation of git's ref validation, but it + # covers most cases that crop up. Its list of allowed characters is a + # bit stricter than git's, but that's for cosmetic reasons. + ref. + # Remove forbidden characters (those not already replaced elsewhere) + gsub(%r{[^A-Za-z0-9/\-_.(){}]}, ""). + # Slashes can't be followed by periods + gsub(%r{/\.}, "/dot-").squeeze(".").squeeze("/"). + # Trailing periods are forbidden + sub(/\.$/, "") + end + end + end + end +end 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 67e59cd0ee..52dc145813 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 @@ -1,32 +1,32 @@ # frozen_string_literal: true +require "dependabot/pull_request_creator/branch_namer/base" + module Dependabot class PullRequestCreator class BranchNamer - class DependencyGroupStrategy + class DependencyGroupStrategy < Base def initialize(dependencies:, files:, target_branch:, dependency_group:, separator: "/", prefix: "dependabot", max_length: nil) - @dependencies = dependencies - @files = files - @target_branch = target_branch + super( + dependencies: dependencies, + files: files, + target_branch: target_branch, + separator: separator, + prefix: prefix, + max_length: max_length + ) + @dependency_group = dependency_group - @separator = separator - @prefix = prefix - @max_length = max_length end - # FIXME: Incorporate max_length truncation once we allow user config - # - # For now, we are using a placeholder DependencyGroup with a - # 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, group_name_with_dependency_digest).gsub("/", separator) + sanitize_branch_name(File.join(prefixes, group_name_with_dependency_digest)) end private - attr_reader :dependencies, :dependency_group, :files, :target_branch, :separator, :prefix, :max_length + attr_reader :dependency_group def prefixes [ diff --git a/common/lib/dependabot/pull_request_creator/branch_namer/solo_strategy.rb b/common/lib/dependabot/pull_request_creator/branch_namer/solo_strategy.rb index 5db9bd8922..b87f434bdb 100644 --- a/common/lib/dependabot/pull_request_creator/branch_namer/solo_strategy.rb +++ b/common/lib/dependabot/pull_request_creator/branch_namer/solo_strategy.rb @@ -3,23 +3,12 @@ require "digest" require "dependabot/metadata_finders" +require "dependabot/pull_request_creator/branch_namer/base" module Dependabot class PullRequestCreator class BranchNamer - class SoloStrategy - attr_reader :dependencies, :files, :target_branch, :separator, :prefix, :max_length - - def initialize(dependencies:, files:, target_branch:, separator: "/", - prefix: "dependabot", max_length: nil) - @dependencies = dependencies - @files = files - @target_branch = target_branch - @separator = separator - @prefix = prefix - @max_length = max_length - end - + class SoloStrategy < Base def new_branch_name @name ||= begin @@ -39,16 +28,7 @@ def new_branch_name "#{dependency_name_part}-#{branch_version_suffix}" end - # Some users need branch names without slashes - sanitized_name = sanitize_ref(File.join(prefixes, @name).gsub("/", separator)) - - # Shorten the ref in case users refs have length limits - if @max_length && (sanitized_name.length > @max_length) - sha = Digest::SHA1.hexdigest(sanitized_name)[0, @max_length] - sanitized_name[[@max_length - sha.size, 0].max..] = sha - end - - sanitized_name + sanitize_branch_name(File.join(prefixes, @name)) end private @@ -189,19 +169,6 @@ def library? def requirements_changed?(dependency) (dependency.requirements - dependency.previous_requirements).any? end - - def sanitize_ref(ref) - # This isn't a complete implementation of git's ref validation, but it - # covers most cases that crop up. Its list of allowed characters is a - # bit stricter than git's, but that's for cosmetic reasons. - ref. - # Remove forbidden characters (those not already replaced elsewhere) - gsub(%r{[^A-Za-z0-9/\-_.(){}]}, ""). - # Slashes can't be followed by periods - gsub(%r{/\.}, "/dot-").squeeze(".").squeeze("/"). - # Trailing periods are forbidden - sub(/\.$/, "") - end end end end 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 43b62c410b..f87c0cb68a 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 @@ -2,6 +2,7 @@ require "digest" +require "spec_helper" require "dependabot/dependency" require "dependabot/dependency_file" require "dependabot/dependency_group" @@ -14,7 +15,8 @@ files: [gemfile], target_branch: target_branch, separator: separator, - dependency_group: dependency_group + dependency_group: dependency_group, + max_length: max_length ) end @@ -40,8 +42,11 @@ let(:dependency_group) do Dependabot::DependencyGroup.new(name: "my-dependency-group", rules: anything) end + let(:max_length) { nil } describe "#new_branch_name" do + subject(:new_branch_name) { namer.new_branch_name } + context "with defaults for separator, target branch and files in the root directory" do let(:directory) { "/" } let(:target_branch) { nil } @@ -126,6 +131,48 @@ end end + context "with a maximum length" do + let(:directory) { "/" } + let(:target_branch) { nil } + let(:separator) { "/" } + + context "with a maximum length longer than branch name" do + let(:max_length) { 50 } + + it { is_expected.to eq("dependabot/bundler/my-dependency-group-b8d660191d") } + its(:length) { is_expected.to eq(49) } + end + + context "with a maximum length shorter than branch name" do + let(:dependency_group) do + Dependabot::DependencyGroup.new(name: "business-and-work-and-desks-and-tables-and-chairs", rules: anything) + end + + let(:sha1_digest) { Digest::SHA1.hexdigest("dependabot/bundler/#{dependency_group.name}-b8d660191d") } + + context "with a maximum length longer than sha1 length" do + let(:max_length) { 50 } + + it { is_expected.to eq("dependabot#{sha1_digest}") } + its(:length) { is_expected.to eq(50) } + end + + context "with a maximum length equal than sha1 length" do + let(:max_length) { 40 } + + it { is_expected.to eq(sha1_digest) } + its(:length) { is_expected.to eq(40) } + end + + context "with a maximum length shorter than sha1 length" do + let(:max_length) { 20 } + + it { is_expected.to eq(sha1_digest[0...20]) } + its(:length) { is_expected.to eq(20) } + end + end + end + context "for files in a non-root directory" do let(:directory) { "rails app/" } # let's make sure we deal with spaces too let(:target_branch) { nil } @@ -146,7 +193,7 @@ end end - context "for files in a non-root directory targetting a branch" do + context "for files in a non-root directory targeting a branch" do let(:directory) { "rails-app/" } let(:target_branch) { "develop" } let(:separator) { "_" }