Merge pull request #15413 from reitermarkus/symlink-sudo

Use `sudo` for symlinks if necessary.

Library/Homebrew/cask/artifact/moved.rb

@@ -82,13 +82,7 @@ module Cask
 
         ohai "Moving #{self.class.english_name} '#{source.basename}' to '#{target}'"
 
-        unless target.dirname.exist?
+        Utils.gain_permissions_mkpath(target.dirname, command: command) unless target.dirname.exist?
-          if target.dirname.ascend.find(&:directory?).writable?
-            target.dirname.mkpath
-          else
-            command.run!("/bin/mkdir", args: ["-p", target.dirname], sudo: true)
-          end
-        end
 
         if target.directory?
           if target.writable?

Library/Homebrew/cask/artifact/symlinked.rb

@@ -43,7 +43,7 @@ module Cask
 
       private
 
-      def link(force: false, **options)
+      def link(force: false, command: nil, **_options)
         unless source.exist?
           raise CaskError,
                 "It seems the #{self.class.link_type_english_name.downcase} " \
@@ -57,26 +57,29 @@ module Cask
           if force && target.symlink? &&
              (target.realpath == source.realpath || target.realpath.to_s.start_with?("#{cask.caskroom_path}/"))
             opoo "#{message}; overwriting."
-            target.delete
+            Utils.gain_permissions_remove(target, command: command)
           else
             raise CaskError, "#{message}."
           end
         end
 
         ohai "Linking #{self.class.english_name} '#{source.basename}' to '#{target}'"
-        create_filesystem_link(**options)
+        create_filesystem_link(command: command)
       end
 
-      def unlink(**)
+      def unlink(command: nil, **)
         return unless target.symlink?
 
         ohai "Unlinking #{self.class.english_name} '#{target}'"
-        target.delete
+        Utils.gain_permissions_remove(target, command: command)
       end
 
-      def create_filesystem_link(command: nil, **_)
+      def create_filesystem_link(command: nil)
-        target.dirname.mkpath
+        Utils.gain_permissions_mkpath(target.dirname, command: command)
-        command.run!("/bin/ln", args: ["-h", "-f", "-s", "--", source, target])
+
+        command.run! "/bin/ln", args: ["-h", "-f", "-s", "--", source, target],
+                                sudo: !target.dirname.writable?
+
         add_altname_metadata(source, target.basename, command: command)
       end
     end

Library/Homebrew/cask/utils.rb

@@ -11,19 +11,44 @@ module Cask
   #
   # @api private
   module Utils
+    def self.gain_permissions_mkpath(path, command: SystemCommand)
+      dir = path.ascend.find(&:directory?)
+      return if path == dir
+
+      if dir.writable?
+        path.mkpath
+      else
+        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
 
@@ -40,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

@@ -0,0 +1,79 @@
+# frozen_string_literal: true
+
+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
+    it "creates a directory" do
+      expect(path).not_to exist
+      described_class.gain_permissions_mkpath path, command: command
+      expect(path).to be_a_directory
+      described_class.gain_permissions_mkpath path, command: command
+      expect(path).to be_a_directory
+    end
+
+    context "when parent directory is not writable" do
+      it "creates a directory with `sudo`" do
+        FileUtils.chmod "-w", dir
+        expect(dir).not_to be_writable
+
+        expect(command).to receive(:run!).exactly(:once).and_wrap_original do |original, *args, **options|
+          FileUtils.chmod "+w", dir
+          original.call(*args, **options)
+          FileUtils.chmod "-w", dir
+        end
+
+        expect(path).not_to exist
+        described_class.gain_permissions_mkpath path, command: command
+        expect(path).to be_a_directory
+        described_class.gain_permissions_mkpath path, command: command
+        expect(path).to be_a_directory
+
+        expect(dir).not_to be_writable
+        FileUtils.chmod "+w", dir
+      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