Stop wrapping commands in cmd.exe on Windows

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<IO>]
       def assign_output_streams(process)

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
 

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

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<b',
+          '(parens)',
+          'caret^escape',
+          'say "hi" now',
+          "it's quoted",
+          'literal%PATH%'
+        ]
+      end
+
+      subject { described_class.spawn([ruby, '-e', argv_dump, *tricky_args]) }
+
+      it 'delivers the arguments to the child untouched' do
+        subject.should be_success
+        received_argv(subject).should == tricky_args
+      end
+    end
+
+    context 'when an argument is empty' do
+      subject { described_class.spawn([ruby, '-e', argv_dump, '', 'after']) }
+
+      it 'preserves the empty argument' do
+        subject.should be_success
+        received_argv(subject).should == ['', 'after']
+      end
+    end
+  end
+
+  # Subprocess no longer branches on platform, but this guards against
+  # reintroducing a shell wrapper -- if one came back, this would catch it
+  # from CI (which only runs Linux -- see
+  # https://github.com/sds/overcommit/issues/836).
+  describe 'the argument vector handed to ChildProcess' do
+    let(:args) do
+      ['git', 'stash', 'save', '--quiet', 'Overcommit: Stash at 2024-04-10 12:34:56 -0700']
+    end
+
+    let(:io) { double('io', :stdout= => 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