fix(tools): pass native Windows paths to ripgrep under MSYS_NO_PATHCONV - #77443
Closed
sotwind wants to merge 1 commit into
Closed
fix(tools): pass native Windows paths to ripgrep under MSYS_NO_PATHCONV#77443sotwind wants to merge 1 commit into
sotwind wants to merge 1 commit into
Conversation
_escape_shell_arg rewrites drive paths to the Git Bash /c/... form,
which MSYS tools (grep/find/test) require. Native binaries are
different: Hermes runs bash with MSYS_NO_PATHCONV=1 and
MSYS2_ARG_CONV_EXCL=*, so argv reaches them verbatim, and a native
ripgrep build (e.g. the WinGet MSVC package) cannot resolve /c/... --
every explicit-path search failed with 'IO error ... The system cannot
find the path specified (os error 3)'. Only the default relative root
('.') worked.
Add ShellFileOperations._escape_native_tool_arg() which converts the
path to the native C:/... form on Windows (via _msys_to_windows_path,
idempotent, no-op off Windows) and use it for the path argument at the
three ripgrep invocation sites (_search_with_rg, _search_files_rg
sorted + plain fallback). MSYS-built rg also accepts native paths, and
grep/find/test call sites intentionally keep the MSYS form.
Collaborator
Duplicate of #67914, which already covers the native-ripgrep path conversion and adds backend gating to protect remote paths. |
Contributor
|
Closing as implemented on main: #84378 (merged) covers this fix — a shared native-path escaper (_escape_native_tool_arg) applied to all six rg call sites, the zero-match probes, and the shell linter interpolation, with Windows regression tests. Thank you for diagnosing this — your PR correctly identified the same root cause (native binaries + MSYS_NO_PATHCONV means /c/... is never translated back), and the merged fix lands the same approach across the full call-site set. Sorry we couldn't land this one directly. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On Windows, the
search_filestool fails for every explicit absolute path when the installed ripgrep is a native Windows build (e.g. the WinGet MSVC package):Root cause chain:
ShellFileOperations._escape_shell_argrewrites drive paths (E:\...) to the Git Bash/e/...form — required for MSYS tools (grep, find, test, bash builtins).MSYS_NO_PATHCONV=1/MSYS2_ARG_CONV_EXCL=*, so argv reaches native binaries verbatim (no/e/...→E:\...translation back).rg.execannot resolve/e/...and errors out.Only the default relative root (
.) worked, because no drive-path rewrite happens there.Fix
Add
ShellFileOperations._escape_native_tool_arg(), which converts the path to the nativeE:/...form on Windows (via_msys_to_windows_path; idempotent, no-op off Windows) and use it for the path argument at the three ripgrep invocation sites:_search_with_rg(content search)_search_files_rgsorted command_search_files_rgplain fallbackMSYS-built rg also accepts native paths, so this is safe for either flavor. grep/find/test call sites intentionally keep the MSYS form.
Testing
tests/tools/test_file_ops_native_tool_arg.py(9 tests): path-form conversion matrix on/off Windows, plus command-construction tests asserting rg receives the native form.search_tool: content search on a single file and a directory,--filesglob search, glob+path combo, no-match case, and context mode all succeed with absolute native paths.test_file_operations.py+test_search_auto_multiline.pyon that machine: 41 passed / 10 failed with this change vs 38 passed / 13 failed without it — the change fixes 3 pre-existing failures and introduces none (the remaining failures are pre-existing Windows-environment issues: POSIX umask, symlink, and rg-multiline cases, all failing on unmodified main too).