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

# HG changeset patch
# User Barry A. Scott <barry@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.

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.

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

clone() has been extended with all the parameters that are present in
__init__.py clone() to allow use of the setcbXXX functions.

Example use:
    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')
    c.status() # works on new repo

On Sat, 22 Oct 2016 18:37:07 +0100, Barry A. Scott wrote:
> # HG changeset patch
> # User Barry A. Scott <barry@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

> 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()

> +    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.

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

>      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.

Also, dest may be a remote (ssh) 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)

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()

> On 23 Oct 2016, at 10:53, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> On Sat, 22 Oct 2016 18:37:07 +0100, Barry A. Scott wrote:
>> # HG changeset patch
>> # User Barry A. Scott <barry@barrys-emacs.org <mailto:barry@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

On Mon, 24 Oct 2016 11:04:39 +0100, Barry Scott wrote:
> > 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.

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

"6. patch does just one thing (if you need a bullet list, split your patch)"

I would split clone/init changes since they solve separate issues on top of
the new callback API.

> >> +    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.

Hmm, but it introduces new naming style, right? I have no strong opinion about
this, but "prompt" seems just fine because we already have one.

> > 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.

Okay, I don't think they are required, but I agree they are semantically
different from setprotocoltrace().

Another option is to provide an interface to set a single callback object
that handles o/e/I/L channels altogether, so we could provide a helper class
to translate o+L and e+L sequences to .prompt() and .password() respectively.

TortoiseHg handles user interaction in that way:
https://bitbucket.org/tortoisehg/thg/src/3.9.2/tortoisehg/hgqt/cmdcore.py#cmdcore.py-44

BTW, is hglib usable in GUI? I think its synchronous I/O will behave pretty
bad in GUI environment.

> >>     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.

No.

  client = hglib.open()
  client.root()
  client.clone('.', 'somewhere')
  client.root()

Here clone() shouldn't reopen the client. Be aware that hglib.open() opens the
repository found in cwd by default.

Maybe you can get some errors by running the test.

  $ python test.py --with-doctest --with-hg path/to/hg

> > 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.

If dest isn't a local path, open() with ['-R', dest] would fail.

> >> +    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.

Maybe they could take a callback as an argument?

On Monday, 24 October 2016 22:30:08 BST Yuya Nishihara wrote:
> On Mon, 24 Oct 2016 11:04:39 +0100, Barry Scott wrote:
> > > 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.
> 
> https://www.mercurial-scm.org/wiki/ContributingChanges#Submission_checklist
> 
> "6. patch does just one thing (if you need a bullet list, split your patch)"
> 
> I would split clone/init changes since they solve separate issues on top of
> the new callback API.
> 
> > >> +    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.
> 
> Hmm, but it introduces new naming style, right? I have no strong opinion
> about this, but "prompt" seems just fine because we already have one.

prompt is not part of the public API.

> 
> > > 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.
> 
> Okay, I don't think they are required, but I agree they are semantically
> different from setprotocoltrace().
> 
> Another option is to provide an interface to set a single callback object
> that handles o/e/I/L channels altogether, so we could provide a helper class
> to translate o+L and e+L sequences to .prompt() and .password()
> respectively.

I've assumed that its better to leave the hglib patch simple.

As we talked about before a getcredentials call back would be great.
But That needs hg itself to change. I sugest that we stay with this API
until that is possible.

> 
> TortoiseHg handles user interaction in that way:
> https://bitbucket.org/tortoisehg/thg/src/3.9.2/tortoisehg/hgqt/cmdcore.py#cm
> dcore.py-44
> 
> BTW, is hglib usable in GUI? I think its synchronous I/O will behave pretty
> bad in GUI environment.

With these patches its usable :-)

I run any long running hg operation in a background thread.

> > >>     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.
> 
> No.
> 
>   client = hglib.open()
>   client.root()
>   client.clone('.', 'somewhere')
>   client.root()
> 
> Here clone() shouldn't reopen the client. Be aware that hglib.open() opens
> the repository found in cwd by default.

What I wanted was to be able to create the hgclient() object,
setup call backs then do the init() or the clone() with the call backs being 
called as required.

If I drop the reopenning that avoids your objects to semantic changhed.

Indeed in my GUI code I do not use the hgclient() object that I do the clone()
of init() on. I use a new object.

> Maybe you can get some errors by running the test.
> 
>   $ python test.py --with-doctest --with-hg path/to/hg

Will do.

> 
> > > 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.
> 
> If dest isn't a local path, open() with ['-R', dest] would fail.

I'll drop the reopen code.

> 
> > >> +    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.
> 
> Maybe they could take a callback as an argument?

You will recall that where I started and you pointed out that it should be set 
once as state of hgclient().

I'll genreate a new set of patches for this and post once I see that test.py
is happy.

Barry

FYI: My code that uses this patch is in SCM Workbench.

Here is the code I use for init() and clone():

https://github.com/barry-scott/scm-workbench/blob/master/Source/Hg/
wb_hg_project.py#L65

Here is the object that handles the call back interface and map into my GUI's 
world:

https://github.com/barry-scott/scm-workbench/blob/master/Source/Hg/
wb_hg_project.py#L448

Barry

On Mon, 24 Oct 2016 15:57:30 +0100, Barry Scott wrote:
> On Monday, 24 October 2016 22:30:08 BST Yuya Nishihara wrote:
> > On Mon, 24 Oct 2016 11:04:39 +0100, Barry Scott wrote:
> > > >> +    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.
> > 
> > Hmm, but it introduces new naming style, right? I have no strong opinion
> > about this, but "prompt" seems just fine because we already have one.
> 
> prompt is not part of the public API.

rawcommand() is a public API. hgclient() exposes APIs from two different
levels.

> > > > 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.
> > 
> > Okay, I don't think they are required, but I agree they are semantically
> > different from setprotocoltrace().
> > 
> > Another option is to provide an interface to set a single callback object
> > that handles o/e/I/L channels altogether, so we could provide a helper class
> > to translate o+L and e+L sequences to .prompt() and .password()
> > respectively.
> 
> I've assumed that its better to leave the hglib patch simple.
> 
> As we talked about before a getcredentials call back would be great.
> But That needs hg itself to change. I sugest that we stay with this API
> until that is possible.

Yeah, that's why I suggested you to extend prompt() to take err output. That
wouldn't be neat, but was the simplest change I could think of. Since we're
introducing more functions, it might be worth thinking a better interface.

> > > > 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.
> > 
> > If dest isn't a local path, open() with ['-R', dest] would fail.
> 
> I'll drop the reopen code.

Sounds fine.

> > > I choose to leave the hglib.init and hglib.clone code untouched for as its
> > > uses of it cannot care about call backs.
> > 
> > Maybe they could take a callback as an argument?
> 
> You will recall that where I started and you pointed out that it should be set 
> once as state of hgclient().

Yes, that's because hgclient has many commands which can potentially need a
callback. I think that's okay to pass a callback which will be set to new
hgclient object created by open()/init()/clone().

Anyway, as you don't use hglib.init() and .clone(), we don't need to discuss
that further.