Process SystemCommand arguments in list format

Previously, SystemCommand commands and arguments were processed as
strings which caused problems during shell interpretation if the
arguments were not escaped properly.  Now all commands are expressed
as arrays and no longer require their arguments to be escaped.
Additionally, stderr and stdout could have been interleaved in the
past and now they are always separated.

lib/cask/artifact/pkg.rb

@@ -84,9 +84,9 @@ class Cask::Artifact::Pkg < Cask::Artifact::Base
     if uninstall_options.key? :quit
       [*uninstall_options[:quit]].each do |id|
         ohai "Quitting application ID #{id}"
-        num_running = @command.run!('/usr/bin/osascript', :args => ['-e', "tell application \"System Events\" to count processes whose bundle identifier is \"#{id}\""], :sudo => true).to_i
+        num_running = @command.run!('/usr/bin/osascript', :args => ['-e', %Q{tell application "System Events" to count processes whose bundle identifier is "#{id}"}], :sudo => true).to_i
         if num_running > 0
-          @command.run!('/usr/bin/osascript', :args => ['-e', "tell application id \"#{id}\" to quit"], :sudo => true)
+          @command.run!('/usr/bin/osascript', :args => ['-e', %Q{tell application id "#{id}" to quit}], :sudo => true)
         end
       end
     end

lib/cask/pkg.rb

@@ -1,6 +1,6 @@
 class Cask::Pkg
   def self.all_matching(regexp, command)
-    command.run(%Q(/usr/sbin/pkgutil --pkgs="#{regexp}")).split("\n").map do |package_id|
+    command.run('/usr/sbin/pkgutil', :args => [%Q{--pkgs=#{regexp}}]).split("\n").map do |package_id|
       new(package_id.chomp, command)
     end
   end

lib/cask/system_command.rb

@@ -1,14 +1,21 @@
+require 'open3'
+
 class Cask::SystemCommand
-  def self.run(command, options={})
-    command = _process_options(command, options)
-    odebug "Executing: #{command}"
+  def self.run(executable, options={})
+    command = _process_options(executable, options)
+    odebug "Executing: #{command.inspect}"
     output = ''
-    IO.popen(command, 'r+') do |pipe|
+    Open3.popen3(*command) do |stdin, stdout, stderr|
       if options[:input]
-        options[:input].each { |line| pipe.puts line }
+        options[:input].each { |line| stdin.puts line }
       end
-      pipe.close_write
-      while line = pipe.gets
+      stdin.close_write
+      while line = stdout.gets
+        output << line
+        ohai line.chomp if options[:print]
+      end
+      while line = stderr.gets
+        next if options[:stderr] == :silence
         output << line
         ohai line.chomp if options[:print]
       end
@@ -25,33 +32,20 @@ class Cask::SystemCommand
     run(command, options.merge(:must_succeed => true))
   end
 
-  def self._process_options(command, options)
+  def self._process_options(executable, options)
+    command = [executable]
     if options[:sudo]
-      command = "/usr/bin/sudo -E -- #{_quote(command)}"
+      command.unshift('/usr/bin/sudo', '-E', '--')
     end
-    if options[:args]
-      command = "#{command} #{options[:args].map { |arg| _quote(arg) }.join(' ')}"
-    end
-    case options[:stderr]
-    when :silence then
-      command = "#{command} 2>/dev/null"
-    when :merge, nil then
-      command = "#{command} 2>&1"
+    if ! options[:args].empty?
+      command.concat options[:args]
     end
     command
   end
 
   def self._assert_success(status, command, output)
     unless status.success?
-      raise CaskCommandFailedError.new(command, output)
-    end
-  end
-
-  def self._quote(string)
-    if %r{^(['"]).*\1$}.match(string)
-      string
-    else
-      %Q('#{string}')
+      raise CaskCommandFailedError.new(command.inspect, output)
     end
   end
 
@@ -62,7 +56,7 @@ class Cask::SystemCommand
       raise CaskError.new(<<-ERRMSG)
 Error parsing plist output from command.
   command was:
-  #{command}
+  #{command.inspect}
   output we attempted to parse:
   #{output}
         ERRMSG

test/cask/artifact/pkg_test.rb

@@ -12,14 +12,11 @@ describe Cask::Artifact::Pkg do
     it 'runs the system installer on the specified pkgs' do
       pkg = Cask::Artifact::Pkg.new(@cask, Cask::FakeSystemCommand)
 
-      expected_command = "/usr/bin/sudo -E -- '/usr/sbin/installer' '-pkg' '#{@cask.destination_path/'MyFancyPkg'/'Fancy.pkg'}' '-target' '/' 2>&1"
-      Cask::FakeSystemCommand.stubs_command(expected_command)
+      Cask::FakeSystemCommand.expects_command(['/usr/bin/sudo', '-E', '--', '/usr/sbin/installer', '-pkg', @cask.destination_path/'MyFancyPkg'/'Fancy.pkg', '-target', '/'])
 
       shutup do
         pkg.install
       end
-
-      Cask::FakeSystemCommand.system_calls[expected_command].must_equal 1
     end
   end
 
@@ -27,17 +24,14 @@ describe Cask::Artifact::Pkg do
     it 'runs the specified uninstaller for the cask' do
       pkg = Cask::Artifact::Pkg.new(@cask, Cask::FakeSystemCommand)
 
-      Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/usr/bin/osascript' '-e' 'tell application "System Events" to count processes whose bundle identifier is "my.fancy.package.app"' 2>&1), '1')
-      Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/usr/bin/osascript' '-e' 'tell application id "my.fancy.package.app" to quit' 2>&1))
+      Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/usr/bin/osascript', '-e', 'tell application "System Events" to count processes whose bundle identifier is "my.fancy.package.app"'], '1')
+      Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/usr/bin/osascript', '-e', 'tell application id "my.fancy.package.app" to quit'])
 
-      expected_command = "/usr/bin/sudo -E -- '#{@cask.destination_path/'MyFancyPkg'/'FancyUninstaller.tool'}' '--please' 2>&1"
-      Cask::FakeSystemCommand.stubs_command(expected_command)
+      Cask::FakeSystemCommand.expects_command(['/usr/bin/sudo', '-E', '--', @cask.destination_path/'MyFancyPkg'/'FancyUninstaller.tool', '--please'])
 
       shutup do
         pkg.uninstall
       end
-
-      Cask::FakeSystemCommand.system_calls[expected_command].must_equal 1
     end
 
     it 'can uninstall using pkgutil, launchctl, and file lists' do
@@ -45,7 +39,7 @@ describe Cask::Artifact::Pkg do
       pkg = Cask::Artifact::Pkg.new(cask, Cask::FakeSystemCommand)
 
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/sbin/pkgutil --pkgs="my.fancy.package.*" 2>&1),
+        ['/usr/sbin/pkgutil', '--pkgs=my.fancy.package.*'],
         [
           'my.fancy.package.main',
           'my.fancy.package.agent',
@@ -53,14 +47,14 @@ describe Cask::Artifact::Pkg do
       )
 
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/sbin/pkgutil '--only-files' '--files' 'my.fancy.package.main' 2>&1),
+        ['/usr/sbin/pkgutil', '--only-files', '--files', 'my.fancy.package.main'],
         [
           'fancy/bin/fancy.exe',
           'fancy/var/fancy.data',
         ].join("\n")
       )
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/sbin/pkgutil '--only-dirs' '--files' 'my.fancy.package.main' 2>&1),
+        ['/usr/sbin/pkgutil', '--only-dirs', '--files', 'my.fancy.package.main'],
         [
           'fancy',
           'fancy/bin',
@@ -68,7 +62,7 @@ describe Cask::Artifact::Pkg do
         ].join("\n")
       )
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/sbin/pkgutil '--pkg-info-plist' 'my.fancy.package.main' 2>&1),
+        ['/usr/sbin/pkgutil', '--pkg-info-plist', 'my.fancy.package.main'],
         <<-PLIST
 <?xml version="1.0" encoding="UTF-8"?>
 <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
@@ -84,11 +78,11 @@ describe Cask::Artifact::Pkg do
       )
 
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/bin/launchctl 'list' '-x' 'my.fancy.package.service' 2>&1),
+        ['/bin/launchctl', 'list', '-x', 'my.fancy.package.service'],
         "launchctl list returned unknown response\n"
       )
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/bin/sudo -E -- '/bin/launchctl' 'list' '-x' 'my.fancy.package.service' 2>&1),
+        ['/usr/bin/sudo', '-E', '--', '/bin/launchctl', 'list', '-x', 'my.fancy.package.service'],
         <<-PLIST
 <?xml version="1.0" encoding="UTF-8"?>
 <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
@@ -112,14 +106,15 @@ describe Cask::Artifact::Pkg do
 </plist>
         PLIST
       )
-      Cask::FakeSystemCommand.expects_command(%Q(/usr/bin/sudo -E -- '/bin/launchctl' 'remove' '--' 'my.fancy.package.service' 2>&1))
 
-      Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/usr/sbin/kextstat' '-l' '-b' 'my.fancy.package.kernelextension' 2>&1), 'loaded')
-      Cask::FakeSystemCommand.expects_command(%Q(/usr/bin/sudo -E -- '/sbin/kextunload' '-b' '--' 'my.fancy.package.kernelextension' 2>&1))
-      Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/usr/sbin/pkgutil' '--forget' 'my.fancy.package.main' 2>&1))
+      Cask::FakeSystemCommand.expects_command(['/usr/bin/sudo', '-E', '--', '/bin/launchctl', 'remove', '--', 'my.fancy.package.service'])
+
+      Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/usr/sbin/kextstat', '-l', '-b', 'my.fancy.package.kernelextension'], 'loaded')
+      Cask::FakeSystemCommand.expects_command(['/usr/bin/sudo', '-E', '--', '/sbin/kextunload', '-b', '--', 'my.fancy.package.kernelextension'])
+      Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/usr/sbin/pkgutil', '--forget', 'my.fancy.package.main'])
 
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/sbin/pkgutil '--only-files' '--files' 'my.fancy.package.agent' 2>&1),
+        ['/usr/sbin/pkgutil', '--only-files', '--files', 'my.fancy.package.agent'],
         [
           'fancy/agent/fancy-agent.exe',
           'fancy/agent/fancy-agent.pid',
@@ -127,14 +122,14 @@ describe Cask::Artifact::Pkg do
         ].join("\n")
       )
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/sbin/pkgutil '--only-dirs' '--files' 'my.fancy.package.agent' 2>&1),
+        ['/usr/sbin/pkgutil', '--only-dirs', '--files', 'my.fancy.package.agent'],
         [
           'fancy',
           'fancy/agent',
         ].join("\n")
       )
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/sbin/pkgutil '--pkg-info-plist' 'my.fancy.package.agent' 2>&1),
+        ['/usr/sbin/pkgutil', '--pkg-info-plist', 'my.fancy.package.agent'],
         <<-PLIST
 <?xml version="1.0" encoding="UTF-8"?>
 <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
@@ -155,13 +150,18 @@ describe Cask::Artifact::Pkg do
         /tmp/fancy/bin
         /tmp/fancy/var
       ].each do |dir|
-        Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/bin/chmod' '--' '777' '#{dir}' 2>&1))
+        Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/bin/chmod', '--', '777', '#{dir}'])
       end
 
-      Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/usr/sbin/pkgutil' '--forget' 'my.fancy.package.agent' 2>&1))
+      Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/usr/sbin/pkgutil', '--forget', 'my.fancy.package.agent'])
 
-      Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/bin/rm' '-f' '--' '/tmp/fancy/bin/fancy.exe' '/tmp/fancy/var/fancy.data' 2>&1))
-      Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/bin/rm' '-f' '--' '/tmp/fancy/agent/fancy-agent.exe' '/tmp/fancy/agent/fancy-agent.pid' '/tmp/fancy/agent/fancy-agent.log' 2>&1))
+      Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/bin/rm', '-f', '--',
+                                             Pathname.new('/tmp/fancy/bin/fancy.exe'),
+                                             Pathname.new('/tmp/fancy/var/fancy.data')])
+      Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/bin/rm', '-f', '--',
+                                             Pathname.new('/tmp/fancy/agent/fancy-agent.exe'),
+                                             Pathname.new('/tmp/fancy/agent/fancy-agent.pid'),
+                                             Pathname.new('/tmp/fancy/agent/fancy-agent.log')])
 
       # No assertions after call since all assertions are implicit from the interactions setup above.
       # TODO: verify rmdir commands (requires setting up actual file tree or faking out .exists?

test/cask/cli/alfred_test.rb

@@ -1,7 +1,7 @@
 require 'test_helper'
 
 def fake_alfred_preference(key, response)
-  Cask::FakeSystemCommand.stubs_command("/usr/bin/defaults 'read' 'com.runningwithcrayons.Alfred-Preferences' '#{key}' 2>&1", response)
+  Cask::FakeSystemCommand.stubs_command(['/usr/bin/defaults', 'read', 'com.runningwithcrayons.Alfred-Preferences', key], response)
 end
 
 def fake_alfred_installed(installed=true)
@@ -78,7 +78,7 @@ describe Cask::CLI::Alfred do
       SCOPE_RESPONSE
 
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/bin/defaults 'write' 'com.runningwithcrayons.Alfred-Preferences' 'features.defaultresults.scope' "('/Applications','/Library/PreferencePanes','/System/Library/PreferencePanes','#{Cask.caskroom}')" 2>&1)
+        ['/usr/bin/defaults', 'write', 'com.runningwithcrayons.Alfred-Preferences', 'features.defaultresults.scope', %Q{"('/Applications','/Library/PreferencePanes','/System/Library/PreferencePanes','#{Cask.caskroom}')"}]
       )
 
       TestHelper.must_output(self, lambda {
@@ -95,7 +95,7 @@ describe Cask::CLI::Alfred do
 
       expected_scopes = (Cask::CLI::Alfred::DEFAULT_SCOPES + [Cask.caskroom]).map { |s| "'#{s}'" }
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/bin/defaults 'write' 'com.runningwithcrayons.Alfred-Preferences' 'features.defaultresults.scope' "(#{expected_scopes.join(',')})" 2>&1)
+        ['/usr/bin/defaults', 'write', 'com.runningwithcrayons.Alfred-Preferences', 'features.defaultresults.scope', %Q{"(#{expected_scopes.join(',')})"}]
       )
 
       TestHelper.must_output(self, lambda {
@@ -140,7 +140,7 @@ describe Cask::CLI::Alfred do
       SCOPE_RESPONSE
 
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/bin/defaults 'write' 'com.runningwithcrayons.Alfred-Preferences' 'features.defaultresults.scope' "('/Applications','/Library/PreferencePanes','/System/Library/PreferencePanes')" 2>&1)
+        ['/usr/bin/defaults', 'write', 'com.runningwithcrayons.Alfred-Preferences', 'features.defaultresults.scope', %Q{"('/Applications','/Library/PreferencePanes','/System/Library/PreferencePanes')"}]
       )
 
       TestHelper.must_output(self, lambda {

test/cask/container/naked_test.rb

@@ -11,7 +11,7 @@ describe Cask::Container::Naked do
     cask                 = SpaceyCask.new
     path                 = '/tmp/downloads/kevin-spacey-1.2.pkg'
     expected_destination = cask.destination_path.join('kevin spacey.pkg')
-    expected_command     = %Q(/usr/bin/ditto '--' '#{path}' '#{expected_destination}' 2>&1)
+    expected_command     = ['/usr/bin/ditto', '--', path, expected_destination]
     Cask::FakeSystemCommand.stubs_command(expected_command)
 
     container = Cask::Container::Naked.new(cask, path, Cask::FakeSystemCommand)

test/cask/pkg_test.rb

@@ -23,14 +23,14 @@ describe Cask::Pkg do
       pkg = Cask::Pkg.new('my.fake.pkg', Cask::FakeSystemCommand)
 
       Cask::FakeSystemCommand.stubs_command(
-        "/usr/sbin/pkgutil '--only-files' '--files' 'my.fake.pkg' 2>&1"
+        ['/usr/sbin/pkgutil', '--only-files', '--files', 'my.fake.pkg']
       )
       Cask::FakeSystemCommand.stubs_command(
-        "/usr/sbin/pkgutil '--only-dirs' '--files' 'my.fake.pkg' 2>&1"
+        ['/usr/sbin/pkgutil', '--only-dirs', '--files', 'my.fake.pkg']
       )
 
       Cask::FakeSystemCommand.expects_command(
-        %q(/usr/bin/sudo -E -- '/usr/sbin/pkgutil' '--forget' 'my.fake.pkg' 2>&1)
+        ['/usr/bin/sudo', '-E', '--', '/usr/sbin/pkgutil', '--forget', 'my.fake.pkg']
       )
 
       pkg.uninstall

test/support/fake_system_command.rb

@@ -29,7 +29,7 @@ class Cask::FakeSystemCommand
   def self.verify_expectations!
     expectations.each do |command, times|
       unless system_calls[command] == times
-        fail("expected #{command} to be run #{times} times, but got #{system_calls[command]}")
+        fail("expected #{command.inspect} to be run #{times} times, but got #{system_calls[command]}")
       end
     end
   end