Patchwork [3,of,3] sshpeer: drop support for not reading stderr

login
register
mail settings
Submitter Yuya Nishihara
Date March 12, 2018, 2:17 p.m.
Message ID <e6071590cd3e6e376ff4.1520864264@mimosa>
Download mbox | patch
Permalink /patch/29314/
State Accepted
Headers show

Comments

Yuya Nishihara - March 12, 2018, 2:17 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1520858773 -32400
#      Mon Mar 12 21:46:13 2018 +0900
# Node ID e6071590cd3e6e376ff45a11204065f08f72eee8
# Parent  d3dd691a3fce0c501a34ed68d1a08b563a78794c
sshpeer: drop support for not reading stderr

It's handled by caller now. This patch backs out most of 1151c731686e and
1a36ef7df70a.
Gregory Szorc - March 12, 2018, 6:16 p.m.
On Mon, Mar 12, 2018 at 7:17 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1520858773 -32400
> #      Mon Mar 12 21:46:13 2018 +0900
> # Node ID e6071590cd3e6e376ff45a11204065f08f72eee8
> # Parent  d3dd691a3fce0c501a34ed68d1a08b563a78794c
> sshpeer: drop support for not reading stderr
>
> It's handled by caller now. This patch backs out most of 1151c731686e and
> 1a36ef7df70a.
>

I'm OK with this approach if part 2 is the path forward. I considered this
feature a bit hacky TBH and would be happy to see the code deleted.


>
> diff --git a/mercurial/debugcommands.py b/mercurial/debugcommands.py
> --- a/mercurial/debugcommands.py
> +++ b/mercurial/debugcommands.py
> @@ -2759,19 +2759,16 @@ def debugwireproto(ui, repo, **opts):
>
>          if opts['peer'] == 'ssh1':
>              ui.write(_('creating ssh peer for wire protocol version 1\n'))
> -            peer = sshpeer.sshv1peer(ui, url, proc, stdin, stdout, stderr,
> -                                     None, autoreadstderr=autoreadstderr)
> +            peer = sshpeer.sshv1peer(ui, url, proc, stdin, stdout,
> stderr, None)
>          elif opts['peer'] == 'ssh2':
>              ui.write(_('creating ssh peer for wire protocol version 2\n'))
> -            peer = sshpeer.sshv2peer(ui, url, proc, stdin, stdout, stderr,
> -                                     None, autoreadstderr=autoreadstderr)
> +            peer = sshpeer.sshv2peer(ui, url, proc, stdin, stdout,
> stderr, None)
>          elif opts['peer'] == 'raw':
>              ui.write(_('using raw connection to peer\n'))
>              peer = None
>          else:
>              ui.write(_('creating ssh peer from handshake results\n'))
> -            peer = sshpeer.makepeer(ui, url, proc, stdin, stdout, stderr,
> -                                    autoreadstderr=autoreadstderr)
> +            peer = sshpeer.makepeer(ui, url, proc, stdin, stdout, stderr)
>
>      else:
>          raise error.Abort(_('only --localssh is currently supported'))
> diff --git a/mercurial/sshpeer.py b/mercurial/sshpeer.py
> --- a/mercurial/sshpeer.py
> +++ b/mercurial/sshpeer.py
> @@ -339,16 +339,13 @@ def _performhandshake(ui, stdin, stdout,
>      return protoname, caps
>
>  class sshv1peer(wireproto.wirepeer):
> -    def __init__(self, ui, url, proc, stdin, stdout, stderr, caps,
> -                 autoreadstderr=True):
> +    def __init__(self, ui, url, proc, stdin, stdout, stderr, caps):
>          """Create a peer from an existing SSH connection.
>
>          ``proc`` is a handle on the underlying SSH process.
>          ``stdin``, ``stdout``, and ``stderr`` are handles on the stdio
> -        pipes for that process.
> +        pipes for that process. ``stderr`` may be None.
>          ``caps`` is a set of capabilities supported by the remote.
> -        ``autoreadstderr`` denotes whether to automatically read from
> -        stderr and to forward its output.
>          """
>          self._url = url
>          self._ui = ui
> @@ -358,7 +355,7 @@ class sshv1peer(wireproto.wirepeer):
>
>          # And we hook up our "doublepipe" wrapper to allow querying
>          # stderr any time we perform I/O.
> -        if autoreadstderr:
> +        if stderr:
>              stdout = doublepipe(ui, util.bufferedinputpipe(stdout),
> stderr)
>              stdin = doublepipe(ui, stdin, stderr)
>
> @@ -366,7 +363,6 @@ class sshv1peer(wireproto.wirepeer):
>          self._pipei = stdout
>          self._pipee = stderr
>          self._caps = caps
> -        self._autoreadstderr = autoreadstderr
>
>      # Commands that have a "framed" response where the first line of the
>      # response contains the length of that response.
> @@ -512,12 +508,10 @@ class sshv1peer(wireproto.wirepeer):
>      def _getamount(self):
>          l = self._pipei.readline()
>          if l == '\n':
> -            if self._autoreadstderr:
> -                self._readerr()
> +            self._readerr()
>              msg = _('check previous remote output')
>              self._abort(error.OutOfBandError(hint=msg))
> -        if self._autoreadstderr:
> -            self._readerr()
> +        self._readerr()
>          try:
>              return int(l)
>          except ValueError:
> @@ -536,8 +530,7 @@ class sshv1peer(wireproto.wirepeer):
>              self._pipeo.write(data)
>          if flush:
>              self._pipeo.flush()
> -        if self._autoreadstderr:
> -            self._readerr()
> +        self._readerr()
>
>  class sshv2peer(sshv1peer):
>      """A peer that speakers version 2 of the transport protocol."""
> @@ -545,7 +538,7 @@ class sshv2peer(sshv1peer):
>      # And handshake is performed before the peer is instantiated. So
>      # we need no custom code.
>
> -def makepeer(ui, path, proc, stdin, stdout, stderr, autoreadstderr=True):
> +def makepeer(ui, path, proc, stdin, stdout, stderr):
>      """Make a peer instance from existing pipes.
>
>      ``path`` and ``proc`` are stored on the eventual peer instance and may
> @@ -566,11 +559,9 @@ def makepeer(ui, path, proc, stdin, stdo
>          raise
>
>      if protoname == wireprototypes.SSHV1:
> -        return sshv1peer(ui, path, proc, stdin, stdout, stderr, caps,
> -                         autoreadstderr=autoreadstderr)
> +        return sshv1peer(ui, path, proc, stdin, stdout, stderr, caps)
>      elif protoname == wireprototypes.SSHV2:
> -        return sshv2peer(ui, path, proc, stdin, stdout, stderr, caps,
> -                         autoreadstderr=autoreadstderr)
> +        return sshv2peer(ui, path, proc, stdin, stdout, stderr, caps)
>      else:
>          _cleanuppipes(ui, stdout, stdin, stderr)
>          raise error.RepoError(_('unknown version of SSH protocol: %s') %
>

Patch

diff --git a/mercurial/debugcommands.py b/mercurial/debugcommands.py
--- a/mercurial/debugcommands.py
+++ b/mercurial/debugcommands.py
@@ -2759,19 +2759,16 @@  def debugwireproto(ui, repo, **opts):
 
         if opts['peer'] == 'ssh1':
             ui.write(_('creating ssh peer for wire protocol version 1\n'))
-            peer = sshpeer.sshv1peer(ui, url, proc, stdin, stdout, stderr,
-                                     None, autoreadstderr=autoreadstderr)
+            peer = sshpeer.sshv1peer(ui, url, proc, stdin, stdout, stderr, None)
         elif opts['peer'] == 'ssh2':
             ui.write(_('creating ssh peer for wire protocol version 2\n'))
-            peer = sshpeer.sshv2peer(ui, url, proc, stdin, stdout, stderr,
-                                     None, autoreadstderr=autoreadstderr)
+            peer = sshpeer.sshv2peer(ui, url, proc, stdin, stdout, stderr, None)
         elif opts['peer'] == 'raw':
             ui.write(_('using raw connection to peer\n'))
             peer = None
         else:
             ui.write(_('creating ssh peer from handshake results\n'))
-            peer = sshpeer.makepeer(ui, url, proc, stdin, stdout, stderr,
-                                    autoreadstderr=autoreadstderr)
+            peer = sshpeer.makepeer(ui, url, proc, stdin, stdout, stderr)
 
     else:
         raise error.Abort(_('only --localssh is currently supported'))
diff --git a/mercurial/sshpeer.py b/mercurial/sshpeer.py
--- a/mercurial/sshpeer.py
+++ b/mercurial/sshpeer.py
@@ -339,16 +339,13 @@  def _performhandshake(ui, stdin, stdout,
     return protoname, caps
 
 class sshv1peer(wireproto.wirepeer):
-    def __init__(self, ui, url, proc, stdin, stdout, stderr, caps,
-                 autoreadstderr=True):
+    def __init__(self, ui, url, proc, stdin, stdout, stderr, caps):
         """Create a peer from an existing SSH connection.
 
         ``proc`` is a handle on the underlying SSH process.
         ``stdin``, ``stdout``, and ``stderr`` are handles on the stdio
-        pipes for that process.
+        pipes for that process. ``stderr`` may be None.
         ``caps`` is a set of capabilities supported by the remote.
-        ``autoreadstderr`` denotes whether to automatically read from
-        stderr and to forward its output.
         """
         self._url = url
         self._ui = ui
@@ -358,7 +355,7 @@  class sshv1peer(wireproto.wirepeer):
 
         # And we hook up our "doublepipe" wrapper to allow querying
         # stderr any time we perform I/O.
-        if autoreadstderr:
+        if stderr:
             stdout = doublepipe(ui, util.bufferedinputpipe(stdout), stderr)
             stdin = doublepipe(ui, stdin, stderr)
 
@@ -366,7 +363,6 @@  class sshv1peer(wireproto.wirepeer):
         self._pipei = stdout
         self._pipee = stderr
         self._caps = caps
-        self._autoreadstderr = autoreadstderr
 
     # Commands that have a "framed" response where the first line of the
     # response contains the length of that response.
@@ -512,12 +508,10 @@  class sshv1peer(wireproto.wirepeer):
     def _getamount(self):
         l = self._pipei.readline()
         if l == '\n':
-            if self._autoreadstderr:
-                self._readerr()
+            self._readerr()
             msg = _('check previous remote output')
             self._abort(error.OutOfBandError(hint=msg))
-        if self._autoreadstderr:
-            self._readerr()
+        self._readerr()
         try:
             return int(l)
         except ValueError:
@@ -536,8 +530,7 @@  class sshv1peer(wireproto.wirepeer):
             self._pipeo.write(data)
         if flush:
             self._pipeo.flush()
-        if self._autoreadstderr:
-            self._readerr()
+        self._readerr()
 
 class sshv2peer(sshv1peer):
     """A peer that speakers version 2 of the transport protocol."""
@@ -545,7 +538,7 @@  class sshv2peer(sshv1peer):
     # And handshake is performed before the peer is instantiated. So
     # we need no custom code.
 
-def makepeer(ui, path, proc, stdin, stdout, stderr, autoreadstderr=True):
+def makepeer(ui, path, proc, stdin, stdout, stderr):
     """Make a peer instance from existing pipes.
 
     ``path`` and ``proc`` are stored on the eventual peer instance and may
@@ -566,11 +559,9 @@  def makepeer(ui, path, proc, stdin, stdo
         raise
 
     if protoname == wireprototypes.SSHV1:
-        return sshv1peer(ui, path, proc, stdin, stdout, stderr, caps,
-                         autoreadstderr=autoreadstderr)
+        return sshv1peer(ui, path, proc, stdin, stdout, stderr, caps)
     elif protoname == wireprototypes.SSHV2:
-        return sshv2peer(ui, path, proc, stdin, stdout, stderr, caps,
-                         autoreadstderr=autoreadstderr)
+        return sshv2peer(ui, path, proc, stdin, stdout, stderr, caps)
     else:
         _cleanuppipes(ui, stdout, stdin, stderr)
         raise error.RepoError(_('unknown version of SSH protocol: %s') %