This is an archive of the discontinued Mercurial Phabricator instance.

readline: provide styled prompt to readline (issue6070)
ClosedPublic

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

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

durin42 created this revision.Mar 22 2019, 11:06 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.

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.

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.

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?

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

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

This revision was automatically updated to reflect the committed changes.