Merge pull request #8754 from jonchang/refactor-git-cmds

dev-cmd/pr-pull: refactor and add tests

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

@@ -35,7 +35,7 @@ module Homebrew
              description: "Automatically reformat and reword commits in the pull request to our "\
                           "preferred format."
       switch "--branch-okay",
-             description: "Do not warn if pulling to a branch besides master (useful for testing)."
+             description: "Do not warn if pulling to a branch besides the repository default (useful for testing)."
       switch "--resolve",
              description: "When a patch fails to apply, leave in progress and allow user to resolve, "\
                           "instead of aborting."
@@ -98,39 +98,43 @@ module Homebrew
     [subject, body, trailers]
   end
 
-  def signoff!(pr, tap:, args:)
+  def signoff!(path, pr: nil, dry_run: false)
-    message = Utils.popen_read "git", "-C", tap.path, "log", "-1", "--pretty=%B"
+    subject, body, trailers = separate_commit_message(Utils::Git.commit_message(path))
-    subject, body, trailers = separate_commit_message(message)
 
-    # Approving reviewers also sign-off on merge.
+    if pr
+      # This is a tap pull request and approving reviewers should also sign-off.
+      tap = Tap.from_path(path)
       trailers += GitHub.approved_reviews(tap.user, "homebrew-#{tap.repo}", pr).map do |r|
         "Signed-off-by: #{r["name"]} <#{r["email"]}>"
       end.join("\n")
 
+      # Append the close message as well, unless the commit body already includes it.
       close_message = "Closes ##{pr}."
       body += "\n\n#{close_message}" unless body.include? close_message
-    new_message = [subject, body, trailers].join("\n\n").strip
+    end
 
-    if args.dry_run?
+    git_args = Utils::Git.git, "-C", path, "commit", "--amend", "--signoff", "--allow-empty", "--quiet",
-      puts "git commit --amend --signoff -m $message"
+               "--message", subject, "--message", body, "--message", trailers
+
+    if dry_run
+      puts(*git_args)
     else
-      safe_system "git", "-C", tap.path, "commit", "--amend", "--signoff", "--allow-empty", "-q", "-m", new_message
+      safe_system(*git_args)
     end
   end
 
-  def determine_bump_subject(file, original_commit, path: ".", reason: nil)
+  def determine_bump_subject(old_contents, new_contents, formula_path, reason: nil)
-    full_path = Pathname.new(path)/file
+    formula_path = Pathname(formula_path)
-    formula_name = File.basename(file.chomp(".rb"))
+    formula_name = formula_path.basename.to_s.chomp(".rb")
 
     new_formula = begin
-      Formulary::FormulaLoader.new(formula_name, full_path).get_formula(:stable)
+      Formulary.from_contents(formula_name, formula_path, new_contents, :stable)
     rescue FormulaUnavailableError
-      return "#{formula_name}: delete #{reason}"
+      return "#{formula_name}: delete #{reason}".strip
     end
 
     old_formula = begin
-      old_file = Utils.popen_read "git", "-C", path, "show", "#{original_commit}:#{file}"
+      Formulary.from_contents(formula_name, formula_path, old_contents, :stable)
-      Formulary.from_contents(formula_name, full_path, old_file, :stable)
     rescue FormulaUnavailableError
       return "#{formula_name} #{new_formula.stable.version} (new formula)"
     end
@@ -138,22 +142,26 @@ module Homebrew
     if old_formula.stable.version != new_formula.stable.version
       "#{formula_name} #{new_formula.stable.version}"
     elsif old_formula.revision != new_formula.revision
-      "#{formula_name}: revision #{reason}"
+      "#{formula_name}: revision #{reason}".strip
     else
-      "#{formula_name}: #{reason || "rebuild"}"
+      "#{formula_name}: #{reason || "rebuild"}".strip
     end
   end
 
   # Cherry picks a single commit that modifies a single file.
   # Potentially rewords this commit using `determine_bump_subject`.
-  def reword_formula_commit(commit, file, args:, path: ".")
+  def reword_formula_commit(commit, file, reason: "", verbose: false, resolve: false, path: ".")
-    formula_name = File.basename(file.chomp(".rb"))
+    formula_file = Pathname.new(path) / file
-    odebug "Cherry-picking #{file}: #{commit}"
+    formula_name = formula_file.basename.to_s.chomp(".rb")
-    Utils::Git.cherry_pick!(path, commit, verbose: args.verbose?, resolve: args.resolve?)
 
-    bump_subject = determine_bump_subject(file, "HEAD^", path: path, reason: args.message).strip
+    odebug "Cherry-picking #{formula_file}: #{commit}"
-    message = Utils.popen_read("git", "-C", path, "log", "-1", "--pretty=%B")
+    Utils::Git.cherry_pick!(path, commit, verbose: verbose, resolve: resolve)
-    subject, body, trailers = separate_commit_message(message)
+
+    old_formula = Utils::Git.file_at_commit(path, file, "HEAD^")
+    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))
 
     if subject != bump_subject && !subject.start_with?("#{formula_name}:")
       safe_system("git", "-C", path, "commit", "--amend", "-q",
@@ -167,7 +175,7 @@ module Homebrew
   # Cherry picks multiple commits that each modify a single file.
   # Words the commit according to `determine_bump_subject` with the body
   # corresponding to all the original commit messages combined.
-  def squash_formula_commits(commits, file, args:, path: ".")
+  def squash_formula_commits(commits, file, reason: "", verbose: false, resolve: false, path: ".")
     odebug "Squashing #{file}: #{commits.join " "}"
 
     # Format commit messages into something similar to `git fmt-merge-message`.
@@ -178,8 +186,7 @@ module Homebrew
     messages = []
     trailers = []
     commits.each do |commit|
-      original_message = Utils.safe_popen_read("git", "-C", path, "show", "--no-patch", "--pretty=%B", commit)
+      subject, body, trailer = separate_commit_message(Utils::Git.commit_message(path, commit))
-      subject, body, trailer = separate_commit_message(original_message)
       body = body.lines.map { |line| "  #{line.strip}" }.join("\n")
       messages << "* #{subject}\n#{body}".strip
       trailers << trailer
@@ -198,10 +205,13 @@ module Homebrew
     trailers = [trailers + co_author_trailers].flatten.uniq.compact
 
     # Apply the patch series but don't commit anything yet.
-    Utils::Git.cherry_pick!(path, "--no-commit", *commits, verbose: args.verbose?, resolve: args.resolve?)
+    Utils::Git.cherry_pick!(path, "--no-commit", *commits, verbose: verbose, resolve: resolve)
 
     # Determine the bump subject by comparing the original state of the tree to its current state.
-    bump_subject = determine_bump_subject(file, "#{commits.first}^", path: path, reason: args.message).strip
+    formula_file = Pathname.new(path) / file
+    old_formula = Utils::Git.file_at_commit(path, file, "#{commits.first}^")
+    new_formula = File.read(formula_file)
+    bump_subject = determine_bump_subject(old_formula, new_formula, formula_file, reason: reason)
 
     # Commit with the new subject, body, and trailers.
     safe_system("git", "-C", path, "commit", "--quiet",
@@ -210,11 +220,7 @@ module Homebrew
     ohai bump_subject
   end
 
-  def autosquash!(original_commit, path: ".", args: nil)
+  def autosquash!(original_commit, path: ".", reason: "", verbose: false, resolve: false)
-    # Autosquash assumes we've already modified the current state of the git repository,
-    # so just exit early if we're in a dry run.
-    return if args.dry_run?
-
     original_head = Utils.safe_popen_read("git", "-C", path, "rev-parse", "HEAD").strip
 
     commits = Utils.safe_popen_read("git", "-C", path, "rev-list",
@@ -251,13 +257,13 @@ module Homebrew
       files = commits_to_files[commit]
       if files.length == 1 && files_to_commits[files.first].length == 1
         # If there's a 1:1 mapping of commits to files, just cherry pick and (maybe) reword.
-        reword_formula_commit(commit, files.first, path: path, args: args)
+        reword_formula_commit(commit, files.first, path: path, reason: reason, verbose: verbose, resolve: resolve)
         processed_commits << commit
       elsif files.length == 1 && files_to_commits[files.first].length > 1
         # If multiple commits modify a single file, squash them down into a single commit.
         file = files.first
         commits = files_to_commits[file]
-        squash_formula_commits(commits, file, path: path, args: args)
+        squash_formula_commits(commits, file, path: path, reason: reason, verbose: verbose, resolve: resolve)
         processed_commits += commits
       else
         # We can't split commits (yet) so just raise an error.
@@ -275,30 +281,20 @@ module Homebrew
     raise
   end
 
-  def cherry_pick_pr!(pr, args:, path: ".")
+  def cherry_pick_pr!(user, repo, pr, args:, path: ".")
     if args.dry_run?
       puts <<~EOS
         git fetch --force origin +refs/pull/#{pr}/head
         git merge-base HEAD FETCH_HEAD
         git cherry-pick --ff --allow-empty $merge_base..FETCH_HEAD
       EOS
-    else
+      return
-      safe_system "git", "-C", path, "fetch", "--quiet", "--force", "origin", "+refs/pull/#{pr}/head"
-      merge_base = Utils.popen_read("git", "-C", path, "merge-base", "HEAD", "FETCH_HEAD").strip
-      commit_count = Utils.popen_read("git", "-C", path, "rev-list", "#{merge_base}..FETCH_HEAD").lines.count
-
-      ohai "Using #{commit_count} commit#{"s" unless commit_count == 1} from ##{pr}"
-      Utils::Git.cherry_pick!(path, "--ff", "--allow-empty", "#{merge_base}..FETCH_HEAD",
-                              verbose: args.verbose?, resolve: args.resolve?)
-    end
     end
 
-  def check_branch(path, ref, args:)
+    commits = GitHub.pull_request_commits(user, repo, pr)
-    branch = Utils.popen_read("git", "-C", path, "symbolic-ref", "--short", "HEAD").strip
+    safe_system "git", "-C", path, "fetch", "--quiet", "--force", "origin", commits.last
-
+    ohai "Using #{commits.count} commit#{"s" unless commits.count == 1} from ##{pr}"
-    return if branch == ref || args.clean? || args.branch_okay?
+    Utils::Git.cherry_pick!(path, "--ff", "--allow-empty", *commits, verbose: args.verbose?, resolve: args.resolve?)
-
-    opoo "Current branch is #{branch}: do you need to pull inside #{ref}?"
   end
 
   def formulae_need_bottles?(tap, original_commit, args:)
@@ -385,15 +381,23 @@ module Homebrew
       _, user, repo, pr = *url_match
       odie "Not a GitHub pull request: #{arg}" unless pr
 
-      check_branch tap.path, "master", args: args
+      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}?"
+      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
-          cherry_pick_pr!(pr, path: tap.path, args: args)
+          cherry_pick_pr!(user, repo, pr, path: tap.path, args: args)
-          autosquash!(original_commit, path: tap.path, args: args) if args.autosquash?
+          if args.autosquash? && !args.dry_run?
-          signoff!(pr, tap: tap, args: args) unless args.clean?
+            autosquash!(original_commit, path: tap.path,
+                        verbose: args.verbose?, resolve: args.resolve?, reason: args.message)
+          end
+          signoff!(tap.path, pr: pr, dry_run: args.dry_run?) unless args.clean?
 
           unless args.no_upload?
             mirror_formulae(tap, original_commit,

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

@@ -1,7 +1,104 @@
 # frozen_string_literal: true
 
+require "dev-cmd/pr-pull"
+require "utils/git"
+require "tap"
 require "cmd/shared_examples/args_parse"
 
-describe "Homebrew.pr_pull_args" do
+describe Homebrew do
+  let(:formula_rebuild) do
+    <<~EOS
+      class Foo < Formula
+        desc "Helpful description"
+        url "https://brew.sh/foo-1.0.tgz"
+      end
+    EOS
+  end
+  let(:formula_revision) do
+    <<~EOS
+      class Foo < Formula
+        url "https://brew.sh/foo-1.0.tgz"
+        revision 1
+      end
+    EOS
+  end
+  let(:formula_version) do
+    <<~EOS
+      class Foo < Formula
+        url "https://brew.sh/foo-2.0.tgz"
+      end
+    EOS
+  end
+  let(:formula) do
+    <<~EOS
+      class Foo < Formula
+        url "https://brew.sh/foo-1.0.tgz"
+      end
+    EOS
+  end
+  let(:formula_file) { path/"Formula/foo.rb" }
+  let(:path) { Tap::TAP_DIRECTORY/"homebrew/homebrew-foo" }
+
+  describe "Homebrew.pr_pull_args" do
     it_behaves_like "parseable arguments"
+  end
+
+  describe "#autosquash!" do
+    it "squashes a formula correctly" do
+      secondary_author = "Someone Else <me@example.com>"
+      (path/"Formula").mkpath
+      formula_file.write(formula)
+      cd path do
+        safe_system Utils::Git.git, "init"
+        safe_system Utils::Git.git, "add", formula_file
+        safe_system Utils::Git.git, "commit", "-m", "foo 1.0 (new formula)"
+        original_hash = `git rev-parse HEAD`.chomp
+        File.open(formula_file, "w") { |f| f.write(formula_revision) }
+        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}")
+      end
+    end
+  end
+
+  describe "#signoff!" do
+    it "signs off a formula" do
+      (path/"Formula").mkpath
+      formula_file.write(formula)
+      cd path do
+        safe_system Utils::Git.git, "init"
+        safe_system Utils::Git.git, "add", formula_file
+        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:")
+    end
+  end
+
+  describe "#determine_bump_subject" do
+    it "correctly bumps a new formula" do
+      expect(described_class.determine_bump_subject("", formula, formula_file)).to eq("foo 1.0 (new formula)")
+    end
+
+    it "correctly bumps a formula version" do
+      expect(described_class.determine_bump_subject(formula, formula_version, formula_file)).to eq("foo 2.0")
+    end
+
+    it "correctly bumps a formula revision with reason" do
+      expect(described_class.determine_bump_subject(
+               formula, formula_revision, formula_file, reason: "for fun"
+             )).to eq("foo: revision for fun")
+    end
+
+    it "correctly bumps a formula rebuild" do
+      expect(described_class.determine_bump_subject(formula, formula_rebuild, formula_file)).to eq("foo: rebuild")
+    end
+
+    it "correctly bumps a formula deletion" do
+      expect(described_class.determine_bump_subject(formula, "", formula_file)).to eq("foo: delete")
+    end
+  end
 end

Library/Homebrew/test/utils/git_spec.rb

@@ -18,22 +18,28 @@ describe Utils::Git do
 
       File.open("README.md", "w") { |f| f.write("README") }
       system git, "add", HOMEBREW_CACHE/"README.md"
-      system git, "commit", "-m", "'File added'"
+      system git, "commit", "-m", "File added"
       @h1 = `git rev-parse HEAD`
 
       File.open("README.md", "w") { |f| f.write("# README") }
       system git, "add", HOMEBREW_CACHE/"README.md"
-      system git, "commit", "-m", "'written to File'"
+      system git, "commit", "-m", "written to File"
       @h2 = `git rev-parse HEAD`
 
       File.open("LICENSE.txt", "w") { |f| f.write("LICENCE") }
       system git, "add", HOMEBREW_CACHE/"LICENSE.txt"
-      system git, "commit", "-m", "'File added'"
+      system git, "commit", "-m", "File added"
       @h3 = `git rev-parse HEAD`
 
       File.open("LICENSE.txt", "w") { |f| f.write("LICENSE") }
       system git, "add", HOMEBREW_CACHE/"LICENSE.txt"
-      system git, "commit", "-m", "'written to File'"
+      system git, "commit", "-m", "written to File"
+
+      File.open("LICENSE.txt", "w") { |f| f.write("test") }
+      system git, "add", HOMEBREW_CACHE/"LICENSE.txt"
+      system git, "commit", "-m", "written to File"
+      @cherry_pick_commit = `git rev-parse HEAD`
+      system git, "reset", "--hard", "HEAD^"
     end
   end
 
@@ -43,8 +49,26 @@ describe Utils::Git do
   let(:files) { ["README.md", "LICENSE.txt"] }
   let(:files_hash1) { [@h3[0..6], ["LICENSE.txt"]] }
   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
+    end
+
     it "aborts when cherry picking an existing hash" do
       expect {
         described_class.cherry_pick!(HOMEBREW_CACHE, file_hash1)
@@ -68,6 +92,17 @@ describe Utils::Git do
     end
   end
 
+  describe "#file_at_commit" do
+    it "returns file contents when file exists" do
+      expect(described_class.file_at_commit(HOMEBREW_CACHE, file, file_hash1)).to eq("README")
+    end
+
+    it "returns empty when file doesn't exist" do
+      expect(described_class.file_at_commit(HOMEBREW_CACHE, "foo.txt", file_hash1)).to eq("")
+      expect(described_class.file_at_commit(HOMEBREW_CACHE, "LICENSE.txt", file_hash1)).to eq("")
+    end
+  end
+
   describe "#last_revision_commit_of_files" do
     context "when before_commit is nil" do
       it "gives last revision commit" do

Library/Homebrew/test/utils/github_spec.rb

@@ -75,4 +75,11 @@ describe GitHub do
       expect(url).to eq("https://api.github.com/repos/Homebrew/homebrew-core/actions/artifacts/3557392/zip")
     end
   end
+
+  describe "::pull_request_commits", :needs_network do
+    it "gets the correct commits hashes for a pull request" do
+      hashes = %w[188606a4a9587365d930b02c98ad6857b1d00150 25a71fe1ea1558415d6496d23834dc70778ddee5]
+      expect(subject.pull_request_commits("Homebrew", "legacy-homebrew", 50678)).to eq(hashes)
+    end
+  end
 end

Library/Homebrew/utils/git.rb

@@ -76,9 +76,19 @@ module Utils
 
     def last_revision_of_file(repo, file, before_commit: nil)
       relative_file = Pathname(file).relative_path_from(repo)
-
       commit_hash = last_revision_commit_of_file(repo, relative_file, before_commit: before_commit)
-      Utils.popen_read(git, "-C", repo, "show", "#{commit_hash}:#{relative_file}")
+      file_at_commit(repo, file, commit_hash)
+    end
+
+    def file_at_commit(repo, file, commit)
+      relative_file = Pathname(file)
+      relative_file = relative_file.relative_path_from(repo) if relative_file.absolute?
+      Utils.popen_read(git, "-C", repo, "show", "#{commit}:#{relative_file}")
+    end
+
+    def commit_message(repo, commit = nil)
+      commit ||= "HEAD"
+      Utils.safe_popen_read(git, "-C", repo, "log", "-1", "--pretty=%B", commit, "--", err: :out).strip
     end
 
     def ensure_installed!
@@ -120,6 +130,10 @@ module Utils
                        "refs/remotes/origin/HEAD").chomp.presence
     end
 
+    def current_branch(repo)
+      Utils.popen_read("git", "-C", repo, "symbolic-ref", "--short", "HEAD").chomp.presence
+    end
+
     # Special case of `git cherry-pick` that permits non-verbose output and
     # optional resolution on merge conflict.
     def cherry_pick!(repo, *args, resolve: false, verbose: false)

Library/Homebrew/utils/github.rb

@@ -747,4 +747,8 @@ module GitHub
       end
     end
   end
+
+  def pull_request_commits(user, repo, pr)
+    open_api(url_to("repos", user, repo, "pulls", pr, "commits?per_page=100")).map { |c| c["sha"] }
+  end
 end

docs/Manpage.md

@@ -1101,7 +1101,7 @@ Requires write access to the repository.
 * `--autosquash`:
   Automatically reformat and reword commits in the pull request to our preferred format.
 * `--branch-okay`:
-  Do not warn if pulling to a branch besides master (useful for testing).
+  Do not warn if pulling to a branch besides the repository default (useful for testing).
 * `--resolve`:
   When a patch fails to apply, leave in progress and allow user to resolve, instead of aborting.
 * `--warn-on-upload-failure`:

manpages/brew.1

@@ -1528,7 +1528,7 @@ Automatically reformat and reword commits in the pull request to our preferred f
 .
 .TP
 \fB\-\-branch\-okay\fR
-Do not warn if pulling to a branch besides master (useful for testing)\.
+Do not warn if pulling to a branch besides the repository default (useful for testing)\.
 .
 .TP
 \fB\-\-resolve\fR