Merge pull request #17793 from Homebrew/fix-frozen-array-modification-errors

Avoid frozen array errors in `brew upgrade`

Library/Homebrew/attestation.rb

@@ -93,6 +93,24 @@ module Homebrew
       T.must(@gh_executable)
     end
 
+    # Prioritize installing `gh` first if it's in the formula list
+    # or check for the existence of the `gh` executable elsewhere.
+    #
+    # This ensures that a valid version of `gh` is installed before
+    # we use it to check the attestations of any other formulae we
+    # want to install.
+    #
+    # @api private
+    sig { params(formulae: T::Array[Formula]).returns(T::Array[Formula]) }
+    def self.sort_formulae_for_install(formulae)
+      if formulae.include?(Formula["gh"])
+        [Formula["gh"]] | formulae
+      else
+        Homebrew::Attestation.gh_executable
+        formulae
+      end
+    end
+
     # Verifies the given bottle against a cryptographic attestation of build provenance.
     #
     # The provenance is verified as originating from `signing_repository`, which is a `String`

Library/Homebrew/cmd/install.rb

@@ -263,13 +263,7 @@ module Homebrew
           end
         end
 
-        if Homebrew::Attestation.enabled?
+        formulae = Homebrew::Attestation.sort_formulae_for_install(formulae) if Homebrew::Attestation.enabled?
-          if formulae.include?(Formula["gh"])
-            formulae.unshift(T.must(formulae.delete(Formula["gh"])))
-          else
-            Homebrew::Attestation.gh_executable
-          end
-        end
 
         # if the user's flags will prevent bottle only-installations when no
         # developer tools are available, we need to stop them early on

Library/Homebrew/cmd/reinstall.rb

@@ -109,7 +109,7 @@ module Homebrew
       sig { override.void }
       def run
         formulae, casks = T.cast(
-          args.named.to_formulae_and_casks(method: :resolve).partition { _1.is_a?(Formula) },
+          args.named.to_resolved_formulae_to_casks,
           [T::Array[Formula], T::Array[Cask::Cask]],
         )
 
@@ -124,13 +124,7 @@ module Homebrew
           end
         end
 
-        if Homebrew::Attestation.enabled?
+        formulae = Homebrew::Attestation.sort_formulae_for_install(formulae) if Homebrew::Attestation.enabled?
-          if formulae.include?(Formula["gh"])
-            formulae.unshift(T.must(formulae.delete(Formula["gh"])))
-          else
-            Homebrew::Attestation.gh_executable
-          end
-        end
 
         Install.perform_preinstall_checks
 

Library/Homebrew/cmd/upgrade.rb

@@ -134,13 +134,7 @@ module Homebrew
         only_upgrade_formulae = formulae.present? && casks.blank?
         only_upgrade_casks = casks.present? && formulae.blank?
 
-        if Homebrew::Attestation.enabled?
+        formulae = Homebrew::Attestation.sort_formulae_for_install(formulae) if Homebrew::Attestation.enabled?
-          if formulae.include?(Formula["gh"])
-            formulae.unshift(formulae.delete(Formula["gh"]))
-          else
-            Homebrew::Attestation.gh_executable
-          end
-        end
 
         upgrade_outdated_formulae(formulae) unless only_upgrade_casks
         upgrade_outdated_casks(casks) unless only_upgrade_formulae

Library/Homebrew/test/attestation_spec.rb

@@ -92,6 +92,46 @@ RSpec.describe Homebrew::Attestation do
     end
   end
 
+  # NOTE: `Homebrew::CLI::NamedArgs` will often return frozen arrays of formulae
+  #       so that's why we test with frozen arrays here.
+  describe "::sort_formulae_for_install", :integration_test do
+    let(:gh) { Formula["gh"] }
+    let(:other) { Formula["other"] }
+
+    before do
+      setup_test_formula("gh")
+      setup_test_formula("other")
+    end
+
+    context "when `gh` is in the formula list" do
+      it "moves `gh` formulae to the front of the list" do
+        expect(described_class).not_to receive(:gh_executable)
+
+        [
+          [[gh], [gh]],
+          [[gh, other], [gh, other]],
+          [[other, gh], [gh, other]],
+        ].each do |input, output|
+          expect(described_class.sort_formulae_for_install(input.freeze)).to eq(output)
+        end
+      end
+    end
+
+    context "when the formula list is empty" do
+      it "checks for the `gh` executable" do
+        expect(described_class).to receive(:gh_executable).once
+        expect(described_class.sort_formulae_for_install([].freeze)).to eq([])
+      end
+    end
+
+    context "when `gh` is not in the formula list" do
+      it "checks for the `gh` executable" do
+        expect(described_class).to receive(:gh_executable).once
+        expect(described_class.sort_formulae_for_install([other].freeze)).to eq([other])
+      end
+    end
+  end
+
   describe "::check_attestation" do
     before do
       allow(described_class).to receive(:gh_executable)