refactor: remove redundant comments, add dedup guard and missing test assertion

Author: markhallen

updater/lib/dependabot/dependency_group_engine.rb

@@ -153,8 +153,6 @@ def matches_group_by_parent_group?(dependency)
     def initialize(dependency_groups:)
       @dependency_groups = dependency_groups
       @ungrouped_dependencies = T.let([], T::Array[Dependabot::Dependency])
-      # Track the names of originally-configured group-by-dependency-name parent groups
-      # so that subgroups created from them are not treated as parents on subsequent calls.
       @group_by_name_parent_names = T.let(
         dependency_groups.select(&:group_by_dependency_name?).to_set(&:name),
         T::Set[String]
@@ -218,8 +216,6 @@ def should_skip_due_to_specificity?(group, dependency, specificity_calculator)
 
     sig { params(dependencies: T::Array[Dependabot::Dependency]).void }
     def create_dynamic_subgroups_for_dependency_name_groups(dependencies)
-      # Only select originally-configured parent groups, not subgroups created by previous calls.
-      # This prevents infinite recursion when assign_to_groups! is called once per directory.
       parent_groups = @dependency_groups.select { |g| @group_by_name_parent_names.include?(g.name) }
 
       parent_groups.each do |parent_group|
@@ -230,10 +226,9 @@ def create_dynamic_subgroups_for_dependency_name_groups(dependencies)
           existing_subgroup = @dependency_groups.find { |g| g.name == subgroup_name }
 
           if existing_subgroup
-            existing_subgroup.dependencies.concat(deps)
+            new_deps = deps.reject { |d| existing_subgroup.dependencies.include?(d) }
+            existing_subgroup.dependencies.concat(new_deps)
           else
-            # Preserve "group-by" in subgroup rules so group_by_dependency_name? returns true,
-            # enabling correct handled-dependency tracking when semver rules reject an update.
             subgroup_rules = parent_group.rules.merge("patterns" => [dep_name])
             subgroup = Dependabot::DependencyGroup.new(
               name: subgroup_name,

updater/lib/dependabot/updater/group_update_creation.rb

@@ -348,8 +348,6 @@ def compile_updates_for(dependency, dependency_files, group) # rubocop:disable M
         [] # return an empty set
       end
 
-      # When grouping by dependency name, mark the dependency as handled to prevent
-      # per-directory individual PRs from being raised.
       sig { params(dependency: Dependabot::Dependency, group: Dependabot::DependencyGroup, reason: String).void }
       def mark_handled_for_group_by_name(dependency, group, reason)
         return unless group.group_by_dependency_name?

updater/spec/dependabot/dependency_group_engine_spec.rb

@@ -890,10 +890,22 @@
       end
 
       it "does not treat subgroups as parents on subsequent calls despite having group_by set" do
-        # Subgroups now retain group-by in their rules, so we need to verify
-        # the recursion guard prevents them from spawning sub-subgroups.
-        # Simulate a second directory by calling assign_to_groups! again.
-        dependency_group_engine.assign_to_groups!(dependencies: [dummy_pkg_a_dir2])
+        # Simulate a third directory by calling assign_to_groups! again with a new dep.
+        dummy_pkg_a_dir3 = Dependabot::Dependency.new(
+          name: "dummy-pkg-a",
+          package_manager: "bundler",
+          version: "1.1.0",
+          requirements: [
+            {
+              file: "Gemfile",
+              requirement: "~> 1.1.0",
+              groups: ["default"],
+              source: nil
+            }
+          ],
+          directory: "/dir3"
+        )
+        dependency_group_engine.assign_to_groups!(dependencies: [dummy_pkg_a_dir3])
 
         # Should reuse the existing subgroup rather than creating a duplicate
         subgroups_a = dependency_group_engine.dependency_groups.select do |g|

updater/spec/dependabot/updater/group_update_creation_spec.rb

@@ -443,7 +443,7 @@ def initialize(dependency_snapshot, error_handler, job, group, service)
     end
   end
 
-  describe "compile_updates_for marks handled deps for group-by-name groups" do
+  describe "#compile_updates_for group-by-name handled deps" do
     let(:dependency) { dependencies.first }
 
     before do
@@ -564,6 +564,11 @@ def initialize(dependency_snapshot, error_handler, job, group, service)
         test_instance.compile_updates_for(dependency, dependency_files, group)
         expect(dependency_snapshot).not_to have_received(:add_handled_dependencies)
       end
+
+      it "returns an empty array" do
+        result = test_instance.compile_updates_for(dependency, dependency_files, group)
+        expect(result).to eq([])
+      end
     end
   end