Patchwork [2,of,5] ui: allow capture of subprocess output

login
register
mail settings
Submitter Pierre-Yves David
Date April 23, 2015, 11:56 p.m.
Message ID <8aea112e68b1f90f8b29.1429833382@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/8774/
State Accepted
Headers show

Comments

Pierre-Yves David - April 23, 2015, 11:56 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1429797459 -3600
#      Thu Apr 23 14:57:39 2015 +0100
# Branch stable
# Node ID 8aea112e68b1f90f8b2917f06fd4081116502c64
# Parent  61a2951a30f7c18501027501f7642dd632e72fcd
ui: allow capture of subprocess output

We want to capture hooks output during bundle2 processing. For this purpose we
introduce a new 'subproc' argument to 'ui.pushbuffer'. When set, the output of
sub process created through 'ui.system' will be captured in the buffer too.

This will be used in the next changeset.
Yuya Nishihara - April 24, 2015, 1:49 p.m.
On Fri, 24 Apr 2015 00:56:22 +0100, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1429797459 -3600
> #      Thu Apr 23 14:57:39 2015 +0100
> # Branch stable
> # Node ID 8aea112e68b1f90f8b2917f06fd4081116502c64
> # Parent  61a2951a30f7c18501027501f7642dd632e72fcd
> ui: allow capture of subprocess output
> 
> We want to capture hooks output during bundle2 processing. For this purpose we
> introduce a new 'subproc' argument to 'ui.pushbuffer'. When set, the output of
> sub process created through 'ui.system' will be captured in the buffer too.
> 
> This will be used in the next changeset.

>      def write_err(self, *args, **opts):
>          try:
> -            if self._bufferstates and self._bufferstates[-1]:
> +            if self._bufferstates and self._bufferstates[-1][0]:
>                  return self.write(*args, **opts)
>              if not getattr(self.fout, 'closed', False):
>                  self.fout.flush()
>              for a in args:
>                  self.ferr.write(str(a))
> @@ -832,12 +836,15 @@ class ui(object):
>  
>      def system(self, cmd, environ={}, cwd=None, onerr=None, errprefix=None):
>          '''execute shell command with appropriate output stream. command
>          output will be redirected if fout is not stdout.
>          '''
> +        out = self.fout
> +        if util.any(s[1] for s in self._bufferstates):
> +            out = self

Nitpick, why does it check any bufferstates unlike write_error() ?
Pierre-Yves David - April 24, 2015, 2:46 p.m.
On 04/24/2015 02:49 PM, Yuya Nishihara wrote:
> On Fri, 24 Apr 2015 00:56:22 +0100, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>> # Date 1429797459 -3600
>> #      Thu Apr 23 14:57:39 2015 +0100
>> # Branch stable
>> # Node ID 8aea112e68b1f90f8b2917f06fd4081116502c64
>> # Parent  61a2951a30f7c18501027501f7642dd632e72fcd
>> ui: allow capture of subprocess output
>>
>> We want to capture hooks output during bundle2 processing. For this purpose we
>> introduce a new 'subproc' argument to 'ui.pushbuffer'. When set, the output of
>> sub process created through 'ui.system' will be captured in the buffer too.
>>
>> This will be used in the next changeset.
>
>>       def write_err(self, *args, **opts):
>>           try:
>> -            if self._bufferstates and self._bufferstates[-1]:
>> +            if self._bufferstates and self._bufferstates[-1][0]:
>>                   return self.write(*args, **opts)
>>               if not getattr(self.fout, 'closed', False):
>>                   self.fout.flush()
>>               for a in args:
>>                   self.ferr.write(str(a))
>> @@ -832,12 +836,15 @@ class ui(object):
>>
>>       def system(self, cmd, environ={}, cwd=None, onerr=None, errprefix=None):
>>           '''execute shell command with appropriate output stream. command
>>           output will be redirected if fout is not stdout.
>>           '''
>> +        out = self.fout
>> +        if util.any(s[1] for s in self._bufferstates):
>> +            out = self
>
> Nitpick, why does it check any bufferstates unlike write_error() ?

The idea is to turn a "flag" on saying "now I want subprocess being 
capture". So buffer push at lower level should not disable the 
subprocess output capture.

That said write_error should probably do something similar.

That said², we should probably capture the errors output in the buffer 
that request it (in both err and subprocess case but it does not seems 
too critical for now/stable.
Yuya Nishihara - April 24, 2015, 3:39 p.m.
On Fri, 24 Apr 2015 15:46:08 +0100, Pierre-Yves David wrote:
> On 04/24/2015 02:49 PM, Yuya Nishihara wrote:
> > On Fri, 24 Apr 2015 00:56:22 +0100, Pierre-Yves David wrote:
> >> # HG changeset patch
> >> # User Pierre-Yves David <pierre-yves.david@fb.com>
> >> # Date 1429797459 -3600
> >> #      Thu Apr 23 14:57:39 2015 +0100
> >> # Branch stable
> >> # Node ID 8aea112e68b1f90f8b2917f06fd4081116502c64
> >> # Parent  61a2951a30f7c18501027501f7642dd632e72fcd
> >> ui: allow capture of subprocess output
> >>
> >> We want to capture hooks output during bundle2 processing. For this purpose we
> >> introduce a new 'subproc' argument to 'ui.pushbuffer'. When set, the output of
> >> sub process created through 'ui.system' will be captured in the buffer too.
> >>
> >> This will be used in the next changeset.
> >
> >>       def write_err(self, *args, **opts):
> >>           try:
> >> -            if self._bufferstates and self._bufferstates[-1]:
> >> +            if self._bufferstates and self._bufferstates[-1][0]:
> >>                   return self.write(*args, **opts)
> >>               if not getattr(self.fout, 'closed', False):
> >>                   self.fout.flush()
> >>               for a in args:
> >>                   self.ferr.write(str(a))
> >> @@ -832,12 +836,15 @@ class ui(object):
> >>
> >>       def system(self, cmd, environ={}, cwd=None, onerr=None, errprefix=None):
> >>           '''execute shell command with appropriate output stream. command
> >>           output will be redirected if fout is not stdout.
> >>           '''
> >> +        out = self.fout
> >> +        if util.any(s[1] for s in self._bufferstates):
> >> +            out = self
> >
> > Nitpick, why does it check any bufferstates unlike write_error() ?
> 
> The idea is to turn a "flag" on saying "now I want subprocess being 
> capture". So buffer push at lower level should not disable the 
> subprocess output capture.
> 
> That said write_error should probably do something similar.

I see.

> That said², we should probably capture the errors output in the buffer 
> that request it (in both err and subprocess case but it does not seems 
> too critical for now/stable.

For default branch, maybe we can do ui.fin = ui.fout = StringIO() to capture
all outputs during bundle2 processing.
Pierre-Yves David - April 24, 2015, 3:49 p.m.
On 04/24/2015 04:39 PM, Yuya Nishihara wrote:
> On Fri, 24 Apr 2015 15:46:08 +0100, Pierre-Yves David wrote:
>> On 04/24/2015 02:49 PM, Yuya Nishihara wrote:
>>> On Fri, 24 Apr 2015 00:56:22 +0100, Pierre-Yves David wrote:
>>>> # HG changeset patch
>>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>>> # Date 1429797459 -3600
>>>> #      Thu Apr 23 14:57:39 2015 +0100
>>>> # Branch stable
>>>> # Node ID 8aea112e68b1f90f8b2917f06fd4081116502c64
>>>> # Parent  61a2951a30f7c18501027501f7642dd632e72fcd
>>>> ui: allow capture of subprocess output
>>>>
>>>> We want to capture hooks output during bundle2 processing. For this purpose we
>>>> introduce a new 'subproc' argument to 'ui.pushbuffer'. When set, the output of
>>>> sub process created through 'ui.system' will be captured in the buffer too.
>>>>
>>>> This will be used in the next changeset.
>>>
>>>>        def write_err(self, *args, **opts):
>>>>            try:
>>>> -            if self._bufferstates and self._bufferstates[-1]:
>>>> +            if self._bufferstates and self._bufferstates[-1][0]:
>>>>                    return self.write(*args, **opts)
>>>>                if not getattr(self.fout, 'closed', False):
>>>>                    self.fout.flush()
>>>>                for a in args:
>>>>                    self.ferr.write(str(a))
>>>> @@ -832,12 +836,15 @@ class ui(object):
>>>>
>>>>        def system(self, cmd, environ={}, cwd=None, onerr=None, errprefix=None):
>>>>            '''execute shell command with appropriate output stream. command
>>>>            output will be redirected if fout is not stdout.
>>>>            '''
>>>> +        out = self.fout
>>>> +        if util.any(s[1] for s in self._bufferstates):
>>>> +            out = self
>>>
>>> Nitpick, why does it check any bufferstates unlike write_error() ?
>>
>> The idea is to turn a "flag" on saying "now I want subprocess being
>> capture". So buffer push at lower level should not disable the
>> subprocess output capture.
>>
>> That said write_error should probably do something similar.
>
> I see.
>
>> That said², we should probably capture the errors output in the buffer
>> that request it (in both err and subprocess case but it does not seems
>> too critical for now/stable.
>
> For default branch, maybe we can do ui.fin = ui.fout = StringIO() to capture
> all outputs during bundle2 processing.

I was trying to say something else, let me give you an example:

   ui.pushbuffer(error=True)
   ui.pushbuffer()
   Idoerr()
   inner = ui.popbuffer()
   outer = ui.popbuffer()

In that case, the err output from "idoerr()" should be returned in 
"outer". (currently not captured, current subprocs behavior return it in 
"inner")
Yuya Nishihara - April 24, 2015, 4:14 p.m.
On Fri, 24 Apr 2015 16:49:43 +0100, Pierre-Yves David wrote:
> On 04/24/2015 04:39 PM, Yuya Nishihara wrote:
> > On Fri, 24 Apr 2015 15:46:08 +0100, Pierre-Yves David wrote:
> >> On 04/24/2015 02:49 PM, Yuya Nishihara wrote:
> >>> On Fri, 24 Apr 2015 00:56:22 +0100, Pierre-Yves David wrote:
> >>>> # HG changeset patch
> >>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
> >>>> # Date 1429797459 -3600
> >>>> #      Thu Apr 23 14:57:39 2015 +0100
> >>>> # Branch stable
> >>>> # Node ID 8aea112e68b1f90f8b2917f06fd4081116502c64
> >>>> # Parent  61a2951a30f7c18501027501f7642dd632e72fcd
> >>>> ui: allow capture of subprocess output
> >>>>
> >>>> We want to capture hooks output during bundle2 processing. For this purpose we
> >>>> introduce a new 'subproc' argument to 'ui.pushbuffer'. When set, the output of
> >>>> sub process created through 'ui.system' will be captured in the buffer too.
> >>>>
> >>>> This will be used in the next changeset.
> >>>
> >>>>        def write_err(self, *args, **opts):
> >>>>            try:
> >>>> -            if self._bufferstates and self._bufferstates[-1]:
> >>>> +            if self._bufferstates and self._bufferstates[-1][0]:
> >>>>                    return self.write(*args, **opts)
> >>>>                if not getattr(self.fout, 'closed', False):
> >>>>                    self.fout.flush()
> >>>>                for a in args:
> >>>>                    self.ferr.write(str(a))
> >>>> @@ -832,12 +836,15 @@ class ui(object):
> >>>>
> >>>>        def system(self, cmd, environ={}, cwd=None, onerr=None, errprefix=None):
> >>>>            '''execute shell command with appropriate output stream. command
> >>>>            output will be redirected if fout is not stdout.
> >>>>            '''
> >>>> +        out = self.fout
> >>>> +        if util.any(s[1] for s in self._bufferstates):
> >>>> +            out = self
> >>>
> >>> Nitpick, why does it check any bufferstates unlike write_error() ?
> >>
> >> The idea is to turn a "flag" on saying "now I want subprocess being
> >> capture". So buffer push at lower level should not disable the
> >> subprocess output capture.
> >>
> >> That said write_error should probably do something similar.
> >
> > I see.
> >
> >> That said², we should probably capture the errors output in the buffer
> >> that request it (in both err and subprocess case but it does not seems
> >> too critical for now/stable.
> >
> > For default branch, maybe we can do ui.fin = ui.fout = StringIO() to capture
> > all outputs during bundle2 processing.
> 
> I was trying to say something else, let me give you an example:
> 
>    ui.pushbuffer(error=True)
>    ui.pushbuffer()
>    Idoerr()
>    inner = ui.popbuffer()
>    outer = ui.popbuffer()
> 
> In that case, the err output from "idoerr()" should be returned in 
> "outer". (currently not captured, current subprocs behavior return it in 
> "inner")

Got it, thanks.

My point is that push/popbuffer isn't always necessary to capture output
because now we have ui.fout and .ferr.

Regards,

Patch

diff --git a/hgext/color.py b/hgext/color.py
--- a/hgext/color.py
+++ b/hgext/color.py
@@ -443,11 +443,11 @@  class colorui(uimod.ui):
     def write_err(self, *args, **opts):
         if self._colormode is None:
             return super(colorui, self).write_err(*args, **opts)
 
         label = opts.get('label', '')
-        if self._bufferstates and self._bufferstates[-1]:
+        if self._bufferstates and self._bufferstates[-1][0]:
             return self.write(*args, **opts)
         if self._colormode == 'win32':
             for a in args:
                 win32print(a, super(colorui, self).write_err, **opts)
         else:
diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -74,11 +74,12 @@  default = %s
 
 class ui(object):
     def __init__(self, src=None):
         # _buffers: used for temporary capture of output
         self._buffers = []
-        # _bufferstates: Should the temporary capture includes stderr
+        # _bufferstates:
+        #   Should the temporary capture includes stderr and subprocess output
         self._bufferstates = []
         self.quiet = self.verbose = self.debugflag = self.tracebackflag = False
         self._reportuntrusted = True
         self._ocfg = config.config() # overlay
         self._tcfg = config.config() # trusted
@@ -538,16 +539,19 @@  class ui(object):
 
     @util.propertycache
     def paths(self):
         return paths(self)
 
-    def pushbuffer(self, error=False):
+    def pushbuffer(self, error=False, subproc=False):
         """install a buffer to capture standard output of the ui object
 
-        If error is True, the error output will be captured too."""
+        If error is True, the error output will be captured too.
+
+        If subproc is True, output from subprocess (typically hooks) will be
+        captured too."""
         self._buffers.append([])
-        self._bufferstates.append(error)
+        self._bufferstates.append((error, subproc))
 
     def popbuffer(self, labeled=False):
         '''pop the last buffer and return the buffered output
 
         If labeled is True, any labels associated with buffered
@@ -583,11 +587,11 @@  class ui(object):
             for a in args:
                 self.fout.write(str(a))
 
     def write_err(self, *args, **opts):
         try:
-            if self._bufferstates and self._bufferstates[-1]:
+            if self._bufferstates and self._bufferstates[-1][0]:
                 return self.write(*args, **opts)
             if not getattr(self.fout, 'closed', False):
                 self.fout.flush()
             for a in args:
                 self.ferr.write(str(a))
@@ -832,12 +836,15 @@  class ui(object):
 
     def system(self, cmd, environ={}, cwd=None, onerr=None, errprefix=None):
         '''execute shell command with appropriate output stream. command
         output will be redirected if fout is not stdout.
         '''
+        out = self.fout
+        if util.any(s[1] for s in self._bufferstates):
+            out = self
         return util.system(cmd, environ=environ, cwd=cwd, onerr=onerr,
-                           errprefix=errprefix, out=self.fout)
+                           errprefix=errprefix, out=out)
 
     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
         printed.'''