Patchwork [V2] posix: update os.popen with subprocess (issue4746)

login
register
mail settings
Submitter Rishabh Madan
Date March 3, 2017, 7:50 a.m.
Message ID <a134680eec2421b86d7f.1488527453@bunty>
Download mbox | patch
Permalink /patch/18882/
State Superseded
Headers show

Comments

Rishabh Madan - March 3, 2017, 7:50 a.m.
# HG changeset patch
# User Rishabh Madan <rishabhmadan96@gmail.com>
# Date 1488526994 -19800
#      Fri Mar 03 13:13:14 2017 +0530
# Node ID a134680eec2421b86d7f240b0b06947c1d777502
# Parent  71f692f1f678d86ffb4f95a3621aacfdaeb96b05
posix: update os.popen with subprocess (issue4746)

While importing a patch, we use a patch tool, say 'patch --merge', and the
patch doesn't merge cleanly, 'patch --merge' will return with exit status 1,
but 'hg import' displays 'abort: patch command failed: exited with status 256'.
The previous version of this patch replaced deprecated os.popen with
subprocess.Popen for mode='r'. But as discussed it would break popen for mode='w'.
This patch implements conditional popen for both read/write mode.
Yuya Nishihara - March 3, 2017, 2:26 p.m.
On Fri, 03 Mar 2017 13:20:53 +0530, Rishabh Madan wrote:
> # HG changeset patch
> # User Rishabh Madan <rishabhmadan96@gmail.com>
> # Date 1488526994 -19800
> #      Fri Mar 03 13:13:14 2017 +0530
> # Node ID a134680eec2421b86d7f240b0b06947c1d777502
> # Parent  71f692f1f678d86ffb4f95a3621aacfdaeb96b05
> posix: update os.popen with subprocess (issue4746)
> 
> While importing a patch, we use a patch tool, say 'patch --merge', and the
> patch doesn't merge cleanly, 'patch --merge' will return with exit status 1,
> but 'hg import' displays 'abort: patch command failed: exited with status 256'.
> The previous version of this patch replaced deprecated os.popen with
> subprocess.Popen for mode='r'. But as discussed it would break popen for mode='w'.
> This patch implements conditional popen for both read/write mode.
> 
> diff -r 71f692f1f678 -r a134680eec24 mercurial/posix.py
> --- a/mercurial/posix.py	Wed Mar 01 20:22:04 2017 +0100
> +++ b/mercurial/posix.py	Fri Mar 03 13:13:14 2017 +0530
> @@ -16,6 +16,7 @@
>  import re
>  import select
>  import stat
> +import subprocess
>  import sys
>  import tempfile
>  import unicodedata
> @@ -419,7 +420,12 @@
>      return cmd
>  
>  def popen(command, mode='r'):
> -    return os.popen(command, mode)
> +    if mode == 'r':
> +        return subprocess.Popen(command, shell=True,
> +            stdout=subprocess.PIPE).stdout
> +    else:
> +        return subprocess.Popen(command, shell=True,
> +            stdin=subprocess.PIPE).stdin

We sometimes use 'rb', so it should be "mode in ('r', 'rb')" or "'r' in mode".

% grep 'util\.popen\b' **/*.py
hgext/bugzilla.py:            fp = util.popen('(%s) 2>&1' % cmd)
hgext/convert/cvsps.py:    pfp = util.popen(' '.join(cmd))
hgext/convert/p4.py:        stdout = util.popen(cmd, mode='rb')
hgext/convert/p4.py:            clientspec = marshal.load(util.popen(cmd, mode='rb'))
hgext/convert/p4.py:                flstdout = util.popen(flcmd, mode='rb')
hgext/convert/p4.py:            stdout = util.popen(cmd, mode='rb')
hgext/convert/p4.py:        stdout = util.popen(cmd, mode='rb')
hgext/patchbomb.py:                fp = util.popen(encoding.environ['PAGER'], 'w')
mercurial/mail.py:    fp = util.popen(cmdline, 'w')
mercurial/patch.py:    fp = util.popen('%s %s -p%d < %s' % (patcher, ' '.join(args), strip,

And can you make it test 'w' explicitly and raise an error otherwise? I think
ValueError or error.ProgrammingError should be okay, though the original
os.popen() would raise OSError in such case.

Patch

diff -r 71f692f1f678 -r a134680eec24 mercurial/posix.py
--- a/mercurial/posix.py	Wed Mar 01 20:22:04 2017 +0100
+++ b/mercurial/posix.py	Fri Mar 03 13:13:14 2017 +0530
@@ -16,6 +16,7 @@ 
 import re
 import select
 import stat
+import subprocess
 import sys
 import tempfile
 import unicodedata
@@ -419,7 +420,12 @@ 
     return cmd
 
 def popen(command, mode='r'):
-    return os.popen(command, mode)
+    if mode == 'r':
+        return subprocess.Popen(command, shell=True,
+            stdout=subprocess.PIPE).stdout
+    else:
+        return subprocess.Popen(command, shell=True,
+            stdin=subprocess.PIPE).stdin
 
 def testpid(pid):
     '''return False if pid dead, True if running or not sure'''