Add support for invoking processes in JRuby

Previously, we used the `Wopen3` library to simplify the act of
executing an external process and collecting its standard out/error
output and exit status.

Unfortunately, Wopen is implemented using `fork`, which isn't supported
by JRuby. After much research it's clear that this is not a trivial task
to support across multiple platforms, but fortunately there is a gem
called `childprocess` which takes care of this for us.

Unfortunately, to use the gem we have to change the signature of
`command` to take an array of arguments rather than a single shell
string, due to the underlying commands `childprocess` utilizes.

Another thing worth mentioning is that while it's perhaps not ideal to
be using temporary files to capture output when spawning external
processes, I was unable to get this to work consistently when using
straight-up pipes. For now, this implementation works consistently and
is acceptable.

Fixes sds#32

Change-Id: Ic4aa71ca61d50f38a8e4ea15b8e8dbcf146c1f69
Reviewed-on: http://gerrit.causes.com/35997
Tested-by: jenkins <jenkins@causes.com>
Reviewed-by: Shane da Silva <shane@causes.com>

Author: Shane da Silva

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Overcommit Changelog
 
+## master (unreleased)
+
+* Change `command` hook helper signature to accept an array of arguments
+  instead of a shell string
+
 ## 0.6.3
 
 * `TextWidth` pre-commit hook now supports customized maximum subject line

README.md

@@ -29,7 +29,13 @@ about Overcommit on our [engineering blog](http://causes.github.io).
 
 ## Requirements
 
-* Ruby 1.8.7+ is supported
+The following Ruby versions are supported:
+
+* 1.8.7
+* 1.9.2
+* 1.9.3
+* 2.0.0
+* 2.1.0
 
 ### Dependencies
 

lib/overcommit/hook/commit_msg/gerrit_change_id.rb

@@ -7,7 +7,7 @@ module Overcommit::Hook::CommitMsg
   # edit the message after a prepare-commit-msg hook was run.
   class GerritChangeId < Base
     def run
-      result = command("#{SCRIPT_LOCATION} #{commit_message_file}")
+      result = command([SCRIPT_LOCATION, commit_message_file])
       return (result.success? ? :good : :bad), result.stdout
     end
 

lib/overcommit/hook/post_checkout/bundle_check.rb

@@ -15,15 +15,15 @@ def run
   private
 
     def dependencies_changed?
-      result = command("git diff --exit-code #{new_head} #{previous_head} --name-only")
+      result = command(%w[git diff --exit-code --name-only] + [new_head, previous_head])
 
       result.stdout.split("\n").any? do |file|
         Array(@config['include']).any? { |glob| File.fnmatch(glob, file) }
       end
     end
 
     def dependencies_satisfied?
-      command('bundle check').success?
+      command(%w[bundle check]).success?
     end
   end
 end

lib/overcommit/hook/pre_commit/author_email.rb

@@ -2,7 +2,7 @@ module Overcommit::Hook::PreCommit
   # Checks the format of an author's email address.
   class AuthorEmail < Base
     def run
-      result = command('git config --get user.email')
+      result = command(%w[git config --get user.email])
       email = result.stdout.chomp
 
       unless email =~ /#{@config['pattern']}/

lib/overcommit/hook/pre_commit/author_name.rb

@@ -2,7 +2,7 @@ module Overcommit::Hook::PreCommit
   # Ensures that a commit author has a name with at least first and last names.
   class AuthorName < Base
     def run
-      result = command('git config --get user.name')
+      result = command(%w[git config --get user.name])
       name = result.stdout.chomp
 
       unless name.split(' ').count >= 2

lib/overcommit/hook/pre_commit/bundle_check.rb

@@ -8,14 +8,14 @@ def run
       end
 
       # Ignore if Gemfile.lock is not tracked by git
-      return :good if command("git check-ignore #{LOCK_FILE}").success?
+      return :good if command(%w[git check-ignore] + [LOCK_FILE]).success?
 
-      result = command('bundle check')
+      result = command(%w[bundle check])
       unless result.success?
         return :bad, result.stdout
       end
 
-      result = command("git diff --quiet -- #{LOCK_FILE}")
+      result = command(%w[git diff --quiet --] + [LOCK_FILE])
       unless result.success?
         return :bad, "#{LOCK_FILE} is not up-to-date -- run `bundle check`"
       end

lib/overcommit/hook/pre_commit/coffee_lint.rb

@@ -6,7 +6,7 @@ def run
         return :warn, 'Run `npm install -g coffeelint`'
       end
 
-      result = command("coffeelint --quiet #{applicable_files.join(' ')}")
+      result = command(%w[coffeelint --quiet] + applicable_files)
       return :good if result.success?
       return :bad, result.stdout
     end

lib/overcommit/hook/pre_commit/css_lint.rb

@@ -6,9 +6,7 @@ def run
         return :warn, 'csslint not installed -- run `npm install -g csslint`'
       end
 
-      paths = applicable_files.join(' ')
-
-      result = command("csslint --quiet --format=compact #{paths} | grep 'Error - '")
+      result = command(%w[csslint --quiet --format=compact] + applicable_files)
       output = result.stdout
       return (output !~ /Error - (?!Unknown @ rule)/ ? :good : :bad), output
     end

lib/overcommit/hook/pre_commit/haml_lint.rb

@@ -6,7 +6,7 @@ def run
         return :warn, 'haml-lint not installed -- run `gem install haml-lint`'
       end
 
-      result = command("haml-lint #{applicable_files.join(' ')}")
+      result = command(%w[haml-lint] + applicable_files)
       return :good if result.success?
 
       # Keep lines from the output for files that we actually modified

lib/overcommit/hook/pre_commit/hard_tabs.rb

@@ -2,10 +2,8 @@ module Overcommit::Hook::PreCommit
   # Checks for hard tabs in files.
   class HardTabs < Base
     def run
-      paths = applicable_files.join(' ')
-
       # Catches hard tabs
-      result = command("grep -IHn \"\t\" #{paths}")
+      result = command(%w[grep -IHn] + ["\t"] + applicable_files)
       unless result.stdout.empty?
         return :bad, "Hard tabs detected:\n#{result.stdout}"
       end

lib/overcommit/hook/pre_commit/js_hint.rb

@@ -6,7 +6,7 @@ def run
         return :warn, 'jshint not installed -- run `npm install -g jshint`'
       end
 
-      result = command("jshint #{applicable_files.join(' ')}")
+      result = command(%w[jshint] + applicable_files)
       output = result.stdout
 
       return (output.empty? ? :good : :bad), output

lib/overcommit/hook/pre_commit/jscs.rb

@@ -7,7 +7,7 @@ def run
         return :warn, 'jscs not installed -- run `npm install -g jscs`'
       end
 
-      result = command("jscs --reporter=inline #{applicable_files.join(' ')}")
+      result = command(%w[jscs --reporter=inline] + applicable_files)
       return :good if result.success?
 
       if /Config.*not found/i =~ result.stderr

lib/overcommit/hook/pre_commit/python_flake8.rb

@@ -6,7 +6,7 @@ def run
         return :warn, 'flake8 not installed -- run `pip install flake8`'
       end
 
-      result = command("flake8 #{applicable_files.join(' ')}")
+      result = command(%w[flake8] + applicable_files)
 
       return (result.success? ? :good : :bad), result.stdout
     end

lib/overcommit/hook/pre_commit/rubocop.rb

@@ -6,11 +6,13 @@ def run
         return :warn, 'Rubocop not installed -- run `gem install rubocop`'
       end
 
-      result = command("rubocop --format=emacs #{applicable_files.join(' ')} 2>&1")
+      result = command(%w[rubocop --format=emacs] + applicable_files)
       return :good if result.success?
 
+      output = result.stdout + result.stderr
+
       # Keep lines from the output for files that we actually modified
-      error_lines, warning_lines = result.stdout.split("\n").partition do |output_line|
+      error_lines, warning_lines = output.split("\n").partition do |output_line|
         if match = output_line.match(/^([^:]+):(\d+)/)
           file = match[1]
           line = match[2]

lib/overcommit/hook/pre_commit/scss_lint.rb

@@ -6,7 +6,7 @@ def run
         return :warn, 'scss-lint not installed -- run `gem install scss-lint`'
       end
 
-      result = command("scss-lint #{applicable_files.join(' ')}")
+      result = command(%w[scss-lint] + applicable_files)
       return :good if result.success?
 
       # Keep lines from the output for files that we actually modified

lib/overcommit/hook/pre_commit/trailing_whitespace.rb

@@ -2,9 +2,7 @@ module Overcommit::Hook::PreCommit
   # Checks for trailing whitespace in files.
   class TrailingWhitespace < Base
     def run
-      paths = applicable_files.join(' ')
-
-      result = command("grep -IHn \"\\s$\" #{paths}")
+      result = command(%w[grep -IHn \s$] + applicable_files)
       unless result.stdout.empty?
         return :bad, "Trailing whitespace detected:\n#{result.stdout}"
       end

lib/overcommit/subprocess.rb

@@ -0,0 +1,37 @@
+require 'childprocess'
+require 'tempfile'
+
+module Overcommit
+  # Manages execution of a child process, collecting the exit status and
+  # standard out/error output.
+  class Subprocess
+    # Encapsulates the result of a process.
+    Result = Struct.new(:status, :stderr, :stdout) do
+      def success?
+        status == 0
+      end
+    end
+
+    # Spawns a new process using the given array of arguments (the first
+    # element is the command).
+    def self.spawn(args)
+      process = ChildProcess.build(*args)
+
+      err = ::Tempfile.new('err')
+      err.sync = true
+      out = ::Tempfile.new('out')
+      out.sync = true
+
+      process.io.stderr = err
+      process.io.stdout = out
+
+      process.start
+      process.wait
+
+      err.rewind
+      out.rewind
+
+      Result.new(process.exit_code, err.read, out.read)
+    end
+  end
+end

lib/overcommit/utils.rb

@@ -1,4 +1,4 @@
-require 'wopen3'
+require 'overcommit/subprocess'
 
 module Overcommit
   # Utility functions for general use.
@@ -62,9 +62,9 @@ def in_path?(cmd)
       # Overcommit to call other Ruby executables without requiring that they be
       # specified in Overcommit's Gemfile--a nasty consequence of using
       # `bundle exec overcommit` while developing locally.
-      def command(command)
+      def command(args)
         with_environment 'RUBYOPT' => nil do
-          Wopen3.system(command)
+          Subprocess.spawn(args)
         end
       end
 

overcommit.gemspec

@@ -26,7 +26,7 @@ Gem::Specification.new do |s|
 
   s.required_ruby_version = '>= 1.8.7'
 
-  s.add_dependency             'wopen3'
+  s.add_dependency             'childprocess', '>= 0.5.1'
 
   s.add_development_dependency 'rspec', '2.14.1'
   s.add_development_dependency 'image_optim', '0.11.1'

spec/integration/committing_spec.rb

@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe 'commiting' do
-  subject { shell('git commit --allow-empty -m "Test"') }
+  subject { shell(%w[git commit --allow-empty -m Test]) }
 
   let(:config) { <<-YML }
     CommitMsg:

spec/overcommit/hook/pre_commit/bundle_check_spec.rb

@@ -38,7 +38,7 @@
     before do
       result.stub(:success? => success, :stdout => 'Bundler error message')
       subject.stub(:command).and_call_original
-      subject.stub(:command).with('bundle check').and_return(result)
+      subject.stub(:command).with(%w[bundle check]).and_return(result)
     end
 
     context 'and bundle check exits unsuccessfully' do

spec/overcommit/hook/pre_commit/rubocop_spec.rb

@@ -41,6 +41,7 @@
         result.stub(:stdout).and_return([
           'file1.rb:1:1: C: Missing top-level class documentation',
         ].join("\n"))
+        result.stub(:stderr).and_return('')
 
         subject.stub(:modified_lines).and_return([2, 3])
       end
@@ -53,6 +54,7 @@
         result.stub(:stdout).and_return([
           'file1.rb:1:1: C: Missing top-level class documentation',
         ].join("\n"))
+        result.stub(:stderr).and_return('')
 
         subject.stub(:modified_lines).and_return([1, 2])
       end

spec/support/shell_helpers.rb

@@ -1,8 +1,8 @@
-require 'wopen3'
+require 'overcommit/subprocess'
 
 # Helpers for executing shell commands in tests.
 module ShellHelpers
   def shell(command)
-    Wopen3.system(command)
+    Overcommit::Subprocess.spawn(command)
   end
 end