From 9bf409111c29af56667cd86a390f46e0b5561c7d Mon Sep 17 00:00:00 2001 From: Seeker Date: Sun, 17 Jan 2021 08:51:20 -0800 Subject: [PATCH] git_repository: raise error instead of returning nil if `safe` --- Library/Homebrew/extend/git_repository.rb | 31 +++++++--- .../test/utils/git_repository_spec.rb | 62 ++++++++++++++++++- Library/Homebrew/utils/git_repository.rb | 12 +++- 3 files changed, 92 insertions(+), 13 deletions(-) diff --git a/Library/Homebrew/extend/git_repository.rb b/Library/Homebrew/extend/git_repository.rb index 2b281f37f0a02..8e11ff5a09325 100644 --- a/Library/Homebrew/extend/git_repository.rb +++ b/Library/Homebrew/extend/git_repository.rb @@ -34,20 +34,14 @@ def git_origin=(origin) # Gets the full commit hash of the HEAD commit. 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", safe: safe, chdir: self).chomp.presence + popen_git("rev-parse", "--verify", "-q", "HEAD", safe: safe) end # Gets a short commit hash of the HEAD commit. 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(git, "rev-parse", "--short#{short_arg}", "--verify", "-q", "HEAD", safe: safe, chdir: self) - .chomp.presence + short_arg = length.present? ? "--short=#{length}" : "--short" + popen_git("rev-parse", short_arg, "--verify", "-q", "HEAD", safe: safe) end # Gets the relative date of the last commit, e.g. "1 hour ago" @@ -96,4 +90,23 @@ def git_commit_message(commit = "HEAD") Utils.popen_read(Utils::Git.git, "log", "-1", "--pretty=%B", commit, "--", chdir: self, err: :out).strip.presence end + + private + + sig { params(args: T.untyped, safe: T::Boolean).returns(T.nilable(String)) } + def popen_git(*args, safe: false) + unless git? + return unless safe + + raise "Not a Git repository: #{self}" + end + + unless Utils::Git.available? + return unless safe + + raise "Git is unavailable" + end + + T.unsafe(Utils).popen_read(Utils::Git.git, *args, safe: safe, chdir: self).chomp.presence + end end diff --git a/Library/Homebrew/test/utils/git_repository_spec.rb b/Library/Homebrew/test/utils/git_repository_spec.rb index 3d54ce9b85602..513334d03a2f0 100644 --- a/Library/Homebrew/test/utils/git_repository_spec.rb +++ b/Library/Homebrew/test/utils/git_repository_spec.rb @@ -17,24 +17,82 @@ let(:short_head_revision) { HOMEBREW_CACHE.cd { `git rev-parse --short HEAD`.chomp } } describe ".git_head" do - it "returns the revision at HEAD" do + it "returns the revision at HEAD if repo parameter is specified" do expect(described_class.git_head(HOMEBREW_CACHE)).to eq(head_revision) expect(described_class.git_head(HOMEBREW_CACHE, length: 5)).to eq(head_revision[0...5]) + end + + it "returns the revision at HEAD if repo parameter is omitted" do HOMEBREW_CACHE.cd do expect(described_class.git_head).to eq(head_revision) expect(described_class.git_head(length: 5)).to eq(head_revision[0...5]) end end + + context "when directory is not a Git repository" do + it "returns nil if `safe` parameter is `false`" do + expect(described_class.git_head(TEST_TMPDIR, safe: false)).to eq(nil) + end + + it "raises an error if `safe` parameter is `true`" do + expect { described_class.git_head(TEST_TMPDIR, safe: true) } + .to raise_error("Not a Git repository: #{TEST_TMPDIR}") + end + end + + context "when Git is unavailable" do + before do + allow(Utils::Git).to receive(:available?).and_return(false) + end + + it "returns nil if `safe` parameter is `false`" do + expect(described_class.git_head(HOMEBREW_CACHE, safe: false)).to eq(nil) + end + + it "raises an error if `safe` parameter is `true`" do + expect { described_class.git_head(HOMEBREW_CACHE, safe: true) } + .to raise_error("Git is unavailable") + end + end end describe ".git_short_head" do - it "returns the short revision at HEAD" do + it "returns the short revision at HEAD if repo parameter is specified" do expect(described_class.git_short_head(HOMEBREW_CACHE)).to eq(short_head_revision) expect(described_class.git_short_head(HOMEBREW_CACHE, length: 5)).to eq(head_revision[0...5]) + end + + it "returns the short revision at HEAD if repo parameter is omitted" do HOMEBREW_CACHE.cd do expect(described_class.git_short_head).to eq(short_head_revision) expect(described_class.git_short_head(length: 5)).to eq(head_revision[0...5]) end end + + context "when directory is not a Git repository" do + it "returns nil if `safe` parameter is `false`" do + expect(described_class.git_short_head(TEST_TMPDIR, safe: false)).to eq(nil) + end + + it "raises an error if `safe` parameter is `true`" do + expect { described_class.git_short_head(TEST_TMPDIR, safe: true) } + .to raise_error("Not a Git repository: #{TEST_TMPDIR}") + end + end + + context "when Git is unavailable" do + before do + allow(Utils::Git).to receive(:available?).and_return(false) + end + + it "returns nil if `safe` parameter is `false`" do + expect(described_class.git_short_head(HOMEBREW_CACHE, safe: false)).to eq(nil) + end + + it "raises an error if `safe` parameter is `true`" do + expect { described_class.git_short_head(HOMEBREW_CACHE, safe: true) } + .to raise_error("Git is unavailable") + end + end end end diff --git a/Library/Homebrew/utils/git_repository.rb b/Library/Homebrew/utils/git_repository.rb index b22c7dfae8022..460deed0d1ea4 100644 --- a/Library/Homebrew/utils/git_repository.rb +++ b/Library/Homebrew/utils/git_repository.rb @@ -5,7 +5,11 @@ module Utils extend T::Sig sig do - params(repo: T.any(String, Pathname), length: T.nilable(Integer), safe: T::Boolean).returns(T.nilable(String)) + params( + repo: T.any(String, Pathname), + length: T.nilable(Integer), + safe: T::Boolean, + ).returns(T.nilable(String)) end def self.git_head(repo = Pathname.pwd, length: nil, safe: true) return git_short_head(repo, length: length) if length.present? @@ -15,7 +19,11 @@ def self.git_head(repo = Pathname.pwd, length: nil, safe: true) end sig do - params(repo: T.any(String, Pathname), length: T.nilable(Integer), safe: T::Boolean).returns(T.nilable(String)) + params( + repo: T.any(String, Pathname), + length: T.nilable(Integer), + safe: T::Boolean, + ).returns(T.nilable(String)) end def self.git_short_head(repo = Pathname.pwd, length: nil, safe: true) repo = Pathname(repo).extend(GitRepositoryExtension)