From 40d85ecb4089cdc8c1163f1afa18279599804962 Mon Sep 17 00:00:00 2001 From: Seeker Date: Tue, 12 Jan 2021 09:49:45 -0800 Subject: [PATCH 1/3] popen_spec: add tests for `safe_popen_read` and `safe_popen_write` --- Library/Homebrew/test/utils/popen_spec.rb | 28 +++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/Library/Homebrew/test/utils/popen_spec.rb b/Library/Homebrew/test/utils/popen_spec.rb index 7ea5474e7a..990d30360b 100644 --- a/Library/Homebrew/test/utils/popen_spec.rb +++ b/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 From 88306d5f5de35306de9dbd6a5ff571f57145f9f4 Mon Sep 17 00:00:00 2001 From: Seeker Date: Tue, 12 Jan 2021 10:22:40 -0800 Subject: [PATCH 2/3] utils/popen: add `safe` argument to `popen_read` and `popen_write` --- Library/Homebrew/utils/popen.rb | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/Library/Homebrew/utils/popen.rb b/Library/Homebrew/utils/popen.rb index b721b3191c..c5546c6d93 100644 --- a/Library/Homebrew/utils/popen.rb +++ b/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) - popen(args, "rb", options, &block) - end - - def self.safe_popen_read(*args, **options, &block) - output = popen_read(*args, **options, &block) - return output if $CHILD_STATUS.success? + def self.popen_read(*args, safe: false, **options, &block) + output = popen(args, "rb", options, &block) + return output if !safe || $CHILD_STATUS.success? raise ErrorDuringExecution.new(args, status: $CHILD_STATUS, output: [[:stdout, output]]) end - def self.popen_write(*args, **options) - popen(args, "w+b", options) do |pipe| - output = "" + def self.safe_popen_read(*args, **options, &block) + popen_read(*args, safe: true, **options, &block) + 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) - return output if $CHILD_STATUS.success? - - raise ErrorDuringExecution.new(args, status: $CHILD_STATUS, output: [[:stdout, output]]) + popen_write(*args, safe: true, **options, &block) end def self.popen(args, mode, options = {}) From eefe5bb2959c77aa93a6b4a1884644fedbd8b730 Mon Sep 17 00:00:00 2001 From: Seeker Date: Tue, 12 Jan 2021 10:29:27 -0800 Subject: [PATCH 3/3] git_repository: add `safe` argument to `git_head`/`git_short_head` --- Library/Homebrew/extend/git_repository.rb | 13 +++++++------ Library/Homebrew/utils/git_repository.rb | 16 ++++++++++------ 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/Library/Homebrew/extend/git_repository.rb b/Library/Homebrew/extend/git_repository.rb index 2c366db0bf..2b281f37f0 100644 --- a/Library/Homebrew/extend/git_repository.rb +++ b/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)) } - def git_head + sig { params(safe: T::Boolean).returns(T.nilable(String)) } + 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)) } - def git_short_head(length: nil) + sig { params(length: T.nilable(Integer), safe: T::Boolean).returns(T.nilable(String)) } + 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 diff --git a/Library/Homebrew/utils/git_repository.rb b/Library/Homebrew/utils/git_repository.rb index 70602c3f7c..ce3142fb8c 100644 --- a/Library/Homebrew/utils/git_repository.rb +++ b/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)) } - def self.git_head(repo, length: nil) + sig do + 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)) } - def self.git_short_head(repo, length: nil) + sig do + 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