Patchwork [1,of,3] ui: introduce util.system() wrapper to make sure ui.fout is used

login
register
mail settings
Submitter Yuya Nishihara
Date Nov. 8, 2014, 5:42 a.m.
Message ID <d83746a91dc410f60b45.1415425333@mimosa>
Download mbox | patch
Permalink /patch/6651/
State Superseded
Headers show

Comments

Yuya Nishihara - Nov. 8, 2014, 5:42 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1415419062 -32400
#      Sat Nov 08 12:57:42 2014 +0900
# Node ID d83746a91dc410f60b45916c2e55dadde4238b42
# Parent  2d54aa5397cdb1c697673ba10b7618d5ac25c69e
ui: introduce util.system() wrapper to make sure ui.fout is used

This change is intended to avoid future problem of data corruption under
command server.  out=ui.fout is mandatory as long as command server uses
stdout as IPC channel.
Pierre-Yves David - Nov. 8, 2014, 10:27 a.m.
On 11/08/2014 05:42 AM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1415419062 -32400
> #      Sat Nov 08 12:57:42 2014 +0900
> # Node ID d83746a91dc410f60b45916c2e55dadde4238b42
> # Parent  2d54aa5397cdb1c697673ba10b7618d5ac25c69e
> ui: introduce util.system() wrapper to make sure ui.fout is used

The idea looks good, but can I get you to add some docstring the new 
method so that people understand its purpose?

As far as I understand the lack of it makes thg command server usage to 
break in multiple occasion. Should this be target on stable?

Finally, Do we have a plan to force people to use ui.system instead of 
util.system?
Yuya Nishihara - Nov. 8, 2014, 2:51 p.m.
On Sat, 08 Nov 2014 10:27:47 +0000, Pierre-Yves David wrote:
> On 11/08/2014 05:42 AM, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1415419062 -32400
> > #      Sat Nov 08 12:57:42 2014 +0900
> > # Node ID d83746a91dc410f60b45916c2e55dadde4238b42
> > # Parent  2d54aa5397cdb1c697673ba10b7618d5ac25c69e
> > ui: introduce util.system() wrapper to make sure ui.fout is used
> 
> The idea looks good, but can I get you to add some docstring the new 
> method so that people understand its purpose?

will do.

> As far as I understand the lack of it makes thg command server usage to
> break in multiple occasion. Should this be target on stable?

Not for stable. This is just a sugar for util.system(..., out=ui.fout).

http://markmail.org/message/fwltwdugndmsb3g7

> Finally, Do we have a plan to force people to use ui.system instead of
> util.system?

No idea. Should we make "out" be a required parameter as well?

Regards,
Pierre-Yves David - Nov. 8, 2014, 3:24 p.m.
On 11/08/2014 02:51 PM, Yuya Nishihara wrote:
> On Sat, 08 Nov 2014 10:27:47 +0000, Pierre-Yves David wrote:
>> On 11/08/2014 05:42 AM, Yuya Nishihara wrote:
>>> # HG changeset patch
>>> # User Yuya Nishihara <yuya@tcha.org>
>>> # Date 1415419062 -32400
>>> #      Sat Nov 08 12:57:42 2014 +0900
>>> # Node ID d83746a91dc410f60b45916c2e55dadde4238b42
>>> # Parent  2d54aa5397cdb1c697673ba10b7618d5ac25c69e
>>> ui: introduce util.system() wrapper to make sure ui.fout is used
>>
>> The idea looks good, but can I get you to add some docstring the new
>> method so that people understand its purpose?
>
> will do.
>
>> As far as I understand the lack of it makes thg command server usage to
>> break in multiple occasion. Should this be target on stable?
>
> Not for stable. This is just a sugar for util.system(..., out=ui.fout).
>
> http://markmail.org/message/fwltwdugndmsb3g7
>
>> Finally, Do we have a plan to force people to use ui.system instead of
>> util.system?
>
> No idea. Should we make "out" be a required parameter as well?

How many case is there where util.system should be used over ui.system ? 
if not too much, we could maybe more util.system away (rename to 
rawsystem + add a comment pointing to ui.system?

(I'm not saying we must do it. I'm asking how reasonable it would be to 
do so)
Yuya Nishihara - Nov. 10, 2014, 1:57 p.m.
On Sat, 08 Nov 2014 15:24:29 +0000, Pierre-Yves David wrote:
> On 11/08/2014 02:51 PM, Yuya Nishihara wrote:
> > On Sat, 08 Nov 2014 10:27:47 +0000, Pierre-Yves David wrote:
> >> On 11/08/2014 05:42 AM, Yuya Nishihara wrote:
> >>> # HG changeset patch
> >>> # User Yuya Nishihara <yuya@tcha.org>
> >>> # Date 1415419062 -32400
> >>> #      Sat Nov 08 12:57:42 2014 +0900
> >>> # Node ID d83746a91dc410f60b45916c2e55dadde4238b42
> >>> # Parent  2d54aa5397cdb1c697673ba10b7618d5ac25c69e
> >>> ui: introduce util.system() wrapper to make sure ui.fout is used
> >>
> >> The idea looks good, but can I get you to add some docstring the new
> >> method so that people understand its purpose?
> >
> > will do.
> >
> >> As far as I understand the lack of it makes thg command server usage to
> >> break in multiple occasion. Should this be target on stable?
> >
> > Not for stable. This is just a sugar for util.system(..., out=ui.fout).
> >
> > http://markmail.org/message/fwltwdugndmsb3g7
> >
> >> Finally, Do we have a plan to force people to use ui.system instead of
> >> util.system?
> >
> > No idea. Should we make "out" be a required parameter as well?
> 
> How many case is there where util.system should be used over ui.system ?

All util.system() in Mercurial can be replaced by ui.system().

Possible use case of util.system() is

    buf = StringIO()  # or temporary file
    util.system(..., out=buf)

> if not too much, we could maybe more util.system away (rename to 
> rawsystem + add a comment pointing to ui.system?
> 
> (I'm not saying we must do it. I'm asking how reasonable it would be to 
> do so)

It might break third-party extensions unnecessarily.

Regards,
Augie Fackler - Nov. 10, 2014, 4:27 p.m.
On Mon, Nov 10, 2014 at 10:57:50PM +0900, Yuya Nishihara wrote:
> On Sat, 08 Nov 2014 15:24:29 +0000, Pierre-Yves David wrote:
> > On 11/08/2014 02:51 PM, Yuya Nishihara wrote:
> > > On Sat, 08 Nov 2014 10:27:47 +0000, Pierre-Yves David wrote:
> > >> On 11/08/2014 05:42 AM, Yuya Nishihara wrote:
> > >>> # HG changeset patch
> > >>> # User Yuya Nishihara <yuya@tcha.org>
> > >>> # Date 1415419062 -32400
> > >>> #      Sat Nov 08 12:57:42 2014 +0900
> > >>> # Node ID d83746a91dc410f60b45916c2e55dadde4238b42
> > >>> # Parent  2d54aa5397cdb1c697673ba10b7618d5ac25c69e
> > >>> ui: introduce util.system() wrapper to make sure ui.fout is used
> > >>
> > >> The idea looks good, but can I get you to add some docstring the new
> > >> method so that people understand its purpose?
> > >
> > > will do.
> > >
> > >> As far as I understand the lack of it makes thg command server usage to
> > >> break in multiple occasion. Should this be target on stable?
> > >
> > > Not for stable. This is just a sugar for util.system(..., out=ui.fout).
> > >
> > > http://markmail.org/message/fwltwdugndmsb3g7
> > >
> > >> Finally, Do we have a plan to force people to use ui.system instead of
> > >> util.system?
> > >
> > > No idea. Should we make "out" be a required parameter as well?
> >
> > How many case is there where util.system should be used over ui.system ?
>
> All util.system() in Mercurial can be replaced by ui.system().
>
> Possible use case of util.system() is
>
>     buf = StringIO()  # or temporary file
>     util.system(..., out=buf)

Does anyone actually do this?

>
> > if not too much, we could maybe more util.system away (rename to
> > rawsystem + add a comment pointing to ui.system?
> >
> > (I'm not saying we must do it. I'm asking how reasonable it would be to
> > do so)
>
> It might break third-party extensions unnecessarily.

I don't think I'd worry about that too much - extension authors know
they're building on quicksand. I don't think shelling out is
super-common in extensions.

>
> Regards,
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Yuya Nishihara - Nov. 11, 2014, 1:33 p.m.
On Mon, 10 Nov 2014 11:27:30 -0500, Augie Fackler wrote:
> On Mon, Nov 10, 2014 at 10:57:50PM +0900, Yuya Nishihara wrote:
> > On Sat, 08 Nov 2014 15:24:29 +0000, Pierre-Yves David wrote:
> > > On 11/08/2014 02:51 PM, Yuya Nishihara wrote:
> > > > On Sat, 08 Nov 2014 10:27:47 +0000, Pierre-Yves David wrote:
> > > >> On 11/08/2014 05:42 AM, Yuya Nishihara wrote:
> > > >>> # HG changeset patch
> > > >>> # User Yuya Nishihara <yuya@tcha.org>
> > > >>> # Date 1415419062 -32400
> > > >>> #      Sat Nov 08 12:57:42 2014 +0900
> > > >>> # Node ID d83746a91dc410f60b45916c2e55dadde4238b42
> > > >>> # Parent  2d54aa5397cdb1c697673ba10b7618d5ac25c69e
> > > >>> ui: introduce util.system() wrapper to make sure ui.fout is used
> > > >>
> > > >> The idea looks good, but can I get you to add some docstring the new
> > > >> method so that people understand its purpose?
> > > >
> > > > will do.
> > > >
> > > >> As far as I understand the lack of it makes thg command server usage to
> > > >> break in multiple occasion. Should this be target on stable?
> > > >
> > > > Not for stable. This is just a sugar for util.system(..., out=ui.fout).
> > > >
> > > > http://markmail.org/message/fwltwdugndmsb3g7
> > > >
> > > >> Finally, Do we have a plan to force people to use ui.system instead of
> > > >> util.system?
> > > >
> > > > No idea. Should we make "out" be a required parameter as well?
> > >
> > > How many case is there where util.system should be used over ui.system ?
> >
> > All util.system() in Mercurial can be replaced by ui.system().
> >
> > Possible use case of util.system() is
> >
> >     buf = StringIO()  # or temporary file
> >     util.system(..., out=buf)
> 
> Does anyone actually do this?

I found it in rupdate extension, but it seems rupdate was dead.

% grep util.system hg*/**/*.py
hgcr-gui/hgcr-gui-gtk.py:            return util.system("\"%s\" \"%s\"" % (editor, file_path), environ={},
hgext-rupdate/__init__.py:    res = util.system(cmd, out=cmdBuf)
hgtk/tortoisehg/hgtk/gdialog.py:            util.system(command,
hgtk/tortoisehg/hgtk/gtklib.py:        util.system('%s "%s"' % (editor, file))

> > > if not too much, we could maybe more util.system away (rename to
> > > rawsystem + add a comment pointing to ui.system?
> > >
> > > (I'm not saying we must do it. I'm asking how reasonable it would be to
> > > do so)
> >
> > It might break third-party extensions unnecessarily.
> 
> I don't think I'd worry about that too much - extension authors know
> they're building on quicksand. I don't think shelling out is
> super-common in extensions.

I guess so. I'll take marmoute's suggestion and reorganize the patches.

Regards,

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -814,10 +814,9 @@  class ui(object):
 
             editor = self.geteditor()
 
-            util.system("%s \"%s\"" % (editor, name),
+            self.system("%s \"%s\"" % (editor, name),
                         environ=environ,
-                        onerr=util.Abort, errprefix=_("edit failed"),
-                        out=self.fout)
+                        onerr=util.Abort, errprefix=_("edit failed"))
 
             f = open(name)
             t = f.read()
@@ -827,6 +826,10 @@  class ui(object):
 
         return t
 
+    def system(self, cmd, environ={}, cwd=None, onerr=None, errprefix=None):
+        return util.system(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.
         only to call in exception handler. returns true if traceback