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 _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
