Don't remove cask directories when upgrading.

Library/Homebrew/cask/artifact/abstract_uninstall.rb

@@ -268,8 +268,8 @@ def uninstall_signal(*signals, command: nil, **_)
         end
       end
 
-      def uninstall_login_item(*login_items, command: nil, upgrade: false, **_)
-        return if upgrade
+      def uninstall_login_item(*login_items, command: nil, successor: nil, **_)
+        return if successor
 
         apps = cask.artifacts.select { |a| a.class.dsl_key == :app }
         derived_login_items = apps.map { |a| { path: a.target } }

Library/Homebrew/cask/artifact/moved.rb

@@ -2,6 +2,7 @@
 # frozen_string_literal: true
 
 require "cask/artifact/relocated"
+require "cask/quarantine"
 
 module Cask
   module Artifact
@@ -32,39 +33,51 @@ def summarize_installed
 
       private
 
-      def move(adopt: false, force: false, verbose: false, command: nil, **options)
+      def move(adopt: false, force: false, verbose: false, predecessor: nil, reinstall: false,
+               command: nil, **options)
         unless source.exist?
           raise CaskError, "It seems the #{self.class.english_name} source '#{source}' is not there."
         end
 
         if Utils.path_occupied?(target)
-          if adopt
-            ohai "Adopting existing #{self.class.english_name} at '#{target}'"
-            same = command.run(
-              "/usr/bin/diff",
-              args:         ["--recursive", "--brief", source, target],
-              verbose:      verbose,
-              print_stdout: verbose,
-            ).success?
-
-            unless same
-              raise CaskError,
-                    "It seems the existing #{self.class.english_name} is different from " \
-                    "the one being installed."
+          if target.directory? && target.children.empty? && matching_artifact?(predecessor)
+            # An upgrade removed the directory contents but left the directory itself (see below).
+            unless source.directory?
+              if target.parent.writable? && !force
+                target.rmdir
+              else
+                Utils.gain_permissions_remove(target, command: command)
+              end
+            end
+          else
+            if adopt
+              ohai "Adopting existing #{self.class.english_name} at '#{target}'"
+              same = command.run(
+                "/usr/bin/diff",
+                args:         ["--recursive", "--brief", source, target],
+                verbose:      verbose,
+                print_stdout: verbose,
+              ).success?
+
+              unless same
+                raise CaskError,
+                      "It seems the existing #{self.class.english_name} is different from " \
+                      "the one being installed."
+              end
+
+              # Remove the source as we don't need to move it to the target location
+              source.rmtree
+
+              return post_move(command)
             end
 
-            # Remove the source as we don't need to move it to the target location
-            source.rmtree
+            message = "It seems there is already #{self.class.english_article} " \
+                      "#{self.class.english_name} at '#{target}'"
+            raise CaskError, "#{message}." unless force
 
-            return post_move(command)
+            opoo "#{message}; overwriting."
+            delete(target, force: force, command: command, **options)
           end
-
-          message = "It seems there is already #{self.class.english_article} " \
-                    "#{self.class.english_name} at '#{target}'"
-          raise CaskError, "#{message}." unless force
-
-          opoo "#{message}; overwriting."
-          delete(target, force: force, command: command, **options)
         end
 
         ohai "Moving #{self.class.english_name} '#{source.basename}' to '#{target}'"
@@ -77,7 +90,16 @@ def move(adopt: false, force: false, verbose: false, command: nil, **options)
           end
         end
 
-        if target.dirname.writable?
+        if target.directory?
+          if target.writable?
+            source.children.each { |child| FileUtils.move(child, target + child.basename) }
+          else
+            command.run!("/bin/cp", args: ["-pR", "#{source}/*", "#{source}/.*", "#{target}/"],
+                                    sudo: true)
+          end
+          Quarantine.copy_xattrs(source, target)
+          source.rmtree
+        elsif target.dirname.writable?
           FileUtils.move(source, target)
         else
           # default sudo user isn't necessarily able to write to Homebrew's locations
@@ -96,6 +118,14 @@ def post_move(command)
         add_altname_metadata(target, source.basename, command: command)
       end
 
+      def matching_artifact?(cask)
+        return false unless cask
+
+        cask.artifacts.any? do |a|
+          a.instance_of?(self.class) && instance_of?(a.class) && a.target == target
+        end
+      end
+
       def move_back(skip: false, force: false, command: nil, **options)
         FileUtils.rm source if source.symlink? && source.dirname.join(source.readlink) == target
 
@@ -123,13 +153,23 @@ def move_back(skip: false, force: false, command: nil, **options)
         delete(target, force: force, command: command, **options)
       end
 
-      def delete(target, force: false, command: nil, **_)
+      def delete(target, force: false, successor: nil, command: nil, **_)
         ohai "Removing #{self.class.english_name} '#{target}'"
         raise CaskError, "Cannot remove undeletable #{self.class.english_name}." if MacOS.undeletable?(target)
 
         return unless Utils.path_occupied?(target)
 
-        if target.parent.writable? && !force
+        if target.directory? && matching_artifact?(successor)
+          # If an app folder is deleted, macOS considers the app uninstalled and removes some data.
+          # Remove only the contents to handle this case.
+          target.children.each do |child|
+            if target.writable? && !force
+              child.rmtree
+            else
+              Utils.gain_permissions_remove(child, command: command)
+            end
+          end
+        elsif target.parent.writable? && !force
           target.rmtree
         else
           Utils.gain_permissions_remove(target, command: command)

Library/Homebrew/cask/installer.rb

@@ -20,7 +20,7 @@ class Installer
 
     def initialize(cask, command: SystemCommand, force: false, adopt: false,
                    skip_cask_deps: false, binaries: true, verbose: false,
-                   zap: false, require_sha: false, upgrade: false,
+                   zap: false, require_sha: false, upgrade: false, reinstall: false,
                    installed_as_dependency: false, quarantine: true,
                    verify_download_integrity: true, quiet: false)
       @cask = cask
@@ -32,7 +32,7 @@ def initialize(cask, command: SystemCommand, force: false, adopt: false,
       @verbose = verbose
       @zap = zap
       @require_sha = require_sha
-      @reinstall = false
+      @reinstall = reinstall
       @upgrade = upgrade
       @installed_as_dependency = installed_as_dependency
       @quarantine = quarantine
@@ -93,6 +93,7 @@ def install
 
         raise CaskAlreadyInstalledError, @cask
       end
+      predecessor = @cask if reinstall? && @cask.installed?
 
       check_conflicts
 
@@ -108,7 +109,7 @@ def install
 
       @cask.config = @cask.default_config.merge(old_config)
 
-      install_artifacts
+      install_artifacts(predecessor: predecessor)
 
       if (tap = @cask.tap) && tap.should_report_analytics?
         ::Utils::Analytics.report_event(:cask_install, package_name: @cask.token, tap_name: tap.name,
@@ -141,18 +142,12 @@ def check_conflicts
       end
     end
 
-    def reinstall
-      odebug "Cask::Installer#reinstall"
-      @reinstall = true
-      install
-    end
-
     def uninstall_existing_cask
       return unless @cask.installed?
 
       # Always force uninstallation, ignore method parameter
-      cask_installer = Installer.new(@cask, verbose: verbose?, force: true, upgrade: upgrade?)
-      zap? ? cask_installer.zap : cask_installer.uninstall
+      cask_installer = Installer.new(@cask, verbose: verbose?, force: true, upgrade: upgrade?, reinstall: true)
+      zap? ? cask_installer.zap : cask_installer.uninstall(successor: @cask)
     end
 
     sig { returns(String) }
@@ -219,7 +214,7 @@ def extract_primary_container(to: @cask.staged_path)
       Quarantine.propagate(from: primary_container.path, to: to)
     end
 
-    def install_artifacts
+    def install_artifacts(predecessor: nil)
       artifacts = @cask.artifacts
       already_installed_artifacts = []
 
@@ -232,7 +227,8 @@ def install_artifacts
 
         next if artifact.is_a?(Artifact::Binary) && !binaries?
 
-        artifact.install_phase(command: @command, verbose: verbose?, adopt: adopt?, force: force?)
+        artifact.install_phase(command: @command, verbose: verbose?, adopt: adopt?, force: force?,
+                               predecessor: predecessor)
         already_installed_artifacts.unshift(artifact)
       end
 
@@ -394,10 +390,10 @@ def save_download_sha
       @cask.download_sha_path.atomic_write(@cask.new_download_sha) if @cask.checksumable?
     end
 
-    def uninstall
+    def uninstall(successor: nil)
       load_installed_caskfile!
       oh1 "Uninstalling Cask #{Formatter.identifier(@cask)}"
-      uninstall_artifacts(clear: true)
+      uninstall_artifacts(clear: true, successor: successor)
       if !reinstall? && !upgrade?
         remove_download_sha
         remove_config_file
@@ -415,8 +411,8 @@ def remove_download_sha
       FileUtils.rm_f @cask.download_sha_path if @cask.download_sha_path.exist?
     end
 
-    def start_upgrade
-      uninstall_artifacts
+    def start_upgrade(successor:)
+      uninstall_artifacts(successor: successor)
       backup
     end
 
@@ -435,10 +431,10 @@ def restore_backup
       backup_metadata_path.rename @cask.metadata_versioned_path
     end
 
-    def revert_upgrade
+    def revert_upgrade(predecessor)
       opoo "Reverting upgrade for Cask #{@cask}"
       restore_backup
-      install_artifacts
+      install_artifacts(predecessor: predecessor)
     end
 
     def finalize_upgrade
@@ -449,7 +445,7 @@ def finalize_upgrade
       puts summary
     end
 
-    def uninstall_artifacts(clear: false)
+    def uninstall_artifacts(clear: false, successor: nil)
       artifacts = @cask.artifacts
 
       odebug "Uninstalling artifacts"
@@ -459,15 +455,23 @@ def uninstall_artifacts(clear: false)
         if artifact.respond_to?(:uninstall_phase)
           odebug "Uninstalling artifact of class #{artifact.class}"
           artifact.uninstall_phase(
-            command: @command, verbose: verbose?, skip: clear, force: force?, upgrade: upgrade?,
+            command:   @command,
+            verbose:   verbose?,
+            skip:      clear,
+            force:     force?,
+            successor: successor,
           )
         end
 
         next unless artifact.respond_to?(:post_uninstall_phase)
 
         odebug "Post-uninstalling artifact of class #{artifact.class}"
         artifact.post_uninstall_phase(
-          command: @command, verbose: verbose?, skip: clear, force: force?, upgrade: upgrade?,
+          command:   @command,
+          verbose:   verbose?,
+          skip:      clear,
+          force:     force?,
+          successor: successor,
         )
       end
     end

Library/Homebrew/cask/quarantine.rb

@@ -12,6 +12,7 @@ module Quarantine
     QUARANTINE_ATTRIBUTE = "com.apple.quarantine"
 
     QUARANTINE_SCRIPT = (HOMEBREW_LIBRARY_PATH/"cask/utils/quarantine.swift").freeze
+    COPY_XATTRS_SCRIPT = (HOMEBREW_LIBRARY_PATH/"cask/utils/copy-xattrs.swift").freeze
 
     def self.swift
       @swift ||= DevelopmentTools.locate("swift")
@@ -172,5 +173,19 @@ def self.propagate(from: nil, to: nil)
 
       raise CaskQuarantinePropagationError.new(to, quarantiner.stderr)
     end
+
+    def self.copy_xattrs(from, to)
+      odebug "Copying xattrs from #{from} to #{to}"
+
+      system_command!(
+        swift,
+        args: [
+          *swift_target_args,
+          COPY_XATTRS_SCRIPT,
+          from,
+          to,
+        ],
+      )
+    end
   end
 end

Library/Homebrew/cask/reinstall.rb

@@ -26,8 +26,9 @@ def self.reinstall_casks(
                       force:          force,
                       skip_cask_deps: skip_cask_deps,
                       require_sha:    require_sha,
+                      reinstall:      true,
                       quarantine:     quarantine,
-                      zap:            zap).reinstall
+                      zap:            zap).install
       end
     end
   end

Library/Homebrew/cask/upgrade.rb

@@ -180,22 +180,22 @@ def self.upgrade_cask(
         new_cask_installer.fetch
 
         # Move the old cask's artifacts back to staging
-        old_cask_installer.start_upgrade
+        old_cask_installer.start_upgrade(successor: new_cask)
         # And flag it so in case of error
         started_upgrade = true
 
         # Install the new cask
         new_cask_installer.stage
 
-        new_cask_installer.install_artifacts
+        new_cask_installer.install_artifacts(predecessor: old_cask)
         new_artifacts_installed = true
 
         # If successful, wipe the old cask from staging
         old_cask_installer.finalize_upgrade
       rescue => e
-        new_cask_installer.uninstall_artifacts if new_artifacts_installed
+        new_cask_installer.uninstall_artifacts(successor: old_cask) if new_artifacts_installed
         new_cask_installer.purge_versioned_files
-        old_cask_installer.revert_upgrade if started_upgrade
+        old_cask_installer.revert_upgrade(predecessor: new_cask) if started_upgrade
         raise e
       end