Curl#curl_args: Fix and refactor options

Library/Homebrew/livecheck/strategy.rb

@@ -67,7 +67,7 @@ module Homebrew
         debug:        false,
         verbose:      false,
         timeout:      CURL_PROCESS_TIMEOUT,
-        retry:        false,
+        retries:      0,
       }.freeze
 
       # HTTP response head(s) and body are typically separated by a double

Library/Homebrew/test/utils/curl_spec.rb

@@ -5,10 +5,10 @@ require "utils/curl"
 
 describe "Utils::Curl" do
   describe "curl_args" do
-    let(:args) { "foo" }
+    let(:args) { ["foo"] }
     let(:user_agent_string) { "Lorem ipsum dolor sit amet" }
 
-    it "returns --disable as the first argument when HOMEBREW_CURLRC is not set" do
+    it "returns `--disable` as the first argument when HOMEBREW_CURLRC is not set" do
       # --disable must be the first argument according to "man curl"
       expect(curl_args(*args).first).to eq("--disable")
     end
@@ -18,6 +18,26 @@ describe "Utils::Curl" do
       expect(curl_args(*args).first).not_to eq("--disable")
     end
 
+    it "uses `--connect-timeout` when `:connect_timeout` is Numeric" do
+      expect(curl_args(*args, connect_timeout: 123).join(" ")).to include("--connect-timeout 123")
+      expect(curl_args(*args, connect_timeout: 123.4).join(" ")).to include("--connect-timeout 123.4")
+      expect(curl_args(*args, connect_timeout: 123.4567).join(" ")).to include("--connect-timeout 123.457")
+    end
+
+    it "errors when `:connect_timeout` is not Numeric" do
+      expect { curl_args(*args, connect_timeout: "test") }.to raise_error(TypeError)
+    end
+
+    it "uses `--max-time` when `:max_time` is Numeric" do
+      expect(curl_args(*args, max_time: 123).join(" ")).to include("--max-time 123")
+      expect(curl_args(*args, max_time: 123.4).join(" ")).to include("--max-time 123.4")
+      expect(curl_args(*args, max_time: 123.4567).join(" ")).to include("--max-time 123.457")
+    end
+
+    it "errors when `:max_time` is not Numeric" do
+      expect { curl_args(*args, max_time: "test") }.to raise_error(TypeError)
+    end
+
     it "uses `--retry 3` when HOMEBREW_CURL_RETRIES is unset" do
       expect(curl_args(*args).join(" ")).to include("--retry 3")
     end
@@ -27,12 +47,27 @@ describe "Utils::Curl" do
       expect(curl_args(*args).join(" ")).to include("--retry 10")
     end
 
-    it "doesn't use `--retry` when `:retry` == `false`" do
+    it "uses `--retry` when `:retries` is a positive Integer" do
-      expect(curl_args(*args, retry: false).join(" ")).not_to include("--retry")
+      expect(curl_args(*args, retries: 5).join(" ")).to include("--retry 5")
     end
 
-    it "uses `--retry 3` when `:retry` == `true`" do
+    it "doesn't use `--retry` when `:retries` is nil or a non-positive Integer" do
-      expect(curl_args(*args, retry: true).join(" ")).to include("--retry 3")
+      expect(curl_args(*args, retries: nil).join(" ")).not_to include("--retry")
+      expect(curl_args(*args, retries: 0).join(" ")).not_to include("--retry")
+      expect(curl_args(*args, retries: -1).join(" ")).not_to include("--retry")
+    end
+
+    it "errors when `:retries` is not Numeric" do
+      expect { curl_args(*args, retries: "test") }.to raise_error(TypeError)
+    end
+
+    it "uses `--retry-max-time` when `:retry_max_time` is Numeric" do
+      expect(curl_args(*args, retry_max_time: 123).join(" ")).to include("--retry-max-time 123")
+      expect(curl_args(*args, retry_max_time: 123.4).join(" ")).to include("--retry-max-time 123")
+    end
+
+    it "errors when `:retry_max_time` is not Numeric" do
+      expect { curl_args(*args, retry_max_time: "test") }.to raise_error(TypeError)
     end
 
     it "uses HOMEBREW_USER_AGENT_FAKE_SAFARI when `:user_agent` is `:browser` or `:fake`" do
@@ -53,6 +88,12 @@ describe "Utils::Curl" do
         .to include("--user-agent #{user_agent_string}")
     end
 
+    it "errors when `:user_agent` is not a String or supported Symbol" do
+      expect { curl_args(*args, user_agent: :an_unsupported_symbol) }
+        .to raise_error(TypeError, ":user_agent must be :browser/:fake, :default, or a String")
+      expect { curl_args(*args, user_agent: 123) }.to raise_error(TypeError)
+    end
+
     it "uses `--fail` unless `:show_output` is `true`" do
       expect(curl_args(*args, show_output: false).join(" ")).to include("--fail")
       expect(curl_args(*args, show_output: nil).join(" ")).to include("--fail")

Library/Homebrew/utils/curl.rb

@@ -10,6 +10,8 @@ module Utils
   #
   # @api private
   module Curl
+    extend T::Sig
+
     using TimeRemaining
 
     module_function
@@ -27,7 +29,26 @@ module Utils
       @curl
     end
 
-    def curl_args(*extra_args, **options)
+    sig {
+      params(
+        extra_args:      T.untyped,
+        connect_timeout: T.any(Integer, Float, NilClass),
+        max_time:        T.any(Integer, Float, NilClass),
+        retries:         T.nilable(Integer),
+        retry_max_time:  T.any(Integer, Float, NilClass),
+        show_output:     T.nilable(T::Boolean),
+        user_agent:      T.any(String, Symbol, NilClass),
+      ).returns(T::Array[T.untyped])
+    }
+    def curl_args(
+      *extra_args,
+      connect_timeout: nil,
+      max_time: nil,
+      retries: Homebrew::EnvConfig.curl_retries.to_i,
+      retry_max_time: nil,
+      show_output: false,
+      user_agent: nil
+    )
       args = []
 
       # do not load .curlrc unless requested (must be the first argument)
@@ -40,28 +61,33 @@ module Utils
 
       args << "--show-error"
 
-      args << "--user-agent" << case options[:user_agent]
+      args << "--user-agent" << case user_agent
       when :browser, :fake
         HOMEBREW_USER_AGENT_FAKE_SAFARI
       when :default, nil
         HOMEBREW_USER_AGENT_CURL
       when String
-        options[:user_agent]
+        user_agent
+      else
+        raise TypeError, ":user_agent must be :browser/:fake, :default, or a String"
       end
 
       args << "--header" << "Accept-Language: en"
 
-      unless options[:show_output] == true
+      unless show_output == true
         args << "--fail"
         args << "--progress-bar" unless Context.current.verbose?
         args << "--verbose" if Homebrew::EnvConfig.curl_verbose?
         args << "--silent" unless $stdout.tty?
       end
 
-      args << "--connect-timeout" << connect_timeout.round(3) if options[:connect_timeout]
+      args << "--connect-timeout" << connect_timeout.round(3) if connect_timeout.present?
-      args << "--max-time" << max_time.round(3) if options[:max_time]
+      args << "--max-time" << max_time.round(3) if max_time.present?
-      args << "--retry" << Homebrew::EnvConfig.curl_retries unless options[:retry] == false
+
-      args << "--retry-max-time" << retry_max_time.round if options[:retry_max_time]
+      # A non-positive integer (e.g., 0) or `nil` will omit this argument
+      args << "--retry" << retries if retries&.positive?
+
+      args << "--retry-max-time" << retry_max_time.round if retry_max_time.present?
 
       args + extra_args
     end