Merge pull request #1590 from reitermarkus/fail-if-app-source-already-exists

Fail if cask source already exists.

9 files changed, 62 insertions, 38 deletions

diff --git a/Library/Homebrew/cask/lib/hbc/artifact/moved.rb b/Library/Homebrew/cask/lib/hbc/artifact/moved.rb
index f1e542e7a..64756fd93 100644
--- a/Library/Homebrew/cask/lib/hbc/artifact/moved.rb
+++ b/Library/Homebrew/cask/lib/hbc/artifact/moved.rb
@@ -42,12 +42,8 @@ module Hbc
 
       def preflight_checks
         if Utils.path_occupied?(target)
-          if force
-            ohai(warning_target_exists { |s| s << "overwriting." })
-          else
-            ohai(warning_target_exists { |s| s << "not moving." })
-            return false
-          end
+          raise CaskError, warning_target_exists << "." unless force
+          opoo(warning_target_exists { |s| s << "overwriting." })
         end
         unless source.exist?
           message = "It seems the #{self.class.artifact_english_name} source is not there: '#{source}'"
diff --git a/Library/Homebrew/cask/lib/hbc/artifact/symlinked.rb b/Library/Homebrew/cask/lib/hbc/artifact/symlinked.rb
index 32d8d6840..2cd172ad3 100644
--- a/Library/Homebrew/cask/lib/hbc/artifact/symlinked.rb
+++ b/Library/Homebrew/cask/lib/hbc/artifact/symlinked.rb
@@ -39,8 +39,7 @@ module Hbc
 
       def preflight_checks(source, target)
         if target.exist? && !self.class.islink?(target)
-          ohai "It seems there is already #{self.class.artifact_english_article} #{self.class.artifact_english_name} at '#{target}'; not linking."
-          return false
+          raise CaskError, "It seems there is already #{self.class.artifact_english_article} #{self.class.artifact_english_name} at '#{target}'; not linking."
         end
         unless source.exist?
           raise CaskError, "It seems the #{self.class.link_type_english_name.downcase} source is not there: '#{source}'"
diff --git a/Library/Homebrew/cask/lib/hbc/cli/install.rb b/Library/Homebrew/cask/lib/hbc/cli/install.rb
index 3e9ce4e4f..3f4c94b6b 100644
--- a/Library/Homebrew/cask/lib/hbc/cli/install.rb
+++ b/Library/Homebrew/cask/lib/hbc/cli/install.rb
@@ -35,6 +35,8 @@ module Hbc
           rescue CaskNoShasumError => e
             opoo e.message
             count += 1
+          rescue CaskError => e
+            onoe e.message
           end
         end
         count.zero? ? nil : count == cask_tokens.length
diff --git a/Library/Homebrew/cask/spec/cask/artifact/binary_spec.rb b/Library/Homebrew/cask/spec/cask/artifact/binary_spec.rb
index af6973777..fbb117f67 100644
--- a/Library/Homebrew/cask/spec/cask/artifact/binary_spec.rb
+++ b/Library/Homebrew/cask/spec/cask/artifact/binary_spec.rb
@@ -26,9 +26,11 @@ describe Hbc::Artifact::Binary do
   it "avoids clobbering an existing binary by linking over it" do
     FileUtils.touch expected_path
 
-    shutup do
-      Hbc::Artifact::Binary.new(cask).install_phase
-    end
+    expect {
+      shutup do
+        Hbc::Artifact::Binary.new(cask).install_phase
+      end
+    }.to raise_error(Hbc::CaskError)
 
     expect(expected_path).not_to be :symlink?
   end
diff --git a/Library/Homebrew/cask/test/cask/artifact/alt_target_test.rb b/Library/Homebrew/cask/test/cask/artifact/alt_target_test.rb
index d5702d4af..d1fe26eaa 100644
--- a/Library/Homebrew/cask/test/cask/artifact/alt_target_test.rb
+++ b/Library/Homebrew/cask/test/cask/artifact/alt_target_test.rb
@@ -69,9 +69,9 @@ describe Hbc::Artifact::App do
     it "avoids clobbering an existing app by moving over it" do
       target_path.mkpath
 
-      install_phase.must_output <<-EOS.undent
-        ==> It seems there is already an App at '#{target_path}'; not moving.
-      EOS
+      err = install_phase.must_raise(Hbc::CaskError)
+
+      err.message.must_equal("It seems there is already an App at '#{target_path}'.")
 
       source_path.must_be :directory?
       target_path.must_be :directory?
diff --git a/Library/Homebrew/cask/test/cask/artifact/app_test.rb b/Library/Homebrew/cask/test/cask/artifact/app_test.rb
index 795e732ba..3eeeb729f 100644
--- a/Library/Homebrew/cask/test/cask/artifact/app_test.rb
+++ b/Library/Homebrew/cask/test/cask/artifact/app_test.rb
@@ -71,9 +71,9 @@ describe Hbc::Artifact::App do
       end
 
       it "avoids clobbering an existing app" do
-        install_phase.must_output <<-EOS.undent
-          ==> It seems there is already an App at '#{target_path}'; not moving.
-        EOS
+        err = install_phase.must_raise(Hbc::CaskError)
+
+        err.message.must_equal("It seems there is already an App at '#{target_path}'.")
 
         source_path.must_be :directory?
         target_path.must_be :directory?
@@ -92,12 +92,17 @@ describe Hbc::Artifact::App do
 
         describe "target is both writable and user-owned" do
           it "overwrites the existing app" do
-            install_phase.must_output <<-EOS.undent
-              ==> It seems there is already an App at '#{target_path}'; overwriting.
+            stdout = <<-EOS.undent
               ==> Removing App: '#{target_path}'
               ==> Moving App 'Caffeine.app' to '#{target_path}'
             EOS
 
+            stderr = <<-EOS.undent
+              Warning: It seems there is already an App at '#{target_path}'; overwriting.
+            EOS
+
+            install_phase.must_output(stdout, stderr)
+
             source_path.wont_be :exist?
             target_path.must_be :directory?
 
@@ -131,12 +136,17 @@ describe Hbc::Artifact::App do
             command.expect_and_pass_through(chmod_cmd)
             command.expect_and_pass_through(chmod_n_cmd)
 
-            install_phase.must_output <<-EOS.undent
-              ==> It seems there is already an App at '#{target_path}'; overwriting.
+            stdout = <<-EOS.undent
               ==> Removing App: '#{target_path}'
               ==> Moving App 'Caffeine.app' to '#{target_path}'
             EOS
 
+            stderr = <<-EOS.undent
+              Warning: It seems there is already an App at '#{target_path}'; overwriting.
+            EOS
+
+            install_phase.must_output(stdout, stderr)
+
             source_path.wont_be :exist?
             target_path.must_be :directory?
 
@@ -161,9 +171,9 @@ describe Hbc::Artifact::App do
       end
 
       it "leaves the target alone" do
-        install_phase.must_output <<-EOS.undent
-          ==> It seems there is already an App at '#{target_path}'; not moving.
-        EOS
+        err = install_phase.must_raise(Hbc::CaskError)
+
+        err.message.must_equal("It seems there is already an App at '#{target_path}'.")
 
         File.symlink?(target_path).must_equal true
       end
@@ -172,12 +182,17 @@ describe Hbc::Artifact::App do
         let(:force) { true }
 
         it "overwrites the existing app" do
-          install_phase.must_output <<-EOS.undent
-            ==> It seems there is already an App at '#{target_path}'; overwriting.
+          stdout = <<-EOS.undent
             ==> Removing App: '#{target_path}'
             ==> Moving App 'Caffeine.app' to '#{target_path}'
           EOS
 
+          stderr = <<-EOS.undent
+            Warning: It seems there is already an App at '#{target_path}'; overwriting.
+          EOS
+
+          install_phase.must_output(stdout, stderr)
+
           source_path.wont_be :exist?
           target_path.must_be :directory?
 
diff --git a/Library/Homebrew/cask/test/cask/artifact/generic_artifact_test.rb b/Library/Homebrew/cask/test/cask/artifact/generic_artifact_test.rb
index c7ad1da61..42740cd44 100644
--- a/Library/Homebrew/cask/test/cask/artifact/generic_artifact_test.rb
+++ b/Library/Homebrew/cask/test/cask/artifact/generic_artifact_test.rb
@@ -34,8 +34,10 @@ describe Hbc::Artifact::Artifact do
   it "avoids clobbering an existing artifact" do
     target_path.mkpath
 
-    shutup do
-      install_phase.call
+    assert_raises Hbc::CaskError do
+      shutup do
+        install_phase.call
+      end
     end
 
     source_path.must_be :directory?
diff --git a/Library/Homebrew/cask/test/cask/artifact/suite_test.rb b/Library/Homebrew/cask/test/cask/artifact/suite_test.rb
index f14a9a67c..b2949950e 100644
--- a/Library/Homebrew/cask/test/cask/artifact/suite_test.rb
+++ b/Library/Homebrew/cask/test/cask/artifact/suite_test.rb
@@ -33,8 +33,10 @@ describe Hbc::Artifact::Suite do
   it "avoids clobbering an existing suite by moving over it" do
     target_path.mkpath
 
-    shutup do
-      install_phase.call
+    assert_raises Hbc::CaskError do
+      shutup do
+        install_phase.call
+      end
     end
 
     source_path.must_be :directory?
diff --git a/Library/Homebrew/cask/test/cask/artifact/two_apps_correct_test.rb b/Library/Homebrew/cask/test/cask/artifact/two_apps_correct_test.rb
index b37ba370e..c699d247f 100644
--- a/Library/Homebrew/cask/test/cask/artifact/two_apps_correct_test.rb
+++ b/Library/Homebrew/cask/test/cask/artifact/two_apps_correct_test.rb
@@ -64,10 +64,13 @@ describe Hbc::Artifact::App do
       it "when the first app of two already exists" do
         target_path_mini.mkpath
 
-        install_phase.must_output <<-EOS.undent
-          ==> It seems there is already an App at '#{target_path_mini}'; not moving.
-          ==> Moving App 'Caffeine Pro.app' to '#{target_path_pro}'
-        EOS
+        err = assert_raises Hbc::CaskError do
+          install_phase.must_output <<-EOS.undent
+            ==> Moving App 'Caffeine Pro.app' to '#{target_path_pro}'
+          EOS
+        end
+
+        err.message.must_equal("It seems there is already an App at '#{target_path_mini}'.")
 
         source_path_mini.must_be :directory?
         target_path_mini.must_be :directory?
@@ -77,10 +80,13 @@ describe Hbc::Artifact::App do
       it "when the second app of two already exists" do
         target_path_pro.mkpath
 
-        install_phase.must_output <<-EOS.undent
-          ==> Moving App 'Caffeine Mini.app' to '#{target_path_mini}'
-          ==> It seems there is already an App at '#{target_path_pro}'; not moving.
-        EOS
+        err = assert_raises Hbc::CaskError do
+          install_phase.must_output <<-EOS.undent
+            ==> Moving App 'Caffeine Mini.app' to '#{target_path_mini}'
+          EOS
+        end
+
+        err.message.must_equal("It seems there is already an App at '#{target_path_pro}'.")
 
         source_path_pro.must_be :directory?
         target_path_pro.must_be :directory?