[PATCH python-hglib v2] Add feature to allow hglib user to get call backs for prompts, output

[Barry Scott]

> On 23 Oct 2016, at 10:53, Yuya Nishihara <yuya at tcha.org> wrote:
> 
> On Sat, 22 Oct 2016 18:37:07 +0100, Barry A. Scott wrote:
>> # HG changeset patch
>> # User Barry A. Scott <barry at barrys-emacs.org <mailto:barry at barrys-emacs.org>>
>> # Date 1477157573 -3600
>> #      Sat Oct 22 18:32:53 2016 +0100
>> # Node ID 12c96b1d8dfff261f91ed010cc59ce57bfa63b4f
>> # Parent  6f15cb7cc9cb4427f35c60080f85dbf4ca5abd10
>> Add feature to allow hglib user to get call backs for prompts, output
>> and errors.
> 
> "topic: short summary line with no trailing period"
> 
> https://www.mercurial-scm.org/wiki/ContributingChanges#Patch_descriptions <https://www.mercurial-scm.org/wiki/ContributingChanges#Patch_descriptions>

Will fix. (I did not notice until it was sent).
> 
>> setcbout(cbout), setcberr(cberr) and setcbprompt(cbprompt) are used to
>> set the call back function used by the hgclient class. cb stands for
>> call back.
>> 
>> cbout is a function that will be called with the stdout data of the
>> command as it runs. cbout is called with output as it is made available,
>> which can be as partial lines or multiple lines.
>> 
>> cberr is a function that will be called with the stderr data of the
>> command as it runs. cberr is called with output as it is made available,
>> which can be as partial lines or multiple lines.
>> 
>> Command that make remote connects can prompt for username and password
>> for HTTP/HTTPS connections.
>> 
>> cbprompt is called when hgclient need a response to a prompt from the
>> server. It receives the max number of bytes to return and the contents
>> of stdout received so far. The last text sent to either cbout or cberr
>> will contain the prompt text itself.
>> 
>> init() has been added to hgclient to allow use of setcbXXX functions
>> with init(). The init() code is based on the version in __init__.py.
> 
> So this patch contains 3 different things. Can you send them as separate
> patches?
> 
> - add callbacks
> - change clone() behavior (which seems wrong)
> - add init()


Yes and no. Both clone and init cannot be used with the call backs with these changes.

I can package as 3 patches that build on each other. But in my mind these are linked.

I will do as you advise, please confirm 3 patches required.

> 
>> +    def setcbout(self, cbout):
>> +        """
>> +        cbout is a function that will be called with the stdout data of
>> +         the command as it runs. Call with None to stop getting call backs.
>> +        """
>> +        self._cbout = cbout
>> +
>> +    def setcberr(self, cberr):
>> +        """
>> +        cberr is a function that will be called with the stderr data of
>> +         the command as it runs.Call with None to stop getting call backs.
>> +        """
>> +        self._cberr = cberr
>> +
>> +    def setcbprompt(self, cbprompt):
>> +        """
>> +        cbprompt is used to reply to prompts by the server
>> +         It receives the max number of bytes to return and the
>> +         contents of stdout received so far.
>> +
>> +        Call with None to stop getting call backs.
>> +
>> +        cbprompt is never called from merge() or import_()
>> +        which already handle the prompt.
>> +        """
>> +        self._cbprompt = cbprompt
> 
> Nit: The "cb" prefix doesn't make sense here because there's little possibility
> of name conflicts.


True but it is descriptive.

> 
> And I think your setprotocoltrace() covers setcbout/err. Do we still need them?

They solve two distinct problems.

protocol tracing provides a way to see the low level, almost unprocessed messages between client and server.
Its purpose is to allow inspection and debugging of the protocol.

cbout and cderr provide a user of hglib with the post processed output and error information.

In production my GUI will use cdout and cderr often, but never protocol trace. But when I cannot
figure out what is happening I turn on protocol trace to get an insight into how hglib works.
That is, for example, how I found that password: is sent on the ‘e’ channel.

Conclusion yes we need both.


>>     def clone(self, source=b('.'), dest=None, branch=None, updaterev=None,
>> -              revrange=None):
>> +              revrange=None, pull=False, uncompressed=False, ssh=None,
>> +              remotecmd=None, insecure=False, encoding=None, configs=None):
>>         """
>>         Create a copy of an existing repository specified by source in a new
>>         directory dest.
>> @@ -536,9 +584,30 @@
>>         revrange - include the specified changeset
>>         """
>>         args = cmdbuilder(b('clone'), source, dest, b=branch,
>> -                          u=updaterev, r=revrange)
>> +                          u=updaterev, r=revrange, pull=pull,
>> +                          uncompresses=uncompressed, e=ssh,
>> +                          remotecmd=remotecmd, insecure=insecure)
>>         self.rawcommand(args)
>> 
>> +        if self._path is None:
>> +            self._path = dest
>> +            # become the client for the cloned hg repo
>> +            self.close()
>> +            self._args += ['-R', dest]
>> +            self.open()
> 
> This changes the behavior of clone(), but hglib should provide a stable API.

It is a backward compatible extension of the API by adding what hglib provided in the __init__.py
version of clone. 

> Also, dest may be a remote (ssh) path.

I do not see how my patch breaks things in that case. I do not assume the
dest is a local path.

> 
>> +    def init(self, dest, ssh=None, remotecmd=None, insecure=False,
>> +             encoding=None, configs=None):
>> +        args = util.cmdbuilder(b'init', dest, e=ssh, remotecmd=remotecmd,
>> +                               insecure=insecure)
>> +        self.rawcommand(args)
>> +
>> +        # become the client for this new hg repo
>> +        self._path = dest
>> +        self.close()
>> +        self._args += ['-R', dest]
>> +        self.open()
> 
> Same here, dest may be a remote path.
> 
> IMHO, methods of hgclient shouldn't have such magic. Instead, we can provide
> a utility function on top of plain init(), e.g.
> 
>  client = client.hgclient()
>  client.init(dest)
>  return open(dest)

That is semantically identical to my patch.

See __init__.py:11 and client.py:49 of the unpatched source.

However that does not work for the call back case. Here is how I use init.

   c = hglib.open( None )
   c.setcbout(cbout)
   c.setcberr(cbout)
   c.init(b'path/for/new/repo')
   c.status() # works on new repo

This works for the local path case. And this for clone:

   c = hglib.open( None )
   c.setcbout(cbout)
   c.setcberr(cbout)
   c.setcbprompt(cbprompt)
   c.clone(b'http://example.com/hgrepo', b'path/for/new/repo <http://example.com/hgrepo',%20b'path/for/new/repo>')
   c.status() # works on new repo


> 
> Maybe we can provide it as a replacement for hglib.init().
> 
>  def init():
>      try:
>          _initbyhgclient()
>      except 'exception raised if command server failed to start':
>          falls back to the original init()


I choose to leave the hglib.init and hglib.clone code untouched for as its uses of it
cannot care about call backs.

Barry



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20161024/6780b803/attachment.html>