Skip to content

Repository navigation

Commit 61b69e9

Browse files
Rowlando13Rowlando13
authored
Resolve the pager command once, in _pager_contextmanager (#3776)
2 parents f36d58b + 9835b0f commit 61b69e9

1 file changed

Lines changed: 64 additions & 87 deletions

File tree

‎src/click/_termui_impl.py‎

Lines changed: 64 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -420,9 +420,37 @@ def __getattr__(self, name: str) -> t.Any:
420420
return getattr(self._stream, name)
421421

422422

423+
def _resolve_pager_command(cmd_parts: list[str]) -> tuple[Path, list[str]] | None:
424+
"""Resolve a pager ``argv`` to an absolute command path and its parameters.
425+
426+
The command is looked up with :func:`shutil.which`, which the
427+
:mod:`subprocess` docs recommend on every platform for an unqualified name:
428+
https://docs.python.org/3/library/subprocess.html#popen-constructor
429+
430+
Returns ``None`` when there is no command to run or it is not on the path,
431+
leaving the caller to pick a fallback.
432+
"""
433+
if not cmd_parts:
434+
return None
435+
436+
import shutil
437+
438+
cmd_filepath = shutil.which(cmd_parts[0])
439+
440+
if not cmd_filepath:
441+
return None
442+
443+
# Normalize to an absolute path without resolving symlinks: multi-call
444+
# binaries such as busybox derive their identity from the link they are
445+
# invoked through, so resolving less -> busybox makes them misbehave.
446+
# See https://github.com/pallets/click/issues/2943 and
447+
# https://github.com/pallets/click/pull/2944
448+
return Path(cmd_filepath).absolute(), cmd_parts[1:]
449+
450+
423451
def _pager_contextmanager(
424452
color: bool | None = None,
425-
) -> t.ContextManager[tuple[t.TextIO, bool]]:
453+
) -> t.ContextManager[tuple[t.TextIO, bool | None]]:
426454
"""Decide what method to use for paging through text."""
427455
stdout = _default_text_stdout()
428456

@@ -439,16 +467,30 @@ def _pager_contextmanager(
439467
# Non-POSIX mode retains quotes in tokens, and wrapping tokens
440468
# with shlex.quote re-introduces quoting issues on Windows.
441469
pager_cmd_parts = shlex.split(os.environ.get("PAGER", ""))
470+
442471
if pager_cmd_parts:
443-
if WIN:
444-
return _tempfilepager(pager_cmd_parts, color)
445-
return _pipepager(pager_cmd_parts, color)
472+
# Piping to `more` on Windows adds spurious \r\n, so it gets the temp
473+
# file strategy whatever the user asked for.
474+
use_tempfile = WIN
475+
else:
476+
if os.environ.get("TERM") in ("dumb", "emacs"):
477+
return _nullpager(stdout, color)
446478

447-
if os.environ.get("TERM") in ("dumb", "emacs"):
479+
use_tempfile = WIN or sys.platform.startswith("os2")
480+
pager_cmd_parts = ["more"] if use_tempfile else ["less"]
481+
482+
resolved = _resolve_pager_command(pager_cmd_parts)
483+
484+
if resolved is None:
485+
# Page through stdout, which _nullpager leaves open for its owner.
448486
return _nullpager(stdout, color)
449-
if WIN or sys.platform.startswith("os2"):
450-
return _tempfilepager(["more"], color)
451-
return _pipepager(["less"], color)
487+
488+
cmd_path, cmd_params = resolved
489+
490+
if use_tempfile:
491+
return _tempfilepager(cmd_path, color)
492+
493+
return _pipepager(cmd_path, cmd_params, color)
452494

453495

454496
@contextlib.contextmanager
@@ -464,9 +506,11 @@ def get_pager_file(color: bool | None = None) -> t.Generator[t.TextIO, None, Non
464506
"""
465507
with _pager_contextmanager(color=color) as (stream, color):
466508
# Every pager strategy yields a text stream, so the only thing left to
467-
# do is strip ANSI styling when colors are disabled. The wrapper does
468-
# not close the stream: each strategy owns its stream's lifecycle.
469-
writer = _PagerWriter(stream, color=color)
509+
# do is strip ANSI styling when colors are disabled. A strategy yields
510+
# None when it has no opinion on colors, which settles as off here. The
511+
# wrapper does not close the stream: each strategy owns its stream's
512+
# lifecycle.
513+
writer = _PagerWriter(stream, color=bool(color))
470514
try:
471515
yield t.cast(t.TextIO, writer)
472516
finally:
@@ -475,66 +519,31 @@ def get_pager_file(color: bool | None = None) -> t.Generator[t.TextIO, None, Non
475519

476520
@contextlib.contextmanager
477521
def _pipepager(
478-
cmd_parts: list[str], color: bool | None = None
479-
) -> t.Iterator[tuple[t.TextIO, bool]]:
522+
cmd_path: Path, cmd_params: list[str], color: bool | None = None
523+
) -> t.Iterator[tuple[t.TextIO, bool | None]]:
480524
"""Page through text by feeding it to another program.
481525
482-
Invokes the pager via :class:`subprocess.Popen` with an ``argv`` list
483-
produced by :func:`shlex.split`. The command is resolved to an absolute
484-
path with :func:`shutil.which` as recommended by the
485-
:mod:`subprocess` docs for Windows compatibility.
526+
Invokes the pager via :class:`subprocess.Popen` with an ``argv`` list.
486527
487528
Invoking a pager through this might support colors: if piping to
488529
``less`` and the user hasn't decided on colors, ``LESS=-R`` is set
489530
automatically.
490531
"""
491-
# Split the command into the invoked CLI and its parameters.
492-
if not cmd_parts:
493-
# No usable pager: fall back to stdout through _nullpager so it gets the
494-
# same borrowed-stream handling and the caller's stream is not closed.
495-
stdout = _default_text_stdout() or StringIO()
496-
with _nullpager(stdout, color) as rv:
497-
yield rv
498-
return
499-
500-
import shutil
501-
502-
cmd = cmd_parts[0]
503-
cmd_params = cmd_parts[1:]
504-
505-
cmd_filepath = shutil.which(cmd)
506-
if not cmd_filepath:
507-
# No usable pager: fall back to stdout through _nullpager so it gets the
508-
# same borrowed-stream handling and the caller's stream is not closed.
509-
stdout = _default_text_stdout() or StringIO()
510-
with _nullpager(stdout, color) as rv:
511-
yield rv
512-
return
513-
514-
# Produces a normalized absolute path string.
515-
# multi-call binaries such as busybox derive their identity from the symlink
516-
# less -> busybox. resolve() causes them to misbehave. (eg. less becomes busybox)
517-
cmd_path = Path(cmd_filepath).absolute()
518-
cmd_name = cmd_path.name
519-
520532
import subprocess
521533

522534
# Make a local copy of the environment to not affect the global one.
523535
env = dict(os.environ)
524536

525537
# If we're piping to less and the user hasn't decided on colors, we enable
526538
# them by default we find the -R flag in the command line arguments.
527-
if color is None and cmd_name == "less":
539+
if color is None and cmd_path.name == "less":
528540
less_flags = f"{os.environ.get('LESS', '')}{' '.join(cmd_params)}"
529541
if not less_flags:
530542
env["LESS"] = "-R"
531543
color = True
532544
elif "r" in less_flags or "R" in less_flags:
533545
color = True
534546

535-
if color is None:
536-
color = False
537-
538547
c = subprocess.Popen(
539548
[str(cmd_path)] + cmd_params,
540549
shell=False,
@@ -586,48 +595,19 @@ def _pipepager(
586595

587596
@contextlib.contextmanager
588597
def _tempfilepager(
589-
cmd_parts: list[str], color: bool | None = None
590-
) -> t.Iterator[tuple[t.TextIO, bool]]:
598+
cmd_path: Path, color: bool | None = None
599+
) -> t.Iterator[tuple[t.TextIO, bool | None]]:
591600
"""Page through text by invoking a program on a temporary file.
592601
593602
Used as the primary pager strategy on Windows (where piping to
594603
``more`` adds spurious ``\\r\\n``), and as a fallback on other
595-
platforms. The command is resolved to an absolute path with
596-
:func:`shutil.which`.
604+
platforms. The command is invoked with the temporary file as its only
605+
argument: any parameters the user set in ``PAGER`` are not passed on.
597606
"""
598-
# Split the command into the invoked CLI and its parameters.
599-
if not cmd_parts:
600-
# No usable pager: fall back to stdout through _nullpager so it gets the
601-
# same borrowed-stream handling and the caller's stream is not closed.
602-
stdout = _default_text_stdout() or StringIO()
603-
with _nullpager(stdout, color) as rv:
604-
yield rv
605-
return
606-
607-
import shutil
608607
import subprocess
609-
610-
cmd = cmd_parts[0]
611-
612-
cmd_filepath = shutil.which(cmd)
613-
if not cmd_filepath:
614-
# No usable pager: fall back to stdout through _nullpager so it gets the
615-
# same borrowed-stream handling and the caller's stream is not closed.
616-
stdout = _default_text_stdout() or StringIO()
617-
with _nullpager(stdout, color) as rv:
618-
yield rv
619-
return
620-
621-
# Produces a normalized absolute path string.
622-
# multi-call binaries such as busybox derive their identity from the symlink
623-
# less -> busybox. resolve() causes them to misbehave. (eg. less becomes busybox)
624-
cmd_path = Path(cmd_filepath).absolute()
625-
626608
import tempfile
627609

628610
encoding = get_best_encoding(sys.stdout)
629-
if color is None:
630-
color = False
631611
# On Windows, NamedTemporaryFile cannot be opened by another process
632612
# while Python still has it open, so we use delete=False and clean up manually
633613
# rather than using a contextmanager here.
@@ -651,16 +631,13 @@ def _tempfilepager(
651631
@contextlib.contextmanager
652632
def _nullpager(
653633
stream: t.TextIO, color: bool | None = None
654-
) -> t.Iterator[tuple[t.TextIO, bool]]:
634+
) -> t.Iterator[tuple[t.TextIO, bool | None]]:
655635
"""Simply print unformatted text. This is the ultimate fallback.
656636
657637
The stream comes from elsewhere (typically ``sys.stdout``), so its lifecycle
658638
is left untouched: :class:`_PagerWriter` never closes what it wraps, and it
659639
is the only thing :func:`get_pager_file` hands to the caller.
660640
"""
661-
if color is None:
662-
color = False
663-
664641
yield stream, color
665642

666643

0 commit comments

Comments
 (0)