Handle broken symlinks on install, install --force (Homebrew#21893)

This commit fixes an error occurring with both `install` and `install --force` where either would fail when encountering a broken symlink.

Users _might_ run into that issue in the wake of PR Homebrew#21857, and certainly _will_ be bitten by it once PR Homebrew#21858 is merged.

This commit changes the `install` case so it handles the condition gracefully. It also fixes `install --force` so before moving an app, it removes any broken symlinks which might stand in the way.

lib/hbc/artifact/moved.rb

@@ -22,7 +22,7 @@ def install_phase
     each_artifact do |artifact|
       load_specification(artifact)
       next unless preflight_checks
-      delete if File.exist?(target) && force
+      delete if Hbc::Utils.path_occupied?(target) && force
       move
     end
   end
@@ -64,7 +64,7 @@ def load_specification(artifact_spec)
   end
 
   def preflight_checks
-    if target.exist?
+    if Hbc::Utils.path_occupied?(target)
       if force
         ohai(warning_target_exists { |s| s << 'overwriting.' })
       else
@@ -96,7 +96,7 @@ def delete
       raise Hbc::CaskError.new(
         "Cannot remove undeletable #{english_name}")
     when force
-      Hbc::Utils.permissions_rmtree(target, command: @command)
+      Hbc::Utils.gain_permissions_remove(target, command: @command)
     else
       target.rmtree
     end

lib/hbc/installer.rb

@@ -312,21 +312,23 @@ def zap
     purge_caskroom_path
   end
 
-  def permissions_rmtree(path)
-    Hbc::Utils.permissions_rmtree(path, command: @command)
+  def gain_permissions_remove(path)
+    Hbc::Utils.gain_permissions_remove(path, command: @command)
   end
 
   def purge_versioned_files
     odebug "Purging files for version #{@cask.version} of Cask #{@cask}"
 
     # versioned staged distribution
-    permissions_rmtree(@cask.staged_path)
+    gain_permissions_remove(@cask.staged_path)
 
     # Homebrew-cask metadata
     if @cask.metadata_versioned_container_path.respond_to?(:children) and
         @cask.metadata_versioned_container_path.exist?
       @cask.metadata_versioned_container_path.children.each do |subdir|
-        permissions_rmtree subdir unless PERSISTENT_METADATA_SUBDIRS.include?(subdir.basename)
+        unless PERSISTENT_METADATA_SUBDIRS.include?(subdir.basename)
+          gain_permissions_remove(subdir)
+        end
       end
     end
     Hbc::Utils.rmdir_if_possible(@cask.metadata_versioned_container_path)
@@ -338,6 +340,6 @@ def purge_versioned_files
 
   def purge_caskroom_path
     odebug "Purging all staged versions of Cask #{@cask}"
-    permissions_rmtree(@cask.caskroom_path)
+    gain_permissions_remove(@cask.caskroom_path)
   end
 end

lib/hbc/utils.rb

@@ -159,41 +159,52 @@ def self.rmdir_if_possible(dir)
     end
   end
 
-  def self.permissions_rmtree(path, options = {})
-    command = options.fetch(:command, Hbc::SystemCommand)
+  def self.gain_permissions_remove(path, options = {})
     if path.respond_to?(:rmtree) and path.exist?
-      tried_permissions = false
-      tried_ownership = false
-      begin
+      gain_permissions(path, ['-R'], options) do |path|
         path.rmtree
-      rescue StandardError => e
-        # in case of permissions problems
-        unless tried_permissions
-          # todo Better handling for the case where path is a symlink.
-          #      The -h and -R flags cannot be combined, and behavior is
-          #      dependent on whether the file argument has a trailing
-          #      slash.  This should do the right thing, but is fragile.
-          command.run('/usr/bin/chflags', must_succeed: false,
-            args: ['-R', '--', '000',     path])
-          command.run('/bin/chmod',       must_succeed: false,
-            args: ['-R', '--', 'u+rwx',   path])
-          command.run('/bin/chmod',       must_succeed: false,
-            args: ['-R', '-N',            path])
-          tried_permissions = true
-          retry # rmtree
-        end
-        unless tried_ownership
-          # in case of ownership problems
-          # todo Further examine files to see if ownership is the problem
-          #      before using sudo+chown
-          ohai "Using sudo to gain ownership of path '#{path}'"
-          command.run('/usr/sbin/chown', :args => ['-R', '--', current_user, path],
-                                         :sudo => true)
-          tried_ownership = true
-          # retry chflags/chmod after chown
-          tried_permissions = false
-          retry # rmtree
-        end
+      end
+    elsif File.symlink?(path)
+      gain_permissions(path, ['-h'], options) do |path|
+        FileUtils.rm_f(path)
+      end
+    end
+  end
+
+  def self.gain_permissions(path, command_args, options = {})
+    command = options.fetch(:command, Hbc::SystemCommand)
+    tried_permissions = false
+    tried_ownership = false
+    begin
+      yield path
+    rescue StandardError => e
+      # in case of permissions problems
+      unless tried_permissions
+        # todo Better handling for the case where path is a symlink.
+        #      The -h and -R flags cannot be combined, and behavior is
+        #      dependent on whether the file argument has a trailing
+        #      slash.  This should do the right thing, but is fragile.
+        command.run('/usr/bin/chflags', must_succeed: false,
+          args: command_args + ['--', '000',     path])
+        command.run('/bin/chmod',       must_succeed: false,
+          args: command_args + ['--', 'u+rwx',   path])
+        command.run('/bin/chmod',       must_succeed: false,
+          args: command_args + ['-N',            path])
+        tried_permissions = true
+        retry # rmtree
+      end
+      unless tried_ownership
+        # in case of ownership problems
+        # todo Further examine files to see if ownership is the problem
+        #      before using sudo+chown
+        ohai "Using sudo to gain ownership of path '#{path}'"
+        command.run('/usr/sbin/chown',
+          :args => command_args + ['--', current_user, path],
+          :sudo => true)
+        tried_ownership = true
+        # retry chflags/chmod after chown
+        tried_permissions = false
+        retry # rmtree
       end
     end
   end
@@ -236,6 +247,10 @@ def self.file_is_descendant(file, dir)
     return false
   end
 
+  def self.path_occupied?(path)
+    File.exist?(path) || File.symlink?(path)
+  end
+
   def self.error_message_with_suggestions
     <<-EOS.undent
     #{ Tty.reset.bold }

test/cask/artifact/app_test.rb

@@ -177,6 +177,61 @@
       end
     end
 
+    describe "when the target is a broken symlink" do
+      let(:target_path) {
+        Hbc.appdir.join('Caffeine.app')
+      }
+
+      let(:deleted_path) {
+        local_caffeine.staged_path.join(
+          'Deleted.app')
+      }
+
+      before do
+        deleted_path.mkdir
+        File.symlink(deleted_path, target_path)
+        deleted_path.rmdir
+      end
+
+      it "leaves the target alone" do
+        cask = local_caffeine
+        TestHelper.must_output(self, lambda {
+          Hbc::Artifact::App.new(cask).install_phase
+        }, "==> It seems there is already an App at '#{target_path}'; not moving.")
+
+        File.symlink?(target_path).must_equal true
+      end
+
+      describe "given the force option" do
+        let(:install_phase) {
+          lambda {
+            Hbc::Artifact::App.new(
+              local_caffeine, force: true).install_phase
+          }
+        }
+
+        it "overwrites the existing app" do
+          cask = local_caffeine
+
+          expected = [
+            "==> It seems there is already an App at '#{target_path}'; overwriting.",
+            "==> Removing App: '#{target_path}'",
+            "==> Moving App 'Caffeine.app' to '#{target_path}'"
+          ]
+          TestHelper.must_output(self, install_phase,
+            expected.join("\n"))
+
+          source_path = cask.staged_path.join('Caffeine.app')
+
+          File.exist?(source_path).must_equal false
+          File.ftype(target_path).must_equal 'directory'
+
+          contents_path = target_path.join('Contents/Info.plist')
+          File.exist?(contents_path).must_equal true
+        end
+      end
+    end
+
     it "gives a warning if the source doesn't exist" do
       cask = local_caffeine
       staged_app_path = cask.staged_path.join('Caffeine.app')