Patchwork [python-hglib] Add the abilty to trace the protocol between the client and server

login
register
mail settings
Submitter Barry A. Scott
Date Oct. 6, 2016, 5:04 p.m.
Message ID <efc527cc43d7394a5bd0.1475773487@varric.chelsea.private>
Download mbox | patch
Permalink /patch/16869/
State Superseded
Delegated to: Matt Mackall
Headers show

Comments

Barry A. Scott - Oct. 6, 2016, 5:04 p.m.
# HG changeset patch
# User Barry A. Scott <barry@barrys-emacs.org>
# Date 1475770955 -3600
#      Thu Oct 06 17:22:35 2016 +0100
# Branch hglib-protocol-trace
# Node ID efc527cc43d7394a5bd0deb1d29c4307592f7528
# Parent  6f15cb7cc9cb4427f35c60080f85dbf4ca5abd10
Add the abilty to trace the protocol between the client and server.
This is useful when debugging issues with driving hg via hglib
where output and error messages can be lost.

call setprotocoltrace with the name of a trace function or None.
If the trace function is None no tracing is done.
The trace function is called with the channel and its data.
Yuya Nishihara - Oct. 16, 2016, 2:50 p.m.
On Thu, 06 Oct 2016 18:04:47 +0100, Barry A. Scott wrote:
> # HG changeset patch
> # User Barry A. Scott <barry@barrys-emacs.org>
> # Date 1475770955 -3600
> #      Thu Oct 06 17:22:35 2016 +0100
> # Branch hglib-protocol-trace
> # Node ID efc527cc43d7394a5bd0deb1d29c4307592f7528
> # Parent  6f15cb7cc9cb4427f35c60080f85dbf4ca5abd10
> Add the abilty to trace the protocol between the client and server.
> This is useful when debugging issues with driving hg via hglib
> where output and error messages can be lost.
> 
> call setprotocoltrace with the name of a trace function or None.
> If the trace function is None no tracing is done.
> The trace function is called with the channel and its data.

This generally looks good to me. Can you update the commit message and
coding style?

https://www.mercurial-scm.org/wiki/ContributingChanges

> diff -r 6f15cb7cc9cb -r efc527cc43d7 hglib/client.py
> --- a/hglib/client.py	Mon Jul 18 23:40:45 2016 -0500
> +++ b/hglib/client.py	Thu Oct 06 17:22:35 2016 +0100
> @@ -62,6 +62,15 @@
>          if connect:
>              self.open()
>  
> +        self._protocoltracefn = None
> +
> +    def setprotocoltrace( self, tracefn=None ):
> +        """
> +        if tracefn is None no trace calls will be made.
> +        Otherwise tracefn is call as tracefn( channel, data )
> +        """
> +        self._protocoltracefn = tracefn
> +
>      def __enter__(self):
>          if self.server is None:
>              self.open()
> @@ -119,6 +128,7 @@
>  
>      def runcommand(self, args, inchannels, outchannels):
>          def writeblock(data):
> +            if self._protocoltracefn is not None: self._protocoltracefn( b'i', data )

Using a fake channel name seems a bit unfortunate. I slightly prefer b'' or
None.s
Barry A. Scott - Oct. 18, 2016, 10:35 a.m.
> On 16 Oct 2016, at 15:50, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> On Thu, 06 Oct 2016 18:04:47 +0100, Barry A. Scott wrote:
>> # HG changeset patch
>> # User Barry A. Scott <barry@barrys-emacs.org>
>> # Date 1475770955 -3600
>> #      Thu Oct 06 17:22:35 2016 +0100
>> # Branch hglib-protocol-trace
>> # Node ID efc527cc43d7394a5bd0deb1d29c4307592f7528
>> # Parent  6f15cb7cc9cb4427f35c60080f85dbf4ca5abd10
>> Add the abilty to trace the protocol between the client and server.
>> This is useful when debugging issues with driving hg via hglib
>> where output and error messages can be lost.
>> 
>> call setprotocoltrace with the name of a trace function or None.
>> If the trace function is None no tracing is done.
>> The trace function is called with the channel and its data.
> 
> This generally looks good to me. Can you update the commit message and
> coding style?
> 
> https://www.mercurial-scm.org/wiki/ContributingChanges <https://www.mercurial-scm.org/wiki/ContributingChanges>
> 

No problem. I have read that link and the coding style and thought that I coded in the
required style. Can you point to the drop off please?

>> diff -r 6f15cb7cc9cb -r efc527cc43d7 hglib/client.py
>> --- a/hglib/client.py	Mon Jul 18 23:40:45 2016 -0500
>> +++ b/hglib/client.py	Thu Oct 06 17:22:35 2016 +0100
>> @@ -62,6 +62,15 @@
>>         if connect:
>>             self.open()
>> 
>> +        self._protocoltracefn = None
>> +
>> +    def setprotocoltrace( self, tracefn=None ):
>> +        """
>> +        if tracefn is None no trace calls will be made.
>> +        Otherwise tracefn is call as tracefn( channel, data )
>> +        """
>> +        self._protocoltracefn = tracefn
>> +
>>     def __enter__(self):
>>         if self.server is None:
>>             self.open()
>> @@ -119,6 +128,7 @@
>> 
>>     def runcommand(self, args, inchannels, outchannels):
>>         def writeblock(data):
>> +            if self._protocoltracefn is not None: self._protocoltracefn( b'i', data )
> 
> Using a fake channel name seems a bit unfortunate. I slightly prefer b'' or
> None.s

I reasoned that given:

    stdout is ‘o’
    stderr is ‘e’

Clearly if there was an actual channel id for stdin the code would use ‘i’.

I could use b’’ or None, but that would just mean I need more code in the callers code mapping
None or b’’ into ‘i’ for printing.

It seemed better to have an obvious identifier.

Barry
Yuya Nishihara - Oct. 18, 2016, 2:09 p.m.
On Tue, 18 Oct 2016 11:35:20 +0100, Barry Scott wrote:
> > On 16 Oct 2016, at 15:50, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Thu, 06 Oct 2016 18:04:47 +0100, Barry A. Scott wrote:
> >> # HG changeset patch
> >> # User Barry A. Scott <barry@barrys-emacs.org>
> >> # Date 1475770955 -3600
> >> #      Thu Oct 06 17:22:35 2016 +0100
> >> # Branch hglib-protocol-trace
> >> # Node ID efc527cc43d7394a5bd0deb1d29c4307592f7528
> >> # Parent  6f15cb7cc9cb4427f35c60080f85dbf4ca5abd10
> >> Add the abilty to trace the protocol between the client and server.
> >> This is useful when debugging issues with driving hg via hglib
> >> where output and error messages can be lost.
> >> 
> >> call setprotocoltrace with the name of a trace function or None.
> >> If the trace function is None no tracing is done.
> >> The trace function is called with the channel and its data.
> > 
> > This generally looks good to me. Can you update the commit message and
> > coding style?
> > 
> > https://www.mercurial-scm.org/wiki/ContributingChanges <https://www.mercurial-scm.org/wiki/ContributingChanges>
> > 
> 
> No problem. I have read that link and the coding style and thought that I coded in the
> required style. Can you point to the drop off please?

I think the 80-col rule apply to hglib. You can run check-code.py to find
style issues.

https://selenic.com/repo/hg/file/3.9.2/contrib/check-code.py

% ../mercurial/contrib/check-code.py hglib/*.py
hglib/client.py:574:
 >             raise ValueError('revision and node not found in hg output: %r' % out)
 line too long
hglib/client.py:879:
 >         if hasattr(patches, 'read') and hasattr(patches, 'readline'):
 hasattr(foo, bar) is broken, use util.safehasattr(foo, bar) instead
 hasattr(foo, bar) is broken, use util.safehasattr(foo, bar) instead
hglib/context.py:82:
 >     def __bool__(self):
 __bool__ should be __nonzero__ in Python 2
hglib/util.py:170:
 >     def __bool__(self):
 __bool__ should be __nonzero__ in Python 2
hglib/util.py:209:
 > def popen(args, env={}):
 don't use mutable default arguments

Some of them are false positives.

> >>     def runcommand(self, args, inchannels, outchannels):
> >>         def writeblock(data):
> >> +            if self._protocoltracefn is not None: self._protocoltracefn( b'i', data )
> > 
> > Using a fake channel name seems a bit unfortunate. I slightly prefer b'' or
> > None.s
> 
> I reasoned that given:
> 
>     stdout is ‘o’
>     stderr is ‘e’
> 
> Clearly if there was an actual channel id for stdin the code would use ‘i’.

The stdin here is an output from client to server (c2s). If c2s and s2c pipes
had symmetric protocols, it would be 'I' channel per the rule "required channels
identifiers are uppercase."

https://www.mercurial-scm.org/wiki/CommandServer#Channels

> I could use b’’ or None, but that would just mean I need more code in the callers code mapping
> None or b’’ into ‘i’ for printing.
> 
> It seemed better to have an obvious identifier.

Then, you can add a direction (s2c/c2s) parameter as the protocols are
different.
Barry A. Scott - Oct. 18, 2016, 2:49 p.m.
I will create new patches taking into account all the feedback so far.

The check-code.py tool shows that these errors exists already in hglib/*.py

hglib/client.py:574:
 >             raise ValueError('revision and node not found in hg output: %r' % out)
 line too long
hglib/client.py:879:
 >         if hasattr(patches, 'read') and hasattr(patches, 'readline'):
 hasattr(foo, bar) is broken, use util.safehasattr(foo, bar) instead
 hasattr(foo, bar) is broken, use util.safehasattr(foo, bar) instead
hglib/util.py:209:
 > def popen(args, env={}):
 don't use mutable default arguments

Barry

Patch

diff -r 6f15cb7cc9cb -r efc527cc43d7 hglib/client.py
--- a/hglib/client.py	Mon Jul 18 23:40:45 2016 -0500
+++ b/hglib/client.py	Thu Oct 06 17:22:35 2016 +0100
@@ -62,6 +62,15 @@ 
         if connect:
             self.open()
 
+        self._protocoltracefn = None
+
+    def setprotocoltrace( self, tracefn=None ):
+        """
+        if tracefn is None no trace calls will be made.
+        Otherwise tracefn is call as tracefn( channel, data )
+        """
+        self._protocoltracefn = tracefn
+
     def __enter__(self):
         if self.server is None:
             self.open()
@@ -119,6 +128,7 @@ 
 
     def runcommand(self, args, inchannels, outchannels):
         def writeblock(data):
+            if self._protocoltracefn is not None: self._protocoltracefn( b'i', data )
             self.server.stdin.write(struct.pack(self.inputfmt, len(data)))
             self.server.stdin.write(data)
             self.server.stdin.flush()
@@ -131,6 +141,7 @@ 
 
         while True:
             channel, data = self._readchannel()
+            if self._protocoltracefn is not None: self._protocoltracefn( channel, data )
 
             # input channels
             if channel in inchannels: