diff --git a/lib/overcommit/subprocess.rb b/lib/overcommit/subprocess.rb index 41175fb9..88ef362e 100644 --- a/lib/overcommit/subprocess.rb +++ b/lib/overcommit/subprocess.rb @@ -2,7 +2,6 @@ require 'childprocess' require 'tempfile' -require 'overcommit/os' module Overcommit # Manages execution of a child process, collecting the exit status and @@ -28,8 +27,6 @@ class << self # @option options [String] input string to pass via standard input stream # @return [Result] def spawn(args, options = {}) - args = win32_prepare_args(args) if OS.windows? - process = ChildProcess.build(*args) out, err = assign_output_streams(process) @@ -58,8 +55,6 @@ def spawn(args, options = {}) # Spawns a new process in the background using the given array of # arguments (the first element is the command). def spawn_detached(args) - args = win32_prepare_args(args) if OS.windows? - process = ChildProcess.build(*args) process.detach = true @@ -70,20 +65,6 @@ def spawn_detached(args) private - # Necessary to run commands in the cmd.exe context. - # Args are joined to properly handle quotes and special characters. - def win32_prepare_args(args) - args = args.map do |arg| - # Quote args that contain whitespace - arg = "\"#{arg}\"" if arg =~ /\s/ - - # Escape cmd.exe metacharacters - arg.gsub(/[()%!^"<>&|]/, '^\0') - end - - %w[cmd.exe /c] + [args.join(' ')] - end - # @param process [ChildProcess] # @return [Array] def assign_output_streams(process) diff --git a/lib/overcommit/utils/file_utils.rb b/lib/overcommit/utils/file_utils.rb index cff2ce0b..1d8b7bb9 100644 --- a/lib/overcommit/utils/file_utils.rb +++ b/lib/overcommit/utils/file_utils.rb @@ -49,13 +49,13 @@ def readlink(link_name) def win32_dir_cmd(file_name) Overcommit::Subprocess.spawn( - %W[dir #{win32_fix_pathsep(file_name)}] + %W[cmd.exe /c dir #{win32_fix_pathsep(file_name)}] ) end def win32_mklink_cmd(old_name, new_name) Overcommit::Subprocess.spawn( - %W[mklink #{win32_fix_pathsep(new_name)} #{win32_fix_pathsep(old_name)}] + %W[cmd.exe /c mklink #{win32_fix_pathsep(new_name)} #{win32_fix_pathsep(old_name)}] ) end diff --git a/overcommit.gemspec b/overcommit.gemspec index 1bb8adaa..a753af6d 100644 --- a/overcommit.gemspec +++ b/overcommit.gemspec @@ -31,7 +31,7 @@ Gem::Specification.new do |s| s.required_ruby_version = '>= 2.6' - s.add_dependency 'childprocess', '>= 0.6.3', '< 6' + s.add_dependency 'childprocess', '>= 5.0.0', '< 6' s.add_dependency 'iniparse', '~> 1.4' s.add_dependency 'rexml', '>= 3.4.2' end diff --git a/spec/overcommit/subprocess_spec.rb b/spec/overcommit/subprocess_spec.rb new file mode 100644 index 00000000..07503d25 --- /dev/null +++ b/spec/overcommit/subprocess_spec.rb @@ -0,0 +1,144 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Overcommit::Subprocess do + # Absolute path to the Ruby currently running the specs. Using this instead of + # a bare `ruby` keeps these specs working regardless of what is on the PATH. + let(:ruby) { Gem.ruby } + + # Script which writes its own ARGV to standard output, using NUL as the + # separator so that arguments containing whitespace remain distinguishable. + let(:argv_dump) { 'STDOUT.print ARGV.join("\0")' } + + # Splits the output of `argv_dump` back into the array the child received. + def received_argv(result) + result.stdout.split("\0", -1) + end + + describe '.spawn' do + context 'when the command succeeds' do + subject do + described_class.spawn([ruby, '-e', 'STDOUT.print "hello"; STDERR.print "world"']) + end + + it 'returns a successful result containing the captured output' do + subject.should be_a described_class::Result + subject.should be_success + subject.status.should == 0 + subject.stdout.should == 'hello' + subject.stderr.should == 'world' + end + end + + context 'when the command fails' do + subject do + described_class.spawn([ruby, '-e', 'STDERR.print "boom"; exit 42']) + end + + it 'returns an unsuccessful result containing the exit status' do + subject.should_not be_success + subject.status.should == 42 + subject.stdout.should == '' + subject.stderr.should == 'boom' + end + end + + context 'when given input' do + subject do + described_class.spawn([ruby, '-e', 'STDOUT.print STDIN.read'], input: 'from-stdin') + end + + it 'passes the input to the standard input stream of the process' do + subject.should be_success + subject.stdout.chomp.should == 'from-stdin' + end + end + + context 'when an argument contains whitespace and a dash-prefixed token' do + # Regression test for https://github.com/sds/overcommit/issues/847, where + # arguments were joined into a single string before being handed to the + # shell, causing the child to re-split them on whitespace. This made + # `git stash save` interpret the tail of the stash message (the UTC offset) + # as a switch of its own, failing with `unknown switch '0'`. + let(:stash_message) do + 'Overcommit: Stash of repo state before hook run at 2024-04-10 12:34:56 -0700' + end + + subject do + described_class.spawn([ruby, '-e', argv_dump, 'save', stash_message]) + end + + it 'delivers the argument to the child as a single atomic argument' do + subject.should be_success + received_argv(subject).should == ['save', stash_message] + end + end + + context 'when arguments contain shell metacharacters' do + let(:tricky_args) do + [ + 'a&b', + 'a|b', + 'a>b', + 'a nil, :stderr= => nil) } + + let(:process) do + double( + 'process', + io: io, + :duplex= => nil, + :detach= => nil, + start: nil, + wait: nil, + exit_code: 0 + ) + end + + it 'passes the arguments through verbatim from .spawn' do + ChildProcess.should_receive(:build).with(*args).and_return(process) + described_class.spawn(args) + end + + it 'passes the arguments through verbatim from .spawn_detached' do + ChildProcess.should_receive(:build).with(*args).and_return(process) + described_class.spawn_detached(args) + end + end +end