dev-cmd/pr-pull: refactor and test signoff!

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

@@ -98,22 +98,28 @@ module Homebrew
     [subject, body, trailers]
   end
 
-  def signoff!(pr, tap:, args:)
-    subject, body, trailers = separate_commit_message(Utils::Git.commit_message(tap.path))
+  def signoff!(path, pr: nil, dry_run: false)
+    subject, body, trailers = separate_commit_message(Utils::Git.commit_message(path))
 
-    # Approving reviewers also sign-off on merge.
-    trailers += GitHub.approved_reviews(tap.user, "homebrew-#{tap.repo}", pr).map do |r|
-      "Signed-off-by: #{r["name"]} <#{r["email"]}>"
-    end.join("\n")
+    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")
 
-    close_message = "Closes ##{pr}."
-    body += "\n\n#{close_message}" unless body.include? close_message
-    new_message = [subject, body, trailers].join("\n\n").strip
+      # 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
+    end
 
-    if args.dry_run?
-      puts "git commit --amend --signoff -m $message"
+    git_args = Utils::Git.git, "-C", path, "commit", "--amend", "--signoff", "--allow-empty", "--quiet",
+               "--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
 
@@ -392,7 +398,7 @@ module Homebrew
           original_commit = Utils.popen_read("git", "-C", tap.path, "rev-parse", "HEAD").chomp
           cherry_pick_pr!(user, repo, pr, path: tap.path, args: args)
           autosquash!(original_commit, path: tap.path, args: args) if args.autosquash?
-          signoff!(pr, tap: tap, args: args) unless args.clean?
+          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,68 +1,83 @@
 # frozen_string_literal: true
 
 require "dev-cmd/pr-pull"
+require "utils/git"
+require "tap"
 require "cmd/shared_examples/args_parse"
 
 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 "#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
-    let(:formula) do
-      <<~EOS
-        class Foo < Formula
-          url "https://brew.sh/foo-1.0.tgz"
-        end
-      EOS
-    end
-
-    let(:formula_version) do
-      <<~EOS
-        class Foo < Formula
-          url "https://brew.sh/foo-2.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_rebuild) do
-      <<~EOS
-        class Foo < Formula
-          desc "Helpful description"
-          url "https://brew.sh/foo-1.0.tgz"
-        end
-      EOS
-    end
-
     it "correctly bumps a new formula" do
-      expect(described_class.determine_bump_subject("", formula, "foo.rb")).to eq("foo 1.0 (new formula)")
+      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, "foo.rb")).to eq("foo 2.0")
+      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, "foo.rb", reason: "for fun"
+               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, "foo.rb")).to eq("foo: rebuild")
+      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, "", "foo.rb")).to eq("foo: delete")
+      expect(described_class.determine_bump_subject(formula, "", formula_file)).to eq("foo: delete")
     end
   end
 end