From 772032f18a174d2cd5982800ebc0682eb569d0e1 Mon Sep 17 00:00:00 2001
From: Claudia <claui@users.noreply.github.com>
Date: Sun, 30 Aug 2020 22:29:46 +0200
Subject: [PATCH 1/3] Add failing tests for `popen_write`
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When using `popen_write`, the expectation is to return the
standard output of the child process.

This expectation is evident in how `safe_popen_write` is written:

```
  def self.safe_popen_write(*args, **options, &block)
    output = popen_write(*args, **options, &block)
    return output if $CHILD_STATUS.success?

    raise ErrorDuringExecution.new(args, status: $CHILD_STATUS, output: [[:stdout, output]])
  end
```

However, no code has been written to actually *obtain* that output
from the child process. The side effects of that are described in
issue #8244. [1]

[1]: https://github.com/Homebrew/brew/issues/8244

The newly-added tests reveal that `popen_write` only returns the
number 4 instead of the expected standard output.

For example, given a file `foo` with the content `Foo\n`, one test
calls `popen_write` with `cat foo -` and an input of `Bar`.
The expected output would be `Foo\nBar\n` but the actual output is
the number 4 (which is what Ruby’s `IO#write` method returns).
---
 Library/Homebrew/test/utils/popen_spec.rb | 42 +++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/Library/Homebrew/test/utils/popen_spec.rb b/Library/Homebrew/test/utils/popen_spec.rb
index c3f6027cf7..dcce91d8b4 100644
--- a/Library/Homebrew/test/utils/popen_spec.rb
+++ b/Library/Homebrew/test/utils/popen_spec.rb
@@ -26,11 +26,49 @@ describe Utils do
   end
 
   describe "::popen_write" do
-    it "with supports writing to a command's standard input" do
+    let(:foo) { mktmpdir/"foo" }
+
+    before { foo.write "Foo\n" }
+
+    it "supports writing to a command's standard input" do
       subject.popen_write("grep", "-q", "success") do |pipe|
-        pipe.write("success\n")
+        pipe.write "success\n"
       end
       expect($CHILD_STATUS).to be_a_success
     end
+
+    it "returns the command's standard output before writing" do
+      child_stdout = subject.popen_write("cat", foo, "-") do |pipe|
+        pipe.write "Bar\n"
+      end
+      expect($CHILD_STATUS).to be_a_success
+      expect(child_stdout).to eq <<~EOS
+        Foo
+        Bar
+      EOS
+    end
+
+    it "returns the command's standard output after writing" do
+      child_stdout = subject.popen_write("cat", "-", foo) do |pipe|
+        pipe.write "Bar\n"
+      end
+      expect($CHILD_STATUS).to be_a_success
+      expect(child_stdout).to eq <<~EOS
+        Bar
+        Foo
+      EOS
+    end
+
+    it "supports interleaved writing between two reads" do
+      child_stdout = subject.popen_write("cat", foo, "-", foo) do |pipe|
+        pipe.write "Bar\n"
+      end
+      expect($CHILD_STATUS).to be_a_success
+      expect(child_stdout).to eq <<~EOS
+        Foo
+        Bar
+        Foo
+      EOS
+    end
   end
 end

From 246db8a134f162b764c36cd0050c8303a2197615 Mon Sep 17 00:00:00 2001
From: Claudia <claui@users.noreply.github.com>
Date: Sun, 30 Aug 2020 22:36:28 +0200
Subject: [PATCH 2/3] Capture stdout during `popen_write`

Fix tests and fulfill intended semantics by having `popen_write`
transparently capture standard output.
---
 Library/Homebrew/utils/popen.rb | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/Library/Homebrew/utils/popen.rb b/Library/Homebrew/utils/popen.rb
index f9d189219c..18cd057fe8 100644
--- a/Library/Homebrew/utils/popen.rb
+++ b/Library/Homebrew/utils/popen.rb
@@ -12,8 +12,25 @@ module Utils
     raise ErrorDuringExecution.new(args, status: $CHILD_STATUS, output: [[:stdout, output]])
   end
 
-  def self.popen_write(*args, **options, &block)
-    popen(args, "wb", options, &block)
+  def self.popen_write(*args, **options)
+    popen(args, "w+b", options) do |pipe|
+      output = ""
+
+      # Before we yield to the block, capture as much output as we can
+      loop do
+        output += pipe.read_nonblock(4096)
+      rescue IO::WaitReadable, EOFError
+        break
+      end
+
+      yield pipe
+      pipe.close_write
+      IO.select([pipe])
+
+      # Capture the rest of the output
+      output += pipe.read
+      output.freeze
+    end
   end
 
   def self.safe_popen_write(*args, **options, &block)

From 09d7889ed8f6eee000c523a7ada42b850ec64e04 Mon Sep 17 00:00:00 2001
From: Claudia <claui@users.noreply.github.com>
Date: Wed, 2 Sep 2020 18:14:56 +0200
Subject: [PATCH 3/3] Extract constant IO_DEFAULT_BUFFER_SIZE

---
 Library/Homebrew/utils/popen.rb | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Library/Homebrew/utils/popen.rb b/Library/Homebrew/utils/popen.rb
index 18cd057fe8..eac4823c64 100644
--- a/Library/Homebrew/utils/popen.rb
+++ b/Library/Homebrew/utils/popen.rb
@@ -1,6 +1,9 @@
 # frozen_string_literal: true
 
 module Utils
+  IO_DEFAULT_BUFFER_SIZE = 4096
+  private_constant :IO_DEFAULT_BUFFER_SIZE
+
   def self.popen_read(*args, **options, &block)
     popen(args, "rb", options, &block)
   end
@@ -18,7 +21,7 @@ module Utils
 
       # Before we yield to the block, capture as much output as we can
       loop do
-        output += pipe.read_nonblock(4096)
+        output += pipe.read_nonblock(IO_DEFAULT_BUFFER_SIZE)
       rescue IO::WaitReadable, EOFError
         break
       end