Implement @reitermarkus's comments

- Split move into a move_back (and clarify when it is used)
- Remove unused flags
- Raise error if installed Caskfile not found
- Error out if an upgrade fails
- Remove some defensive programming checks

6 files changed, 42 insertions, 20 deletions

diff --git a/Library/Homebrew/cask/lib/hbc/artifact/moved.rb b/Library/Homebrew/cask/lib/hbc/artifact/moved.rb
index dacbe20b7..11e019af2 100644
--- a/Library/Homebrew/cask/lib/hbc/artifact/moved.rb
+++ b/Library/Homebrew/cask/lib/hbc/artifact/moved.rb
@@ -8,11 +8,11 @@ module Hbc
       end
 
       def install_phase(**options)
-        move(source, target, **options)
+        move(**options)
       end
 
       def uninstall_phase(**options)
-        move(target, source, **options)
+        move_back(**options)
       end
 
       def summarize_installed
@@ -25,7 +25,7 @@ module Hbc
 
       private
 
-      def move(source, target, skip: false, force: false, command: nil, **options)
+      def move(force: false, command: nil, **options)
         if Utils.path_occupied?(target)
           message = "It seems there is already #{self.class.english_article} #{self.class.english_name} at '#{target}'"
           raise CaskError, "#{message}." unless force
@@ -34,8 +34,6 @@ module Hbc
         end
 
         unless source.exist?
-          return if skip
-
           raise CaskError, "It seems the #{self.class.english_name} source '#{source}' is not there."
         end
 
@@ -51,6 +49,31 @@ module Hbc
         add_altname_metadata(target, source.basename, command: command)
       end
 
+      def move_back(skip: false, force: false, command: nil, **options)
+        if Utils.path_occupied?(source)
+          message = "It seems there is already #{self.class.english_article} #{self.class.english_name} at '#{source}'"
+          raise CaskError, "#{message}." unless force
+          opoo "#{message}; overwriting."
+          delete(source, force: force, command: command, **options)
+        end
+
+        unless target.exist?
+          return if skip
+          raise CaskError, "It seems the #{self.class.english_name} source '#{target}' is not there."
+        end
+
+        ohai "Moving #{self.class.english_name} '#{target.basename}' back to '#{source}'."
+        source.dirname.mkpath
+
+        if source.parent.writable?
+          FileUtils.move(target, source)
+        else
+          command.run("/bin/mv", args: [target, source], sudo: true)
+        end
+
+        add_altname_metadata(target, source.basename, command: command)
+      end
+
       def delete(target, force: false, command: nil, **_)
         ohai "Removing #{self.class.english_name} '#{target}'."
         raise CaskError, "Cannot remove undeletable #{self.class.english_name}." if MacOS.undeletable?(target)
diff --git a/Library/Homebrew/cask/lib/hbc/cli/upgrade.rb b/Library/Homebrew/cask/lib/hbc/cli/upgrade.rb
index 94fe56a47..993ec6ac6 100644
--- a/Library/Homebrew/cask/lib/hbc/cli/upgrade.rb
+++ b/Library/Homebrew/cask/lib/hbc/cli/upgrade.rb
@@ -4,7 +4,6 @@ module Hbc
       option "--greedy", :greedy, false
       option "--quiet",  :quiet, false
       option "--force", :force, false
-      option "--force-update", :force_update, false
       option "--skip-cask-deps", :skip_cask_deps, false
 
       def initialize(*)
@@ -24,10 +23,9 @@ module Hbc
           odebug "Started upgrade process for Cask #{old_cask}"
           raise CaskNotInstalledError, old_cask unless old_cask.installed? || force?
 
-          unless old_cask.installed_caskfile.nil?
-            # use the same cask file that was used for installation, if possible
-            old_cask = CaskLoader.load(old_cask.installed_caskfile) if old_cask.installed_caskfile.exist?
-          end
+          raise CaskUnavailableError.new(old_cask, "The Caskfile is missing!") if old_cask.installed_caskfile.nil?
+
+          old_cask = CaskLoader.load(old_cask.installed_caskfile)
 
           old_cask_installer = Installer.new(old_cask, binaries: binaries?, verbose: verbose?, force: force?, upgrade: true)
 
@@ -66,10 +64,10 @@ module Hbc
             # If successful, wipe the old Cask from staging
             old_cask_installer.finalize_upgrade
           rescue CaskError => e
-            opoo e.message
             new_cask_installer.uninstall_artifacts if new_artifacts_installed
             new_cask_installer.purge_versioned_files
             old_cask_installer.revert_upgrade if started_upgrade
+            raise e
           end
         end
       end
diff --git a/Library/Homebrew/cask/lib/hbc/installer.rb b/Library/Homebrew/cask/lib/hbc/installer.rb
index aa6f04dbe..6056250fc 100644
--- a/Library/Homebrew/cask/lib/hbc/installer.rb
+++ b/Library/Homebrew/cask/lib/hbc/installer.rb
@@ -83,8 +83,8 @@ module Hbc
     def install
       odebug "Hbc::Installer#install"
 
-      if @cask.installed? && !force? && !@reinstall
-        raise CaskAlreadyInstalledError, @cask unless upgrade?
+      if @cask.installed? && !force? && !@reinstall && !upgrade?
+        raise CaskAlreadyInstalledError, @cask
       end
 
       check_conflicts
@@ -374,7 +374,6 @@ module Hbc
     end
 
     def start_upgrade
-      return unless upgrade?
       oh1 "Starting upgrade for Cask #{@cask}"
 
       disable_accessibility_access
@@ -382,14 +381,12 @@ module Hbc
     end
 
     def revert_upgrade
-      return unless upgrade?
       opoo "Reverting upgrade for Cask #{@cask}"
       install_artifacts
       enable_accessibility_access
     end
 
     def finalize_upgrade
-      return unless upgrade?
       purge_versioned_files
 
       puts summary
diff --git a/Library/Homebrew/test/cask/cli/reinstall_spec.rb b/Library/Homebrew/test/cask/cli/reinstall_spec.rb
index 1df814715..3737a7a70 100644
--- a/Library/Homebrew/test/cask/cli/reinstall_spec.rb
+++ b/Library/Homebrew/test/cask/cli/reinstall_spec.rb
@@ -13,7 +13,7 @@ describe Hbc::CLI::Reinstall, :cask do
       Already downloaded: .*local-caffeine--1.2.3.zip
       ==> Verifying checksum for Cask local-caffeine
       ==> Uninstalling Cask local-caffeine
-      ==> Moving App 'Caffeine.app' to '.*Caffeine.app'.
+      ==> Moving App 'Caffeine.app' back to '.*Caffeine.app'.
       ==> Purging files for version 1.2.3 of Cask local-caffeine
       ==> Installing Cask local-caffeine
       ==> Moving App 'Caffeine.app' to '.*Caffeine.app'.
diff --git a/Library/Homebrew/test/cask/cli/uninstall_spec.rb b/Library/Homebrew/test/cask/cli/uninstall_spec.rb
index 578c0d43a..345e1b9f2 100644
--- a/Library/Homebrew/test/cask/cli/uninstall_spec.rb
+++ b/Library/Homebrew/test/cask/cli/uninstall_spec.rb
@@ -12,7 +12,7 @@ describe Hbc::CLI::Uninstall, :cask do
 
     output = Regexp.new <<~EOS
       ==> Uninstalling Cask local-caffeine
-      ==> Moving App 'Caffeine.app' to '.*Caffeine.app'.
+      ==> Moving App 'Caffeine.app' back to '.*Caffeine.app'.
       ==> Purging files for version 1.2.3 of Cask local-caffeine
     EOS
 
diff --git a/Library/Homebrew/test/cask/cli/upgrade_spec.rb b/Library/Homebrew/test/cask/cli/upgrade_spec.rb
index ee7ef2261..5c08a4728 100644
--- a/Library/Homebrew/test/cask/cli/upgrade_spec.rb
+++ b/Library/Homebrew/test/cask/cli/upgrade_spec.rb
@@ -158,7 +158,9 @@ describe Hbc::CLI::Upgrade, :cask do
       expect(Hbc::CaskLoader.load("will-fail-if-upgraded").versions).to include("1.2.2")
 
       expect {
-        described_class.run("will-fail-if-upgraded")
+        expect {
+          described_class.run("will-fail-if-upgraded")
+        }.to raise_error(Hbc::CaskError)
       }.to output(output_reverted).to_stderr
 
       expect(Hbc::CaskLoader.load("will-fail-if-upgraded")).to be_installed
@@ -173,7 +175,9 @@ describe Hbc::CLI::Upgrade, :cask do
       expect(Hbc::CaskLoader.load("bad-checksum").versions).to include("1.2.2")
 
       expect {
-        described_class.run("bad-checksum")
+        expect {
+          described_class.run("bad-checksum")
+        }.to raise_error(Hbc::CaskSha256MismatchError)
       }.to_not output(output_reverted).to_stderr
 
       expect(Hbc::CaskLoader.load("bad-checksum")).to be_installed