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

[Yuya Nishihara]

On Tue, 18 Oct 2016 11:35:20 +0100, Barry Scott wrote:
> > On 16 Oct 2016, at 15:50, Yuya Nishihara <yuya at tcha.org> wrote:
> > On Thu, 06 Oct 2016 18:04:47 +0100, Barry A. Scott wrote:
> >> # HG changeset patch
> >> # User Barry A. Scott <barry at 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.