Merge #7007

7007: Remove lockfile incompatibility created by the `lockfile_uses_separate_rubygems_sources` setting r=deivid-rodriguez a=deivid-rodriguez

This is more of a question PR, I created this patch to try it out and try to understand, not necessarily get it merged.

### What was the end-user problem that led to this PR?

The problem was that once we enable the `lockfile_uses_separate_rubygems_sources` setting, all lockfiles in the world will become incompatible with the previous version. Actually, not necessarily incompatible, but bundler will reorder the sections when the setting is enabled, that will generate churn lock file diffs, and _maybe_ some confusion / merge conflicts, and so on.

### What was your diagnosis of the problem?

My diagnosis was that maybe this is not necessary. I read over the issues where this setting was added and what I understood is that previously if a Gemfile specified multiple rubygems sources, they would all get merged together and that's dangerous because it's not deterministic from which source each gem will be picked up, and that could be maliciously exploited. So now each source gets its own separate section. However, how does that affect the ordering of the sections? I don't think it should affect it?

### What is your fix for the problem, implemented in this PR?

My fix is to change the `lock_sources` method so that both code branches (`lockfile_uses_separate_rubygems_sources == true`, and `lockfile_uses_separate_rubygems_sources == false`) result in the same ordering of the source sections.

### Why did you choose this fix out of the possible options?

I chose this fix because I _think_ it keeps the setting doing the same thing, but also keeps lock file compatibility.


Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

2 files changed, 13 insertions, 50 deletions

diff --git a/spec/bundler/definition_spec.rb b/spec/bundler/definition_spec.rb
index ceb7b4bf05..163ec507b0 100644
--- a/spec/bundler/definition_spec.rb
+++ b/spec/bundler/definition_spec.rb
@@ -88,17 +88,17 @@ RSpec.describe Bundler::Definition do
 
       expect(out).to match(/re-resolving dependencies/)
       lockfile_should_be <<-G
-        GEM
-          remote: file://localhost#{gem_repo1}/
-          specs:
-            rack (1.0.0)
-
         PATH
           remote: #{lib_path("foo")}
           specs:
             foo (1.0)
               rack (= 1.0)
 
+        GEM
+          remote: file://localhost#{gem_repo1}/
+          specs:
+            rack (1.0.0)
+
         PLATFORMS
           #{lockfile_platforms}
 
@@ -110,7 +110,7 @@ RSpec.describe Bundler::Definition do
       G
     end
 
-    it "for a path gem with deps and no changes", :bundler => "< 2" do
+    it "for a path gem with deps and no changes" do
       build_lib "foo", "1.0", :path => lib_path("foo") do |s|
         s.add_dependency "rack", "1.0"
         s.add_development_dependency "net-ssh", "1.0"
@@ -137,43 +137,6 @@ RSpec.describe Bundler::Definition do
             rack (1.0.0)
 
         PLATFORMS
-          ruby
-
-        DEPENDENCIES
-          foo!
-
-        BUNDLED WITH
-           #{Bundler::VERSION}
-      G
-    end
-
-    it "for a path gem with deps and no changes", :bundler => "2" do
-      build_lib "foo", "1.0", :path => lib_path("foo") do |s|
-        s.add_dependency "rack", "1.0"
-        s.add_development_dependency "net-ssh", "1.0"
-      end
-
-      install_gemfile <<-G
-        source "file://localhost#{gem_repo1}"
-        gem "foo", :path => "#{lib_path("foo")}"
-      G
-
-      bundle :check, :env => { "DEBUG" => 1 }
-
-      expect(out).to match(/using resolution from the lockfile/)
-      lockfile_should_be <<-G
-        GEM
-          remote: file://localhost#{gem_repo1}/
-          specs:
-            rack (1.0.0)
-
-        PATH
-          remote: #{lib_path("foo")}
-          specs:
-            foo (1.0)
-              rack (= 1.0)
-
-        PLATFORMS
           #{lockfile_platforms}
 
         DEPENDENCIES
diff --git a/spec/bundler/source_list_spec.rb b/spec/bundler/source_list_spec.rb
index 729311421a..971f1042dc 100644
--- a/spec/bundler/source_list_spec.rb
+++ b/spec/bundler/source_list_spec.rb
@@ -393,19 +393,19 @@ RSpec.describe Bundler::SourceList do
 
     it "returns all sources, without combining rubygems sources", :bundler => "2" do
       expect(source_list.lock_sources).to eq [
-        Bundler::Source::Rubygems.new,
-        Bundler::Source::Rubygems.new("remotes" => ["https://duplicate-rubygems.org"]),
-        Bundler::Source::Rubygems.new("remotes" => ["https://first-rubygems.org"]),
-        Bundler::Source::Rubygems.new("remotes" => ["https://second-rubygems.org"]),
-        Bundler::Source::Rubygems.new("remotes" => ["https://third-rubygems.org"]),
         Bundler::Source::Git.new("uri" => "git://first-git.org/path.git"),
         Bundler::Source::Git.new("uri" => "git://second-git.org/path.git"),
         Bundler::Source::Git.new("uri" => "git://third-git.org/path.git"),
+        ASourcePlugin.new("uri" => "https://second-plugin.org/random"),
+        ASourcePlugin.new("uri" => "https://third-bar.org/foo"),
         Bundler::Source::Path.new("path" => "/first/path/to/gem"),
         Bundler::Source::Path.new("path" => "/second/path/to/gem"),
         Bundler::Source::Path.new("path" => "/third/path/to/gem"),
-        ASourcePlugin.new("uri" => "https://second-plugin.org/random"),
-        ASourcePlugin.new("uri" => "https://third-bar.org/foo"),
+        Bundler::Source::Rubygems.new,
+        Bundler::Source::Rubygems.new("remotes" => ["https://duplicate-rubygems.org"]),
+        Bundler::Source::Rubygems.new("remotes" => ["https://first-rubygems.org"]),
+        Bundler::Source::Rubygems.new("remotes" => ["https://second-rubygems.org"]),
+        Bundler::Source::Rubygems.new("remotes" => ["https://third-rubygems.org"]),
       ]
     end
   end