dev-cmd: avoid uploading identical bottles.

If the `rebuild`, `root_url`, `cellar` and checksums are all identical
between an old and new bottle block: we don't need a new bottle at
all. Handle this by deleting the relevant files with
`brew bottle --merge --write` and gracefully notifying the caller of
`brew pr-upload`.

This should avoid e.g.
39340a11ea
occurring in future.

Library/Homebrew/.rubocop.yml

@@ -18,6 +18,8 @@ Metrics/AbcSize:
 Metrics/BlockLength:
   Max: 100
   Exclude:
+    # TODO: extract more of the bottling logic
+    - "dev-cmd/bottle.rb"
     - "test/**/*"
 Metrics/BlockNesting:
   Max: 5
@@ -33,8 +35,10 @@ Metrics/PerceivedComplexity:
 Metrics/MethodLength:
   Max: 260
 Metrics/ModuleLength:
-  Max: 610
+  Max: 500
   Exclude:
+    # TODO: extract more of the bottling logic
+    - "dev-cmd/bottle.rb"
     - "test/**/*"
 
 Naming/PredicateName:

Library/Homebrew/dev-cmd/bottle.rb

@@ -640,93 +640,109 @@ module Homebrew
         break if all_bottle
       end
 
-      if args.write?
+      unless args.write?
-        if all_bottle
-          all_bottle_hash = nil
-          bottle_hash["bottle"]["tags"].each do |tag, tag_hash|
-            filename = Bottle::Filename.new(
-              formula_name,
-              bottle_hash["formula"]["pkg_version"],
-              tag,
-              bottle_hash["bottle"]["rebuild"],
-            )
-
-            if all_bottle_hash.nil?
-              all_bottle_tag_hash = tag_hash.dup
-
-              all_filename = Bottle::Filename.new(
-                formula_name,
-                bottle_hash["formula"]["pkg_version"],
-                "all",
-                bottle_hash["bottle"]["rebuild"],
-              )
-
-              all_bottle_tag_hash["filename"] = all_filename.url_encode
-              all_bottle_tag_hash["local_filename"] = all_filename.to_s
-              cellar = all_bottle_tag_hash.delete("cellar")
-
-              all_bottle_formula_hash = bottle_hash.dup
-              all_bottle_formula_hash["bottle"]["cellar"] = cellar
-              all_bottle_formula_hash["bottle"]["tags"] = { all: all_bottle_tag_hash }
-
-              all_bottle_hash = { formula_name => all_bottle_formula_hash }
-
-              puts "Copying #{filename} to #{all_filename}" if args.verbose?
-              FileUtils.cp filename.to_s, all_filename.to_s
-
-              puts "Writing #{all_filename.json}" if args.verbose?
-              all_local_json_path = Pathname(all_filename.json)
-              all_local_json_path.unlink if all_local_json_path.exist?
-              all_local_json_path.write(JSON.pretty_generate(all_bottle_hash))
-            end
-
-            puts "Removing #{filename} and #{filename.json}" if args.verbose?
-            FileUtils.rm_f [filename.to_s, filename.json]
-          end
-        end
-
-        Homebrew.install_bundler_gems!
-        require "utils/ast"
-
-        path = Pathname.new((HOMEBREW_REPOSITORY/bottle_hash["formula"]["path"]).to_s)
-        formula = Formulary.factory(path)
-        formula_ast = Utils::AST::FormulaAST.new(path.read)
-        checksums = old_checksums(formula, formula_ast, bottle_hash, args: args)
-        update_or_add = checksums.nil? ? "add" : "update"
-
-        checksums&.each(&bottle.method(:sha256))
-        output = bottle_output(bottle)
-        puts output
-
-        case update_or_add
-        when "update"
-          formula_ast.replace_bottle_block(output)
-        when "add"
-          formula_ast.add_bottle_block(output)
-        end
-        path.atomic_write(formula_ast.process)
-
-        unless args.no_commit?
-          Utils::Git.set_name_email!(committer: args.committer.blank?)
-          Utils::Git.setup_gpg!
-
-          if (committer = args.committer)
-            committer = Utils.parse_author!(committer)
-            ENV["GIT_COMMITTER_NAME"] = committer[:name]
-            ENV["GIT_COMMITTER_EMAIL"] = committer[:email]
-          end
-
-          short_name = formula_name.split("/", -1).last
-          pkg_version = bottle_hash["formula"]["pkg_version"]
-
-          path.parent.cd do
-            safe_system "git", "commit", "--no-edit", "--verbose",
-                        "--message=#{short_name}: #{update_or_add} #{pkg_version} bottle.",
-                        "--", path
-          end
-        end
-      else
         puts bottle_output(bottle)
+        next
+      end
+
+      path = HOMEBREW_REPOSITORY/bottle_hash["formula"]["path"]
+      formula = Formulary.factory(path)
+      old_bottle_spec = formula.bottle_specification
+
+      no_bottle_changes = if old_bottle_spec &&
+                             bottle_hash["formula"]["pkg_version"] == formula.pkg_version.to_s &&
+                             bottle.rebuild  != old_bottle_spec.rebuild &&
+                             bottle.root_url == old_bottle_spec.root_url
+        bottle.collector.keys.all? do |tag|
+          next false if bottle.collector[tag][:cellar] != old_bottle_spec.collector[tag][:cellar]
+
+          bottle.collector[tag][:checksum].hexdigest == old_bottle_spec.collector[tag][:checksum].hexdigest
+        end
+      end
+
+      all_bottle_hash = nil
+      bottle_hash["bottle"]["tags"].each do |tag, tag_hash|
+        filename = Bottle::Filename.new(
+          formula_name,
+          bottle_hash["formula"]["pkg_version"],
+          tag,
+          bottle_hash["bottle"]["rebuild"],
+        )
+
+        if all_bottle && all_bottle_hash.nil?
+          all_bottle_tag_hash = tag_hash.dup
+
+          all_filename = Bottle::Filename.new(
+            formula_name,
+            bottle_hash["formula"]["pkg_version"],
+            "all",
+            bottle_hash["bottle"]["rebuild"],
+          )
+
+          all_bottle_tag_hash["filename"] = all_filename.url_encode
+          all_bottle_tag_hash["local_filename"] = all_filename.to_s
+          cellar = all_bottle_tag_hash.delete("cellar")
+
+          all_bottle_formula_hash = bottle_hash.dup
+          all_bottle_formula_hash["bottle"]["cellar"] = cellar
+          all_bottle_formula_hash["bottle"]["tags"] = { all: all_bottle_tag_hash }
+
+          all_bottle_hash = { formula_name => all_bottle_formula_hash }
+
+          puts "Copying #{filename} to #{all_filename}" if args.verbose?
+          FileUtils.cp filename.to_s, all_filename.to_s
+
+          puts "Writing #{all_filename.json}" if args.verbose?
+          all_local_json_path = Pathname(all_filename.json)
+          all_local_json_path.unlink if all_local_json_path.exist?
+          all_local_json_path.write(JSON.pretty_generate(all_bottle_hash))
+        end
+
+        if all_bottle || no_bottle_changes
+          puts "Removing #{filename} and #{filename.json}" if args.verbose?
+          FileUtils.rm_f [filename.to_s, filename.json]
+        end
+      end
+
+      next if no_bottle_changes
+
+      Homebrew.install_bundler_gems!
+      require "utils/ast"
+
+      formula_ast = Utils::AST::FormulaAST.new(path.read)
+      checksums = old_checksums(formula, formula_ast, bottle_hash, args: args)
+      update_or_add = checksums.nil? ? "add" : "update"
+
+      checksums&.each(&bottle.method(:sha256))
+      output = bottle_output(bottle)
+      puts output
+
+      case update_or_add
+      when "update"
+        formula_ast.replace_bottle_block(output)
+      when "add"
+        formula_ast.add_bottle_block(output)
+      end
+      path.atomic_write(formula_ast.process)
+
+      next if args.no_commit?
+
+      Utils::Git.set_name_email!(committer: args.committer.blank?)
+      Utils::Git.setup_gpg!
+
+      if (committer = args.committer)
+        committer = Utils.parse_author!(committer)
+        ENV["GIT_COMMITTER_NAME"] = committer[:name]
+        ENV["GIT_COMMITTER_EMAIL"] = committer[:email]
+      end
+
+      short_name = formula_name.split("/", -1).last
+      pkg_version = bottle_hash["formula"]["pkg_version"]
+
+      path.parent.cd do
+        safe_system "git", "commit", "--no-edit", "--verbose",
+                    "--message=#{short_name}: #{update_or_add} #{pkg_version} bottle.",
+                    "--", path
       end
     end
   end

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

@@ -84,9 +84,7 @@ module Homebrew
     end
   end
 
-  def json_files_and_bottles_hash(args)
+  def bottles_hash_from_json_files(json_files, args)
-    json_files = Dir["*.bottle.json"]
-    odie "No bottle JSON files found in the current working directory" if json_files.blank?
     puts "Reading JSON files: #{json_files.join(", ")}" if args.verbose?
 
     bottles_hash = json_files.reduce({}) do |hash, json_file|
@@ -99,13 +97,15 @@ module Homebrew
       end
     end
 
-    [json_files, bottles_hash]
+    bottles_hash
   end
 
   def pr_upload
     args = pr_upload_args.parse
 
-    json_files, bottles_hash = json_files_and_bottles_hash(args)
+    json_files = Dir["*.bottle.json"]
+    odie "No bottle JSON files found in the current working directory" if json_files.blank?
+    bottles_hash = bottles_hash_from_json_files(json_files, args)
 
     bottle_args = ["bottle", "--merge", "--write"]
     bottle_args << "--verbose" if args.verbose?
@@ -144,9 +144,15 @@ module Homebrew
 
     safe_system HOMEBREW_BREW_FILE, *bottle_args
 
+    json_files = Dir["*.bottle.json"]
+    if json_files.blank?
+      puts "No bottle JSON files after merge, no upload needed!"
+      return
+    end
+
     # Reload the JSON files (in case `brew bottle --merge` generated
     # `all: $SHA256` bottles)
-    _, bottles_hash = json_files_and_bottles_hash(args)
+    bottles_hash = bottles_hash_from_json_files(json_files, args)
 
     # Check the bottle commits did not break `brew audit`
     unless args.no_commit?