Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

`read --silent` is broken in Fish 3.0 #5519

Closed
psliwka opened this Issue Jan 12, 2019 · 15 comments

Comments

Projects
None yet
6 participants
@psliwka
Copy link

psliwka commented Jan 12, 2019

This is regression from 2.7.1. read --silent works correctly when used in interactive shell, but echoes back typed characters when used inside a script.

~> fish --version
fish, version 3.0.0
~> lsb_release -a
No LSB modules are available.
Distributor ID:	Debian
Description:	Debian GNU/Linux buster/sid
Release:	unstable
Codename:	sid
~> echo $TERM
xterm-256color
~> read --silent foo  # this is correct
read> ●●●●●●
~> cat test.fish 
#!/usr/bin/env fish
read --silent foo
~> ./test.fish  # this is wrong
read> secret
●●●●●●
~> fish -c "read --silent foo"  # this is wrong too
read> secret
●●●●●●

@zanchey zanchey referenced this issue Jan 12, 2019

Closed

fish 3.0.1? #5520

7 of 7 tasks complete
@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Jan 12, 2019

I can't reproduce this on Debian stretch with 2.7.1, 3.0.0 or 3.0.0-167-g2d3e8ec0. Which terminal emulator are you using? Are you connecting over SSH or using a local terminal? And does it go away if you use env HOME=(mktemp -d) fish -c 'read --silent foo'?

@psliwka

This comment has been minimized.

Copy link
Author

psliwka commented Jan 12, 2019

Hi @zanchey!

I can't reproduce this on Debian stretch with 2.7.1, 3.0.0 or 3.0.0-167-g2d3e8ec0.

For the record, I've reproduced it on:

  • Debian Sid, Fish installed through APT from official Debian repo
  • macOS Mojave, Fish installed through Homebrew

Which terminal emulator are you using?

xfce-terminal on Debian, iTerm2 on Mac. Tried few other emulators and all exhibit the same behavior.

Are you connecting over SSH or using a local terminal?

Tried both, same result.

And does it go away if you use env HOME=(mktemp -d) fish -c 'read --silent foo'?

Nope.

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Jan 12, 2019

Got it - my path was a bit funky and wasn't running the fish I was expecting with fish -c.

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Jan 13, 2019

This bisects to - oh, no - af0c8d5

@zanchey zanchey added this to the fish-future milestone Jan 13, 2019

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Jan 13, 2019

Thank you for your commiseration, @zanchey. A small part of me died when you bisected it, but your support helps. 😕

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Jan 17, 2019

This is (without digging into the code) almost certainly caused by fish being unable to set the terminal attributes to turn off echo.

A previous iteration of the job overhaul attempted to set terminal attributes when running in non-interactive mode but simply omitted displaying any errors or returning early in case the operation failed, the current version of the code does not try to set the terminal attributes if running in non-interactive mode.

I'm guessing we need to go back to the earlier of the two.

@InspectorMustache

This comment has been minimized.

Copy link

InspectorMustache commented Jan 18, 2019

For me read actually echoes the input twice.

$ fish -c 'read'
read> bla
bla
bla
$ fish -c 'read --silent'
read> bla
●●●
bla

I'm on ArchLinux using 3.0. I'm using Termite but I don't think this is related to the terminal emulator as I get the same results with alacritty and xfce-terminal.

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Jan 21, 2019

This was due to the addition of the 'is_interactive_session' check in af0c8d5, whether we only set the shell modes if we were started interactively.

is_interactive_session is a weird variable that should probably go away. Whether fish is interactive is a dynamic property but that variable is only set at initialization time.

ridiculousfish added a commit that referenced this issue Jan 21, 2019

Unconditionally set the tty mode in reader_readline
There was a bogus check for is_interactive_session. But if we are in
reader_readline we are necessarily interactive (even if we are not in
an interactive session, i.e. a fish script invoked some interactive
functionality).

Remove this check.

Fixes #5519

ridiculousfish added a commit that referenced this issue Jan 21, 2019

mqudsi added a commit that referenced this issue Jan 22, 2019

Fix regression for #4178 and others introduced by 364c839
...while still keeping intact the fix for #5519.
@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Jan 22, 2019

@ridiculousfish that check wasn't spurious, it was one component of the changes in the job overhaul that was involved in fixing #4178 and others.

As I mentioned in my earlier comment, that check needs to be persisted in some form or the other.

My fix (with documentation this time around) is in 0edaf42. It was not possible (or at least I didn't know how) to add a regression test at the time (see also #5451), but the new status fish-path in fish 3.0 made it possible (after a fashion) and a regression test was included in f65f389 / #5567 but I need some help getting it to pass the naive stdout comparison because read introduces a prompt to the output.

mqudsi added a commit that referenced this issue Jan 22, 2019

Fix regression for #4178 and others introduced by 364c839
...while still keeping intact the fix for #5519.
@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Jan 23, 2019

I'm unclear on what the status of this in 3.0.1, reopening so we don't forget to sort it out

@faho

This comment has been minimized.

Copy link
Member

faho commented Jan 23, 2019

I can confirm this works in Integration_3.0.1 as of aee8e52, both ./fish -c and ./fish $file.

Anything else to test?

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Jan 27, 2019

@mqudsi I can't speak with confidence for this issue, is it good in 3.0.1?

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Jan 27, 2019

Yup, all seems to be well. #5549 also checks out clean.

Note that regardless of what path to fish you specify when running ./fish -c foo.fish, the script will actually run under the path specified in the shebang in foo.fish (as ./fish is invoked only to directly run foo.fish as an executable).

@zanchey zanchey closed this Jan 28, 2019

@faho

This comment has been minimized.

Copy link
Member

faho commented Jan 28, 2019

Note that regardless of what path to fish you specify when running ./fish -c foo.fish

@mqudsi: Note that I ran ./fish foo.fish without -c. I ran ./fish -c 'read --silent foo' with -c.

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Jan 29, 2019

Ah, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.