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

Add a shell suspend function #2269

Closed
wants to merge 3 commits into from
Closed

Add a shell suspend function #2269

wants to merge 3 commits into from

Conversation

@mwm
Copy link
Contributor

@mwm mwm commented Aug 4, 2015

The "suspend" function to suspend the current shell - which is common in most shells - isn't in fish. This adds it as a function. Not clear if it should be doing any tty cleanup before stopping, but things seem to work ok.

@faho
Copy link
Member

@faho faho commented Aug 4, 2015

Just one question, since I've never used the job control stuff:

If I run fish directly in my konsole or xterm or whatever and then suspend it, how do I get it to run again, besides opening another fish and sending SIGCONT to the stopped instance?

@mwm
Copy link
Contributor Author

@mwm mwm commented Aug 4, 2015

Well, if you're running it directly like that, it should be a login shell, in which case the function will print an error instead of suspending. This is standard behavior for most shells.

@faho
Copy link
Member

@faho faho commented Aug 4, 2015

Is there any way in which we can have this behave better in the non-login-xterm case, which I assume is quite common (I believe it's even default for konsole)?

@mwm
Copy link
Contributor Author

@mwm mwm commented Aug 4, 2015

Is there some way to detect that case from inside of fish? If not, then probably not.

A quick look at the bash sources show it does what this function does, and only refused to suspend if it believes it was a login shell. it even has a "--force" option to override that test.

@faho
Copy link
Member

@faho faho commented Aug 4, 2015

Hmm.. I'd have said SHLVL (when interactive - when it's non-interactive it doesn't matter), but after checking it's actually 2 for the konsole-case.

@mwm
Copy link
Contributor Author

@mwm mwm commented Aug 4, 2015

I think it's normal for SHLVL to be 2 in X terminal emulators, as the X startup script is usually a shell script of some sort. xterm, rxterm and uxterm all do that.

How about adding a suspend_minimum_SHLVL variable, defaulting to 3 (or 2, depending) and we also issue a warning if SHLVL isn't < than (or ≤ to) that variable? In that case, I think a flag to ignore the shell level would be appropriate as well.

@faho
Copy link
Member

@faho faho commented Aug 5, 2015

I believe the logic should be

if begin not status --is-interactive
         or not count $suspend_minimum_SHLVL
         or test $SHLVL -ge $suspend_minimum_SHLVL
         or contains -- $argv --force
   end
   # Do suspend
end

i.e. when the shell's non-interactive, when SHLVL is high enough or when "--force" is given suspend. Also when the user unset the special "suspend_minimum_SHLVL" variable (and it unfortunately needs to be a variable if I understand this correctly).

The idea is to do everything possible to stop the user from getting a non-functioning terminal without an easy way back (i.e. opening another terminal and doing pkill -SIGCONT fish), but that doesn't apply when the shell's not connected to an interactive terminal in the first place, or when the user explicitly overrides it.

@mwm
Copy link
Contributor Author

@mwm mwm commented Aug 5, 2015

I think you might be overreacting a bit. After all, we don't have a function for kill that checks all that to stop the user from doing "kill -STOP %self". After all, this is essentially an alias for that.

But I think the test for not being a login shell should be done as well. Possibly the SHLVL test makes it redundant, but given all the odd systems out there, who knows?

@faho
Copy link
Member

@faho faho commented Aug 5, 2015

After all, we don't have a function for kill that checks all that to stop the user from doing "kill -STOP %self".

Yeah, but if you send sigstop to the shell manually you very much know what you are doing. You might execute a function called suspend without that - though reading the description would be nice, we currently don't expose it nicely (i.e. without reading the entire sourcecode through type) IIRC.

But I think the test for not being a login shell should be done as well. Possibly the SHLVL test makes it redundant, but given all the odd systems out there, who knows?

I don't care too much - whether something's a login shell should correlate with whether it should suspend, I'm just not sure how strong the correlation is.

@mwm
Copy link
Contributor Author

@mwm mwm commented Aug 6, 2015

Ok, I think it now does what we both want. If it's interactive or --force is used, it suspends. Otherwise, it won't suspend if it's a login shell or SHLVL is below suspend_minimum_SHLVL, which defaults to 3.

@faho
Copy link
Member

@faho faho commented Aug 30, 2015

Squashed merge with rebase as 17c7569. Thanks!

@faho faho closed this Aug 30, 2015
@zanchey zanchey added this to the next-2.x milestone Aug 31, 2015
@zanchey zanchey added the enhancement label Aug 31, 2015
@zanchey
Copy link
Member

@zanchey zanchey commented Aug 31, 2015

Should we add a help page for this command?

@zanchey zanchey added the docs label Aug 31, 2015
@mwm
Copy link
Contributor Author

@mwm mwm commented Aug 31, 2015

I wrote a help page for it. See PR #2347

@faho faho removed the docs label Sep 11, 2015
@faho faho added the release notes label Oct 26, 2015
@floam
Copy link
Member

@floam floam commented Apr 10, 2016

Why is the minimum $SHLVL defaulting to 3?

I was vaguely aware that OS X is strange in that all interactive sessions are login sessions, and I assumed that was messing this up when I compared to bash, tcsh, zsh, and mksh. On them, I could start a subshell, hit suspend, it works, on fish I couldn't.

What is the danger we want to avoid at SHLVL=2? I have a PR ready to change this (and actually describe the cause of the problem in the error) but I'd like to figure out what I'm probably messing up.

@floam
Copy link
Member

@floam floam commented Apr 10, 2016

Also, kill itself is left as a zombie process.

screenshot 2016-04-10 at 6 37 14 am

screenshot 2016-04-10 at 6 37 00 am

@floam
Copy link
Member

@floam floam commented Apr 10, 2016

OK, I read this fully and learned that for @faho his SHLVL is 2 when suspending is not supposed to happen. @faho can you find another terminal that prevents you from having to resume your shell here? I want to see how they do this.

Is it the case that SHLVL is simply expected to be +1 in any non-login shell? In that case we can set the minimum to 2 but require minimum+1 for non-login shells.

@mwm
Copy link
Contributor Author

@mwm mwm commented Apr 10, 2016

The minimum shell level depends on how your shells get launched. It defaults to three because if you start your X session via xinit, then the shell that runs .xinitrc counts as 1 level of shell, so your shells start at 2. I know mine do.

Changing the default to 2 now will leave people in that situation in the state where a suspend could leave them with a dead window. If all you are worried about is login shells - we already check for that. So I'd say we'd be better off ripping out the SHLVL test completely than changing the default to 2.

@floam
Copy link
Member

@floam floam commented Apr 10, 2016

kill -SIGSTOP %self (or suspend)
followed by (in parent) bg %1

It crashes fish - (on my system after spewing a random file path, and/or garbage to the command line)
bg %1 ends up triggering a (clean) exit in bash, and zsh just does something that seems successful.

Does anyone expect anything at all from that? Does the crash matter?

@floam
Copy link
Member

@floam floam commented Jun 16, 2016

So I'd say we'd be better off ripping out the SHLVL test completely than changing the default to 2.

Except, regular terminals are login shells on OS X. So that's painful to always refuse. I'm going to decrement the minimum SHLVL as suspend is kind of annoying on OS X needing to --force when I don't in other shells. A dead terminal in an X session after you use suspend on accident is not the most terrible thing in the world.

floam added a commit that referenced this pull request Jun 16, 2016
There is some discussion on #2269
@mwm
Copy link
Contributor Author

@mwm mwm commented Jun 16, 2016

So those of us who set SHLVL properly depending on system type will have to fix it everywhere? Thanx.

@floam
Copy link
Member

@floam floam commented Jun 16, 2016

I believe my change should make this work correctly for proper SHLVL.

@floam
Copy link
Member

@floam floam commented Jun 16, 2016

Actually, duh, while an initial fish is indeed a login shell the subshells will not be. So I'd agree that removing the SHLVL check makes most sense and matches what other shells do.

@faho
Copy link
Member

@faho faho commented Jun 16, 2016

@floam: The original issue here was that in linux plenty of startup paths use a shell somewhere and nothing ever seems to reset SHLVL, so you'll end up with SHLVL = 2 in a shell directly in an e.g. xterm.

Of course the problem is that we can't actually detect what we really want - "if I fall, will somebody catch me". What we really want to know is if there's someone else who can take over the tty for us if we stop. That's the only thing I can see SHLVL being useful for, but as it stands it's basically useless.

So I'm okay with nixing the check.

@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Jun 19, 2016

I reopened to nix the check, but then noticed that this is quite old, so I'm going to file a new issue.

@ridiculousfish ridiculousfish mentioned this pull request Jun 19, 2016
2 of 2 tasks complete
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.