Stop wrapping commands in cmd.exe on Windows

[bwilde-castle]

An attempt at Issue #847. This isn't a corner case: pre-commit and commit-msg hooks both stash unstaged changes before running hooks, by default, so any Windows user doing a partial commit (staging some but not all changes) hits this — hooks fail outright with error: unknown switch '0' instead of running at all.

I commented on the issue a couple of months ago; I've since tested this fix end to end on Windows 11 / Ruby 3.3, and while the suggestion still looks right, my description of the cause back then was off. Here's what I think is actually going on.

The problem

win32_prepare_args escapes metacharacters per argument, then joins everything into one string behind cmd.exe /c:

%w[cmd.exe /c] + [args.join(' ')]

The escaping doesn't seem to be what breaks; the join is. It collapses the command into a single argv element, and childprocess 5.x on Windows is a thin shim over Process.spawn, which sees the embedded spaces, wraps the element in quotes, and backslash-escapes the quotes already inside it. cmd.exe strips the outer pair, and the child's C runtime then reads the surviving \" as a literal quote rather than a delimiter — so the argument re-splits on whitespace.

With the pre-commit stash message (Overcommit: Stash of repo state before hook run at 2024-04-10 12:34:56 -0700), git stash save receives the UTC offset as a switch of its own and aborts with error: unknown switch '0'. On a throwaway repo I get exit 129 with that message on main, and exit 0 with the message intact on this branch.

I couldn't find an escaping scheme that fixes this while keeping the wrapper: inside double quotes & | < > ( ) are already literal, %VAR% still expands and can't be caret-escaped, and carets inside quotes reach the child as literal carets.

The change

Drop the wrapper and hand the argument vector to ChildProcess.build unmodified. Process.spawn appears to do the command lookup the wrapper was standing in for — .bat, .cmd, extensionless RubyGems shims and PATHEXT names all launched fine in my testing, including the real bundle.bat — and arguments stay atomic because no shell re-parses them.

Both call sites were already gated on OS.windows?, so this is Windows-only in effect; macOS and Linux behaviour is unchanged.

That Process.spawn backend arrived in childprocess 5.0.0, which replaced the Windows FFI layer. Before that, Windows used raw CreateProcess, which does no PATHEXT resolution, so I've raised the gemspec floor from >= 0.6.3 to >= 5.0.0. I realise that's the debatable part and entirely your call — for context, the < 6 ceiling was already there, 5.0.0 is from 2022, this repo's Gemfile.lock resolves to 5.x, and 4.1.0/5.0.0/5.1.0 all declare required_ruby_version >= 2.4.0.

One wrinkle: mklink and dir (used by FileUtils for symlinks) are cmd.exe built-ins with no standalone executable, so dropping the wrapper broke FileUtils.symlink. Those two call sites now pass cmd.exe, /c, and the command as separate argv elements, which avoids reintroducing the join-based bug above.

Tests

Adds spec/overcommit/subprocess_spec.rb — there wasn't coverage for Subprocess before. It uses Gem.ruby and a NUL-joined ARGV dump so it should be platform-agnostic and pass on the current Linux CI. It covers the #847 case directly, plus results and exit status, options[:input], metacharacter and empty-string arguments, and that ChildProcess.build receives the argv verbatim from both .spawn and .spawn_detached.

On Windows locally, bundle exec overcommit --run passes and rubocop is clean. bundle exec rspec gives 1643 examples / 2 failures, the same as on main (git_repo_spec.rb:341, and utils_spec.rb:142, which needs wmic); git_repo_spec.rb:218 fails on main and passes here.

Notes

• I also dropped the now-unused require 'overcommit/os'; happy to put it back if you'd rather keep the diff minimal.
• No spec coverage for the mklink/dir fix — FileUtils has no existing spec file and Windows CI is disabled (Various specs fail on Windows for all Ruby versions #836).
• Arguments containing both whitespace and a trailing backslash are still mangled, but that reproduces on bare Process.spawn, so it looks unrelated.
• Anyone relying on cmd.exe expansion inside a hook command would lose it. The API has always been argv-based and execute_in_background already raises on |, so I don't think it'll be an issue, but the reviewer will probably know better.