[rubygems/rubygems] Don't treat a git-sourced gem install as complete if only the '.git' directory is present. This recovers cases where a git-sourced install can be left in a partially installed state.

ruby/rubygems@d132b7008d

Author: taralbass

lib/bundler/source/git.rb

@@ -360,7 +360,11 @@ def requires_checkout?
       end
 
       def locked_revision_checked_out?
-        locked_revision && locked_revision == revision && install_path.exist?
+        locked_revision && locked_revision == revision && installed?
+      end
+
+      def installed?
+        git_proxy.installed_to?(install_path)
       end
 
       def base_name

lib/bundler/source/git/git_proxy.rb

@@ -147,6 +147,12 @@ def copy_to(destination, submodules = false)
           end
         end
 
+        def installed_to?(destination)
+          # if copy_to is interrupted, it may leave a partially installed directory that
+          # contains .git but no other files -- consider this not to be installed
+          Dir.exist?(destination) && (Dir.children(destination) - [".git"]).any?
+        end
+
         private
 
         def git_remote_fetch(args)

spec/bundler/bundler/source/git/git_proxy_spec.rb

@@ -211,4 +211,45 @@
       subject.checkout
     end
   end
+
+  describe "#installed_to?" do
+    let(:destination) { "install/dir" }
+    let(:destination_dir_exists) { true }
+    let(:children) { ["gem.gemspec", "README.me", ".git", "Rakefile"] }
+
+    before do
+      allow(Dir).to receive(:exist?).with(destination).and_return(destination_dir_exists)
+      allow(Dir).to receive(:children).with(destination).and_return(children)
+    end
+
+    context "when destination dir exists with children other than just .git" do
+      it "returns true" do
+        expect(git_proxy.installed_to?(destination)).to be true
+      end
+    end
+
+    context "when destination dir does not exist" do
+      let(:destination_dir_exists) { false }
+
+      it "returns false" do
+        expect(git_proxy.installed_to?(destination)).to be false
+      end
+    end
+
+    context "when destination dir is empty" do
+      let(:children) { [] }
+
+      it "returns false" do
+        expect(git_proxy.installed_to?(destination)).to be false
+      end
+    end
+
+    context "when destination dir has only .git directory" do
+      let(:children) { [".git"] }
+
+      it "returns false" do
+        expect(git_proxy.installed_to?(destination)).to be false
+      end
+    end
+  end
 end

spec/bundler/bundler/source/git_spec.rb

@@ -70,4 +70,54 @@
       end
     end
   end
+
+  describe "#locked_revision_checked_out?" do
+    let(:revision) { "abc" }
+    let(:git_proxy_revision) { revision }
+    let(:git_proxy_installed) { true }
+    let(:git_proxy) { subject.send(:git_proxy) }
+    let(:options) do
+      {
+        "uri" => uri,
+        "revision" => revision,
+      }
+    end
+
+    before do
+      allow(git_proxy).to receive(:revision).and_return(git_proxy_revision)
+      allow(git_proxy).to receive(:installed_to?).with(subject.install_path).and_return(git_proxy_installed)
+    end
+
+    context "when the locked revision is checked out" do
+      it "returns true" do
+        expect(subject.send(:locked_revision_checked_out?)).to be true
+      end
+    end
+
+    context "when no revision is provided" do
+      let(:options) do
+        { "uri" => uri }
+      end
+
+      it "returns falsey value" do
+        expect(subject.send(:locked_revision_checked_out?)).to be_falsey
+      end
+    end
+
+    context "when the git proxy revision is different than the git revision" do
+      let(:git_proxy_revision) { revision.next }
+
+      it "returns falsey value" do
+        expect(subject.send(:locked_revision_checked_out?)).to be_falsey
+      end
+    end
+
+    context "when the gem hasn't been installed" do
+      let(:git_proxy_installed) { false }
+
+      it "returns falsey value" do
+        expect(subject.send(:locked_revision_checked_out?)).to be_falsey
+      end
+    end
+  end
 end

spec/bundler/install/git_spec.rb

@@ -214,5 +214,80 @@
       expect(out).to include("Using foo 1.0 from #{lib_path("foo")} (at main@#{rev[0..6]})")
       expect(the_bundle).to include_gems "foo 1.0", source: "git@#{lib_path("foo")}"
     end
+
+    context "when install directory exists" do
+      let(:checkout_confirmation_log_message) { "Checking out revision" }
+      let(:using_foo_confirmation_log_message) { "Using foo 1.0 from #{lib_path("foo")} (at main@#{revision_for(lib_path("foo"))[0..6]})" }
+
+      context "and no contents besides .git directory are present" do
+        it "reinstalls gem" do
+          build_git "foo", "1.0", path: lib_path("foo")
+
+          gemfile = <<-G
+            source "https://gem.repo1"
+            gem "foo", :git => "#{lib_path("foo")}"
+          G
+
+          install_gemfile gemfile, verbose: true
+
+          expect(out).to include(checkout_confirmation_log_message)
+          expect(out).to include(using_foo_confirmation_log_message)
+          expect(the_bundle).to include_gems "foo 1.0", source: "git@#{lib_path("foo")}"
+
+          # validate that the installed directory exists and has some expected contents
+          install_directory = default_bundle_path("bundler/gems/foo-#{revision_for(lib_path("foo"))[0..11]}")
+          dot_git_directory = install_directory.join(".git")
+          lib_directory = install_directory.join("lib")
+          gemspec = install_directory.join("foo.gemspec")
+          expect([install_directory, dot_git_directory, lib_directory, gemspec]).to all exist
+
+          # remove all elements in the install directory except .git directory
+          FileUtils.rm_rf(lib_directory)
+          gemspec.delete
+
+          expect(dot_git_directory).to exist
+          expect(lib_directory).not_to exist
+          expect(gemspec).not_to exist
+
+          # rerun bundle install
+          install_gemfile gemfile, verbose: true
+
+          expect(out).to include(checkout_confirmation_log_message)
+          expect(out).to include(using_foo_confirmation_log_message)
+          expect(the_bundle).to include_gems "foo 1.0", source: "git@#{lib_path("foo")}"
+
+          # validate that it reinstalls all components
+          expect([install_directory, dot_git_directory, lib_directory, gemspec]).to all exist
+        end
+      end
+
+      context "and contents besides .git directory are present" do
+        # we want to confirm that the change to try to detect partial installs and reinstall does not
+        # result in repeatedly reinstalling the gem when it is fully installed
+        it "does not reinstall gem" do
+          build_git "foo", "1.0", path: lib_path("foo")
+
+          gemfile = <<-G
+            source "https://gem.repo1"
+            gem "foo", :git => "#{lib_path("foo")}"
+          G
+
+          install_gemfile gemfile, verbose: true
+
+          expect(out).to include(checkout_confirmation_log_message)
+          expect(out).to include(using_foo_confirmation_log_message)
+          expect(the_bundle).to include_gems "foo 1.0", source: "git@#{lib_path("foo")}"
+
+          # rerun bundle install
+          install_gemfile gemfile, verbose: true
+
+          # it isn't altogether straight-forward to validate that bundle didn't do soething on the second run, however,
+          # the presence of the 2nd log message confirms install got past the point that it would have logged the above if
+          # it was going to
+          expect(out).not_to include(checkout_confirmation_log_message)
+          expect(out).to include(using_foo_confirmation_log_message)
+        end
+      end
+    end
   end
 end