Split PAGER and EDITOR with a Windows-specific shlex equivalent

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
 

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))
 

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()

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)

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]

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,23 +690,75 @@ 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"],
             id="unquoted backslashes eaten in POSIX mode",
         ),
     ],
 )
-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"])