Patchwork D6168: readline: provide styled prompt to readline (issue6070)

login
register
mail settings
Submitter phabricator
Date March 22, 2019, 3:06 p.m.
Message ID <differential-rev-PHID-DREV-eym6kapykykokjxerkbn-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/39357/
State Superseded
Headers show

Comments

phabricator - March 22, 2019, 3:06 p.m.
durin42 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6168

AFFECTED FILES
  mercurial/ui.py

CHANGE DETAILS




To: durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - March 22, 2019, 3:07 p.m.
durin42 added a comment.


  This is Kyle's patch from https://bz.mercurial-scm.org/show_bug.cgi?id=6070 - it feels regrettable but necessary, and anecdotally it seems to work.  I'd give it an LGTM stamp here, but I can't since I uploaded it.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6168

To: durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - March 22, 2019, 3:07 p.m.
durin42 added a subscriber: spectral.
durin42 added a comment.


  FYI @spectral I uploaded this.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6168

To: durin42, #hg-reviewers
Cc: spectral, mercurial-devel
phabricator - March 22, 2019, 6:19 p.m.
jeffpc accepted this revision.
jeffpc added a comment.


  FWIW, I tested this on both Unleashed (an illumos fork) and FreeBSD.  It fixes the issue for me on both.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6168

To: durin42, #hg-reviewers, jeffpc
Cc: jeffpc, spectral, mercurial-devel
phabricator - March 22, 2019, 6:27 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D6168#89872, @jeffpc wrote:
  
  > FWIW, I tested this on both Unleashed (an illumos fork) and FreeBSD.  It fixes the issue for me on both.
  
  
  Thanks for testing, Jeff. I'll queue based on that.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6168

To: durin42, #hg-reviewers, jeffpc
Cc: martinvonz, jeffpc, spectral, mercurial-devel
phabricator - March 22, 2019, 6:30 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D6168#89874, @martinvonz wrote:
  
  > In https://phab.mercurial-scm.org/D6168#89872, @jeffpc wrote:
  >
  > > FWIW, I tested this on both Unleashed (an illumos fork) and FreeBSD.  It fixes the issue for me on both.
  >
  >
  > Thanks for testing, Jeff. I'll queue based on that.
  
  
  Except that `test-command-server.t` fails like this:
  
    @@ -776,11 +776,9 @@
       message: '\xa3DdataJpassword: Hpassword\xf5DtypeFprompt'
       1234
       *** runcommand debugprompt --config ui.interactive=True
    -  message: '\xa3DdataGprompt:GdefaultAyDtypeFprompt'
    -   5678
    +  prompt: 5678
       *** runcommand debugpromptchoice --config ui.interactive=True
    -  message: '\xa4Gchoices\x82\x82AyCYes\x82AnBNoDdataTpromptchoice (y/n)? GdefaultAyDtypeFprompt'
    -   1
    +  promptchoice (y/n)?  1
    
     bad message encoding:
  
  @spectral, any idea how to fix?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6168

To: durin42, #hg-reviewers, jeffpc
Cc: martinvonz, jeffpc, spectral, mercurial-devel
phabricator - March 22, 2019, 7:40 p.m.
spectral added a comment.


  In https://phab.mercurial-scm.org/D6168#89876, @martinvonz wrote:
  
  > Except that `test-command-server.t` fails like this:
  >
  >   @@ -776,11 +776,9 @@
  >      message: '\xa3DdataJpassword: Hpassword\xf5DtypeFprompt'
  >      1234
  >      *** runcommand debugprompt --config ui.interactive=True
  >   -  message: '\xa3DdataGprompt:GdefaultAyDtypeFprompt'
  >   -   5678
  >   +  prompt: 5678
  >      *** runcommand debugpromptchoice --config ui.interactive=True
  >   -  message: '\xa4Gchoices\x82\x82AyCYes\x82AnBNoDdataTpromptchoice (y/n)? GdefaultAyDtypeFprompt'
  >   -   1
  >   +  promptchoice (y/n)?  1
  >  
  >    bad message encoding:
  >
  >
  > @spectral, any idea how to fix?
  
  
  Oh, right... I saw that and that was part of why I didn't mail earlier :D  I think this is just an "expected" difference in behavior, but I don't know enough about what this test was doing or why it was verifying the chg "wireprotocol" to know for sure if this was highlighting a problem or just brittleness.  As far as I can tell, this is just the way that '_writemsgnobuf' works in the command server: it encodes everything passed to it (args, kwargs, etc.) and throws it on the channel, letting the other side figure out what to do with it. Since we're using readline now, this doesn't get encoded like most chg messages.
  
  That said, I haven't been able to show this breaking anything, but perhaps I'm holding it wrong? In terminal window 1, I ran HG=<hg_I_just_built> chg debuguiprompt -p 'are kittens cute?', resized the window, everything was fine.  In terminal window 2, I did the same thing, but it started a second chg. I was hoping it'd use the same chg server so that I'd end up possibly reusing the same tty and causing an issue?
  
  We already have unencoded output in this test: the space and the default response, so that's clearly not breaking the protocol.
  
  tl;dr: I *think* this is fine/expected to just update the test with this new output, but we might want yuya to verify.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6168

To: durin42, #hg-reviewers, jeffpc
Cc: martinvonz, jeffpc, spectral, mercurial-devel
Yuya Nishihara - March 23, 2019, 1:59 a.m.
>   Oh, right... I saw that and that was part of why I didn't mail earlier :D  I think this is just an "expected" difference in behavior,

No. It provides metadata to command-server clients (e.g. GUI/editor frontend.)
Basically we'll need:

```
if usereadline:
    rawinput(labeled_prompt)
else:
    write_prompt_as_before()
    fin.readline()
```

In chg session, stdio is directly accessible from the server process. No
command-server protocol is used after the connection is established.
phabricator - March 23, 2019, 2:01 a.m.
yuja added a comment.


  >   Oh, right... I saw that and that was part of why I didn't mail earlier :D  I think this is just an "expected" difference in behavior,
  
  No. It provides metadata to command-server clients (e.g. GUI/editor frontend.)
  Basically we'll need:
  
    if usereadline:
        rawinput(labeled_prompt)
    else:
        write_prompt_as_before()
        fin.readline()
  
  In chg session, stdio is directly accessible from the server process. No
  command-server protocol is used after the connection is established.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6168

To: durin42, #hg-reviewers, jeffpc, yuja
Cc: martinvonz, jeffpc, spectral, mercurial-devel
phabricator - April 15, 2019, 9:37 p.m.
spectral added a comment.


  I believe I've incorporated yuja's suggestions.  Please take another look.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6168

To: durin42, #hg-reviewers, jeffpc, yuja
Cc: martinvonz, jeffpc, spectral, mercurial-devel

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -1427,7 +1427,7 @@ 
 
         return i
 
-    def _readline(self):
+    def _readline(self, prompt=' '):
         # Replacing stdin/stdout temporarily is a hard problem on Python 3
         # because they have to be text streams with *no buffering*. Instead,
         # we use rawinput() only if call_readline() will be invoked by
@@ -1450,13 +1450,13 @@ 
         # - http://bugs.python.org/issue12833
         with self.timeblockedsection('stdio'):
             if usereadline:
-                line = encoding.strtolocal(pycompat.rawinput(r' '))
+                line = encoding.strtolocal(pycompat.rawinput(prompt))
                 # When stdin is in binary mode on Windows, it can cause
                 # raw_input() to emit an extra trailing carriage return
                 if pycompat.oslinesep == b'\r\n' and line.endswith(b'\r'):
                     line = line[:-1]
             else:
-                self._fout.write(b' ')
+                self._fout.write(pycompat.bytestr(prompt))
                 self._fout.flush()
                 line = self._fin.readline()
                 if not line:
@@ -1478,10 +1478,14 @@ 
             self._writemsg(self._fmsgout, default or '', "\n",
                            type='promptecho')
             return default
-        self._writemsgnobuf(self._fmsgout, msg, type='prompt', **opts)
-        self.flush()
+        if self._colormode == 'win32':
+            readline_prompt = ' '
+            self._writemsgnobuf(self._fmsgout, msg, type='prompt', **opts)
+            self.flush()
+        else:
+            readline_prompt = self.label(msg, 'ui.prompt') + ' '
         try:
-            r = self._readline()
+            r = self._readline(prompt=readline_prompt)
             if not r:
                 r = default
             if self.configbool('ui', 'promptecho'):