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 fish_kill_signal to track the signal that terminated the last command #6824

Closed
wants to merge 1 commit into from
Closed

Conversation

soumya92
Copy link
Contributor

@soumya92 soumya92 commented Mar 28, 2020

Description

Adds a new variable $fish_kill_signal, which stores the signal that terminated the last interactive command (e.g. 2 for SIGINT). Set to 0 if the command exited normally.

Fixes #6822

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.md

@krobelus
Copy link
Member

Looks good.

I'm curious where something to the effect of exit 130 is used. I think it happens mostly when a child process (for example another shell) exits after interrupting a job.

@faho
Copy link
Member

faho commented Mar 28, 2020

Since we already have cancellation_signal, it seems much easier and more generally useful to just expose that, e.g. via a variable, fish_kill_signal or so?

@soumya92
Copy link
Contributor Author

Yeah, that sounds better. I'll update this tomorrow to use an electric var.

For my purpose I could then check that var in the postexec handler.

@soumya92 soumya92 changed the title Add fish_exec_cancel to detect Ctrl-C on a running command Add fish_kill_signal to track the signal that terminated the last command Mar 28, 2020
@soumya92
Copy link
Contributor Author

Done. I ended up using the proc_status since that includes other signals as well which could be useful down the line.

Since cancellation sets the status to sigint already, everything works as expected.

Copy link
Member

@krobelus krobelus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just some minor comments (I can do the fixups).

I guess for now we don't use this in the default prompt, we just assume that 130 is a signal.

tests/signals.expect Outdated Show resolved Hide resolved
tests/signals.expect Outdated Show resolved Hide resolved
src/env.h Outdated Show resolved Hide resolved
@krobelus
Copy link
Member

krobelus commented Apr 2, 2020

Merged as 61a9cda, thank you!

@krobelus krobelus closed this Apr 2, 2020
@soumya92
Copy link
Contributor Author

soumya92 commented Apr 2, 2020

Happy to contribute :)

@krobelus krobelus added this to the fish 3.2.0 milestone Apr 2, 2020
@soumya92 soumya92 deleted the exec-cancel branch April 3, 2020 04:50
@zanchey
Copy link
Member

zanchey commented Apr 3, 2020

Is this for the last job or the last command?

@krobelus
Copy link
Member

krobelus commented Apr 3, 2020

From the last foreground job it's the rightmost command in the pipeline that was terminated by a signal.
If I do sleep 100 | true and pkill sleep, then it is the kill signal of sleep, not true.
So "job" might be a better wording than "command" for the docs/changelog?

@soumya92
Copy link
Contributor Author

soumya92 commented Apr 3, 2020

Yep, job makes more sense and is consistent with $status. I'll send a PR to update that shortly

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Distinction between Ctrl-C and exit 130 is gone
4 participants