Merge pull request #9336 from jonchang/refactor-git-extensions

git_extensions: refactor and delete redundant functions

Library/Homebrew/dev-cmd/bump-cask-pr.rb

@@ -77,8 +77,8 @@ def bump_cask_pr
     old_hash = cask.sha256.to_s
 
     tap_full_name = cask.tap&.full_name
-    origin_branch = Utils::Git.origin_branch(cask.tap.path) if cask.tap
-    origin_branch ||= "origin/master"
+    default_remote_branch = cask.tap.path.git_origin_branch if cask.tap
+    default_remote_branch ||= "master"
     previous_branch = "-"
 
     check_open_pull_requests(cask, tap_full_name, args: args)
@@ -200,7 +200,7 @@ def bump_cask_pr
     pr_info = {
       sourcefile_path: cask.sourcefile_path,
       old_contents:    old_contents,
-      origin_branch:   origin_branch,
+      remote_branch:   default_remote_branch,
       branch_name:     "bump-#{cask.token}-#{new_version.tr(",:", "-")}",
       commit_message:  "Update #{cask.token} from #{old_version} to #{new_version}",
       previous_branch: previous_branch,

Library/Homebrew/dev-cmd/bump-formula-pr.rb

@@ -84,44 +84,38 @@ def bump_formula_pr_args
   end
 
   def use_correct_linux_tap(formula, args:)
-    if OS.linux? && formula.tap.core_tap?
-      tap_full_name = formula.tap.full_name.gsub("linuxbrew", "homebrew")
-      homebrew_core_url = "https://github.com/#{tap_full_name}"
-      homebrew_core_remote = "homebrew"
-      homebrew_core_branch = "master"
-      origin_branch = "#{homebrew_core_remote}/#{homebrew_core_branch}"
-      previous_branch = Utils.popen_read("git -C \"#{formula.tap.path}\" symbolic-ref -q --short HEAD").chomp
-      previous_branch = "master" if previous_branch.empty?
-      formula_path = formula.path.to_s[%r{(Formula/.*)}, 1]
-
-      if args.dry_run? || args.write?
-        ohai "git remote add #{homebrew_core_remote} #{homebrew_core_url}"
-        ohai "git fetch #{homebrew_core_remote} #{homebrew_core_branch}"
-        ohai "git cat-file -e #{origin_branch}:#{formula_path}"
-        ohai "git checkout #{origin_branch}"
-        return tap_full_name, origin_branch, previous_branch
-      else
-        formula.path.parent.cd do
-          unless Utils.popen_read("git remote -v").match?(%r{^homebrew.*Homebrew/homebrew-core.*$})
-            ohai "Adding #{homebrew_core_remote} remote"
-            safe_system "git", "remote", "add", homebrew_core_remote, homebrew_core_url
-          end
-          ohai "Fetching #{origin_branch}"
-          safe_system "git", "fetch", homebrew_core_remote, homebrew_core_branch
-          if quiet_system "git", "cat-file", "-e", "#{origin_branch}:#{formula_path}"
-            ohai "#{formula.full_name} exists in #{origin_branch}"
-            safe_system "git", "checkout", origin_branch
-            return tap_full_name, origin_branch, previous_branch
-          end
-        end
-      end
+    default_origin_branch = formula.tap.path.git_origin_branch if formula.tap
+
+    return formula.tap&.full_name, "origin", default_origin_branch, "-" if !OS.linux? || !formula.tap.core_tap?
+
+    tap_full_name = formula.tap.full_name.gsub("linuxbrew", "homebrew")
+    homebrew_core_url = "https://github.com/#{tap_full_name}"
+    homebrew_core_remote = "homebrew"
+    previous_branch = formula.tap.path.git_branch || "master"
+    formula_path = formula.path.relative_path_from(formula.tap.path)
+    full_origin_branch = "#{homebrew_core_remote}/#{default_origin_branch}"
+
+    if args.dry_run? || args.write?
+      ohai "git remote add #{homebrew_core_remote} #{homebrew_core_url}"
+      ohai "git fetch #{homebrew_core_remote} HEAD #{default_origin_branch}"
+      ohai "git cat-file -e #{full_origin_branch}:#{formula_path}"
+      ohai "git checkout #{full_origin_branch}"
+      return tap_full_name, homebrew_core_remote, default_origin_branch, previous_branch
     end
-    if formula.tap
-      origin_branch = Utils.popen_read("git", "-C", formula.tap.path.to_s, "symbolic-ref", "-q", "--short",
-                                       "refs/remotes/origin/HEAD").chomp.presence
+
+    formula.tap.path.cd do
+      unless Utils.popen_read("git remote -v").match?(%r{^homebrew.*Homebrew/homebrew-core.*$})
+        ohai "Adding #{homebrew_core_remote} remote"
+        safe_system "git", "remote", "add", homebrew_core_remote, homebrew_core_url
+      end
+      ohai "Fetching remote #{homebrew_core_remote}"
+      safe_system "git", "fetch", homebrew_core_remote, "HEAD", default_origin_branch
+      if quiet_system "git", "cat-file", "-e", "#{full_origin_branch}:#{formula_path}"
+        ohai "#{formula.full_name} exists in #{full_origin_branch}"
+        safe_system "git", "checkout", full_origin_branch
+        return tap_full_name, homebrew_core_remote, default_origin_branch, previous_branch
+      end
     end
-    origin_branch ||= "origin/master"
-    [formula.tap&.full_name, origin_branch, "-"]
   end
 
   def bump_formula_pr
@@ -142,7 +136,7 @@ def bump_formula_pr
 
     odie "This formula is disabled!" if formula.disabled?
 
-    tap_full_name, origin_branch, previous_branch = use_correct_linux_tap(formula, args: args)
+    tap_full_name, remote, remote_branch, previous_branch = use_correct_linux_tap(formula, args: args)
     check_open_pull_requests(formula, tap_full_name, args: args)
 
     new_version = args.version
@@ -355,7 +349,8 @@ def bump_formula_pr
       sourcefile_path:  formula.path,
       old_contents:     old_contents,
       additional_files: alias_rename,
-      origin_branch:    origin_branch,
+      remote:           remote,
+      remote_branch:    remote_branch,
       branch_name:      "bump-#{formula.name}-#{new_formula_version}",
       commit_message:   "#{formula.name} #{new_formula_version}",
       previous_branch:  previous_branch,

Library/Homebrew/dev-cmd/pr-pull.rb

@@ -89,7 +89,7 @@ def separate_commit_message(message)
   end
 
   def signoff!(path, pr: nil, dry_run: false)
-    subject, body, trailers = separate_commit_message(Utils::Git.commit_message(path))
+    subject, body, trailers = separate_commit_message(path.git_commit_message)
 
     if pr
       # This is a tap pull request and approving reviewers should also sign-off.
@@ -156,7 +156,7 @@ def reword_formula_commit(commit, file, reason: "", verbose: false, resolve: fal
     new_formula = Utils::Git.file_at_commit(path, file, "HEAD")
 
     bump_subject = determine_bump_subject(old_formula, new_formula, formula_file, reason: reason).strip
-    subject, body, trailers = separate_commit_message(Utils::Git.commit_message(path))
+    subject, body, trailers = separate_commit_message(path.git_commit_message)
 
     if subject != bump_subject && !subject.start_with?("#{formula_name}:")
       safe_system("git", "-C", path, "commit", "--amend", "-q",
@@ -181,7 +181,7 @@ def squash_formula_commits(commits, file, reason: "", verbose: false, resolve: f
     messages = []
     trailers = []
     commits.each do |commit|
-      subject, body, trailer = separate_commit_message(Utils::Git.commit_message(path, commit))
+      subject, body, trailer = separate_commit_message(path.git_commit_message(commit))
       body = body.lines.map { |line| "  #{line.strip}" }.join("\n")
       messages << "* #{subject}\n#{body}".strip
       trailers << trailer
@@ -216,7 +216,8 @@ def squash_formula_commits(commits, file, reason: "", verbose: false, resolve: f
   end
 
   def autosquash!(original_commit, path: ".", reason: "", verbose: false, resolve: false)
-    original_head = Utils.safe_popen_read("git", "-C", path, "rev-parse", "HEAD").strip
+    path = Pathname(path).extend(GitRepositoryExtension)
+    original_head = path.git_head
 
     commits = Utils.safe_popen_read("git", "-C", path, "rev-list",
                                     "--reverse", "#{original_commit}..HEAD").lines.map(&:strip)
@@ -384,17 +385,14 @@ def pr_pull
       _, user, repo, pr = *url_match
       odie "Not a GitHub pull request: #{arg}" unless pr
 
-      current_branch = Utils::Git.current_branch(tap.path)
-      origin_branch = Utils::Git.origin_branch(tap.path).split("/").last
-
-      if current_branch != origin_branch || args.branch_okay? || args.clean?
-        opoo "Current branch is #{current_branch}: do you need to pull inside #{origin_branch}?"
+      if !tap.path.git_default_origin_branch? || args.branch_okay? || args.clean?
+        opoo "Current branch is #{tap.path.git_branch}: do you need to pull inside #{tap.path.git_origin_branch}?"
       end
 
       ohai "Fetching #{tap} pull request ##{pr}"
       Dir.mktmpdir pr do |dir|
         cd dir do
-          original_commit = Utils.popen_read("git", "-C", tap.path, "rev-parse", "HEAD").chomp
+          original_commit = tap.path.git_head
           cherry_pick_pr!(user, repo, pr, path: tap.path, args: args)
           if args.autosquash? && !args.dry_run?
             autosquash!(original_commit, path: tap.path,

Library/Homebrew/diagnostic.rb

@@ -572,16 +572,9 @@ def check_tap_git_branch
         return unless Utils::Git.available?
 
         commands = Tap.map do |tap|
-          next unless tap.path.git?
-          next if tap.path.git_origin.blank?
+          next if tap.path.git_default_origin_branch?
 
-          branch = tap.path.git_branch
-          next if branch.blank?
-
-          origin_branch = Utils::Git.origin_branch(tap.path)&.split("/")&.last
-          next if origin_branch == branch
-
-          "git -C $(brew --repo #{tap.name}) checkout #{origin_branch}"
+          "git -C $(brew --repo #{tap.name}) checkout #{tap.path.git_origin_branch}"
         end.compact
 
         return if commands.blank?

Library/Homebrew/extend/git_repository.rb

@@ -1,53 +1,96 @@
-# typed: false
+# typed: strict
 # frozen_string_literal: true
 
 require "utils/git"
 require "utils/popen"
 
+# Extensions to {Pathname} for querying Git repository information.
+# @see Utils::Git
+# @api private
 module GitRepositoryExtension
+  extend T::Sig
+
+  sig { returns(T::Boolean) }
   def git?
     join(".git").exist?
   end
 
+  # Gets the URL of the Git origin remote.
+  sig { returns(T.nilable(String)) }
   def git_origin
     return unless git? && Utils::Git.available?
 
     Utils.popen_read("git", "config", "--get", "remote.origin.url", chdir: self).chomp.presence
   end
 
+  # Sets the URL of the Git origin remote.
+  sig { params(origin: String).returns(T.nilable(T::Boolean)) }
   def git_origin=(origin)
     return unless git? && Utils::Git.available?
 
     safe_system "git", "remote", "set-url", "origin", origin, chdir: self
   end
 
+  # Gets the full commit hash of the HEAD commit.
+  sig { returns(T.nilable(String)) }
   def git_head
     return unless git? && Utils::Git.available?
 
     Utils.popen_read("git", "rev-parse", "--verify", "-q", "HEAD", chdir: self).chomp.presence
   end
 
+  # Gets a short commit hash of the HEAD commit.
+  sig { returns(T.nilable(String)) }
   def git_short_head
     return unless git? && Utils::Git.available?
 
     Utils.popen_read("git", "rev-parse", "--short=4", "--verify", "-q", "HEAD", chdir: self).chomp.presence
   end
 
+  # Gets the relative date of the last commit, e.g. "1 hour ago"
+  sig { returns(T.nilable(String)) }
   def git_last_commit
     return unless git? && Utils::Git.available?
 
     Utils.popen_read("git", "show", "-s", "--format=%cr", "HEAD", chdir: self).chomp.presence
   end
 
+  # Gets the name of the currently checked-out branch, or HEAD if the repository is in a detached HEAD state.
+  sig { returns(T.nilable(String)) }
   def git_branch
     return unless git? && Utils::Git.available?
 
     Utils.popen_read("git", "rev-parse", "--abbrev-ref", "HEAD", chdir: self).chomp.presence
   end
 
+  # Gets the name of the default origin HEAD branch.
+  sig { returns(T.nilable(String)) }
+  def git_origin_branch
+    return unless git? && Utils::Git.available?
+
+    Utils.popen_read("git", "symbolic-ref", "-q", "--short", "refs/remotes/origin/HEAD", chdir: self)
+         .chomp.presence&.split("/")&.last
+  end
+
+  # Returns true if the repository's current branch matches the default origin branch.
+  sig { returns(T.nilable(T::Boolean)) }
+  def git_default_origin_branch?
+    git_origin_branch == git_branch
+  end
+
+  # Returns the date of the last commit, in YYYY-MM-DD format.
+  sig { returns(T.nilable(String)) }
   def git_last_commit_date
     return unless git? && Utils::Git.available?
 
     Utils.popen_read("git", "show", "-s", "--format=%cd", "--date=short", "HEAD", chdir: self).chomp.presence
   end
+
+  # Gets the full commit message of the specified commit, or of the HEAD commit if unspecified.
+  sig { params(commit: String).returns(T.nilable(String)) }
+  def git_commit_message(commit = "HEAD")
+    return unless git? && Utils::Git.available?
+
+    Utils.popen_read("git", "log", "-1", "--pretty=%B", commit, "--", chdir: self, err: :out).strip.presence
+  end
 end

Library/Homebrew/extend/git_repository.rbi

@@ -0,0 +1,8 @@
+# typed: strict
+
+module GitRepositoryExtension
+  include Kernel
+
+  sig { params(args: T.any(String, Pathname)).returns(Pathname) }
+  def join(*args); end
+end

Library/Homebrew/test/dev-cmd/pr-pull_spec.rb

@@ -38,7 +38,7 @@ class Foo < Formula
     EOS
   end
   let(:formula_file) { path/"Formula/foo.rb" }
-  let(:path) { Tap::TAP_DIRECTORY/"homebrew/homebrew-foo" }
+  let(:path) { (Tap::TAP_DIRECTORY/"homebrew/homebrew-foo").extend(GitRepositoryExtension) }
 
   describe "Homebrew.pr_pull_args" do
     it_behaves_like "parseable arguments"
@@ -58,9 +58,9 @@ class Foo < Formula
         safe_system Utils::Git.git, "commit", formula_file, "-m", "revision"
         File.open(formula_file, "w") { |f| f.write(formula_version) }
         safe_system Utils::Git.git, "commit", formula_file, "-m", "version", "--author=#{secondary_author}"
-        described_class.autosquash!(original_hash, path: ".")
-        expect(Utils::Git.commit_message(path)).to include("foo 2.0")
-        expect(Utils::Git.commit_message(path)).to include("Co-authored-by: #{secondary_author}")
+        described_class.autosquash!(original_hash, path: path)
+        expect(path.git_commit_message).to include("foo 2.0")
+        expect(path.git_commit_message).to include("Co-authored-by: #{secondary_author}")
       end
     end
   end
@@ -75,7 +75,7 @@ class Foo < Formula
         safe_system Utils::Git.git, "commit", "-m", "foo 1.0 (new formula)"
       end
       described_class.signoff!(path)
-      expect(Utils::Git.commit_message(path)).to include("Signed-off-by:")
+      expect(path.git_commit_message).to include("Signed-off-by:")
     end
   end
 

Library/Homebrew/test/utils/git_spec.rb

@@ -52,19 +52,6 @@
   let(:files_hash2) { [@h2[0..6], ["README.md"]] }
   let(:cherry_pick_commit) { @cherry_pick_commit[0..6] }
 
-  describe "#commit_message" do
-    it "returns the commit message" do
-      expect(described_class.commit_message(HOMEBREW_CACHE, file_hash1)).to eq("File added")
-      expect(described_class.commit_message(HOMEBREW_CACHE, file_hash2)).to eq("written to File")
-    end
-
-    it "errors when commit doesn't exist" do
-      expect {
-        described_class.commit_message(HOMEBREW_CACHE, "bad_refspec")
-      }.to raise_error(ErrorDuringExecution, /bad revision/)
-    end
-  end
-
   describe "#cherry_pick!" do
     it "can cherry pick a commit" do
       expect(described_class.cherry_pick!(HOMEBREW_CACHE, cherry_pick_commit)).to be_truthy

Library/Homebrew/utils/git.rb

@@ -4,6 +4,7 @@
 module Utils
   # Helper functions for querying Git information.
   #
+  # @see GitRepositoryExtension
   # @api private
   module Git
     module_function
@@ -88,8 +89,9 @@ def file_at_commit(repo, file, commit)
     end
 
     def commit_message(repo, commit = nil)
+      odeprecated "Utils::Git.commit_message(repo)", "Pathname(repo).git_commit_message"
       commit ||= "HEAD"
-      Utils.safe_popen_read(git, "-C", repo, "log", "-1", "--pretty=%B", commit, "--", err: :out).strip
+      Pathname(repo).extend(GitRepositoryExtension).git_commit_message(commit)
     end
 
     def ensure_installed!
@@ -134,12 +136,13 @@ def setup_gpg!
     end
 
     def origin_branch(repo)
-      Utils.popen_read(git, "-C", repo, "symbolic-ref", "-q", "--short",
-                       "refs/remotes/origin/HEAD").chomp.presence
+      odeprecated "Utils::Git.origin_branch(repo)", "Pathname(repo).git_origin_branch"
+      Pathname(repo).extend(GitRepositoryExtension).git_origin_branch
     end
 
     def current_branch(repo)
-      Utils.popen_read("git", "-C", repo, "symbolic-ref", "--short", "HEAD").chomp.presence
+      odeprecated "Utils::Git.current_branch(repo)", "Pathname(repo).git_branch"
+      Pathname(repo).extend(GitRepositoryExtension).git_branch
     end
 
     # Special case of `git cherry-pick` that permits non-verbose output and