Use SystemCommand for curl.

Library/Homebrew/cask/lib/hbc/download_strategy.rb

@@ -106,10 +106,10 @@ module Hbc
           LockFile.new(temporary_path.basename).with_lock do
             _fetch
           end
-        rescue ErrorDuringExecution
+        rescue ErrorDuringExecution => e
           # 33 == range not supported
           # try wiping the incomplete download and retrying once
-          if $CHILD_STATUS.exitstatus == 33 && had_incomplete_download
+          if e.status.exitstatus == 33 && had_incomplete_download
             ohai "Trying a full download"
             temporary_path.unlink
             had_incomplete_download = false

Library/Homebrew/exceptions.rb

@@ -526,7 +526,11 @@ end
 
 # raised by safe_system in utils.rb
 class ErrorDuringExecution < RuntimeError
+  attr_reader :status
+
   def initialize(cmd, status:, output: nil)
+    @status = status
+
     s = "Failure while executing; `#{cmd.shelljoin.gsub(/\\=/, "=")}` exited with #{status.exitstatus}."
 
     unless [*output].empty?

Library/Homebrew/system_command.rb

@@ -37,7 +37,13 @@ class SystemCommand
         puts line.chomp if print_stdout?
         @merged_output << [:stdout, line]
       when :stderr
-        $stderr.puts Formatter.error(line.chomp) if print_stderr?
+        if print_stderr?
+          if line.start_with?("\r")
+            $stderr.print line
+          else
+            $stderr.puts Formatter.error(line.chomp)
+          end
+        end
         @merged_output << [:stderr, line]
       end
     end
@@ -76,13 +82,18 @@ class SystemCommand
   def env_args
     return [] if env.empty?
 
-    variables = env.map do |name, value|
+    unset_variables = env.select { |_, value| value.nil? }
+                         .flat_map { |name,| ["-u", name] }
+
+    set_variables =
+      env.reject { |_, value| value.nil? }
+         .flat_map do |name, value|
            sanitized_name = Shellwords.escape(name)
            sanitized_value = Shellwords.escape(value)
            "#{sanitized_name}=#{sanitized_value}"
          end
 
-    ["env", *variables]
+    ["env", *unset_variables, *set_variables]
   end
 
   def sudo_prefix
@@ -102,7 +113,7 @@ class SystemCommand
     @expanded_args ||= args.map do |arg|
       if arg.respond_to?(:to_path)
         File.absolute_path(arg)
-      elsif arg.is_a?(Integer) || arg.is_a?(Float)
+      elsif arg.is_a?(Integer) || arg.is_a?(Float) || arg.is_a?(URI)
         arg.to_s
       else
         arg.to_str
@@ -157,23 +168,28 @@ class SystemCommand
       hash[type] << line
     end
 
-    Result.new(command, output[:stdout], output[:stderr], @status.exitstatus)
+    Result.new(command, output[:stdout], output[:stderr], @status)
   end
 
   class Result
-    attr_accessor :command, :stdout, :stderr, :exit_status
+    attr_accessor :command, :stdout, :stderr, :status, :exit_status
 
-    def initialize(command, stdout, stderr, exit_status)
+    def initialize(command, stdout, stderr, status)
       @command     = command
       @stdout      = stdout
       @stderr      = stderr
-      @exit_status = exit_status
+      @status      = status
+      @exit_status = status.exitstatus
     end
 
     def success?
       @exit_status.zero?
     end
 
+    def to_ary
+      [stdout, stderr, status]
+    end
+
     def plist
       @plist ||= begin
         output = stdout

Library/Homebrew/test/cask/download_strategy_spec.rb

@@ -39,7 +39,7 @@ describe "download strategies", :cask do
       let(:url_options) { { user_agent: "Mozilla/25.0.1" } }
 
       it "adds the appropriate curl args" do
-        expect(downloader).to receive(:safe_system) { |*args|
+        expect(downloader).to receive(:system_command!) { |*, args:, **|
           expect(args.each_cons(2)).to include(["--user-agent", "Mozilla/25.0.1"])
         }
 
@@ -53,7 +53,7 @@ describe "download strategies", :cask do
       let(:url_options) { { user_agent: :fake } }
 
       it "adds the appropriate curl args" do
-        expect(downloader).to receive(:safe_system) { |*args|
+        expect(downloader).to receive(:system_command!) { |*, args:, **|
           expect(args.each_cons(2).to_a).to include(["--user-agent", a_string_matching(/Mozilla.*Mac OS X 10.*AppleWebKit/)])
         }
 

Library/Homebrew/test/cask/pkg_spec.rb

@@ -148,7 +148,7 @@ describe Hbc::Pkg, :cask do
         "/usr/sbin/pkgutil",
         args: ["--pkg-info-plist", pkg_id],
       ).and_return(
-        SystemCommand::Result.new(nil, pkg_info_plist, nil, 0),
+        SystemCommand::Result.new(nil, pkg_info_plist, nil, instance_double(Process::Status, exitstatus: 0)),
       )
 
       info = pkg.info

Library/Homebrew/test/support/helper/cask/fake_system_command.rb

@@ -52,7 +52,7 @@ class FakeSystemCommand
     if response.respond_to?(:call)
       response.call(command_string, options)
     else
-      SystemCommand::Result.new(command, response, "", 0)
+      SystemCommand::Result.new(command, response, "", OpenStruct.new(exitstatus: 0))
     end
   end
 

Library/Homebrew/test/system_command_result_spec.rb

@@ -1,8 +1,22 @@
 require "system_command"
 
 describe SystemCommand::Result do
+  describe "#to_ary" do
+    subject(:result) {
+      described_class.new([], "output", "error", instance_double(Process::Status, exitstatus: 0, success?: true))
+    }
+
+    it "can be destructed like `Open3.capture3`" do
+      out, err, status = result
+
+      expect(out).to eq "output"
+      expect(err).to eq "error"
+      expect(status).to be_a_success
+    end
+  end
+
   describe "#plist" do
-    subject { described_class.new(command, stdout, "", 0).plist }
+    subject { described_class.new(command, stdout, "", instance_double(Process::Status, exitstatus: 0)).plist }
 
     let(:command) { ["true"] }
     let(:garbage) {

Library/Homebrew/test/system_command_spec.rb

@@ -1,17 +1,20 @@
 describe SystemCommand do
   describe "#initialize" do
     let(:env_args) { ["bash", "-c", 'printf "%s" "${A?}" "${B?}" "${C?}"'] }
+    let(:env) { { "A" => "1", "B" => "2", "C" => "3" } }
+    let(:sudo) { false }
 
-    context "when given some environment variables" do
+    subject(:command) {
-      subject {
       described_class.new(
         "env",
         args: env_args,
-          env: { "A" => "1", "B" => "2", "C" => "3" },
+        env: env,
         must_succeed: true,
+        sudo: sudo,
       )
     }
 
+    context "when given some environment variables" do
       its("run!.stdout") { is_expected.to eq("123") }
 
       describe "the resulting command line" do
@@ -21,21 +24,23 @@ describe SystemCommand do
             .with(["env", "env"], "A=1", "B=2", "C=3", "env", *env_args, {})
             .and_call_original
 
-          subject.run!
+          command.run!
         end
       end
     end
 
+    context "when given an environment variable which is set to nil" do
+      let(:env) { { "A" => "1", "B" => "2", "C" => nil } }
+
+      it "unsets them" do
+        expect {
+          command.run!
+        }.to raise_error(/C: parameter null or not set/)
+      end
+    end
+
     context "when given some environment variables and sudo: true" do
-      subject {
+      let(:sudo) { true }
-        described_class.new(
-          "env",
-          args: env_args,
-          env: { "A" => "1", "B" => "2", "C" => "3" },
-          must_succeed: true,
-          sudo: true,
-        )
-      }
 
       describe "the resulting command line" do
         it "includes the given variables explicitly" do
@@ -47,7 +52,7 @@ describe SystemCommand do
               original_popen3.call("true", &block)
             end
 
-          subject.run!
+          command.run!
         end
       end
     end
@@ -215,5 +220,12 @@ describe SystemCommand do
         described_class.run("non_existent_executable")
       }.not_to raise_error
     end
+
+    it 'does not format `stderr` when it starts with \r' do
+      expect {
+        system_command "bash",
+                       args: ["-c", 'printf "\r%s" "###################                                                       27.6%" 1>&2']
+      }.to output("\r###################                                                       27.6%").to_stderr
+    end
   end
 end

Library/Homebrew/test/utils/curl_spec.rb

@@ -4,12 +4,12 @@ describe "curl" do
   describe "curl_args" do
     it "returns -q as the first argument when HOMEBREW_CURLRC is not set" do
       # -q must be the first argument according to "man curl"
-      expect(curl_args("foo")[1]).to eq("-q")
+      expect(curl_args("foo").first).to eq("-q")
     end
 
     it "doesn't return -q as the first argument when HOMEBREW_CURLRC is set" do
       ENV["HOMEBREW_CURLRC"] = "1"
-      expect(curl_args("foo")[1]).to_not eq("-q")
+      expect(curl_args("foo").first).to_not eq("-q")
     end
   end
 end

Library/Homebrew/utils/curl.rb

@@ -11,7 +11,7 @@ def curl_executable
 end
 
 def curl_args(*extra_args, show_output: false, user_agent: :default)
-  args = [curl_executable.to_s]
+  args = []
 
   # do not load .curlrc unless requested (must be the first argument)
   args << "-q" unless ENV["HOMEBREW_CURLRC"]
@@ -40,18 +40,18 @@ end
 def curl(*args)
   # SSL_CERT_FILE can be incorrectly set by users or portable-ruby and screw
   # with SSL downloads so unset it here.
-  with_env SSL_CERT_FILE: nil do
+  system_command! curl_executable,
-    safe_system(*curl_args(*args))
+                  args: curl_args(*args),
-  end
+                  env: { "SSL_CERT_FILE" => nil }
 end
 
 def curl_download(*args, to: nil, continue_at: "-", **options)
   had_incomplete_download ||= File.exist?(to)
   curl("--location", "--remote-time", "--continue-at", continue_at.to_s, "--output", to, *args, **options)
-rescue ErrorDuringExecution
+rescue ErrorDuringExecution => e
   # `curl` error 33: HTTP server doesn't seem to support byte ranges. Cannot resume.
   # HTTP status 416: Requested range not satisfiable
-  if ($CHILD_STATUS.exitstatus == 33 || had_incomplete_download) && continue_at == "-"
+  if (e.status.exitstatus == 33 || had_incomplete_download) && continue_at == "-"
     continue_at = 0
     had_incomplete_download = false
     retry
@@ -61,7 +61,9 @@ rescue ErrorDuringExecution
 end
 
 def curl_output(*args, **options)
-  Open3.capture3(*curl_args(*args, show_output: true, **options))
+  system_command(curl_executable,
+                 args: curl_args(*args, show_output: true, **options),
+                 print_stderr: false)
 end
 
 def curl_check_http_content(url, user_agents: [:default], check_content: false, strict: false, require_http: false)

Library/Homebrew/utils/github.rb

@@ -131,13 +131,12 @@ module GitHub
     # This is a no-op if the user is opting out of using the GitHub API.
     return block_given? ? yield({}) : {} if ENV["HOMEBREW_NO_GITHUB_API"]
 
-    args = %W[--header application/vnd.github.v3+json --write-out \n%{http_code}] # rubocop:disable Lint/NestedPercentLiteral
+    args = ["--header", "application/vnd.github.v3+json", "--write-out", "\n%{http_code}"]
-    args += curl_args
 
     token, username = api_credentials
     case api_credentials_type
     when :keychain
-      args += %W[--user #{username}:#{token}]
+      args += ["--user", "#{username}:#{token}"]
     when :environment
       args += ["--header", "Authorization: token #{token}"]
     end
@@ -162,7 +161,7 @@ module GitHub
 
       args += ["--dump-header", headers_tmpfile.path]
 
-      output, errors, status = curl_output(url.to_s, "--location", *args)
+      output, errors, status = curl_output("--location", url.to_s, *args)
       output, _, http_code = output.rpartition("\n")
       output, _, http_code = output.rpartition("\n") if http_code == "000"
       headers = headers_tmpfile.read