Patchwork [6,of,6,V3] util.system: rename to rawsystem in favor of ui.system()

login
register
mail settings
Submitter Yuya Nishihara
Date Nov. 12, 2014, 2:38 p.m.
Message ID <18c87a673070fbce7884.1415803085@mimosa>
Download mbox | patch
Permalink /patch/6691/
State Rejected
Headers show

Comments

Yuya Nishihara - Nov. 12, 2014, 2:38 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1415799589 -32400
#      Wed Nov 12 22:39:49 2014 +0900
# Node ID 18c87a673070fbce78849c3beced2b9d14835abe
# Parent  b6944cd4197b1bb1d1fd9e2d6f94453d297c3f03
util.system: rename to rawsystem in favor of ui.system()

This forces third-party extensions to migrate to ui.system(). util.rawsystem()
is unsafe in command server.
Pierre-Yves David - Nov. 12, 2014, 2:43 p.m.
On 11/12/2014 02:38 PM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1415799589 -32400
> #      Wed Nov 12 22:39:49 2014 +0900
> # Node ID 18c87a673070fbce78849c3beced2b9d14835abe
> # Parent  b6944cd4197b1bb1d1fd9e2d6f94453d297c3f03
> util.system: rename to rawsystem in favor of ui.system()

This needs an (API) flag
Pierre-Yves David - Nov. 12, 2014, 5:56 p.m.
On 11/12/2014 02:38 PM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1415799589 -32400
> #      Wed Nov 12 22:39:49 2014 +0900
> # Node ID 18c87a673070fbce78849c3beced2b9d14835abe
> # Parent  b6944cd4197b1bb1d1fd9e2d6f94453d297c3f03
> util.system: rename to rawsystem in favor of ui.system()
>
> This forces third-party extensions to migrate to ui.system(). util.rawsystem()
> is unsafe in command server.

So this needs an (API) flag to be properly included in  3.3 changelog.

> -def system(cmd, environ={}, cwd=None, onerr=None, errprefix=None, out=None):
> +def rawsystem(cmd, environ={}, cwd=None, onerr=None, errprefix=None, out=None):
>       '''enhanced shell command execution.
>       run with environment maybe modified, maybe in different dir.
>
> @@ -623,7 +623,10 @@ def system(cmd, environ={}, cwd=None, on
>       object as exception.
>
>       if out is specified, it is assumed to be a file-like object that has a
> -    write() method. stdout and stderr will be redirected to out.'''
> +    write() method. stdout and stderr will be redirected to out.
> +
> +    use ui.system() unless you want to redirect output to the specified file.
> +    '''

Can we make a reference to ui.system here? This is probably a layer 
violation but this would clarify thing a alot.
Matt Mackall - Nov. 12, 2014, 10:43 p.m.
On Wed, 2014-11-12 at 23:38 +0900, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1415799589 -32400
> #      Wed Nov 12 22:39:49 2014 +0900
> # Node ID 18c87a673070fbce78849c3beced2b9d14835abe
> # Parent  b6944cd4197b1bb1d1fd9e2d6f94453d297c3f03
> util.system: rename to rawsystem in favor of ui.system()
> 
> This forces third-party extensions to migrate to ui.system(). util.rawsystem()
> is unsafe in command server.

Wouldn't it be better for the commandserver to modify sys.stdout?
Yuya Nishihara - Nov. 13, 2014, 1:43 p.m.
On Wed, 12 Nov 2014 16:43:02 -0600, Matt Mackall wrote:
> On Wed, 2014-11-12 at 23:38 +0900, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1415799589 -32400
> > #      Wed Nov 12 22:39:49 2014 +0900
> > # Node ID 18c87a673070fbce78849c3beced2b9d14835abe
> > # Parent  b6944cd4197b1bb1d1fd9e2d6f94453d297c3f03
> > util.system: rename to rawsystem in favor of ui.system()
> > 
> > This forces third-party extensions to migrate to ui.system(). util.rawsystem()
> > is unsafe in command server.
> 
> Wouldn't it be better for the commandserver to modify sys.stdout?

We did introduce ui.fin and fout in order to avoid the monkeypatching.

http://thread.gmane.org/gmane.comp.version-control.mercurial.devel/42230
http://thread.gmane.org/gmane.comp.version-control.mercurial.devel/42131/focus=42140

BTW, we can make "out" parameter mandatory instead of renaming the function.

-def system(cmd, environ={}, cwd=None, onerr=None, errprefix=None, out=None):
+def system(cmd, out, environ={}, cwd=None, onerr=None, errprefix=None):
     '''enhanced shell command execution.
     run with environment maybe modified, maybe in different dir.
 
     if command fails and onerr is None, return status, else raise onerr
     object as exception.
 
-    if out is specified, it is assumed to be a file-like object that has a
-    write() method. stdout and stderr will be redirected to out.'''
+    out must be a file-like object that has a write() method. stdout and
+    stderr will be redirected to out. use ui.system() unless you want to
+    redirect output to the specified file.
+    '''

Regards,
Matt Mackall - Nov. 13, 2014, 7:34 p.m.
On Thu, 2014-11-13 at 22:43 +0900, Yuya Nishihara wrote:
> On Wed, 12 Nov 2014 16:43:02 -0600, Matt Mackall wrote:
> > On Wed, 2014-11-12 at 23:38 +0900, Yuya Nishihara wrote:
> > > # HG changeset patch
> > > # User Yuya Nishihara <yuya@tcha.org>
> > > # Date 1415799589 -32400
> > > #      Wed Nov 12 22:39:49 2014 +0900
> > > # Node ID 18c87a673070fbce78849c3beced2b9d14835abe
> > > # Parent  b6944cd4197b1bb1d1fd9e2d6f94453d297c3f03
> > > util.system: rename to rawsystem in favor of ui.system()
> > > 
> > > This forces third-party extensions to migrate to ui.system(). util.rawsystem()
> > > is unsafe in command server.
> > 
> > Wouldn't it be better for the commandserver to modify sys.stdout?
> 
> We did introduce ui.fin and fout in order to avoid the monkeypatching.

Yep, but it doesn't seem to me that strategy has been the most
successful... because lots of things below the ui layer (including
Python libs) might decide to bypass it.
Pierre-Yves David - Nov. 13, 2014, 11:09 p.m.
On 11/13/2014 07:34 PM, Matt Mackall wrote:
> On Thu, 2014-11-13 at 22:43 +0900, Yuya Nishihara wrote:
>> On Wed, 12 Nov 2014 16:43:02 -0600, Matt Mackall wrote:
>>> On Wed, 2014-11-12 at 23:38 +0900, Yuya Nishihara wrote:
>>>> # HG changeset patch
>>>> # User Yuya Nishihara <yuya@tcha.org>
>>>> # Date 1415799589 -32400
>>>> #      Wed Nov 12 22:39:49 2014 +0900
>>>> # Node ID 18c87a673070fbce78849c3beced2b9d14835abe
>>>> # Parent  b6944cd4197b1bb1d1fd9e2d6f94453d297c3f03
>>>> util.system: rename to rawsystem in favor of ui.system()
>>>>
>>>> This forces third-party extensions to migrate to ui.system(). util.rawsystem()
>>>> is unsafe in command server.
>>>
>>> Wouldn't it be better for the commandserver to modify sys.stdout?
>>
>> We did introduce ui.fin and fout in order to avoid the monkeypatching.
>
> Yep, but it doesn't seem to me that strategy has been the most
> successful... because lots of things below the ui layer (including
> Python libs) might decide to bypass it.

In my vague memory, various python stuff tend to bypass sys.stdout too. 
However:
- It is a vague memory
- It is still less stuff than the ones who bypass ui.
Yuya Nishihara - Nov. 14, 2014, 1:04 p.m.
On Thu, 13 Nov 2014 23:09:18 +0000, Pierre-Yves David wrote:
> On 11/13/2014 07:34 PM, Matt Mackall wrote:
> > On Thu, 2014-11-13 at 22:43 +0900, Yuya Nishihara wrote:
> >> On Wed, 12 Nov 2014 16:43:02 -0600, Matt Mackall wrote:
> >>> On Wed, 2014-11-12 at 23:38 +0900, Yuya Nishihara wrote:
> >>>> # HG changeset patch
> >>>> # User Yuya Nishihara <yuya@tcha.org>
> >>>> # Date 1415799589 -32400
> >>>> #      Wed Nov 12 22:39:49 2014 +0900
> >>>> # Node ID 18c87a673070fbce78849c3beced2b9d14835abe
> >>>> # Parent  b6944cd4197b1bb1d1fd9e2d6f94453d297c3f03
> >>>> util.system: rename to rawsystem in favor of ui.system()
> >>>>
> >>>> This forces third-party extensions to migrate to ui.system(). util.rawsystem()
> >>>> is unsafe in command server.
> >>>
> >>> Wouldn't it be better for the commandserver to modify sys.stdout?
> >>
> >> We did introduce ui.fin and fout in order to avoid the monkeypatching.
> >
> > Yep, but it doesn't seem to me that strategy has been the most
> > successful... because lots of things below the ui layer (including
> > Python libs) might decide to bypass it.
> 
> In my vague memory, various python stuff tend to bypass sys.stdout too. 
> However:
> - It is a vague memory
> - It is still less stuff than the ones who bypass ui.

stdc functions such as system() won't use sys.stdout.

IMHO, if we really want to protect command-server channels from bad extensions
or in-process hooks, we should redirect STDOUT to /dev/null.

  nullfd = os.open(os.devnull)
  foutfd = os.dup(1)
  os.dup2(nullfd, 1)
  os.close(nullfd)
  fout = os.fdopen(foutfd)  # only commandserver can use it directly

Regards,
Matt Mackall - Nov. 14, 2014, 4:06 p.m.
On Fri, 2014-11-14 at 22:04 +0900, Yuya Nishihara wrote:
> On Thu, 13 Nov 2014 23:09:18 +0000, Pierre-Yves David wrote:
> > On 11/13/2014 07:34 PM, Matt Mackall wrote:
> > > On Thu, 2014-11-13 at 22:43 +0900, Yuya Nishihara wrote:
> > >> On Wed, 12 Nov 2014 16:43:02 -0600, Matt Mackall wrote:
> > >>> On Wed, 2014-11-12 at 23:38 +0900, Yuya Nishihara wrote:
> > >>>> # HG changeset patch
> > >>>> # User Yuya Nishihara <yuya@tcha.org>
> > >>>> # Date 1415799589 -32400
> > >>>> #      Wed Nov 12 22:39:49 2014 +0900
> > >>>> # Node ID 18c87a673070fbce78849c3beced2b9d14835abe
> > >>>> # Parent  b6944cd4197b1bb1d1fd9e2d6f94453d297c3f03
> > >>>> util.system: rename to rawsystem in favor of ui.system()
> > >>>>
> > >>>> This forces third-party extensions to migrate to ui.system(). util.rawsystem()
> > >>>> is unsafe in command server.
> > >>>
> > >>> Wouldn't it be better for the commandserver to modify sys.stdout?
> > >>
> > >> We did introduce ui.fin and fout in order to avoid the monkeypatching.
> > >
> > > Yep, but it doesn't seem to me that strategy has been the most
> > > successful... because lots of things below the ui layer (including
> > > Python libs) might decide to bypass it.
> > 
> > In my vague memory, various python stuff tend to bypass sys.stdout too. 
> > However:
> > - It is a vague memory
> > - It is still less stuff than the ones who bypass ui.
> 
> stdc functions such as system() won't use sys.stdout.
> 
> IMHO, if we really want to protect command-server channels from bad extensions
> or in-process hooks, we should redirect STDOUT to /dev/null.
> 
>   nullfd = os.open(os.devnull)
>   foutfd = os.dup(1)
>   os.dup2(nullfd, 1)
>   os.close(nullfd)
>   fout = os.fdopen(foutfd)  # only commandserver can use it directly

+1.

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -830,8 +830,8 @@  class ui(object):
         '''execute shell command with appropriate output stream. command
         output will be redirected if fout is not stdout.
         '''
-        return util.system(cmd, environ=environ, cwd=cwd, onerr=onerr,
-                           errprefix=errprefix, out=self.fout)
+        return util.rawsystem(cmd, environ=environ, cwd=cwd, onerr=onerr,
+                              errprefix=errprefix, out=self.fout)
 
     def traceback(self, exc=None, force=False):
         '''print exception traceback if traceback printing enabled or forced.
diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -615,7 +615,7 @@  def _sethgexecutable(path):
     global _hgexecutable
     _hgexecutable = path
 
-def system(cmd, environ={}, cwd=None, onerr=None, errprefix=None, out=None):
+def rawsystem(cmd, environ={}, cwd=None, onerr=None, errprefix=None, out=None):
     '''enhanced shell command execution.
     run with environment maybe modified, maybe in different dir.
 
@@ -623,7 +623,10 @@  def system(cmd, environ={}, cwd=None, on
     object as exception.
 
     if out is specified, it is assumed to be a file-like object that has a
-    write() method. stdout and stderr will be redirected to out.'''
+    write() method. stdout and stderr will be redirected to out.
+
+    use ui.system() unless you want to redirect output to the specified file.
+    '''
     try:
         sys.stdout.flush()
     except Exception:
diff --git a/tests/test-demandimport.py b/tests/test-demandimport.py
--- a/tests/test-demandimport.py
+++ b/tests/test-demandimport.py
@@ -20,9 +20,9 @@  print "os =", f(os)
 from mercurial import util
 
 print "util =", f(util)
-print "util.system =", f(util.system)
+print "util.rawsystem =", f(util.rawsystem)
 print "util =", f(util)
-print "util.system =", f(util.system)
+print "util.rawsystem =", f(util.rawsystem)
 
 import re as fred
 print "fred =", f(fred)
diff --git a/tests/test-demandimport.py.out b/tests/test-demandimport.py.out
--- a/tests/test-demandimport.py.out
+++ b/tests/test-demandimport.py.out
@@ -2,9 +2,9 @@  os = <unloaded module 'os'>
 os.system = <built-in function system>
 os = <module 'os' from '?'>
 util = <unloaded module 'util'>
-util.system = <function system at 0x?>
+util.rawsystem = <function rawsystem at 0x?>
 util = <module 'mercurial.util' from '?'>
-util.system = <function system at 0x?>
+util.rawsystem = <function rawsystem at 0x?>
 fred = <unloaded module 're'>
 re = <unloaded module 'sys'>
 fred = <unloaded module 're'>