Merge pull request #10305 from SeekingMeaning/safe-popen-tests

utils/popen: add `safe` argument to `popen_read` and `popen_write`

Library/Homebrew/extend/git_repository.rb

@@ -32,20 +32,21 @@ module GitRepositoryExtension
   end
 
   # Gets the full commit hash of the HEAD commit.
-  sig { returns(T.nilable(String)) }
+  sig { params(safe: T::Boolean).returns(T.nilable(String)) }
-  def git_head
+  def git_head(safe: false)
     return if !git? || !Utils::Git.available?
 
-    Utils.popen_read(Utils::Git.git, "rev-parse", "--verify", "-q", "HEAD", chdir: self).chomp.presence
+    Utils.popen_read(Utils::Git.git, "rev-parse", "--verify", "-q", "HEAD", safe: safe, chdir: self).chomp.presence
   end
 
   # Gets a short commit hash of the HEAD commit.
-  sig { params(length: T.nilable(Integer)).returns(T.nilable(String)) }
+  sig { params(length: T.nilable(Integer), safe: T::Boolean).returns(T.nilable(String)) }
-  def git_short_head(length: nil)
+  def git_short_head(length: nil, safe: false)
     return if !git? || !Utils::Git.available?
 
+    git = Utils::Git.git
     short_arg = length&.to_s&.prepend("=")
-    Utils.popen_read(Utils::Git.git, "rev-parse", "--short#{short_arg}", "--verify", "-q", "HEAD", chdir: self)
+    Utils.popen_read(git, "rev-parse", "--short#{short_arg}", "--verify", "-q", "HEAD", safe: safe, chdir: self)
          .chomp.presence
   end
 

Library/Homebrew/test/utils/popen_spec.rb

@@ -72,4 +72,32 @@ describe Utils do
       EOS
     end
   end
+
+  describe "::safe_popen_read" do
+    it "does not raise an error if the command succeeds" do
+      expect(subject.safe_popen_read("sh", "-c", "true")).to eq("")
+      expect($CHILD_STATUS).to be_a_success
+    end
+
+    it "raises an error if the command fails" do
+      expect { subject.safe_popen_read("sh", "-c", "false") }.to raise_error(ErrorDuringExecution)
+      expect($CHILD_STATUS).to be_a_failure
+    end
+  end
+
+  describe "::safe_popen_write" do
+    it "does not raise an error if the command succeeds" do
+      expect(
+        subject.safe_popen_write("grep", "success") { |pipe| pipe.write "success\n" }.chomp,
+      ).to eq("success")
+      expect($CHILD_STATUS).to be_a_success
+    end
+
+    it "raises an error if the command fails" do
+      expect {
+        subject.safe_popen_write("grep", "success") { |pipe| pipe.write "failure\n" }
+      }.to raise_error(ErrorDuringExecution)
+      expect($CHILD_STATUS).to be_a_failure
+    end
+  end
 end

Library/Homebrew/utils/git_repository.rb

@@ -4,17 +4,21 @@
 module Utils
   extend T::Sig
 
-  sig { params(repo: T.any(String, Pathname), length: T.nilable(Integer)).returns(T.nilable(String)) }
+  sig do
-  def self.git_head(repo, length: nil)
+    params(repo: T.any(String, Pathname), length: T.nilable(Integer), safe: T::Boolean).returns(T.nilable(String))
+  end
+  def self.git_head(repo, length: nil, safe: true)
     return git_short_head(repo, length: length) if length.present?
 
     repo = Pathname(repo).extend(GitRepositoryExtension)
-    repo.git_head
+    repo.git_head(safe: safe)
   end
 
-  sig { params(repo: T.any(String, Pathname), length: T.nilable(Integer)).returns(T.nilable(String)) }
+  sig do
-  def self.git_short_head(repo, length: nil)
+    params(repo: T.any(String, Pathname), length: T.nilable(Integer), safe: T::Boolean).returns(T.nilable(String))
+  end
+  def self.git_short_head(repo, length: nil, safe: true)
     repo = Pathname(repo).extend(GitRepositoryExtension)
-    repo.git_short_head(length: length)
+    repo.git_short_head(length: length, safe: safe)
   end
 end

Library/Homebrew/utils/popen.rb

@@ -5,21 +5,20 @@ module Utils
   IO_DEFAULT_BUFFER_SIZE = 4096
   private_constant :IO_DEFAULT_BUFFER_SIZE
 
-  def self.popen_read(*args, **options, &block)
+  def self.popen_read(*args, safe: false, **options, &block)
-    popen(args, "rb", options, &block)
+    output = popen(args, "rb", options, &block)
-  end
+    return output if !safe || $CHILD_STATUS.success?
-
-  def self.safe_popen_read(*args, **options, &block)
-    output = popen_read(*args, **options, &block)
-    return output if $CHILD_STATUS.success?
 
     raise ErrorDuringExecution.new(args, status: $CHILD_STATUS, output: [[:stdout, output]])
   end
 
-  def self.popen_write(*args, **options)
+  def self.safe_popen_read(*args, **options, &block)
-    popen(args, "w+b", options) do |pipe|
+    popen_read(*args, safe: true, **options, &block)
-      output = ""
+  end
 
+  def self.popen_write(*args, safe: false, **options)
+    output = ""
+    popen(args, "w+b", options) do |pipe|
       # Before we yield to the block, capture as much output as we can
       loop do
         output += pipe.read_nonblock(IO_DEFAULT_BUFFER_SIZE)
@@ -35,13 +34,13 @@ module Utils
       output += pipe.read
       output.freeze
     end
+    return output if !safe || $CHILD_STATUS.success?
+
+    raise ErrorDuringExecution.new(args, status: $CHILD_STATUS, output: [[:stdout, output]])
   end
 
   def self.safe_popen_write(*args, **options, &block)
-    output = popen_write(*args, **options, &block)
+    popen_write(*args, safe: true, **options, &block)
-    return output if $CHILD_STATUS.success?
-
-    raise ErrorDuringExecution.new(args, status: $CHILD_STATUS, output: [[:stdout, output]])
   end
 
   def self.popen(args, mode, options = {})