Merge pull request #18420 from ctaintor/improve_adopt_cask

Improve cask --adopt to only care about the installed version if auto…

Library/Homebrew/cask/artifact/moved.rb

@@ -31,7 +31,7 @@ module Cask
 
       private
 
-      def move(adopt: false, force: false, verbose: false, predecessor: nil, reinstall: false,
+      def move(adopt: false, auto_updates: 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."
@@ -51,38 +51,40 @@ module Cask
             if adopt
               ohai "Adopting existing #{self.class.english_name} at '#{target}'"
 
-              source_plist = Pathname("#{source}/Contents/Info.plist")
-              target_plist = Pathname("#{target}/Contents/Info.plist")
-              same = if source_plist.size? &&
-                        (source_bundle_version = Homebrew::BundleVersion.from_info_plist(source_plist)) &&
-                        target_plist.size? &&
-                        (target_bundle_version = Homebrew::BundleVersion.from_info_plist(target_plist))
-                if source_bundle_version.short_version == target_bundle_version.short_version
-                  if source_bundle_version.version == target_bundle_version.version
-                    true
+              unless auto_updates
+                source_plist = Pathname("#{source}/Contents/Info.plist")
+                target_plist = Pathname("#{target}/Contents/Info.plist")
+                same = if source_plist.size? &&
+                          (source_bundle_version = Homebrew::BundleVersion.from_info_plist(source_plist)) &&
+                          target_plist.size? &&
+                          (target_bundle_version = Homebrew::BundleVersion.from_info_plist(target_plist))
+                  if source_bundle_version.short_version == target_bundle_version.short_version
+                    if source_bundle_version.version == target_bundle_version.version
+                      true
+                    else
+                      onoe "The bundle version of #{source} is #{source_bundle_version.version} but " \
+                           "is #{target_bundle_version.version} for #{target}!"
+                      false
+                    end
                   else
-                    onoe "The bundle version of #{source} is #{source_bundle_version.version} but " \
-                         "is #{target_bundle_version.version} for #{target}!"
+                    onoe "The bundle short version of #{source} is #{source_bundle_version.short_version} but " \
+                         "is #{target_bundle_version.short_version} for #{target}!"
                     false
                   end
                 else
-                  onoe "The bundle short version of #{source} is #{source_bundle_version.short_version} but " \
-                       "is #{target_bundle_version.short_version} for #{target}!"
-                  false
+                  command.run(
+                    "/usr/bin/diff",
+                    args:         ["--recursive", "--brief", source, target],
+                    verbose:,
+                    print_stdout: verbose,
+                  ).success?
                 end
-              else
-                command.run(
-                  "/usr/bin/diff",
-                  args:         ["--recursive", "--brief", source, target],
-                  verbose:,
-                  print_stdout: verbose,
-                ).success?
-              end
 
-              unless same
-                raise CaskError,
-                      "It seems the existing #{self.class.english_name} is different from " \
-                      "the one being installed."
+                unless same
+                  raise CaskError,
+                        "It seems the existing #{self.class.english_name} is different from " \
+                        "the one being installed."
+                end
               end
 
               # Remove the source as we don't need to move it to the target location

Library/Homebrew/cask/installer.rb

@@ -253,7 +253,8 @@ on_request: true)
         next if artifact.is_a?(Artifact::Binary) && !binaries?
 
         artifact.install_phase(
-          command: @command, verbose: verbose?, adopt: adopt?, force: force?, predecessor:,
+          command: @command, verbose: verbose?, adopt: adopt?, auto_updates: @cask.auto_updates,
+          force: force?, predecessor:
         )
         already_installed_artifacts.unshift(artifact)
       end

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

@@ -5,12 +5,13 @@ RSpec.describe Cask::Artifact::App, :cask do
   let(:command) { NeverSudoSystemCommand }
   let(:adopt) { false }
   let(:force) { false }
+  let(:auto_updates) { false }
   let(:app) { cask.artifacts.find { |a| a.is_a?(described_class) } }
 
   let(:source_path) { cask.staged_path.join("Caffeine.app") }
   let(:target_path) { cask.config.appdir.join("Caffeine.app") }
 
-  let(:install_phase) { app.install_phase(command:, adopt:, force:) }
+  let(:install_phase) { app.install_phase(command:, adopt:, force:, auto_updates:) }
   let(:uninstall_phase) { app.uninstall_phase(command:, force:) }
 
   before do
@@ -83,24 +84,55 @@ RSpec.describe Cask::Artifact::App, :cask do
         let(:adopt) { true }
 
         describe "when the target compares different from the source" do
-          it "avoids clobbering the existing app" do
-            stdout = <<~EOS
-              ==> Adopting existing App at '#{target_path}'
-            EOS
+          describe "when the cask does not auto_updates" do
+            it "avoids clobbering the existing app if brew manages updates" do
+              stdout = <<~EOS
+                ==> Adopting existing App at '#{target_path}'
+              EOS
 
-            expect { install_phase }
-              .to output(stdout).to_stdout
-              .and raise_error(
-                Cask::CaskError,
-                "It seems the existing App is different from the one being installed.",
-              )
+              expect { install_phase }
+                .to output(stdout).to_stdout
+                .and raise_error(
+                  Cask::CaskError,
+                  "It seems the existing App is different from the one being installed.",
+                )
 
-            expect(source_path).to be_a_directory
-            expect(target_path).to be_a_directory
-            expect(File.identical?(source_path, target_path)).to be false
+              expect(source_path).to be_a_directory
+              expect(target_path).to be_a_directory
+              expect(File.identical?(source_path, target_path)).to be false
 
-            contents_path = target_path.join("Contents/Info.plist")
-            expect(contents_path).not_to exist
+              contents_path = target_path.join("Contents/Info.plist")
+              expect(contents_path).not_to exist
+            end
+          end
+
+          describe "when the cask auto_updates" do
+            before do
+              target_path.delete
+              FileUtils.cp_r source_path, target_path
+              File.write(target_path.join("Contents/Info.plist"), "different")
+            end
+
+            let(:auto_updates) { true }
+
+            it "adopts the existing app" do
+              stdout = <<~EOS
+                ==> Adopting existing App at '#{target_path}'
+              EOS
+
+              stderr = ""
+
+              expect { install_phase }
+                .to output(stdout).to_stdout
+                .and output(stderr).to_stderr
+
+              expect(source_path).to be_a_symlink
+              expect(target_path).to be_a_directory
+
+              contents_path = target_path.join("Contents/Info.plist")
+              expect(contents_path).to exist
+              expect(File.read(contents_path)).to eq("different")
+            end
           end
         end