pydoc: Use of MANPAGER variable is incorrect

[RPigott]

BPO	39400
Nosy	@serhiy-storchaka, @rpigott
PRs	gh-83581: Do not pass MANPAGER value to shell in pydoc #18123

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2020-01-20.21:02:23.588>
labels = ['type-feature', 'library', '3.9']
title = 'pydoc: Use of MANPAGER variable is incorrect'
updated_at = <Date 2020-02-03.18:34:42.431>
user = 'https://github.com/RPigott'

bugs.python.org fields:

activity = <Date 2020-02-03.18:34:42.431>
actor = 'Brocellous'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2020-01-20.21:02:23.588>
creator = 'Brocellous'
dependencies = []
files = []
hgrepos = []
issue_num = 39400
keywords = ['patch']
message_count = 3.0
messages = ['360332', '360336', '361318']
nosy_count = 2.0
nosy_names = ['serhiy.storchaka', 'Brocellous']
pr_nums = ['18123']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue39400'
versions = ['Python 3.9']

[RPigott]

pydoc references the value of both MANPAGER and PAGER variables when selecting a command to present the user with documentation. Those values are passed directly to subprocess.Popen. However, MANPAGER may contain arguments that need splitting, and is explicitly documented as such in the man 1 man manpage:

If $MANPAGER or $PAGER is set ($MANPAGER is used in preference), its value is used as the name of the program used to display the manual page. [...] The value may be a simple command name or a command with arguments, and may use shell quoting (backslashes, single quotes, or double quotes). It may not use pipes to connect multiple commands; if you need that, use a wrapper script [...]

pydoc should perform word splitting a la shlex.split on the values of MANPAGER and PAGER to retain compatibility with man.

[RPigott]

Actually my previous comment is incorrect, the value of the PAGER is split with the shell as a result of shell=True. There _is_ still an incompatibility with man.

I have a value for MANPAGER set like 'env LESS_TERMCAP_mb=^[[01;31m ... less' which becomes split on ';' by the shell as a result of 'shell=True'. The MANPAGER value is not interpreted by a shell with man, so it works as expected when using man.

Becuase man explicitly allows the use of quotes, it is possible to set a value of MANPAGER that has the desired effect for both programs by quoting the terminal control sequences like so: 'env LESS="^[[01;31m ... less"'.

Still, it might be reasonable to change pydocs interpretation of MANPAGER to better align with man's.

[RPigott]

Any reason a change like this could not be added to a 3.8.x release?

Like I pointed out in the PR, pydoc's use of MANPAGER is undocumented and, as far as I'm concerned, incorrect.

A lot of software references idiosyncratic <PROGRAM>_PAGER vars, but pydoc reuses MANPAGER instead of something like PYDOC_PAGER. I don't think it's such a bad idea to reuse MANPAGER considering the two programs serve a similar function, but I think matching man's interpretation of MANPAGER is then the only reasonable behavior pydoc can take. If it does not, pydoc is by-default broken on plenty of otherwise fine setups.

As it stands, pydoc is broken _because_ of undocumented behavior. A user reading the docs carefully still might not recognize that pydoc is preferring their MANPAGER value over PAGER. For that reason, I'd call this a bugfix. The bug here could be considered that pydoc references MANPAGER _at all_. However, given what I think is the intent of the original code - to allow pydoc to benefit from a user's MANPAGER customization - I've tried to update pydoc to reflect that intent in the PR, rather than remove the feature.

I think this change could be eligible for 3.8.x. Thoughts?