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

login
register
mail settings
Submitter matthieu.laneuville@octobus.net
Date Sept. 4, 2017, 12:22 p.m.
Message ID <d892e64bf89c7f3e7903.1504527756@carbon>
Download mbox | patch
Permalink /patch/23653/
State Changes Requested
Headers show

Comments

matthieu.laneuville@octobus.net - Sept. 4, 2017, 12:22 p.m.
# HG changeset patch
# User Matthieu Laneuville <matthieu.laneuville@octobus.net>
# Date 1504454402 -7200
#      Sun Sep 03 18:00:02 2017 +0200
# Node ID d892e64bf89c7f3e79039c5a2d1bd051b32eb686
# Parent  b2eb0aa445cbe7cadc8b7691c6266908adfc5057
# EXP-Topic issue4746
posix: update os.popen with subprocess (issue4746)

'hg import' can be configured to use a custom patch tool via the 'ui.patch'
option. However, it sometimes claims that the exit status of the tool is
different from what it really is.

posix.explainexit() was rewritten for the subprocess module, but
posix.popen() still calls os.popen(). This patch fixes the issue by replacing
the deprecated os.popen() with subprocess.Popen() in posix.popen().
Yuya Nishihara - Sept. 4, 2017, 1:23 p.m.
On Mon, 04 Sep 2017 14:22:36 +0200, matthieu.laneuville@octobus.net wrote:
> # HG changeset patch
> # User Matthieu Laneuville <matthieu.laneuville@octobus.net>
> # Date 1504454402 -7200
> #      Sun Sep 03 18:00:02 2017 +0200
> # Node ID d892e64bf89c7f3e79039c5a2d1bd051b32eb686
> # Parent  b2eb0aa445cbe7cadc8b7691c6266908adfc5057
> # EXP-Topic issue4746
> posix: update os.popen with subprocess (issue4746)

> --- a/mercurial/posix.py	Tue Aug 22 21:21:43 2017 -0400
> +++ b/mercurial/posix.py	Sun Sep 03 18:00:02 2017 +0200

Windows?

>  def popen(command, mode='r'):
> -    return os.popen(command, mode)
> +    if 'r' in mode:
> +        return subprocess.Popen(command, shell=True, stdout=subprocess.PIPE)
> +    elif 'w' in mode:
> +        return subprocess.Popen(command, shell=True, stdin=subprocess.PIPE)
> +    else:
> +        raise ValueError

So this isn't popen(). It's probably better to rename the function.

http://mercurial.markmail.org/message/q5wpzw5g4lzmmbpb
matthieu.laneuville@octobus.net - Sept. 5, 2017, 1:52 p.m.
I don't have access to a windows machine to test a potential patch,
what do you suggest? I found a very similar function in windows.py so
the exact same modification there may work as well, but I can't test
it.

Regarding the name, is there a good naming convention to follow? Should
I use popen_wrapper() for instance?

Thank you for the review.
Matthieu

On Mon, 2017-09-04 at 22:23 +0900, Yuya Nishihara wrote:
> On Mon, 04 Sep 2017 14:22:36 +0200, matthieu.laneuville@octobus.net
> wrote:
> > # HG changeset patch
> > # User Matthieu Laneuville <matthieu.laneuville@octobus.net>
> > # Date 1504454402 -7200
> > #      Sun Sep 03 18:00:02 2017 +0200
> > # Node ID d892e64bf89c7f3e79039c5a2d1bd051b32eb686
> > # Parent  b2eb0aa445cbe7cadc8b7691c6266908adfc5057
> > # EXP-Topic issue4746
> > posix: update os.popen with subprocess (issue4746)
> > --- a/mercurial/posix.py	Tue Aug 22 21:21:43 2017 -0400
> > +++ b/mercurial/posix.py	Sun Sep 03 18:00:02 2017 +0200
> 
> Windows?
> 
> >  def popen(command, mode='r'):
> > -    return os.popen(command, mode)
> > +    if 'r' in mode:
> > +        return subprocess.Popen(command, shell=True,
> > stdout=subprocess.PIPE)
> > +    elif 'w' in mode:
> > +        return subprocess.Popen(command, shell=True,
> > stdin=subprocess.PIPE)
> > +    else:
> > +        raise ValueError
> 
> So this isn't popen(). It's probably better to rename the function.
> 
> http://mercurial.markmail.org/message/q5wpzw5g4lzmmbpb
Yuya Nishihara - Sept. 5, 2017, 2:43 p.m.
On Tue, 05 Sep 2017 15:52:44 +0200, Matthieu Laneuville wrote:
> I don't have access to a windows machine to test a potential patch,
> what do you suggest? I found a very similar function in windows.py so
> the exact same modification there may work as well, but I can't test
> it.

Perhaps it can be moved to util.py.

FWIW, most devs don't have Windows machine. I just pointed out that because
you've changed the interface of util.popen() only in posix.py, which would
be most likely to break things on Windows.

> Regarding the name, is there a good naming convention to follow? Should
> I use popen_wrapper() for instance?

Naming is hard. I can't think of any nicer name than subproc() or pipeproc().
What I don't like about the name "popen()" is it's completely different from
the real popen(), which returns a file-like object.

Patch

diff -r b2eb0aa445cb -r d892e64bf89c mercurial/mail.py
--- a/mercurial/mail.py	Tue Aug 22 21:21:43 2017 -0400
+++ b/mercurial/mail.py	Sun Sep 03 18:00:02 2017 +0200
@@ -139,9 +139,11 @@ 
     cmdline = '%s -f %s %s' % (program, util.email(sender),
                                ' '.join(map(util.email, recipients)))
     ui.note(_('sending mail: %s\n') % cmdline)
-    fp = util.popen(cmdline, 'w')
+    f = util.popen(cmdline, 'w')
+    fp = f.stdin
     fp.write(msg)
-    ret = fp.close()
+    fp.close()
+    ret = f.wait()
     if ret:
         raise error.Abort('%s %s' % (
             os.path.basename(program.split(None, 1)[0]),
diff -r b2eb0aa445cb -r d892e64bf89c mercurial/patch.py
--- a/mercurial/patch.py	Tue Aug 22 21:21:43 2017 -0400
+++ b/mercurial/patch.py	Sun Sep 03 18:00:02 2017 +0200
@@ -2087,8 +2087,9 @@ 
     cwd = repo.root
     if cwd:
         args.append('-d %s' % util.shellquote(cwd))
-    fp = util.popen('%s %s -p%d < %s' % (patcher, ' '.join(args), strip,
-                                       util.shellquote(patchname)))
+    f = util.popen('%s %s -p%d < %s' % (patcher, ' '.join(args), strip,
+                                        util.shellquote(patchname)))
+    fp = f.stdout
     try:
         for line in util.iterfile(fp):
             line = line.rstrip()
@@ -2113,7 +2114,7 @@ 
     finally:
         if files:
             scmutil.marktouched(repo, files, similarity)
-    code = fp.close()
+    code = f.wait()
     if code:
         raise PatchError(_("patch command failed: %s") %
                          util.explainexit(code)[0])
diff -r b2eb0aa445cb -r d892e64bf89c mercurial/posix.py
--- a/mercurial/posix.py	Tue Aug 22 21:21:43 2017 -0400
+++ b/mercurial/posix.py	Sun Sep 03 18:00:02 2017 +0200
@@ -16,6 +16,7 @@ 
 import re
 import select
 import stat
+import subprocess
 import sys
 import tempfile
 import unicodedata
@@ -448,7 +449,12 @@ 
     return cmd
 
 def popen(command, mode='r'):
-    return os.popen(command, mode)
+    if 'r' in mode:
+        return subprocess.Popen(command, shell=True, stdout=subprocess.PIPE)
+    elif 'w' in mode:
+        return subprocess.Popen(command, shell=True, stdin=subprocess.PIPE)
+    else:
+        raise ValueError
 
 def testpid(pid):
     '''return False if pid dead, True if running or not sure'''
diff -r b2eb0aa445cb -r d892e64bf89c tests/test-import.t
--- a/tests/test-import.t	Tue Aug 22 21:21:43 2017 -0400
+++ b/tests/test-import.t	Sun Sep 03 18:00:02 2017 +0200
@@ -1810,3 +1810,12 @@ 
   a not tracked!
   abort: source file 'a' does not exist
   [255]
+
+correct exit status should be provided when using external tool (issue4746)
+
+  $ hg import $TESTTMP/foo.patch --config ui.patch="patch --bogus-option"
+  patch: unrecognized option '--bogus-option'
+  patch: Try 'patch --help' for more information.
+  applying $TESTTMP/foo.patch
+  abort: patch command failed: exited with status 2
+  [255]