Improve Cask::Utils.gain_permissions_remove for symlinks.

Library/Homebrew/cask/utils.rb

@@ -18,23 +18,37 @@ module Cask
       if dir.writable?
         path.mkpath
       else
-        command.run!("/bin/mkdir", args: ["-p", path], sudo: true)
+        command.run!("/bin/mkdir", args: ["-p", "--", path], sudo: true)
       end
     end
 
     def self.gain_permissions_remove(path, command: SystemCommand)
-      if path.respond_to?(:rmtree) && path.exist?
+      directory = false
-        gain_permissions(path, ["-R"], command) do |p|
+      permission_flags = if path.symlink?
-          if p.parent.writable?
+        ["-h"]
+      elsif path.directory?
+        directory = true
+        ["-R"]
+      elsif path.exist?
+        []
+      else
+        # Nothing to remove.
+        return
+      end
+
+      gain_permissions(path, permission_flags, command) do |p|
+        if p.parent.writable?
+          if directory
             p.rmtree
           else
-            command.run("/bin/rm",
+            FileUtils.rm_f p
-                        args: ["-r", "-f", "--", p],
-                        sudo: true)
           end
+        else
+          recursive_flag = directory ? ["-R"] : []
+          command.run!("/bin/rm",
+                       args: recursive_flag + ["-f", "--", p],
+                       sudo: true)
         end
-      elsif File.symlink?(path)
-        gain_permissions(path, ["-h"], command, &FileUtils.method(:rm_f))
       end
     end
 
@@ -51,13 +65,10 @@ module Cask
           #       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

Library/Homebrew/test/cask/artifact/app_spec.rb

@@ -171,21 +171,12 @@ describe Cask::Artifact::App, :cask do
           end
 
           it "overwrites the existing app" do
-            expect(command).to receive(:run).with(
+            expect(command).to receive(:run).with("/usr/bin/chflags",
-              "/bin/chmod", args: [
+                                                  args: ["-R", "--", "000", target_path]).and_call_original
-                "-R", "--", "u+rwx", target_path
+            expect(command).to receive(:run).with("/bin/chmod",
-              ], must_succeed: false
+                                                  args: ["-R", "--", "u+rwx", target_path]).and_call_original
-            ).and_call_original
+            expect(command).to receive(:run).with("/bin/chmod",
-            expect(command).to receive(:run).with(
+                                                  args: ["-R", "-N", target_path]).and_call_original
-              "/bin/chmod", args: [
-                "-R", "-N", target_path
-              ], must_succeed: false
-            ).and_call_original
-            expect(command).to receive(:run).with(
-              "/usr/bin/chflags", args: [
-                "-R", "--", "000", target_path
-              ], must_succeed: false
-            ).and_call_original
 
             stdout = <<~EOS
               ==> Removing App '#{target_path}'

Library/Homebrew/test/cask/utils_spec.rb

@@ -2,11 +2,11 @@
 
 describe Cask::Utils do
   let(:command) { NeverSudoSystemCommand }
+  let(:dir) { mktmpdir }
+  let(:path) { dir/"a/b/c" }
+  let(:link) { dir/"link" }
 
   describe "::gain_permissions_mkpath" do
-    let(:dir) { mktmpdir }
-    let(:path) { dir/"a/b/c" }
-
     it "creates a directory" do
       expect(path).not_to exist
       described_class.gain_permissions_mkpath path, command: command
@@ -37,4 +37,43 @@ describe Cask::Utils do
       end
     end
   end
+
+  describe "::gain_permissions_remove" do
+    it "removes the symlink, not the file it points to" do
+      path.dirname.mkpath
+      FileUtils.touch path
+      FileUtils.ln_s path, link
+
+      expect(path).to be_a_file
+      expect(link).to be_a_symlink
+      expect(link.realpath).to eq path
+
+      described_class.gain_permissions_remove link, command: command
+
+      expect(path).to be_a_file
+      expect(link).not_to exist
+
+      described_class.gain_permissions_remove path, command: command
+
+      expect(path).not_to exist
+    end
+
+    it "removes the symlink, not the directory it points to" do
+      path.mkpath
+      FileUtils.ln_s path, link
+
+      expect(path).to be_a_directory
+      expect(link).to be_a_symlink
+      expect(link.realpath).to eq path
+
+      described_class.gain_permissions_remove link, command: command
+
+      expect(path).to be_a_directory
+      expect(link).not_to exist
+
+      described_class.gain_permissions_remove path, command: command
+
+      expect(path).not_to exist
+    end
+  end
 end