This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6168

readline: provide styled prompt to readline (issue6070)
ClosedPublic

Authored by durin42 on Mar 22 2019, 11:06 AM.

Details

Reviewers
jeffpc
yuja
Group Reviewers
hg-reviewers
Commits
rHG2d428b859282: readline: provide styled prompt to readline (issue6070)

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

durin42 created this revision.Mar 22 2019, 11:06 AM
durin42 added a comment.Mar 22 2019, 11:07 AM

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.

durin42 added a subscriber: spectral.Mar 22 2019, 11:07 AM

FYI @spectral I uploaded this.

jeffpc accepted this revision.Mar 22 2019, 2:19 PM
jeffpc added a subscriber: jeffpc.

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

martinvonz added a subscriber: martinvonz.Mar 22 2019, 2:27 PM
In 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.

martinvonz added a comment.Mar 22 2019, 2:30 PM
In D6168#89874, @martinvonz wrote:
In 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?

spectral added a comment.Mar 22 2019, 3:40 PM
In 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.

yuja added a comment.Mar 22 2019, 10:01 PM
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.

spectral updated this revision to Diff 14746.Apr 15 2019, 5:31 PM
spectral updated this revision to Diff 14747.
spectral updated this revision to Diff 14748.Apr 15 2019, 5:34 PM
spectral added a comment.Apr 15 2019, 5:37 PM

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

Closed by commit rHG2d428b859282: readline: provide styled prompt to readline (issue6070) (authored by spectral). · Explain WhyApr 15 2019, 7:41 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/ui.py (20 lines)

Diff 14747

mercurial/ui.py

i = self.configbool("ui", "formatted") i = self.configbool("ui", "formatted")
if i is None: if i is None:
# some environments replace stdout without implementing isatty # some environments replace stdout without implementing isatty
# usually those are non-interactive # usually those are non-interactive
return self._isatty(self._fout) return self._isatty(self._fout)
return i return i
def _readline(self): def _readline(self, prompt=' ', promptopts=None):
# Replacing stdin/stdout temporarily is a hard problem on Python 3 # Replacing stdin/stdout temporarily is a hard problem on Python 3
# because they have to be text streams with *no buffering*. Instead, # because they have to be text streams with *no buffering*. Instead,
# we use rawinput() only if call_readline() will be invoked by # we use rawinput() only if call_readline() will be invoked by
# PyOS_Readline(), so no I/O will be made at Python layer. # PyOS_Readline(), so no I/O will be made at Python layer.
usereadline = (self._isatty(self._fin) and self._isatty(self._fout) usereadline = (self._isatty(self._fin) and self._isatty(self._fout)
and procutil.isstdin(self._fin) and procutil.isstdin(self._fin)
and procutil.isstdout(self._fout)) and procutil.isstdout(self._fout))
if usereadline: if usereadline:
try: try:
# magically add command line editing support, where # magically add command line editing support, where
# available # available
import readline import readline
# force demandimport to really load the module # force demandimport to really load the module
readline.read_history_file readline.read_history_file
# windows sometimes raises something other than ImportError # windows sometimes raises something other than ImportError
except Exception: except Exception:
usereadline = False usereadline = False
if self._colormode == 'win32' or not usereadline:
if not promptopts:
promptopts = {}
self._writemsgnobuf(self._fmsgout, prompt, type='prompt',
**promptopts)
self.flush()
prompt = ' '
else:
prompt = self.label(prompt, 'ui.prompt') + ' '
# prompt ' ' must exist; otherwise readline may delete entire line # prompt ' ' must exist; otherwise readline may delete entire line
# - http://bugs.python.org/issue12833 # - http://bugs.python.org/issue12833
with self.timeblockedsection('stdio'): with self.timeblockedsection('stdio'):
if usereadline: 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 # When stdin is in binary mode on Windows, it can cause
# raw_input() to emit an extra trailing carriage return # raw_input() to emit an extra trailing carriage return
if pycompat.oslinesep == b'\r\n' and line.endswith(b'\r'): if pycompat.oslinesep == b'\r\n' and line.endswith(b'\r'):
line = line[:-1] line = line[:-1]
else: else:
self._fout.write(b' ') self._fout.write(pycompat.bytestr(prompt))
self._fout.flush() self._fout.flush()
line = self._fin.readline() line = self._fin.readline()
if not line: if not line:
raise EOFError raise EOFError
line = line.rstrip(pycompat.oslinesep) line = line.rstrip(pycompat.oslinesep)
return line return line
def prompt(self, msg, default="y"): def prompt(self, msg, default="y"):
"""Prompt user with msg, read response. """Prompt user with msg, read response.
If ui is not interactive, the default is returned. If ui is not interactive, the default is returned.
""" """
return self._prompt(msg, default=default) return self._prompt(msg, default=default)
def _prompt(self, msg, **opts): def _prompt(self, msg, **opts):
default = opts[r'default'] default = opts[r'default']
if not self.interactive(): if not self.interactive():
self._writemsg(self._fmsgout, msg, ' ', type='prompt', **opts) self._writemsg(self._fmsgout, msg, ' ', type='prompt', **opts)
self._writemsg(self._fmsgout, default or '', "\n", self._writemsg(self._fmsgout, default or '', "\n",
type='promptecho') type='promptecho')
return default return default
self._writemsgnobuf(self._fmsgout, msg, type='prompt', **opts)
self.flush()
try: try:
r = self._readline() r = self._readline(prompt=msg, opts=opts)
if not r: if not r:
r = default r = default
if self.configbool('ui', 'promptecho'): if self.configbool('ui', 'promptecho'):
self._writemsg(self._fmsgout, r, "\n", type='promptecho') self._writemsg(self._fmsgout, r, "\n", type='promptecho')
return r return r
except EOFError: except EOFError:
raise error.ResponseExpected() raise error.ResponseExpected()