From d2a4dc2c838c668fb9668fba87f65d543648d15a Mon Sep 17 00:00:00 2001 From: Kevin Deldycke Date: Thu, 24 Sep 2026 18:10:32 +0400 Subject: [PATCH] Split `PAGER` and `EDITOR` with a Windows-specific `shlex` equivalent --- CHANGES.md | 4 ++ src/click/_compat.py | 28 ++++++++ src/click/_termui_impl.py | 23 ++++--- src/click/_winconsole.py | 33 ++++++++++ src/click/shell_completion.py | 17 +++++ tests/test_termui.py | 118 ++++++++++++++++++++++++++++++---- 6 files changed, 200 insertions(+), 23 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 7f1b48543..21f83a8df 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -24,6 +24,10 @@ Unreleased - A command's short help no longer stops at an abbreviation such as `vs.` or `e.g.` inside the first sentence. A period ends the sentence only when it closes the text or the next word does not start in lowercase. {pr}`3865` +- On Windows, Click splits `PAGER` and `EDITOR` into an `argv` list with the + Windows rules instead of the POSIX rules. An unquoted path such as + `EDITOR=C:\Windows\notepad.exe` keeps its backslashes. POSIX platforms do not + change. {pr}`3880` ## Version 8.5.0 diff --git a/src/click/_compat.py b/src/click/_compat.py index 8507a503a..f6218ea69 100644 --- a/src/click/_compat.py +++ b/src/click/_compat.py @@ -533,6 +533,34 @@ def _get_windows_console_stream( return None +def _split_command_line(cmd: str) -> list[str]: + """Split a command line string into an ``argv`` list. + + Each platform has its own rules, and the rules disagree about a backslash. + POSIX uses :func:`shlex.split`, where a backslash escapes the character + after it. Windows uses ``CommandLineToArgvW``, where a backslash is a + normal character. The Windows rule keeps the path ``C:\\Users\\click`` as + one token. The POSIX rule removes the backslashes and gives + ``C:Usersclick``. + + Click uses this function for the command lines that a user sets in the + environment, such as ``PAGER`` and ``EDITOR``. + + :param cmd: The command line to split. + :raises ValueError: On POSIX only, if a quote has no matching close quote. + Windows does not report this error. It reads the rest of the string as + one token. + """ + if sys.platform == "win32": + from ._winconsole import _split_windows_command_line + + return _split_windows_command_line(cmd) + + import shlex + + return shlex.split(cmd) + + def term_len(x: str) -> int: return len(strip_ansi(x)) diff --git a/src/click/_termui_impl.py b/src/click/_termui_impl.py index 70aabf8f1..113331183 100644 --- a/src/click/_termui_impl.py +++ b/src/click/_termui_impl.py @@ -20,6 +20,7 @@ from types import TracebackType from ._compat import _default_text_stdout +from ._compat import _split_command_line from ._compat import CYGWIN from ._compat import get_best_encoding from ._compat import isatty @@ -462,11 +463,11 @@ def _pager_contextmanager( if not isatty(sys.stdin) or not isatty(stdout): return _nullpager(stdout, color) - # Split using POSIX mode (the default) so that quote characters are - # stripped from tokens and quoted Windows paths are preserved. - # Non-POSIX mode retains quotes in tokens, and wrapping tokens - # with shlex.quote re-introduces quoting issues on Windows. - pager_cmd_parts = shlex.split(os.environ.get("PAGER", "")) + # Split PAGER with the rules of the current platform. A quoted token + # keeps its spaces, and a Windows path keeps its backslashes. Both + # platforms remove the quote characters, and subprocess adds the quotes + # again when it starts the child process. + pager_cmd_parts = _split_command_line(os.environ.get("PAGER", "")) if pager_cmd_parts: # Piping to `more` on Windows adds spurious \r\n, so it gets the temp @@ -525,6 +526,10 @@ def _less_uses_raw_mode(less_env: str, cmd_params: list[str]) -> bool: ``--raw-control-chars`` long option request raw mode. Filenames and long-option values may carry the letter ``r`` without being such a request. + + ``LESS`` uses POSIX splitting on every platform, and ``PAGER`` does not. + ``less`` reads this variable and parses it with its own rules. The + ``less`` on Windows comes from Git for Windows or MSYS. """ try: env_tokens = shlex.split(less_env) @@ -712,7 +717,6 @@ def get_editor(self) -> str: def edit_files(self, filenames: cabc.Iterable[str | os.PathLike[str]]) -> None: """Open files in the user's editor.""" - import shlex import subprocess editor = self.get_editor() @@ -723,11 +727,10 @@ def edit_files(self, filenames: cabc.Iterable[str | os.PathLike[str]]) -> None: environ.update(self.env) try: - # Split in POSIX mode (the default) for the same reasons as - # in pager(): strips quotes from tokens and preserves quoted - # Windows paths. + # Split EDITOR with the rules of the current platform. See the + # related explanation in the pager code above. c = subprocess.Popen( - args=shlex.split(editor) + list(filenames), + args=_split_command_line(editor) + list(filenames), env=environ, ) exit_code = c.wait() diff --git a/src/click/_winconsole.py b/src/click/_winconsole.py index dc287bc28..62ba9499e 100644 --- a/src/click/_winconsole.py +++ b/src/click/_winconsole.py @@ -35,6 +35,7 @@ assert sys.platform == "win32" import msvcrt # noqa: E402 from ctypes import windll # noqa: E402 +from ctypes import WinError # noqa: E402 from ctypes import WINFUNCTYPE # noqa: E402 c_ssize_p = POINTER(c_ssize_t) @@ -290,3 +291,35 @@ def _get_windows_console_stream( return None return func(b) + + +def _split_windows_command_line(cmd: str) -> list[str]: + """Split a command line into arguments with the Windows rules. + + ``CommandLineToArgvW`` is the ``shell32`` function that gives a Windows + program its ``argv``. It applies the rules that Windows applies to every + command line. A backslash is a normal character, so the path + ``C:\\Users\\click`` stays one token. A quoted string keeps its spaces. + + Windows parses the first token with different rules. That token ends at the + first space, and a backslash in it is not an escape. An empty command line + makes the function return the path of the running executable, not an empty + list. This function puts the program name ``sentinel`` in front of ``cmd`` + to avoid all three cases, and then removes that token. Every token of + ``cmd`` then gets the same rules. + + :param cmd: The command line to split. + :raises OSError: If ``CommandLineToArgvW`` fails. + """ + argc = c_int(0) + argv = CommandLineToArgvW(f"sentinel {cmd}", byref(argc)) + + if not argv: + raise WinError() + + try: + # Do not return the sentinel program name at index 0. + return [argv[index] for index in range(1, argc.value)] + finally: + # The caller owns the buffer, so the caller frees it. + LocalFree(argv) diff --git a/src/click/shell_completion.py b/src/click/shell_completion.py index 8a2668b09..a92aed1f0 100644 --- a/src/click/shell_completion.py +++ b/src/click/shell_completion.py @@ -528,6 +528,23 @@ class PowerShellComplete(ShellComplete): source_template: t.ClassVar[str] = _SOURCE_POWERSHELL def get_completion_args(self) -> tuple[list[str], str]: + """Split ``COMP_WORDS``, the command line that PowerShell parsed. + + .. caution:: + + POSIX splitting removes the backslashes of a path, so + ``C:\\Users\\click`` arrives here as ``C:Usersclick``. + + ``click._compat._split_command_line`` does not fix this. Click uses + that function for ``PAGER`` and ``EDITOR``. ``COMP_CWORD`` is the + number of elements that PowerShell counted, so this split must + return the same number of tokens. PowerShell uses a single quote to + delimit a string, but ``CommandLineToArgvW`` reads it as a normal + character. So ``'C:\\My Files\\a.toml'`` becomes two tokens, and + every index after it points at the wrong word. + + A fix needs a tokenizer that uses the PowerShell rules. + """ cwords = split_arg_string(os.environ["COMP_WORDS"]) cword = int(os.environ["COMP_CWORD"]) args = cwords[1:cword] diff --git a/tests/test_termui.py b/tests/test_termui.py index 1825b1987..c55178fb4 100644 --- a/tests/test_termui.py +++ b/tests/test_termui.py @@ -4,7 +4,6 @@ import os import pathlib import platform -import shlex import shutil import subprocess import sys @@ -16,6 +15,7 @@ import click import click._termui_impl +from click._compat import _split_command_line from click._compat import WIN from click._termui_impl import Editor from click._utils import UNSET @@ -461,6 +461,7 @@ def test_edit_pathlib(runner, tmp_path, use_iterable): assert file_path.read_text(encoding="UTF-8") == "aTest\nbTest\n" +@pytest.mark.skipif(WIN, reason="Uses the POSIX splitting rules.") @pytest.mark.parametrize( ("editor_cmd", "filenames", "expected_args"), [ @@ -553,9 +554,23 @@ def test_edit_pathlib(runner, tmp_path, use_iterable): ["editor", "file'name.txt"], id="filename with single quote", ), + # Issue #1760: a filename holding paired quotes and spaces stays one + # argument. Only the editor command is split; filenames are appended. + pytest.param( + "editor", + ['foo "bar baz"'], + ["editor", 'foo "bar baz"'], + id="filename with paired quotes and spaces", + ), ], ) def test_editor_path_normalization(editor_cmd, filenames, expected_args): + """Verify the argv that Click builds from a POSIX ``EDITOR`` value. + + Click splits ``EDITOR`` with the rules of the platform that set it. On + Windows a backslash is a normal character and not an escape, so the + Windows cases are in the next table. + """ with patch("subprocess.Popen") as mock_popen: mock_popen.return_value.wait.return_value = 0 Editor(editor=editor_cmd).edit_files(filenames) @@ -580,6 +595,26 @@ def test_editor_path_normalization(editor_cmd, filenames, expected_args): ["C:\\Program Files\\Sublime Text 3\\sublime_text.exe", "--wait"], id="quoted path with flag", ), + # A Windows path needs no quotes when it has no space, and its + # backslashes stay. POSIX splitting removed them and gave + # ``C:Windowsnotepad.exe``. + pytest.param( + "C:\\Windows\\notepad.exe", + ["C:\\Windows\\notepad.exe"], + id="unquoted path with backslashes", + ), + pytest.param( + "C:\\tools\\vim\\vim.exe -u NONE", + ["C:\\tools\\vim\\vim.exe", "-u", "NONE"], + id="unquoted path with flags", + ), + # Windows does not report an unclosed quote. The quote runs to the end + # of the string. On POSIX the same value raises ValueError. + pytest.param( + '"C:\\Program Files\\Vim\\vim.exe', + ["C:\\Program Files\\Vim\\vim.exe"], + id="unclosed quote runs to end of string", + ), ], ) def test_editor_windows_path_normalization(editor_cmd, expected_cmd): @@ -617,6 +652,7 @@ def test_editor_nonexistent_exception(): Editor(editor="nonexistent").edit_files(["f.txt"]) +@pytest.mark.skipif(WIN, reason="Uses the POSIX splitting rules.") @pytest.mark.parametrize( ("pager_env", "expected_parts"), [ @@ -654,8 +690,9 @@ def test_editor_nonexistent_exception(): ["/usr/bin/my pager"], id="escaped space in unix path", ), - # PR #1477: POSIX mode (the default) eats unquoted backslashes. - # On Windows, users must quote paths that contain backslashes. + # PR #1477: POSIX splitting removes a backslash that is not inside + # quotes, so a Windows path needs quotes to keep them. Windows uses its + # own rules, which keep the backslashes. See the next test. pytest.param( "C:\\path\\to\\exe /test other\\path", ["C:pathtoexe", "/test", "otherpath"], @@ -663,14 +700,65 @@ def test_editor_nonexistent_exception(): ), ], ) -def test_pager_shlex_split(pager_env, expected_parts): - """Verify shlex.split produces the expected argv for PAGER values. +def test_pager_command_line_split(pager_env, expected_parts): + """Verify the argv that Click builds from a POSIX ``PAGER`` value. Tests the splitting logic used by :func:`click._termui_impl.pager` to turn the ``PAGER`` environment variable into an ``argv`` list. See issue #1026, PR #1477, PR #1543, PR #2775. """ - assert shlex.split(pager_env) == expected_parts + assert _split_command_line(pager_env) == expected_parts + + +@pytest.mark.skipif(not WIN, reason="Uses the Windows splitting rules.") +@pytest.mark.parametrize( + ("pager_env", "expected_parts"), + [ + # This list is empty because Click adds the program name ``sentinel`` + # first. Without it, CommandLineToArgvW returns the path of the running + # executable for an empty command line. + pytest.param("", [], id="empty string"), + pytest.param("more", ["more"], id="simple command"), + pytest.param("less -FRSX", ["less", "-FRSX"], id="command with flags"), + # The fix: an unquoted Windows path keeps its backslashes. POSIX + # splitting gave ``C:Toolsless.exe`` instead. + pytest.param( + "C:\\Tools\\less.exe -R", + ["C:\\Tools\\less.exe", "-R"], + id="unquoted path with backslashes", + ), + # A path with a space still needs quotes. The split removes them. + pytest.param( + '"C:\\Program Files\\Git\\usr\\bin\\less.exe" -R', + ["C:\\Program Files\\Git\\usr\\bin\\less.exe", "-R"], + id="quoted path with spaces", + ), + # An unclosed quote runs to the end of the string. It does not raise. + pytest.param( + '"C:\\Program Files\\Git\\usr\\bin\\less.exe', + ["C:\\Program Files\\Git\\usr\\bin\\less.exe"], + id="unclosed quote runs to end of string", + ), + # Issue #2486: a backslash before a quote escapes it, so a quoted + # path ending in one turns its closing quote into a literal and the + # run swallows what follows. A Windows rule, not a Click bug. + pytest.param( + '"C:\\Tools\\Git\\" -R', + ['C:\\Tools\\Git" -R'], + id="quoted path with trailing backslash swallows the rest", + ), + # The control: with no quote to escape, the same trailing backslash + # is just a backslash. + pytest.param( + "C:\\Tools\\Git\\", + ["C:\\Tools\\Git\\"], + id="unquoted trailing backslash", + ), + ], +) +def test_pager_command_line_split_windows(pager_env, expected_parts): + """Verify the argv that Click builds from a Windows ``PAGER`` value.""" + assert _split_command_line(pager_env) == expected_parts def _get_real_pager_command() -> str: @@ -1173,13 +1261,12 @@ def _force_tempfile_pager(monkeypatch, pager_cmd="cat"): That backend is only reachable on Windows, so the platform flag and the tty probes are faked to exercise it from any runner. - ``PAGER`` is set to the bare command name, not to the path - :func:`shutil.which` resolves it to. ``pager()`` splits ``PAGER`` with - :func:`shlex.split` in POSIX mode, where a Windows path loses its - backslashes and splits on the space in ``C:\\Program Files``, leaving a - command that resolves to nothing. Click resolves the bare name itself. + ``PAGER`` gets the command name alone, not the path that + :func:`shutil.which` finds. An unquoted pager path splits at the space in + ``C:\\Program Files``, and the first token then finds no command. Click + resolves the command name itself. """ - cmd = shlex.split(pager_cmd)[0] + cmd = _split_command_line(pager_cmd)[0] assert shutil.which(cmd) is not None, f"{cmd} not available" monkeypatch.setattr(click._termui_impl, "isatty", lambda _: True) monkeypatch.setattr(click._termui_impl, "WIN", True) @@ -1308,8 +1395,13 @@ def spy_unlink(path, **kwargs): ) +@pytest.mark.skipif(WIN, reason="Windows does not report an unclosed quote.") def test_editor_unclosed_quote(): - """An unclosed quote in the editor command raises ValueError.""" + """An unclosed quote in the editor command raises ValueError. + + This is POSIX only. The Windows split runs the quote to the end of the + string. ``test_editor_windows_path_normalization`` covers that case. + """ with pytest.raises(ValueError, match="No closing quotation"): Editor(editor='"unclosed').edit_files(["f.txt"])