unpack_strategy/directory: use mv for nested unpack

`mv` should preserve hardlinks and allow faster unpack on the same
filesystem. A secondary pass is done with `cp` to copy over attributes
onto any existing directories.

We only run this for nested unpacks as most direct Directory strategy
usage is for repositories where moving files breaks existing code.

This uses `cp -pR` for non-move as some potential user reported issues
could be due to Apple's `cp -l` on specific macOS versions. Can
consider re-adding `cp -l` with better handling for older macOS.

Library/Homebrew/test/unpack_strategy/directory_spec.rb

@@ -3,8 +3,6 @@
 require_relative "shared_examples"
 
 RSpec.describe UnpackStrategy::Directory do
-  subject(:strategy) { described_class.new(path) }
-
   let(:path) do
     mktmpdir.tap do |path|
       FileUtils.touch path/"file"
@@ -17,33 +15,100 @@ RSpec.describe UnpackStrategy::Directory do
 
   let(:unpack_dir) { mktmpdir }
 
-  it "does not follow symlinks" do
-    strategy.extract(to: unpack_dir)
-    expect(unpack_dir/"symlink").to be_a_symlink
+  shared_examples "extract directory" do |move:|
+    subject(:strategy) { described_class.new(path, move:) }
+
+    it "does not follow symlinks" do
+      strategy.extract(to: unpack_dir)
+      expect(unpack_dir/"symlink").to be_a_symlink
+    end
+
+    it "does not follow top level symlinks to directories" do
+      strategy.extract(to: unpack_dir)
+      expect(unpack_dir/"folderSymlink").to be_a_symlink
+    end
+
+    it "preserves permissions of contained files" do
+      FileUtils.chmod 0644, path/"file"
+
+      strategy.extract(to: unpack_dir)
+      expect((unpack_dir/"file").stat.mode & 0777).to eq 0644
+    end
+
+    it "preserves permissions of contained subdirectories" do
+      FileUtils.mkdir unpack_dir/"folder"
+      FileUtils.chmod 0755, unpack_dir/"folder"
+      FileUtils.chmod 0700, path/"folder"
+
+      strategy.extract(to: unpack_dir)
+      expect((unpack_dir/"folder").stat.mode & 0777).to eq 0700
+    end
+
+    it "preserves permissions of the destination directory" do
+      FileUtils.chmod 0700, path
+      FileUtils.chmod 0755, unpack_dir
+
+      strategy.extract(to: unpack_dir)
+      expect(unpack_dir.stat.mode & 0777).to eq 0755
+    end
+
+    it "preserves mtime of contained files and directories" do
+      FileUtils.mkdir unpack_dir/"folder"
+      FileUtils.touch path/"folder", mtime: Time.utc(2000, 1, 2, 3, 4, 5, 678999), nocreate: true
+      mtimes = path.children.to_h { |child| [child.basename, child.lstat.mtime] }
+
+      strategy.extract(to: unpack_dir)
+      expect(unpack_dir.children.to_h { |child| [child.basename, child.lstat.mtime] }).to eq mtimes
+    end
+
+    it "preserves unrelated destination files and subdirectories" do
+      FileUtils.touch unpack_dir/"existing_file"
+      FileUtils.mkdir unpack_dir/"existing_folder"
+
+      strategy.extract(to: unpack_dir)
+      expect(unpack_dir/"existing_file").to be_a_file
+      expect(unpack_dir/"existing_folder").to be_a_directory
+    end
+
+    it "overwrites destination files/symlinks with source files/symlinks" do
+      FileUtils.mkdir unpack_dir/"existing_folder"
+      FileUtils.ln_s unpack_dir/"existing_folder", unpack_dir/"symlink"
+      (unpack_dir/"file").write "existing contents"
+
+      strategy.extract(to: unpack_dir)
+      expect((unpack_dir/"file").read).to be_empty
+      expect((unpack_dir/"symlink").readlink).to eq Pathname("file")
+    end
+
+    it "fails when overwriting a directory with a file" do
+      FileUtils.mkdir unpack_dir/"file"
+      expect { strategy.extract(to: unpack_dir) }.to raise_error(/Is a directory|cannot overwrite directory/i)
+    end
+
+    it "fails when overwriting a nested directory with a file" do
+      FileUtils.touch path/"folder/nested"
+      FileUtils.mkdir_p unpack_dir/"folder/nested"
+      expect { strategy.extract(to: unpack_dir) }.to raise_error(/Is a directory|cannot overwrite directory/i)
+    end
   end
 
-  it "does not follow top level symlinks to directories" do
-    strategy.extract(to: unpack_dir)
-    expect(unpack_dir/"folderSymlink").to be_a_symlink
+  context "with `move: false`" do
+    include_examples "extract directory", move: false
   end
 
-  it "preserves hardlinks" do
-    strategy.extract(to: unpack_dir)
-    expect((unpack_dir/"file").stat.ino).to eq (unpack_dir/"hardlink").stat.ino
-  end
+  context "with `move: true`" do
+    include_examples "extract directory", move: true
 
-  it "preserves permissions of contained files" do
-    FileUtils.chmod 0644, path/"file"
+    it "preserves hardlinks" do
+      strategy.extract(to: unpack_dir)
+      expect((unpack_dir/"file").stat.ino).to eq (unpack_dir/"hardlink").stat.ino
+    end
 
-    strategy.extract(to: unpack_dir)
-    expect((unpack_dir/"file").stat.mode & 0777).to eq 0644
-  end
-
-  it "preserves the permissions of the destination directory" do
-    FileUtils.chmod 0700, path
-    FileUtils.chmod 0755, unpack_dir
-
-    strategy.extract(to: unpack_dir)
-    expect(unpack_dir.stat.mode & 0777).to eq 0755
+    # NOTE: We don't test `move: false` because system cp behaviour is inconsistent,
+    # e.g. Ventura cp does not error but Sequoia and Linux cp will error
+    it "fails when overwriting a file with a directory" do
+      FileUtils.touch unpack_dir/"folder"
+      expect { strategy.extract(to: unpack_dir) }.to raise_error(/cannot overwrite non-directory/i)
+    end
   end
 end

Library/Homebrew/unpack_strategy.rb

@@ -183,7 +183,7 @@ module UnpackStrategy
         FileUtils.chmod "u+w", path, verbose:
       end
 
-      Directory.new(tmp_unpack_dir).extract(to:, verbose:)
+      Directory.new(tmp_unpack_dir, move: true).extract(to:, verbose:)
     end
   end
 

Library/Homebrew/unpack_strategy/directory.rb

@@ -16,33 +16,56 @@ module UnpackStrategy
       path.directory?
     end
 
+    sig {
+      params(
+        path:         T.any(String, Pathname),
+        ref_type:     T.nilable(Symbol),
+        ref:          T.nilable(String),
+        merge_xattrs: T::Boolean,
+        move:         T::Boolean,
+      ).void
+    }
+    def initialize(path, ref_type: nil, ref: nil, merge_xattrs: false, move: false)
+      super(path, ref_type:, ref:, merge_xattrs:)
+      @move = move
+    end
+
     private
 
     sig { override.params(unpack_dir: Pathname, basename: Pathname, verbose: T::Boolean).void }
     def extract_to_dir(unpack_dir, basename:, verbose:)
+      move_to_dir(unpack_dir, verbose:) if @move
       path_children = path.children
       return if path_children.empty?
 
-      existing = unpack_dir.children
+      system_command!("cp", args: ["-pR", *path_children, unpack_dir], verbose:)
+    end
 
-      # We run a few cp attempts in the following order:
-      #
-      # 1. Start with `-al` to create hardlinks rather than copying files if the source and
-      #    target are on the same filesystem. On macOS, this is the only cp option that can
-      #    preserve hardlinks but it is only available since macOS 12.3 (file_cmds-353.100.22).
-      # 2. Try `-a` as GNU `cp -a` preserves hardlinks. macOS `cp -a` is identical to `cp -pR`.
-      # 3. Fall back on `-pR` to handle the case where GNU `cp -a` failed. This may happen if
-      #    installing into a filesystem that doesn't support hardlinks like an exFAT USB drive.
-      cp_arg_attempts = ["-a", "-pR"]
-      cp_arg_attempts.unshift("-al") if path.stat.dev == unpack_dir.stat.dev
+    # Move all files from source `path` to target `unpack_dir`. Any existing
+    # subdirectories are not modified and only the contents are moved.
+    #
+    # @raise [RuntimeError] on unsupported `mv` operation, e.g. overwriting a file with a directory
+    sig { params(unpack_dir: Pathname, verbose: T::Boolean).void }
+    def move_to_dir(unpack_dir, verbose:)
+      path.find do |src|
+        next if src == path
 
-      cp_arg_attempts.each do |arg|
-        args = [arg, *path_children, unpack_dir]
-        must_succeed = print_stderr = (arg == cp_arg_attempts.last)
-        result = system_command("cp", args:, verbose:, must_succeed:, print_stderr:)
-        break if result.success?
+        dst = unpack_dir/src.relative_path_from(path)
+        if dst.exist?
+          dst_real_dir = dst.directory? && !dst.symlink?
+          src_real_dir = src.directory? && !src.symlink?
+          # Avoid moving a directory over an existing non-directory and vice versa.
+          # This outputs the same error message as GNU mv which is more readable than macOS mv.
+          raise "mv: cannot overwrite non-directory '#{dst}' with directory '#{src}'" if src_real_dir && !dst_real_dir
+          raise "mv: cannot overwrite directory '#{dst}' with non-directory '#{src}'" if !src_real_dir && dst_real_dir
+          # Defer writing over existing directories. Handle this later on to copy attributes
+          next if dst_real_dir
 
-        FileUtils.rm_r(unpack_dir.children - existing)
+          FileUtils.rm(dst, verbose:)
+        end
+
+        FileUtils.mv(src, dst, verbose:)
+        Find.prune
       end
     end
   end