From f8d84249aaf748ba4e5699b2327623b69083737d Mon Sep 17 00:00:00 2001 From: Ben Muschol Date: Fri, 15 Feb 2019 00:31:13 -0500 Subject: [PATCH 1/5] Prioritize CLI arguments over env vars when they conflict --- Library/Homebrew/cli_parser.rb | 24 +++++++++++++++++++++--- Library/Homebrew/test/cli_parser_spec.rb | 19 +++++++++++++++++-- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/Library/Homebrew/cli_parser.rb b/Library/Homebrew/cli_parser.rb index 160071f87faef..d18811ef94e07 100644 --- a/Library/Homebrew/cli_parser.rb +++ b/Library/Homebrew/cli_parser.rb @@ -27,6 +27,7 @@ def initialize(&block) Homebrew.args.instance_eval { undef tap } @constraints = [] @conflicts = [] + @switch_sources = {} @processed_options = [] @desc_line_length = 43 @hide_from_man_page = false @@ -58,7 +59,7 @@ def switch(*names, description: nil, env: nil, required_for: nil, depends_on: ni 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, source: :env_var) if !env.nil? && !ENV["HOMEBREW_#{env.to_s.upcase}"].nil? end alias switch_option switch @@ -172,12 +173,19 @@ def hide_from_man_page! private - def enable_switch(*names) + def enable_switch(*names, source: :cli_arg) names.each do |name| + @switch_sources[option_to_name(name)] = source 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,17 @@ def check_conflicts 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 + + if violations.count - env_var_options.count == 1 + env_var_options.each do |option| + disable_switch option + end + else + raise OptionConflictError, violations.map(&method(:name_to_option)) + end end end diff --git a/Library/Homebrew/test/cli_parser_spec.rb b/Library/Homebrew/test/cli_parser_spec.rb index 975f5ed86bed7..caaa71d78f1cf 100644 --- a/Library/Homebrew/test/cli_parser_spec.rb +++ b/Library/Homebrew/test/cli_parser_spec.rb @@ -145,8 +145,8 @@ 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,21 @@ 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") + parser.parse(["--switch-b"]) + expect(Homebrew.args.switch_a?).to be_falsy + expect(Homebrew.args.switch_b?).to be true + 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 From 440fb185dd6f702bce85691c6fda4cc50876b47d Mon Sep 17 00:00:00 2001 From: Ben Muschol Date: Fri, 15 Feb 2019 01:16:07 -0500 Subject: [PATCH 2/5] Fix 1 style conflict, keep the other for consistency --- Library/Homebrew/cli_parser.rb | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/Library/Homebrew/cli_parser.rb b/Library/Homebrew/cli_parser.rb index d18811ef94e07..c8616dbc4a38c 100644 --- a/Library/Homebrew/cli_parser.rb +++ b/Library/Homebrew/cli_parser.rb @@ -237,13 +237,9 @@ def check_conflicts @switch_sources[option_to_name(option)] == :env_var end - if violations.count - env_var_options.count == 1 - env_var_options.each do |option| - disable_switch option - end - else - raise OptionConflictError, violations.map(&method(:name_to_option)) - 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 From ac005fa4cecea51f519c5de9720b3150d629ae86 Mon Sep 17 00:00:00 2001 From: Ben Muschol Date: Fri, 15 Feb 2019 11:01:03 -0500 Subject: [PATCH 3/5] rename source to from, change values --- Library/Homebrew/cli_parser.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/cli_parser.rb b/Library/Homebrew/cli_parser.rb index c8616dbc4a38c..d4e4ebb7e1a85 100644 --- a/Library/Homebrew/cli_parser.rb +++ b/Library/Homebrew/cli_parser.rb @@ -59,7 +59,7 @@ def switch(*names, description: nil, env: nil, required_for: nil, depends_on: ni set_constraints(name, required_for: required_for, depends_on: depends_on) end - enable_switch(*names, source: :env_var) 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 @@ -173,9 +173,9 @@ def hide_from_man_page! private - def enable_switch(*names, source: :cli_arg) + def enable_switch(*names, from: :arg) names.each do |name| - @switch_sources[option_to_name(name)] = source + @switch_sources[option_to_name(name)] = from Homebrew.args["#{option_to_name(name)}?"] = true end end From c745738c55e231c49c9e853449734db70ee9e300 Mon Sep 17 00:00:00 2001 From: Ben Muschol Date: Fri, 15 Feb 2019 14:15:34 -0500 Subject: [PATCH 4/5] Make from: required, fix flaky test --- Library/Homebrew/cli_parser.rb | 4 ++-- Library/Homebrew/test/cli_parser_spec.rb | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/cli_parser.rb b/Library/Homebrew/cli_parser.rb index d4e4ebb7e1a85..f35328a574ac3 100644 --- a/Library/Homebrew/cli_parser.rb +++ b/Library/Homebrew/cli_parser.rb @@ -52,7 +52,7 @@ def switch(*names, description: nil, env: nil, required_for: nil, depends_on: ni 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| @@ -173,7 +173,7 @@ def hide_from_man_page! private - def enable_switch(*names, from: :arg) + def enable_switch(*names, from:) names.each do |name| @switch_sources[option_to_name(name)] = from Homebrew.args["#{option_to_name(name)}?"] = true diff --git a/Library/Homebrew/test/cli_parser_spec.rb b/Library/Homebrew/test/cli_parser_spec.rb index caaa71d78f1cf..53504dd28755e 100644 --- a/Library/Homebrew/test/cli_parser_spec.rb +++ b/Library/Homebrew/test/cli_parser_spec.rb @@ -181,6 +181,7 @@ 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.switch_b?).to be true From c7831a457d738d9723231cae77b30b1b22b39dbc Mon Sep 17 00:00:00 2001 From: Ben Muschol Date: Sat, 16 Feb 2019 07:39:48 -0500 Subject: [PATCH 5/5] Get brew style to pass --- Library/Homebrew/test/cli_parser_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/test/cli_parser_spec.rb b/Library/Homebrew/test/cli_parser_spec.rb index 53504dd28755e..c5c1f257642c8 100644 --- a/Library/Homebrew/test/cli_parser_spec.rb +++ b/Library/Homebrew/test/cli_parser_spec.rb @@ -183,8 +183,8 @@ 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.switch_b?).to be true + 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