Merge pull request #5738 from BenMusch/handle-conflicts

Prioritize CLI arguments over env vars when they conflict

Library/Homebrew/cli_parser.rb

@@ -27,6 +27,7 @@ module Homebrew
         Homebrew.args.instance_eval { undef tap }
         @constraints = []
         @conflicts = []
+        @switch_sources = {}
         @processed_options = []
         @desc_line_length = 43
         @hide_from_man_page = false
@@ -51,14 +52,14 @@ module Homebrew
         end
         process_option(*names, description)
         @parser.on(*names, *wrap_option_desc(description)) do
-          enable_switch(*names)
+          enable_switch(*names, from: :args)
         end
 
         names.each do |name|
           set_constraints(name, required_for: required_for, depends_on: depends_on)
         end
 
-        enable_switch(*names) if !env.nil? && !ENV["HOMEBREW_#{env.to_s.upcase}"].nil?
+        enable_switch(*names, from: :env) if !env.nil? && !ENV["HOMEBREW_#{env.to_s.upcase}"].nil?
       end
       alias switch_option switch
 
@@ -172,12 +173,19 @@ module Homebrew
 
       private
 
-      def enable_switch(*names)
+      def enable_switch(*names, from:)
         names.each do |name|
+          @switch_sources[option_to_name(name)] = from
           Homebrew.args["#{option_to_name(name)}?"] = true
         end
       end
 
+      def disable_switch(*names)
+        names.each do |name|
+          Homebrew.args.delete_field("#{option_to_name(name)}?")
+        end
+      end
+
       # These are common/global switches accessible throughout Homebrew
       def common_switch(name)
         Homebrew::CLI::Parser.global_options.fetch(name, name)
@@ -225,7 +233,13 @@ module Homebrew
 
           next if violations.count < 2
 
-          raise OptionConflictError, violations.map(&method(:name_to_option))
+          env_var_options = violations.select do |option|
+            @switch_sources[option_to_name(option)] == :env_var
+          end
+
+          select_cli_arg = violations.count - env_var_options.count == 1
+          raise OptionConflictError, violations.map(&method(:name_to_option)) unless select_cli_arg
+          env_var_options.each(&method(:disable_switch))
         end
       end
 

Library/Homebrew/test/cli_parser_spec.rb

@@ -145,8 +145,8 @@ describe Homebrew::CLI::Parser do
   describe "test constraints for switch options" do
     subject(:parser) {
       described_class.new do
-        switch      "-a", "--switch-a"
-        switch      "-b", "--switch-b"
+        switch      "-a", "--switch-a", env: "switch_a"
+        switch      "-b", "--switch-b", env: "switch_b"
         switch      "--switch-c", required_for: "--switch-a"
         switch      "--switch-d", depends_on: "--switch-b"
 
@@ -177,6 +177,22 @@ describe Homebrew::CLI::Parser do
       parser.parse(["--switch-b"])
       expect(Homebrew.args.switch_b?).to be true
     end
+
+    it "prioritizes cli arguments over env vars when they conflict" do
+      allow(ENV).to receive(:[]).with("HOMEBREW_SWITCH_A").and_return("1")
+      allow(ENV).to receive(:[]).with("HOMEBREW_SWITCH_B").and_return("0")
+      allow(ENV).to receive(:[])
+      parser.parse(["--switch-b"])
+      expect(Homebrew.args.switch_a).to be_falsy
+      expect(Homebrew.args).to be_switch_b
+    end
+
+    it "raises an exception on constraint violation when both are env vars" do
+      allow(ENV).to receive(:[]).with("HOMEBREW_SWITCH_A").and_return("1")
+      allow(ENV).to receive(:[]).with("HOMEBREW_SWITCH_B").and_return("1")
+      allow(ENV).to receive(:[])
+      expect { parser.parse(["--switch-a", "--switch-b"]) }.to raise_error(Homebrew::CLI::OptionConflictError)
+    end
   end
 
   describe "test immutability of args" do