Merge pull request #12692 from SMillerDev/feature/pr-pull/casks

pr-pull: allow casks to be pulled

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

@@ -108,51 +108,63 @@ module Homebrew
     end
   end
 
-  def determine_bump_subject(old_contents, new_contents, formula_path, reason: nil)
+  def get_package(tap, subject_name, subject_path, content)
-    formula_path = Pathname(formula_path)
+    if subject_path.dirname == tap.cask_dir
-    formula_name = formula_path.basename.to_s.chomp(".rb")
+      cask = begin
+        Cask::CaskLoader.load(content.dup)
+      rescue Cask::CaskUnavailableError
+        nil
+      end
+      return cask
+    end
 
-    new_formula = begin
+    begin
-      Formulary.from_contents(formula_name, formula_path, new_contents, :stable)
+      Formulary.from_contents(subject_name, subject_path, content, :stable)
     rescue FormulaUnavailableError
       nil
     end
+  end
 
-    return "#{formula_name}: delete #{reason}".strip if new_formula.blank?
+  def determine_bump_subject(old_contents, new_contents, subject_path, reason: nil)
+    subject_path = Pathname(subject_path)
+    tap          = Tap.from_path(subject_path)
+    subject_name = subject_path.basename.to_s.chomp(".rb")
+    is_cask      = subject_path.dirname == tap.cask_dir
+    name         = is_cask ? "cask" : "formula"
 
-    old_formula = begin
+    new_package = get_package(tap, subject_name, subject_path, new_contents)
-      Formulary.from_contents(formula_name, formula_path, old_contents, :stable)
-    rescue FormulaUnavailableError
-      nil
-    end
 
-    return "#{formula_name} #{new_formula.stable.version} (new formula)" if old_formula.blank?
+    return "#{subject_name}: delete #{reason}".strip if new_package.blank?
 
-    if old_formula.stable.version != new_formula.stable.version
+    old_package = get_package(tap, subject_name, subject_path, old_contents)
-      "#{formula_name} #{new_formula.stable.version}"
+
-    elsif old_formula.revision != new_formula.revision
+    if old_package.blank?
-      "#{formula_name}: revision #{reason}".strip
+      "#{subject_name} #{new_package.version} (new #{name})"
+    elsif old_package.version != new_package.version
+      "#{subject_name} #{new_package.version}"
+    elsif !is_cask && old_package.revision != new_package.revision
+      "#{subject_name}: revision #{reason}".strip
     else
-      "#{formula_name}: #{reason || "rebuild"}".strip
+      "#{subject_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, reason: "", verbose: false, resolve: false, path: ".")
+  def reword_package_commit(commit, file, reason: "", verbose: false, resolve: false, path: ".")
-    formula_file = Pathname.new(path) / file
+    package_file = Pathname.new(path) / file
-    formula_name = formula_file.basename.to_s.chomp(".rb")
+    package_name = package_file.basename.to_s.chomp(".rb")
 
-    odebug "Cherry-picking #{formula_file}: #{commit}"
+    odebug "Cherry-picking #{package_file}: #{commit}"
     Utils::Git.cherry_pick!(path, commit, verbose: verbose, resolve: resolve)
 
-    old_formula = Utils::Git.file_at_commit(path, file, "HEAD^")
+    old_package = Utils::Git.file_at_commit(path, file, "HEAD^")
-    new_formula = Utils::Git.file_at_commit(path, file, "HEAD")
+    new_package = Utils::Git.file_at_commit(path, file, "HEAD")
 
-    bump_subject = determine_bump_subject(old_formula, new_formula, formula_file, reason: reason).strip
+    bump_subject = determine_bump_subject(old_package, new_package, package_file, reason: reason).strip
     subject, body, trailers = separate_commit_message(path.git_commit_message)
 
-    if subject != bump_subject && !subject.start_with?("#{formula_name}:")
+    if subject != bump_subject && !subject.start_with?("#{package_name}:")
       safe_system("git", "-C", path, "commit", "--amend", "-q",
                   "-m", bump_subject, "-m", subject, "-m", body, "-m", trailers)
       ohai bump_subject
@@ -164,7 +176,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, reason: "", verbose: false, resolve: false, path: ".")
+  def squash_package_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`.
@@ -197,10 +209,10 @@ module Homebrew
     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.
-    formula_file = Pathname.new(path) / file
+    package_file = Pathname.new(path) / file
-    old_formula = Utils::Git.file_at_commit(path, file, "#{commits.first}^")
+    old_package = Utils::Git.file_at_commit(path, file, "#{commits.first}^")
-    new_formula = File.read(formula_file)
+    new_package = package_file.read
-    bump_subject = determine_bump_subject(old_formula, new_formula, formula_file, reason: reason)
+    bump_subject = determine_bump_subject(old_package, new_package, package_file, reason: reason)
 
     # Commit with the new subject, body, and trailers.
     safe_system("git", "-C", path, "commit", "--quiet",
@@ -215,7 +227,7 @@ module Homebrew
     commits = Utils.safe_popen_read("git", "-C", tap.path, "rev-list",
                                     "--reverse", "#{original_commit}..HEAD").lines.map(&:strip)
 
-    # Generate a bidirectional mapping of commits <=> formula files.
+    # Generate a bidirectional mapping of commits <=> formula/cask files.
     files_to_commits = {}
     commits_to_files = commits.to_h do |commit|
       files = Utils.safe_popen_read("git", "-C", tap.path, "diff-tree", "--diff-filter=AMD",
@@ -223,10 +235,14 @@ module Homebrew
       files.each do |file|
         files_to_commits[file] ||= []
         files_to_commits[file] << commit
-        next if (tap.path/file).dirname == tap.formula_dir && File.extname(file) == ".rb"
+        tap_file = tap.path/file
+        if (tap_file.dirname == tap.formula_dir || tap_file.dirname == tap.cask_dir) &&
+           File.extname(file) == ".rb"
+          next
+        end
 
         odie <<~EOS
-          Autosquash can't squash commits that modify non-formula files.
+          Autosquash can only squash commits that modify formula or cask files.
             File:   #{file}
             Commit: #{commit}
         EOS
@@ -246,13 +262,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: tap.path, reason: reason, verbose: verbose, resolve: resolve)
+        reword_package_commit(commit, files.first, path: tap.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: tap.path, reason: reason, verbose: verbose, resolve: resolve)
+        squash_package_commits(commits, file, path: tap.path, reason: reason, verbose: verbose, resolve: resolve)
         processed_commits += commits
       else
         # We can't split commits (yet) so just raise an error.
@@ -293,27 +309,45 @@ module Homebrew
 
     return false if labels.include?("CI-syntax-only") || labels.include?("CI-no-bottles")
 
-    changed_formulae(tap, original_commit).any? do |f|
+    changed_packages(tap, original_commit).any? do |f|
-      !f.bottle_unneeded? && !f.bottle_disabled?
+      !f.instance_of?(Cask::Cask) && !f.bottle_unneeded? && !f.bottle_disabled?
     end
   end
 
-  def changed_formulae(tap, original_commit)
+  def changed_packages(tap, original_commit)
-    if Homebrew::EnvConfig.disable_load_formula?
+    formulae = Utils.popen_read("git", "-C", tap.path, "diff-tree",
-      opoo "Can't check if updated bottles are necessary as HOMEBREW_DISABLE_LOAD_FORMULA is set!"
+                                "-r", "--name-only", "--diff-filter=AM",
-      return
+                                original_commit, "HEAD", "--", tap.formula_dir)
-    end
+                    .lines
-
+                    .map do |line|
-    Utils.popen_read("git", "-C", tap.path, "diff-tree",
-                     "-r", "--name-only", "--diff-filter=AM",
-                     original_commit, "HEAD", "--", tap.formula_dir)
-         .lines
-         .map do |line|
       next unless line.end_with? ".rb\n"
 
       name = "#{tap.name}/#{File.basename(line.chomp, ".rb")}"
-      Formula[name]
+      if Homebrew::EnvConfig.disable_load_formula?
+        opoo "Can't check if updated bottles are necessary as HOMEBREW_DISABLE_LOAD_FORMULA is set!"
+        break
+      end
+      begin
+        Formulary.resolve(name)
+      rescue FormulaUnavailableError
+        nil
+      end
     end.compact
+    casks = Utils.popen_read("git", "-C", tap.path, "diff-tree",
+                             "-r", "--name-only", "--diff-filter=AM",
+                             original_commit, "HEAD", "--", tap.cask_dir)
+                 .lines
+                 .map do |line|
+      next unless line.end_with? ".rb\n"
+
+      name = "#{tap.name}/#{File.basename(line.chomp, ".rb")}"
+      begin
+        Cask::CaskLoader.load(name)
+      rescue Cask::CaskUnavailableError
+        nil
+      end
+    end.compact
+    formulae + casks
   end
 
   def download_artifact(url, dir, pr)

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

@@ -40,11 +40,38 @@ describe "brew pr-pull" do
         end
       EOS
     end
+    let(:cask_rebuild) do
+      <<~EOS
+        cask "food" do
+          desc "Helpful description"
+          version "1.0"
+          url "https://brew.sh/food-\#{version}.tgz"
+        end
+      EOS
+    end
+    let(:cask_version) do
+      <<~EOS
+        cask "food" do
+          version "2.0"
+          url "https://brew.sh/food-\#{version}.tgz"
+        end
+      EOS
+    end
+    let(:cask) do
+      <<~EOS
+        cask "food" do
+          version "1.0"
+          url "https://brew.sh/food-\#{version}.tgz"
+        end
+      EOS
+    end
     let(:tap) { Tap.fetch("Homebrew", "foo") }
     let(:formula_file) { tap.path/"Formula/foo.rb" }
+    let(:cask_file) { tap.cask_dir/"food.rb" }
+    let(:path) { (Tap::TAP_DIRECTORY/"homebrew/homebrew-foo").extend(GitRepositoryExtension) }
 
     describe "#autosquash!" do
-      it "squashes a formula correctly" do
+      it "squashes a formula or cask correctly" do
         secondary_author = "Someone Else <me@example.com>"
         (tap.path/"Formula").mkpath
         formula_file.write(formula)
@@ -61,11 +88,26 @@ describe "brew pr-pull" do
           expect(tap.path.git_commit_message).to include("foo 2.0")
           expect(tap.path.git_commit_message).to include("Co-authored-by: #{secondary_author}")
         end
+
+        (path/"Casks").mkpath
+        cask_file.write(cask)
+        cd path do
+          safe_system Utils::Git.git, "add", cask_file
+          safe_system Utils::Git.git, "commit", "-m", "food 1.0 (new cask)"
+          original_hash = `git rev-parse HEAD`.chomp
+          File.write(cask_file, cask_rebuild)
+          safe_system Utils::Git.git, "commit", cask_file, "-m", "rebuild"
+          File.write(cask_file, cask_version)
+          safe_system Utils::Git.git, "commit", cask_file, "-m", "version", "--author=#{secondary_author}"
+          described_class.autosquash!(original_hash, tap: tap)
+          expect(path.git_commit_message).to include("food 2.0")
+          expect(path.git_commit_message).to include("Co-authored-by: #{secondary_author}")
+        end
       end
     end
 
     describe "#signoff!" do
-      it "signs off a formula" do
+      it "signs off a formula or cask" do
         (tap.path/"Formula").mkpath
         formula_file.write(formula)
         cd tap.path do
@@ -75,6 +117,33 @@ describe "brew pr-pull" do
         end
         described_class.signoff!(tap.path)
         expect(tap.path.git_commit_message).to include("Signed-off-by:")
+
+        (path/"Casks").mkpath
+        cask_file.write(cask)
+        cd path do
+          safe_system Utils::Git.git, "add", cask_file
+          safe_system Utils::Git.git, "commit", "-m", "food 1.0 (new cask)"
+        end
+        described_class.signoff!(tap.path)
+        expect(tap.path.git_commit_message).to include("Signed-off-by:")
+      end
+    end
+
+    describe "#get_package" do
+      it "returns a formula" do
+        expect(described_class.get_package(tap, "foo", formula_file, formula)).to be_a(Formula)
+      end
+
+      it "returns nil for an unknown formula" do
+        expect(described_class.get_package(tap, "foo", formula_file, "")).to be_nil
+      end
+
+      it "returns a cask" do
+        expect(described_class.get_package(tap, "foo", cask_file, cask)).to be_a(Cask::Cask)
+      end
+
+      it "returns nil for an unknown cask" do
+        expect(described_class.get_package(tap, "foo", cask_file, "")).to be_nil
       end
     end
 
@@ -83,10 +152,18 @@ describe "brew pr-pull" do
         expect(described_class.determine_bump_subject("", formula, formula_file)).to eq("foo 1.0 (new formula)")
       end
 
+      it "correctly bumps a new cask" do
+        expect(described_class.determine_bump_subject("", cask, cask_file)).to eq("food 1.0 (new cask)")
+      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 cask version" do
+        expect(described_class.determine_bump_subject(cask, cask_version, cask_file)).to eq("food 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"
@@ -100,6 +177,10 @@ describe "brew pr-pull" do
       it "correctly bumps a formula deletion" do
         expect(described_class.determine_bump_subject(formula, "", formula_file)).to eq("foo: delete")
       end
+
+      it "correctly bumps a cask deletion" do
+        expect(described_class.determine_bump_subject(cask, "", cask_file)).to eq("food: delete")
+      end
     end
   end
 end