Cleanup tap_git_head and uninstall_flight_blocks?

Co-authored-by: Kevin <apainintheneck@gmail.com>

Library/Homebrew/cask/artifact/abstract_flight_block.rb

@@ -34,12 +34,6 @@ module Cask
         directives.keys.map(&:to_s).join(", ")
       end
 
-      def uninstall?
-        directives.keys.any? do |key|
-          key.to_s.start_with?("uninstall_")
-        end
-      end
-
       private
 
       def class_for_dsl_key(dsl_key)

Library/Homebrew/cask/cask.rb

@@ -161,7 +161,12 @@ module Cask
 
     def uninstall_flight_blocks?
       artifacts.any? do |artifact|
-        artifact.is_a?(Artifact::AbstractFlightBlock) && artifact.uninstall?
+        case artifact
+        when Artifact::PreflightBlock
+          artifact.directives.key?(:uninstall_preflight)
+        when Artifact::PostflightBlock
+          artifact.directives.key?(:uninstall_postflight)
+        end
       end
     end
 
@@ -480,11 +485,13 @@ module Cask
       artifacts.filter_map do |artifact|
         case artifact
         when Artifact::AbstractFlightBlock
-          next if uninstall_only && !artifact.uninstall?
+          uninstall_flight_block = artifact.directives.key?(:uninstall_preflight) ||
+                                   artifact.directives.key?(:uninstall_postflight)
+          next if uninstall_only && !uninstall_flight_block
 
           # Only indicate whether this block is used as we don't load it from the API
           # We can skip this entirely once we move to internal JSON v3.
-          { artifact.summarize => nil } unless compact
+          { artifact.summarize.to_sym => nil } unless compact
         else
           zap_artifact = artifact.is_a?(Artifact::Zap)
           uninstall_artifact = artifact.respond_to?(:uninstall_phase) || artifact.respond_to?(:post_uninstall_phase)

Library/Homebrew/cask/installer.rb

@@ -426,7 +426,7 @@ on_request: true)
 
     def remove_tabfile
       tabfile = @cask.tab.tabfile
-      FileUtils.rm_f tabfile if tabfile.present? && tabfile.exist?
+      FileUtils.rm_f tabfile if tabfile
       @cask.config_path.parent.rmdir_if_possible
     end
 

Library/Homebrew/cask/tab.rb

@@ -32,7 +32,7 @@ module Cask
       tab.source = {
         "path"         => cask.sourcefile_path.to_s,
         "tap"          => cask.tap&.name,
-        "tap_git_head" => tap_git_head(cask),
+        "tap_git_head" => cask.tap_git_head,
         "version"      => cask.version.to_s,
       }
       tab.uninstall_artifacts = cask.artifacts_list(uninstall_only: true)

Library/Homebrew/tab.rb

@@ -41,7 +41,7 @@ class AbstractTab
       "arch"                    => Hardware::CPU.arch,
       "source"                  => {
         "tap"          => formula_or_cask.tap&.name,
-        "tap_git_head" => tap_git_head(formula_or_cask),
+        "tap_git_head" => formula_or_cask.tap_git_head,
       },
       "built_on"                => DevelopmentTools.build_system_info,
     }
@@ -93,12 +93,6 @@ class AbstractTab
     new(attributes)
   end
 
-  def self.tap_git_head(formula_or_cask)
-    return unless formula_or_cask.tap&.installed?
-
-    formula_or_cask.tap.git_head
-  end
-
   def initialize(attributes = {})
     attributes.each { |key, value| instance_variable_set(:"@#{key}", value) }
   end
@@ -134,6 +128,7 @@ class Tab < AbstractTab
 
   attr_accessor :built_as_bottle, :changed_files, :stdlib, :aliases
   attr_writer :used_options, :unused_options, :compiler, :source_modified_time
+  attr_reader :tapped_from
 
   # Instantiates a {Tab} for a new installation of a formula.
   def self.create(formula, compiler, stdlib)
@@ -169,8 +164,7 @@ class Tab < AbstractTab
     tab.source_modified_time ||= 0
     tab.source ||= {}
 
-    tapped_from = tab.instance_variable_get(:@tapped_from)
+    tab.tap = tab.tapped_from if !tab.tapped_from.nil? && tab.tapped_from != "path or URL"
-    tab.tap = tapped_from if !tapped_from.nil? && tapped_from != "path or URL"
     tab.tap = "homebrew/core" if tab.tap == "mxcl/master" || tab.tap == "Homebrew/homebrew"
 
     if tab.source["spec"].nil?
@@ -255,7 +249,7 @@ class Tab < AbstractTab
       tab.source = {
         "path"         => formula.specified_path.to_s,
         "tap"          => formula.tap&.name,
-        "tap_git_head" => tap_git_head(formula),
+        "tap_git_head" => formula.tap_git_head,
         "spec"         => formula.active_spec_sym.to_s,
         "versions"     => {
           "stable"         => formula.stable&.version&.to_s,

Library/Homebrew/test/cask/cask_spec.rb

@@ -217,16 +217,16 @@ RSpec.describe Cask::Cask, :cask do
 
     it "returns all artifacts when no options are given" do
       expected_artifacts = [
-        { "uninstall_preflight" => nil },
+        { uninstall_preflight: nil },
-        { "preflight" => nil },
+        { preflight: nil },
         { uninstall: [{
           rmdir: "#{TEST_TMPDIR}/empty_directory_path",
           trash: ["#{TEST_TMPDIR}/foo", "#{TEST_TMPDIR}/bar"],
         }] },
         { pkg: ["ManyArtifacts/ManyArtifacts.pkg"] },
         { app: ["ManyArtifacts/ManyArtifacts.app"] },
-        { "uninstall_postflight" => nil },
+        { uninstall_postflight: nil },
-        { "postflight" => nil },
+        { postflight: nil },
         { zap: [{
           rmdir: ["~/Library/Caches/ManyArtifacts", "~/Library/Application Support/ManyArtifacts"],
           trash: "~/Library/Logs/ManyArtifacts.log",
@@ -255,13 +255,13 @@ RSpec.describe Cask::Cask, :cask do
 
     it "returns only uninstall artifacts when uninstall_only is true" do
       expected_artifacts = [
-        { "uninstall_preflight" => nil },
+        { uninstall_preflight: nil },
         { uninstall: [{
           rmdir: "#{TEST_TMPDIR}/empty_directory_path",
           trash: ["#{TEST_TMPDIR}/foo", "#{TEST_TMPDIR}/bar"],
         }] },
         { app: ["ManyArtifacts/ManyArtifacts.app"] },
-        { "uninstall_postflight" => nil },
+        { uninstall_postflight: nil },
         { zap: [{
           rmdir: ["~/Library/Caches/ManyArtifacts", "~/Library/Application Support/ManyArtifacts"],
           trash: "~/Library/Logs/ManyArtifacts.log",
@@ -288,6 +288,39 @@ RSpec.describe Cask::Cask, :cask do
     end
   end
 
+  describe "#uninstall_flight_blocks?" do
+    matcher :have_uninstall_flight_blocks do
+      match do |actual|
+        actual.uninstall_flight_blocks? == true
+      end
+    end
+
+    it "returns true when there are uninstall_preflight blocks" do
+      cask = Cask::CaskLoader.load("with-uninstall-preflight")
+      expect(cask).to have_uninstall_flight_blocks
+    end
+
+    it "returns true when there are uninstall_postflight blocks" do
+      cask = Cask::CaskLoader.load("with-uninstall-postflight")
+      expect(cask).to have_uninstall_flight_blocks
+    end
+
+    it "returns false when there are only preflight blocks" do
+      cask = Cask::CaskLoader.load("with-preflight")
+      expect(cask).not_to have_uninstall_flight_blocks
+    end
+
+    it "returns false when there are only postflight blocks" do
+      cask = Cask::CaskLoader.load("with-postflight")
+      expect(cask).not_to have_uninstall_flight_blocks
+    end
+
+    it "returns false when there are no flight blocks" do
+      cask = Cask::CaskLoader.load("local-caffeine")
+      expect(cask).not_to have_uninstall_flight_blocks
+    end
+  end
+
   describe "#to_h" do
     let(:expected_json) { (TEST_FIXTURE_DIR/"cask/everything.json").read.strip }
 

Library/Homebrew/test/tab_spec.rb

@@ -397,25 +397,6 @@ RSpec.describe Tab do
     end
   end
 
-  describe "::tap_git_head" do
-    it "returns nil if the tap is nil" do
-      formula = instance_double(Formula, tap: nil)
-      expect(described_class.tap_git_head(formula)).to be_nil
-    end
-
-    it "returns nil if the tap is not installed" do
-      tap = instance_double(Tap, installed?: false)
-      formula = instance_double(Formula, tap:)
-      expect(described_class.tap_git_head(formula)).to be_nil
-    end
-
-    it "returns the tap git head if the tap is installed" do
-      tap = instance_double(Tap, installed?: true, git_head: "0453e16c8e3fac73104da50927a86221ca0740c2")
-      formula = instance_double(Formula, tap:)
-      expect(described_class.tap_git_head(formula)).to eq("0453e16c8e3fac73104da50927a86221ca0740c2")
-    end
-  end
-
   specify "#to_json" do
     json_tab = described_class.new(JSON.parse(tab.to_json))
     expect(json_tab.homebrew_version).to eq(tab.homebrew_version)