Mercurial > public > mercurial-scm > hg-stable
comparison mercurial/ui.py @ 31485:9335dc6b2a9c
pager: avoid shell=True on subprocess.Popen for better errors (issue5491)
man(1) behaves as poorly as Mercurial without this change. This cribs
from git's run-command[0], which has a list of characters that imply a
string that needs to be run using 'sh -c'. If none of those characters
are present in the command string, we can use shell=False mode on
subprocess and get significantly better error messages (see the test)
when the pager process is invalid. With a complicated pager command
(that contains one of the unsafe characters), we behave as we do today
(which is no worse than git manages.)
I briefly tried tapdancing in a thread to catch early pager exits, but
it's just too perilous: you get races between fd duping operations and
a bad pager exiting, and it's too hard to differentiate between a
slow-bad-pager result and a fast-human-quit-pager-early result.
I've observed some weird variation in exit code handling in the "bad
experience" case in test-pager.t: on my Mac hg predictably exits
nonzero, but on Linux hg always exits zero in that case. For now,
we'll work around it with || true. :(
0: https://github.com/git/git/blob/cddbda4bc87b9d2c985b6749b1cf026b15e2d3e7/run-command.c#L201
| author | Augie Fackler <augie@google.com> |
|---|---|
| date | Wed, 15 Mar 2017 20:33:47 -0400 |
| parents | 75e4bae56068 |
| children | 96929bd6e58d |
comparison
equal
deleted
inserted
replaced
| 31484:3fb2081ef896 | 31485:9335dc6b2a9c |
|---|---|
| 933 """Actually start the pager and set up file descriptors. | 933 """Actually start the pager and set up file descriptors. |
| 934 | 934 |
| 935 This is separate in part so that extensions (like chg) can | 935 This is separate in part so that extensions (like chg) can |
| 936 override how a pager is invoked. | 936 override how a pager is invoked. |
| 937 """ | 937 """ |
| 938 pager = subprocess.Popen(command, shell=True, bufsize=-1, | 938 # If the command doesn't contain any of these characters, we |
| 939 close_fds=util.closefds, stdin=subprocess.PIPE, | 939 # assume it's a binary and exec it directly. This means for |
| 940 stdout=util.stdout, stderr=util.stderr) | 940 # simple pager command configurations, we can degrade |
| 941 # gracefully and tell the user about their broken pager. | |
| 942 shell = any(c in command for c in "|&;<>()$`\\\"' \t\n*?[#~=%") | |
| 943 try: | |
| 944 pager = subprocess.Popen( | |
| 945 command, shell=shell, bufsize=-1, | |
| 946 close_fds=util.closefds, stdin=subprocess.PIPE, | |
| 947 stdout=util.stdout, stderr=util.stderr) | |
| 948 except OSError as e: | |
| 949 if e.errno == errno.ENOENT and not shell: | |
| 950 self.warn(_("missing pager command '%s', skipping pager\n") | |
| 951 % command) | |
| 952 return | |
| 953 raise | |
| 941 | 954 |
| 942 # back up original file descriptors | 955 # back up original file descriptors |
| 943 stdoutfd = os.dup(util.stdout.fileno()) | 956 stdoutfd = os.dup(util.stdout.fileno()) |
| 944 stderrfd = os.dup(util.stderr.fileno()) | 957 stderrfd = os.dup(util.stderr.fileno()) |
| 945 | 958 |