git_repository: raise error instead of returning nil if safe

Library/Homebrew/extend/git_repository.rb

@@ -34,20 +34,14 @@ module GitRepositoryExtension
   # 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 @@ module GitRepositoryExtension
 
     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

Library/Homebrew/test/utils/git_repository_spec.rb

@@ -17,24 +17,82 @@ describe Utils do
   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

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 @@ module Utils
   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)