Auto merge of #4977 - chrismo:cons_lock_shared_deps, r=segiddins

Fix #4934. Make GVP _after_ eager unlock.

When Definition creates the GemVersionPromoter, it needs to do so
_after_ it's performed the eager_unlock so the GVP gets the correct list
of unlocked gems.

Prior to this fix, it had a stricter list of gems being updated with the
new `--patch` or `--minor` options used with `bundle update` and in some
cases would have inconsistent results when used without a conservative
switch or the `--major` option. See #4934 for details.

2 files changed, 70 insertions, 3 deletions

diff --git a/lib/bundler/definition.rb b/lib/bundler/definition.rb
index 05d4c4cddb..f14085f60a 100644
--- a/lib/bundler/definition.rb
+++ b/lib/bundler/definition.rb
@@ -103,8 +103,6 @@ module Bundler
       end
       @unlocking ||= @unlock[:ruby] ||= (!@locked_ruby_version ^ !@ruby_version)
 
-      @gem_version_promoter = create_gem_version_promoter
-
       current_platform = Bundler.rubygems.platforms.map {|p| generic(p) }.compact.last
       add_platform(current_platform)
 
@@ -112,6 +110,8 @@ module Bundler
       eager_unlock = expand_dependencies(@unlock[:gems])
       @unlock[:gems] = @locked_specs.for(eager_unlock).map(&:name)
 
+      @gem_version_promoter = create_gem_version_promoter
+
       @source_changes = converge_sources
       @dependency_changes = converge_dependencies
       @local_changes = converge_locals
diff --git a/spec/bundler/definition_spec.rb b/spec/bundler/definition_spec.rb
index c72f50f0d1..71e5bb00aa 100644
--- a/spec/bundler/definition_spec.rb
+++ b/spec/bundler/definition_spec.rb
@@ -137,7 +137,7 @@ describe Bundler::Definition do
   describe "initialize" do
     context "gem version promoter" do
       context "with lockfile" do
-        before :each do
+        before do
           install_gemfile <<-G
           source "file://#{gem_repo1}"
           gem "foo"
@@ -159,6 +159,73 @@ describe Bundler::Definition do
         end
       end
 
+      context "shared dependent gems" do
+        before do
+          build_repo4 do
+            build_gem "isolated_owner", %w(1.0.1 1.0.2) do |s|
+              s.add_dependency "isolated_dep", "~> 2.0"
+            end
+            build_gem "isolated_dep", %w(2.0.1 2.0.2)
+
+            build_gem "shared_owner_a", %w(3.0.1 3.0.2) do |s|
+              s.add_dependency "shared_dep", "~> 5.0"
+            end
+            build_gem "shared_owner_b", %w(4.0.1 4.0.2) do |s|
+              s.add_dependency "shared_dep", "~> 5.0"
+            end
+            build_gem "shared_dep", %w(5.0.1 5.0.2)
+          end
+
+          gemfile <<-G
+            source "file://#{gem_repo4}"
+            gem 'isolated_owner'
+
+            gem 'shared_owner_a'
+            gem 'shared_owner_b'
+          G
+
+          lockfile <<-L
+            GEM
+              remote: file://#{gem_repo4}
+              specs:
+                isolated_dep (2.0.1)
+                isolated_owner (1.0.1)
+                  isolated_dep (~> 2.0)
+                shared_dep (5.0.1)
+                shared_owner_a (3.0.1)
+                  shared_dep (~> 5.0)
+                shared_owner_b (4.0.1)
+                  shared_dep (~> 5.0)
+
+            PLATFORMS
+              ruby
+
+            DEPENDENCIES
+              shared_owner_a
+              shared_owner_b
+              isolated_owner
+
+            BUNDLED WITH
+               1.13.0
+          L
+        end
+
+        it "should unlock isolated and shared dependencies equally" do
+          # setup for these test costs about 3/4 of a second, much faster to just jam them all in here.
+          # the global before :each defeats any ability to have re-usable setup for many examples in a
+          # single context by wiping out the tmp dir and contents.
+
+          unlock_deps_test(%w(isolated_owner), %w(isolated_dep isolated_owner))
+          unlock_deps_test(%w(isolated_owner shared_owner_a), %w(isolated_dep isolated_owner shared_dep shared_owner_a))
+        end
+
+        def unlock_deps_test(passed_unlocked, expected_calculated)
+          definition = Bundler::Definition.new(bundled_app("Gemfile.lock"), [], Bundler::SourceList.new, :gems => passed_unlocked)
+          unlock_gems = definition.gem_version_promoter.unlock_gems
+          expect(unlock_gems.sort).to eq expected_calculated
+        end
+      end
+
       def mock_source_list
         Class.new do
           def all_sources