Refactor CLI to remove unless args_parsed

Refactor the CLI::Args module so it doesn't have different paths to
check arguments depending on whether the arguments have been parsed or
not. Instead, set the values we need from the global ARGV at
first, global initialisation time where they will be thrown away when
the actual arguments are parsed.

To do this some other general refactoring was needed:
- more methods made private when possible
- e.g. `HEAD?` used consistently instead of `head` before arguments
  are parsed.
- formula options are only parsed after named arguments are extracted

Library/Homebrew/cli/args.rb

@@ -5,56 +5,44 @@ require "ostruct"
 module Homebrew
   module CLI
     class Args < OpenStruct
-      attr_reader :processed_options, :args_parsed
       # undefine tap to allow --tap argument
       undef tap
 
-      def initialize
+      def initialize(argv = ARGV.dup.freeze, set_default_args: false)
-        super
+        super()
 
-        self[:remaining] = []
-        self[:argv] = ARGV.dup.freeze
-
-        @args_parsed = false
         @processed_options = []
+
+        # Set values needed before Parser#parse has been run.
+        return unless set_default_args
+
+        self[:build_from_source?] = argv.include?("--build-from-source") || argv.include?("-s")
+        self[:build_bottle?] = argv.include?("--build-bottle")
+        self[:force_bottle?] = argv.include?("--force-bottle")
+        self[:HEAD?] = argv.include?("--HEAD")
+        self[:devel?] = argv.include?("--devel")
+        self[:universal?] = argv.include?("--universal")
+        self[:named_args] = argv.reject { |arg| arg.start_with?("-") }
+      end
+
+      def freeze_named_args!(named_args)
+        self[:named_args] = named_args
+        self[:named_args].freeze
       end
 
       def freeze_processed_options!(processed_options)
         @processed_options += processed_options
         @processed_options.freeze
-        @args_parsed = true
-      end
-
-      def option_to_name(option)
-        option.sub(/\A--?/, "")
-              .tr("-", "_")
-      end
-
-      def cli_args
-        return @cli_args if @cli_args
-
-        @cli_args = []
-        processed_options.each do |short, long|
-          option = long || short
-          switch = "#{option_to_name(option)}?".to_sym
-          flag = option_to_name(option).to_sym
-          if @table[switch] == true || @table[flag] == true
-            @cli_args << option
-          elsif @table[flag].instance_of? String
-            @cli_args << option + "=" + @table[flag]
-          elsif @table[flag].instance_of? Array
-            @cli_args << option + "=" + @table[flag].join(",")
-          end
-        end
-        @cli_args
       end
 
       def options_only
         @options_only ||= cli_args.select { |arg| arg.start_with?("-") }
+                                  .freeze
       end
 
       def flags_only
         @flags_only ||= cli_args.select { |arg| arg.start_with?("--") }
+                                .freeze
       end
 
       def passthrough
@@ -62,7 +50,7 @@ module Homebrew
       end
 
       def named
-        remaining
+        named_args || []
       end
 
       def no_named?
@@ -74,16 +62,17 @@ module Homebrew
       def collect_build_args
         build_flags = []
 
-        build_flags << "--HEAD" if head
+        build_flags << "--HEAD" if HEAD?
-        build_flags << "--universal" if build_universal
+        build_flags << "--universal" if build_universal?
-        build_flags << "--build-bottle" if build_bottle
+        build_flags << "--build-bottle" if build_bottle?
-        build_flags << "--build-from-source" if build_from_source
+        build_flags << "--build-from-source" if build_from_source?
 
         build_flags
       end
 
       def formulae
         require "formula"
+
         @formulae ||= (downcased_unique_named - casks).map do |name|
           if name.include?("/") || File.exist?(name)
             Formulary.factory(name, spec)
@@ -91,29 +80,35 @@ module Homebrew
             Formulary.find_with_priority(name, spec)
           end
         end.uniq(&:name)
+                                                      .freeze
       end
 
       def resolved_formulae
         require "formula"
+
         @resolved_formulae ||= (downcased_unique_named - casks).map do |name|
           Formulary.resolve(name, spec: spec(nil))
         end.uniq(&:name)
+                                                               .freeze
       end
 
       def formulae_paths
         @formulae_paths ||= (downcased_unique_named - casks).map do |name|
           Formulary.path(name)
         end.uniq
+                                                            .freeze
       end
 
       def casks
-        @casks ||= downcased_unique_named.grep HOMEBREW_CASK_TAP_CASK_REGEX
+        @casks ||= downcased_unique_named.grep(HOMEBREW_CASK_TAP_CASK_REGEX)
+                                         .freeze
       end
 
       def kegs
         require "keg"
         require "formula"
         require "missing_formula"
+
         @kegs ||= downcased_unique_named.map do |name|
           raise UsageError if name.empty?
 
@@ -158,7 +153,7 @@ module Homebrew
               Please delete (with rm -rf!) all but one and then try again.
             EOS
           end
-        end
+        end.freeze
       end
 
       def build_stable?
@@ -168,39 +163,40 @@ module Homebrew
       # Whether a given formula should be built from source during the current
       # installation run.
       def build_formula_from_source?(f)
-        return false if !build_from_source && !build_bottle
+        return false if !build_from_source? && !build_bottle?
 
         formulae.any? { |args_f| args_f.full_name == f.full_name }
       end
 
-      def build_from_source
-        return argv.include?("--build-from-source") || argv.include?("-s") unless args_parsed
-
-        build_from_source? || s?
-      end
-
-      def build_bottle
-        return argv.include?("--build-bottle") unless args_parsed
-
-        build_bottle?
-      end
-
-      def force_bottle
-        return argv.include?("--force-bottle") unless args_parsed
-
-        force_bottle?
-      end
-
       private
 
+      def option_to_name(option)
+        option.sub(/\A--?/, "")
+              .tr("-", "_")
+      end
+
+      def cli_args
+        return @cli_args if @cli_args
+
+        @cli_args = []
+        @processed_options.each do |short, long|
+          option = long || short
+          switch = "#{option_to_name(option)}?".to_sym
+          flag = option_to_name(option).to_sym
+          if @table[switch] == true || @table[flag] == true
+            @cli_args << option
+          elsif @table[flag].instance_of? String
+            @cli_args << option + "=" + @table[flag]
+          elsif @table[flag].instance_of? Array
+            @cli_args << option + "=" + @table[flag].join(",")
+          end
+        end
+        @cli_args.freeze
+      end
+
       def downcased_unique_named
         # Only lowercase names, not paths, bottle filenames or URLs
-        arguments = if args_parsed
+        named.map do |arg|
-          named
-        else
-          argv.reject { |arg| arg.start_with?("-") }
-        end
-        arguments.map do |arg|
           if arg.include?("/") || arg.end_with?(".tar.gz") || File.exist?(arg)
             arg
           else
@@ -209,28 +205,10 @@ module Homebrew
         end.uniq
       end
 
-      def head
-        return argv.include?("--HEAD") unless args_parsed
-
-        HEAD?
-      end
-
-      def devel
-        return argv.include?("--devel") unless args_parsed
-
-        devel?
-      end
-
-      def build_universal
-        return argv.include?("--universal") unless args_parsed
-
-        universal?
-      end
-
       def spec(default = :stable)
-        if head
+        if HEAD?
           :head
-        elsif devel
+        elsif devel?
           :devel
         else
           default

Library/Homebrew/cli/parser.rb

@@ -12,8 +12,8 @@ module Homebrew
     class Parser
       attr_reader :processed_options, :hide_from_man_page
 
-      def self.parse(args = ARGV, allow_no_named_args: false, &block)
+      def self.parse(argv = ARGV.dup.freeze, allow_no_named_args: false, &block)
-        new(args, &block).parse(args, allow_no_named_args: allow_no_named_args)
+        new(argv, &block).parse(allow_no_named_args: allow_no_named_args)
       end
 
       def self.from_cmd_path(cmd_path)
@@ -37,9 +37,10 @@ module Homebrew
         }
       end
 
-      def initialize(&block)
+      def initialize(argv = ARGV.dup.freeze, &block)
         @parser = OptionParser.new
-        @args = Homebrew::CLI::Args.new
+        @argv = argv
+        @args = Homebrew::CLI::Args.new(@argv)
 
         @constraints = []
         @conflicts = []
@@ -152,7 +153,7 @@ module Homebrew
         @parser.to_s
       end
 
-      def parse(argv = ARGV, allow_no_named_args: false)
+      def parse(argv = @argv, allow_no_named_args: false)
         raise "Arguments were already parsed!" if @args_parsed
 
         begin
@@ -161,12 +162,14 @@ module Homebrew
           $stderr.puts generate_help_text
           raise e
         end
+
         check_constraint_violations
         check_named_args(named_args, allow_no_named_args: allow_no_named_args)
-        @args[:remaining] = named_args
+        @args.freeze_named_args!(named_args)
+        parse_formula_options
         @args.freeze_processed_options!(@processed_options)
         Homebrew.args = @args
-        argv.freeze
+
         @args_parsed = true
         @parser
       end
@@ -186,21 +189,7 @@ module Homebrew
       end
 
       def formula_options
-        @args.formulae.each do |f|
+        @parse_formula_options = true
-          next if f.options.empty?
-
-          f.options.each do |o|
-            name = o.flag
-            description = "`#{f.name}`: #{o.description}"
-            if name.end_with? "="
-              flag   name, description: description
-            else
-              switch name, description: description
-            end
-          end
-        end
-      rescue FormulaUnavailableError
-        []
       end
 
       def max_named(count)
@@ -239,6 +228,26 @@ module Homebrew
 
       private
 
+      def parse_formula_options
+        return unless @parse_formula_options
+
+        @args.formulae.each do |f|
+          next if f.options.empty?
+
+          f.options.each do |o|
+            name = o.flag
+            description = "`#{f.name}`: #{o.description}"
+            if name.end_with? "="
+              flag   name, description: description
+            else
+              switch name, description: description
+            end
+          end
+        end
+      rescue FormulaUnavailableError
+        []
+      end
+
       def enable_switch(*names, from:)
         names.each do |name|
           @switch_sources[option_to_name(name)] = from

Library/Homebrew/cmd/install.rb

@@ -152,7 +152,7 @@ module Homebrew
       end
 
       # --HEAD, fail with no head defined
-      raise "No head is defined for #{f.full_name}" if args.head? && f.head.nil?
+      raise "No head is defined for #{f.full_name}" if args.HEAD? && f.head.nil?
 
       # --devel, fail with no devel defined
       raise "No devel block is defined for #{f.full_name}" if args.devel? && f.devel.nil?

Library/Homebrew/dev-cmd/irb.rb

@@ -18,7 +18,8 @@ module Homebrew
   module_function
 
   def irb_args
-    Homebrew::CLI::Parser.new do
+    # work around IRB modifying ARGV.
+    Homebrew::CLI::Parser.new(ARGV.dup) do
       usage_banner <<~EOS
         `irb` [<options>]
 
@@ -33,8 +34,7 @@ module Homebrew
   end
 
   def irb
-    # work around IRB modifying ARGV.
+    irb_args.parse
-    irb_args.parse(ARGV.dup)
 
     if args.examples?
       puts "'v8'.f # => instance of the v8 formula"

Library/Homebrew/extend/ENV/shared.rb

@@ -268,7 +268,7 @@ module SharedEnvExtension
 
   # @private
   def effective_arch
-    if Homebrew.args.build_bottle && ARGV.bottle_arch
+    if Homebrew.args.build_bottle? && ARGV.bottle_arch
       ARGV.bottle_arch
     else
       Hardware.oldest_cpu

Library/Homebrew/fetch.rb

@@ -5,7 +5,7 @@ module Homebrew
     module_function
 
     def fetch_bottle?(f)
-      return true if Homebrew.args.force_bottle && f.bottle
+      return true if Homebrew.args.force_bottle? && f.bottle
       return false unless f.bottle && f.pour_bottle?
       return false if Homebrew.args.build_formula_from_source?(f)
       return false unless f.bottle.compatible_cellar?

Library/Homebrew/formula_installer.rb

@@ -49,9 +49,9 @@ class FormulaInstaller
     @show_header = false
     @ignore_deps = false
     @only_deps = false
-    @build_from_source = Homebrew.args.build_from_source
+    @build_from_source = Homebrew.args.build_from_source?
     @build_bottle = false
-    @force_bottle = Homebrew.args.force_bottle
+    @force_bottle = Homebrew.args.force_bottle?
     @include_test = ARGV.include?("--include-test")
     @interactive = false
     @git = false

Library/Homebrew/global.rb

@@ -85,7 +85,7 @@ module Homebrew
     end
 
     def args
-      @args ||= CLI::Args.new
+      @args ||= CLI::Args.new(set_default_args: true)
     end
 
     def messages

Library/Homebrew/reinstall.rb

@@ -25,7 +25,7 @@ module Homebrew
 
     fi = FormulaInstaller.new(f)
     fi.options              = options
-    fi.build_bottle         = Homebrew.args.build_bottle
+    fi.build_bottle         = Homebrew.args.build_bottle?
     fi.interactive          = Homebrew.args.interactive?
     fi.git                  = Homebrew.args.git?
     fi.link_keg           ||= keg_was_linked if keg_had_linked_opt

Library/Homebrew/software_spec.rb

@@ -95,7 +95,7 @@ class SoftwareSpec
 
   def bottled?
     bottle_specification.tag?(Utils::Bottles.tag) && \
-      (bottle_specification.compatible_cellar? || Homebrew.args.force_bottle)
+      (bottle_specification.compatible_cellar? || Homebrew.args.force_bottle?)
   end
 
   def bottle(disable_type = nil, disable_reason = nil, &block)