rubocop: separate args for shell commands

Use `system "foo", "bar"` instead of `system "foo bar"`. Also applies to
`Utils.popen_read` and `Utils.popen_write` commands. RuboCop can
automatically fix these problems.

Library/Homebrew/dev-cmd/audit.rb

@@ -844,15 +844,6 @@ module Homebrew
       # TODO: check could be in RuboCop
       problem "`env :userpaths` in formulae is deprecated" if line.include?("env :userpaths")
 
-      if line =~ /system ((["'])[^"' ]*(?:\s[^"' ]*)+\2)/
-        bad_system = Regexp.last_match(1)
-        unless %w[| < > & ; *].any? { |c| bad_system.include? c }
-          good_system = bad_system.gsub(" ", "\", \"")
-          # TODO: check could be in RuboCop
-          problem "Use `system #{good_system}` instead of `system #{bad_system}` "
-        end
-      end
-
       # TODO: check could be in RuboCop
       problem "`#{Regexp.last_match(1)}` is now unnecessary" if line =~ /(require ["']formula["'])/
 

Library/Homebrew/rubocops/lines.rb

@@ -252,6 +252,55 @@ module RuboCop
         end
       end
 
+      class ShellCommands < FormulaCop
+        def audit_formula(_node, _class_node, _parent_class_node, body_node)
+          # Match shell commands separated by spaces in the same string
+          shell_cmd_with_spaces_regex = /[^"' ]*(?:\s[^"' ]*)+/
+
+          popen_commands = [
+            :popen_read,
+            :safe_popen_read,
+            :popen_write,
+            :safe_popen_write,
+          ]
+
+          shell_metacharacters = %w[> >> < << | ; & && || *]
+
+          find_every_method_call_by_name(body_node, :system).each do |method|
+            # Continue if a shell metacharacter is present
+            next if shell_metacharacters.any? { |meta| string_content(parameters(method).first).include?(meta) }
+
+            next unless match = regex_match_group(parameters(method).first, shell_cmd_with_spaces_regex)
+
+            good_args = match[0].gsub(" ", "\", \"")
+            offending_node(parameters(method).first)
+            problem "Separate `system` commands into `\"#{good_args}\"`"
+          end
+
+          popen_commands.each do |command|
+            find_instance_method_call(body_node, "Utils", command) do |method|
+              index = parameters(method).first.hash_type? ? 1 : 0
+
+              # Continue if a shell metacharacter is present
+              next if shell_metacharacters.any? { |meta| string_content(parameters(method)[index]).include?(meta) }
+
+              next unless match = regex_match_group(parameters(method)[index], shell_cmd_with_spaces_regex)
+
+              good_args = match[0].gsub(" ", "\", \"")
+              offending_node(parameters(method)[index])
+              problem "Separate `Utils.#{command}` commands into `\"#{good_args}\"`"
+            end
+          end
+        end
+
+        def autocorrect(node)
+          lambda do |corrector|
+            good_args = node.source.gsub(" ", "\", \"")
+            corrector.replace(node.source_range, good_args)
+          end
+        end
+      end
+
       class Miscellaneous < FormulaCop
         def audit_formula(_node, _class_node, _parent_class_node, body_node)
           # FileUtils is included in Formula

Library/Homebrew/test/rubocops/lines_spec.rb

@@ -574,6 +574,150 @@ describe RuboCop::Cop::FormulaAudit::ShellVariables do
   end
 end
 
+describe RuboCop::Cop::FormulaAudit::ShellCommands do
+  subject(:cop) { described_class.new }
+
+  context "When auditing shell commands" do
+    it "system arguments should be separated" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            system "foo bar"
+                   ^^^^^^^^^ Separate `system` commands into `\"foo\", \"bar\"`
+          end
+        end
+      RUBY
+    end
+
+    it "system arguments with string interpolation should be separated" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            system "\#{bin}/foo bar"
+                   ^^^^^^^^^^^^^^^^ Separate `system` commands into `\"\#{bin}/foo\", \"bar\"`
+          end
+        end
+      RUBY
+    end
+
+    it "system arguments with metacharacters should not be separated" do
+      expect_no_offenses(<<~RUBY)
+        class Foo < Formula
+          def install
+            system "foo bar > baz"
+          end
+        end
+      RUBY
+    end
+
+    it "only the first system argument should be separated" do
+      expect_no_offenses(<<~RUBY)
+        class Foo < Formula
+          def install
+            system "foo", "bar baz"
+          end
+        end
+      RUBY
+    end
+
+    it "Utils.popen arguments should not be separated" do
+      expect_no_offenses(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.popen("foo bar")
+          end
+        end
+      RUBY
+    end
+
+    it "Utils.popen_read arguments should be separated" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.popen_read("foo bar")
+                             ^^^^^^^^^ Separate `Utils.popen_read` commands into `\"foo\", \"bar\"`
+          end
+        end
+      RUBY
+    end
+
+    it "Utils.safe_popen_read arguments should be separated" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.safe_popen_read("foo bar")
+                                  ^^^^^^^^^ Separate `Utils.safe_popen_read` commands into `\"foo\", \"bar\"`
+          end
+        end
+      RUBY
+    end
+
+    it "Utils.popen_write arguments should be separated" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.popen_write("foo bar")
+                              ^^^^^^^^^ Separate `Utils.popen_write` commands into `\"foo\", \"bar\"`
+          end
+        end
+      RUBY
+    end
+
+    it "Utils.safe_popen_write arguments should be separated" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.safe_popen_write("foo bar")
+                                   ^^^^^^^^^ Separate `Utils.safe_popen_write` commands into `\"foo\", \"bar\"`
+          end
+        end
+      RUBY
+    end
+
+    it "Utils.popen_read arguments with string interpolation should be separated" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.popen_read("\#{bin}/foo bar")
+                             ^^^^^^^^^^^^^^^^ Separate `Utils.popen_read` commands into `\"\#{bin}/foo\", \"bar\"`
+          end
+        end
+      RUBY
+    end
+
+    it "Utils.popen_read arguments with metacharacters should not be separated" do
+      expect_no_offenses(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.popen_read("foo bar > baz")
+          end
+        end
+      RUBY
+    end
+
+    it "only the first Utils.popen_read argument should be separated" do
+      expect_no_offenses(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.popen_read("foo", "bar baz")
+          end
+        end
+      RUBY
+    end
+
+    it "Utils.popen_read arguments should be separated following a shell variable" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.popen_read({ "SHELL" => "bash"}, "foo bar")
+                                                   ^^^^^^^^^ Separate `Utils.popen_read` commands into `\"foo\", \"bar\"`
+          end
+        end
+      RUBY
+    end
+  end
+end
+
 describe RuboCop::Cop::FormulaAudit::Miscellaneous do
   subject(:cop) { described_class.new }