re-enable bracketed paste on not(windows) (fixes #9944) (fixes #648) #657

LevitatingBusinessMan · 2023-11-05T02:22:01Z

Due to a supposed issue (nushell/nushell#9218) in nushell regarding commands that read standard input, a fix (nushell/nushell#9399) was proposed that temporarily disables bracketed_paste during the execution of eval_source.

Instead, this issue was moved over to reedline and a new PR was created (#598). In this version of the "fix", bracketed_paste is disabled completely after the first execution of reedline. This has led to issue #648, where multiline paste is now being disabled on non-windows environments.

This PR effectively reverts #598. This might cause nushell/nushell#9218 to reappear in nushell.

I have personally not been able to reproduce nushell/nushell#9218, modern versions of gpg use a GUI prompt for the password, or alternatively a curses window. I did try to replicate the issue using GNU readline and other prompting methods but I wasn't succesful.

If nushell/nushell#9218 is a reproducible issue, I think the best course of action is to merge nushell/nushell#9399, which supposedly fixes the issue, but on the nushell side. It seems like this issue might be specific to nushell, and thus it should be fixed in that repository. No need to affect all other reedline dependent packages with it.

codecov · 2023-11-05T02:33:34Z

Codecov Report

Merging #657 (5e0892b) into main (60b3b62) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #657      +/-   ##
==========================================
+ Coverage   49.21%   49.22%   +0.01%     
==========================================
  Files          43       43              
  Lines        7914     7912       -2     
==========================================
  Hits         3895     3895              
+ Misses       4019     4017       -2

Files	Coverage Δ
src/engine.rs	`5.15% <ø> (+<0.01%)`	⬆️

fdncred · 2023-11-05T02:41:25Z

src/engine.rs

@@ -630,9 +630,6 @@ impl Reedline {

        let result = self.read_line_helper(prompt);

-        #[cfg(not(target_os = "windows"))]


I kind of wonder if this was originally just a typo and should've really been #[cfg(target_os = "windows"))] because bracketed paste doesn't work in windows since crossterm doesn't support it yet.

The author of the original issue was using Linux. Though he was using the Windows Terminal (using debian via WSL2).

Thus it might be a Windows Terminal issue? I don't think anyone tried it to reproduce it with Windows Terminal specifically. And it seems nobody was actually able to reproduce it except @theAkito himself.

I can try to reproduce with WezTerm on Windows. Just tell me which commits to try & any additional useful information...

LevitatingBusinessMan · 2023-11-05T04:46:52Z

@theAkito

I can try to reproduce with WezTerm on Windows. Just tell me which commits to try & any additional useful information...

I think the easiest way to test it would be to compile nushell with:

[patch.crates-io]
reedline = { git = "https://github.com/LevitatingBusinessMan/reedline.git", branch = "re-enable-bracketed-paste" }

And then try to reproduce the error (first in Windows Terminal like before, if that works in WezTerm). Have you seen this error only with gpg or also other commands that take input?

theAkito · 2023-11-05T04:48:06Z

And then try to reproduce the error.

What's the point of trying to reproduce it in Windows Terminal? If it's gonna happen again, it still could be due to this terminal.

LevitatingBusinessMan · 2023-11-05T04:57:45Z

If it's gonna happen again, it still could be due to this terminal.

Exactly. You can first verify the bug is still present on Windows Terminal. And then you can remove variables to find the root cause. Like if it is related to the terminal by switching to another.

You can also test for the bug in WezTerm first, but if that fails you'd have to test it in Windows Terminal anyway.

I am curious if the bug is specific to gpg somehow. If it isn't than I think more people would experience it. If the bug does appear in gpg try using passwd.

If it is specific to gpg than it must be related to pinentry.

fdncred · 2023-11-05T12:18:48Z

also related nushell/nushell#9944

LevitatingBusinessMan · 2023-11-05T13:46:26Z

I tested it and believe nushell/nushell#9944 is actually fixed by this PR.

@amtoine is on Linux so it makes sense.

theAkito · 2023-11-05T14:09:10Z

Exactly. You can first verify the bug is still present on Windows Terminal. And then you can remove variables to find the root cause. Like if it is related to the terminal by switching to another.

You can also test for the bug in WezTerm first, but if that fails you'd have to test it in Windows Terminal anyway.

Makes sense. Will try, when I get the time.

Am following this pull request.

WindSoilder · 2023-11-05T23:05:13Z

Thanks, generally I'm ok with your point.

I believe it will cause nushell/nushell#9218 to reappear. But we can bring nushell/nushell#9399 back, so both reedline and nushell are useful to us again.

theAkito · 2023-11-06T00:06:49Z

You can also test for the bug in WezTerm first, but if that fails you'd have to test it in Windows Terminal anyway.

Done, did it.

Here is what I did, so there are no misunderstandings.

Compiled Nushell master branch on WSL: cargo build --release --workspace; cargo run --release
Testing GPG signing, which requires the passphrase. gpg --yes -s s.txt
Same issue, as in Pasting into Terminal entirely broken since 0.80.0 (Example: GPG Passphrase Paste) nushell#9218.
Connected to WSL via WezTerm.
Same issue, once again.

I am curious if the bug is specific to gpg somehow. If it isn't than I think more people would experience it. If the bug does appear in gpg try using passwd.

If it is specific to gpg than it must be related to pinentry.

There are probably tons of other programs requiring password entry in a similar fashion, but right now none come to my mind. If you can tell me an example program, I can test, I will test the password entry there.

LevitatingBusinessMan · 2023-11-06T00:40:12Z

There are probably tons of other programs requiring password entry in a similar fashion, but right now none come to my mind. If you can tell me an example program, I can test, I will test the password entry there.

Passwd maybe? Modern GPG uses its own pinentry tool, which will prompt using curses (those terminal user interfaces).

Does the prompt you get look like this?

If it happens in wezterm, it could be specific to WSL2? More likely would be that it is specific to GPG I'd think.

theAkito · 2023-11-06T01:46:36Z

Tried passwd & works perfectly fine without issues.

These are the literal characters presented to me on pinentry.

                           ┌────────────────────────────────────────────────────────────────┐
                           │ Please enter the passphrase to unlock the OpenPGP secret key:  │
                           │ "Name <[email protected]>"                                         │
                           │ 4096-bit RSA key, ID HEXHEXHEXHEXHEHX,                         │
                           │ created 2001-11-11.                                            │
                           │                                                                │
                           │                                                                │
                           │ Passphrase: __________________________________________________ │
                           │                                                                │
                           │         <OK>                                    <Cancel>       │
                           └────────────────────────────────────────────────────────────────┘

GPG version

gpg (GnuPG) 2.2.27
libgcrypt 1.8.8
Copyright (C) 2021 Free Software Foundation, Inc.
License GNU GPL-3.0-or-later <https://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

fdncred · 2023-11-06T12:51:58Z

Are we saying this PR is ready to land or is it going to create other problems?

LevitatingBusinessMan · 2023-11-06T20:10:50Z

Thanks to @theAkito I can now confirm that this issue is specific to curses.

I was able to replicate it with a password prompt written in python (with curses).

import curses

def main():
    # Initialize curses
    stdscr = curses.initscr()

    # Prompt for password
    stdscr.addstr("Paste text using bracketed_paste: ")
    password = ""
    while True:
        key = stdscr.getch()

        password += chr(key)

        stdscr.clear()  # Clear screen
        stdscr.addstr("Paste text using bracketed_paste: " + password)

if __name__ == "__main__":
    main()

Escape characters 200 and 201 are prefixed and suffixed respectively. Curses captures the key and converts it to a character, and apparently nushell is sending some escape characters that get converted like this.

LevitatingBusinessMan · 2023-11-06T20:12:37Z

I personally think this PR should be merged, so nushell/nushell#9944 and #648 can be closed.

After this is merged, I will open a new issue at nushell, covering this issue with bracketed paste and curses.

fdncred · 2023-11-06T20:36:08Z

ok @LevitatingBusinessMan let's try your solution. I'll be looking for that nushell PR. Appreciate it.

theAkito · 2023-11-06T20:43:24Z

I was able to replicate it with a password prompt written in python (with curses).

Weird. Cannot reproduce it in Nim.

##[
  Works on Nim 2.0.0

  nimble install ncurses # https://github.com/walkre-niboshi/nim-ncurses
  nim c -r reedline_657.nim ; rm reedline_657
]##

import pkg/ncurses

when isMainModule:
  let stdscr = initscr()
  stdscr.waddstr("Paste text using bracketed_paste: ")
  var password = ""
  while true:
    var key = stdscr.wgetch()

    if key == 0: break

    password &= key.chr

    stdscr.werase
    stdscr.waddstr(cstring("Paste text using bracketed_paste: " & password.repr))

Output just shows the characters I entered, which is, I suppose, the correct behaviour.

This program's ncurses library uses libncursesw.so(.5|.6|) in the background, so it's the behaviour I get from my system's ncurses library.

Same with this Python program. No incorrect characters echoed.

LevitatingBusinessMan · 2023-11-06T20:48:26Z

@theAkito Did you test it using reedline compiled from git? Including this merge?

theAkito · 2023-11-06T20:49:25Z

@theAkito Did you test it using reedline compiled from git? Including this merge?

I tested with the product of yesterday's compilation, where I did reproduce it with GPG, earlier.

LevitatingBusinessMan · 2023-11-06T21:14:46Z

I can reproduce it just fine, by having reedline pointed to the git's main branch. And doing cargo update to update the dependencies.

theAkito · 2023-11-06T22:11:40Z

@LevitatingBusinessMan

Well, turns out, I had a user error, when trying to reproduce. Can reproduce now.

Confirmed. 👍🏻

re-enable bracketed past on not(windows)

5e0892b

LevitatingBusinessMan changed the title ~~re-enable bracketed past on not(windows)~~ re-enable bracketed paste on not(windows) Nov 5, 2023

fdncred reviewed Nov 5, 2023

View reviewed changes

LevitatingBusinessMan changed the title ~~re-enable bracketed paste on not(windows)~~ re-enable bracketed paste on not(windows) (fixes #9944) (fixes #648) Nov 6, 2023

fdncred merged commit c853d71 into nushell:main Nov 6, 2023
8 checks passed

LevitatingBusinessMan deleted the re-enable-bracketed-paste branch November 6, 2023 20:37

LevitatingBusinessMan mentioned this pull request Nov 6, 2023

Pasting with bracketed_paste creates escape codes that affect some programs nushell/nushell#10982

Closed

This was referenced Nov 6, 2023

bracketed paste partially broken nushell/nushell#9944

Closed

bracketed paste only works the first time #648

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

re-enable bracketed paste on not(windows) (fixes #9944) (fixes #648) #657

re-enable bracketed paste on not(windows) (fixes #9944) (fixes #648) #657

LevitatingBusinessMan commented Nov 5, 2023 •

edited

Loading

codecov bot commented Nov 5, 2023

fdncred Nov 5, 2023

LevitatingBusinessMan Nov 5, 2023 •

edited

Loading

theAkito Nov 5, 2023

LevitatingBusinessMan commented Nov 5, 2023 •

edited

Loading

theAkito commented Nov 5, 2023

LevitatingBusinessMan commented Nov 5, 2023

fdncred commented Nov 5, 2023

LevitatingBusinessMan commented Nov 5, 2023 •

edited

Loading

theAkito commented Nov 5, 2023

WindSoilder commented Nov 5, 2023

theAkito commented Nov 6, 2023

LevitatingBusinessMan commented Nov 6, 2023

theAkito commented Nov 6, 2023

fdncred commented Nov 6, 2023

LevitatingBusinessMan commented Nov 6, 2023

LevitatingBusinessMan commented Nov 6, 2023 •

edited

Loading

fdncred commented Nov 6, 2023

theAkito commented Nov 6, 2023 •

edited

Loading

LevitatingBusinessMan commented Nov 6, 2023 •

edited

Loading

theAkito commented Nov 6, 2023

LevitatingBusinessMan commented Nov 6, 2023

theAkito commented Nov 6, 2023

		@@ -630,9 +630,6 @@ impl Reedline {

		let result = self.read_line_helper(prompt);

		#[cfg(not(target_os = "windows"))]

re-enable bracketed paste on not(windows) (fixes #9944) (fixes #648) #657

re-enable bracketed paste on not(windows) (fixes #9944) (fixes #648) #657

Conversation

LevitatingBusinessMan commented Nov 5, 2023 • edited Loading

codecov bot commented Nov 5, 2023

Codecov Report

fdncred Nov 5, 2023

Choose a reason for hiding this comment

LevitatingBusinessMan Nov 5, 2023 • edited Loading

Choose a reason for hiding this comment

theAkito Nov 5, 2023

Choose a reason for hiding this comment

LevitatingBusinessMan commented Nov 5, 2023 • edited Loading

theAkito commented Nov 5, 2023

LevitatingBusinessMan commented Nov 5, 2023

fdncred commented Nov 5, 2023

LevitatingBusinessMan commented Nov 5, 2023 • edited Loading

theAkito commented Nov 5, 2023

WindSoilder commented Nov 5, 2023

theAkito commented Nov 6, 2023

LevitatingBusinessMan commented Nov 6, 2023

theAkito commented Nov 6, 2023

fdncred commented Nov 6, 2023

LevitatingBusinessMan commented Nov 6, 2023

LevitatingBusinessMan commented Nov 6, 2023 • edited Loading

fdncred commented Nov 6, 2023

theAkito commented Nov 6, 2023 • edited Loading

LevitatingBusinessMan commented Nov 6, 2023 • edited Loading

theAkito commented Nov 6, 2023

LevitatingBusinessMan commented Nov 6, 2023

theAkito commented Nov 6, 2023

LevitatingBusinessMan commented Nov 5, 2023 •

edited

Loading

LevitatingBusinessMan Nov 5, 2023 •

edited

Loading

LevitatingBusinessMan commented Nov 5, 2023 •

edited

Loading

LevitatingBusinessMan commented Nov 5, 2023 •

edited

Loading

LevitatingBusinessMan commented Nov 6, 2023 •

edited

Loading

theAkito commented Nov 6, 2023 •

edited

Loading

LevitatingBusinessMan commented Nov 6, 2023 •

edited

Loading