Repository navigation
Commit 55ea3fd
authored
Stop wrapping commands in cmd.exe on Windows (#898)
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](#847 (comment))
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`:
```ruby
%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 (#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.1 parent 4ddaec6 commit 55ea3fd
4 files changed
Lines changed: 147 additions & 22 deletions
File tree
- lib/overcommit
- utils
- spec/overcommit
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
5 | | - | |
6 | 5 | | |
7 | 6 | | |
8 | 7 | | |
| |||
28 | 27 | | |
29 | 28 | | |
30 | 29 | | |
31 | | - | |
32 | | - | |
33 | 30 | | |
34 | 31 | | |
35 | 32 | | |
| |||
58 | 55 | | |
59 | 56 | | |
60 | 57 | | |
61 | | - | |
62 | | - | |
63 | 58 | | |
64 | 59 | | |
65 | 60 | | |
| |||
70 | 65 | | |
71 | 66 | | |
72 | 67 | | |
73 | | - | |
74 | | - | |
75 | | - | |
76 | | - | |
77 | | - | |
78 | | - | |
79 | | - | |
80 | | - | |
81 | | - | |
82 | | - | |
83 | | - | |
84 | | - | |
85 | | - | |
86 | | - | |
87 | 68 | | |
88 | 69 | | |
89 | 70 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
49 | 49 | | |
50 | 50 | | |
51 | 51 | | |
52 | | - | |
| 52 | + | |
53 | 53 | | |
54 | 54 | | |
55 | 55 | | |
56 | 56 | | |
57 | 57 | | |
58 | | - | |
| 58 | + | |
59 | 59 | | |
60 | 60 | | |
61 | 61 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
31 | 31 | | |
32 | 32 | | |
33 | 33 | | |
34 | | - | |
| 34 | + | |
35 | 35 | | |
36 | 36 | | |
37 | 37 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
0 commit comments